linux-kernel - Re: [PATCH] btrfs: Accept and ignore compression level for lzo

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aKiSpTytAOXgHan5@mozart.vkv.me>
Date: Fri, 22 Aug 2025 08:54:13 -0700
From: Calvin Owens <calvin@...nvd.org>
To: Qu Wenruo <quwenruo.btrfs@....com>
Cc: Sun YangKai <sunk67188@...il.com>, clm@...com, dsterba@...e.com,
	josef@...icpanda.com, linux-btrfs@...r.kernel.org,
	linux-kernel@...r.kernel.org, neelx@...e.com
Subject: Re: [PATCH] btrfs: Accept and ignore compression level for lzo

On Friday 08/22 at 19:53 +0930, Qu Wenruo wrote:
> 在 2025/8/22 19:50, Sun YangKai 写道:
> > > The compression level is meaningless for lzo, but before commit
> > > 3f093ccb95f30 ("btrfs: harden parsing of compression mount options"),
> > > it was silently ignored if passed.
> > > 
> > > After that commit, passing a level with lzo fails to mount:
> > >      BTRFS error: unrecognized compression value lzo:1
> > > 
> > > Restore the old behavior, in case any users were relying on it.
> > > 
> > > Fixes: 3f093ccb95f30 ("btrfs: harden parsing of compression mount options")
> > > Signed-off-by: Calvin Owens <calvin@...nvd.org>
> > > ---
> > > 
> > >   fs/btrfs/super.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> > > index a262b494a89f..7ee35038c7fb 100644
> > > --- a/fs/btrfs/super.c
> > > +++ b/fs/btrfs/super.c
> > > @@ -299,7 +299,7 @@ static int btrfs_parse_compress(struct btrfs_fs_context
> > > *ctx,>
> > >   		btrfs_set_opt(ctx->mount_opt, COMPRESS);
> > >   		btrfs_clear_opt(ctx->mount_opt, NODATACOW);
> > >   		btrfs_clear_opt(ctx->mount_opt, NODATASUM);
> > > 
> > > -	} else if (btrfs_match_compress_type(string, "lzo", false)) {
> > > +	} else if (btrfs_match_compress_type(string, "lzo", true)) {
> > > 
> > >   		ctx->compress_type = BTRFS_COMPRESS_LZO;
> > >   		ctx->compress_level = 0;
> > >   		btrfs_set_opt(ctx->mount_opt, COMPRESS);
> > > 
> > > --
> > > 2.47.2
> > 
> > A possible improvement would be to emit a warning in
> > btrfs_match_compress_type() when @may_have_level is false but a
> > level is still provided. And the warning message can be something like
> > "Providing a compression level for {compression_type} is not supported, the
> > level is ignored."
> > 
> > This way:
> > 1. users receive a clearer hint about what happened,
> 
> I'm fine with the extra warning, but I do not believe those kind of users
> who provides incorrect mount option will really read the dmesg.
> 
> > 2. existing setups relying on this behavior continue to work,
> 
> Or let them fix the damn incorrect mount option.

You're acting like I'm asking for "compress=lzo:iamafancyboy" to keep
working here. I think what I proposed is a lot more reasonable than
that, I'm *really* surprised you feel so strongly about this.

In my case it was actually little ARM boards with an /etc/fstab
generated by templating code that didn't understand lzo is special.

I'm not debating that it's incorrect (I've already fixed it). But given
that passing the level has worked forever, I'm sure this thing sitting
on my desk right now is not the only thing in the world that assumed it
would keep working...

> I'm fine with warning, but the mount should still fail.
> Or those people will never learn to read the doc.

The warning is pointless IMHO, it's already obvious why it failed. My
only goal was to avoid breaking existing systems in the real world when
they upgrade the kernel.

If you'd take a patch that makes it work with a WARN(), I'll happily
send you that. But I'm not going to add the WARN() and keep it failing:
if that's all you'll accept, let's just drop it.

Thanks,
Calvin

> > 3. the @may_have_level semantics remain consistent.
> > 
> >