Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1244555 > unrolled thread

GPF in shm_lock ipc

Back to article view | Back to linux.kernel

Contents

  GPF in shm_lock ipc Dmitry Vyukov <dvyukov@google.com> - 2015-10-12 12:00 +0200
    Re: GPF in shm_lock ipc Dmitry Vyukov <dvyukov@google.com> - 2015-10-12 13:50 +0200
    Re: GPF in shm_lock ipc Vlastimil Babka <vbabka@suse.cz> - 2015-10-12 13:50 +0200
    Re: GPF in shm_lock ipc "Kirill A. Shutemov" <kirill@shutemov.name> - 2015-10-12 14:30 +0200
      Re: GPF in shm_lock ipc Davidlohr Bueso <dave@stgolabs.net> - 2015-10-12 20:00 +0200
        Re: GPF in shm_lock ipc "Kirill A. Shutemov" <kirill@shutemov.name> - 2015-10-12 20:20 +0200
          Re: GPF in shm_lock ipc Davidlohr Bueso <dave@stgolabs.net> - 2015-10-12 21:00 +0200
            Re: GPF in shm_lock ipc Davidlohr Bueso <dave@stgolabs.net> - 2015-10-13 05:20 +0200
              Re: GPF in shm_lock ipc "Kirill A. Shutemov" <kirill@shutemov.name> - 2015-10-13 14:40 +0200

#1244555 — GPF in shm_lock ipc

Hello,

The following program crashes kernel:

// autogenerated by syzkaller (http://github.com/google/syzkaller)
#include <syscall.h>
#include <string.h>
#include <stdint.h>

int main()
{
        long r0 = syscall(SYS_shmget, 0x0ul, 0x2ul, 0x8ul);
        long r1 = syscall(SYS_shmat, r0, 0x20000000ul, 0x0ul);
        long r2 = syscall(SYS_mremap, 0x20000000ul, 0x1000ul,
0x3000ul, 0x3ul, 0x207f9000ul);
        long r19 = syscall(SYS_shmctl, r0, 0x0ul, 0);
        long r20 = syscall(SYS_remap_file_pages, 0x207f9000ul,
0x3000ul, 0x0ul, 0x7ul, 0x0ul);
        return 0;
}

On commit dd36d7393d6310b0c1adefb22fba79c3cf8a577c
(git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git)

------------[ cut here ]------------
WARNING: CPU: 2 PID: 2636 at ipc/shm.c:162 shm_open+0x74/0x80()
Modules linked in:
CPU: 2 PID: 2636 Comm: a.out Not tainted 4.3.0-rc3+ #37
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
 ffffffff81bcb43c ffff88081bf0bd70 ffffffff812fe8d6 0000000000000000
 ffff88081bf0bda8 ffffffff81051ff1 ffffffffffffffea ffff88081b896ca8
 ffff880819b81620 ffff8800bbaa6d00 ffff880819b81600 ffff88081bf0bdb8
Call Trace:
 [<     inline     >] __dump_stack lib/dump_stack.c:15
 [<ffffffff812fe8d6>] dump_stack+0x44/0x5e lib/dump_stack.c:50
 [<ffffffff81051ff1>] warn_slowpath_common+0x81/0xc0 kernel/panic.c:447
 [<ffffffff810520e5>] warn_slowpath_null+0x15/0x20 kernel/panic.c:480
 [<     inline     >] shm_lock ipc/shm.c:162
 [<ffffffff81295c64>] shm_open+0x74/0x80 ipc/shm.c:196
 [<ffffffff81295cbe>] shm_mmap+0x4e/0x80 ipc/shm.c:399 (discriminator 2)
 [<ffffffff81142d14>] mmap_region+0x3c4/0x5e0 mm/mmap.c:1627
 [<ffffffff81143227>] do_mmap+0x2f7/0x3d0 mm/mmap.c:1402
 [<     inline     >] do_mmap_pgoff include/linux/mm.h:1930
 [<     inline     >] SYSC_remap_file_pages mm/mmap.c:2694
 [<ffffffff811434a9>] SyS_remap_file_pages+0x179/0x240 mm/mmap.c:2641
 [<ffffffff81859a97>] entry_SYSCALL_64_fastpath+0x12/0x6a
