| Message ID | 20241030094953.1921574-1-liujian56@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [net,v2] sunrpc: fix one UAF issue caused by sunrpc kernel tcp socket | expand |
From: Liu Jian <liujian56@huawei.com> Date: Wed, 30 Oct 2024 17:49:53 +0800 > BUG: KASAN: slab-use-after-free in tcp_write_timer_handler+0x156/0x3e0 > Read of size 1 at addr ffff888111f322cd by task swapper/0/0 > > CPU: 0 UID: 0 PID: 0 Comm: swapper/0 Not tainted 6.12.0-rc4-dirty #7 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 > Call Trace: > <IRQ> > dump_stack_lvl+0x68/0xa0 > print_address_description.constprop.0+0x2c/0x3d0 > print_report+0xb4/0x270 > kasan_report+0xbd/0xf0 > tcp_write_timer_handler+0x156/0x3e0 > tcp_write_timer+0x66/0x170 > call_timer_fn+0xfb/0x1d0 > __run_timers+0x3f8/0x480 > run_timer_softirq+0x9b/0x100 > handle_softirqs+0x153/0x390 > __irq_exit_rcu+0x103/0x120 > irq_exit_rcu+0xe/0x20 > sysvec_apic_timer_interrupt+0x76/0x90 > </IRQ> > <TASK> > asm_sysvec_apic_timer_interrupt+0x1a/0x20 > RIP: 0010:default_idle+0xf/0x20 > Code: 4c 01 c7 4c 29 c2 e9 72 ff ff ff 90 90 90 90 90 90 90 90 90 90 90 90 > 90 90 90 90 f3 0f 1e fa 66 90 0f 00 2d 33 f8 25 00 fb f4 <fa> c3 cc cc cc > cc 66 66 2e 0f 1f 84 00 00 00 00 00 90 90 90 90 90 > RSP: 0018:ffffffffa2007e28 EFLAGS: 00000242 > RAX: 00000000000f3b31 RBX: 1ffffffff4400fc7 RCX: ffffffffa09c3196 > RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffffffff9f00590f > RBP: 0000000000000000 R08: 0000000000000001 R09: ffffed102360835d > R10: ffff88811b041aeb R11: 0000000000000001 R12: 0000000000000000 > R13: ffffffffa202d7c0 R14: 0000000000000000 R15: 00000000000147d0 > default_idle_call+0x6b/0xa0 > cpuidle_idle_call+0x1af/0x1f0 > do_idle+0xbc/0x130 > cpu_startup_entry+0x33/0x40 > rest_init+0x11f/0x210 > start_kernel+0x39a/0x420 > x86_64_start_reservations+0x18/0x30 > x86_64_start_kernel+0x97/0xa0 > common_startup_64+0x13e/0x141 > </TASK> > > Allocated by task 595: > kasan_save_stack+0x24/0x50 > kasan_save_track+0x14/0x30 > __kasan_slab_alloc+0x87/0x90 > kmem_cache_alloc_noprof+0x12b/0x3f0 > copy_net_ns+0x94/0x380 > create_new_namespaces+0x24c/0x500 > unshare_nsproxy_namespaces+0x75/0xf0 > ksys_unshare+0x24e/0x4f0 > __x64_sys_unshare+0x1f/0x30 > do_syscall_64+0x70/0x180 > entry_SYSCALL_64_after_hwframe+0x76/0x7e > > Freed by task 100: > kasan_save_stack+0x24/0x50 > kasan_save_track+0x14/0x30 > kasan_save_free_info+0x3b/0x60 > __kasan_slab_free+0x54/0x70 > kmem_cache_free+0x156/0x5d0 > cleanup_net+0x5d3/0x670 > process_one_work+0x776/0xa90 > worker_thread+0x2e2/0x560 > kthread+0x1a8/0x1f0 > ret_from_fork+0x34/0x60 > ret_from_fork_asm+0x1a/0x30 > > Reproduction script: > > mkdir -p /mnt/nfsshare > mkdir -p /mnt/nfs/netns_1 > mkfs.ext4 /dev/sdb > mount /dev/sdb /mnt/nfsshare > systemctl restart nfs-server > chmod 777 /mnt/nfsshare > exportfs -i -o rw,no_root_squash *:/mnt/nfsshare > > ip netns add netns_1 > ip link add name veth_1_peer type veth peer veth_1 > ifconfig veth_1_peer 11.11.0.254 up > ip link set veth_1 netns netns_1 > ip netns exec netns_1 ifconfig veth_1 11.11.0.1 > > ip netns exec netns_1 /root/iptables -A OUTPUT -d 11.11.0.254 -p tcp \ > --tcp-flags FIN FIN -j DROP > > (note: In my environment, a DESTROY_CLIENTID operation is always sent > immediately, breaking the nfs tcp connection.) > ip netns exec netns_1 timeout -s 9 300 mount -t nfs -o proto=tcp,vers=4.1 \ > 11.11.0.254:/mnt/nfsshare /mnt/nfs/netns_1 > > ip netns del netns_1 > > The reason here is that the tcp socket in netns_1 (nfs side) has been > shutdown and closed (done in xs_destroy), but the FIN message (with ack) > is discarded, and the nfsd side keeps sending retransmission messages. > As a result, when the tcp sock in netns_1 processes the received message, > it sends the message (FIN message) in the sending queue, and the tcp timer > is re-established. When the network namespace is deleted, the net structure > accessed by tcp's timer handler function causes problems. > > To fix this problem: > Add the sock_create_kern_getnet() helper function, add the get_net() > operation for the kernel socket. > > Fixes: 26abe14379f8 ("net: Modify sk_alloc to not reference count the netns of kernel sockets.") > Signed-off-by: Liu Jian <liujian56@huawei.com> > --- > v1: https://lore.kernel.org/all/20241024015543.568476-1-liujian56@huawei.com/ > v1->v2: change to get netns reference count. > include/linux/net.h | 1 + > net/socket.c | 28 ++++++++++++++++++++++++++++ > net/sunrpc/svcsock.c | 2 +- > net/sunrpc/xprtsock.c | 2 +- > 4 files changed, 31 insertions(+), 2 deletions(-) > > diff --git a/include/linux/net.h b/include/linux/net.h > index b75bc534c1b3..58216da3b62c 100644 > --- a/include/linux/net.h > +++ b/include/linux/net.h > @@ -255,6 +255,7 @@ int __sock_create(struct net *net, int family, int type, int proto, > struct socket **res, int kern); > int sock_create(int family, int type, int proto, struct socket **res); > int sock_create_kern(struct net *net, int family, int type, int proto, struct socket **res); > +int sock_create_kern_getnet(struct net *net, int family, int type, int proto, struct socket **res); > int sock_create_lite(int family, int type, int proto, struct socket **res); > struct socket *sock_alloc(void); > void sock_release(struct socket *sock); > diff --git a/net/socket.c b/net/socket.c > index 042451f01c65..e64a02445b1a 100644 > --- a/net/socket.c > +++ b/net/socket.c > @@ -1651,6 +1651,34 @@ int sock_create_kern(struct net *net, int family, int type, int protocol, struct > } > EXPORT_SYMBOL(sock_create_kern); > > +int sock_create_kern_getnet(struct net *net, int family, int type, int proto, struct socket **res) > +{ > + struct sock *sk; > + int ret; > + > + if (!maybe_get_net(net)) > + return -EINVAL; Is this really safe ? IIUC, maybe_get_net() is safe for a net only when it is fetched under RCU, then rcu_read_lock() prevents cleanup_net() from reusing the net by rcu_barrier(). Otherwise, there should be a small chance that the same slab object is reused for another netns between fetching the net and reaching here. svc_create_socket() is called much later after the netns is fetched, and _svc_xprt_create() calls try_module_get() before ->xpo_create(). So, it seems the path is not under RCU and maybe_get_net() must be called much earlier by each call site. For this reason, when I write a patch for the same issue in CIFS, I delayed put_net() to cifsd kthread so that the netns refcnt taken for each CIFS server info lives until the last __sock_create() attempt from cifsd. https://lore.kernel.org/linux-cifs/20241102212438.76691-1-kuniyu@amazon.com/ > + > + ret = sock_create_kern(net, family, type, proto, res); > + if (ret < 0) { > + put_net(net); > + return ret; > + } > + > + sk = (*res)->sk; > + lock_sock(sk); > + /* Update ns_tracker to current stack trace and refcounted tracker */ > + __netns_tracker_free(net, &sk->ns_tracker, false); > + > + sk->sk_net_refcnt = 1; > + netns_tracker_alloc(net, &sk->ns_tracker, GFP_KERNEL); > + sock_inuse_add(net, 1); > + release_sock(sk); > + > + return ret; > +} > +EXPORT_SYMBOL(sock_create_kern_getnet); > + > static struct socket *__sys_socket_create(int family, int type, int protocol) > { > struct socket *sock; > diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c > index 825ec5357691..6f272013fd9b 100644 > --- a/net/sunrpc/svcsock.c > +++ b/net/sunrpc/svcsock.c > @@ -1526,7 +1526,7 @@ static struct svc_xprt *svc_create_socket(struct svc_serv *serv, > return ERR_PTR(-EINVAL); > } > > - error = __sock_create(net, family, type, protocol, &sock, 1); > + error = sock_create_kern_getnet(net, family, type, protocol, &sock); > if (error < 0) > return ERR_PTR(error); > > diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c > index 110749b85040..f7734ce5eec9 100644 > --- a/net/sunrpc/xprtsock.c > +++ b/net/sunrpc/xprtsock.c > @@ -1925,7 +1925,7 @@ static struct socket *xs_create_sock(struct rpc_xprt *xprt, > struct socket *sock; > int err; > > - err = __sock_create(xprt->xprt_net, family, type, protocol, &sock, 1); > + err = sock_create_kern_getnet(xprt->xprt_net, family, type, protocol, &sock); > if (err < 0) { > dprintk("RPC: can't create %d transport socket (%d).\n", > protocol, -err); > -- > 2.34.1
在 2024/11/3 12:09, Kuniyuki Iwashima 写道: > From: Liu Jian <liujian56@huawei.com> > Date: Wed, 30 Oct 2024 17:49:53 +0800 >> BUG: KASAN: slab-use-after-free in tcp_write_timer_handler+0x156/0x3e0 >> Read of size 1 at addr ffff888111f322cd by task swapper/0/0 >> >> CPU: 0 UID: 0 PID: 0 Comm: swapper/0 Not tainted 6.12.0-rc4-dirty #7 >> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 >> Call Trace: >> <IRQ> >> dump_stack_lvl+0x68/0xa0 >> print_address_description.constprop.0+0x2c/0x3d0 >> print_report+0xb4/0x270 >> kasan_report+0xbd/0xf0 >> tcp_write_timer_handler+0x156/0x3e0 >> tcp_write_timer+0x66/0x170 >> call_timer_fn+0xfb/0x1d0 >> __run_timers+0x3f8/0x480 >> run_timer_softirq+0x9b/0x100 >> handle_softirqs+0x153/0x390 >> __irq_exit_rcu+0x103/0x120 >> irq_exit_rcu+0xe/0x20 >> sysvec_apic_timer_interrupt+0x76/0x90 >> </IRQ> >> <TASK> >> asm_sysvec_apic_timer_interrupt+0x1a/0x20 >> RIP: 0010:default_idle+0xf/0x20 >> Code: 4c 01 c7 4c 29 c2 e9 72 ff ff ff 90 90 90 90 90 90 90 90 90 90 90 90 >> 90 90 90 90 f3 0f 1e fa 66 90 0f 00 2d 33 f8 25 00 fb f4 <fa> c3 cc cc cc >> cc 66 66 2e 0f 1f 84 00 00 00 00 00 90 90 90 90 90 >> RSP: 0018:ffffffffa2007e28 EFLAGS: 00000242 >> RAX: 00000000000f3b31 RBX: 1ffffffff4400fc7 RCX: ffffffffa09c3196 >> RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffffffff9f00590f >> RBP: 0000000000000000 R08: 0000000000000001 R09: ffffed102360835d >> R10: ffff88811b041aeb R11: 0000000000000001 R12: 0000000000000000 >> R13: ffffffffa202d7c0 R14: 0000000000000000 R15: 00000000000147d0 >> default_idle_call+0x6b/0xa0 >> cpuidle_idle_call+0x1af/0x1f0 >> do_idle+0xbc/0x130 >> cpu_startup_entry+0x33/0x40 >> rest_init+0x11f/0x210 >> start_kernel+0x39a/0x420 >> x86_64_start_reservations+0x18/0x30 >> x86_64_start_kernel+0x97/0xa0 >> common_startup_64+0x13e/0x141 >> </TASK> >> >> Allocated by task 595: >> kasan_save_stack+0x24/0x50 >> kasan_save_track+0x14/0x30 >> __kasan_slab_alloc+0x87/0x90 >> kmem_cache_alloc_noprof+0x12b/0x3f0 >> copy_net_ns+0x94/0x380 >> create_new_namespaces+0x24c/0x500 >> unshare_nsproxy_namespaces+0x75/0xf0 >> ksys_unshare+0x24e/0x4f0 >> __x64_sys_unshare+0x1f/0x30 >> do_syscall_64+0x70/0x180 >> entry_SYSCALL_64_after_hwframe+0x76/0x7e >> >> Freed by task 100: >> kasan_save_stack+0x24/0x50 >> kasan_save_track+0x14/0x30 >> kasan_save_free_info+0x3b/0x60 >> __kasan_slab_free+0x54/0x70 >> kmem_cache_free+0x156/0x5d0 >> cleanup_net+0x5d3/0x670 >> process_one_work+0x776/0xa90 >> worker_thread+0x2e2/0x560 >> kthread+0x1a8/0x1f0 >> ret_from_fork+0x34/0x60 >> ret_from_fork_asm+0x1a/0x30 >> >> Reproduction script: >> >> mkdir -p /mnt/nfsshare >> mkdir -p /mnt/nfs/netns_1 >> mkfs.ext4 /dev/sdb >> mount /dev/sdb /mnt/nfsshare >> systemctl restart nfs-server >> chmod 777 /mnt/nfsshare >> exportfs -i -o rw,no_root_squash *:/mnt/nfsshare >> >> ip netns add netns_1 >> ip link add name veth_1_peer type veth peer veth_1 >> ifconfig veth_1_peer 11.11.0.254 up >> ip link set veth_1 netns netns_1 >> ip netns exec netns_1 ifconfig veth_1 11.11.0.1 >> >> ip netns exec netns_1 /root/iptables -A OUTPUT -d 11.11.0.254 -p tcp \ >> --tcp-flags FIN FIN -j DROP >> >> (note: In my environment, a DESTROY_CLIENTID operation is always sent >> immediately, breaking the nfs tcp connection.) >> ip netns exec netns_1 timeout -s 9 300 mount -t nfs -o proto=tcp,vers=4.1 \ >> 11.11.0.254:/mnt/nfsshare /mnt/nfs/netns_1 >> >> ip netns del netns_1 >> >> The reason here is that the tcp socket in netns_1 (nfs side) has been >> shutdown and closed (done in xs_destroy), but the FIN message (with ack) >> is discarded, and the nfsd side keeps sending retransmission messages. >> As a result, when the tcp sock in netns_1 processes the received message, >> it sends the message (FIN message) in the sending queue, and the tcp timer >> is re-established. When the network namespace is deleted, the net structure >> accessed by tcp's timer handler function causes problems. >> >> To fix this problem: >> Add the sock_create_kern_getnet() helper function, add the get_net() >> operation for the kernel socket. >> >> Fixes: 26abe14379f8 ("net: Modify sk_alloc to not reference count the netns of kernel sockets.") >> Signed-off-by: Liu Jian <liujian56@huawei.com> >> --- >> v1: https://lore.kernel.org/all/20241024015543.568476-1-liujian56@huawei.com/ >> v1->v2: change to get netns reference count. >> include/linux/net.h | 1 + >> net/socket.c | 28 ++++++++++++++++++++++++++++ >> net/sunrpc/svcsock.c | 2 +- >> net/sunrpc/xprtsock.c | 2 +- >> 4 files changed, 31 insertions(+), 2 deletions(-) >> >> diff --git a/include/linux/net.h b/include/linux/net.h >> index b75bc534c1b3..58216da3b62c 100644 >> --- a/include/linux/net.h >> +++ b/include/linux/net.h >> @@ -255,6 +255,7 @@ int __sock_create(struct net *net, int family, int type, int proto, >> struct socket **res, int kern); >> int sock_create(int family, int type, int proto, struct socket **res); >> int sock_create_kern(struct net *net, int family, int type, int proto, struct socket **res); >> +int sock_create_kern_getnet(struct net *net, int family, int type, int proto, struct socket **res); >> int sock_create_lite(int family, int type, int proto, struct socket **res); >> struct socket *sock_alloc(void); >> void sock_release(struct socket *sock); >> diff --git a/net/socket.c b/net/socket.c >> index 042451f01c65..e64a02445b1a 100644 >> --- a/net/socket.c >> +++ b/net/socket.c >> @@ -1651,6 +1651,34 @@ int sock_create_kern(struct net *net, int family, int type, int protocol, struct >> } >> EXPORT_SYMBOL(sock_create_kern); >> >> +int sock_create_kern_getnet(struct net *net, int family, int type, int proto, struct socket **res) >> +{ >> + struct sock *sk; >> + int ret; >> + >> + if (!maybe_get_net(net)) >> + return -EINVAL; > > Is this really safe ? > > IIUC, maybe_get_net() is safe for a net only when it is fetched under > RCU, then rcu_read_lock() prevents cleanup_net() from reusing the net > by rcu_barrier(). > > Otherwise, there should be a small chance that the same slab object is > reused for another netns between fetching the net and reaching here. > > svc_create_socket() is called much later after the netns is fetched, > and _svc_xprt_create() calls try_module_get() before ->xpo_create(). > So, it seems the path is not under RCU and maybe_get_net() must be > called much earlier by each call site. > > For this reason, when I write a patch for the same issue in CIFS, > I delayed put_net() to cifsd kthread so that the netns refcnt taken > for each CIFS server info lives until the last __sock_create() attempt > from cifsd. > > https://lore.kernel.org/linux-cifs/20241102212438.76691-1-kuniyu@amazon.com/ > Okay, got it. thank you. Looking at the nfs and nfsd processing flow, it seems that the call to __sock_create() to create a TCP socket is always after the mount operation get_net(). So it should be fine to use get_net() directly. So here I'm going to change may_get_net() to get_net(), move sock_create_kern_getnet() to the sunrpc module, and rename it to something more appropriate. Is that okay? > >> + >> + ret = sock_create_kern(net, family, type, proto, res); >> + if (ret < 0) { >> + put_net(net); >> + return ret; >> + } >> + >> + sk = (*res)->sk; >> + lock_sock(sk); >> + /* Update ns_tracker to current stack trace and refcounted tracker */ >> + __netns_tracker_free(net, &sk->ns_tracker, false); >> + >> + sk->sk_net_refcnt = 1; >> + netns_tracker_alloc(net, &sk->ns_tracker, GFP_KERNEL); >> + sock_inuse_add(net, 1); >> + release_sock(sk); >> + >> + return ret; >> +} >> +EXPORT_SYMBOL(sock_create_kern_getnet); >> + >> static struct socket *__sys_socket_create(int family, int type, int protocol) >> { >> struct socket *sock; >> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c >> index 825ec5357691..6f272013fd9b 100644 >> --- a/net/sunrpc/svcsock.c >> +++ b/net/sunrpc/svcsock.c >> @@ -1526,7 +1526,7 @@ static struct svc_xprt *svc_create_socket(struct svc_serv *serv, >> return ERR_PTR(-EINVAL); >> } >> >> - error = __sock_create(net, family, type, protocol, &sock, 1); >> + error = sock_create_kern_getnet(net, family, type, protocol, &sock); >> if (error < 0) >> return ERR_PTR(error); >> >> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c >> index 110749b85040..f7734ce5eec9 100644 >> --- a/net/sunrpc/xprtsock.c >> +++ b/net/sunrpc/xprtsock.c >> @@ -1925,7 +1925,7 @@ static struct socket *xs_create_sock(struct rpc_xprt *xprt, >> struct socket *sock; >> int err; >> >> - err = __sock_create(xprt->xprt_net, family, type, protocol, &sock, 1); >> + err = sock_create_kern_getnet(xprt->xprt_net, family, type, protocol, &sock); >> if (err < 0) { >> dprintk("RPC: can't create %d transport socket (%d).\n", >> protocol, -err); >> -- >> 2.34.1 >
From: "liujian (CE)" <liujian56@huawei.com> Date: Thu, 7 Nov 2024 20:03:40 +0800 > >> diff --git a/net/socket.c b/net/socket.c > >> index 042451f01c65..e64a02445b1a 100644 > >> --- a/net/socket.c > >> +++ b/net/socket.c > >> @@ -1651,6 +1651,34 @@ int sock_create_kern(struct net *net, int family, int type, int protocol, struct > >> } > >> EXPORT_SYMBOL(sock_create_kern); > >> > >> +int sock_create_kern_getnet(struct net *net, int family, int type, int proto, struct socket **res) > >> +{ > >> + struct sock *sk; > >> + int ret; > >> + > >> + if (!maybe_get_net(net)) > >> + return -EINVAL; > > > > Is this really safe ? > > > > IIUC, maybe_get_net() is safe for a net only when it is fetched under > > RCU, then rcu_read_lock() prevents cleanup_net() from reusing the net > > by rcu_barrier(). > > > > Otherwise, there should be a small chance that the same slab object is > > reused for another netns between fetching the net and reaching here. > > > > svc_create_socket() is called much later after the netns is fetched, > > and _svc_xprt_create() calls try_module_get() before ->xpo_create(). > > So, it seems the path is not under RCU and maybe_get_net() must be > > called much earlier by each call site. > > > > For this reason, when I write a patch for the same issue in CIFS, > > I delayed put_net() to cifsd kthread so that the netns refcnt taken > > for each CIFS server info lives until the last __sock_create() attempt > > from cifsd. > > > > https://lore.kernel.org/linux-cifs/20241102212438.76691-1-kuniyu@amazon.com/ > > > Okay, got it. thank you. > Looking at the nfs and nfsd processing flow, it seems that the call to > __sock_create() to create a TCP socket is always after the mount > operation get_net(). So it should be fine to use get_net() directly. Is there any chance that a concurrent unmount releases the last refcount by put_net() while another thread trying to call __sock_create() ? CIFS was the case. > So > here I'm going to change may_get_net() to get_net(), move > sock_create_kern_getnet() to the sunrpc module, and rename it to > something more appropriate. Is that okay? Could you go without adding a helper and do the conversion in sunrpc code as CIFS did ? I plan to resurrect my patch and remove such socket conversion altogether in the next cycle after the CIFS fix lands on net-next. https://lore.kernel.org/netdev/20240227011041.97375-4-kuniyu@amazon.com/ https://github.com/q2ven/linux/commits/427_2 https://github.com/q2ven/linux/commit/2e54a8cc84f1e9ce60a0e4693c79a8e74c3dbeb9 I inspected all the callers of __sock_create() and friends, and all __sock_create() can be replaced with sock_create_kern(), so I will unexport __sock_create() and then add a new parameter hold_net to it. Then, I'll rename sock_create_kern() to sock_create_net_noref() and add a fat comment to catch in-kernel users attention so that they no longer use _kern() API blindly without care about netns reference. Also, I'll add sock_create_net() and use it for MPTCP, SMC, CIFS, (and sunrpc) etc. RDS uses maybe_net_get() but I think this is still buggy and I need to check more.
在 2024/11/8 4:59, Kuniyuki Iwashima 写道: > From: "liujian (CE)" <liujian56@huawei.com> > Date: Thu, 7 Nov 2024 20:03:40 +0800 >>>> diff --git a/net/socket.c b/net/socket.c >>>> index 042451f01c65..e64a02445b1a 100644 >>>> --- a/net/socket.c >>>> +++ b/net/socket.c >>>> @@ -1651,6 +1651,34 @@ int sock_create_kern(struct net *net, int family, int type, int protocol, struct >>>> } >>>> EXPORT_SYMBOL(sock_create_kern); >>>> >>>> +int sock_create_kern_getnet(struct net *net, int family, int type, int proto, struct socket **res) >>>> +{ >>>> + struct sock *sk; >>>> + int ret; >>>> + >>>> + if (!maybe_get_net(net)) >>>> + return -EINVAL; >>> >>> Is this really safe ? >>> >>> IIUC, maybe_get_net() is safe for a net only when it is fetched under >>> RCU, then rcu_read_lock() prevents cleanup_net() from reusing the net >>> by rcu_barrier(). >>> >>> Otherwise, there should be a small chance that the same slab object is >>> reused for another netns between fetching the net and reaching here. >>> >>> svc_create_socket() is called much later after the netns is fetched, >>> and _svc_xprt_create() calls try_module_get() before ->xpo_create(). >>> So, it seems the path is not under RCU and maybe_get_net() must be >>> called much earlier by each call site. >>> >>> For this reason, when I write a patch for the same issue in CIFS, >>> I delayed put_net() to cifsd kthread so that the netns refcnt taken >>> for each CIFS server info lives until the last __sock_create() attempt >>> from cifsd. >>> >>> https://lore.kernel.org/linux-cifs/20241102212438.76691-1-kuniyu@amazon.com/ >>> >> Okay, got it. thank you. >> Looking at the nfs and nfsd processing flow, it seems that the call to >> __sock_create() to create a TCP socket is always after the mount >> operation get_net(). So it should be fine to use get_net() directly. > > Is there any chance that a concurrent unmount releases the > last refcount by put_net() while another thread trying to call > __sock_create() ? > > CIFS was the case. > > >> So >> here I'm going to change may_get_net() to get_net(), move >> sock_create_kern_getnet() to the sunrpc module, and rename it to >> something more appropriate. Is that okay? > > Could you go without adding a helper and do the conversion in sunrpc > code as CIFS did ? > Ok, I will send v3 as you said. Looking forward to your changes as described below. Thank you. > I plan to resurrect my patch and remove such socket conversion altogether > in the next cycle after the CIFS fix lands on net-next. > > https://lore.kernel.org/netdev/20240227011041.97375-4-kuniyu@amazon.com/ > https://github.com/q2ven/linux/commits/427_2 > https://github.com/q2ven/linux/commit/2e54a8cc84f1e9ce60a0e4693c79a8e74c3dbeb9 > > I inspected all the callers of __sock_create() and friends, and all > __sock_create() can be replaced with sock_create_kern(), so I will > unexport __sock_create() and then add a new parameter hold_net to it. > > Then, I'll rename sock_create_kern() to sock_create_net_noref() and add > a fat comment to catch in-kernel users attention so that they no longer > use _kern() API blindly without care about netns reference. Also, I'll > add sock_create_net() and use it for MPTCP, SMC, CIFS, (and sunrpc) etc. > > RDS uses maybe_net_get() but I think this is still buggy and I need > to check more.
diff --git a/include/linux/net.h b/include/linux/net.h index b75bc534c1b3..58216da3b62c 100644 --- a/include/linux/net.h +++ b/include/linux/net.h @@ -255,6 +255,7 @@ int __sock_create(struct net *net, int family, int type, int proto, struct socket **res, int kern); int sock_create(int family, int type, int proto, struct socket **res); int sock_create_kern(struct net *net, int family, int type, int proto, struct socket **res); +int sock_create_kern_getnet(struct net *net, int family, int type, int proto, struct socket **res); int sock_create_lite(int family, int type, int proto, struct socket **res); struct socket *sock_alloc(void); void sock_release(struct socket *sock); diff --git a/net/socket.c b/net/socket.c index 042451f01c65..e64a02445b1a 100644 --- a/net/socket.c +++ b/net/socket.c @@ -1651,6 +1651,34 @@ int sock_create_kern(struct net *net, int family, int type, int protocol, struct } EXPORT_SYMBOL(sock_create_kern); +int sock_create_kern_getnet(struct net *net, int family, int type, int proto, struct socket **res) +{ + struct sock *sk; + int ret; + + if (!maybe_get_net(net)) + return -EINVAL; + + ret = sock_create_kern(net, family, type, proto, res); + if (ret < 0) { + put_net(net); + return ret; + } + + sk = (*res)->sk; + lock_sock(sk); + /* Update ns_tracker to current stack trace and refcounted tracker */ + __netns_tracker_free(net, &sk->ns_tracker, false); + + sk->sk_net_refcnt = 1; + netns_tracker_alloc(net, &sk->ns_tracker, GFP_KERNEL); + sock_inuse_add(net, 1); + release_sock(sk); + + return ret; +} +EXPORT_SYMBOL(sock_create_kern_getnet); + static struct socket *__sys_socket_create(int family, int type, int protocol) { struct socket *sock; diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c index 825ec5357691..6f272013fd9b 100644 --- a/net/sunrpc/svcsock.c +++ b/net/sunrpc/svcsock.c @@ -1526,7 +1526,7 @@ static struct svc_xprt *svc_create_socket(struct svc_serv *serv, return ERR_PTR(-EINVAL); } - error = __sock_create(net, family, type, protocol, &sock, 1); + error = sock_create_kern_getnet(net, family, type, protocol, &sock); if (error < 0) return ERR_PTR(error); diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c index 110749b85040..f7734ce5eec9 100644 --- a/net/sunrpc/xprtsock.c +++ b/net/sunrpc/xprtsock.c @@ -1925,7 +1925,7 @@ static struct socket *xs_create_sock(struct rpc_xprt *xprt, struct socket *sock; int err; - err = __sock_create(xprt->xprt_net, family, type, protocol, &sock, 1); + err = sock_create_kern_getnet(xprt->xprt_net, family, type, protocol, &sock); if (err < 0) { dprintk("RPC: can't create %d transport socket (%d).\n", protocol, -err);
BUG: KASAN: slab-use-after-free in tcp_write_timer_handler+0x156/0x3e0 Read of size 1 at addr ffff888111f322cd by task swapper/0/0 CPU: 0 UID: 0 PID: 0 Comm: swapper/0 Not tainted 6.12.0-rc4-dirty #7 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 Call Trace: <IRQ> dump_stack_lvl+0x68/0xa0 print_address_description.constprop.0+0x2c/0x3d0 print_report+0xb4/0x270 kasan_report+0xbd/0xf0 tcp_write_timer_handler+0x156/0x3e0 tcp_write_timer+0x66/0x170 call_timer_fn+0xfb/0x1d0 __run_timers+0x3f8/0x480 run_timer_softirq+0x9b/0x100 handle_softirqs+0x153/0x390 __irq_exit_rcu+0x103/0x120 irq_exit_rcu+0xe/0x20 sysvec_apic_timer_interrupt+0x76/0x90 </IRQ> <TASK> asm_sysvec_apic_timer_interrupt+0x1a/0x20 RIP: 0010:default_idle+0xf/0x20 Code: 4c 01 c7 4c 29 c2 e9 72 ff ff ff 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 f3 0f 1e fa 66 90 0f 00 2d 33 f8 25 00 fb f4 <fa> c3 cc cc cc cc 66 66 2e 0f 1f 84 00 00 00 00 00 90 90 90 90 90 RSP: 0018:ffffffffa2007e28 EFLAGS: 00000242 RAX: 00000000000f3b31 RBX: 1ffffffff4400fc7 RCX: ffffffffa09c3196 RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffffffff9f00590f RBP: 0000000000000000 R08: 0000000000000001 R09: ffffed102360835d R10: ffff88811b041aeb R11: 0000000000000001 R12: 0000000000000000 R13: ffffffffa202d7c0 R14: 0000000000000000 R15: 00000000000147d0 default_idle_call+0x6b/0xa0 cpuidle_idle_call+0x1af/0x1f0 do_idle+0xbc/0x130 cpu_startup_entry+0x33/0x40 rest_init+0x11f/0x210 start_kernel+0x39a/0x420 x86_64_start_reservations+0x18/0x30 x86_64_start_kernel+0x97/0xa0 common_startup_64+0x13e/0x141 </TASK> Allocated by task 595: kasan_save_stack+0x24/0x50 kasan_save_track+0x14/0x30 __kasan_slab_alloc+0x87/0x90 kmem_cache_alloc_noprof+0x12b/0x3f0 copy_net_ns+0x94/0x380 create_new_namespaces+0x24c/0x500 unshare_nsproxy_namespaces+0x75/0xf0 ksys_unshare+0x24e/0x4f0 __x64_sys_unshare+0x1f/0x30 do_syscall_64+0x70/0x180 entry_SYSCALL_64_after_hwframe+0x76/0x7e Freed by task 100: kasan_save_stack+0x24/0x50 kasan_save_track+0x14/0x30 kasan_save_free_info+0x3b/0x60 __kasan_slab_free+0x54/0x70 kmem_cache_free+0x156/0x5d0 cleanup_net+0x5d3/0x670 process_one_work+0x776/0xa90 worker_thread+0x2e2/0x560 kthread+0x1a8/0x1f0 ret_from_fork+0x34/0x60 ret_from_fork_asm+0x1a/0x30 Reproduction script: mkdir -p /mnt/nfsshare mkdir -p /mnt/nfs/netns_1 mkfs.ext4 /dev/sdb mount /dev/sdb /mnt/nfsshare systemctl restart nfs-server chmod 777 /mnt/nfsshare exportfs -i -o rw,no_root_squash *:/mnt/nfsshare ip netns add netns_1 ip link add name veth_1_peer type veth peer veth_1 ifconfig veth_1_peer 11.11.0.254 up ip link set veth_1 netns netns_1 ip netns exec netns_1 ifconfig veth_1 11.11.0.1 ip netns exec netns_1 /root/iptables -A OUTPUT -d 11.11.0.254 -p tcp \ --tcp-flags FIN FIN -j DROP (note: In my environment, a DESTROY_CLIENTID operation is always sent immediately, breaking the nfs tcp connection.) ip netns exec netns_1 timeout -s 9 300 mount -t nfs -o proto=tcp,vers=4.1 \ 11.11.0.254:/mnt/nfsshare /mnt/nfs/netns_1 ip netns del netns_1 The reason here is that the tcp socket in netns_1 (nfs side) has been shutdown and closed (done in xs_destroy), but the FIN message (with ack) is discarded, and the nfsd side keeps sending retransmission messages. As a result, when the tcp sock in netns_1 processes the received message, it sends the message (FIN message) in the sending queue, and the tcp timer is re-established. When the network namespace is deleted, the net structure accessed by tcp's timer handler function causes problems. To fix this problem: Add the sock_create_kern_getnet() helper function, add the get_net() operation for the kernel socket. Fixes: 26abe14379f8 ("net: Modify sk_alloc to not reference count the netns of kernel sockets.") Signed-off-by: Liu Jian <liujian56@huawei.com> --- v1: https://lore.kernel.org/all/20241024015543.568476-1-liujian56@huawei.com/ v1->v2: change to get netns reference count. include/linux/net.h | 1 + net/socket.c | 28 ++++++++++++++++++++++++++++ net/sunrpc/svcsock.c | 2 +- net/sunrpc/xprtsock.c | 2 +- 4 files changed, 31 insertions(+), 2 deletions(-)