Re: net: BUG in unix_notinflight

On Mon, Mar 6, 2017 at 11:34 PM, Cong Wang [off-list ref] wrote:

On Mon, Mar 6, 2017 at 2:40 AM, Dmitry Vyukov [off-list ref] wrote:

quoted

Now with a nice single-threaded C reproducer!

Excellent...

quoted

// autogenerated by syzkaller (http://github.com/google/syzkaller)
#define _GNU_SOURCE
#include <sys/syscall.h>
#include <sys/types.h>
#include <sys/wait.h>
#include <stddef.h>
#include <stdint.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>

void test()
{
  long r[54];
  memset(r, -1, sizeof(r));
  syscall(__NR_mmap, 0x20000000ul, 0xfff000ul, 0x3ul, 0x32ul, -1, 0);
  r[1] = syscall(__NR_socketpair, 0x1ul, 0x5ul, 0x0ul, 0x20521ff8ul);
  r[2] = *(uint32_t*)0x20521ff8;
  r[3] = *(uint32_t*)0x20521ffc;
  r[5] = syscall(__NR_open, "/dev/net/tun", 0x200000ul);
  r[6] = syscall(__NR_socketpair, 0x1ul, 0x5ul, 0x0ul,
                         0x20d85000ul, 0, 0, 0, 0, 0);
  r[7] = *(uint32_t*)0x20d85000;
  (*(uint64_t*)0x20000fc8 = (uint64_t)0x20000000);
  (*(uint32_t*)0x20000fd0 = (uint32_t)0xa);
  (*(uint64_t*)0x20000fd8 = (uint64_t)0x2005d000);
  (*(uint64_t*)0x20000fe0 = (uint64_t)0x8);
  (*(uint64_t*)0x20000fe8 = (uint64_t)0x20000ff0);
  (*(uint64_t*)0x20000ff0 = (uint64_t)0x1);
  (*(uint32_t*)0x20000ff8 = (uint32_t)0x0);
  (*(uint16_t*)0x20000000 = (uint16_t)0x1);
  memcpy((void*)0x20000002, "\x2e\x2f\x66\x69\x6c\x65\x30\x00", 8);
  (*(uint64_t*)0x2005d000 = (uint64_t)0x20784f06);
  (*(uint64_t*)0x2005d008 = (uint64_t)0x0);
  (*(uint64_t*)0x2005d010 = (uint64_t)0x209a5f78);
  (*(uint64_t*)0x2005d018 = (uint64_t)0x0);
  (*(uint64_t*)0x2005d020 = (uint64_t)0x20ec3ffc);
  (*(uint64_t*)0x2005d028 = (uint64_t)0x0);
  (*(uint64_t*)0x2005d030 = (uint64_t)0x2057e000);
  (*(uint64_t*)0x2005d038 = (uint64_t)0x0);
  (*(uint64_t*)0x2005d040 = (uint64_t)0x200c9f9d);
  (*(uint64_t*)0x2005d048 = (uint64_t)0x0);
  (*(uint64_t*)0x2005d050 = (uint64_t)0x20331000);
  (*(uint64_t*)0x2005d058 = (uint64_t)0x0);
  (*(uint64_t*)0x2005d060 = (uint64_t)0x206a1f7b);
  (*(uint64_t*)0x2005d068 = (uint64_t)0x0);
  (*(uint64_t*)0x2005d070 = (uint64_t)0x20e7f000);
  (*(uint64_t*)0x2005d078 = (uint64_t)0x0);
  (*(uint64_t*)0x20000ff0 = (uint64_t)0x18);
  (*(uint32_t*)0x20000ff8 = (uint32_t)0x1);
  (*(uint32_t*)0x20000ffc = (uint32_t)0x1);
  (*(uint32_t*)0x20001000 = r[5]);
  (*(uint32_t*)0x20001004 = r[7]);
  syscall(__NR_sendmsg, r[7], 0x20000fc8ul, 0x0ul);
  (*(uint64_t*)0x20000fc8 = (uint64_t)0x20000000);
  (*(uint32_t*)0x20000fd0 = (uint32_t)0x8);
  (*(uint64_t*)0x20000fd8 = (uint64_t)0x20026000);
  (*(uint64_t*)0x20000fe0 = (uint64_t)0x0);
  (*(uint64_t*)0x20000fe8 = (uint64_t)0x20000ff0);
  (*(uint64_t*)0x20000ff0 = (uint64_t)0x1);
  (*(uint32_t*)0x20000ff8 = (uint32_t)0x0);
  (*(uint16_t*)0x20000000 = (uint16_t)0x0);
  (*(uint8_t*)0x20000002 = (uint8_t)0x0);
  (*(uint32_t*)0x20000004 = (uint32_t)0x4e20);
  (*(uint64_t*)0x20000ff0 = (uint64_t)0x18);
  (*(uint32_t*)0x20000ff8 = (uint32_t)0x1);
  (*(uint32_t*)0x20000ffc = (uint32_t)0x1);
  (*(uint32_t*)0x20001000 = r[2]);
  syscall(__NR_sendmsg, r[3], 0x20000fc8ul, 0x0ul);
}

