Re: [PATCH] ext2/super: Fix a possible sleep-in-atomic bug in parse_options
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 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 SUSE Labs, CR
Re: [PATCH] ext2/super: Fix a possible sleep-in-atomic bug in parse_options
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 > --- > 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 da...@fromorbit.com
Re: [PATCH] ext2/super: Fix a possible sleep-in-atomic bug in parse_options
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...
Re: [PATCH] ext2/super: Fix a possible sleep-in-atomic bug in parse_options
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 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...
Re: [PATCH] ext2/super: Fix a possible sleep-in-atomic bug in parse_options
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 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
Re: [PATCH] ext2/super: Fix a possible sleep-in-atomic bug in parse_options
On Fri, Oct 6, 2017 at 6:20 PM, Jia-Ju Bai 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
[PATCH] ext2/super: Fix a possible sleep-in-atomic bug in parse_options
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 --- 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
