Add sanity check for CK_CLYGRP

[imp]

Summary

Only try to enable CK_CLYGRP if we're running on kernel newer than 1200046, the first version that supports this feature. If we set it, then use an old kernel, we'll break the 'contract' of having checksummed cylinder groups this flag signifies. To avoid creating something with an inconsistent state, don't turn the flag on in these cases. The first full fsck with a new kernel will turn this on.

Spnsored by: Netflix

[kib]

sys/ufs/ffs/fs.h

457 ↗

(On Diff #35309)

Do not do this.

Please look at the sys/param.h and define a new symbol in the P_OSREL namespace. IN_RTLD worth to be renamed but I did not bothered yet.

[emaste]

I think this change is good (modulo kib's comment).

sbin/newfs/mkfs.c

498 ↗

(On Diff #35309)

I think we should still have a warning here, since this is quite likely not expected/intended. It may also serve as a reminder to add such a check for any future additions.

[kib]

sbin/newfs/mkfs.c

498 ↗

(On Diff #35309)

We already have a check in the kernel. Look for p_osrel in kern/kern_exec.c and disallow_high_osrel. By default, if you run too new binary on older kernel, you get the warning.

Of course, this assumes that crt used for binary linking is synced with the binary sources.

[emaste]

sbin/newfs/mkfs.c

498 ↗

(On Diff #35309)

I mean specifically for this case of running a tool that will create a long-lived object with different properties based on the running kernel, that we use as part of the release process.

[imp]

sbin/newfs/mkfs.c

498 ↗

(On Diff #35309)

I absolutely do not think we need a warning here. The warning is ~useless in this case. What can a user do about it? Often, in the moment, nothing. It will scare them for no reason. And if it's part of the release, it will be lost in the noise. The release process is fundamentally flawed, we can't fix it here.

In addition, fsck will automatically turn this on when the kernel is the right version. Having it not turned on at this stage is 100% OK.

There's a lot of places in the filesystem code where we do things conditionally based on what we're looking at right now. We don't enable TRIM, for example, on devices that don't support it, which is a bad default for things like SD cards or SSDs/NVMe drives. We fail safe. In addition, if TRIM is turned on in the filesystem, but the underlying system doesn't support it, we turn it off. We have layout policies based on spinning vs non-spinning that we don't warn about, but quietly adapt to if it changes for some reason. This issue is on par with all of those: we do the safe thing.

In this case, the user didn't specifically say "I want this new feature." It was something we automatically turned on because we thought we could use it. If we later changed our minds to say "oh, we can't automatically do this smart thing we though we could earlier" it seems rather silly when the rest of the system is designed to cope with this feature appearing. fsck doesn't have a specific knob to turn it off, other than answering 'no' at a particular prompt.

sys/ufs/ffs/fs.h

457 ↗

(On Diff #35309)

I'd forgotten about that.

[emaste]

sbin/newfs/mkfs.c

498 ↗

(On Diff #35309)

What can a user do about it? Often, in the moment, nothing. It will scare them for no reason.

Any user who would encounter such a warning is doing something unsupported, and ought to take a step back and understand why.

Anyway, as I said elsewhere I think it's sensible to have a warning for this but don't have a strong objection to excluding it. I guess the warning emitted by the kernel upon executing a too-new executable will be sufficient for scaring users.

[imp]

Fix per kib's comments.

[kib]

sys/ufs/ffs/fs.h

457 ↗

(On Diff #35309)

The define ought to be removed.

That said, the version values in FREEBSD_VERS_CYLGRP and P_OSREL_CK_CLYGRP are different.

[imp]

Use correct version

[imp]

sys/ufs/ffs/fs.h

457 ↗

(On Diff #35309)

Yes. Removed. And Yes, fixed. This value was right and the one I put in sys/sys/param.h was from memory and wrong. Corrected that.

[emaste]

Ok, no objection to this patch.

[kib]

How does this compile without -DIN_RTLD ?

[imp]

In D13114#272815, @kib wrote:

How does this compile without -DIN_RTLD ?

Apparently, it doesn't.... So, move the #define outside of the IN_RTLD ifdef, or define IN_RTLD for these two files? I'm guessing due to name space pollution the latter is better...

[kib]

In D13114#272832, @imp wrote:

In D13114#272815, @kib wrote:

How does this compile without -DIN_RTLD ?

Apparently, it doesn't.... So, move the #define outside of the IN_RTLD ifdef, or define IN_RTLD for these two files? I'm guessing due to name space pollution the latter is better...

Yes, define IN_RTLD for now. One day I will do a sweep to rename the controlling symbol.

[imp]

Define IN_RTLD to pickup P_OSREL stuff