Fix the Linux kernel version number calculation

[chuck]

Summary

The Linux compatibility code is converting the version number (e.g.
2.6.32) in two different ways and then comparing the results.

The linux_map_osrel() function converted MAJOR.MINOR.PATCH similar to
what FreeBSD does natively. I.e. where major=v0, minor=v1, and patch=v2
v = v0 * 1000000 + v1 * 1000 + v2;

The LINUX_KERNVER() macro, on the other hand, converted the value with
bit shifts. I.e. where major=a, minor=b, and patch=c
v = (((a) << 16) + ((b) << 8) + (c))

Fix is to use multiplication in the macro, and then use the macro in the
function.
The Linux kernel uses the later format via the KERNEL_VERSION() macro in
include/generated/uapi/linux/version.h

Fix is to use the LINUX_KERNVER() macro in linux_map_osrel().

PR: 229209

[cem]

Why not leave the macro alone and compute v using it instead?

v = LINUX_KERNVER(v0, v1, v2);

(As I noted in the bug, the macro matches the way Linux defines kernel version numbers for a given major, minor, and patch, so it makes the most sense to leave it that way.)

[chuck]

In D15952#337588, @cem wrote:

Why not leave the macro alone and compute v using it instead?

v = LINUX_KERNVER(v0, v1, v2);

(As I noted in the bug, the macro matches the way Linux defines kernel version numbers for a given major, minor, and patch, so it makes the most sense to leave it that way.)

I'm good either way, but the thinking from @emaste in D15858 was to model this after the __FreeBSD_version style

[cem]

I don't think there's any real reason to diverge from how Linux computes its own version numbers, just to be "more native" FreeBSD. I don't know if these numbers are exposed in any userspace interface, but if they are obviously they need to match Linux, not our own made-up scheme.

I don't fully understand why @emaste suggested the decimal multiply approach instead of following the Linux convention. I know he's a smart and reasonable guy, so maybe I'm just missing something. Ed, can you explain it in a little more detail?

[emaste]

In D15952#337607, @cem wrote:

Ed, can you explain it in a little more detail?

I haven't seen evidence of a Linux convention -- if you have a reference can you share it?

Previously the Linuxulator calculated the Linux version via linux_map_osrel() using decimal shift (multiply) and then compared it to a value from this binary shift macro. So we need either the diff here, or an alternate diff that replaces x * 1000000 + y * 1000 + z in linux_map_osrel() with x << 16 | y << 8 | z.

If there's a Linux convention we should indeed follow it, but if there is no convention we should follow the normal FreeBSD approach.

[emaste]

Ah, I see the comment in the bug now - yes, if Linux uses shifts we should too. It would be good to put a reference in the commit that mentions the Linux macro/etc. that calculates it though.

[cem]

So we need either the diff here, or an alternate diff that replaces x * 1000000 + y * 1000 + z in linux_map_osrel() with x << 16 | y << 8 | z.

Right, I suggested using LINUX_KERNVER() to do this in my initial comment :-).

I don't know where it lives in the Linux source tree, but it ends up installed as /usr/include/linux/version.h with a numerically identical definition to the existing one in compat/linux.

[emaste]

Ah, top level Linux Makefile emits a definition of KERNEL_VERSION using shifts.

[emaste]

In D15952#337697, @cem wrote:

Right, I suggested using LINUX_KERNVER() to do this in my initial comment :-).

I didn't read that either, at first :-)

Your change is the correct one. Note that the linux_trans_osrel implementations in the linux*_machdep.c files probably need to be updated too.

[chuck]

[v2] Fix the Linux kernel version number calculation

Change code to match Linux bit-shift style version encoding.

[chuck]

The updated summary is what will go into the commit message.

[emaste]

LGTM.

We will also need to address the four copies of linux_trans_osrel (but after further investigation if desired, I have not checked exactly how the value they calculate is used).

[chuck]

[v3] Fix the Linux kernel version number calculation

