NACK: [SRU][L, K, J, F][M, U][PATCH 0/1] UBUNTU: [Packaging] Expose base (parent) abi for derivatives

Tue Jun 6 10:38:07 UTC 2023

On Tue, Jun 06, 2023 at 09:43:36AM +0200, Roxana Nicolescu wrote:
> BugLink: https://bugs.launchpad.net/bugs/2017006
> 
> SRU Justification
> 
> [ Impact ]
> 
> Upstream stable added a change in the format of jbd2 in 5.15.87 that
> was cherry-picked to kinetic:linux.
> This is incompatible with the current changes in the lttng-module
> kinetic.
> 
> We previously encountered this issue for focal #lp2004644
> and we proactively released a new version for kinetic, but the fix did
> not work as expected.
> 
> As shown in the buglink, the build error is triggered because some
> function declaration does not match the kernel's.
> In `include/instrumentation/events/jbd2.h` in lttng-modules, there is
> a conditional declaration based on the kernel version.
> But for 5.19 kernels, the upstream kernel version has remained the
> same.
> 
> [ Fix ]
> Versioning based on linux upstream version does not work in this case.
> We expose the abi version of the kernel, but it alone will not solve
> this issue because we deliver many derivatives, each with its own
> version. Hence a base main abi version needs to be exposed in the
> headers.

I don't like this approach. lttng-modules and every other out-of-tree module
out there are doing the wrong thing. I worked with openvswitch in the past and
they kept out-of-tree modules even though the module was already in the tree,
and they kept it updated with the changing kernel ABI. What they did is that
they had their build look into the kernel headers for the function signatures
that were relevant and created a config file stating which signature each
relevant function had and then checking for that in their source code.

A couple of years ago, we had the same issue with wireguard and we attempted a
similar solution. There was a bug where it was not looking into the right
headers, but aside from that, it worked just fine. I believe the same solution
should be looked after here, and be considered for upstream too.

Cascardo.

> 
> The actual fix for lttng-modules requires 2 steps:
> 1. Expose UTS_UBUNTU_BASE_RELEASE_ABI variable equal with the parent
> abi in <generated/utsrelease.h>. For backports with multiple
> inheritance (like jammy:linux-aws-5.19) the version from debian.master
> (kinetic:linux) will be used. In case there is no inheritance, the
> actual kernel abi will be used. This is needed for jammy, kinetic and
> lunar but it may be needed in the future for similar use cases,
> so it makes sense to push this to all releases.
> This patch will cover the first part.
> 2. Use this variable in the lttng-module to construct
> LTTNG_UBUNTU_BASE_VERSION_CODE and then use it for versioning:
> LTTNG_UBUNTU_BASE_VERSION_CODE >= LTTNG_UBUNTU_KERNEL_VERSION(5,19,0,42)
> The lttng-module fix will be done only for lunar, kinetic, and jammy.
> 
> [ Where problems would occur ]
> This patch alone should not have a big impact on our kernels.
> It will only expose a variable in the headers.
> In terms of the lttng-modules solution:
> We may notice new failures in ubuntu_lttng_smoke_test once this
> compiles and run. But it was tested locally and the results were good,
> so the probability is very very low.
> 
> [ Test Plan ]
> Since we hit backward issues in the past, multiple kernel versions
> were tested out to make sure this is the proper fix.
> General test instructions:
> 1. Install kinetic (desired version)
> 2. sudo apt install lttng-modules-dkms(or dpkg -i)
> 3. Depending on the version it may work or not
> 4. Additionally lttng-modules regression test should be triggered to
> confirm everything works fine.
> 
> My testing:
> I build fixed lttng-modules with the fixes described in launchpad.
> A. Kinetic kernel where I expose UTS_UBUNTU_BASE_RELEASE_ABI.
> The module built and rts passed:
> 09:32:05 INFO | PASSED (simple system call tracing with babeltrace)
> 09:32:05 INFO | Summary: 7 passed, 0 failed
> B. The break commit was introduced in 5.19.0.42. Versions between 42
> and 44 (depending on where this fix will land) will break.
> C. Tested with a kernel version that does not have the break commit:
> 5.19.0-41 to see if it's backwards compatible. Results were good.
> D. Tested with jammy-aws-5.19 that has this patch. It works fine.
> E. Tested with jammy-aws-5.19.0-1024 that does not have the break
> commit to see backwards compatibility. It succeeded.
> 
> Roxana Nicolescu (1):
>   UBUNTU: [Packaging] Expose base (parent) abi for derivatives
> 
>  debian/rules.d/0-common-vars.mk | 9 +++++++++
>  debian/rules.d/2-binary-arch.mk | 4 ++++
>  2 files changed, 13 insertions(+)
> 
> -- 
> 2.34.1
> 
> 
> -- 
> kernel-team mailing list
> kernel-team at lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team