linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* net/atm: warning in alloc_tx/__might_sleep
@ 2017-01-09 17:20 Andrey Konovalov
  2017-01-10 17:35 ` Cong Wang
  2017-01-11 14:37 ` Chas Williams
  0 siblings, 2 replies; 14+ messages in thread
From: Andrey Konovalov @ 2017-01-09 17:20 UTC (permalink / raw)
  To: David S. Miller, Alexey Kuznetsov, James Morris,
	Hideaki YOSHIFUJI, Patrick McHardy, netdev, LKML, Al Viro
  Cc: Dmitry Vyukov, Kostya Serebryany, Eric Dumazet, syzkaller

[-- Attachment #1: Type: text/plain, Size: 1866 bytes --]

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
 vcc_sendmsg+0x5e8/0x1010 net/atm/common.c:609
 sock_sendmsg_nosec net/socket.c:635
 sock_sendmsg+0xca/0x110 net/socket.c:645
 ___sys_sendmsg+0x9d2/0xae0 net/socket.c:1985
 __sys_sendmsg+0x138/0x320 net/socket.c:2019
 SYSC_sendmsg net/socket.c:2030
 SyS_sendmsg+0x2d/0x50 net/socket.c:2026
 entry_SYSCALL_64_fastpath+0x1f/0xc2 arch/x86/entry/entry_64.S:203
RIP: 0033:0x7fcbacfddb79
RSP: 002b:00007ffed8b5a7b8 EFLAGS: 00000206 ORIG_RAX: 000000000000002e
RAX: ffffffffffffffda RBX: 00007ffed8b5a950 RCX: 00007fcbacfddb79
RDX: 000000000000c000 RSI: 0000000020002000 RDI: 0000000000000003
RBP: 0000000000400af0 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000206 R12: 0000000000000000
R13: 00007ffed8b5a950 R14: 0000000000000000 R15: 0000000000000000
---[ end trace 9edf2da84d8112da ]---
atm:sigd_send: bad message type 0

[-- Attachment #2: sched-sleep-warn-poc.c --]
[-- Type: text/x-csrc, Size: 5526 bytes --]

// autogenerated by syzkaller (http://github.com/google/syzkaller)

#ifndef __NR_mmap
#define __NR_mmap 9
#endif
#ifndef __NR_socket
#define __NR_socket 41
#endif
#ifndef __NR_ioctl
#define __NR_ioctl 16
#endif
#ifndef __NR_sendmsg
#define __NR_sendmsg 46
#endif

#define _GNU_SOURCE

#include <sys/ioctl.h>
#include <sys/mman.h>
#include <sys/mount.h>
#include <sys/prctl.h>
#include <sys/resource.h>
#include <sys/socket.h>
#include <sys/stat.h>
#include <sys/syscall.h>
#include <sys/time.h>
#include <sys/types.h>
#include <sys/wait.h>

#include <linux/capability.h>
#include <linux/if.h>
#include <linux/if_tun.h>
#include <linux/sched.h>
#include <net/if_arp.h>

#include <assert.h>
#include <dirent.h>
#include <errno.h>
#include <fcntl.h>
#include <grp.h>
#include <pthread.h>
#include <setjmp.h>
#include <signal.h>
#include <stdarg.h>
#include <stdbool.h>
#include <stddef.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>

const int kFailStatus = 67;
const int kErrorStatus = 68;
const int kRetryStatus = 69;

__attribute__((noreturn)) void doexit(int status)
{
  syscall(__NR_exit_group, status);
  volatile unsigned i;
  for (i = 0;; i++) {
  }
}

__attribute__((noreturn)) void fail(const char* msg, ...)
{
  int e = errno;
  fflush(stdout);
  va_list args;
  va_start(args, msg);
  vfprintf(stderr, msg, args);
  va_end(args);
  fprintf(stderr, " (errno %d)\n", e);
  doexit(e == ENOMEM ? kRetryStatus : kFailStatus);
}

__attribute__((noreturn)) void exitf(const char* msg, ...)
{
  int e = errno;
  fflush(stdout);
  va_list args;
  va_start(args, msg);
  vfprintf(stderr, msg, args);
  va_end(args);
  fprintf(stderr, " (errno %d)\n", e);
  doexit(kRetryStatus);
}

static int flag_debug;

void debug(const char* msg, ...)
{
  if (!flag_debug)
    return;
  va_list args;
  va_start(args, msg);
  vfprintf(stdout, msg, args);
  va_end(args);
  fflush(stdout);
}

__thread int skip_segv;
__thread jmp_buf segv_env;

static void segv_handler(int sig, siginfo_t* info, void* uctx)
{
  if (__atomic_load_n(&skip_segv, __ATOMIC_RELAXED))
    _longjmp(segv_env, 1);
  doexit(sig);
  for (;;) {
  }
}

static void install_segv_handler()
{
  struct sigaction sa;
  memset(&sa, 0, sizeof(sa));
  sa.sa_sigaction = segv_handler;
  sa.sa_flags = SA_NODEFER | SA_SIGINFO;
  sigaction(SIGSEGV, &sa, NULL);
  sigaction(SIGBUS, &sa, NULL);
}

#define NONFAILING(...)                                                \
  {                                                                    \
    __atomic_fetch_add(&skip_segv, 1, __ATOMIC_SEQ_CST);               \
    if (_setjmp(segv_env) == 0) {                                      \
      __VA_ARGS__;                                                     \
    }                                                                  \
    __atomic_fetch_sub(&skip_segv, 1, __ATOMIC_SEQ_CST);               \
  }

static uintptr_t execute_syscall(int nr, uintptr_t a0, uintptr_t a1,
                                 uintptr_t a2, uintptr_t a3,
                                 uintptr_t a4, uintptr_t a5,
                                 uintptr_t a6, uintptr_t a7,
                                 uintptr_t a8)
{
  switch (nr) {
  default:
    return syscall(nr, a0, a1, a2, a3, a4, a5);
  }
}

static void setup_main_process(uint64_t pid, bool enable_tun)
{
  struct sigaction sa;
  memset(&sa, 0, sizeof(sa));
  sa.sa_handler = SIG_IGN;
  syscall(SYS_rt_sigaction, 0x20, &sa, NULL, 8);
  syscall(SYS_rt_sigaction, 0x21, &sa, NULL, 8);
  install_segv_handler();

  char tmpdir_template[] = "./syzkaller.XXXXXX";
  char* tmpdir = mkdtemp(tmpdir_template);
  if (!tmpdir)
    fail("failed to mkdtemp");
  if (chmod(tmpdir, 0777))
    fail("failed to chmod");
  if (chdir(tmpdir))
    fail("failed to chdir");
}

static void loop();

static void sandbox_common()
{
  prctl(PR_SET_PDEATHSIG, SIGKILL, 0, 0, 0);
  setpgrp();
  setsid();

  struct rlimit rlim;
  rlim.rlim_cur = rlim.rlim_max = 128 << 20;
  setrlimit(RLIMIT_AS, &rlim);
  rlim.rlim_cur = rlim.rlim_max = 1 << 20;
  setrlimit(RLIMIT_FSIZE, &rlim);
  rlim.rlim_cur = rlim.rlim_max = 1 << 20;
  setrlimit(RLIMIT_STACK, &rlim);
  rlim.rlim_cur = rlim.rlim_max = 0;
  setrlimit(RLIMIT_CORE, &rlim);

  unshare(CLONE_NEWNS);
  unshare(CLONE_NEWIPC);
  unshare(CLONE_IO);
}

static int do_sandbox_none()
{
  int pid = fork();
  if (pid)
    return pid;
  sandbox_common();
  loop();
  doexit(1);
}

long r[11];
void loop()
{
  memset(r, -1, sizeof(r));
  r[0] = execute_syscall(__NR_mmap, 0x20000000ul, 0x4000ul, 0x3ul,
                         0x32ul, 0xfffffffffffffffful, 0x0ul, 0, 0, 0);
  r[1] = execute_syscall(__NR_socket, 0x8ul, 0x0ul, 0x0ul, 0, 0, 0, 0,
                         0, 0);
  r[2] = execute_syscall(__NR_ioctl, r[1], 0x61f0ul, 0x20003000ul, 0, 0,
                         0, 0, 0, 0);
  NONFAILING(*(uint64_t*)0x20002000 = (uint64_t)0x0);
  NONFAILING(*(uint32_t*)0x20002008 = (uint32_t)0x0);
  NONFAILING(*(uint64_t*)0x20002010 = (uint64_t)0x20002000);
  NONFAILING(*(uint64_t*)0x20002018 = (uint64_t)0x2);
  NONFAILING(*(uint64_t*)0x20002020 = (uint64_t)0x0);
  NONFAILING(*(uint64_t*)0x20002028 = (uint64_t)0x0);
  NONFAILING(*(uint32_t*)0x20002030 = (uint32_t)0x0);
  r[10] = execute_syscall(__NR_sendmsg, r[1], 0x20002000ul, 0xc000ul, 0,
                          0, 0, 0, 0, 0);
}
int main()
{
  setup_main_process(0, false);
  int pid = do_sandbox_none();
  int status = 0;
  while (waitpid(pid, &status, __WALL) != pid) {
  }
  return 0;
}

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: net/atm: warning in alloc_tx/__might_sleep
  2017-01-09 17:20 net/atm: warning in alloc_tx/__might_sleep Andrey Konovalov
@ 2017-01-10 17:35 ` Cong Wang
  2017-01-10 17:40   ` Eric Dumazet
  2017-01-11 14:37 ` Chas Williams
  1 sibling, 1 reply; 14+ messages in thread
From: Cong Wang @ 2017-01-10 17:35 UTC (permalink / raw)
  To: Andrey Konovalov
  Cc: David S. Miller, Alexey Kuznetsov, James Morris,
	Hideaki YOSHIFUJI, Patrick McHardy, netdev, LKML, Al Viro,
	Dmitry Vyukov, Kostya Serebryany, Eric Dumazet, syzkaller

[-- Attachment #1: Type: text/plain, Size: 2099 bytes --]

On Mon, Jan 9, 2017 at 9:20 AM, Andrey Konovalov <andreyknvl@google.com> 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
>  vcc_sendmsg+0x5e8/0x1010 net/atm/common.c:609
>  sock_sendmsg_nosec net/socket.c:635
>  sock_sendmsg+0xca/0x110 net/socket.c:645
>  ___sys_sendmsg+0x9d2/0xae0 net/socket.c:1985
>  __sys_sendmsg+0x138/0x320 net/socket.c:2019
>  SYSC_sendmsg net/socket.c:2030
>  SyS_sendmsg+0x2d/0x50 net/socket.c:2026
>  entry_SYSCALL_64_fastpath+0x1f/0xc2 arch/x86/entry/entry_64.S:203
> RIP: 0033:0x7fcbacfddb79
> RSP: 002b:00007ffed8b5a7b8 EFLAGS: 00000206 ORIG_RAX: 000000000000002e
> RAX: ffffffffffffffda RBX: 00007ffed8b5a950 RCX: 00007fcbacfddb79
> RDX: 000000000000c000 RSI: 0000000020002000 RDI: 0000000000000003
> RBP: 0000000000400af0 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000206 R12: 0000000000000000
> R13: 00007ffed8b5a950 R14: 0000000000000000 R15: 0000000000000000
> ---[ end trace 9edf2da84d8112da ]---
> atm:sigd_send: bad message type 0

The fix should be straight-forward. Mind to try the attached patch?

[-- Attachment #2: atm.diff --]
[-- Type: text/plain, Size: 1418 bytes --]

diff --git a/net/atm/common.c b/net/atm/common.c
index a3ca922..b7d9661 100644
--- a/net/atm/common.c
+++ b/net/atm/common.c
@@ -571,8 +571,8 @@ int vcc_recvmsg(struct socket *sock, struct msghdr *msg, size_t size,
 
 int vcc_sendmsg(struct socket *sock, struct msghdr *m, size_t size)
 {
+	DEFINE_WAIT_FUNC(wait, woken_wake_function);
 	struct sock *sk = sock->sk;
-	DEFINE_WAIT(wait);
 	struct atm_vcc *vcc;
 	struct sk_buff *skb;
 	int eff, error;
@@ -604,7 +604,7 @@ int vcc_sendmsg(struct socket *sock, struct msghdr *m, size_t size)
 	}
 
 	eff = (size+3) & ~3; /* align to word boundary */
-	prepare_to_wait(sk_sleep(sk), &wait, TASK_INTERRUPTIBLE);
+	add_wait_queue(sk_sleep(sk), &wait);
 	error = 0;
 	while (!(skb = alloc_tx(vcc, eff))) {
 		if (m->msg_flags & MSG_DONTWAIT) {
@@ -612,6 +612,7 @@ int vcc_sendmsg(struct socket *sock, struct msghdr *m, size_t size)
 			break;
 		}
 		schedule();
+		wait_woken(&wait, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
 		if (signal_pending(current)) {
 			error = -ERESTARTSYS;
 			break;
@@ -623,9 +624,8 @@ int vcc_sendmsg(struct socket *sock, struct msghdr *m, size_t size)
 			send_sig(SIGPIPE, current, 0);
 			break;
 		}
-		prepare_to_wait(sk_sleep(sk), &wait, TASK_INTERRUPTIBLE);
 	}
-	finish_wait(sk_sleep(sk), &wait);
+	remove_wait_queue(sk_sleep(sk), &wait);
 	if (error)
 		goto out;
 	skb->dev = NULL; /* for paths shared with net_device interfaces */

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: net/atm: warning in alloc_tx/__might_sleep
  2017-01-10 17:35 ` Cong Wang
@ 2017-01-10 17:40   ` Eric Dumazet
  2017-01-10 17:55     ` Cong Wang
                       ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: Eric Dumazet @ 2017-01-10 17:40 UTC (permalink / raw)
  To: Cong Wang
  Cc: Andrey Konovalov, David S. Miller, Alexey Kuznetsov,
	James Morris, Hideaki YOSHIFUJI, Patrick McHardy, netdev, LKML,
	Al Viro, Dmitry Vyukov, Kostya Serebryany, syzkaller

On Tue, Jan 10, 2017 at 9:35 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Mon, Jan 9, 2017 at 9:20 AM, Andrey Konovalov <andreyknvl@google.com> wrote:
>
> The fix should be straight-forward. Mind to try the attached patch?


You forgot to remove schedule() ?

  schedule();
+ wait_woken(&wait, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: net/atm: warning in alloc_tx/__might_sleep
  2017-01-10 17:40   ` Eric Dumazet
@ 2017-01-10 17:55     ` Cong Wang
  2017-01-10 22:47     ` Francois Romieu
  2017-01-11 10:18     ` Andrey Konovalov
  2 siblings, 0 replies; 14+ messages in thread
From: Cong Wang @ 2017-01-10 17:55 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Andrey Konovalov, David S. Miller, Alexey Kuznetsov,
	James Morris, Hideaki YOSHIFUJI, Patrick McHardy, netdev, LKML,
	Al Viro, Dmitry Vyukov, Kostya Serebryany, syzkaller

On Tue, Jan 10, 2017 at 9:40 AM, Eric Dumazet <edumazet@google.com> wrote:
> On Tue, Jan 10, 2017 at 9:35 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> On Mon, Jan 9, 2017 at 9:20 AM, Andrey Konovalov <andreyknvl@google.com> wrote:
>>
>> The fix should be straight-forward. Mind to try the attached patch?
>
>
> You forgot to remove schedule() ?
>
>   schedule();
> + wait_woken(&wait, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);

Ah, of course, never even compile it... :-/

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: net/atm: warning in alloc_tx/__might_sleep
  2017-01-10 17:40   ` Eric Dumazet
  2017-01-10 17:55     ` Cong Wang
@ 2017-01-10 22:47     ` Francois Romieu
  2017-01-11 19:11       ` Eric Dumazet
  2017-01-11 10:18     ` Andrey Konovalov
  2 siblings, 1 reply; 14+ messages in thread
From: Francois Romieu @ 2017-01-10 22:47 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Cong Wang, Andrey Konovalov, David S. Miller, Alexey Kuznetsov,
	James Morris, Hideaki YOSHIFUJI, Patrick McHardy, netdev, LKML,
	Al Viro, Dmitry Vyukov, Kostya Serebryany, syzkaller

Eric Dumazet <edumazet@google.com> :
> On Tue, Jan 10, 2017 at 9:35 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > On Mon, Jan 9, 2017 at 9:20 AM, Andrey Konovalov <andreyknvl@google.com> wrote:
> >
> > The fix should be straight-forward. Mind to try the attached patch?
> 
> 
> You forgot to remove schedule() ?

It may be clearer to split alloc_tx in two parts: only the unsleepable
"if (sk_wmem_alloc_get(sk) && !atm_may_send(vcc, size)) {" part of it
contributes to the inner "while (!(skb = alloc_tx(vcc, eff))) {" block.

See net/atm/common.c
[...]
static struct sk_buff *alloc_tx(struct atm_vcc *vcc, unsigned int size)
{
        struct sk_buff *skb;
        struct sock *sk = sk_atm(vcc);

        if (sk_wmem_alloc_get(sk) && !atm_may_send(vcc, size)) {
                pr_debug("Sorry: wmem_alloc = %d, size = %d, sndbuf = %d\n",
                         sk_wmem_alloc_get(sk), size, sk->sk_sndbuf);
                return NULL;
        }
        while (!(skb = alloc_skb(size, GFP_KERNEL)))
                schedule();
        pr_debug("%d += %d\n", sk_wmem_alloc_get(sk), skb->truesize);
        atomic_add(skb->truesize, &sk->sk_wmem_alloc);
        return skb;
}

The waiting stuff is related to vcc drain but the code makes it look as
if it were also related to skb alloc (it isn't).

It may be obvious for you but it took me a while to figure what the
code is supposed to achieve.

-- 
Ueimor

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: net/atm: warning in alloc_tx/__might_sleep
  2017-01-10 17:40   ` Eric Dumazet
  2017-01-10 17:55     ` Cong Wang
  2017-01-10 22:47     ` Francois Romieu
@ 2017-01-11 10:18     ` Andrey Konovalov
  2 siblings, 0 replies; 14+ messages in thread
From: Andrey Konovalov @ 2017-01-11 10:18 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Cong Wang, David S. Miller, Alexey Kuznetsov, James Morris,
	Hideaki YOSHIFUJI, Patrick McHardy, netdev, LKML, Al Viro,
	Dmitry Vyukov, Kostya Serebryany, syzkaller

On Tue, Jan 10, 2017 at 6:40 PM, Eric Dumazet <edumazet@google.com> wrote:
> On Tue, Jan 10, 2017 at 9:35 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> On Mon, Jan 9, 2017 at 9:20 AM, Andrey Konovalov <andreyknvl@google.com> wrote:
>>
>> The fix should be straight-forward. Mind to try the attached patch?

Hi Cong,

Your patch with schedule() removed as suggested by Eric fixes the issue.

Tested-by: Andrey Konovalov <andreyknvl@google.com>

Thanks!

>
>
> You forgot to remove schedule() ?
>
>   schedule();
> + wait_woken(&wait, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: net/atm: warning in alloc_tx/__might_sleep
  2017-01-09 17:20 net/atm: warning in alloc_tx/__might_sleep Andrey Konovalov
  2017-01-10 17:35 ` Cong Wang
@ 2017-01-11 14:37 ` Chas Williams
  2017-01-11 19:45   ` Michal Hocko
  1 sibling, 1 reply; 14+ messages in thread
From: Chas Williams @ 2017-01-11 14:37 UTC (permalink / raw)
  To: Andrey Konovalov, David S. Miller, Alexey Kuznetsov,
	James Morris, Hideaki YOSHIFUJI, Patrick McHardy, netdev, LKML,
	Al Viro
  Cc: Dmitry Vyukov, Kostya Serebryany, Eric Dumazet, syzkaller

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);

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: net/atm: warning in alloc_tx/__might_sleep
  2017-01-10 22:47     ` Francois Romieu
@ 2017-01-11 19:11       ` Eric Dumazet
  0 siblings, 0 replies; 14+ messages in thread
From: Eric Dumazet @ 2017-01-11 19:11 UTC (permalink / raw)
  To: Francois Romieu
  Cc: Eric Dumazet, Cong Wang, Andrey Konovalov, David S. Miller,
	Alexey Kuznetsov, James Morris, Hideaki YOSHIFUJI,
	Patrick McHardy, netdev, LKML, Al Viro, Dmitry Vyukov,
	Kostya Serebryany, syzkaller

On Tue, 2017-01-10 at 23:47 +0100, Francois Romieu wrote:
> Eric Dumazet <edumazet@google.com> :
> > On Tue, Jan 10, 2017 at 9:35 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > > On Mon, Jan 9, 2017 at 9:20 AM, Andrey Konovalov <andreyknvl@google.com> wrote:
> > >
> > > The fix should be straight-forward. Mind to try the attached patch?
> > 
> > 
> > You forgot to remove schedule() ?
> 
> It may be clearer to split alloc_tx in two parts: only the unsleepable
> "if (sk_wmem_alloc_get(sk) && !atm_may_send(vcc, size)) {" part of it
> contributes to the inner "while (!(skb = alloc_tx(vcc, eff))) {" block.
> 
> See net/atm/common.c
> [...]
> static struct sk_buff *alloc_tx(struct atm_vcc *vcc, unsigned int size)
> {
>         struct sk_buff *skb;
>         struct sock *sk = sk_atm(vcc);
> 
>         if (sk_wmem_alloc_get(sk) && !atm_may_send(vcc, size)) {
>                 pr_debug("Sorry: wmem_alloc = %d, size = %d, sndbuf = %d\n",
>                          sk_wmem_alloc_get(sk), size, sk->sk_sndbuf);
>                 return NULL;
>         }
>         while (!(skb = alloc_skb(size, GFP_KERNEL)))
>                 schedule();

Yeah, this code looks quite wrong anyway.

We can read it as an infinite loop in some stress conditions or memcg
constraints.


> The waiting stuff is related to vcc drain but the code makes it look as
> if it were also related to skb alloc (it isn't).
> 
> It may be obvious for you but it took me a while to figure what the
> code is supposed to achieve.
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: net/atm: warning in alloc_tx/__might_sleep
  2017-01-11 14:37 ` Chas Williams
@ 2017-01-11 19:45   ` Michal Hocko
  2017-01-11 19:46     ` Michal Hocko
  0 siblings, 1 reply; 14+ messages in thread
From: Michal Hocko @ 2017-01-11 19:45 UTC (permalink / raw)
  To: Chas Williams
  Cc: Andrey Konovalov, David S. Miller, Alexey Kuznetsov,
	James Morris, Hideaki YOSHIFUJI, Patrick McHardy, netdev, LKML,
	Al Viro, Dmitry Vyukov, Kostya Serebryany, Eric Dumazet,
	syzkaller

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!

-- 
Michal Hocko
SUSE Labs

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: net/atm: warning in alloc_tx/__might_sleep
  2017-01-11 19:45   ` Michal Hocko
@ 2017-01-11 19:46     ` Michal Hocko
  2017-01-12  4:36       ` Cong Wang
  0 siblings, 1 reply; 14+ messages in thread
From: Michal Hocko @ 2017-01-11 19:46 UTC (permalink / raw)
  To: Chas Williams
  Cc: Andrey Konovalov, David S. Miller, Alexey Kuznetsov,
	James Morris, Hideaki YOSHIFUJI, Patrick McHardy, netdev, LKML,
	Al Viro, Dmitry Vyukov, Kostya Serebryany, Eric Dumazet,
	syzkaller

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.
-- 
Michal Hocko
SUSE Labs

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: net/atm: warning in alloc_tx/__might_sleep
  2017-01-11 19:46     ` Michal Hocko
@ 2017-01-12  4:36       ` Cong Wang
  2017-01-12 10:40         ` Chas Williams
  0 siblings, 1 reply; 14+ messages in thread
From: Cong Wang @ 2017-01-12  4:36 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Chas Williams, Andrey Konovalov, David S. Miller,
	Alexey Kuznetsov, James Morris, Hideaki YOSHIFUJI,
	Patrick McHardy, netdev, LKML, Al Viro, Dmitry Vyukov,
	Kostya Serebryany, Eric Dumazet, syzkaller

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().

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: net/atm: warning in alloc_tx/__might_sleep
  2017-01-12  4:36       ` Cong Wang
@ 2017-01-12 10:40         ` Chas Williams
  2017-03-13 17:43           ` Andrey Konovalov
  0 siblings, 1 reply; 14+ messages in thread
From: Chas Williams @ 2017-01-12 10:40 UTC (permalink / raw)
  To: Cong Wang, Michal Hocko
  Cc: Andrey Konovalov, David S. Miller, Alexey Kuznetsov,
	James Morris, Hideaki YOSHIFUJI, Patrick McHardy, netdev, LKML,
	Al Viro, Dmitry Vyukov, Kostya Serebryany, Eric Dumazet,
	syzkaller

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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: net/atm: warning in alloc_tx/__might_sleep
  2017-01-12 10:40         ` Chas Williams
@ 2017-03-13 17:43           ` Andrey Konovalov
  2017-03-14 16:04             ` Chas Williams
  0 siblings, 1 reply; 14+ messages in thread
From: Andrey Konovalov @ 2017-03-13 17:43 UTC (permalink / raw)
  To: Chas Williams
  Cc: Cong Wang, Michal Hocko, David S. Miller, Alexey Kuznetsov,
	Hideaki YOSHIFUJI, Patrick McHardy, netdev, LKML, Al Viro,
	Dmitry Vyukov, Kostya Serebryany, Eric Dumazet, syzkaller

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!

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: net/atm: warning in alloc_tx/__might_sleep
  2017-03-13 17:43           ` Andrey Konovalov
@ 2017-03-14 16:04             ` Chas Williams
  0 siblings, 0 replies; 14+ messages in thread
From: Chas Williams @ 2017-03-14 16:04 UTC (permalink / raw)
  To: Andrey Konovalov
  Cc: Cong Wang, Michal Hocko, David S. Miller, Alexey Kuznetsov,
	Hideaki YOSHIFUJI, Patrick McHardy, netdev, LKML, Al Viro,
	Dmitry Vyukov, Kostya Serebryany, Eric Dumazet, syzkaller

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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2017-03-14 16:05 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-09 17:20 net/atm: warning in alloc_tx/__might_sleep Andrey Konovalov
2017-01-10 17:35 ` Cong Wang
2017-01-10 17:40   ` Eric Dumazet
2017-01-10 17:55     ` Cong Wang
2017-01-10 22:47     ` Francois Romieu
2017-01-11 19:11       ` Eric Dumazet
2017-01-11 10:18     ` Andrey Konovalov
2017-01-11 14:37 ` Chas Williams
2017-01-11 19:45   ` Michal Hocko
2017-01-11 19:46     ` Michal Hocko
2017-01-12  4:36       ` Cong Wang
2017-01-12 10:40         ` Chas Williams
2017-03-13 17:43           ` Andrey Konovalov
2017-03-14 16:04             ` Chas Williams

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).