Re: [PATCH 09/12] btrfs: change set_level() to bound the level passed in

From: David Sterba <dsterba@suse.cz>
To: Dennis Zhou <dennis@kernel.org>
Cc: David Sterba <dsterba@suse.cz>, David Sterba <dsterba@suse.com>,
	Josef Bacik <josef@toxicpanda.com>, Chris Mason <clm@fb.com>,
	Omar Sandoval <osandov@osandov.com>,
	Nick Terrell <terrelln@fb.com>,
	Nikolay Borisov <nborisov@suse.com>,
	kernel-team@fb.com, linux-btrfs@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 09/12] btrfs: change set_level() to bound the level passed in
Date: Tue, 5 Feb 2019 20:54:15 +0100	[thread overview]
Message-ID: <20190205195414.GA2900@twin.jikos.cz> (raw)
In-Reply-To: <20190205193254.GB24701@dennisz-mbp.dhcp.thefacebook.com>

On Tue, Feb 05, 2019 at 02:32:54PM -0500, Dennis Zhou wrote:
> On Tue, Feb 05, 2019 at 08:06:37PM +0100, David Sterba wrote:
> > On Mon, Feb 04, 2019 at 03:20:05PM -0500, Dennis Zhou wrote:
> > > -unsigned int btrfs_compress_str2level(const char *str)
> > > +unsigned int btrfs_compress_str2level(unsigned int type, const char *str)
> > >  {
> > > -	if (strncmp(str, "zlib", 4) != 0)
> > > +	unsigned int level;
> > > +	int ret;
> > > +
> > > +	if (!type)
> > >  		return 0;
> > >  
> > > -	/* Accepted form: zlib:1 up to zlib:9 and nothing left after the number */
> > > -	if (str[4] == ':' && '1' <= str[5] && str[5] <= '9' && str[6] == 0)
> > > -		return str[5] - '0';
> > > +	if (str[0] == ':') {
> > > +		ret = kstrtouint(str + 1, 10, &level);
> > 
> > The docs kstrtouint of say that initial + is also accepted, I'd rather
> > keep the level specification strict, ie. no "zlib:+3" and no garbage
> > after the number.
> > 
> > The validation is currently missing but I think we should catch levels
> > out of range during mount/remount. The fallback to default is a safety
> > but wrong specification should be communicated to the user early.
> 
> Ok. To make sure I understand properly for improper level (ie "30",
> "+3", "+3d") set the level to default (already done) and pr_warn saying
> invalid level?

So we have (at least) two ways how to handle:

- warn and fail the mount -- catch typos and the like, continuing with
  default could cause problems later though not that severe as the
  compressionw would be different than expected, but this could be
  considered a usability bug

- only warn and continue with the default -- not mounting a root
  filesystem just because of a typo can be worse than mounting with
  wrong level; as long as there's a warning in the log, the user still
  has a working system and can fix it manually

Both have some pros and cons and initially I was more inclined to pick
the 1st option, but now that I'm thinking about that again, the other
has some merit.

Given that zlib now falls back to default for unrecognized level, we
should probably stick to that for zstd too.