Paths

Table of Contentst

Fix the Linux kernel version number calculation
ClosedPublic
Actions

Authored by chuck on Jun 21 2018, 2:47 PM.

Details

Reviewers

imp
ken
emaste
cem

Group Reviewers

Linux Emulation

Commits

rS335515: Fix the Linux kernel version number calculation

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

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

chuck created this revision.Jun 21 2018, 2:47 PM

Harbormaster completed remote builds in B17512: Diff 44224.Jun 21 2018, 2:47 PM

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.)

This revision now requires changes to proceed.Jun 21 2018, 4:23 PM

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

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?

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.

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.

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.

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

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.

[v2] Fix the Linux kernel version number calculation

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

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

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).

[v3] Fix the Linux kernel version number calculation

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

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.

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.

LGTM modulo one nit below

sys/compat/linux/linux_mib.c
154	Maybe spell the magic number as `LINUX_KERNVER(1, 0, 0)`.

This revision is now accepted and ready to land.Jun 21 2018, 8:04 PM

emaste added inline comments.Jun 21 2018, 8:07 PM

sys/compat/linux/linux_mib.c
154	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 added inline comments.Jun 21 2018, 8:13 PM

sys/compat/linux/linux_mib.c
154	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 added inline comments.Jun 21 2018, 8:18 PM

sys/compat/linux/linux_mib.c
154	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.

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

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

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
154	Patch has been updated to use `LINUX_KERNVER(1, 0, 0)` to keep the behavior consistent with previous versions.

chuck marked an inline comment as done.Jun 21 2018, 9:54 PM

imp accepted this revision.Jun 21 2018, 10:44 PM

imp added inline comments.

sys/compat/linux/linux_mib.c
154	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...

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.

Closed by commit rS335515: Fix the Linux kernel version number calculation (authored by chuck). · Explain WhyJun 22 2018, 12:02 AM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Path

Size

sys/

compat/

linux/

linux_mib.c

2 lines

Diff 44253

View Options

sys/compat/linux/linux_mib.c

Fix the Linux kernel version number calculationClosedPublicActions