Fix linux_trans_osrel() calls in i386 and amd64 as well.

[chuck]

In D15952#337725, @emaste wrote:

LGTM.

We will also need to address the four copies of linux_trans_osrel (but after further investigation if desired, I have not checked exactly how the value they calculate is used).

I'm only finding two copies: sys/i386/linux/linux_sysvec.c and sys/amd64/linux/linux_sysvec.c but have updated the patch to fix those copies.

[emaste]

In D15952#337749, @chuck wrote:

I'm only finding two copies: sys/i386/linux/linux_sysvec.c and sys/amd64/linux/linux_sysvec.c but have updated the patch to fix those copies.

One more in sys/amd64/linux32/linux32_sysvec.c. Oh, and the last one is not yet committed, it is in D15834.

[cem]

LGTM modulo one nit below

sys/compat/linux/linux_mib.c

153 ↗

(On Diff #44256)

Maybe spell the magic number as LINUX_KERNVER(1, 0, 0).

[emaste]

sys/compat/linux/linux_mib.c

153 ↗

(On Diff #44256)

I think that would be more clear, although I wonder why we need to test for >= 1.0.0 at all - the first Linux version I ever used was 0.9<mumble>. Maybe just EINVAL for v <= 0?

[cem]

sys/compat/linux/linux_mib.c

153 ↗

(On Diff #44256)

It appears to be a sanity check on the "linux.osrelease" knob administrators configure for Linux jails to consume.

I don't think we (FreeBSD Linuxulator) claim or care to support Linux pre-1.0 (nor pre-2.0, and likely not pre-2.6) so I would maybe advocate for moving in the opposite direction — LINUX_KERNVER(2,4,0) or at least 2,0,0.

Then again, maybe there is no harm in allowing administrators to set whatever they like — maybe your old Linux 2.2 binaries will run just fine.

[emaste]

sys/compat/linux/linux_mib.c

153 ↗

(On Diff #44256)

Right, but we generally don't try to impose arbitrary restrictions; disallowing <= 0 seems reasonable because it probably means something's broken, and might lead to miscalculation elsewhere. But other than that I don't see much value in preventing the user from setting what they want. If a user wants to report a Linux kernel version of 2.0, 1.0, or even 0.0.1, why should we prevent them from doing so?

Anyhow it's not a big deal and your proposed LINUX_KERNVER(1, 0, 0) is a better way to write what's there already, anyhow.

[cem]

Yeah, I buy that argument. We should check for non-negative but otherwise be permissive.

[emaste]

I'm fine with either check (<= 0 or @cem's version) with the same change applied to linux32_sysvec.c

[chuck]

In D15952#337751, @emaste wrote:

In D15952#337749, @chuck wrote:

I'm only finding two copies: sys/i386/linux/linux_sysvec.c and sys/amd64/linux/linux_sysvec.c but have updated the patch to fix those copies.

One more in sys/amd64/linux32/linux32_sysvec.c. Oh, and the last one is not yet committed, it is in D15834.

Bah, this one was named linux32_trans_osrel() so my search missed it. I'll add that to this patch as well.

sys/compat/linux/linux_mib.c

153 ↗

(On Diff #44256)

Patch has been updated to use LINUX_KERNVER(1, 0, 0) to keep the behavior consistent with previous versions.

[imp]

sys/compat/linux/linux_mib.c

153 ↗

(On Diff #44256)

0.13 was the first release. It then went to 0.14 and 0.15 and jumped to 0.93 and lumbered towards 1.0 from there...

I doubt anything pre 2.6 is still relevant. 2.4 might be for this purpose, but even that's unlikely. It's been dead for a very long time outside embedded. And for merely a long time in embedded...

[emaste]

In D15952#337808, @chuck wrote:

Bah, this one was named linux32_trans_osrel() so my search missed it. I'll add that to this patch as well.

Ah, I forgot it had a different name too. We should (separate from this change) deduplicate linux*_trans_osrel.