Re: INFO: rcu detected stall in bitmap_parselist

[Yury Norov <ynorov@caviumnetworks.com>]

On Thu, Apr 05, 2018 at 12:58:46AM +0900, Tetsuo Handa wrote:
> Yury Norov wrote:
> > Hi Tetsuo,
> > 
> > Thanks for the patch.
> > 
> > On Wed, Apr 04, 2018 at 09:21:43PM +0900, Tetsuo Handa wrote:
> > > Yury, are you OK with this patch?
> > > 
> > > 
> > > >From 7f21827cdfe9780b4949b22bcd19efa721b463d2 Mon Sep 17 00:00:00 2001
> > > From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> > > Date: Wed, 4 Apr 2018 21:12:10 +0900
> > > Subject: [PATCH] lib/bitmap: Rewrite __bitmap_parselist().
> > > 
> > > syzbot is catching stalls at __bitmap_parselist() [1]. The trigger is
> > > 
> > >   unsigned long v = 0;
> > >   bitmap_parselist("7:,", &v, BITS_PER_LONG);
> > 
> > Could you add this case to the test_bitmap_parselist()?
> > 
> > > which results in hitting infinite loop at
> > > 
> > >   while (a <= b) {
> > >     off = min(b - a + 1, used_size);
> > >     bitmap_set(maskp, a, off);
> > >     a += group_size;
> > >   }
> > > 
> > > due to used_size == group_size == 0.
> > > 
> > > Current code is difficult to read due to too many flag variables.
> > > Let's rewrite it.
> > 
> > I also don't like current implementation of bitmap_parselist(), but
> > discussion on new code  may take some time. Can you submit minimal
> > fix in separated patch to let people discuss your new implementation
> > without rush?
> 
> OK. Then you can write the patch. You know current code better than I.

Done.
 
> > > @@ -485,6 +485,58 @@ int bitmap_print_to_pagebuf(bool list, char *buf, const unsigned long *maskp,
> > >  }
> > >  EXPORT_SYMBOL(bitmap_print_to_pagebuf);
> > >  
> > > +static bool get_uint(const char **buf, unsigned int *res)
> > > +{
> > > +	const char *p = *buf;
> > > +
> > > +	if (!isdigit(*p))
> > > +		return false;
> > > +	*res = simple_strtoul(p, (char **) buf, 10);
> > 
> > In comment to simple_strtoul(): "This function is obsolete. Please
> > use kstrtoul instead."
> 
> I intentionally choose simple_strtoul() because next delimiter (e.g. '-')
> starts at returned address. kstrtoul() fails if next letter starts.

OK, but then it should be explained in comment, I think.
 
> > > +	return p < *buf;
> 
> I think I should limit to "0 <= *res <= INT_MAX" range in order to avoid
> overflow at start += group_size.
> 
> > > +}
> > > +
> > > +static int __bitmap_parse_one_chunk(const char *buf, unsigned long *maskp,
> > > +				    const unsigned int nmaskbits)
> > > +{
> > > +	unsigned int start;
> > > +	unsigned int end;
> > > +	unsigned int group_size;
> > > +	unsigned int used_size;
> > > +
> > > +	while (*buf && isspace(*buf))
> > > +		buf++;
> > > +	if (!get_uint(&buf, &start))
> > > +		return -EINVAL;
> > > +	if (*buf == '-') {
> > > +		buf++;
> > > +		if (!get_uint(&buf, &end) || start > end)
> > > +			return -EINVAL;
> > > +		if (*buf == ':') {
> > > +			buf++;
> > > +			if (!get_uint(&buf, &used_size) || *buf++ != '/' ||
> > > +			    !get_uint(&buf, &group_size) ||
> > > +			    used_size > group_size)
> > > +				return -EINVAL;
> > 
> > So this is still not safe against "1-10:0/0", or I miss something?
> > (This is another testcase we should add to test_bitmap.c)
> 
> Indeed. We need to make more testcases.
> 
> > > +	while (buflen && !err) {
> > > +		char *cp;
> > > +		char tmpbuf[256];
> > > +		unsigned int size = min(buflen,
> > > +					(unsigned int) sizeof(tmpbuf) - 1);
> > > +
> > > +		if (!is_user)
> > > +			memcpy(tmpbuf, buf, size);
> > > +		else if (copy_from_user(tmpbuf, (const char __user __force *)
> > > +					buf, size))
> > > +			return -EFAULT;
> > 
> > This is not safe against this:
> > "[250 whitespaces] 567-890:123/456"
> 
> Do we need to accept such insane entry?
 
This is how current implementation works - no limit on number of whitespaces
before and after the cunk. It's userspace interface, and we should be careful
adding new limitations. God forbid us break userspace. :-)

It looks insane, but this kind of things is quite possible if input string
is the result of heavy scripting.
 
> > And it will be Schlemiel the painter's-styled algorithm for input like:
> > "1,2,3,4, ... ,98,99,100". 
> > 
> > I think we need something like __bitmap_parse_get_chunk() to copy
> > coma-separated substrings.