Re: net: BUG in unix_notinflight

[Dmitry Vyukov]

On Mon, Mar 6, 2017 at 11:34 PM, Cong Wang <xiyou.wangcong@xxxxxxxxx> wrote:
> On Mon, Mar 6, 2017 at 2:40 AM, Dmitry Vyukov <dvyukov@xxxxxxxxxx> wrote:
>> Now with a nice single-threaded C reproducer!
>
> Excellent...
>
>>
>> // 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?