| Message ID | 20250117163258.52885-1-okorniev@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] nfsd: fix management of listener transports | expand |
On Fri, 2025-01-17 at 11:32 -0500, Olga Kornievskaia wrote: > Currently, when no active threads are running, a root user using nfsdctl > command can try to remove a particular listener from the list of previously > added ones, then start the server by increasing the number of threads, > it leads to the following problem: > > [ 158.835354] refcount_t: addition on 0; use-after-free. > [ 158.835603] WARNING: CPU: 2 PID: 9145 at lib/refcount.c:25 refcount_warn_saturate+0x160/0x1a0 > [ 158.836017] Modules linked in: rpcrdma rdma_cm iw_cm ib_cm ib_core nfsd auth_rpcgss nfs_acl lockd grace overlay isofs uinput snd_seq_dummy snd_hrtimer nft_fib_inet nft_fib_ipv4 nft_fib_ipv6 nft_fib nft_reject_inet nf_reject_ipv4 nf_reject_ipv6 nft_reject nft_ct nft_chain_nat nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 rfkill ip_set nf_tables qrtr sunrpc vfat fat uvcvideo videobuf2_vmalloc videobuf2_memops uvc videobuf2_v4l2 videodev videobuf2_common snd_hda_codec_generic mc e1000e snd_hda_intel snd_intel_dspcfg snd_hda_codec snd_hda_core snd_hwdep snd_seq snd_seq_device snd_pcm snd_timer snd soundcore sg loop dm_multipath dm_mod nfnetlink vsock_loopback vmw_vsock_virtio_transport_common vmw_vsock_vmci_transport vmw_vmci vsock xfs libcrc32c crct10dif_ce ghash_ce vmwgfx sha2_ce sha256_arm64 sr_mod sha1_ce cdrom nvme drm_client_lib drm_ttm_helper ttm nvme_core drm_kms_helper nvme_auth drm fuse > [ 158.840093] CPU: 2 UID: 0 PID: 9145 Comm: nfsd Kdump: loaded Tainted: G B W 6.13.0-rc6+ #7 > [ 158.840624] Tainted: [B]=BAD_PAGE, [W]=WARN > [ 158.840802] Hardware name: VMware, Inc. VMware20,1/VBSA, BIOS VMW201.00V.24006586.BA64.2406042154 06/04/2024 > [ 158.841220] pstate: 61400005 (nZCv daif +PAN -UAO -TCO +DIT -SSBS BTYPE=--) > [ 158.841563] pc : refcount_warn_saturate+0x160/0x1a0 > [ 158.841780] lr : refcount_warn_saturate+0x160/0x1a0 > [ 158.842000] sp : ffff800089be7d80 > [ 158.842147] x29: ffff800089be7d80 x28: ffff00008e68c148 x27: ffff00008e68c148 > [ 158.842492] x26: ffff0002e3b5c000 x25: ffff600011cd1829 x24: ffff00008653c010 > [ 158.842832] x23: ffff00008653c000 x22: 1fffe00011cd1829 x21: ffff00008653c028 > [ 158.843175] x20: 0000000000000002 x19: ffff00008653c010 x18: 0000000000000000 > [ 158.843505] x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000 > [ 158.843836] x14: 0000000000000000 x13: 0000000000000001 x12: ffff600050a26493 > [ 158.844143] x11: 1fffe00050a26492 x10: ffff600050a26492 x9 : dfff800000000000 > [ 158.844475] x8 : 00009fffaf5d9b6e x7 : ffff000285132493 x6 : 0000000000000001 > [ 158.844823] x5 : ffff000285132490 x4 : ffff600050a26493 x3 : ffff8000805e72bc > [ 158.845174] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff000098588000 > [ 158.845528] Call trace: > [ 158.845658] refcount_warn_saturate+0x160/0x1a0 (P) > [ 158.845894] svc_recv+0x58c/0x680 [sunrpc] > [ 158.846183] nfsd+0x1fc/0x348 [nfsd] > [ 158.846390] kthread+0x274/0x2f8 > [ 158.846546] ret_from_fork+0x10/0x20 > [ 158.846714] ---[ end trace 0000000000000000 ]--- > > nfsd_nl_listener_set_doit() would manipulate the list of transports of > server's sv_permsocks and close the specified listener but the other > list of transports (server's sp_xprts list) would not be changed leading > to the problem above. > > Instead, determined if the nfsdctl is trying to remove a listener, in > which case, delete all the existing listener transports and re-create > all-but-the-removed ones. > > Fixes: 16a471177496 ("NFSD: add listener-{set,get} netlink command") > Signed-off-by: Olga Kornievskaia <okorniev@redhat.com> > --- > fs/nfsd/nfsctl.c | 41 ++++++++++++++++++----------------------- > 1 file changed, 18 insertions(+), 23 deletions(-) > > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c > index 95ea4393305b..079c1fe2eee7 100644 > --- a/fs/nfsd/nfsctl.c > +++ b/fs/nfsd/nfsctl.c > @@ -1918,6 +1918,7 @@ int nfsd_nl_listener_set_doit(struct sk_buff *skb, struct genl_info *info) > LIST_HEAD(permsocks); > struct nfsd_net *nn; > int err, rem; > + bool delete = false; > > mutex_lock(&nfsd_mutex); > > @@ -1977,35 +1978,26 @@ int nfsd_nl_listener_set_doit(struct sk_buff *skb, struct genl_info *info) > } > } > > - /* For now, no removing old sockets while server is running */ > - if (serv->sv_nrthreads && !list_empty(&permsocks)) { > + /* If we have listener transports left on permsocks list, it means > + * we are asked to remove a listener > + */ > + if (!list_empty(&permsocks)) { > list_splice_init(&permsocks, &serv->sv_permsocks); > - spin_unlock_bh(&serv->sv_lock); > - err = -EBUSY; > - goto out_unlock_mtx; > + delete = true; > } > + spin_unlock_bh(&serv->sv_lock); > > - /* Close the remaining sockets on the permsocks list */ > - while (!list_empty(&permsocks)) { > - xprt = list_first_entry(&permsocks, struct svc_xprt, xpt_list); > - list_move(&xprt->xpt_list, &serv->sv_permsocks); > - > - /* > - * Newly-created sockets are born with the BUSY bit set. Clear > - * it if there are no threads, since nothing can pick it up > - * in that case. > + /* Not removing old listener transports while server is running */ > + if (serv->sv_nrthreads) { > + err = -EBUSY; > + goto out_unlock_mtx; > + } else if (delete) { > + /* since we can't delete a single entry, we will destroy > + * all remaining listeners and recreate the list > */ > - if (!serv->sv_nrthreads) > - clear_bit(XPT_BUSY, &xprt->xpt_flags); > - > - set_bit(XPT_CLOSE, &xprt->xpt_flags); > - spin_unlock_bh(&serv->sv_lock); > - svc_xprt_close(xprt); > - spin_lock_bh(&serv->sv_lock); > + svc_xprt_destroy_all(serv, net); > } > > - spin_unlock_bh(&serv->sv_lock); > - > /* walk list of addrs again, open any that still don't exist */ > nlmsg_for_each_attr(attr, info->nlhdr, GENL_HDRLEN, rem) { > struct nlattr *tb[NFSD_A_SOCK_MAX + 1]; > @@ -2031,6 +2023,9 @@ int nfsd_nl_listener_set_doit(struct sk_buff *skb, struct genl_info *info) > > xprt = svc_find_listener(serv, xcl_name, net, sa); > if (xprt) { > + if (delete) > + WARN_ONCE("Transport type=%s already exists\n", > + xcl_name); > svc_xprt_put(xprt); > continue; > } This does seem a bit safer than trying to dequeue a since entry from the lwq. Reviewed-by: Jeff Layton <jlayton@kernel.org>
On Fri, Jan 17, 2025 at 11:43 AM Jeff Layton <jlayton@kernel.org> wrote: > > On Fri, 2025-01-17 at 11:32 -0500, Olga Kornievskaia wrote: > > Currently, when no active threads are running, a root user using nfsdctl > > command can try to remove a particular listener from the list of previously > > added ones, then start the server by increasing the number of threads, > > it leads to the following problem: > > > > [ 158.835354] refcount_t: addition on 0; use-after-free. > > [ 158.835603] WARNING: CPU: 2 PID: 9145 at lib/refcount.c:25 refcount_warn_saturate+0x160/0x1a0 > > [ 158.836017] Modules linked in: rpcrdma rdma_cm iw_cm ib_cm ib_core nfsd auth_rpcgss nfs_acl lockd grace overlay isofs uinput snd_seq_dummy snd_hrtimer nft_fib_inet nft_fib_ipv4 nft_fib_ipv6 nft_fib nft_reject_inet nf_reject_ipv4 nf_reject_ipv6 nft_reject nft_ct nft_chain_nat nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 rfkill ip_set nf_tables qrtr sunrpc vfat fat uvcvideo videobuf2_vmalloc videobuf2_memops uvc videobuf2_v4l2 videodev videobuf2_common snd_hda_codec_generic mc e1000e snd_hda_intel snd_intel_dspcfg snd_hda_codec snd_hda_core snd_hwdep snd_seq snd_seq_device snd_pcm snd_timer snd soundcore sg loop dm_multipath dm_mod nfnetlink vsock_loopback vmw_vsock_virtio_transport_common vmw_vsock_vmci_transport vmw_vmci vsock xfs libcrc32c crct10dif_ce ghash_ce vmwgfx sha2_ce sha256_arm64 sr_mod sha1_ce cdrom nvme drm_client_lib drm_ttm_helper ttm nvme_core drm_kms_helper nvme_auth drm fuse > > [ 158.840093] CPU: 2 UID: 0 PID: 9145 Comm: nfsd Kdump: loaded Tainted: G B W 6.13.0-rc6+ #7 > > [ 158.840624] Tainted: [B]=BAD_PAGE, [W]=WARN > > [ 158.840802] Hardware name: VMware, Inc. VMware20,1/VBSA, BIOS VMW201.00V.24006586.BA64.2406042154 06/04/2024 > > [ 158.841220] pstate: 61400005 (nZCv daif +PAN -UAO -TCO +DIT -SSBS BTYPE=--) > > [ 158.841563] pc : refcount_warn_saturate+0x160/0x1a0 > > [ 158.841780] lr : refcount_warn_saturate+0x160/0x1a0 > > [ 158.842000] sp : ffff800089be7d80 > > [ 158.842147] x29: ffff800089be7d80 x28: ffff00008e68c148 x27: ffff00008e68c148 > > [ 158.842492] x26: ffff0002e3b5c000 x25: ffff600011cd1829 x24: ffff00008653c010 > > [ 158.842832] x23: ffff00008653c000 x22: 1fffe00011cd1829 x21: ffff00008653c028 > > [ 158.843175] x20: 0000000000000002 x19: ffff00008653c010 x18: 0000000000000000 > > [ 158.843505] x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000 > > [ 158.843836] x14: 0000000000000000 x13: 0000000000000001 x12: ffff600050a26493 > > [ 158.844143] x11: 1fffe00050a26492 x10: ffff600050a26492 x9 : dfff800000000000 > > [ 158.844475] x8 : 00009fffaf5d9b6e x7 : ffff000285132493 x6 : 0000000000000001 > > [ 158.844823] x5 : ffff000285132490 x4 : ffff600050a26493 x3 : ffff8000805e72bc > > [ 158.845174] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff000098588000 > > [ 158.845528] Call trace: > > [ 158.845658] refcount_warn_saturate+0x160/0x1a0 (P) > > [ 158.845894] svc_recv+0x58c/0x680 [sunrpc] > > [ 158.846183] nfsd+0x1fc/0x348 [nfsd] > > [ 158.846390] kthread+0x274/0x2f8 > > [ 158.846546] ret_from_fork+0x10/0x20 > > [ 158.846714] ---[ end trace 0000000000000000 ]--- > > > > nfsd_nl_listener_set_doit() would manipulate the list of transports of > > server's sv_permsocks and close the specified listener but the other > > list of transports (server's sp_xprts list) would not be changed leading > > to the problem above. > > > > Instead, determined if the nfsdctl is trying to remove a listener, in > > which case, delete all the existing listener transports and re-create > > all-but-the-removed ones. > > > > Fixes: 16a471177496 ("NFSD: add listener-{set,get} netlink command") > > Signed-off-by: Olga Kornievskaia <okorniev@redhat.com> > > --- > > fs/nfsd/nfsctl.c | 41 ++++++++++++++++++----------------------- > > 1 file changed, 18 insertions(+), 23 deletions(-) > > > > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c > > index 95ea4393305b..079c1fe2eee7 100644 > > --- a/fs/nfsd/nfsctl.c > > +++ b/fs/nfsd/nfsctl.c > > @@ -1918,6 +1918,7 @@ int nfsd_nl_listener_set_doit(struct sk_buff *skb, struct genl_info *info) > > LIST_HEAD(permsocks); > > struct nfsd_net *nn; > > int err, rem; > > + bool delete = false; > > > > mutex_lock(&nfsd_mutex); > > > > @@ -1977,35 +1978,26 @@ int nfsd_nl_listener_set_doit(struct sk_buff *skb, struct genl_info *info) > > } > > } > > > > - /* For now, no removing old sockets while server is running */ > > - if (serv->sv_nrthreads && !list_empty(&permsocks)) { > > + /* If we have listener transports left on permsocks list, it means > > + * we are asked to remove a listener > > + */ > > + if (!list_empty(&permsocks)) { > > list_splice_init(&permsocks, &serv->sv_permsocks); > > - spin_unlock_bh(&serv->sv_lock); > > - err = -EBUSY; > > - goto out_unlock_mtx; > > + delete = true; > > } > > + spin_unlock_bh(&serv->sv_lock); > > > > - /* Close the remaining sockets on the permsocks list */ > > - while (!list_empty(&permsocks)) { > > - xprt = list_first_entry(&permsocks, struct svc_xprt, xpt_list); > > - list_move(&xprt->xpt_list, &serv->sv_permsocks); > > - > > - /* > > - * Newly-created sockets are born with the BUSY bit set. Clear > > - * it if there are no threads, since nothing can pick it up > > - * in that case. > > + /* Not removing old listener transports while server is running */ > > + if (serv->sv_nrthreads) { > > + err = -EBUSY; > > + goto out_unlock_mtx; > > + } else if (delete) { > > + /* since we can't delete a single entry, we will destroy > > + * all remaining listeners and recreate the list > > */ > > - if (!serv->sv_nrthreads) > > - clear_bit(XPT_BUSY, &xprt->xpt_flags); > > - > > - set_bit(XPT_CLOSE, &xprt->xpt_flags); > > - spin_unlock_bh(&serv->sv_lock); > > - svc_xprt_close(xprt); > > - spin_lock_bh(&serv->sv_lock); > > + svc_xprt_destroy_all(serv, net); > > } > > > > - spin_unlock_bh(&serv->sv_lock); > > - > > /* walk list of addrs again, open any that still don't exist */ > > nlmsg_for_each_attr(attr, info->nlhdr, GENL_HDRLEN, rem) { > > struct nlattr *tb[NFSD_A_SOCK_MAX + 1]; > > @@ -2031,6 +2023,9 @@ int nfsd_nl_listener_set_doit(struct sk_buff *skb, struct genl_info *info) > > > > xprt = svc_find_listener(serv, xcl_name, net, sa); > > if (xprt) { > > + if (delete) > > + WARN_ONCE("Transport type=%s already exists\n", > > + xcl_name); > > svc_xprt_put(xprt); > > continue; > > } > > This does seem a bit safer than trying to dequeue a since entry from > the lwq. To be honest I don't understand the value in being able to remove a listener. There has to be no active threads. Then somebody has to do nfsdctl listener +<protocol>... but then decide oh wait I dont need it and do listener -<protocol>... then increase the thread count. They can (-) listener by running "nfsdctl threads 0" again and that clears all the listeners anyway. Is the ultimate goal to remove a listener on an active server? If there isn't such a goal, it seems better to remove the ability altogether. > > Reviewed-by: Jeff Layton <jlayton@kernel.org> >
On Fri, 2025-01-17 at 11:56 -0500, Olga Kornievskaia wrote: > On Fri, Jan 17, 2025 at 11:43 AM Jeff Layton <jlayton@kernel.org> wrote: > > > > On Fri, 2025-01-17 at 11:32 -0500, Olga Kornievskaia wrote: > > > Currently, when no active threads are running, a root user using nfsdctl > > > command can try to remove a particular listener from the list of previously > > > added ones, then start the server by increasing the number of threads, > > > it leads to the following problem: > > > > > > [ 158.835354] refcount_t: addition on 0; use-after-free. > > > [ 158.835603] WARNING: CPU: 2 PID: 9145 at lib/refcount.c:25 refcount_warn_saturate+0x160/0x1a0 > > > [ 158.836017] Modules linked in: rpcrdma rdma_cm iw_cm ib_cm ib_core nfsd auth_rpcgss nfs_acl lockd grace overlay isofs uinput snd_seq_dummy snd_hrtimer nft_fib_inet nft_fib_ipv4 nft_fib_ipv6 nft_fib nft_reject_inet nf_reject_ipv4 nf_reject_ipv6 nft_reject nft_ct nft_chain_nat nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 rfkill ip_set nf_tables qrtr sunrpc vfat fat uvcvideo videobuf2_vmalloc videobuf2_memops uvc videobuf2_v4l2 videodev videobuf2_common snd_hda_codec_generic mc e1000e snd_hda_intel snd_intel_dspcfg snd_hda_codec snd_hda_core snd_hwdep snd_seq snd_seq_device snd_pcm snd_timer snd soundcore sg loop dm_multipath dm_mod nfnetlink vsock_loopback vmw_vsock_virtio_transport_common vmw_vsock_vmci_transport vmw_vmci vsock xfs libcrc32c crct10dif_ce ghash_ce vmwgfx sha2_ce sha256_arm64 sr_mod sha1_ce cdrom nvme drm_client_lib drm_ttm_helper ttm nvme_core drm_kms_helper nvme_auth drm fuse > > > [ 158.840093] CPU: 2 UID: 0 PID: 9145 Comm: nfsd Kdump: loaded Tainted: G B W 6.13.0-rc6+ #7 > > > [ 158.840624] Tainted: [B]=BAD_PAGE, [W]=WARN > > > [ 158.840802] Hardware name: VMware, Inc. VMware20,1/VBSA, BIOS VMW201.00V.24006586.BA64.2406042154 06/04/2024 > > > [ 158.841220] pstate: 61400005 (nZCv daif +PAN -UAO -TCO +DIT -SSBS BTYPE=--) > > > [ 158.841563] pc : refcount_warn_saturate+0x160/0x1a0 > > > [ 158.841780] lr : refcount_warn_saturate+0x160/0x1a0 > > > [ 158.842000] sp : ffff800089be7d80 > > > [ 158.842147] x29: ffff800089be7d80 x28: ffff00008e68c148 x27: ffff00008e68c148 > > > [ 158.842492] x26: ffff0002e3b5c000 x25: ffff600011cd1829 x24: ffff00008653c010 > > > [ 158.842832] x23: ffff00008653c000 x22: 1fffe00011cd1829 x21: ffff00008653c028 > > > [ 158.843175] x20: 0000000000000002 x19: ffff00008653c010 x18: 0000000000000000 > > > [ 158.843505] x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000 > > > [ 158.843836] x14: 0000000000000000 x13: 0000000000000001 x12: ffff600050a26493 > > > [ 158.844143] x11: 1fffe00050a26492 x10: ffff600050a26492 x9 : dfff800000000000 > > > [ 158.844475] x8 : 00009fffaf5d9b6e x7 : ffff000285132493 x6 : 0000000000000001 > > > [ 158.844823] x5 : ffff000285132490 x4 : ffff600050a26493 x3 : ffff8000805e72bc > > > [ 158.845174] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff000098588000 > > > [ 158.845528] Call trace: > > > [ 158.845658] refcount_warn_saturate+0x160/0x1a0 (P) > > > [ 158.845894] svc_recv+0x58c/0x680 [sunrpc] > > > [ 158.846183] nfsd+0x1fc/0x348 [nfsd] > > > [ 158.846390] kthread+0x274/0x2f8 > > > [ 158.846546] ret_from_fork+0x10/0x20 > > > [ 158.846714] ---[ end trace 0000000000000000 ]--- > > > > > > nfsd_nl_listener_set_doit() would manipulate the list of transports of > > > server's sv_permsocks and close the specified listener but the other > > > list of transports (server's sp_xprts list) would not be changed leading > > > to the problem above. > > > > > > Instead, determined if the nfsdctl is trying to remove a listener, in > > > which case, delete all the existing listener transports and re-create > > > all-but-the-removed ones. > > > > > > Fixes: 16a471177496 ("NFSD: add listener-{set,get} netlink command") > > > Signed-off-by: Olga Kornievskaia <okorniev@redhat.com> > > > --- > > > fs/nfsd/nfsctl.c | 41 ++++++++++++++++++----------------------- > > > 1 file changed, 18 insertions(+), 23 deletions(-) > > > > > > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c > > > index 95ea4393305b..079c1fe2eee7 100644 > > > --- a/fs/nfsd/nfsctl.c > > > +++ b/fs/nfsd/nfsctl.c > > > @@ -1918,6 +1918,7 @@ int nfsd_nl_listener_set_doit(struct sk_buff *skb, struct genl_info *info) > > > LIST_HEAD(permsocks); > > > struct nfsd_net *nn; > > > int err, rem; > > > + bool delete = false; > > > > > > mutex_lock(&nfsd_mutex); > > > > > > @@ -1977,35 +1978,26 @@ int nfsd_nl_listener_set_doit(struct sk_buff *skb, struct genl_info *info) > > > } > > > } > > > > > > - /* For now, no removing old sockets while server is running */ > > > - if (serv->sv_nrthreads && !list_empty(&permsocks)) { > > > + /* If we have listener transports left on permsocks list, it means > > > + * we are asked to remove a listener > > > + */ > > > + if (!list_empty(&permsocks)) { > > > list_splice_init(&permsocks, &serv->sv_permsocks); > > > - spin_unlock_bh(&serv->sv_lock); > > > - err = -EBUSY; > > > - goto out_unlock_mtx; > > > + delete = true; > > > } > > > + spin_unlock_bh(&serv->sv_lock); > > > > > > - /* Close the remaining sockets on the permsocks list */ > > > - while (!list_empty(&permsocks)) { > > > - xprt = list_first_entry(&permsocks, struct svc_xprt, xpt_list); > > > - list_move(&xprt->xpt_list, &serv->sv_permsocks); > > > - > > > - /* > > > - * Newly-created sockets are born with the BUSY bit set. Clear > > > - * it if there are no threads, since nothing can pick it up > > > - * in that case. > > > + /* Not removing old listener transports while server is running */ > > > + if (serv->sv_nrthreads) { > > > + err = -EBUSY; > > > + goto out_unlock_mtx; > > > + } else if (delete) { > > > + /* since we can't delete a single entry, we will destroy > > > + * all remaining listeners and recreate the list > > > */ > > > - if (!serv->sv_nrthreads) > > > - clear_bit(XPT_BUSY, &xprt->xpt_flags); > > > - > > > - set_bit(XPT_CLOSE, &xprt->xpt_flags); > > > - spin_unlock_bh(&serv->sv_lock); > > > - svc_xprt_close(xprt); > > > - spin_lock_bh(&serv->sv_lock); > > > + svc_xprt_destroy_all(serv, net); > > > } > > > > > > - spin_unlock_bh(&serv->sv_lock); > > > - > > > /* walk list of addrs again, open any that still don't exist */ > > > nlmsg_for_each_attr(attr, info->nlhdr, GENL_HDRLEN, rem) { > > > struct nlattr *tb[NFSD_A_SOCK_MAX + 1]; > > > @@ -2031,6 +2023,9 @@ int nfsd_nl_listener_set_doit(struct sk_buff *skb, struct genl_info *info) > > > > > > xprt = svc_find_listener(serv, xcl_name, net, sa); > > > if (xprt) { > > > + if (delete) > > > + WARN_ONCE("Transport type=%s already exists\n", > > > + xcl_name); > > > svc_xprt_put(xprt); > > > continue; > > > } > > > > This does seem a bit safer than trying to dequeue a since entry from > > the lwq. > > To be honest I don't understand the value in being able to remove a > listener. There has to be no active threads. Then somebody has to do > nfsdctl listener +<protocol>... but then decide oh wait I dont need it > and do listener -<protocol>... then increase the thread count. They > can (-) listener by running "nfsdctl threads 0" again and that clears > all the listeners anyway. > > Is the ultimate goal to remove a listener on an active server? If > there isn't such a goal, it seems better to remove the ability > altogether. > > Mostly I added that because you could end up making a mistake when adding a listener, so I thought that being able to remove one would be helpful. If the consensus is that it's not helpful, then I'm fine with dropping that functionality. If you do that though, then I think you need some mechanism that is more intuitive to clear the list of listeners than "threads 0".
On 1/17/25 12:00 PM, Jeff Layton wrote: > On Fri, 2025-01-17 at 11:56 -0500, Olga Kornievskaia wrote: >> On Fri, Jan 17, 2025 at 11:43 AM Jeff Layton <jlayton@kernel.org> wrote: >>> >>> On Fri, 2025-01-17 at 11:32 -0500, Olga Kornievskaia wrote: >>>> Currently, when no active threads are running, a root user using nfsdctl >>>> command can try to remove a particular listener from the list of previously >>>> added ones, then start the server by increasing the number of threads, >>>> it leads to the following problem: >>>> >>>> [ 158.835354] refcount_t: addition on 0; use-after-free. >>>> [ 158.835603] WARNING: CPU: 2 PID: 9145 at lib/refcount.c:25 refcount_warn_saturate+0x160/0x1a0 >>>> [ 158.836017] Modules linked in: rpcrdma rdma_cm iw_cm ib_cm ib_core nfsd auth_rpcgss nfs_acl lockd grace overlay isofs uinput snd_seq_dummy snd_hrtimer nft_fib_inet nft_fib_ipv4 nft_fib_ipv6 nft_fib nft_reject_inet nf_reject_ipv4 nf_reject_ipv6 nft_reject nft_ct nft_chain_nat nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 rfkill ip_set nf_tables qrtr sunrpc vfat fat uvcvideo videobuf2_vmalloc videobuf2_memops uvc videobuf2_v4l2 videodev videobuf2_common snd_hda_codec_generic mc e1000e snd_hda_intel snd_intel_dspcfg snd_hda_codec snd_hda_core snd_hwdep snd_seq snd_seq_device snd_pcm snd_timer snd soundcore sg loop dm_multipath dm_mod nfnetlink vsock_loopback vmw_vsock_virtio_transport_common vmw_vsock_vmci_transport vmw_vmci vsock xfs libcrc32c crct10dif_ce ghash_ce vmwgfx sha2_ce sha256_arm64 sr_mod sha1_ce cdrom nvme drm_client_lib drm_ttm_helper ttm nvme_core drm_kms_helper nvme_auth drm fuse >>>> [ 158.840093] CPU: 2 UID: 0 PID: 9145 Comm: nfsd Kdump: loaded Tainted: G B W 6.13.0-rc6+ #7 >>>> [ 158.840624] Tainted: [B]=BAD_PAGE, [W]=WARN >>>> [ 158.840802] Hardware name: VMware, Inc. VMware20,1/VBSA, BIOS VMW201.00V.24006586.BA64.2406042154 06/04/2024 >>>> [ 158.841220] pstate: 61400005 (nZCv daif +PAN -UAO -TCO +DIT -SSBS BTYPE=--) >>>> [ 158.841563] pc : refcount_warn_saturate+0x160/0x1a0 >>>> [ 158.841780] lr : refcount_warn_saturate+0x160/0x1a0 >>>> [ 158.842000] sp : ffff800089be7d80 >>>> [ 158.842147] x29: ffff800089be7d80 x28: ffff00008e68c148 x27: ffff00008e68c148 >>>> [ 158.842492] x26: ffff0002e3b5c000 x25: ffff600011cd1829 x24: ffff00008653c010 >>>> [ 158.842832] x23: ffff00008653c000 x22: 1fffe00011cd1829 x21: ffff00008653c028 >>>> [ 158.843175] x20: 0000000000000002 x19: ffff00008653c010 x18: 0000000000000000 >>>> [ 158.843505] x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000 >>>> [ 158.843836] x14: 0000000000000000 x13: 0000000000000001 x12: ffff600050a26493 >>>> [ 158.844143] x11: 1fffe00050a26492 x10: ffff600050a26492 x9 : dfff800000000000 >>>> [ 158.844475] x8 : 00009fffaf5d9b6e x7 : ffff000285132493 x6 : 0000000000000001 >>>> [ 158.844823] x5 : ffff000285132490 x4 : ffff600050a26493 x3 : ffff8000805e72bc >>>> [ 158.845174] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff000098588000 >>>> [ 158.845528] Call trace: >>>> [ 158.845658] refcount_warn_saturate+0x160/0x1a0 (P) >>>> [ 158.845894] svc_recv+0x58c/0x680 [sunrpc] >>>> [ 158.846183] nfsd+0x1fc/0x348 [nfsd] >>>> [ 158.846390] kthread+0x274/0x2f8 >>>> [ 158.846546] ret_from_fork+0x10/0x20 >>>> [ 158.846714] ---[ end trace 0000000000000000 ]--- >>>> >>>> nfsd_nl_listener_set_doit() would manipulate the list of transports of >>>> server's sv_permsocks and close the specified listener but the other >>>> list of transports (server's sp_xprts list) would not be changed leading >>>> to the problem above. >>>> >>>> Instead, determined if the nfsdctl is trying to remove a listener, in >>>> which case, delete all the existing listener transports and re-create >>>> all-but-the-removed ones. >>>> >>>> Fixes: 16a471177496 ("NFSD: add listener-{set,get} netlink command") >>>> Signed-off-by: Olga Kornievskaia <okorniev@redhat.com> >>>> --- >>>> fs/nfsd/nfsctl.c | 41 ++++++++++++++++++----------------------- >>>> 1 file changed, 18 insertions(+), 23 deletions(-) >>>> >>>> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c >>>> index 95ea4393305b..079c1fe2eee7 100644 >>>> --- a/fs/nfsd/nfsctl.c >>>> +++ b/fs/nfsd/nfsctl.c >>>> @@ -1918,6 +1918,7 @@ int nfsd_nl_listener_set_doit(struct sk_buff *skb, struct genl_info *info) >>>> LIST_HEAD(permsocks); >>>> struct nfsd_net *nn; >>>> int err, rem; >>>> + bool delete = false; >>>> >>>> mutex_lock(&nfsd_mutex); >>>> >>>> @@ -1977,35 +1978,26 @@ int nfsd_nl_listener_set_doit(struct sk_buff *skb, struct genl_info *info) >>>> } >>>> } >>>> >>>> - /* For now, no removing old sockets while server is running */ >>>> - if (serv->sv_nrthreads && !list_empty(&permsocks)) { >>>> + /* If we have listener transports left on permsocks list, it means >>>> + * we are asked to remove a listener >>>> + */ >>>> + if (!list_empty(&permsocks)) { >>>> list_splice_init(&permsocks, &serv->sv_permsocks); >>>> - spin_unlock_bh(&serv->sv_lock); >>>> - err = -EBUSY; >>>> - goto out_unlock_mtx; >>>> + delete = true; >>>> } >>>> + spin_unlock_bh(&serv->sv_lock); >>>> >>>> - /* Close the remaining sockets on the permsocks list */ >>>> - while (!list_empty(&permsocks)) { >>>> - xprt = list_first_entry(&permsocks, struct svc_xprt, xpt_list); >>>> - list_move(&xprt->xpt_list, &serv->sv_permsocks); >>>> - >>>> - /* >>>> - * Newly-created sockets are born with the BUSY bit set. Clear >>>> - * it if there are no threads, since nothing can pick it up >>>> - * in that case. >>>> + /* Not removing old listener transports while server is running */ >>>> + if (serv->sv_nrthreads) { >>>> + err = -EBUSY; >>>> + goto out_unlock_mtx; >>>> + } else if (delete) { >>>> + /* since we can't delete a single entry, we will destroy >>>> + * all remaining listeners and recreate the list >>>> */ >>>> - if (!serv->sv_nrthreads) >>>> - clear_bit(XPT_BUSY, &xprt->xpt_flags); >>>> - >>>> - set_bit(XPT_CLOSE, &xprt->xpt_flags); >>>> - spin_unlock_bh(&serv->sv_lock); >>>> - svc_xprt_close(xprt); >>>> - spin_lock_bh(&serv->sv_lock); >>>> + svc_xprt_destroy_all(serv, net); >>>> } >>>> >>>> - spin_unlock_bh(&serv->sv_lock); >>>> - >>>> /* walk list of addrs again, open any that still don't exist */ >>>> nlmsg_for_each_attr(attr, info->nlhdr, GENL_HDRLEN, rem) { >>>> struct nlattr *tb[NFSD_A_SOCK_MAX + 1]; >>>> @@ -2031,6 +2023,9 @@ int nfsd_nl_listener_set_doit(struct sk_buff *skb, struct genl_info *info) >>>> >>>> xprt = svc_find_listener(serv, xcl_name, net, sa); >>>> if (xprt) { >>>> + if (delete) >>>> + WARN_ONCE("Transport type=%s already exists\n", >>>> + xcl_name); >>>> svc_xprt_put(xprt); >>>> continue; >>>> } >>> >>> This does seem a bit safer than trying to dequeue a since entry from >>> the lwq. >> >> To be honest I don't understand the value in being able to remove a >> listener. There has to be no active threads. Then somebody has to do >> nfsdctl listener +<protocol>... but then decide oh wait I dont need it >> and do listener -<protocol>... then increase the thread count. They >> can (-) listener by running "nfsdctl threads 0" again and that clears >> all the listeners anyway. >> >> Is the ultimate goal to remove a listener on an active server? If >> there isn't such a goal, it seems better to remove the ability >> altogether. >> >> > > Mostly I added that because you could end up making a mistake when > adding a listener, so I thought that being able to remove one would be > helpful. > > If the consensus is that it's not helpful, then I'm fine with dropping > that functionality. If you do that though, then I think you need some > mechanism that is more intuitive to clear the list of listeners than > "threads 0". > My sense is that automated CI/CD will want to create and destroy NFS services, and that will need a mechanism to remove a listener that is no longer wanted without disturbing other active listeners. I can see this being useful in a containers environment, for instance.
From: Chuck Lever <chuck.lever@oracle.com> On Fri, 17 Jan 2025 11:32:58 -0500, Olga Kornievskaia wrote: > Currently, when no active threads are running, a root user using nfsdctl > command can try to remove a particular listener from the list of previously > added ones, then start the server by increasing the number of threads, > it leads to the following problem: > > [...] Applied to nfsd-testing, thanks! This looks much better to me! [1/1] nfsd: fix management of listener transports commit: ea27619ed650d8f62cb9d4a9f3c6e8132e8f24fd -- Chuck Lever
diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c index 95ea4393305b..079c1fe2eee7 100644 --- a/fs/nfsd/nfsctl.c +++ b/fs/nfsd/nfsctl.c @@ -1918,6 +1918,7 @@ int nfsd_nl_listener_set_doit(struct sk_buff *skb, struct genl_info *info) LIST_HEAD(permsocks); struct nfsd_net *nn; int err, rem; + bool delete = false; mutex_lock(&nfsd_mutex); @@ -1977,35 +1978,26 @@ int nfsd_nl_listener_set_doit(struct sk_buff *skb, struct genl_info *info) } } - /* For now, no removing old sockets while server is running */ - if (serv->sv_nrthreads && !list_empty(&permsocks)) { + /* If we have listener transports left on permsocks list, it means + * we are asked to remove a listener + */ + if (!list_empty(&permsocks)) { list_splice_init(&permsocks, &serv->sv_permsocks); - spin_unlock_bh(&serv->sv_lock); - err = -EBUSY; - goto out_unlock_mtx; + delete = true; } + spin_unlock_bh(&serv->sv_lock); - /* Close the remaining sockets on the permsocks list */ - while (!list_empty(&permsocks)) { - xprt = list_first_entry(&permsocks, struct svc_xprt, xpt_list); - list_move(&xprt->xpt_list, &serv->sv_permsocks); - - /* - * Newly-created sockets are born with the BUSY bit set. Clear - * it if there are no threads, since nothing can pick it up - * in that case. + /* Not removing old listener transports while server is running */ + if (serv->sv_nrthreads) { + err = -EBUSY; + goto out_unlock_mtx; + } else if (delete) { + /* since we can't delete a single entry, we will destroy + * all remaining listeners and recreate the list */ - if (!serv->sv_nrthreads) - clear_bit(XPT_BUSY, &xprt->xpt_flags); - - set_bit(XPT_CLOSE, &xprt->xpt_flags); - spin_unlock_bh(&serv->sv_lock); - svc_xprt_close(xprt); - spin_lock_bh(&serv->sv_lock); + svc_xprt_destroy_all(serv, net); } - spin_unlock_bh(&serv->sv_lock); - /* walk list of addrs again, open any that still don't exist */ nlmsg_for_each_attr(attr, info->nlhdr, GENL_HDRLEN, rem) { struct nlattr *tb[NFSD_A_SOCK_MAX + 1]; @@ -2031,6 +2023,9 @@ int nfsd_nl_listener_set_doit(struct sk_buff *skb, struct genl_info *info) xprt = svc_find_listener(serv, xcl_name, net, sa); if (xprt) { + if (delete) + WARN_ONCE("Transport type=%s already exists\n", + xcl_name); svc_xprt_put(xprt); continue; }
Currently, when no active threads are running, a root user using nfsdctl command can try to remove a particular listener from the list of previously added ones, then start the server by increasing the number of threads, it leads to the following problem: [ 158.835354] refcount_t: addition on 0; use-after-free. [ 158.835603] WARNING: CPU: 2 PID: 9145 at lib/refcount.c:25 refcount_warn_saturate+0x160/0x1a0 [ 158.836017] Modules linked in: rpcrdma rdma_cm iw_cm ib_cm ib_core nfsd auth_rpcgss nfs_acl lockd grace overlay isofs uinput snd_seq_dummy snd_hrtimer nft_fib_inet nft_fib_ipv4 nft_fib_ipv6 nft_fib nft_reject_inet nf_reject_ipv4 nf_reject_ipv6 nft_reject nft_ct nft_chain_nat nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 rfkill ip_set nf_tables qrtr sunrpc vfat fat uvcvideo videobuf2_vmalloc videobuf2_memops uvc videobuf2_v4l2 videodev videobuf2_common snd_hda_codec_generic mc e1000e snd_hda_intel snd_intel_dspcfg snd_hda_codec snd_hda_core snd_hwdep snd_seq snd_seq_device snd_pcm snd_timer snd soundcore sg loop dm_multipath dm_mod nfnetlink vsock_loopback vmw_vsock_virtio_transport_common vmw_vsock_vmci_transport vmw_vmci vsock xfs libcrc32c crct10dif_ce ghash_ce vmwgfx sha2_ce sha256_arm64 sr_mod sha1_ce cdrom nvme drm_client_lib drm_ttm_helper ttm nvme_core drm_kms_helper nvme_auth drm fuse [ 158.840093] CPU: 2 UID: 0 PID: 9145 Comm: nfsd Kdump: loaded Tainted: G B W 6.13.0-rc6+ #7 [ 158.840624] Tainted: [B]=BAD_PAGE, [W]=WARN [ 158.840802] Hardware name: VMware, Inc. VMware20,1/VBSA, BIOS VMW201.00V.24006586.BA64.2406042154 06/04/2024 [ 158.841220] pstate: 61400005 (nZCv daif +PAN -UAO -TCO +DIT -SSBS BTYPE=--) [ 158.841563] pc : refcount_warn_saturate+0x160/0x1a0 [ 158.841780] lr : refcount_warn_saturate+0x160/0x1a0 [ 158.842000] sp : ffff800089be7d80 [ 158.842147] x29: ffff800089be7d80 x28: ffff00008e68c148 x27: ffff00008e68c148 [ 158.842492] x26: ffff0002e3b5c000 x25: ffff600011cd1829 x24: ffff00008653c010 [ 158.842832] x23: ffff00008653c000 x22: 1fffe00011cd1829 x21: ffff00008653c028 [ 158.843175] x20: 0000000000000002 x19: ffff00008653c010 x18: 0000000000000000 [ 158.843505] x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000 [ 158.843836] x14: 0000000000000000 x13: 0000000000000001 x12: ffff600050a26493 [ 158.844143] x11: 1fffe00050a26492 x10: ffff600050a26492 x9 : dfff800000000000 [ 158.844475] x8 : 00009fffaf5d9b6e x7 : ffff000285132493 x6 : 0000000000000001 [ 158.844823] x5 : ffff000285132490 x4 : ffff600050a26493 x3 : ffff8000805e72bc [ 158.845174] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff000098588000 [ 158.845528] Call trace: [ 158.845658] refcount_warn_saturate+0x160/0x1a0 (P) [ 158.845894] svc_recv+0x58c/0x680 [sunrpc] [ 158.846183] nfsd+0x1fc/0x348 [nfsd] [ 158.846390] kthread+0x274/0x2f8 [ 158.846546] ret_from_fork+0x10/0x20 [ 158.846714] ---[ end trace 0000000000000000 ]--- nfsd_nl_listener_set_doit() would manipulate the list of transports of server's sv_permsocks and close the specified listener but the other list of transports (server's sp_xprts list) would not be changed leading to the problem above. Instead, determined if the nfsdctl is trying to remove a listener, in which case, delete all the existing listener transports and re-create all-but-the-removed ones. Fixes: 16a471177496 ("NFSD: add listener-{set,get} netlink command") Signed-off-by: Olga Kornievskaia <okorniev@redhat.com> --- fs/nfsd/nfsctl.c | 41 ++++++++++++++++++----------------------- 1 file changed, 18 insertions(+), 23 deletions(-)