The kernel may sleep under a spinlock, and the function call path is:
ext2_remount
parse_options
match_int
match_number (lib/parser.c)
kmalloc(GFP_KERNEL) --> may sleep
To fix it, GFP_KERNEL is replaced with GFP_ATOMIC.
This bug is found by my static analysis tool and my code review.
Signed-off-by: Jia-Ju Bai <[email protected]>
---
lib/parser.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/parser.c b/lib/parser.c
index 3278958..bc6e2ce 100644
--- a/lib/parser.c
+++ b/lib/parser.c
@@ -133,7 +133,7 @@ static int match_number(substring_t *s, int *result, int base)
long val;
size_t len = s->to - s->from;
- buf = kmalloc(len + 1, GFP_KERNEL);
+ buf = kmalloc(len + 1, GFP_ATOMIC);
if (!buf)
return -ENOMEM;
memcpy(buf, s->from, len);
--
1.7.9.5
On Fri, Oct 6, 2017 at 6:20 PM, Jia-Ju Bai <[email protected]> wrote:
>
> To fix it, GFP_KERNEL is replaced with GFP_ATOMIC.
> This bug is found by my static analysis tool and my code review.
I'm not saying your patch is wrong, but it's a shame that we do that
extra allocation in match_number() and match_u64int(), and that we
don't have anything that is just size-limited.
And there really isn't anything saying that we shouldn't do the same
silly thing to match_u64int(). Maybe we don't have any actual users
that need it for now, but still..
Oh well.
I do wonder if we shouldn't just use something like
"skip leading zeroes, copy to size-limited stack location instead"
because the input length really *is* limited once you skip leading
zeroes (and whatever base marker we have). We might have at most a
64-bit value in octal, so 22 bytes max.
But I guess just changing the two GFP_KERNEL's to GFP_ATOMIC is much simpler.
Linus
Thanks for your reply.
I agree that extra allocation in match_number() and match_u64int() may
be unnecessary.
Thanks,
Jia-Ju Bai
On 2017/10/7 9:37, Linus Torvalds wrote:
> On Fri, Oct 6, 2017 at 6:20 PM, Jia-Ju Bai <[email protected]> wrote:
>> To fix it, GFP_KERNEL is replaced with GFP_ATOMIC.
>> This bug is found by my static analysis tool and my code review.
> I'm not saying your patch is wrong, but it's a shame that we do that
> extra allocation in match_number() and match_u64int(), and that we
> don't have anything that is just size-limited.
>
> And there really isn't anything saying that we shouldn't do the same
> silly thing to match_u64int(). Maybe we don't have any actual users
> that need it for now, but still..
>
> Oh well.
>
> I do wonder if we shouldn't just use something like
>
> "skip leading zeroes, copy to size-limited stack location instead"
>
> because the input length really *is* limited once you skip leading
> zeroes (and whatever base marker we have). We might have at most a
> 64-bit value in octal, so 22 bytes max.
>
> But I guess just changing the two GFP_KERNEL's to GFP_ATOMIC is much simpler.
>
> Linus
On Fri, Oct 06, 2017 at 06:37:11PM -0700, Linus Torvalds wrote:
> On Fri, Oct 6, 2017 at 6:20 PM, Jia-Ju Bai <[email protected]> wrote:
> >
> > To fix it, GFP_KERNEL is replaced with GFP_ATOMIC.
> > This bug is found by my static analysis tool and my code review.
>
> I'm not saying your patch is wrong, but it's a shame that we do that
> extra allocation in match_number() and match_u64int(), and that we
> don't have anything that is just size-limited.
>
> And there really isn't anything saying that we shouldn't do the same
> silly thing to match_u64int(). Maybe we don't have any actual users
> that need it for now, but still..
>
> Oh well.
>
> I do wonder if we shouldn't just use something like
>
> "skip leading zeroes, copy to size-limited stack location instead"
>
> because the input length really *is* limited once you skip leading
> zeroes (and whatever base marker we have). We might have at most a
> 64-bit value in octal, so 22 bytes max.
>
> But I guess just changing the two GFP_KERNEL's to GFP_ATOMIC is much simpler.
There's match_strdup() as well...
FWIW, ext2 side also looks fishy; it might be cleaner if we
collected new state into some object and applied it only after the last
possible failure exit. The entire "restore the original state" logics
would go away...
On Sat, Oct 07, 2017 at 03:02:17AM +0100, Al Viro wrote:
> > I do wonder if we shouldn't just use something like
> >
> > "skip leading zeroes, copy to size-limited stack location instead"
> >
> > because the input length really *is* limited once you skip leading
> > zeroes (and whatever base marker we have). We might have at most a
> > 64-bit value in octal, so 22 bytes max.
> >
> > But I guess just changing the two GFP_KERNEL's to GFP_ATOMIC is much simpler.
>
> There's match_strdup() as well...
>
> FWIW, ext2 side also looks fishy; it might be cleaner if we
> collected new state into some object and applied it only after the last
> possible failure exit. The entire "restore the original state" logics
> would go away...
I'm not saying that the bug had been introduced by conversion to
spinlock, BTW - it was racy back when ext2_remount() relied upon BKL.
I hadn't considered the atomicity issues back then - mea culpa...
On Sat, Oct 07, 2017 at 09:20:46AM +0800, Jia-Ju Bai wrote:
> The kernel may sleep under a spinlock, and the function call path is:
> ext2_remount
> parse_options
> match_int
> match_number (lib/parser.c)
> kmalloc(GFP_KERNEL) --> may sleep
>
> To fix it, GFP_KERNEL is replaced with GFP_ATOMIC.
> This bug is found by my static analysis tool and my code review.
>
> Signed-off-by: Jia-Ju Bai <[email protected]>
> ---
> lib/parser.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/parser.c b/lib/parser.c
> index 3278958..bc6e2ce 100644
> --- a/lib/parser.c
> +++ b/lib/parser.c
> @@ -133,7 +133,7 @@ static int match_number(substring_t *s, int *result, int base)
> long val;
> size_t len = s->to - s->from;
>
> - buf = kmalloc(len + 1, GFP_KERNEL);
> + buf = kmalloc(len + 1, GFP_ATOMIC);
That seems like the wrong thing to do.
The problem is that ext2_remount is running it's internal
parse_options() under a spinlock, rather than doing the parsing with
no locks held and then only taking the locks when it needs to change
the superblock state.
At a quick glance, I don't see any other filesystem with the same
problem....
Cheers,
Dave.
--
Dave Chinner
[email protected]
On Sat 07-10-17 03:02:17, Al Viro wrote:
> On Fri, Oct 06, 2017 at 06:37:11PM -0700, Linus Torvalds wrote:
> > On Fri, Oct 6, 2017 at 6:20 PM, Jia-Ju Bai <[email protected]> wrote:
> > >
> > > To fix it, GFP_KERNEL is replaced with GFP_ATOMIC.
> > > This bug is found by my static analysis tool and my code review.
> >
> > I'm not saying your patch is wrong, but it's a shame that we do that
> > extra allocation in match_number() and match_u64int(), and that we
> > don't have anything that is just size-limited.
> >
> > And there really isn't anything saying that we shouldn't do the same
> > silly thing to match_u64int(). Maybe we don't have any actual users
> > that need it for now, but still..
> >
> > Oh well.
> >
> > I do wonder if we shouldn't just use something like
> >
> > "skip leading zeroes, copy to size-limited stack location instead"
> >
> > because the input length really *is* limited once you skip leading
> > zeroes (and whatever base marker we have). We might have at most a
> > 64-bit value in octal, so 22 bytes max.
> >
> > But I guess just changing the two GFP_KERNEL's to GFP_ATOMIC is much simpler.
>
> There's match_strdup() as well...
>
> FWIW, ext2 side also looks fishy; it might be cleaner if we
> collected new state into some object and applied it only after the last
> possible failure exit. The entire "restore the original state" logics
> would go away...
Well, it's not like the restore logic would be that difficult for ext2. But
I agree that running the whole parsing logic under a spinlock is
unnecessary and accumulating all the changes in one structure and then
applying them looks like a cleaner way to go. I'll look into that.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR