svn commit: r325860 - head/sbin/newfs

[Warner Losh]

On Wed, Nov 15, 2017 at 10:17 PM, Warner Losh <imp at bsdimp.com> wrote:

>
>
> On Wed, Nov 15, 2017 at 6:46 PM, Ed Maste <emaste at freebsd.org> wrote:
>
>> On 15 November 2017 at 19:36, Warner Losh <imp at bsdimp.com> wrote:
>> >
>> > On Wed, Nov 15, 2017 at 5:05 PM, Ed Maste <emaste at freebsd.org> wrote:
>> >>
>> >> On 15 November 2017 at 13:47, Rodney W. Grimes
>> >> <freebsd at pdx.rh.cn85.dnsmgr.net> wrote:
>> >> >> Author: emaste
>> >> >> Date: Wed Nov 15 18:40:40 2017
>> >> >> New Revision: 325860
>> >> >> URL: https://svnweb.freebsd.org/changeset/base/325860
>> >> >>
>> >> >> Log:
>> >> >>   newfs: warn if newer than kernel
>> >> >>
>> >> >>   Creating a UFS filesystem with a newfs newer than the running
>> kernel,
>> >> >>   and then mounting that filesystem, can lead to interesting
>> failures.
>> >> >>
>> >> >>   Add a safety belt to explicitly warn when newfs is newer than the
>> >> >>   running kernel.
>> >> >
>> >> > You should probably make the warning if (newer || older) as
>> >> > either is likely to have interesting side effects, as are
>> >> > mounting ufs file systems on different versions.
>> >>
>> >> Why would an older newfs cause trouble? Forward compatibility should be
>> >> fine
>> >
>> > The only scenario that 'old' would cause problems is that if you did a
>> newfs
>> > with a new binary on a new kernel, mounted the file system, wrote files
>> to
>> > it, then rebooted with an old kernel, mounted the filesystem there,
>> writing
>> > new files to it, and then unmounting and running with a new kernel.
>>
>> Right, but that's not older newfs. AFAICT there's no reason at all for
>> a (newer || older) warning.
>
>
> I concur.
>
> > I'm not sure that the new safety belt is reasonable. Today it's fine, but
>> > over time it will start producing false-positive warnings since the real
>> > issue is just before/after the cg change, not old/new in general. I'd be
>> > tempted to make a check against newfs being >= 1200046 while the kernel
>> is <
>> > 1200046. There wasn't a specific bump for this change to sys/param.h,
>> but
>> > this version was bumped within a few hours of Kirk's change.
>>
>> Well, we don't in general support using a userland newer than the
>> running kernel, other than on a best-effort basis to facilitate
>> upgrades and development. This one is only a warning so I don't see
>> much harm in leaving it in place, and it would catch any new cases of
>> a similar nature. If such a warning was already in place we might have
>> avoided the issue where our snapshots produced checksum mismatch
>> messages. But I don't have a strong objection to a hardcoded version
>> check.
>>
>
> What would have fixed the snapshot isn't a warning that nobody will
> notice. But rather something like the following:
>
> diff --git a/sbin/fsck_ffs/pass5.c b/sbin/fsck_ffs/pass5.c
> index 16c46bece00..06e1838a7f1 100644
> --- a/sbin/fsck_ffs/pass5.c
> +++ b/sbin/fsck_ffs/pass5.c
> @@ -73,6 +73,7 @@ pass5(void)
>         newcg->cg_niblk = fs->fs_ipg;
>         if (preen == 0 && yflag == 0 && fs->fs_magic == FS_UFS2_MAGIC &&
>             fswritefd != -1 && (fs->fs_metackhash & CK_CYLGRP) == 0 &&
> +           getosreldate() >= 1200046 &&
>             reply("ADD CYLINDER GROUP CHECKSUM PROTECTION") != 0) {
>                 fs->fs_metackhash |= CK_CYLGRP;
>                 rewritecg = 1;
> diff --git a/sbin/newfs/mkfs.c b/sbin/newfs/mkfs.c
> index f68c42ec6b3..0e7ee539265 100644
> --- a/sbin/newfs/mkfs.c
> +++ b/sbin/newfs/mkfs.c
> @@ -495,7 +495,7 @@ restart:
>         /*
>          * Set flags for metadata that is being check-hashed.
>          */
> -       if (Oflag > 1)
> +       if (Oflag > 1 && getosreldate() >= 1200046)
>                 sblock.fs_metackhash = CK_CYLGRP;
>
>         /*
>
> which would avoid setting the flag on a problematical kernel. Here forward
> compat is easy, and the consequences are scary messages, so I think we
> should do something more like the above. I don't think we need some kind of
> "do it anyway" override flag. since that doesn't fit well with the rest of
> UFS "works by default where we can figure it out" philosophy.
>
> I'll cleanup the above with a #define for 1200046. I've cc'd Kirk to see
> what he thinks of the idea. It generally fits with what we've done in the
> past for forward compat that's easy but protects the user from harshness.
>

I've gone ahead and tidied it up in https://reviews.freebsd.org/D13114 for
anybody that's interested.

Warner