2018-03-28 07:37:14

by Fengguang Wu

[permalink] [raw]
Subject: 98f929b1bd ("ipc/shm: Fix shmctl(..., IPC_STAT, ...) between .."): Oops: 0000 [#1]

Greetings,

0day kernel testing robot got the below dmesg and the first bad commit is

https://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git for-next

commit 98f929b1bd4d0b7c7a77d0d9776d1b924db2e454
Author: Eric W. Biederman <[email protected]>
AuthorDate: Fri Mar 23 00:29:57 2018 -0500
Commit: Eric W. Biederman <[email protected]>
CommitDate: Tue Mar 27 15:53:09 2018 -0500

ipc/shm: Fix shmctl(..., IPC_STAT, ...) between pid namespaces.

Today shm_cpid and shm_lpid are remembered in the pid namespace of the
creator and the processes that last touched a sysvipc shared memory
segment. If you have processes in multiple pid namespaces that
is just wrong, and I don't know how this has been over-looked for
so long.

As only creation and shared memory attach and shared memory detach
update the pids I do not expect there to be a repeat of the issues
when struct pid was attached to each af_unix skb, which in some
notable cases cut the performance in half. The problem was threads of
the same process updating same struct pid from different cpus causing
the cache line to be highly contended and bounce between cpus.

As creation, attach, and detach are expected to be rare operations for
sysvipc shared memory segments I do not expect that kind of cache line
ping pong to cause probems. In addition because the pid is at a fixed
location in the structure instead of being dynamic on a skb, the
reference count of the pid does not need to be updated on each
operation if the pid is the same. This ability to simply skip the pid
reference count changes if the pid is unchanging further reduces the
likelihood of the a cache line holding a pid reference count
ping-ponging between cpus.

Fixes: b488893a390e ("pid namespaces: changes to show virtual ids to user")
Reviewed-by: Nagarathnam Muthusamy <[email protected]>
Signed-off-by: "Eric W. Biederman" <[email protected]>

03f1fc0918 ipc/util: Helpers for making the sysvipc operations pid namespace aware
98f929b1bd ipc/shm: Fix shmctl(..., IPC_STAT, ...) between pid namespaces.
0d79cbf83b ipc/smack: Tidy up from the change in type of the ipc security hooks
+------------------------------------------+------------+------------+------------+
| | 03f1fc0918 | 98f929b1bd | 0d79cbf83b |
+------------------------------------------+------------+------------+------------+
| boot_successes | 33 | 4 | 2 |
| boot_failures | 0 | 11 | 19 |
| Oops:#[##] | 0 | 10 | 12 |
| RIP:put_pid | 0 | 11 | 15 |
| Kernel_panic-not_syncing:Fatal_exception | 0 | 11 | 15 |
| BUG:unable_to_handle_kernel | 0 | 2 | 4 |
| BUG:kernel_in_stage | 0 | 0 | 4 |
| general_protection_fault:#[##] | 0 | 0 | 3 |
+------------------------------------------+------------+------------+------------+

[ 8.040360] gfs2: path_lookup on rootfs returned error -2
[ 8.044048] mount (541) used greatest stack depth: 13352 bytes left
Kernel tests: Boot OK!
[ 18.532190] IP: put_pid+0x22/0x5c
[ 18.532552] PGD 19efa067 P4D 19efa067 PUD 0
[ 18.533006] Oops: 0000 [#1]
[ 18.533318] CPU: 0 PID: 727 Comm: trinity Not tainted 4.16.0-rc2-00010-g98f929b #1
[ 18.534144] RIP: 0010:put_pid+0x22/0x5c
[ 18.534586] RSP: 0018:ffff986719f73e48 EFLAGS: 00010202
[ 18.535129] RAX: 00000006d765f710 RBX: ffff98671a4fa4d0 RCX: ffff986719f73d40
[ 18.535871] RDX: 000000006f6e6125 RSI: 0000000000000000 RDI: ffffffffa01e6d21
[ 18.536616] RBP: ffffffffa0955fe0 R08: 0000000000000020 R09: 0000000000000000
[ 18.537386] R10: 0000000000000078 R11: ffff986719f73e76 R12: 0000000000001000
[ 18.538120] R13: 00000000ffffffea R14: 0000000054000fb0 R15: 0000000000000000
[ 18.538892] FS: 00000000028c2880(0000) GS:ffffffffa06ad000(0000) knlGS:0000000000000000
[ 18.539736] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 18.540360] CR2: 0000000677846439 CR3: 0000000019fc1005 CR4: 00000000000606b0
[ 18.541115] Call Trace:
[ 18.541385] ? ipc_update_pid+0x36/0x3e
[ 18.541792] ? newseg+0x34c/0x3a6
[ 18.542146] ? ipcget+0x5d/0x528
[ 18.542523] ? entry_SYSCALL_64_after_hwframe+0x52/0xb7
[ 18.543068] ? SyS_shmget+0x5a/0x84
[ 18.543444] ? do_syscall_64+0x194/0x1b3
[ 18.543884] ? entry_SYSCALL_64_after_hwframe+0x42/0xb7
[ 18.544434] Code: ff 05 e7 20 9b 03 58 c9 c3 48 ff 05 85 21 9b 03 48 85 ff 74 4f 8b 47 04 8b 17 48 ff 05 7c 21 9b 03 48 83 c0 03 48 c1 e0 04 ff ca <48> 8b 44 07 08 74 1f 48 ff 05 6c 21 9b 03 ff 0f 0f 94 c2 48 ff
[ 18.546443] RIP: put_pid+0x22/0x5c RSP: ffff986719f73e48
[ 18.547026] CR2: 0000000677846439
[ 18.547395] ---[ end trace ab8c5cb4389d37c5 ]---
[ 18.547888] Kernel panic - not syncing: Fatal exception

# HH:MM RESULT GOOD BAD GOOD_BUT_DIRTY DIRTY_NOT_BAD
git bisect start 77a1df249bf89af82d6a2a6fe5f57c3da270f8d9 3eb2ce825ea1ad89d20f7a3b5780df850e4be274 --
git bisect bad dd5963ef89e4dbe5abfea40df18549a440570cdf # 11:17 B 0 11 24 0 Merge 'tj-libata/for-next' into devel-spot-201803280843
git bisect bad 3fb00b899e5e53dd42cc3ef865c017e34f52563d # 11:39 B 0 2 15 0 Merge 'linux-review/NeilBrown/rhashtable-assorted-fixes-and-enhancements/20180328-024648' into devel-spot-201803280843
git bisect good 0d5dbbbdbdf1b6dc50c40fe936e3b0eaf278ab32 # 11:59 G 11 0 4 4 Merge 'masahiroy/for-next' into devel-spot-201803280843
git bisect good 44cf2417eb69f79e9ed9c4023907a286c7a48501 # 12:23 G 11 0 0 0 Merge 'userns/userns-next' into devel-spot-201803280843
git bisect bad a299bee013ccdda737d11cbafeb024e5a8ebcc56 # 12:40 B 0 11 24 0 Merge 'linux-review/Li-RongQing/mm-list_lru-replace-spinlock-with-RCU-in-__list_lru_count_one/20180328-042620' into devel-spot-201803280843
git bisect bad 919db40dc9754fa11973ffcbfbb95f7cf87db991 # 13:00 B 0 11 26 2 Merge 'linux-review/Pablo-Neira-Ayuso/netfilter-ipt_CLUSTERIP-Allow-configuring-local-node-0-again/20180328-044503' into devel-spot-201803280843
git bisect bad a878805a8837c1bfde9745c505d3c37b1f0f73e0 # 13:14 B 0 2 15 0 Merge 'userns/userns-test' into devel-spot-201803280843
git bisect good 34b56df922b10ac2876f268c522951785bf333fd # 13:31 G 11 0 0 0 msg: Move struct msg_queue into ipc/msg.c
git bisect good 03f1fc09180b345582889a344b012d069b3a6dbe # 13:52 G 11 0 0 0 ipc/util: Helpers for making the sysvipc operations pid namespace aware
git bisect bad 39a4940eaa185910bb802ca9829c12268fd2c855 # 14:10 B 0 11 24 0 ipc/msg: Fix msgctl(..., IPC_STAT, ...) between pid namespaces
git bisect bad 98f929b1bd4d0b7c7a77d0d9776d1b924db2e454 # 14:20 B 0 11 25 0 ipc/shm: Fix shmctl(..., IPC_STAT, ...) between pid namespaces.
# first bad commit: [98f929b1bd4d0b7c7a77d0d9776d1b924db2e454] ipc/shm: Fix shmctl(..., IPC_STAT, ...) between pid namespaces.
git bisect good 03f1fc09180b345582889a344b012d069b3a6dbe # 14:23 G 31 0 2 2 ipc/util: Helpers for making the sysvipc operations pid namespace aware
# extra tests with debug options
git bisect bad 98f929b1bd4d0b7c7a77d0d9776d1b924db2e454 # 14:35 B 0 11 25 0 ipc/shm: Fix shmctl(..., IPC_STAT, ...) between pid namespaces.
# extra tests on HEAD of linux-devel/devel-spot-201803280843
git bisect bad 77a1df249bf89af82d6a2a6fe5f57c3da270f8d9 # 14:35 B 0 18 42 7 0day head guard for 'devel-spot-201803280843'
# extra tests on tree/branch userns/for-next
git bisect bad 0d79cbf83be07bb38a1224f47fd0e2b163310442 # 15:06 B 0 4 17 0 ipc/smack: Tidy up from the change in type of the ipc security hooks
# extra tests with first bad commit reverted
git bisect good 96c582dca4d93f55c7dbcb67e4dcaa28b00bc2b1 # 15:29 G 11 0 1 1 Revert "ipc/shm: Fix shmctl(..., IPC_STAT, ...) between pid namespaces."

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/lkp Intel Corporation


Attachments:
(No filename) (8.44 kB)
dmesg-yocto-vp-13:20180328142009:x86_64-randconfig-s3-03281023:4.16.0-rc2-00010-g98f929b:1.gz (19.12 kB)
dmesg-yocto-vp-42:20180328142210:x86_64-randconfig-s3-03281023:4.16.0-rc2-00009-g03f1fc0:1.gz (22.37 kB)
reproduce-yocto-vp-13:20180328142009:x86_64-randconfig-s3-03281023:4.16.0-rc2-00010-g98f929b:1 (977.00 B)
config-4.16.0-rc2-00010-g98f929b (119.63 kB)
Download all attachments

2018-03-28 17:29:52

by Nagarathnam Muthusamy

[permalink] [raw]
Subject: Re: 98f929b1bd ("ipc/shm: Fix shmctl(..., IPC_STAT, ...) between .."): Oops: 0000 [#1]

Hi Eric,

??? From
https://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git/tree/ipc/shm.c?h=for-next

It looks like if the following condition in Line 616 succeeds

error = PTR_ERR(file);
if (IS_ERR(file))
??? goto no_file;

we get to no_file with garbage value in shm_cprid. An attempt to put_pid
on this garbage value might be causing panic.

We could initialize shm_cprid to NULL as soon as it was created.

Thanks,

Nagarathnam.


On 03/28/2018 12:29 AM, kernel test robot wrote:
> Greetings,
>
> 0day kernel testing robot got the below dmesg and the first bad commit is
>
> https://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git for-next
>
> commit 98f929b1bd4d0b7c7a77d0d9776d1b924db2e454
> Author: Eric W. Biederman <[email protected]>
> AuthorDate: Fri Mar 23 00:29:57 2018 -0500
> Commit: Eric W. Biederman <[email protected]>
> CommitDate: Tue Mar 27 15:53:09 2018 -0500
>
> ipc/shm: Fix shmctl(..., IPC_STAT, ...) between pid namespaces.
>
> Today shm_cpid and shm_lpid are remembered in the pid namespace of the
> creator and the processes that last touched a sysvipc shared memory
> segment. If you have processes in multiple pid namespaces that
> is just wrong, and I don't know how this has been over-looked for
> so long.
>
> As only creation and shared memory attach and shared memory detach
> update the pids I do not expect there to be a repeat of the issues
> when struct pid was attached to each af_unix skb, which in some
> notable cases cut the performance in half. The problem was threads of
> the same process updating same struct pid from different cpus causing
> the cache line to be highly contended and bounce between cpus.
>
> As creation, attach, and detach are expected to be rare operations for
> sysvipc shared memory segments I do not expect that kind of cache line
> ping pong to cause probems. In addition because the pid is at a fixed
> location in the structure instead of being dynamic on a skb, the
> reference count of the pid does not need to be updated on each
> operation if the pid is the same. This ability to simply skip the pid
> reference count changes if the pid is unchanging further reduces the
> likelihood of the a cache line holding a pid reference count
> ping-ponging between cpus.
>
> Fixes: b488893a390e ("pid namespaces: changes to show virtual ids to user")
> Reviewed-by: Nagarathnam Muthusamy <[email protected]>
> Signed-off-by: "Eric W. Biederman" <[email protected]>
>
> 03f1fc0918 ipc/util: Helpers for making the sysvipc operations pid namespace aware
> 98f929b1bd ipc/shm: Fix shmctl(..., IPC_STAT, ...) between pid namespaces.
> 0d79cbf83b ipc/smack: Tidy up from the change in type of the ipc security hooks
> +------------------------------------------+------------+------------+------------+
> | | 03f1fc0918 | 98f929b1bd | 0d79cbf83b |
> +------------------------------------------+------------+------------+------------+
> | boot_successes | 33 | 4 | 2 |
> | boot_failures | 0 | 11 | 19 |
> | Oops:#[##] | 0 | 10 | 12 |
> | RIP:put_pid | 0 | 11 | 15 |
> | Kernel_panic-not_syncing:Fatal_exception | 0 | 11 | 15 |
> | BUG:unable_to_handle_kernel | 0 | 2 | 4 |
> | BUG:kernel_in_stage | 0 | 0 | 4 |
> | general_protection_fault:#[##] | 0 | 0 | 3 |
> +------------------------------------------+------------+------------+------------+
>
> [ 8.040360] gfs2: path_lookup on rootfs returned error -2
> [ 8.044048] mount (541) used greatest stack depth: 13352 bytes left
> Kernel tests: Boot OK!
> [ 18.532190] IP: put_pid+0x22/0x5c
> [ 18.532552] PGD 19efa067 P4D 19efa067 PUD 0
> [ 18.533006] Oops: 0000 [#1]
> [ 18.533318] CPU: 0 PID: 727 Comm: trinity Not tainted 4.16.0-rc2-00010-g98f929b #1
> [ 18.534144] RIP: 0010:put_pid+0x22/0x5c
> [ 18.534586] RSP: 0018:ffff986719f73e48 EFLAGS: 00010202
> [ 18.535129] RAX: 00000006d765f710 RBX: ffff98671a4fa4d0 RCX: ffff986719f73d40
> [ 18.535871] RDX: 000000006f6e6125 RSI: 0000000000000000 RDI: ffffffffa01e6d21
> [ 18.536616] RBP: ffffffffa0955fe0 R08: 0000000000000020 R09: 0000000000000000
> [ 18.537386] R10: 0000000000000078 R11: ffff986719f73e76 R12: 0000000000001000
> [ 18.538120] R13: 00000000ffffffea R14: 0000000054000fb0 R15: 0000000000000000
> [ 18.538892] FS: 00000000028c2880(0000) GS:ffffffffa06ad000(0000) knlGS:0000000000000000
> [ 18.539736] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 18.540360] CR2: 0000000677846439 CR3: 0000000019fc1005 CR4: 00000000000606b0
> [ 18.541115] Call Trace:
> [ 18.541385] ? ipc_update_pid+0x36/0x3e
> [ 18.541792] ? newseg+0x34c/0x3a6
> [ 18.542146] ? ipcget+0x5d/0x528
> [ 18.542523] ? entry_SYSCALL_64_after_hwframe+0x52/0xb7
> [ 18.543068] ? SyS_shmget+0x5a/0x84
> [ 18.543444] ? do_syscall_64+0x194/0x1b3
> [ 18.543884] ? entry_SYSCALL_64_after_hwframe+0x42/0xb7
> [ 18.544434] Code: ff 05 e7 20 9b 03 58 c9 c3 48 ff 05 85 21 9b 03 48 85 ff 74 4f 8b 47 04 8b 17 48 ff 05 7c 21 9b 03 48 83 c0 03 48 c1 e0 04 ff ca <48> 8b 44 07 08 74 1f 48 ff 05 6c 21 9b 03 ff 0f 0f 94 c2 48 ff
> [ 18.546443] RIP: put_pid+0x22/0x5c RSP: ffff986719f73e48
> [ 18.547026] CR2: 0000000677846439
> [ 18.547395] ---[ end trace ab8c5cb4389d37c5 ]---
> [ 18.547888] Kernel panic - not syncing: Fatal exception
>
> # HH:MM RESULT GOOD BAD GOOD_BUT_DIRTY DIRTY_NOT_BAD
> git bisect start 77a1df249bf89af82d6a2a6fe5f57c3da270f8d9 3eb2ce825ea1ad89d20f7a3b5780df850e4be274 --
> git bisect bad dd5963ef89e4dbe5abfea40df18549a440570cdf # 11:17 B 0 11 24 0 Merge 'tj-libata/for-next' into devel-spot-201803280843
> git bisect bad 3fb00b899e5e53dd42cc3ef865c017e34f52563d # 11:39 B 0 2 15 0 Merge 'linux-review/NeilBrown/rhashtable-assorted-fixes-and-enhancements/20180328-024648' into devel-spot-201803280843
> git bisect good 0d5dbbbdbdf1b6dc50c40fe936e3b0eaf278ab32 # 11:59 G 11 0 4 4 Merge 'masahiroy/for-next' into devel-spot-201803280843
> git bisect good 44cf2417eb69f79e9ed9c4023907a286c7a48501 # 12:23 G 11 0 0 0 Merge 'userns/userns-next' into devel-spot-201803280843
> git bisect bad a299bee013ccdda737d11cbafeb024e5a8ebcc56 # 12:40 B 0 11 24 0 Merge 'linux-review/Li-RongQing/mm-list_lru-replace-spinlock-with-RCU-in-__list_lru_count_one/20180328-042620' into devel-spot-201803280843
> git bisect bad 919db40dc9754fa11973ffcbfbb95f7cf87db991 # 13:00 B 0 11 26 2 Merge 'linux-review/Pablo-Neira-Ayuso/netfilter-ipt_CLUSTERIP-Allow-configuring-local-node-0-again/20180328-044503' into devel-spot-201803280843
> git bisect bad a878805a8837c1bfde9745c505d3c37b1f0f73e0 # 13:14 B 0 2 15 0 Merge 'userns/userns-test' into devel-spot-201803280843
> git bisect good 34b56df922b10ac2876f268c522951785bf333fd # 13:31 G 11 0 0 0 msg: Move struct msg_queue into ipc/msg.c
> git bisect good 03f1fc09180b345582889a344b012d069b3a6dbe # 13:52 G 11 0 0 0 ipc/util: Helpers for making the sysvipc operations pid namespace aware
> git bisect bad 39a4940eaa185910bb802ca9829c12268fd2c855 # 14:10 B 0 11 24 0 ipc/msg: Fix msgctl(..., IPC_STAT, ...) between pid namespaces
> git bisect bad 98f929b1bd4d0b7c7a77d0d9776d1b924db2e454 # 14:20 B 0 11 25 0 ipc/shm: Fix shmctl(..., IPC_STAT, ...) between pid namespaces.
> # first bad commit: [98f929b1bd4d0b7c7a77d0d9776d1b924db2e454] ipc/shm: Fix shmctl(..., IPC_STAT, ...) between pid namespaces.
> git bisect good 03f1fc09180b345582889a344b012d069b3a6dbe # 14:23 G 31 0 2 2 ipc/util: Helpers for making the sysvipc operations pid namespace aware
> # extra tests with debug options
> git bisect bad 98f929b1bd4d0b7c7a77d0d9776d1b924db2e454 # 14:35 B 0 11 25 0 ipc/shm: Fix shmctl(..., IPC_STAT, ...) between pid namespaces.
> # extra tests on HEAD of linux-devel/devel-spot-201803280843
> git bisect bad 77a1df249bf89af82d6a2a6fe5f57c3da270f8d9 # 14:35 B 0 18 42 7 0day head guard for 'devel-spot-201803280843'
> # extra tests on tree/branch userns/for-next
> git bisect bad 0d79cbf83be07bb38a1224f47fd0e2b163310442 # 15:06 B 0 4 17 0 ipc/smack: Tidy up from the change in type of the ipc security hooks
> # extra tests with first bad commit reverted
> git bisect good 96c582dca4d93f55c7dbcb67e4dcaa28b00bc2b1 # 15:29 G 11 0 1 1 Revert "ipc/shm: Fix shmctl(..., IPC_STAT, ...) between pid namespaces."
>
> ---
> 0-DAY kernel test infrastructure Open Source Technology Center
> https://lists.01.org/pipermail/lkp Intel Corporation


2018-03-28 17:56:19

by Eric W. Biederman

[permalink] [raw]
Subject: Re: 98f929b1bd ("ipc/shm: Fix shmctl(..., IPC_STAT, ...) between .."): Oops: 0000 [#1]

Nagarathnam Muthusamy <[email protected]> writes:

> Hi Eric,
>
>     From
> https://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git/tree/ipc/shm.c?h=for-next
>
> It looks like if the following condition in Line 616 succeeds
>
> error = PTR_ERR(file);
> if (IS_ERR(file))
>     goto no_file;
>
> we get to no_file with garbage value in shm_cprid. An attempt to
> put_pid on this garbage value might be causing panic.
>
> We could initialize shm_cprid to NULL as soon as it was created.

Yes. I misread the kvmalloc as a kvzalloc.
I am planning on placing the pid freeing under the no_id label
instead of the no_file label. Which should also avoid the issue.

It is a rare enough issue an incremental patch should be fine.

Eric

2018-03-28 18:50:53

by Eric W. Biederman

[permalink] [raw]
Subject: [PATCH] ipc/shm: Fix pid freeing.


The 0day kernel test build report reported an oops:
>
> IP: put_pid+0x22/0x5c
> PGD 19efa067 P4D 19efa067 PUD 0
> Oops: 0000 [#1]
> CPU: 0 PID: 727 Comm: trinity Not tainted 4.16.0-rc2-00010-g98f929b #1
> RIP: 0010:put_pid+0x22/0x5c
> RSP: 0018:ffff986719f73e48 EFLAGS: 00010202
> RAX: 00000006d765f710 RBX: ffff98671a4fa4d0 RCX: ffff986719f73d40
> RDX: 000000006f6e6125 RSI: 0000000000000000 RDI: ffffffffa01e6d21
> RBP: ffffffffa0955fe0 R08: 0000000000000020 R09: 0000000000000000
> R10: 0000000000000078 R11: ffff986719f73e76 R12: 0000000000001000
> R13: 00000000ffffffea R14: 0000000054000fb0 R15: 0000000000000000
> FS: 00000000028c2880(0000) GS:ffffffffa06ad000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000677846439 CR3: 0000000019fc1005 CR4: 00000000000606b0
> Call Trace:
> ? ipc_update_pid+0x36/0x3e
> ? newseg+0x34c/0x3a6
> ? ipcget+0x5d/0x528
> ? entry_SYSCALL_64_after_hwframe+0x52/0xb7
> ? SyS_shmget+0x5a/0x84
> ? do_syscall_64+0x194/0x1b3
> ? entry_SYSCALL_64_after_hwframe+0x42/0xb7
> Code: ff 05 e7 20 9b 03 58 c9 c3 48 ff 05 85 21 9b 03 48 85 ff 74 4f 8b 47 04 8b 17 48 ff 05 7c 21 9b 03 48 83 c0 03 48 c1 e0 04 ff ca <48> 8b 44 07 08 74 1f 48 ff 05 6c 21 9b 03 ff 0f 0f 94 c2 48 ff
> RIP: put_pid+0x22/0x5c RSP: ffff986719f73e48
> CR2: 0000000677846439
> ---[ end trace ab8c5cb4389d37c5 ]---
> Kernel panic - not syncing: Fatal exception

In newseg when changing shm_cprid and shm_lprid from pid_t to struct
pid* I misread the kvmalloc as kvzalloc and thought shp was
initialized to 0. As that is not the case it is not safe to for the
error handling to address shm_cprid and shm_lprid before they are
initialized.

Therefore move the cleanup of shm_cprid and shm_lprid from the no_file
error cleanup path to the no_id error cleanup path. Ensuring that an
early error exit won't cause the oops above.

Reported-by: kernel test robot <[email protected]>
Signed-off-by: Eric W. Biederman <[email protected]>
---
ipc/shm.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ipc/shm.c b/ipc/shm.c
index 018db3d0e70e..35bdfe76d11b 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -646,12 +646,12 @@ static int newseg(struct ipc_namespace *ns, struct ipc_params *params)
return error;

no_id:
+ ipc_update_pid(&shp->shm_cprid, NULL);
+ ipc_update_pid(&shp->shm_lprid, NULL);
if (is_file_hugepages(file) && shp->mlock_user)
user_shm_unlock(size, shp->mlock_user);
fput(file);
no_file:
- ipc_update_pid(&shp->shm_cprid, NULL);
- ipc_update_pid(&shp->shm_lprid, NULL);
call_rcu(&shp->shm_perm.rcu, shm_rcu_free);
return error;
}
--
2.14.1


2018-03-28 20:03:25

by Nagarathnam Muthusamy

[permalink] [raw]
Subject: Re: [PATCH] ipc/shm: Fix pid freeing.



On 03/28/2018 11:47 AM, [email protected] wrote:
> The 0day kernel test build report reported an oops:
>> IP: put_pid+0x22/0x5c
>> PGD 19efa067 P4D 19efa067 PUD 0
>> Oops: 0000 [#1]
>> CPU: 0 PID: 727 Comm: trinity Not tainted 4.16.0-rc2-00010-g98f929b #1
>> RIP: 0010:put_pid+0x22/0x5c
>> RSP: 0018:ffff986719f73e48 EFLAGS: 00010202
>> RAX: 00000006d765f710 RBX: ffff98671a4fa4d0 RCX: ffff986719f73d40
>> RDX: 000000006f6e6125 RSI: 0000000000000000 RDI: ffffffffa01e6d21
>> RBP: ffffffffa0955fe0 R08: 0000000000000020 R09: 0000000000000000
>> R10: 0000000000000078 R11: ffff986719f73e76 R12: 0000000000001000
>> R13: 00000000ffffffea R14: 0000000054000fb0 R15: 0000000000000000
>> FS: 00000000028c2880(0000) GS:ffffffffa06ad000(0000) knlGS:0000000000000000
>> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> CR2: 0000000677846439 CR3: 0000000019fc1005 CR4: 00000000000606b0
>> Call Trace:
>> ? ipc_update_pid+0x36/0x3e
>> ? newseg+0x34c/0x3a6
>> ? ipcget+0x5d/0x528
>> ? entry_SYSCALL_64_after_hwframe+0x52/0xb7
>> ? SyS_shmget+0x5a/0x84
>> ? do_syscall_64+0x194/0x1b3
>> ? entry_SYSCALL_64_after_hwframe+0x42/0xb7
>> Code: ff 05 e7 20 9b 03 58 c9 c3 48 ff 05 85 21 9b 03 48 85 ff 74 4f 8b 47 04 8b 17 48 ff 05 7c 21 9b 03 48 83 c0 03 48 c1 e0 04 ff ca <48> 8b 44 07 08 74 1f 48 ff 05 6c 21 9b 03 ff 0f 0f 94 c2 48 ff
>> RIP: put_pid+0x22/0x5c RSP: ffff986719f73e48
>> CR2: 0000000677846439
>> ---[ end trace ab8c5cb4389d37c5 ]---
>> Kernel panic - not syncing: Fatal exception
> In newseg when changing shm_cprid and shm_lprid from pid_t to struct
> pid* I misread the kvmalloc as kvzalloc and thought shp was
> initialized to 0. As that is not the case it is not safe to for the
> error handling to address shm_cprid and shm_lprid before they are
> initialized.
>
> Therefore move the cleanup of shm_cprid and shm_lprid from the no_file
> error cleanup path to the no_id error cleanup path. Ensuring that an
> early error exit won't cause the oops above.
>
> Reported-by: kernel test robot <[email protected]>
> Signed-off-by: Eric W. Biederman <[email protected]>
Thanks!

Reviewed-by: Nagarathnam Muthusamy <[email protected]>
> ---
> ipc/shm.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/ipc/shm.c b/ipc/shm.c
> index 018db3d0e70e..35bdfe76d11b 100644
> --- a/ipc/shm.c
> +++ b/ipc/shm.c
> @@ -646,12 +646,12 @@ static int newseg(struct ipc_namespace *ns, struct ipc_params *params)
> return error;
>
> no_id:
> + ipc_update_pid(&shp->shm_cprid, NULL);
> + ipc_update_pid(&shp->shm_lprid, NULL);
> if (is_file_hugepages(file) && shp->mlock_user)
> user_shm_unlock(size, shp->mlock_user);
> fput(file);
> no_file:
> - ipc_update_pid(&shp->shm_cprid, NULL);
> - ipc_update_pid(&shp->shm_lprid, NULL);
> call_rcu(&shp->shm_perm.rcu, shm_rcu_free);
> return error;
> }