| Message ID | 1484145426.1643.7.camel@gmail.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed 11-01-17 09:37:06, Chas Williams wrote: > On Mon, 2017-01-09 at 18:20 +0100, Andrey Konovalov wrote: > > Hi! > > > > I've got the following error report while running the syzkaller fuzzer. > > > > On commit a121103c922847ba5010819a3f250f1f7fc84ab8 (4.10-rc3). > > > > A reproducer is attached. > > > > ------------[ cut here ]------------ > > WARNING: CPU: 0 PID: 4114 at kernel/sched/core.c:7737 __might_sleep+0x149/0x1a0 > > do not call blocking ops when !TASK_RUNNING; state=1 set at > > [<ffffffff813fcb22>] prepare_to_wait+0x182/0x530 > > Modules linked in: > > CPU: 0 PID: 4114 Comm: a.out Not tainted 4.10.0-rc3+ #59 > > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 > > Call Trace: > > __dump_stack lib/dump_stack.c:15 > > dump_stack+0x292/0x398 lib/dump_stack.c:51 > > __warn+0x19f/0x1e0 kernel/panic.c:547 > > warn_slowpath_fmt+0xc5/0x110 kernel/panic.c:562 > > __might_sleep+0x149/0x1a0 kernel/sched/core.c:7732 > > slab_pre_alloc_hook mm/slab.h:408 > > slab_alloc_node mm/slub.c:2634 > > kmem_cache_alloc_node+0x14a/0x280 mm/slub.c:2744 > > __alloc_skb+0x10f/0x800 net/core/skbuff.c:219 > > alloc_skb ./include/linux/skbuff.h:926 > > alloc_tx net/atm/common.c:75 > > This is likely alloc_skb(..., GFP_KERNEL) in alloc_tx(). The simplest > fix for this would be simply to switch this GFP_ATOMIC. See if this is > any better. > > diff --git a/net/atm/common.c b/net/atm/common.c > index a3ca922..d84220c 100644 > --- a/net/atm/common.c > +++ b/net/atm/common.c > @@ -72,7 +72,7 @@ static struct sk_buff *alloc_tx(struct atm_vcc *vcc, unsigned int size) > sk_wmem_alloc_get(sk), size, sk->sk_sndbuf); > return NULL; > } > - while (!(skb = alloc_skb(size, GFP_KERNEL))) > + while (!(skb = alloc_skb(size, GFP_ATOMIC))) > schedule(); > pr_debug("%d += %d\n", sk_wmem_alloc_get(sk), skb->truesize); > atomic_add(skb->truesize, &sk->sk_wmem_alloc); Blee, this code is just horrendous. But the "fix" is obviously broken! schedule() is just a noop if you do not change the task state and what you are just asking for is a never failing non sleeping allocation - aka a busy loop in the kernel!
On Wed 11-01-17 20:45:25, Michal Hocko wrote: > On Wed 11-01-17 09:37:06, Chas Williams wrote: > > On Mon, 2017-01-09 at 18:20 +0100, Andrey Konovalov wrote: > > > Hi! > > > > > > I've got the following error report while running the syzkaller fuzzer. > > > > > > On commit a121103c922847ba5010819a3f250f1f7fc84ab8 (4.10-rc3). > > > > > > A reproducer is attached. > > > > > > ------------[ cut here ]------------ > > > WARNING: CPU: 0 PID: 4114 at kernel/sched/core.c:7737 __might_sleep+0x149/0x1a0 > > > do not call blocking ops when !TASK_RUNNING; state=1 set at > > > [<ffffffff813fcb22>] prepare_to_wait+0x182/0x530 > > > Modules linked in: > > > CPU: 0 PID: 4114 Comm: a.out Not tainted 4.10.0-rc3+ #59 > > > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 > > > Call Trace: > > > __dump_stack lib/dump_stack.c:15 > > > dump_stack+0x292/0x398 lib/dump_stack.c:51 > > > __warn+0x19f/0x1e0 kernel/panic.c:547 > > > warn_slowpath_fmt+0xc5/0x110 kernel/panic.c:562 > > > __might_sleep+0x149/0x1a0 kernel/sched/core.c:7732 > > > slab_pre_alloc_hook mm/slab.h:408 > > > slab_alloc_node mm/slub.c:2634 > > > kmem_cache_alloc_node+0x14a/0x280 mm/slub.c:2744 > > > __alloc_skb+0x10f/0x800 net/core/skbuff.c:219 > > > alloc_skb ./include/linux/skbuff.h:926 > > > alloc_tx net/atm/common.c:75 > > > > This is likely alloc_skb(..., GFP_KERNEL) in alloc_tx(). The simplest > > fix for this would be simply to switch this GFP_ATOMIC. See if this is > > any better. > > > > diff --git a/net/atm/common.c b/net/atm/common.c > > index a3ca922..d84220c 100644 > > --- a/net/atm/common.c > > +++ b/net/atm/common.c > > @@ -72,7 +72,7 @@ static struct sk_buff *alloc_tx(struct atm_vcc *vcc, unsigned int size) > > sk_wmem_alloc_get(sk), size, sk->sk_sndbuf); > > return NULL; > > } > > - while (!(skb = alloc_skb(size, GFP_KERNEL))) > > + while (!(skb = alloc_skb(size, GFP_ATOMIC))) > > schedule(); > > pr_debug("%d += %d\n", sk_wmem_alloc_get(sk), skb->truesize); > > atomic_add(skb->truesize, &sk->sk_wmem_alloc); > > Blee, this code is just horrendous. But the "fix" is obviously broken! > schedule() is just a noop if you do not change the task state and what > you are just asking for is a never failing non sleeping allocation - aka > a busy loop in the kernel! And btw. this while loop should be really turned into GFP_KERNEL | __GFP_NOFAIL with and explanation why this allocation cannot possibly fail.
On Wed, Jan 11, 2017 at 11:46 AM, Michal Hocko <mhocko@kernel.org> wrote: > On Wed 11-01-17 20:45:25, Michal Hocko wrote: >> On Wed 11-01-17 09:37:06, Chas Williams wrote: >> > On Mon, 2017-01-09 at 18:20 +0100, Andrey Konovalov wrote: >> > > Hi! >> > > >> > > I've got the following error report while running the syzkaller fuzzer. >> > > >> > > On commit a121103c922847ba5010819a3f250f1f7fc84ab8 (4.10-rc3). >> > > >> > > A reproducer is attached. >> > > >> > > ------------[ cut here ]------------ >> > > WARNING: CPU: 0 PID: 4114 at kernel/sched/core.c:7737 __might_sleep+0x149/0x1a0 >> > > do not call blocking ops when !TASK_RUNNING; state=1 set at >> > > [<ffffffff813fcb22>] prepare_to_wait+0x182/0x530 >> > > Modules linked in: >> > > CPU: 0 PID: 4114 Comm: a.out Not tainted 4.10.0-rc3+ #59 >> > > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 >> > > Call Trace: >> > > __dump_stack lib/dump_stack.c:15 >> > > dump_stack+0x292/0x398 lib/dump_stack.c:51 >> > > __warn+0x19f/0x1e0 kernel/panic.c:547 >> > > warn_slowpath_fmt+0xc5/0x110 kernel/panic.c:562 >> > > __might_sleep+0x149/0x1a0 kernel/sched/core.c:7732 >> > > slab_pre_alloc_hook mm/slab.h:408 >> > > slab_alloc_node mm/slub.c:2634 >> > > kmem_cache_alloc_node+0x14a/0x280 mm/slub.c:2744 >> > > __alloc_skb+0x10f/0x800 net/core/skbuff.c:219 >> > > alloc_skb ./include/linux/skbuff.h:926 >> > > alloc_tx net/atm/common.c:75 >> > >> > This is likely alloc_skb(..., GFP_KERNEL) in alloc_tx(). The simplest >> > fix for this would be simply to switch this GFP_ATOMIC. See if this is >> > any better. >> > >> > diff --git a/net/atm/common.c b/net/atm/common.c >> > index a3ca922..d84220c 100644 >> > --- a/net/atm/common.c >> > +++ b/net/atm/common.c >> > @@ -72,7 +72,7 @@ static struct sk_buff *alloc_tx(struct atm_vcc *vcc, unsigned int size) >> > sk_wmem_alloc_get(sk), size, sk->sk_sndbuf); >> > return NULL; >> > } >> > - while (!(skb = alloc_skb(size, GFP_KERNEL))) >> > + while (!(skb = alloc_skb(size, GFP_ATOMIC))) >> > schedule(); >> > pr_debug("%d += %d\n", sk_wmem_alloc_get(sk), skb->truesize); >> > atomic_add(skb->truesize, &sk->sk_wmem_alloc); >> >> Blee, this code is just horrendous. But the "fix" is obviously broken! >> schedule() is just a noop if you do not change the task state and what >> you are just asking for is a never failing non sleeping allocation - aka >> a busy loop in the kernel! > > And btw. this while loop should be really turned into GFP_KERNEL | > __GFP_NOFAIL with and explanation why this allocation cannot possibly > fail. I think a nested loop is quite unnecessary, probably due to the code itself is pretty old. The alloc_tx() is in the outer loop, the alloc_skb() is in the inner loop, both seem to wait for a successful GFP allocation. The inner one is even more unnecessary. Of course, I am not surprised MM may already have a mechanism to do the similar logic. There maybe some reason ATM needs such a logic, although other proto could handle skb allocation failure quite well in ->sendmsg().
On Wed, 2017-01-11 at 20:36 -0800, Cong Wang wrote: > On Wed, Jan 11, 2017 at 11:46 AM, Michal Hocko <mhocko@kernel.org> wrote: > > On Wed 11-01-17 20:45:25, Michal Hocko wrote: > >> On Wed 11-01-17 09:37:06, Chas Williams wrote: > >> > On Mon, 2017-01-09 at 18:20 +0100, Andrey Konovalov wrote: > >> > > Hi! > >> > > > >> > > I've got the following error report while running the syzkaller fuzzer. > >> > > > >> > > On commit a121103c922847ba5010819a3f250f1f7fc84ab8 (4.10-rc3). > >> > > > >> > > A reproducer is attached. > >> > > > >> > > ------------[ cut here ]------------ > >> > > WARNING: CPU: 0 PID: 4114 at kernel/sched/core.c:7737 __might_sleep+0x149/0x1a0 > >> > > do not call blocking ops when !TASK_RUNNING; state=1 set at > >> > > [<ffffffff813fcb22>] prepare_to_wait+0x182/0x530 > >> > > Modules linked in: > >> > > CPU: 0 PID: 4114 Comm: a.out Not tainted 4.10.0-rc3+ #59 > >> > > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 > >> > > Call Trace: > >> > > __dump_stack lib/dump_stack.c:15 > >> > > dump_stack+0x292/0x398 lib/dump_stack.c:51 > >> > > __warn+0x19f/0x1e0 kernel/panic.c:547 > >> > > warn_slowpath_fmt+0xc5/0x110 kernel/panic.c:562 > >> > > __might_sleep+0x149/0x1a0 kernel/sched/core.c:7732 > >> > > slab_pre_alloc_hook mm/slab.h:408 > >> > > slab_alloc_node mm/slub.c:2634 > >> > > kmem_cache_alloc_node+0x14a/0x280 mm/slub.c:2744 > >> > > __alloc_skb+0x10f/0x800 net/core/skbuff.c:219 > >> > > alloc_skb ./include/linux/skbuff.h:926 > >> > > alloc_tx net/atm/common.c:75 > >> > > >> > This is likely alloc_skb(..., GFP_KERNEL) in alloc_tx(). The simplest > >> > fix for this would be simply to switch this GFP_ATOMIC. See if this is > >> > any better. > >> > > >> > diff --git a/net/atm/common.c b/net/atm/common.c > >> > index a3ca922..d84220c 100644 > >> > --- a/net/atm/common.c > >> > +++ b/net/atm/common.c > >> > @@ -72,7 +72,7 @@ static struct sk_buff *alloc_tx(struct atm_vcc *vcc, unsigned int size) > >> > sk_wmem_alloc_get(sk), size, sk->sk_sndbuf); > >> > return NULL; > >> > } > >> > - while (!(skb = alloc_skb(size, GFP_KERNEL))) > >> > + while (!(skb = alloc_skb(size, GFP_ATOMIC))) > >> > schedule(); > >> > pr_debug("%d += %d\n", sk_wmem_alloc_get(sk), skb->truesize); > >> > atomic_add(skb->truesize, &sk->sk_wmem_alloc); > >> > >> Blee, this code is just horrendous. But the "fix" is obviously broken! > >> schedule() is just a noop if you do not change the task state and what > >> you are just asking for is a never failing non sleeping allocation - aka > >> a busy loop in the kernel! > > > > And btw. this while loop should be really turned into GFP_KERNEL | > > __GFP_NOFAIL with and explanation why this allocation cannot possibly > > fail. > > I think a nested loop is quite unnecessary, probably due to the code itself > is pretty old. The alloc_tx() is in the outer loop, the alloc_skb() is > in the inner > loop, both seem to wait for a successful GFP allocation. The inner one > is even more unnecessary. > > Of course, I am not surprised MM may already have a mechanism to do > the similar logic. > > There maybe some reason ATM needs such a logic, although other proto > could handle skb allocation failure quite well in ->sendmsg(). I can't think of any particular reason that it needs this loop here. I suspect that the loop for alloc_tx() predates the wait logic in ->sendmsg() and that the original looping was in alloc_tx() initially and was simply never removed. Changes here would date back to before the git conversion.
On Thu, Jan 12, 2017 at 11:40 AM, Chas Williams <3chas3@gmail.com> wrote: > On Wed, 2017-01-11 at 20:36 -0800, Cong Wang wrote: >> On Wed, Jan 11, 2017 at 11:46 AM, Michal Hocko <mhocko@kernel.org> wrote: >> > On Wed 11-01-17 20:45:25, Michal Hocko wrote: >> >> On Wed 11-01-17 09:37:06, Chas Williams wrote: >> >> > On Mon, 2017-01-09 at 18:20 +0100, Andrey Konovalov wrote: >> >> > > Hi! >> >> > > >> >> > > I've got the following error report while running the syzkaller fuzzer. >> >> > > >> >> > > On commit a121103c922847ba5010819a3f250f1f7fc84ab8 (4.10-rc3). >> >> > > >> >> > > A reproducer is attached. >> >> > > >> >> > > ------------[ cut here ]------------ >> >> > > WARNING: CPU: 0 PID: 4114 at kernel/sched/core.c:7737 __might_sleep+0x149/0x1a0 >> >> > > do not call blocking ops when !TASK_RUNNING; state=1 set at >> >> > > [<ffffffff813fcb22>] prepare_to_wait+0x182/0x530 >> >> > > Modules linked in: >> >> > > CPU: 0 PID: 4114 Comm: a.out Not tainted 4.10.0-rc3+ #59 >> >> > > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 >> >> > > Call Trace: >> >> > > __dump_stack lib/dump_stack.c:15 >> >> > > dump_stack+0x292/0x398 lib/dump_stack.c:51 >> >> > > __warn+0x19f/0x1e0 kernel/panic.c:547 >> >> > > warn_slowpath_fmt+0xc5/0x110 kernel/panic.c:562 >> >> > > __might_sleep+0x149/0x1a0 kernel/sched/core.c:7732 >> >> > > slab_pre_alloc_hook mm/slab.h:408 >> >> > > slab_alloc_node mm/slub.c:2634 >> >> > > kmem_cache_alloc_node+0x14a/0x280 mm/slub.c:2744 >> >> > > __alloc_skb+0x10f/0x800 net/core/skbuff.c:219 >> >> > > alloc_skb ./include/linux/skbuff.h:926 >> >> > > alloc_tx net/atm/common.c:75 >> >> > >> >> > This is likely alloc_skb(..., GFP_KERNEL) in alloc_tx(). The simplest >> >> > fix for this would be simply to switch this GFP_ATOMIC. See if this is >> >> > any better. >> >> > >> >> > diff --git a/net/atm/common.c b/net/atm/common.c >> >> > index a3ca922..d84220c 100644 >> >> > --- a/net/atm/common.c >> >> > +++ b/net/atm/common.c >> >> > @@ -72,7 +72,7 @@ static struct sk_buff *alloc_tx(struct atm_vcc *vcc, unsigned int size) >> >> > sk_wmem_alloc_get(sk), size, sk->sk_sndbuf); >> >> > return NULL; >> >> > } >> >> > - while (!(skb = alloc_skb(size, GFP_KERNEL))) >> >> > + while (!(skb = alloc_skb(size, GFP_ATOMIC))) >> >> > schedule(); >> >> > pr_debug("%d += %d\n", sk_wmem_alloc_get(sk), skb->truesize); >> >> > atomic_add(skb->truesize, &sk->sk_wmem_alloc); >> >> >> >> Blee, this code is just horrendous. But the "fix" is obviously broken! >> >> schedule() is just a noop if you do not change the task state and what >> >> you are just asking for is a never failing non sleeping allocation - aka >> >> a busy loop in the kernel! >> > >> > And btw. this while loop should be really turned into GFP_KERNEL | >> > __GFP_NOFAIL with and explanation why this allocation cannot possibly >> > fail. >> >> I think a nested loop is quite unnecessary, probably due to the code itself >> is pretty old. The alloc_tx() is in the outer loop, the alloc_skb() is >> in the inner >> loop, both seem to wait for a successful GFP allocation. The inner one >> is even more unnecessary. >> >> Of course, I am not surprised MM may already have a mechanism to do >> the similar logic. >> >> There maybe some reason ATM needs such a logic, although other proto >> could handle skb allocation failure quite well in ->sendmsg(). > > > I can't think of any particular reason that it needs this loop here. I suspect > that the loop for alloc_tx() predates the wait logic in ->sendmsg() and that the > original looping was in alloc_tx() initially and was simply never removed. Changes > here would date back to before the git conversion. > Hi, I'm still seeing this on 4495c08e84729385774601b5146d51d9e5849f81 (4.11-rc2). Thanks!
On Mon, 2017-03-13 at 18:43 +0100, Andrey Konovalov wrote: > On Thu, Jan 12, 2017 at 11:40 AM, Chas Williams <3chas3@gmail.com> wrote: > > On Wed, 2017-01-11 at 20:36 -0800, Cong Wang wrote: > >> On Wed, Jan 11, 2017 at 11:46 AM, Michal Hocko <mhocko@kernel.org> wrote: > >> > On Wed 11-01-17 20:45:25, Michal Hocko wrote: > >> >> On Wed 11-01-17 09:37:06, Chas Williams wrote: > >> >> > On Mon, 2017-01-09 at 18:20 +0100, Andrey Konovalov wrote: > >> >> > > Hi! > >> >> > > > >> >> > > I've got the following error report while running the syzkaller fuzzer. > >> >> > > > >> >> > > On commit a121103c922847ba5010819a3f250f1f7fc84ab8 (4.10-rc3). > >> >> > > > >> >> > > A reproducer is attached. > >> >> > > > >> >> > > ------------[ cut here ]------------ > >> >> > > WARNING: CPU: 0 PID: 4114 at kernel/sched/core.c:7737 __might_sleep+0x149/0x1a0 > >> >> > > do not call blocking ops when !TASK_RUNNING; state=1 set at > >> >> > > [<ffffffff813fcb22>] prepare_to_wait+0x182/0x530 > >> >> > > Modules linked in: > >> >> > > CPU: 0 PID: 4114 Comm: a.out Not tainted 4.10.0-rc3+ #59 > >> >> > > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 > >> >> > > Call Trace: > >> >> > > __dump_stack lib/dump_stack.c:15 > >> >> > > dump_stack+0x292/0x398 lib/dump_stack.c:51 > >> >> > > __warn+0x19f/0x1e0 kernel/panic.c:547 > >> >> > > warn_slowpath_fmt+0xc5/0x110 kernel/panic.c:562 > >> >> > > __might_sleep+0x149/0x1a0 kernel/sched/core.c:7732 > >> >> > > slab_pre_alloc_hook mm/slab.h:408 > >> >> > > slab_alloc_node mm/slub.c:2634 > >> >> > > kmem_cache_alloc_node+0x14a/0x280 mm/slub.c:2744 > >> >> > > __alloc_skb+0x10f/0x800 net/core/skbuff.c:219 > >> >> > > alloc_skb ./include/linux/skbuff.h:926 > >> >> > > alloc_tx net/atm/common.c:75 > >> >> > > >> >> > This is likely alloc_skb(..., GFP_KERNEL) in alloc_tx(). The simplest > >> >> > fix for this would be simply to switch this GFP_ATOMIC. See if this is > >> >> > any better. > >> >> > > >> >> > diff --git a/net/atm/common.c b/net/atm/common.c > >> >> > index a3ca922..d84220c 100644 > >> >> > --- a/net/atm/common.c > >> >> > +++ b/net/atm/common.c > >> >> > @@ -72,7 +72,7 @@ static struct sk_buff *alloc_tx(struct atm_vcc *vcc, unsigned int size) > >> >> > sk_wmem_alloc_get(sk), size, sk->sk_sndbuf); > >> >> > return NULL; > >> >> > } > >> >> > - while (!(skb = alloc_skb(size, GFP_KERNEL))) > >> >> > + while (!(skb = alloc_skb(size, GFP_ATOMIC))) > >> >> > schedule(); > >> >> > pr_debug("%d += %d\n", sk_wmem_alloc_get(sk), skb->truesize); > >> >> > atomic_add(skb->truesize, &sk->sk_wmem_alloc); > >> >> > >> >> Blee, this code is just horrendous. But the "fix" is obviously broken! > >> >> schedule() is just a noop if you do not change the task state and what > >> >> you are just asking for is a never failing non sleeping allocation - aka > >> >> a busy loop in the kernel! > >> > > >> > And btw. this while loop should be really turned into GFP_KERNEL | > >> > __GFP_NOFAIL with and explanation why this allocation cannot possibly > >> > fail. > >> > >> I think a nested loop is quite unnecessary, probably due to the code itself > >> is pretty old. The alloc_tx() is in the outer loop, the alloc_skb() is > >> in the inner > >> loop, both seem to wait for a successful GFP allocation. The inner one > >> is even more unnecessary. > >> > >> Of course, I am not surprised MM may already have a mechanism to do > >> the similar logic. > >> > >> There maybe some reason ATM needs such a logic, although other proto > >> could handle skb allocation failure quite well in ->sendmsg(). > > > > > > I can't think of any particular reason that it needs this loop here. I suspect > > that the loop for alloc_tx() predates the wait logic in ->sendmsg() and that the > > original looping was in alloc_tx() initially and was simply never removed. Changes > > here would date back to before the git conversion. > > > > Hi, > > I'm still seeing this on 4495c08e84729385774601b5146d51d9e5849f81 (4.11-rc2). > > Thanks! David Miller just accepted a patch for net-next that should resolve this issue.
diff --git a/net/atm/common.c b/net/atm/common.c index a3ca922..d84220c 100644 --- a/net/atm/common.c +++ b/net/atm/common.c @@ -72,7 +72,7 @@ static struct sk_buff *alloc_tx(struct atm_vcc *vcc, unsigned int size) sk_wmem_alloc_get(sk), size, sk->sk_sndbuf); return NULL; } - while (!(skb = alloc_skb(size, GFP_KERNEL))) + while (!(skb = alloc_skb(size, GFP_ATOMIC))) schedule(); pr_debug("%d += %d\n", sk_wmem_alloc_get(sk), skb->truesize); atomic_add(skb->truesize, &sk->sk_wmem_alloc);