arch/x86/entry/entry_64.S:185
---[ end trace 0873e743fc645a8c ]---
BUG: unable to handle kernel NULL pointer dereference at 000000000000003a
IP: [<ffffffff81295c25>] shm_open+0x35/0x80 ipc/shm.c:197
PGD 81a08b067 PUD 81a01b067 PMD 0
Oops: 0002 [#1] SMP
Modules linked in:
CPU: 2 PID: 2636 Comm: a.out Tainted: G        W       4.3.0-rc3+ #37
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
task: ffff880819b26800 ti: ffff88081bf08000 task.ti: ffff88081bf08000
RIP: 0010:[<ffffffff81295c25>]  [<ffffffff81295c25>] shm_open+0x35/0x80
RSP: 0018:ffff88081bf0bdc8  EFLAGS: 00010296
RAX: 00000000560d1d13 RBX: ffffffffffffffea RCX: ffffffff8151e710
RDX: 0000000000000001 RSI: 0000000000000000 RDI: 0000000000000009
RBP: ffff88081bf0bdd0 R08: 000000000000000a R09: 0000000000000001
R10: 0000000000000000 R11: 00000000000001bc R12: ffff88081b896ca8
R13: ffff880819b81620 R14: ffff8800bbaa6d00 R15: ffff880819b81600
FS:  00007fbfd966f700(0000) GS:ffff88083fd00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 000000000000003a CR3: 0000000819a97000 CR4: 00000000000006e0
Stack:
 ffff880819b81200 ffff88081bf0bdf8 ffffffff81295cbe 0000000019b81620
 ffff880819b81628 00000000207f9000 ffff88081bf0be80 ffffffff81142d14
 ffff8800bbaa6d00 0000000000000007 0000000000000000 0000000000000007
Call Trace:
 [<ffffffff81295cbe>] shm_mmap+0x4e/0x80 ipc/shm.c:399 (discriminator 2)
 [<ffffffff81142d14>] mmap_region+0x3c4/0x5e0 mm/mmap.c:1627
 [<ffffffff81143227>] do_mmap+0x2f7/0x3d0 mm/mmap.c:1402
 [<     inline     >] do_mmap_pgoff include/linux/mm.h:1930
 [<     inline     >] SYSC_remap_file_pages mm/mmap.c:2694
 [<ffffffff811434a9>] SyS_remap_file_pages+0x179/0x240 mm/mmap.c:2641
 [<ffffffff81859a97>] entry_SYSCALL_64_fastpath+0x12/0x6a
arch/x86/entry/entry_64.S:185
Code: 00 00 48 8b 80 d0 00 00 00 48 8b 50 08 8b 30 48 8d ba c8 00 00
00 e8 6b b1 ff ff 48 3d 00 f0 ff ff 48 89 c3 77 33 e8 5b 6b e1 ff <48>
89 43 50 65 48 8b 04 25 c0 ad 00 00 48 8b 80 40 04 00 00 48
RIP  [<ffffffff81295c25>] shm_open+0x35/0x80 ipc/shm.c:197
 RSP <ffff88081bf0bdc8>
CR2: 000000000000003a
---[ end trace 0873e743fc645a8d ]---

Found with syzkaller fuzzer.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1244616

On Mon, Oct 12, 2015 at 1:41 PM, Vlastimil Babka <vbabka@suse.cz> wrote:
> On 10/12/2015 11:55 AM, Dmitry Vyukov wrote:
>>
>> Hello,
>>
>> The following program crashes kernel:
>>
>> // autogenerated by syzkaller (http://github.com/google/syzkaller)
>> #include <syscall.h>
>> #include <string.h>
>> #include <stdint.h>
>>
>> int main()
>> {
>>          long r0 = syscall(SYS_shmget, 0x0ul, 0x2ul, 0x8ul);
>>          long r1 = syscall(SYS_shmat, r0, 0x20000000ul, 0x0ul);
>>          long r2 = syscall(SYS_mremap, 0x20000000ul, 0x1000ul,
>> 0x3000ul, 0x3ul, 0x207f9000ul);
>>          long r19 = syscall(SYS_shmctl, r0, 0x0ul, 0);
>>          long r20 = syscall(SYS_remap_file_pages, 0x207f9000ul,
>> 0x3000ul, 0x0ul, 0x7ul, 0x0ul);
>>          return 0;
>> }
>>
>> On commit dd36d7393d6310b0c1adefb22fba79c3cf8a577c
>> (git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git)
>>
>> ------------[ cut here ]------------
>> WARNING: CPU: 2 PID: 2636 at ipc/shm.c:162 shm_open+0x74/0x80()
>> Modules linked in:
>> CPU: 2 PID: 2636 Comm: a.out Not tainted 4.3.0-rc3+ #37
>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs
>> 01/01/2011
>>   ffffffff81bcb43c ffff88081bf0bd70 ffffffff812fe8d6 0000000000000000
>>   ffff88081bf0bda8 ffffffff81051ff1 ffffffffffffffea ffff88081b896ca8
>>   ffff880819b81620 ffff8800bbaa6d00 ffff880819b81600 ffff88081bf0bdb8
>> Call Trace:
>>   [<     inline     >] __dump_stack lib/dump_stack.c:15
>>   [<ffffffff812fe8d6>] dump_stack+0x44/0x5e lib/dump_stack.c:50
>>   [<ffffffff81051ff1>] warn_slowpath_common+0x81/0xc0 kernel/panic.c:447
>>   [<ffffffff810520e5>] warn_slowpath_null+0x15/0x20 kernel/panic.c:480
>>   [<     inline     >] shm_lock ipc/shm.c:162
>>   [<ffffffff81295c64>] shm_open+0x74/0x80 ipc/shm.c:196
>>   [<ffffffff81295cbe>] shm_mmap+0x4e/0x80 ipc/shm.c:399 (discriminator 2)
>>   [<ffffffff81142d14>] mmap_region+0x3c4/0x5e0 mm/mmap.c:1627
>>   [<ffffffff81143227>] do_mmap+0x2f7/0x3d0 mm/mmap.c:1402
>>   [<     inline     >] do_mmap_pgoff include/linux/mm.h:1930
>>   [<     inline     >] SYSC_remap_file_pages mm/mmap.c:2694
>
>
> Hmm what kind of stack unwinder catches inlines? Some external patch, based
> on debuginfo?

We use the following script to symbolize kernel stack traces. It adds
file:line info and inlined frames.
https://github.com/google/sanitizers/blob/master/address-sanitizer/tools/kasan_symbolize.py
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1244618

On 10/12/2015 11:55 AM, Dmitry Vyukov wrote:
> Hello,
>
> The following program crashes kernel:
>
> // autogenerated by syzkaller (http://github.com/google/syzkaller)
> #include <syscall.h>
> #include <string.h>
> #include <stdint.h>
>
> int main()
> {
>          long r0 = syscall(SYS_shmget, 0x0ul, 0x2ul, 0x8ul);
>          long r1 = syscall(SYS_shmat, r0, 0x20000000ul, 0x0ul);
>          long r2 = syscall(SYS_mremap, 0x20000000ul, 0x1000ul,
> 0x3000ul, 0x3ul, 0x207f9000ul);
>          long r19 = syscall(SYS_shmctl, r0, 0x0ul, 0);
>          long r20 = syscall(SYS_remap_file_pages, 0x207f9000ul,
> 0x3000ul, 0x0ul, 0x7ul, 0x0ul);
>          return 0;
> }
>
> On commit dd36d7393d6310b0c1adefb22fba79c3cf8a577c
> (git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git)
>
> ------------[ cut here ]------------
> WARNING: CPU: 2 PID: 2636 at ipc/shm.c:162 shm_open+0x74/0x80()
> Modules linked in:
> CPU: 2 PID: 2636 Comm: a.out Not tainted 4.3.0-rc3+ #37
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
>   ffffffff81bcb43c ffff88081bf0bd70 ffffffff812fe8d6 0000000000000000
>   ffff88081bf0bda8 ffffffff81051ff1 ffffffffffffffea ffff88081b896ca8
>   ffff880819b81620 ffff8800bbaa6d00 ffff880819b81600 ffff88081bf0bdb8
> Call Trace:
>   [<     inline     >] __dump_stack lib/dump_stack.c:15
>   [<ffffffff812fe8d6>] dump_stack+0x44/0x5e lib/dump_stack.c:50
>   [<ffffffff81051ff1>] warn_slowpath_common+0x81/0xc0 kernel/panic.c:447
>   [<ffffffff810520e5>] warn_slowpath_null+0x15/0x20 kernel/panic.c:480
>   [<     inline     >] shm_lock ipc/shm.c:162
>   [<ffffffff81295c64>] shm_open+0x74/0x80 ipc/shm.c:196
>   [<ffffffff81295cbe>] shm_mmap+0x4e/0x80 ipc/shm.c:399 (discriminator 2)
>   [<ffffffff81142d14>] mmap_region+0x3c4/0x5e0 mm/mmap.c:1627
>   [<ffffffff81143227>] do_mmap+0x2f7/0x3d0 mm/mmap.c:1402
>   [<     inline     >] do_mmap_pgoff include/linux/mm.h:1930
>   [<     inline     >] SYSC_remap_file_pages mm/mmap.c:2694

Hmm what kind of stack unwinder catches inlines? Some external patch, 
based on debuginfo?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1244644

On Mon, Oct 12, 2015 at 11:55:44AM +0200, Dmitry Vyukov wrote:
> Hello,
> 
> The following program crashes kernel:
> 
> // autogenerated by syzkaller (http://github.com/google/syzkaller)
> #include <syscall.h>
> #include <string.h>
> #include <stdint.h>
> 
> int main()
> {
>         long r0 = syscall(SYS_shmget, 0x0ul, 0x2ul, 0x8ul);
>         long r1 = syscall(SYS_shmat, r0, 0x20000000ul, 0x0ul);
>         long r2 = syscall(SYS_mremap, 0x20000000ul, 0x1000ul,
> 0x3000ul, 0x3ul, 0x207f9000ul);
>         long r19 = syscall(SYS_shmctl, r0, 0x0ul, 0);
>         long r20 = syscall(SYS_remap_file_pages, 0x207f9000ul,
> 0x3000ul, 0x0ul, 0x7ul, 0x0ul);
>         return 0;
> }

Here's slightly simplified and more human readable reproducer:

#define _GNU_SOURCE
#include <stdlib.h>
#include <sys/ipc.h>
#include <sys/mman.h>
#include <sys/shm.h>

#define PAGE_SIZE 4096

int main()
{
	int id;
	void *p;

	id = shmget(IPC_PRIVATE, 3 * PAGE_SIZE, 0);
	p = shmat(id, NULL, 0);
	shmctl(id, IPC_RMID, NULL);
	remap_file_pages(p, 3 * PAGE_SIZE, 0, 7, 0);

        return 0;
}

> 
> On commit dd36d7393d6310b0c1adefb22fba79c3cf8a577c
> (git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git)
> 
> ------------[ cut here ]------------
> WARNING: CPU: 2 PID: 2636 at ipc/shm.c:162 shm_open+0x74/0x80()
> Modules linked in:
> CPU: 2 PID: 2636 Comm: a.out Not tainted 4.3.0-rc3+ #37
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
>  ffffffff81bcb43c ffff88081bf0bd70 ffffffff812fe8d6 0000000000000000
>  ffff88081bf0bda8 ffffffff81051ff1 ffffffffffffffea ffff88081b896ca8
>  ffff880819b81620 ffff8800bbaa6d00 ffff880819b81600 ffff88081bf0bdb8
> Call Trace:
>  [<     inline     >] __dump_stack lib/dump_stack.c:15
>  [<ffffffff812fe8d6>] dump_stack+0x44/0x5e lib/dump_stack.c:50
>  [<ffffffff81051ff1>] warn_slowpath_common+0x81/0xc0 kernel/panic.c:447
>  [<ffffffff810520e5>] warn_slowpath_null+0x15/0x20 kernel/panic.c:480
>  [<     inline     >] shm_lock ipc/shm.c:162
>  [<ffffffff81295c64>] shm_open+0x74/0x80 ipc/shm.c:196
>  [<ffffffff81295cbe>] shm_mmap+0x4e/0x80 ipc/shm.c:399 (discriminator 2)
>  [<ffffffff81142d14>] mmap_region+0x3c4/0x5e0 mm/mmap.c:1627
>  [<ffffffff81143227>] do_mmap+0x2f7/0x3d0 mm/mmap.c:1402
>  [<     inline     >] do_mmap_pgoff include/linux/mm.h:1930
>  [<     inline     >] SYSC_remap_file_pages mm/mmap.c:2694
>  [<ffffffff811434a9>] SyS_remap_file_pages+0x179/0x240 mm/mmap.c:2641
>  [<ffffffff81859a97>] entry_SYSCALL_64_fastpath+0x12/0x6a
> arch/x86/entry/entry_64.S:185
> ---[ end trace 0873e743fc645a8c ]---

Okay. The problem is that SysV IPC SHM doesn't expect the memory region to
be mmap'ed after IPC_RMID, but remap_file_pages() manages to create new
VMA using existing one.

I'm not sure what the right way to fix it. The SysV SHM VMA is pretty
normal from mm POV (no special flags, etc.) and it meats remap_file_pages
criteria (shared mapping). Every fix I can think of on mm side is ugly.

Probably better to teach shm_mmap() to fall off gracefully in case of
non-existing shmid? I'm not familiar with IPC code.
Could anyone look into it?

-- 
 Kirill A. Shutemov
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1244968

On Mon, 12 Oct 2015, Kirill A. Shutemov wrote:

>On Mon, Oct 12, 2015 at 11:55:44AM +0200, Dmitry Vyukov wrote:
>Here's slightly simplified and more human readable reproducer:
>
>#define _GNU_SOURCE
>#include <stdlib.h>
>#include <sys/ipc.h>
>#include <sys/mman.h>
>#include <sys/shm.h>
>
>#define PAGE_SIZE 4096
>
>int main()
>{
>	int id;
>	void *p;
>
>	id = shmget(IPC_PRIVATE, 3 * PAGE_SIZE, 0);
>	p = shmat(id, NULL, 0);
>	shmctl(id, IPC_RMID, NULL);
>	remap_file_pages(p, 3 * PAGE_SIZE, 0, 7, 0);
>
>        return 0;
>}

Thanks!

>>
>> On commit dd36d7393d6310b0c1adefb22fba79c3cf8a577c
>> (git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git)
>>
>> ------------[ cut here ]------------
>> WARNING: CPU: 2 PID: 2636 at ipc/shm.c:162 shm_open+0x74/0x80()
>> Modules linked in:
>> CPU: 2 PID: 2636 Comm: a.out Not tainted 4.3.0-rc3+ #37
>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
>>  ffffffff81bcb43c ffff88081bf0bd70 ffffffff812fe8d6 0000000000000000
>>  ffff88081bf0bda8 ffffffff81051ff1 ffffffffffffffea ffff88081b896ca8
>>  ffff880819b81620 ffff8800bbaa6d00 ffff880819b81600 ffff88081bf0bdb8
>> Call Trace:
>>  [<     inline     >] __dump_stack lib/dump_stack.c:15
>>  [<ffffffff812fe8d6>] dump_stack+0x44/0x5e lib/dump_stack.c:50
>>  [<ffffffff81051ff1>] warn_slowpath_common+0x81/0xc0 kernel/panic.c:447
>>  [<ffffffff810520e5>] warn_slowpath_null+0x15/0x20 kernel/panic.c:480
>>  [<     inline     >] shm_lock ipc/shm.c:162
>>  [<ffffffff81295c64>] shm_open+0x74/0x80 ipc/shm.c:196
>>  [<ffffffff81295cbe>] shm_mmap+0x4e/0x80 ipc/shm.c:399 (discriminator 2)
>>  [<ffffffff81142d14>] mmap_region+0x3c4/0x5e0 mm/mmap.c:1627
>>  [<ffffffff81143227>] do_mmap+0x2f7/0x3d0 mm/mmap.c:1402
>>  [<     inline     >] do_mmap_pgoff include/linux/mm.h:1930
>>  [<     inline     >] SYSC_remap_file_pages mm/mmap.c:2694
>>  [<ffffffff811434a9>] SyS_remap_file_pages+0x179/0x240 mm/mmap.c:2641
>>  [<ffffffff81859a97>] entry_SYSCALL_64_fastpath+0x12/0x6a
>> arch/x86/entry/entry_64.S:185
>> ---[ end trace 0873e743fc645a8c ]---
>
>Okay. The problem is that SysV IPC SHM doesn't expect the memory region to
>be mmap'ed after IPC_RMID, but remap_file_pages() manages to create new
>VMA using existing one.

Indeed, naughty users should not be mapping/(re)attaching after IPC_RMID.
This is common to all things ipc, not only to shm. And while Linux nowadays
does enforce that nothing touch a segment marked for deletion[1], we have
contradictory scenarios where the resource is only freed once the last attached
process exits. 

[1] https://lkml.org/lkml/2015/10/12/483

So this warning used to in fact be a full BUG_ON, but ultimately the ipc
subsystem acknowledges that this situation is possible but fully blames the
user responsible, and therefore we only warn about bogus usage.

>I'm not sure what the right way to fix it. The SysV SHM VMA is pretty
>normal from mm POV (no special flags, etc.) and it meats remap_file_pages
>criteria (shared mapping). Every fix I can think of on mm side is ugly.
>
>Probably better to teach shm_mmap() to fall off gracefully in case of
>non-existing shmid? I'm not familiar with IPC code.
>Could anyone look into it?

Yeah, this was my approach as well. Very little tested other than it solves
the above warning. Basically we don't want to be doing mmap if the segment
was deleted, thus return a corresponding error instead of triggering the
same error later on after mmaping, via shm_open(). I still need to think
a bit more about this, but seems legit if we don't hurt userspace while
at it (at least the idea, not considering any overhead in doing the idr
lookup). Thoughts?

Thanks,
Davidlohr

diff --git a/ipc/shm.c b/ipc/shm.c
index 4178727..9615f19 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -385,9 +385,25 @@ static struct mempolicy *shm_get_policy(struct vm_area_struct *vma,
  
  static int shm_mmap(struct file *file, struct vm_area_struct *vma)
  {
-	struct shm_file_data *sfd = shm_file_data(file);
+	struct file *vma_file = vma->vm_file;
+	struct shm_file_data *sfd = shm_file_data(vma_file);
+	struct ipc_ids *ids = &shm_ids(sfd->ns);
+	struct kern_ipc_perm *shp;
  	int ret;
  
+	rcu_read_lock();
+	shp = ipc_obtain_object_check(ids, sfd->id);
+	if (IS_ERR(shp)) {
+		ret = -EINVAL;
+		goto err;
+	}
+
+	if (!ipc_valid_object(shp)) {
+		ret = -EIDRM;
+		goto err;
+	}
+	rcu_read_unlock();
+
  	ret = sfd->file->f_op->mmap(sfd->file, vma);
  	if (ret != 0)
  		return ret;
@@ -399,6 +415,9 @@ static int shm_mmap(struct file *file, struct vm_area_struct *vma)
  	shm_open(vma);
  
  	return ret;
+err:
+	rcu_read_unlock();
+	return ret;
  }
  
  static int shm_release(struct inode *ino, struct file *file)






--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1244979

On Mon, Oct 12, 2015 at 10:49:45AM -0700, Davidlohr Bueso wrote:
> On Mon, 12 Oct 2015, Kirill A. Shutemov wrote:
> 
> >On Mon, Oct 12, 2015 at 11:55:44AM +0200, Dmitry Vyukov wrote:
> >Here's slightly simplified and more human readable reproducer:
> >
> >#define _GNU_SOURCE
> >#include <stdlib.h>
> >#include <sys/ipc.h>
> >#include <sys/mman.h>
> >#include <sys/shm.h>
> >
> >#define PAGE_SIZE 4096
> >
> >int main()
> >{
> >	int id;
> >	void *p;
> >
> >	id = shmget(IPC_PRIVATE, 3 * PAGE_SIZE, 0);
> >	p = shmat(id, NULL, 0);
> >	shmctl(id, IPC_RMID, NULL);
> >	remap_file_pages(p, 3 * PAGE_SIZE, 0, 7, 0);
> >
> >       return 0;
> >}
> 
> Thanks!
> 
> >>
> >>On commit dd36d7393d6310b0c1adefb22fba79c3cf8a577c
> >>(git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git)
> >>
> >>------------[ cut here ]------------
> >>WARNING: CPU: 2 PID: 2636 at ipc/shm.c:162 shm_open+0x74/0x80()
> >>Modules linked in:
> >>CPU: 2 PID: 2636 Comm: a.out Not tainted 4.3.0-rc3+ #37
> >>Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> >> ffffffff81bcb43c ffff88081bf0bd70 ffffffff812fe8d6 0000000000000000
> >> ffff88081bf0bda8 ffffffff81051ff1 ffffffffffffffea ffff88081b896ca8
> >> ffff880819b81620 ffff8800bbaa6d00 ffff880819b81600 ffff88081bf0bdb8
> >>Call Trace:
> >> [<     inline     >] __dump_stack lib/dump_stack.c:15
> >> [<ffffffff812fe8d6>] dump_stack+0x44/0x5e lib/dump_stack.c:50
> >> [<ffffffff81051ff1>] warn_slowpath_common+0x81/0xc0 kernel/panic.c:447
> >> [<ffffffff810520e5>] warn_slowpath_null+0x15/0x20 kernel/panic.c:480
> >> [<     inline     >] shm_lock ipc/shm.c:162
> >> [<ffffffff81295c64>] shm_open+0x74/0x80 ipc/shm.c:196
> >> [<ffffffff81295cbe>] shm_mmap+0x4e/0x80 ipc/shm.c:399 (discriminator 2)
> >> [<ffffffff81142d14>] mmap_region+0x3c4/0x5e0 mm/mmap.c:1627
> >> [<ffffffff81143227>] do_mmap+0x2f7/0x3d0 mm/mmap.c:1402
> >> [<     inline     >] do_mmap_pgoff include/linux/mm.h:1930
> >> [<     inline     >] SYSC_remap_file_pages mm/mmap.c:2694
> >> [<ffffffff811434a9>] SyS_remap_file_pages+0x179/0x240 mm/mmap.c:2641
> >> [<ffffffff81859a97>] entry_SYSCALL_64_fastpath+0x12/0x6a
> >>arch/x86/entry/entry_64.S:185
> >>---[ end trace 0873e743fc645a8c ]---
> >
> >Okay. The problem is that SysV IPC SHM doesn't expect the memory region to
> >be mmap'ed after IPC_RMID, but remap_file_pages() manages to create new
> >VMA using existing one.
> 
> Indeed, naughty users should not be mapping/(re)attaching after IPC_RMID.
> This is common to all things ipc, not only to shm. And while Linux nowadays
> does enforce that nothing touch a segment marked for deletion[1], we have
> contradictory scenarios where the resource is only freed once the last attached
> process exits.
> 
> [1] https://lkml.org/lkml/2015/10/12/483
> 
> So this warning used to in fact be a full BUG_ON, but ultimately the ipc
> subsystem acknowledges that this situation is possible but fully blames the
> user responsible, and therefore we only warn about bogus usage.
> 
> >I'm not sure what the right way to fix it. The SysV SHM VMA is pretty
> >normal from mm POV (no special flags, etc.) and it meats remap_file_pages
> >criteria (shared mapping). Every fix I can think of on mm side is ugly.
> >
> >Probably better to teach shm_mmap() to fall off gracefully in case of
> >non-existing shmid? I'm not familiar with IPC code.
> >Could anyone look into it?
> 
> Yeah, this was my approach as well. Very little tested other than it solves
> the above warning. Basically we don't want to be doing mmap if the segment
> was deleted, thus return a corresponding error instead of triggering the
> same error later on after mmaping, via shm_open(). I still need to think
> a bit more about this, but seems legit if we don't hurt userspace while
> at it (at least the idea, not considering any overhead in doing the idr
> lookup). Thoughts?
> 
> Thanks,
> Davidlohr
> 
> diff --git a/ipc/shm.c b/ipc/shm.c
> index 4178727..9615f19 100644
> --- a/ipc/shm.c
> +++ b/ipc/shm.c
> @@ -385,9 +385,25 @@ static struct mempolicy *shm_get_policy(struct vm_area_struct *vma,
>  static int shm_mmap(struct file *file, struct vm_area_struct *vma)
>  {
> -	struct shm_file_data *sfd = shm_file_data(file);
> +	struct file *vma_file = vma->vm_file;
> +	struct shm_file_data *sfd = shm_file_data(vma_file);
> +	struct ipc_ids *ids = &shm_ids(sfd->ns);
> +	struct kern_ipc_perm *shp;
>  	int ret;
> +	rcu_read_lock();
> +	shp = ipc_obtain_object_check(ids, sfd->id);
> +	if (IS_ERR(shp)) {
> +		ret = -EINVAL;
> +		goto err;
> +	}
> +
> +	if (!ipc_valid_object(shp)) {
> +		ret = -EIDRM;
> +		goto err;
> +	}
> +	rcu_read_unlock();
> +

Hm. Isn't it racy? What prevents IPC_RMID from happening after this point?
Shouldn't we bump shm_nattch here? Or some other refcount?


>  	ret = sfd->file->f_op->mmap(sfd->file, vma);
>  	if (ret != 0)
>  		return ret;
> @@ -399,6 +415,9 @@ static int shm_mmap(struct file *file, struct vm_area_struct *vma)
>  	shm_open(vma);
>  	return ret;
> +err:
> +	rcu_read_unlock();
> +	return ret;
>  }
>  static int shm_release(struct inode *ino, struct file *file)
> 
> 
> 
> 
> 
> 

-- 
 Kirill A. Shutemov
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1245035

On Mon, 12 Oct 2015, Kirill A. Shutemov wrote:

>On Mon, Oct 12, 2015 at 10:49:45AM -0700, Davidlohr Bueso wrote:
>> diff --git a/ipc/shm.c b/ipc/shm.c
>> index 4178727..9615f19 100644
>> --- a/ipc/shm.c
>> +++ b/ipc/shm.c
>> @@ -385,9 +385,25 @@ static struct mempolicy *shm_get_policy(struct vm_area_struct *vma,
>>  static int shm_mmap(struct file *file, struct vm_area_struct *vma)
>>  {
>> -	struct shm_file_data *sfd = shm_file_data(file);
>> +	struct file *vma_file = vma->vm_file;
>> +	struct shm_file_data *sfd = shm_file_data(vma_file);
>> +	struct ipc_ids *ids = &shm_ids(sfd->ns);
>> +	struct kern_ipc_perm *shp;
>>  	int ret;
>> +	rcu_read_lock();
>> +	shp = ipc_obtain_object_check(ids, sfd->id);
>> +	if (IS_ERR(shp)) {
>> +		ret = -EINVAL;
>> +		goto err;
>> +	}
>> +
>> +	if (!ipc_valid_object(shp)) {
>> +		ret = -EIDRM;
>> +		goto err;
>> +	}
>> +	rcu_read_unlock();
>> +
>
>Hm. Isn't it racy? What prevents IPC_RMID from happening after this point?

Nothing, but that is later caught by shm_open() doing similar checks. We
basically end up doing a check between ->mmap() calls, which is fair imho.
Note that this can occur anywhere in ipc as IPC_RMID is a user request/cmd,
and we try to respect it -- thus you can argue this race anywhere, which is
why we have EIDRM/EINVL. Ultimately the user should not be doing such hacks
_anyway_. So I'm not really concerned about it.

Another similar alternative would be perhaps to make shm_lock() return an
error, and thus propagate that error to mmap return. That way we would have
a silent way out of the warning scenario (afterward we cannot race as we
hold the ipc object lock). However, the users would now have to take this
into account...

      [validity check lockless]
      ->mmap()
      [validity check lock]

>Shouldn't we bump shm_nattch here? Or some other refcount?

At least not shm_nattach, as that would acknowledge a new attachment after
a valid IPC_RMID. But the problem is also with how we check for marked for
deletion segments -- ipc_valid_object() checking the deleted flag. As such,
we always rely on explicitly checking against the deleted flag.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1245310

On Mon, 12 Oct 2015, Bueso wrote:

>On Mon, 12 Oct 2015, Kirill A. Shutemov wrote:
>
>>On Mon, Oct 12, 2015 at 10:49:45AM -0700, Davidlohr Bueso wrote:
>>>diff --git a/ipc/shm.c b/ipc/shm.c
>>>index 4178727..9615f19 100644
>>>--- a/ipc/shm.c
>>>+++ b/ipc/shm.c
>>>@@ -385,9 +385,25 @@ static struct mempolicy *shm_get_policy(struct vm_area_struct *vma,
>>> static int shm_mmap(struct file *file, struct vm_area_struct *vma)
>>> {
>>>-	struct shm_file_data *sfd = shm_file_data(file);
>>>+	struct file *vma_file = vma->vm_file;
>>>+	struct shm_file_data *sfd = shm_file_data(vma_file);
>>>+	struct ipc_ids *ids = &shm_ids(sfd->ns);
>>>+	struct kern_ipc_perm *shp;
>>> 	int ret;
>>>+	rcu_read_lock();
>>>+	shp = ipc_obtain_object_check(ids, sfd->id);
>>>+	if (IS_ERR(shp)) {
>>>+		ret = -EINVAL;
>>>+		goto err;
>>>+	}
>>>+
>>>+	if (!ipc_valid_object(shp)) {
>>>+		ret = -EIDRM;
>>>+		goto err;
>>>+	}
>>>+	rcu_read_unlock();
>>>+
>>
>>Hm. Isn't it racy? What prevents IPC_RMID from happening after this point?
>
>Nothing, but that is later caught by shm_open() doing similar checks. We
>basically end up doing a check between ->mmap() calls, which is fair imho.
>Note that this can occur anywhere in ipc as IPC_RMID is a user request/cmd,
>and we try to respect it -- thus you can argue this race anywhere, which is
>why we have EIDRM/EINVL. Ultimately the user should not be doing such hacks
>_anyway_. So I'm not really concerned about it.
>
>Another similar alternative would be perhaps to make shm_lock() return an
>error, and thus propagate that error to mmap return. That way we would have
>a silent way out of the warning scenario (afterward we cannot race as we
>hold the ipc object lock). However, the users would now have to take this
>into account...
>
>     [validity check lockless]
>     ->mmap()
>     [validity check lock]

Something like this, maybe. Although I could easily be missing things...
I've tested it enough to see Dimitry's testcase handled ok, and put it
through ltp. Also adding Manfred to the Cc, who always catches my idiotic
mistakes.

8<---------------------------------------------------------------------
From: Davidlohr Bueso <dave@stgolabs.net>
Date: Mon, 12 Oct 2015 19:38:34 -0700
Subject: [PATCH] ipc/shm: fix handling of (re)attaching to a deleted segment

There are currently two issues when dealing with segments that are
marked for deletion:

(i) With d0edd8528362 (ipc: convert invalid scenarios to use WARN_ON)
we relaxed the system-wide impact of using a deleted segment. However,
we can now perfectly well trigger the warning and then deference a nil
pointer -- where shp does not exist.

(ii) As of a399b29dfbaa (ipc,shm: fix shm_file deletion races) we
forbid attaching/mapping a previously deleted segment; a feature once
unique to Linux, but removed[1] as a side effect of lockless ipc object
lookups and security checks. Similarly, Dmitry Vyukov reported[2] a
simple test case that creates a new vma for a previously deleted
segment, triggering the WARN_ON mentioned in (i).

This patch tries to address (i) by moving the shp error check out
of shm_lock() and handled by the caller instead. The benefit of this
is that it allows better handling out of situations where we end up
returning ERMID or EINVAL. Specifically, there are three callers
of shm_lock which we must look into:

  - open/close -- which we ensure to never do any operations on
                  the pairs, thus becoming no-ops if found a prev
		 IPC_RMID.

  - loosing the reference of nattch upon shmat(2) -- not feasible.

In addition, the common WARN_ON call is technically removed, but
we add a new one for the bogus shmat(2) case, which is definitely
unacceptable to race with RMID if nattch is bumped up.

To address (ii), a new shm_check_vma_validity() helper is added
(for lack of a better name), which attempts to detect early on
any races with RMID, before doing the full ->mmap. There is still
a window between the callback and the shm_open call where we can
race with IPC_RMID. If this is the case, it is handled by the next
shm_lock().

shm_mmap:
     [shm validity checks lockless]
     ->mmap()
     [shm validity checks lock] <-- at this point there after there
                                    is no race as we hold the ipc
                                    object lock.

[1] https://lkml.org/lkml/2015/10/12/483
[2] https://lkml.org/lkml/2015/10/12/284

Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
---
  ipc/shm.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
  1 file changed, 73 insertions(+), 5 deletions(-)

diff --git a/ipc/shm.c b/ipc/shm.c
index 4178727..47a7a67 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -156,11 +156,10 @@ static inline struct shmid_kernel *shm_lock(struct ipc_namespace *ns, int id)
  	struct kern_ipc_perm *ipcp = ipc_lock(&shm_ids(ns), id);
  
  	/*
-	 * We raced in the idr lookup or with shm_destroy().  Either way, the
-	 * ID is busted.
+	 * Callers of shm_lock() must validate the status of the returned
+	 * ipc object pointer (as returned by ipc_lock()), and error out as
+	 * appropriate.
  	 */
-	WARN_ON(IS_ERR(ipcp));
-
  	return container_of(ipcp, struct shmid_kernel, shm_perm);
  }
  
@@ -194,6 +193,15 @@ static void shm_open(struct vm_area_struct *vma)
  	struct shmid_kernel *shp;
  
  	shp = shm_lock(sfd->ns, sfd->id);
+	/*
+	 * We raced in the idr lookup or with shm_destroy().
+	 * Either way, the ID is busted. In the same scenario,
+	 * but for the close counter-part, the nattch counter
+	 * is never decreased, thus we can safely return.
+	 */
+	if (IS_ERR(shp))
+		return; /* no-op */
+
  	shp->shm_atim = get_seconds();
  	shp->shm_lprid = task_tgid_vnr(current);
  	shp->shm_nattch++;
@@ -218,6 +226,7 @@ static void shm_destroy(struct ipc_namespace *ns, struct shmid_kernel *shp)
  	ns->shm_tot -= (shp->shm_segsz + PAGE_SIZE - 1) >> PAGE_SHIFT;
  	shm_rmid(ns, shp);
  	shm_unlock(shp);
+
  	if (!is_file_hugepages(shm_file))
  		shmem_lock(shm_file, 0, shp->mlock_user);
  	else if (shp->mlock_user)
@@ -258,8 +267,17 @@ static void shm_close(struct vm_area_struct *vma)
  	struct ipc_namespace *ns = sfd->ns;
  
  	down_write(&shm_ids(ns).rwsem);
-	/* remove from the list of attaches of the shm segment */
  	shp = shm_lock(ns, sfd->id);
+	/*
+	 * We raced in the idr lookup or with shm_destroy().
+	 * Either way, the ID is busted. In the same scenario,
+	 * but for the open counter-part, the nattch counter
+	 * is never increased, thus we can safely return.
+	 */
+	if (IS_ERR(shp))
+		goto done; /* no-op */
+
+	/* Remove from the list of attaches of the shm segment */
  	shp->shm_lprid = task_tgid_vnr(current);
  	shp->shm_dtim = get_seconds();
  	shp->shm_nattch--;
@@ -267,6 +285,7 @@ static void shm_close(struct vm_area_struct *vma)
  		shm_destroy(ns, shp);
  	else
  		shm_unlock(shp);
+done:
  	up_write(&shm_ids(ns).rwsem);
  }
  
@@ -383,14 +402,50 @@ static struct mempolicy *shm_get_policy(struct vm_area_struct *vma,
  }
  #endif
  
+static inline int shm_check_vma_validity(struct vm_area_struct *vma)
+{
+	struct file *file = vma->vm_file;
+	struct shm_file_data *sfd = shm_file_data(file);
+	struct ipc_ids *ids = &shm_ids(sfd->ns);
+	struct kern_ipc_perm *shp;
+	int ret = 0;
+
+	rcu_read_lock();
+	shp = ipc_obtain_object_idr(ids, sfd->id);
+	if (IS_ERR(shp)) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	if (!ipc_valid_object(shp)) {
+		ret = -EIDRM;
+		goto out;
+	}
+out:
+	rcu_read_unlock();
+	return ret;
+}
+
  static int shm_mmap(struct file *file, struct vm_area_struct *vma)
  {
  	struct shm_file_data *sfd = shm_file_data(file);
  	int ret;
  
+	/*
+	 * Ensure that we have not raced with IPC_RMID, such that
+	 * we avoid doing the ->mmap altogether. This is a preventive
+	 * lockless check, and thus exposed to races during the mmap.
+	 * However, this is later caught in shm_open(), and handled
+	 * accordingly.
+	 */
+	ret = shm_check_vma_validity(vma);
+	if (ret)
+		return ret;
+
  	ret = sfd->file->f_op->mmap(sfd->file, vma);
  	if (ret != 0)
  		return ret;
+
  	sfd->vm_ops = vma->vm_ops;
  #ifdef CONFIG_MMU
  	WARN_ON(!sfd->vm_ops->fault);
@@ -1193,6 +1248,19 @@ out_fput:
  out_nattch:
  	down_write(&shm_ids(ns).rwsem);
  	shp = shm_lock(ns, shmid);
+	if (unlikely(IS_ERR(shp))) {
+		up_write(&shm_ids(ns).rwsem);
+		err = PTR_ERR(shp);
+		/*
+		 * Before dropping the lock, nattch was incremented,
+		 * thus we cannot race with IPC_RMID (ipc object is
+		 * marked IPC_PRIVATE). As such, this scenario should
+		 * _never_ occur.
+		 */
+		 WARN_ON(1);
+		 goto out;
+	}
+
  	shp->shm_nattch--;
  	if (shm_may_destroy(ns, shp))
  		shm_destroy(ns, shp);
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1245670

On Mon, Oct 12, 2015 at 08:18:21PM -0700, Davidlohr Bueso wrote:
> On Mon, 12 Oct 2015, Bueso wrote:
> 
> >On Mon, 12 Oct 2015, Kirill A. Shutemov wrote:
> >
> >>On Mon, Oct 12, 2015 at 10:49:45AM -0700, Davidlohr Bueso wrote:
> >>>diff --git a/ipc/shm.c b/ipc/shm.c
> >>>index 4178727..9615f19 100644
> >>>--- a/ipc/shm.c
> >>>+++ b/ipc/shm.c
> >>>@@ -385,9 +385,25 @@ static struct mempolicy *shm_get_policy(struct vm_area_struct *vma,
> >>>static int shm_mmap(struct file *file, struct vm_area_struct *vma)
> >>>{
> >>>-	struct shm_file_data *sfd = shm_file_data(file);
> >>>+	struct file *vma_file = vma->vm_file;
> >>>+	struct shm_file_data *sfd = shm_file_data(vma_file);
> >>>+	struct ipc_ids *ids = &shm_ids(sfd->ns);
> >>>+	struct kern_ipc_perm *shp;
> >>>	int ret;
> >>>+	rcu_read_lock();
> >>>+	shp = ipc_obtain_object_check(ids, sfd->id);
> >>>+	if (IS_ERR(shp)) {
> >>>+		ret = -EINVAL;
> >>>+		goto err;
> >>>+	}
> >>>+
> >>>+	if (!ipc_valid_object(shp)) {
> >>>+		ret = -EIDRM;
> >>>+		goto err;
> >>>+	}
> >>>+	rcu_read_unlock();
> >>>+
> >>
> >>Hm. Isn't it racy? What prevents IPC_RMID from happening after this point?
> >
> >Nothing, but that is later caught by shm_open() doing similar checks. We
> >basically end up doing a check between ->mmap() calls, which is fair imho.
> >Note that this can occur anywhere in ipc as IPC_RMID is a user request/cmd,
> >and we try to respect it -- thus you can argue this race anywhere, which is
> >why we have EIDRM/EINVL. Ultimately the user should not be doing such hacks
> >_anyway_. So I'm not really concerned about it.
> >
> >Another similar alternative would be perhaps to make shm_lock() return an
> >error, and thus propagate that error to mmap return. That way we would have
> >a silent way out of the warning scenario (afterward we cannot race as we
> >hold the ipc object lock). However, the users would now have to take this
> >into account...
> >
> >    [validity check lockless]
> >    ->mmap()
> >    [validity check lock]
> 
> Something like this, maybe. Although I could easily be missing things...
> I've tested it enough to see Dimitry's testcase handled ok, and put it
> through ltp. Also adding Manfred to the Cc, who always catches my idiotic
> mistakes.
> 
> 8<---------------------------------------------------------------------
> From: Davidlohr Bueso <dave@stgolabs.net>
> Date: Mon, 12 Oct 2015 19:38:34 -0700
> Subject: [PATCH] ipc/shm: fix handling of (re)attaching to a deleted segment
> 
> There are currently two issues when dealing with segments that are
> marked for deletion:
> 
> (i) With d0edd8528362 (ipc: convert invalid scenarios to use WARN_ON)
> we relaxed the system-wide impact of using a deleted segment. However,
> we can now perfectly well trigger the warning and then deference a nil
> pointer -- where shp does not exist.
> 
> (ii) As of a399b29dfbaa (ipc,shm: fix shm_file deletion races) we
> forbid attaching/mapping a previously deleted segment; a feature once
> unique to Linux, but removed[1] as a side effect of lockless ipc object
> lookups and security checks. Similarly, Dmitry Vyukov reported[2] a
> simple test case that creates a new vma for a previously deleted
> segment, triggering the WARN_ON mentioned in (i).
> 
> This patch tries to address (i) by moving the shp error check out
> of shm_lock() and handled by the caller instead. The benefit of this
> is that it allows better handling out of situations where we end up
> returning ERMID or EINVAL. Specifically, there are three callers
> of shm_lock which we must look into:
> 
>  - open/close -- which we ensure to never do any operations on
>                  the pairs, thus becoming no-ops if found a prev
> 		 IPC_RMID.
> 
>  - loosing the reference of nattch upon shmat(2) -- not feasible.
> 
> In addition, the common WARN_ON call is technically removed, but
> we add a new one for the bogus shmat(2) case, which is definitely
> unacceptable to race with RMID if nattch is bumped up.
> 
> To address (ii), a new shm_check_vma_validity() helper is added
> (for lack of a better name), which attempts to detect early on
> any races with RMID, before doing the full ->mmap. There is still
> a window between the callback and the shm_open call where we can
> race with IPC_RMID. If this is the case, it is handled by the next
> shm_lock().
> 
> shm_mmap:
>     [shm validity checks lockless]
>     ->mmap()
>     [shm validity checks lock] <-- at this point there after there
>                                    is no race as we hold the ipc
>                                    object lock.
> 
> [1] https://lkml.org/lkml/2015/10/12/483
> [2] https://lkml.org/lkml/2015/10/12/284
> 
> Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
> ---
>  ipc/shm.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 73 insertions(+), 5 deletions(-)
> 
> diff --git a/ipc/shm.c b/ipc/shm.c
> index 4178727..47a7a67 100644
> --- a/ipc/shm.c
> +++ b/ipc/shm.c
> @@ -156,11 +156,10 @@ static inline struct shmid_kernel *shm_lock(struct ipc_namespace *ns, int id)
>  	struct kern_ipc_perm *ipcp = ipc_lock(&shm_ids(ns), id);
>  	/*
> -	 * We raced in the idr lookup or with shm_destroy().  Either way, the
> -	 * ID is busted.
> +	 * Callers of shm_lock() must validate the status of the returned
> +	 * ipc object pointer (as returned by ipc_lock()), and error out as
> +	 * appropriate.
>  	 */
> -	WARN_ON(IS_ERR(ipcp));
> -
>  	return container_of(ipcp, struct shmid_kernel, shm_perm);
>  }
> @@ -194,6 +193,15 @@ static void shm_open(struct vm_area_struct *vma)
>  	struct shmid_kernel *shp;
>  	shp = shm_lock(sfd->ns, sfd->id);
> +	/*
> +	 * We raced in the idr lookup or with shm_destroy().
> +	 * Either way, the ID is busted. In the same scenario,
> +	 * but for the close counter-part, the nattch counter
> +	 * is never decreased, thus we can safely return.
> +	 */
> +	if (IS_ERR(shp))
> +		return; /* no-op */
> +
>  	shp->shm_atim = get_seconds();
>  	shp->shm_lprid = task_tgid_vnr(current);
>  	shp->shm_nattch++;

...

>  static int shm_mmap(struct file *file, struct vm_area_struct *vma)
>  {
>  	struct shm_file_data *sfd = shm_file_data(file);
>  	int ret;
> +	/*
> +	 * Ensure that we have not raced with IPC_RMID, such that
> +	 * we avoid doing the ->mmap altogether. This is a preventive
> +	 * lockless check, and thus exposed to races during the mmap.
> +	 * However, this is later caught in shm_open(), and handled
> +	 * accordingly.
> +	 */
> +	ret = shm_check_vma_validity(vma);
> +	if (ret)
> +		return ret;
> +
>  	ret = sfd->file->f_op->mmap(sfd->file, vma);
>  	if (ret != 0)
>  		return ret;
> +
>  	sfd->vm_ops = vma->vm_ops;
>  #ifdef CONFIG_MMU
>  	WARN_ON(!sfd->vm_ops->fault);

If I read it correctly, with the patch we would ignore locking failure
inside shm_open() and mmap will succeed in this case. So the idea is to
have shm_close() no-op and therefore symmetrical. That's look fragile to
me. We would silently miss some other broken open/close pattern.

I would rather propagate error to shm_mmap() caller and therefore to
userspace. I guess it's better to opencode shm_open() in shm_mmap() and
return error this way. shm_open() itself can have WARN_ON_ONCE() for
failure or something.

-- 
 Kirill A. Shutemov
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web