int main()
{
  int i, pid, status;
  for (i = 0; i < 4; i++) {
    if (fork() == 0) {
      for (;;) {
        pid = fork();
        if (pid == 0) {
          test();
          exit(0);
        }
        while (waitpid(pid, &status, __WALL) != pid) {}
      }
    }
  }
  sleep(1000000);
  return 0;
}



New report from linux-next/c0b7b2b33bd17f7155956d0338ce92615da686c9

------------[ cut here ]------------
kernel BUG at net/unix/garbage.c:149!
invalid opcode: 0000 [#1] SMP KASAN
Dumping ftrace buffer:
   (ftrace buffer empty)
Modules linked in:
CPU: 0 PID: 1806 Comm: syz-executor7 Not tainted 4.10.0-next-20170303+ #6
Hardware name: Google Google Compute Engine/Google Compute Engine,
BIOS Google 01/01/2011
task: ffff880121c64740 task.stack: ffff88012c9e8000
RIP: 0010:unix_notinflight+0x417/0x5d0 net/unix/garbage.c:149
RSP: 0018:ffff88012c9ef0f8 EFLAGS: 00010297
RAX: ffff880121c64740 RBX: 1ffff1002593de23 RCX: ffff8801c490c628
RDX: 0000000000000000 RSI: 1ffff1002593de27 RDI: ffffffff8557e504
RBP: ffff88012c9ef220 R08: 0000000000000001 R09: 0000000000000000
R10: dffffc0000000000 R11: ffffed002593de55 R12: ffff8801c490c0c0
R13: ffff88012c9ef1f8 R14: ffffffff85101620 R15: dffffc0000000000
FS:  00000000013d3940(0000) GS:ffff8801dbe00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000001fd8cd8 CR3: 00000001cce69000 CR4: 00000000001426f0
Call Trace:
 unix_detach_fds.isra.23+0xfa/0x170 net/unix/af_unix.c:1490
 unix_destruct_scm+0xf4/0x200 net/unix/af_unix.c:1499

The problem here is there is no lock protecting concurrent unix_detach_fds()
even though unix_notinflight() is already serialized, if we call
unix_notinflight()
twice on the same file pointer, we trigger this bug...

I don't know what is the right lock here to serialize it.

What exactly here needs to be protected?

1484 static void unix_detach_fds(struct scm_cookie *scm, struct sk_buff *skb)
1485 {
1486         int i;
1487
1488         scm->fp = UNIXCB(skb).fp;
1489         UNIXCB(skb).fp = NULL;
1490
1491         for (i = scm->fp->count-1; i >= 0; i--)
1492                 unix_notinflight(scm->fp->user, scm->fp->fp[i]);
1493 }

Whole unix_notinflight happens under global unix_gc_lock.

Is it that 2 threads call unix_detach_fds for the same skb, and then
call unix_notinflight for the same fd twice?