| Message ID | 1507339246-13067-1-git-send-email-baijiaju1990@163.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | ext2/super: Fix a possible sleep-in-atomic bug in parse_options | expand |
On Fri, Oct 6, 2017 at 6:20 PM, Jia-Ju Bai <baijiaju1990@163.com> 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 <baijiaju1990@163.com> 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 <baijiaju1990@163.com> 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 <baijiaju1990@163.com> > --- > 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.
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 <baijiaju1990@163.com> 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
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);
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 <baijiaju1990@163.com> --- lib/parser.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)