| Message ID | d1d6a20f5090829629df76809fc5d25d055be49a.1712849802.git.dcaratti@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [net] netlabel: fix RCU annotation for IPv4 options on socket creation | expand |
On Thu, Apr 11, 2024 at 5:44 PM Davide Caratti <dcaratti@redhat.com> wrote: > > Xiumei reports the following splat when netlabel and TCP socket are used: > > ============================= > WARNING: suspicious RCU usage > 6.9.0-rc2+ #637 Not tainted > ----------------------------- > net/ipv4/cipso_ipv4.c:1880 suspicious rcu_dereference_protected() usage! > > other info that might help us debug this: > > rcu_scheduler_active = 2, debug_locks = 1 > 1 lock held by ncat/23333: > #0: ffffffff906030c0 (rcu_read_lock){....}-{1:2}, at: netlbl_sock_setattr+0x25/0x1b0 > > stack backtrace: > CPU: 11 PID: 23333 Comm: ncat Kdump: loaded Not tainted 6.9.0-rc2+ #637 > Hardware name: Supermicro SYS-6027R-72RF/X9DRH-7TF/7F/iTF/iF, BIOS 3.0 07/26/2013 > Call Trace: > <TASK> > dump_stack_lvl+0xa9/0xc0 > lockdep_rcu_suspicious+0x117/0x190 > cipso_v4_sock_setattr+0x1ab/0x1b0 > netlbl_sock_setattr+0x13e/0x1b0 > selinux_netlbl_socket_post_create+0x3f/0x80 > selinux_socket_post_create+0x1a0/0x460 > security_socket_post_create+0x42/0x60 > __sock_create+0x342/0x3a0 > __sys_socket_create.part.22+0x42/0x70 > __sys_socket+0x37/0xb0 > __x64_sys_socket+0x16/0x20 > do_syscall_64+0x96/0x180 > ? do_user_addr_fault+0x68d/0xa30 > ? exc_page_fault+0x171/0x280 > ? asm_exc_page_fault+0x22/0x30 > entry_SYSCALL_64_after_hwframe+0x71/0x79 > RIP: 0033:0x7fbc0ca3fc1b > Code: 73 01 c3 48 8b 0d 05 f2 1b 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa b8 29 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d d5 f1 1b 00 f7 d8 64 89 01 48 > RSP: 002b:00007fff18635208 EFLAGS: 00000246 ORIG_RAX: 0000000000000029 > RAX: ffffffffffffffda RBX: 0000000000000001 RCX: 00007fbc0ca3fc1b > RDX: 0000000000000006 RSI: 0000000000000001 RDI: 0000000000000002 > RBP: 000055d24f80f8a0 R08: 0000000000000003 R09: 0000000000000001 > R10: 0000000000020000 R11: 0000000000000246 R12: 000055d24f80f8a0 > R13: 0000000000000000 R14: 000055d24f80fb88 R15: 0000000000000000 > </TASK> > > The current implementation of cipso_v4_sock_setattr() replaces IP options > under the assumption that the caller holds the socket lock; however, such > assumption is not true, nor needed, in selinux_socket_post_create() hook. > > Using rcu_dereference_check() instead of rcu_dereference_protected() will > avoid the reported splat for the netlbl_sock_setattr() case, and preserve > the legitimate check when the caller is netlbl_conn_setattr(). > > Fixes: f6d8bd051c39 ("inet: add RCU protection to inet->opt") > Reported-by: Xiumei Mu <xmu@redhat.com> > Suggested-by: Paolo Abeni <pabeni@redhat.com> > Signed-off-by: Davide Caratti <dcaratti@redhat.com> > --- > net/ipv4/cipso_ipv4.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c > index 8b17d83e5fde..1d0c2a905078 100644 > --- a/net/ipv4/cipso_ipv4.c > +++ b/net/ipv4/cipso_ipv4.c > @@ -1876,8 +1876,10 @@ int cipso_v4_sock_setattr(struct sock *sk, > > sk_inet = inet_sk(sk); > > - old = rcu_dereference_protected(sk_inet->inet_opt, > - lockdep_sock_is_held(sk)); > + /* caller either holds rcu_read_lock() (on socket creation) > + * or socket lock (in all other cases). */ > + old = rcu_dereference_check(sk_inet->inet_opt, > + lockdep_sock_is_held(sk)); > if (inet_test_bit(IS_ICSK, sk)) { > sk_conn = inet_csk(sk); > if (old) > -- > 2.44.0 > OK, but rcu_read_lock() being held (incidentally by the caller) here is not protecting the write operation, so this looks wrong IMO. Whenever we can not ensure a mutex/spinlock is held, we usually use rcu_dereference_protected(XXX, 1), and a comment might simply explain the reason we assert it is protected. (We also could add a new boolean parameter, set to true or false depending on the caller) old = rcu_dereference_protected(sk_inet->inet_opt, from_socket_creation || lockdep_sock_is_held(sk));
On 4/11/2024 8:56 AM, Eric Dumazet wrote: > On Thu, Apr 11, 2024 at 5:44 PM Davide Caratti <dcaratti@redhat.com> wrote: >> Xiumei reports the following splat when netlabel and TCP socket are used: >> >> ============================= >> WARNING: suspicious RCU usage >> 6.9.0-rc2+ #637 Not tainted >> ----------------------------- >> net/ipv4/cipso_ipv4.c:1880 suspicious rcu_dereference_protected() usage! >> >> other info that might help us debug this: >> >> rcu_scheduler_active = 2, debug_locks = 1 >> 1 lock held by ncat/23333: >> #0: ffffffff906030c0 (rcu_read_lock){....}-{1:2}, at: netlbl_sock_setattr+0x25/0x1b0 >> >> stack backtrace: >> CPU: 11 PID: 23333 Comm: ncat Kdump: loaded Not tainted 6.9.0-rc2+ #637 >> Hardware name: Supermicro SYS-6027R-72RF/X9DRH-7TF/7F/iTF/iF, BIOS 3.0 07/26/2013 >> Call Trace: >> <TASK> >> dump_stack_lvl+0xa9/0xc0 >> lockdep_rcu_suspicious+0x117/0x190 >> cipso_v4_sock_setattr+0x1ab/0x1b0 >> netlbl_sock_setattr+0x13e/0x1b0 >> selinux_netlbl_socket_post_create+0x3f/0x80 >> selinux_socket_post_create+0x1a0/0x460 >> security_socket_post_create+0x42/0x60 >> __sock_create+0x342/0x3a0 >> __sys_socket_create.part.22+0x42/0x70 >> __sys_socket+0x37/0xb0 >> __x64_sys_socket+0x16/0x20 >> do_syscall_64+0x96/0x180 >> ? do_user_addr_fault+0x68d/0xa30 >> ? exc_page_fault+0x171/0x280 >> ? asm_exc_page_fault+0x22/0x30 >> entry_SYSCALL_64_after_hwframe+0x71/0x79 >> RIP: 0033:0x7fbc0ca3fc1b >> Code: 73 01 c3 48 8b 0d 05 f2 1b 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa b8 29 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d d5 f1 1b 00 f7 d8 64 89 01 48 >> RSP: 002b:00007fff18635208 EFLAGS: 00000246 ORIG_RAX: 0000000000000029 >> RAX: ffffffffffffffda RBX: 0000000000000001 RCX: 00007fbc0ca3fc1b >> RDX: 0000000000000006 RSI: 0000000000000001 RDI: 0000000000000002 >> RBP: 000055d24f80f8a0 R08: 0000000000000003 R09: 0000000000000001 >> R10: 0000000000020000 R11: 0000000000000246 R12: 000055d24f80f8a0 >> R13: 0000000000000000 R14: 000055d24f80fb88 R15: 0000000000000000 >> </TASK> >> >> The current implementation of cipso_v4_sock_setattr() replaces IP options >> under the assumption that the caller holds the socket lock; however, such >> assumption is not true, nor needed, in selinux_socket_post_create() hook. >> >> Using rcu_dereference_check() instead of rcu_dereference_protected() will >> avoid the reported splat for the netlbl_sock_setattr() case, and preserve >> the legitimate check when the caller is netlbl_conn_setattr(). >> >> Fixes: f6d8bd051c39 ("inet: add RCU protection to inet->opt") >> Reported-by: Xiumei Mu <xmu@redhat.com> >> Suggested-by: Paolo Abeni <pabeni@redhat.com> >> Signed-off-by: Davide Caratti <dcaratti@redhat.com> Please be sure to verify that this is appropriate for all users of netlabel. SELinux is not the only user of netlabel. >> --- >> net/ipv4/cipso_ipv4.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c >> index 8b17d83e5fde..1d0c2a905078 100644 >> --- a/net/ipv4/cipso_ipv4.c >> +++ b/net/ipv4/cipso_ipv4.c >> @@ -1876,8 +1876,10 @@ int cipso_v4_sock_setattr(struct sock *sk, >> >> sk_inet = inet_sk(sk); >> >> - old = rcu_dereference_protected(sk_inet->inet_opt, >> - lockdep_sock_is_held(sk)); >> + /* caller either holds rcu_read_lock() (on socket creation) >> + * or socket lock (in all other cases). */ >> + old = rcu_dereference_check(sk_inet->inet_opt, >> + lockdep_sock_is_held(sk)); >> if (inet_test_bit(IS_ICSK, sk)) { >> sk_conn = inet_csk(sk); >> if (old) >> -- >> 2.44.0 >> > OK, but rcu_read_lock() being held (incidentally by the caller) here > is not protecting the write operation, > so this looks wrong IMO. > > Whenever we can not ensure a mutex/spinlock is held, we usually use > rcu_dereference_protected(XXX, 1), > and a comment might simply explain the reason we assert it is protected. > > (We also could add a new boolean parameter, set to true or false > depending on the caller) > > old = rcu_dereference_protected(sk_inet->inet_opt, from_socket_creation || > > lockdep_sock_is_held(sk)); >
On Thu, Apr 11, 2024 at 11:57 AM Eric Dumazet <edumazet@google.com> wrote: > On Thu, Apr 11, 2024 at 5:44 PM Davide Caratti <dcaratti@redhat.com> wrote: > > > > Xiumei reports the following splat when netlabel and TCP socket are used: > > > > ============================= > > WARNING: suspicious RCU usage > > 6.9.0-rc2+ #637 Not tainted > > ----------------------------- > > net/ipv4/cipso_ipv4.c:1880 suspicious rcu_dereference_protected() usage! > > > > other info that might help us debug this: > > > > rcu_scheduler_active = 2, debug_locks = 1 > > 1 lock held by ncat/23333: > > #0: ffffffff906030c0 (rcu_read_lock){....}-{1:2}, at: netlbl_sock_setattr+0x25/0x1b0 > > > > stack backtrace: > > CPU: 11 PID: 23333 Comm: ncat Kdump: loaded Not tainted 6.9.0-rc2+ #637 > > Hardware name: Supermicro SYS-6027R-72RF/X9DRH-7TF/7F/iTF/iF, BIOS 3.0 07/26/2013 > > Call Trace: > > <TASK> > > dump_stack_lvl+0xa9/0xc0 > > lockdep_rcu_suspicious+0x117/0x190 > > cipso_v4_sock_setattr+0x1ab/0x1b0 > > netlbl_sock_setattr+0x13e/0x1b0 > > selinux_netlbl_socket_post_create+0x3f/0x80 > > selinux_socket_post_create+0x1a0/0x460 > > security_socket_post_create+0x42/0x60 > > __sock_create+0x342/0x3a0 > > __sys_socket_create.part.22+0x42/0x70 > > __sys_socket+0x37/0xb0 > > __x64_sys_socket+0x16/0x20 > > do_syscall_64+0x96/0x180 > > ? do_user_addr_fault+0x68d/0xa30 > > ? exc_page_fault+0x171/0x280 > > ? asm_exc_page_fault+0x22/0x30 > > entry_SYSCALL_64_after_hwframe+0x71/0x79 > > RIP: 0033:0x7fbc0ca3fc1b > > Code: 73 01 c3 48 8b 0d 05 f2 1b 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa b8 29 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d d5 f1 1b 00 f7 d8 64 89 01 48 > > RSP: 002b:00007fff18635208 EFLAGS: 00000246 ORIG_RAX: 0000000000000029 > > RAX: ffffffffffffffda RBX: 0000000000000001 RCX: 00007fbc0ca3fc1b > > RDX: 0000000000000006 RSI: 0000000000000001 RDI: 0000000000000002 > > RBP: 000055d24f80f8a0 R08: 0000000000000003 R09: 0000000000000001 > > R10: 0000000000020000 R11: 0000000000000246 R12: 000055d24f80f8a0 > > R13: 0000000000000000 R14: 000055d24f80fb88 R15: 0000000000000000 > > </TASK> > > > > The current implementation of cipso_v4_sock_setattr() replaces IP options > > under the assumption that the caller holds the socket lock; however, such > > assumption is not true, nor needed, in selinux_socket_post_create() hook. > > > > Using rcu_dereference_check() instead of rcu_dereference_protected() will > > avoid the reported splat for the netlbl_sock_setattr() case, and preserve > > the legitimate check when the caller is netlbl_conn_setattr(). > > > > Fixes: f6d8bd051c39 ("inet: add RCU protection to inet->opt") > > Reported-by: Xiumei Mu <xmu@redhat.com> > > Suggested-by: Paolo Abeni <pabeni@redhat.com> > > Signed-off-by: Davide Caratti <dcaratti@redhat.com> > > --- > > net/ipv4/cipso_ipv4.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c > > index 8b17d83e5fde..1d0c2a905078 100644 > > --- a/net/ipv4/cipso_ipv4.c > > +++ b/net/ipv4/cipso_ipv4.c > > @@ -1876,8 +1876,10 @@ int cipso_v4_sock_setattr(struct sock *sk, > > > > sk_inet = inet_sk(sk); > > > > - old = rcu_dereference_protected(sk_inet->inet_opt, > > - lockdep_sock_is_held(sk)); > > + /* caller either holds rcu_read_lock() (on socket creation) > > + * or socket lock (in all other cases). */ > > + old = rcu_dereference_check(sk_inet->inet_opt, > > + lockdep_sock_is_held(sk)); > > if (inet_test_bit(IS_ICSK, sk)) { > > sk_conn = inet_csk(sk); > > if (old) > > -- > > 2.44.0 > > > > OK, but rcu_read_lock() being held (incidentally by the caller) here > is not protecting the write operation, > so this looks wrong IMO. > > Whenever we can not ensure a mutex/spinlock is held, we usually use > rcu_dereference_protected(XXX, 1), > and a comment might simply explain the reason we assert it is protected. > > (We also could add a new boolean parameter, set to true or false > depending on the caller) > > old = rcu_dereference_protected(sk_inet->inet_opt, from_socket_creation || > > lockdep_sock_is_held(sk)); Agree with everything Eric said. I'm okay with the rcu_deref_prot(XXX, 1) + comment approach, but if you wanted to put in the work to chase the callers and setup the boolean that would be great too.
On Thu, Apr 11, 2024 at 1:41 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > > Please be sure to verify that this is appropriate for all users of netlabel. > SELinux is not the only user of netlabel. Adding my support to Casey's comment above. If you go the boolean route, please work with Casey to ensure that the Smack usage is properly handled.
diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c index 8b17d83e5fde..1d0c2a905078 100644 --- a/net/ipv4/cipso_ipv4.c +++ b/net/ipv4/cipso_ipv4.c @@ -1876,8 +1876,10 @@ int cipso_v4_sock_setattr(struct sock *sk, sk_inet = inet_sk(sk); - old = rcu_dereference_protected(sk_inet->inet_opt, - lockdep_sock_is_held(sk)); + /* caller either holds rcu_read_lock() (on socket creation) + * or socket lock (in all other cases). */ + old = rcu_dereference_check(sk_inet->inet_opt, + lockdep_sock_is_held(sk)); if (inet_test_bit(IS_ICSK, sk)) { sk_conn = inet_csk(sk); if (old)
Xiumei reports the following splat when netlabel and TCP socket are used: ============================= WARNING: suspicious RCU usage 6.9.0-rc2+ #637 Not tainted ----------------------------- net/ipv4/cipso_ipv4.c:1880 suspicious rcu_dereference_protected() usage! other info that might help us debug this: rcu_scheduler_active = 2, debug_locks = 1 1 lock held by ncat/23333: #0: ffffffff906030c0 (rcu_read_lock){....}-{1:2}, at: netlbl_sock_setattr+0x25/0x1b0 stack backtrace: CPU: 11 PID: 23333 Comm: ncat Kdump: loaded Not tainted 6.9.0-rc2+ #637 Hardware name: Supermicro SYS-6027R-72RF/X9DRH-7TF/7F/iTF/iF, BIOS 3.0 07/26/2013 Call Trace: <TASK> dump_stack_lvl+0xa9/0xc0 lockdep_rcu_suspicious+0x117/0x190 cipso_v4_sock_setattr+0x1ab/0x1b0 netlbl_sock_setattr+0x13e/0x1b0 selinux_netlbl_socket_post_create+0x3f/0x80 selinux_socket_post_create+0x1a0/0x460 security_socket_post_create+0x42/0x60 __sock_create+0x342/0x3a0 __sys_socket_create.part.22+0x42/0x70 __sys_socket+0x37/0xb0 __x64_sys_socket+0x16/0x20 do_syscall_64+0x96/0x180 ? do_user_addr_fault+0x68d/0xa30 ? exc_page_fault+0x171/0x280 ? asm_exc_page_fault+0x22/0x30 entry_SYSCALL_64_after_hwframe+0x71/0x79 RIP: 0033:0x7fbc0ca3fc1b Code: 73 01 c3 48 8b 0d 05 f2 1b 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa b8 29 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d d5 f1 1b 00 f7 d8 64 89 01 48 RSP: 002b:00007fff18635208 EFLAGS: 00000246 ORIG_RAX: 0000000000000029 RAX: ffffffffffffffda RBX: 0000000000000001 RCX: 00007fbc0ca3fc1b RDX: 0000000000000006 RSI: 0000000000000001 RDI: 0000000000000002 RBP: 000055d24f80f8a0 R08: 0000000000000003 R09: 0000000000000001 R10: 0000000000020000 R11: 0000000000000246 R12: 000055d24f80f8a0 R13: 0000000000000000 R14: 000055d24f80fb88 R15: 0000000000000000 </TASK> The current implementation of cipso_v4_sock_setattr() replaces IP options under the assumption that the caller holds the socket lock; however, such assumption is not true, nor needed, in selinux_socket_post_create() hook. Using rcu_dereference_check() instead of rcu_dereference_protected() will avoid the reported splat for the netlbl_sock_setattr() case, and preserve the legitimate check when the caller is netlbl_conn_setattr(). Fixes: f6d8bd051c39 ("inet: add RCU protection to inet->opt") Reported-by: Xiumei Mu <xmu@redhat.com> Suggested-by: Paolo Abeni <pabeni@redhat.com> Signed-off-by: Davide Caratti <dcaratti@redhat.com> --- net/ipv4/cipso_ipv4.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)