[PATCH 0/3] fix removal of nfsd listeners

[PATCH 0/3] fix removal of nfsd listeners

[Olga Kornievskaia]

Currently if a root user using nfsdctl command tries to remove a particular
listener from the list of previously added ones, then starting the nfsd
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 svc_xprt_close() the specified listener but
the other list of transports (server's sp_xprts list) would not be
changed leading to the problem.

the other problem is that sp_xprt is a lwq structure of lockless
list which does not have an ability to remove a single entry from
the list.

this patch series addis a function to remove a single entry, then modifies
nfsd_nl_listener_set_doit() to make sure the to-be-removed listener is
removed from both lists and then it also ensures that the remaining
listeners are added back in the correct state.

Olga Kornievskaia (3):
  llist: add ability to remove a particular entry from the list
  SUNRPC: add ability to remove specific server transport
  nfsd: fix management of listener transports

 fs/nfsd/nfsctl.c           |  4 +++-
 include/linux/llist.h      | 36 ++++++++++++++++++++++++++++++++++++
 include/linux/sunrpc/svc.h |  1 +
 net/sunrpc/svc_xprt.c      | 11 +++++++++++
 4 files changed, 51 insertions(+), 1 deletion(-)

-- 
2.47.1

[PATCH 1/3] llist: add ability to remove a particular entry from the list

[Olga Kornievskaia]

nfsd stores its network transports in a lwq (which is a lockless list)
llist has no ability to remove a particular entry which nfsd needs
to remove a listener thread.

Suggested-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
---
 include/linux/llist.h | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/include/linux/llist.h b/include/linux/llist.h
index 2c982ff7475a..fe6be21897d9 100644
--- a/include/linux/llist.h
+++ b/include/linux/llist.h
@@ -253,6 +253,42 @@ static inline bool __llist_add(struct llist_node *new, struct llist_head *head)
 	return __llist_add_batch(new, new, head);
 }
 
+/**
+ * llist_del_entry - remove a particular entry from a lock-less list
+ * @head: head of the list to remove the entry from
+ * @entry: entry to be removed from the list
+ *
+ * Walk the list, find the given entry and remove it from the list.
+ * The caller must ensure that nothing can race in and change the
+ * list while this is running.
+ *
+ * Returns true if the entry was found and removed.
+ */
+static inline bool llist_del_entry(struct llist_head *head, struct llist_node *entry)
+{
+	struct llist_node *pos;
+
+	if (!head->first)
+		return false;
+
+	/* Is it the first entry? */
+	if (head->first == entry) {
+		head->first = entry->next;
+		entry->next = entry;
+		return true;
+	}
+
+	/* Find it in the list */
+	llist_for_each(head->first, pos) {
+		if (pos->next == entry) {
+			pos->next = entry->next;
+			entry->next = entry;
+			return true;
+		}
+	}
+	return false;
+}
+
 /**
  * llist_del_all - delete all entries from lock-less list
  * @head:	the head of lock-less list to delete all entries
-- 
2.47.1

Re: [PATCH 1/3] llist: add ability to remove a particular entry from the list

[Chuck Lever]

On 1/15/25 6:24 PM, Olga Kornievskaia wrote:
> nfsd stores its network transports in a lwq (which is a lockless list)
> llist has no ability to remove a particular entry which nfsd needs
> to remove a listener thread.

Note that scripts/get_maintainer.pl says that the maintainer of this
file is:

   linux-kernel@vger.kernel.org

so that address should appear on the cc: list of this series. So
should the list of reviewers for NFSD that appear in MAINTAINERS.

ISTR Neil found the same gap in the llist API. I don't think it's
possible to safely remove an item from the middle of an llist. Much
of the complexity of the current svc thread scheduler is due to this
complication.

I think you will need to mark the listener as dead and find some
way of safely dealing with it later.


> Suggested-by: Jeff Layton <jlayton@kernel.org>
> Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
> ---
>   include/linux/llist.h | 36 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 36 insertions(+)
> 
> diff --git a/include/linux/llist.h b/include/linux/llist.h
> index 2c982ff7475a..fe6be21897d9 100644
> --- a/include/linux/llist.h
> +++ b/include/linux/llist.h
> @@ -253,6 +253,42 @@ static inline bool __llist_add(struct llist_node *new, struct llist_head *head)
>   	return __llist_add_batch(new, new, head);
>   }
>   
> +/**
> + * llist_del_entry - remove a particular entry from a lock-less list
> + * @head: head of the list to remove the entry from
> + * @entry: entry to be removed from the list
> + *
> + * Walk the list, find the given entry and remove it from the list.
> + * The caller must ensure that nothing can race in and change the
> + * list while this is running.
> + *
> + * Returns true if the entry was found and removed.
> + */
> +static inline bool llist_del_entry(struct llist_head *head, struct llist_node *entry)
> +{
> +	struct llist_node *pos;
> +
> +	if (!head->first)
> +		return false;
> +
> +	/* Is it the first entry? */
> +	if (head->first == entry) {
> +		head->first = entry->next;
> +		entry->next = entry;
> +		return true;
> +	}
> +
> +	/* Find it in the list */
> +	llist_for_each(head->first, pos) {
> +		if (pos->next == entry) {
> +			pos->next = entry->next;
> +			entry->next = entry;
> +			return true;
> +		}
> +	}
> +	return false;
> +}
> +
>   /**
>    * llist_del_all - delete all entries from lock-less list
>    * @head:	the head of lock-less list to delete all entries


-- 
Chuck Lever

Re: [PATCH 1/3] llist: add ability to remove a particular entry from the list

[Olga Kornievskaia]

On Thu, Jan 16, 2025 at 9:27 AM Chuck Lever <chuck.lever@oracle.com> wrote:
>
> On 1/15/25 6:24 PM, Olga Kornievskaia wrote:
> > nfsd stores its network transports in a lwq (which is a lockless list)
> > llist has no ability to remove a particular entry which nfsd needs
> > to remove a listener thread.
>
> Note that scripts/get_maintainer.pl says that the maintainer of this
> file is:
>
>    linux-kernel@vger.kernel.org
>
> so that address should appear on the cc: list of this series. So
> should the list of reviewers for NFSD that appear in MAINTAINERS.

I will resend and include the mentioned list.

> ISTR Neil found the same gap in the llist API. I don't think it's
> possible to safely remove an item from the middle of an llist. Much
> of the complexity of the current svc thread scheduler is due to this
> complication.
>
> I think you will need to mark the listener as dead and find some
> way of safely dealing with it later.

A listener can only be removed if there are no active threads. This
code in nfsd_nl_listener_set_doit()  won't allow it which happens
before we are manipulating the listener:
        /* For now, no removing old sockets while server is running */
        if (serv->sv_nrthreads && !list_empty(&permsocks)) {
                list_splice_init(&permsocks, &serv->sv_permsocks);
                spin_unlock_bh(&serv->sv_lock);
                err = -EBUSY;
                goto out_unlock_mtx;
        }


>
>
> > Suggested-by: Jeff Layton <jlayton@kernel.org>
> > Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
> > ---
> >   include/linux/llist.h | 36 ++++++++++++++++++++++++++++++++++++
> >   1 file changed, 36 insertions(+)
> >
> > diff --git a/include/linux/llist.h b/include/linux/llist.h
> > index 2c982ff7475a..fe6be21897d9 100644
> > --- a/include/linux/llist.h
> > +++ b/include/linux/llist.h
> > @@ -253,6 +253,42 @@ static inline bool __llist_add(struct llist_node *new, struct llist_head *head)
> >       return __llist_add_batch(new, new, head);
> >   }
> >
> > +/**
> > + * llist_del_entry - remove a particular entry from a lock-less list
> > + * @head: head of the list to remove the entry from
> > + * @entry: entry to be removed from the list
> > + *
> > + * Walk the list, find the given entry and remove it from the list.
> > + * The caller must ensure that nothing can race in and change the
> > + * list while this is running.
> > + *
> > + * Returns true if the entry was found and removed.
> > + */
> > +static inline bool llist_del_entry(struct llist_head *head, struct llist_node *entry)
> > +{
> > +     struct llist_node *pos;
> > +
> > +     if (!head->first)
> > +             return false;
> > +
> > +     /* Is it the first entry? */
> > +     if (head->first == entry) {
> > +             head->first = entry->next;
> > +             entry->next = entry;
> > +             return true;
> > +     }
> > +
> > +     /* Find it in the list */
> > +     llist_for_each(head->first, pos) {
> > +             if (pos->next == entry) {
> > +                     pos->next = entry->next;
> > +                     entry->next = entry;
> > +                     return true;
> > +             }
> > +     }
> > +     return false;
> > +}
> > +
> >   /**
> >    * llist_del_all - delete all entries from lock-less list
> >    * @head:   the head of lock-less list to delete all entries
>
>
> --
> Chuck Lever
>

Re: [PATCH 1/3] llist: add ability to remove a particular entry from the list

[Chuck Lever]

On 1/16/25 9:54 AM, Olga Kornievskaia wrote:
> On Thu, Jan 16, 2025 at 9:27 AM Chuck Lever <chuck.lever@oracle.com> wrote:
>>
>> On 1/15/25 6:24 PM, Olga Kornievskaia wrote:
>>> nfsd stores its network transports in a lwq (which is a lockless list)
>>> llist has no ability to remove a particular entry which nfsd needs
>>> to remove a listener thread.
>>
>> Note that scripts/get_maintainer.pl says that the maintainer of this
>> file is:
>>
>>     linux-kernel@vger.kernel.org
>>
>> so that address should appear on the cc: list of this series. So
>> should the list of reviewers for NFSD that appear in MAINTAINERS.
> 
> I will resend and include the mentioned list.
> 
>> ISTR Neil found the same gap in the llist API. I don't think it's
>> possible to safely remove an item from the middle of an llist. Much
>> of the complexity of the current svc thread scheduler is due to this
>> complication.
>>
>> I think you will need to mark the listener as dead and find some
>> way of safely dealing with it later.
> 
> A listener can only be removed if there are no active threads. This
> code in nfsd_nl_listener_set_doit()  won't allow it which happens
> before we are manipulating the listener:
>          /* For now, no removing old sockets while server is running */
>          if (serv->sv_nrthreads && !list_empty(&permsocks)) {
>                  list_splice_init(&permsocks, &serv->sv_permsocks);
>                  spin_unlock_bh(&serv->sv_lock);
>                  err = -EBUSY;
>                  goto out_unlock_mtx;
>          }

Got it.

I'm splitting hairs, but this seems like a special case that might be
true only for NFSD and could be abused by other API consumers.

At the least, the opening block comment in llist.h should be updated
to include del_entry in the locking table.

I would be more comfortable with a solution that works in alignment with
the current llist API, but if others are fine with this solution, then I
won't object strenuously.

(And to be clear, I agree that there is a bug in set_doit() that needs
to be addressed quickly -- it's the specific fix that I'm queasy about).


>>> Suggested-by: Jeff Layton <jlayton@kernel.org>
>>> Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
>>> ---
>>>    include/linux/llist.h | 36 ++++++++++++++++++++++++++++++++++++
>>>    1 file changed, 36 insertions(+)
>>>
>>> diff --git a/include/linux/llist.h b/include/linux/llist.h
>>> index 2c982ff7475a..fe6be21897d9 100644
>>> --- a/include/linux/llist.h
>>> +++ b/include/linux/llist.h
>>> @@ -253,6 +253,42 @@ static inline bool __llist_add(struct llist_node *new, struct llist_head *head)
>>>        return __llist_add_batch(new, new, head);
>>>    }
>>>
>>> +/**
>>> + * llist_del_entry - remove a particular entry from a lock-less list
>>> + * @head: head of the list to remove the entry from
>>> + * @entry: entry to be removed from the list
>>> + *
>>> + * Walk the list, find the given entry and remove it from the list.

The above sentence repeats the comments in the code and the code itself.
It visually obscures the next, much more important, sentence.


>>> + * The caller must ensure that nothing can race in and change the
>>> + * list while this is running.
>>> + *
>>> + * Returns true if the entry was found and removed.
>>> + */
>>> +static inline bool llist_del_entry(struct llist_head *head, struct llist_node *entry)
>>> +{
>>> +     struct llist_node *pos;
>>> +
>>> +     if (!head->first)
>>> +             return false;

if (llist_empty()) ?


>>> +
>>> +     /* Is it the first entry? */
>>> +     if (head->first == entry) {
>>> +             head->first = entry->next;
>>> +             entry->next = entry;
>>> +             return true;

llist_del_first() or even llist_del_first_this()

Basically I would avoid open-coding this logic and use the existing
helpers where possible. The new code doesn't provide memory release
semantics that would ensure the next access of the llist sees these
updates.


>>> +     }
>>> +
>>> +     /* Find it in the list */
>>> +     llist_for_each(head->first, pos) {
>>> +             if (pos->next == entry) {
>>> +                     pos->next = entry->next;
>>> +                     entry->next = entry;
>>> +                     return true;
>>> +             }
>>> +     }
>>> +     return false;
>>> +}
>>> +
>>>    /**
>>>     * llist_del_all - delete all entries from lock-less list
>>>     * @head:   the head of lock-less list to delete all entries
>>
>>
>> --
>> Chuck Lever
>>
> 


-- 
Chuck Lever

Re: [PATCH 1/3] llist: add ability to remove a particular entry from the list

[Jeff Layton]

On Thu, 2025-01-16 at 10:33 -0500, Chuck Lever wrote:
> On 1/16/25 9:54 AM, Olga Kornievskaia wrote:
> > On Thu, Jan 16, 2025 at 9:27 AM Chuck Lever <chuck.lever@oracle.com> wrote:
> > > 
> > > On 1/15/25 6:24 PM, Olga Kornievskaia wrote:
> > > > nfsd stores its network transports in a lwq (which is a lockless list)
> > > > llist has no ability to remove a particular entry which nfsd needs
> > > > to remove a listener thread.
> > > 
> > > Note that scripts/get_maintainer.pl says that the maintainer of this
> > > file is:
> > > 
> > >     linux-kernel@vger.kernel.org
> > > 
> > > so that address should appear on the cc: list of this series. So
> > > should the list of reviewers for NFSD that appear in MAINTAINERS.
> > 
> > I will resend and include the mentioned list.
> > 
> > > ISTR Neil found the same gap in the llist API. I don't think it's
> > > possible to safely remove an item from the middle of an llist. Much
> > > of the complexity of the current svc thread scheduler is due to this
> > > complication.
> > > 
> > > I think you will need to mark the listener as dead and find some
> > > way of safely dealing with it later.
> > 
> > A listener can only be removed if there are no active threads. This
> > code in nfsd_nl_listener_set_doit()  won't allow it which happens
> > before we are manipulating the listener:
> >          /* For now, no removing old sockets while server is running */
> >          if (serv->sv_nrthreads && !list_empty(&permsocks)) {
> >                  list_splice_init(&permsocks, &serv->sv_permsocks);
> >                  spin_unlock_bh(&serv->sv_lock);
> >                  err = -EBUSY;
> >                  goto out_unlock_mtx;
> >          }
> 
> Got it.
> 
> I'm splitting hairs, but this seems like a special case that might be
> true only for NFSD and could be abused by other API consumers.
> 
> At the least, the opening block comment in llist.h should be updated
> to include del_entry in the locking table.
> 
> I would be more comfortable with a solution that works in alignment with
> the current llist API, but if others are fine with this solution, then I
> won't object strenuously.
> 
> (And to be clear, I agree that there is a bug in set_doit() that needs
> to be addressed quickly -- it's the specific fix that I'm queasy about).
> 

Yeah, it would be good to address it quickly since you can crash the
box with this right now.

I'm not thrilled with adding the llist_del_entry() footgun either, but
it should work.

Another approach we could consider instead would be to delay queueing
all of these sockets to the lwq until after the sv_permsocks list is
settled. You could even just queue up the whole sv_permsocks list at
the end of this. If there's no work there, then there's no real harm.
That is a bit more surgery, however, since you'd have to lift
svc_xprt_received() handling out of svc_xprt_create_from_sa().

> 
> > > > Suggested-by: Jeff Layton <jlayton@kernel.org>
> > > > Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
> > > > ---
> > > >    include/linux/llist.h | 36 ++++++++++++++++++++++++++++++++++++
> > > >    1 file changed, 36 insertions(+)
> > > > 
> > > > diff --git a/include/linux/llist.h b/include/linux/llist.h
> > > > index 2c982ff7475a..fe6be21897d9 100644
> > > > --- a/include/linux/llist.h
> > > > +++ b/include/linux/llist.h
> > > > @@ -253,6 +253,42 @@ static inline bool __llist_add(struct llist_node *new, struct llist_head *head)
> > > >        return __llist_add_batch(new, new, head);
> > > >    }
> > > > 
> > > > +/**
> > > > + * llist_del_entry - remove a particular entry from a lock-less list
> > > > + * @head: head of the list to remove the entry from
> > > > + * @entry: entry to be removed from the list
> > > > + *
> > > > + * Walk the list, find the given entry and remove it from the list.
> 
> The above sentence repeats the comments in the code and the code itself.
> It visually obscures the next, much more important, sentence.
> 
> 
> > > > + * The caller must ensure that nothing can race in and change the
> > > > + * list while this is running.
> > > > + *
> > > > + * Returns true if the entry was found and removed.
> > > > + */
> > > > +static inline bool llist_del_entry(struct llist_head *head, struct llist_node *entry)
> > > > +{
> > > > +     struct llist_node *pos;
> > > > +
> > > > +     if (!head->first)
> > > > +             return false;
> 
> if (llist_empty()) ?
> 
> 
> > > > +
> > > > +     /* Is it the first entry? */
> > > > +     if (head->first == entry) {
> > > > +             head->first = entry->next;
> > > > +             entry->next = entry;
> > > > +             return true;
> 
> llist_del_first() or even llist_del_first_this()
> 
> Basically I would avoid open-coding this logic and use the existing
> helpers where possible. The new code doesn't provide memory release
> semantics that would ensure the next access of the llist sees these
> updates.
> 
> 
> > > > +     }
> > > > +
> > > > +     /* Find it in the list */
> > > > +     llist_for_each(head->first, pos) {
> > > > +             if (pos->next == entry) {
> > > > +                     pos->next = entry->next;
> > > > +                     entry->next = entry;
> > > > +                     return true;
> > > > +             }
> > > > +     }
> > > > +     return false;
> > > > +}
> > > > +
> > > >    /**
> > > >     * llist_del_all - delete all entries from lock-less list
> > > >     * @head:   the head of lock-less list to delete all entries
> > > 
> > > 
> > > --
> > > Chuck Lever
> > > 
> > 
> 
> 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 1/3] llist: add ability to remove a particular entry from the list

[Chuck Lever]

On 1/16/25 10:42 AM, Jeff Layton wrote:
> On Thu, 2025-01-16 at 10:33 -0500, Chuck Lever wrote:
>> On 1/16/25 9:54 AM, Olga Kornievskaia wrote:
>>> On Thu, Jan 16, 2025 at 9:27 AM Chuck Lever <chuck.lever@oracle.com> wrote:
>>>>
>>>> On 1/15/25 6:24 PM, Olga Kornievskaia wrote:
>>>>> nfsd stores its network transports in a lwq (which is a lockless list)
>>>>> llist has no ability to remove a particular entry which nfsd needs
>>>>> to remove a listener thread.
>>>>
>>>> Note that scripts/get_maintainer.pl says that the maintainer of this
>>>> file is:
>>>>
>>>>      linux-kernel@vger.kernel.org
>>>>
>>>> so that address should appear on the cc: list of this series. So
>>>> should the list of reviewers for NFSD that appear in MAINTAINERS.
>>>
>>> I will resend and include the mentioned list.
>>>
>>>> ISTR Neil found the same gap in the llist API. I don't think it's
>>>> possible to safely remove an item from the middle of an llist. Much
>>>> of the complexity of the current svc thread scheduler is due to this
>>>> complication.
>>>>
>>>> I think you will need to mark the listener as dead and find some
>>>> way of safely dealing with it later.
>>>
>>> A listener can only be removed if there are no active threads. This
>>> code in nfsd_nl_listener_set_doit()  won't allow it which happens
>>> before we are manipulating the listener:
>>>           /* For now, no removing old sockets while server is running */
>>>           if (serv->sv_nrthreads && !list_empty(&permsocks)) {
>>>                   list_splice_init(&permsocks, &serv->sv_permsocks);
>>>                   spin_unlock_bh(&serv->sv_lock);
>>>                   err = -EBUSY;
>>>                   goto out_unlock_mtx;
>>>           }
>>
>> Got it.
>>
>> I'm splitting hairs, but this seems like a special case that might be
>> true only for NFSD and could be abused by other API consumers.
>>
>> At the least, the opening block comment in llist.h should be updated
>> to include del_entry in the locking table.
>>
>> I would be more comfortable with a solution that works in alignment with
>> the current llist API, but if others are fine with this solution, then I
>> won't object strenuously.
>>
>> (And to be clear, I agree that there is a bug in set_doit() that needs
>> to be addressed quickly -- it's the specific fix that I'm queasy about).
>>
> 
> Yeah, it would be good to address it quickly since you can crash the
> box with this right now.

I'm asking myself why this isn't a problem during server shutdown or
when removing just one listener -- with rpc.nfsd. Have we never done
this before we had netlink?


> I'm not thrilled with adding the llist_del_entry() footgun either, but
> it should work.

I would like to see one or two alternatives before sticking with this
llist_del_entry() idea.


> Another approach we could consider instead would be to delay queueing
> all of these sockets to the lwq until after the sv_permsocks list is
> settled. You could even just queue up the whole sv_permsocks list at
> the end of this. If there's no work there, then there's no real harm.
> That is a bit more surgery, however, since you'd have to lift
> svc_xprt_received() handling out of svc_xprt_create_from_sa().

Handling the list once instead of adding and/or removing one at a time
seems like a better plan to me (IIUC).

Also, nit: the use of the term "sockets" throughout this code is
confusing. We're dealing with RDMA endpoints as well in here. We can't
easily rename the structure fields, true, but the comments could be more
helpful.


>>>>> Suggested-by: Jeff Layton <jlayton@kernel.org>
>>>>> Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
>>>>> ---
>>>>>     include/linux/llist.h | 36 ++++++++++++++++++++++++++++++++++++
>>>>>     1 file changed, 36 insertions(+)
>>>>>
>>>>> diff --git a/include/linux/llist.h b/include/linux/llist.h
>>>>> index 2c982ff7475a..fe6be21897d9 100644
>>>>> --- a/include/linux/llist.h
>>>>> +++ b/include/linux/llist.h
>>>>> @@ -253,6 +253,42 @@ static inline bool __llist_add(struct llist_node *new, struct llist_head *head)
>>>>>         return __llist_add_batch(new, new, head);
>>>>>     }
>>>>>
>>>>> +/**
>>>>> + * llist_del_entry - remove a particular entry from a lock-less list
>>>>> + * @head: head of the list to remove the entry from
>>>>> + * @entry: entry to be removed from the list
>>>>> + *
>>>>> + * Walk the list, find the given entry and remove it from the list.
>>
>> The above sentence repeats the comments in the code and the code itself.
>> It visually obscures the next, much more important, sentence.
>>
>>
>>>>> + * The caller must ensure that nothing can race in and change the
>>>>> + * list while this is running.
>>>>> + *
>>>>> + * Returns true if the entry was found and removed.
>>>>> + */
>>>>> +static inline bool llist_del_entry(struct llist_head *head, struct llist_node *entry)
>>>>> +{
>>>>> +     struct llist_node *pos;
>>>>> +
>>>>> +     if (!head->first)
>>>>> +             return false;
>>
>> if (llist_empty()) ?
>>
>>
>>>>> +
>>>>> +     /* Is it the first entry? */
>>>>> +     if (head->first == entry) {
>>>>> +             head->first = entry->next;
>>>>> +             entry->next = entry;
>>>>> +             return true;
>>
>> llist_del_first() or even llist_del_first_this()
>>
>> Basically I would avoid open-coding this logic and use the existing
>> helpers where possible. The new code doesn't provide memory release
>> semantics that would ensure the next access of the llist sees these
>> updates.
>>
>>
>>>>> +     }
>>>>> +
>>>>> +     /* Find it in the list */
>>>>> +     llist_for_each(head->first, pos) {
>>>>> +             if (pos->next == entry) {
>>>>> +                     pos->next = entry->next;
>>>>> +                     entry->next = entry;
>>>>> +                     return true;
>>>>> +             }
>>>>> +     }
>>>>> +     return false;
>>>>> +}
>>>>> +
>>>>>     /**
>>>>>      * llist_del_all - delete all entries from lock-less list
>>>>>      * @head:   the head of lock-less list to delete all entries
>>>>
>>>>
>>>> --
>>>> Chuck Lever
>>>>
>>>
>>
>>
> 


-- 
Chuck Lever

Re: [PATCH 1/3] llist: add ability to remove a particular entry from the list

[Olga Kornievskaia]

On Thu, Jan 16, 2025 at 11:00 AM Chuck Lever <chuck.lever@oracle.com> wrote:
>
> On 1/16/25 10:42 AM, Jeff Layton wrote:
> > On Thu, 2025-01-16 at 10:33 -0500, Chuck Lever wrote:
> >> On 1/16/25 9:54 AM, Olga Kornievskaia wrote:
> >>> On Thu, Jan 16, 2025 at 9:27 AM Chuck Lever <chuck.lever@oracle.com> wrote:
> >>>>
> >>>> On 1/15/25 6:24 PM, Olga Kornievskaia wrote:
> >>>>> nfsd stores its network transports in a lwq (which is a lockless list)
> >>>>> llist has no ability to remove a particular entry which nfsd needs
> >>>>> to remove a listener thread.
> >>>>
> >>>> Note that scripts/get_maintainer.pl says that the maintainer of this
> >>>> file is:
> >>>>
> >>>>      linux-kernel@vger.kernel.org
> >>>>
> >>>> so that address should appear on the cc: list of this series. So
> >>>> should the list of reviewers for NFSD that appear in MAINTAINERS.
> >>>
> >>> I will resend and include the mentioned list.
> >>>
> >>>> ISTR Neil found the same gap in the llist API. I don't think it's
> >>>> possible to safely remove an item from the middle of an llist. Much
> >>>> of the complexity of the current svc thread scheduler is due to this
> >>>> complication.
> >>>>
> >>>> I think you will need to mark the listener as dead and find some
> >>>> way of safely dealing with it later.
> >>>
> >>> A listener can only be removed if there are no active threads. This
> >>> code in nfsd_nl_listener_set_doit()  won't allow it which happens
> >>> before we are manipulating the listener:
> >>>           /* For now, no removing old sockets while server is running */
> >>>           if (serv->sv_nrthreads && !list_empty(&permsocks)) {
> >>>                   list_splice_init(&permsocks, &serv->sv_permsocks);
> >>>                   spin_unlock_bh(&serv->sv_lock);
> >>>                   err = -EBUSY;
> >>>                   goto out_unlock_mtx;
> >>>           }
> >>
> >> Got it.
> >>
> >> I'm splitting hairs, but this seems like a special case that might be
> >> true only for NFSD and could be abused by other API consumers.
> >>
> >> At the least, the opening block comment in llist.h should be updated
> >> to include del_entry in the locking table.
> >>
> >> I would be more comfortable with a solution that works in alignment with
> >> the current llist API, but if others are fine with this solution, then I
> >> won't object strenuously.
> >>
> >> (And to be clear, I agree that there is a bug in set_doit() that needs
> >> to be addressed quickly -- it's the specific fix that I'm queasy about).
> >>
> >
> > Yeah, it would be good to address it quickly since you can crash the
> > box with this right now.
>
> I'm asking myself why this isn't a problem during server shutdown or
> when removing just one listener -- with rpc.nfsd. Have we never done
> this before we had netlink?

Removing a single listener was never been an option before. Shutdown
removes listeners and then frees the net, serv. proc fs only allowed
to view listeners. To change them, you had to change nfs.conf and
restart the server.

The problem here is new code that didn't handle a single entry removal
correctly.

> > I'm not thrilled with adding the llist_del_entry() footgun either, but
> > it should work.
>
> I would like to see one or two alternatives before sticking with this
> llist_del_entry() idea.
>
>
> > Another approach we could consider instead would be to delay queueing
> > all of these sockets to the lwq until after the sv_permsocks list is
> > settled. You could even just queue up the whole sv_permsocks list at
> > the end of this. If there's no work there, then there's no real harm.
> > That is a bit more surgery, however, since you'd have to lift
> > svc_xprt_received() handling out of svc_xprt_create_from_sa().
>
> Handling the list once instead of adding and/or removing one at a time
> seems like a better plan to me (IIUC).

I'll attempt an alternative that creates anew. I don't understand this
suggestion to "wait for sv_permsock to be settled". How can you know
when nfsdctl is done adding/removing listeners?

> Also, nit: the use of the term "sockets" throughout this code is
> confusing. We're dealing with RDMA endpoints as well in here. We can't
> easily rename the structure fields, true, but the comments could be more
> helpful.
>
>
> >>>>> Suggested-by: Jeff Layton <jlayton@kernel.org>
> >>>>> Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
> >>>>> ---
> >>>>>     include/linux/llist.h | 36 ++++++++++++++++++++++++++++++++++++
> >>>>>     1 file changed, 36 insertions(+)
> >>>>>
> >>>>> diff --git a/include/linux/llist.h b/include/linux/llist.h
> >>>>> index 2c982ff7475a..fe6be21897d9 100644
> >>>>> --- a/include/linux/llist.h
> >>>>> +++ b/include/linux/llist.h
> >>>>> @@ -253,6 +253,42 @@ static inline bool __llist_add(struct llist_node *new, struct llist_head *head)
> >>>>>         return __llist_add_batch(new, new, head);
> >>>>>     }
> >>>>>
> >>>>> +/**
> >>>>> + * llist_del_entry - remove a particular entry from a lock-less list
> >>>>> + * @head: head of the list to remove the entry from
> >>>>> + * @entry: entry to be removed from the list
> >>>>> + *
> >>>>> + * Walk the list, find the given entry and remove it from the list.
> >>
> >> The above sentence repeats the comments in the code and the code itself.
> >> It visually obscures the next, much more important, sentence.
> >>
> >>
> >>>>> + * The caller must ensure that nothing can race in and change the
> >>>>> + * list while this is running.
> >>>>> + *
> >>>>> + * Returns true if the entry was found and removed.
> >>>>> + */
> >>>>> +static inline bool llist_del_entry(struct llist_head *head, struct llist_node *entry)
> >>>>> +{
> >>>>> +     struct llist_node *pos;
> >>>>> +
> >>>>> +     if (!head->first)
> >>>>> +             return false;
> >>
> >> if (llist_empty()) ?
> >>
> >>
> >>>>> +
> >>>>> +     /* Is it the first entry? */
> >>>>> +     if (head->first == entry) {
> >>>>> +             head->first = entry->next;
> >>>>> +             entry->next = entry;
> >>>>> +             return true;
> >>
> >> llist_del_first() or even llist_del_first_this()
> >>
> >> Basically I would avoid open-coding this logic and use the existing
> >> helpers where possible. The new code doesn't provide memory release
> >> semantics that would ensure the next access of the llist sees these
> >> updates.
> >>
> >>
> >>>>> +     }
> >>>>> +
> >>>>> +     /* Find it in the list */
> >>>>> +     llist_for_each(head->first, pos) {
> >>>>> +             if (pos->next == entry) {
> >>>>> +                     pos->next = entry->next;
> >>>>> +                     entry->next = entry;
> >>>>> +                     return true;
> >>>>> +             }
> >>>>> +     }
> >>>>> +     return false;
> >>>>> +}
> >>>>> +
> >>>>>     /**
> >>>>>      * llist_del_all - delete all entries from lock-less list
> >>>>>      * @head:   the head of lock-less list to delete all entries
> >>>>
> >>>>
> >>>> --
> >>>> Chuck Lever
> >>>>
> >>>
> >>
> >>
> >
>
>
> --
> Chuck Lever
>

Re: [PATCH 1/3] llist: add ability to remove a particular entry from the list

[Chuck Lever]

On 1/16/25 11:31 AM, Olga Kornievskaia wrote:
> On Thu, Jan 16, 2025 at 11:00 AM Chuck Lever <chuck.lever@oracle.com> wrote:
>>
>> On 1/16/25 10:42 AM, Jeff Layton wrote:
>>> On Thu, 2025-01-16 at 10:33 -0500, Chuck Lever wrote:
>>>> On 1/16/25 9:54 AM, Olga Kornievskaia wrote:
>>>>> On Thu, Jan 16, 2025 at 9:27 AM Chuck Lever <chuck.lever@oracle.com> wrote:
>>>>>>
>>>>>> On 1/15/25 6:24 PM, Olga Kornievskaia wrote:
>>>>>>> nfsd stores its network transports in a lwq (which is a lockless list)
>>>>>>> llist has no ability to remove a particular entry which nfsd needs
>>>>>>> to remove a listener thread.
>>>>>>
>>>>>> Note that scripts/get_maintainer.pl says that the maintainer of this
>>>>>> file is:
>>>>>>
>>>>>>       linux-kernel@vger.kernel.org
>>>>>>
>>>>>> so that address should appear on the cc: list of this series. So
>>>>>> should the list of reviewers for NFSD that appear in MAINTAINERS.
>>>>>
>>>>> I will resend and include the mentioned list.
>>>>>
>>>>>> ISTR Neil found the same gap in the llist API. I don't think it's
>>>>>> possible to safely remove an item from the middle of an llist. Much
>>>>>> of the complexity of the current svc thread scheduler is due to this
>>>>>> complication.
>>>>>>
>>>>>> I think you will need to mark the listener as dead and find some
>>>>>> way of safely dealing with it later.
>>>>>
>>>>> A listener can only be removed if there are no active threads. This
>>>>> code in nfsd_nl_listener_set_doit()  won't allow it which happens
>>>>> before we are manipulating the listener:
>>>>>            /* For now, no removing old sockets while server is running */
>>>>>            if (serv->sv_nrthreads && !list_empty(&permsocks)) {
>>>>>                    list_splice_init(&permsocks, &serv->sv_permsocks);
>>>>>                    spin_unlock_bh(&serv->sv_lock);
>>>>>                    err = -EBUSY;
>>>>>                    goto out_unlock_mtx;
>>>>>            }
>>>>
>>>> Got it.
>>>>
>>>> I'm splitting hairs, but this seems like a special case that might be
>>>> true only for NFSD and could be abused by other API consumers.
>>>>
>>>> At the least, the opening block comment in llist.h should be updated
>>>> to include del_entry in the locking table.
>>>>
>>>> I would be more comfortable with a solution that works in alignment with
>>>> the current llist API, but if others are fine with this solution, then I
>>>> won't object strenuously.
>>>>
>>>> (And to be clear, I agree that there is a bug in set_doit() that needs
>>>> to be addressed quickly -- it's the specific fix that I'm queasy about).
>>>>
>>>
>>> Yeah, it would be good to address it quickly since you can crash the
>>> box with this right now.
>>
>> I'm asking myself why this isn't a problem during server shutdown or
>> when removing just one listener -- with rpc.nfsd. Have we never done
>> this before we had netlink?
> 
> Removing a single listener was never been an option before. Shutdown
> removes listeners and then frees the net, serv. proc fs only allowed
> to view listeners. To change them, you had to change nfs.conf and
> restart the server.
> 
> The problem here is new code that didn't handle a single entry removal
> correctly.
> 
>>> I'm not thrilled with adding the llist_del_entry() footgun either, but
>>> it should work.
>>
>> I would like to see one or two alternatives before sticking with this
>> llist_del_entry() idea.
>>
>>
>>> Another approach we could consider instead would be to delay queueing
>>> all of these sockets to the lwq until after the sv_permsocks list is
>>> settled. You could even just queue up the whole sv_permsocks list at
>>> the end of this. If there's no work there, then there's no real harm.
>>> That is a bit more surgery, however, since you'd have to lift
>>> svc_xprt_received() handling out of svc_xprt_create_from_sa().
>>
>> Handling the list once instead of adding and/or removing one at a time
>> seems like a better plan to me (IIUC).
> 
> I'll attempt an alternative that creates anew.

Thanks! I appreciate your work on this.


> I don't understand this
> suggestion to "wait for sv_permsock to be settled". How can you know
> when nfsdctl is done adding/removing listeners?
> 
>> Also, nit: the use of the term "sockets" throughout this code is
>> confusing. We're dealing with RDMA endpoints as well in here. We can't
>> easily rename the structure fields, true, but the comments could be more
>> helpful.
>>
>>
>>>>>>> Suggested-by: Jeff Layton <jlayton@kernel.org>
>>>>>>> Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
>>>>>>> ---
>>>>>>>      include/linux/llist.h | 36 ++++++++++++++++++++++++++++++++++++
>>>>>>>      1 file changed, 36 insertions(+)
>>>>>>>
>>>>>>> diff --git a/include/linux/llist.h b/include/linux/llist.h
>>>>>>> index 2c982ff7475a..fe6be21897d9 100644
>>>>>>> --- a/include/linux/llist.h
>>>>>>> +++ b/include/linux/llist.h
>>>>>>> @@ -253,6 +253,42 @@ static inline bool __llist_add(struct llist_node *new, struct llist_head *head)
>>>>>>>          return __llist_add_batch(new, new, head);
>>>>>>>      }
>>>>>>>
>>>>>>> +/**
>>>>>>> + * llist_del_entry - remove a particular entry from a lock-less list
>>>>>>> + * @head: head of the list to remove the entry from
>>>>>>> + * @entry: entry to be removed from the list
>>>>>>> + *
>>>>>>> + * Walk the list, find the given entry and remove it from the list.
>>>>
>>>> The above sentence repeats the comments in the code and the code itself.
>>>> It visually obscures the next, much more important, sentence.
>>>>
>>>>
>>>>>>> + * The caller must ensure that nothing can race in and change the
>>>>>>> + * list while this is running.
>>>>>>> + *
>>>>>>> + * Returns true if the entry was found and removed.
>>>>>>> + */
>>>>>>> +static inline bool llist_del_entry(struct llist_head *head, struct llist_node *entry)
>>>>>>> +{
>>>>>>> +     struct llist_node *pos;
>>>>>>> +
>>>>>>> +     if (!head->first)
>>>>>>> +             return false;
>>>>
>>>> if (llist_empty()) ?
>>>>
>>>>
>>>>>>> +
>>>>>>> +     /* Is it the first entry? */
>>>>>>> +     if (head->first == entry) {
>>>>>>> +             head->first = entry->next;
>>>>>>> +             entry->next = entry;
>>>>>>> +             return true;
>>>>
>>>> llist_del_first() or even llist_del_first_this()
>>>>
>>>> Basically I would avoid open-coding this logic and use the existing
>>>> helpers where possible. The new code doesn't provide memory release
>>>> semantics that would ensure the next access of the llist sees these
>>>> updates.
>>>>
>>>>
>>>>>>> +     }
>>>>>>> +
>>>>>>> +     /* Find it in the list */
>>>>>>> +     llist_for_each(head->first, pos) {
>>>>>>> +             if (pos->next == entry) {
>>>>>>> +                     pos->next = entry->next;
>>>>>>> +                     entry->next = entry;
>>>>>>> +                     return true;
>>>>>>> +             }
>>>>>>> +     }
>>>>>>> +     return false;
>>>>>>> +}
>>>>>>> +
>>>>>>>      /**
>>>>>>>       * llist_del_all - delete all entries from lock-less list
>>>>>>>       * @head:   the head of lock-less list to delete all entries
>>>>>>
>>>>>>
>>>>>> --
>>>>>> Chuck Lever
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>>
>> --
>> Chuck Lever
>>
> 


-- 
Chuck Lever

Re: [PATCH 1/3] llist: add ability to remove a particular entry from the list

[kernel test robot]

Hi Olga,

kernel test robot noticed the following build errors:

[auto build test ERROR on trondmy-nfs/linux-next]
[also build test ERROR on brauner-vfs/vfs.all linus/master v6.13-rc7 next-20250116]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Olga-Kornievskaia/llist-add-ability-to-remove-a-particular-entry-from-the-list/20250116-072951
base:   git://git.linux-nfs.org/projects/trondmy/linux-nfs.git linux-next
patch link:    https://lore.kernel.org/r/20250115232406.44815-2-okorniev%40redhat.com
patch subject: [PATCH 1/3] llist: add ability to remove a particular entry from the list
config: s390-randconfig-002-20250117 (https://download.01.org/0day-ci/archive/20250117/202501171157.gr1JO94W-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project c23f2417dc5f6dc371afb07af5627ec2a9d373a0)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250117/202501171157.gr1JO94W-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202501171157.gr1JO94W-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

   In file included from arch/s390/kernel/asm-offsets.c:11:
   In file included from include/linux/kvm_host.h:7:
   In file included from include/linux/hardirq.h:5:
   In file included from include/linux/context_tracking_state.h:5:
   In file included from include/linux/percpu.h:5:
   In file included from include/linux/alloc_tag.h:14:
   In file included from include/linux/smp.h:15:
   In file included from include/linux/smp_types.h:5:
>> include/linux/llist.h:283:7: warning: variable 'pos' is uninitialized when used here [-Wuninitialized]
     283 |                 if (pos->next == entry) {
         |                     ^~~
   include/linux/llist.h:269:24: note: initialize the variable 'pos' to silence this warning
     269 |         struct llist_node *pos;
         |                               ^
         |                                = NULL
   In file included from arch/s390/kernel/asm-offsets.c:11:
   In file included from include/linux/kvm_host.h:16:
   In file included from include/linux/mm.h:2224:
   include/linux/vmstat.h:504:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     504 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     505 |                            item];
         |                            ~~~~
   include/linux/vmstat.h:511:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     511 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     512 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   include/linux/vmstat.h:524:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     524 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     525 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   4 warnings generated.
--
   In file included from <built-in>:4:
   In file included from lib/vdso/getrandom.c:8:
   In file included from include/vdso/datapage.h:164:
   In file included from arch/s390/include/asm/vdso/gettimeofday.h:11:
   In file included from arch/s390/include/asm/syscall.h:13:
   In file included from include/linux/sched.h:20:
   In file included from include/linux/smp_types.h:5:
>> include/linux/llist.h:283:7: warning: variable 'pos' is uninitialized when used here [-Wuninitialized]
     283 |                 if (pos->next == entry) {
         |                     ^~~
   include/linux/llist.h:269:24: note: initialize the variable 'pos' to silence this warning
     269 |         struct llist_node *pos;
         |                               ^
         |                                = NULL
   1 warning generated.
--
   In file included from lib/vsprintf.c:22:
   In file included from include/linux/clk.h:14:
   In file included from include/linux/notifier.h:14:
   In file included from include/linux/mutex.h:17:
   In file included from include/linux/lockdep.h:14:
   In file included from include/linux/smp.h:15:
   In file included from include/linux/smp_types.h:5:
>> include/linux/llist.h:283:7: warning: variable 'pos' is uninitialized when used here [-Wuninitialized]
     283 |                 if (pos->next == entry) {
         |                     ^~~
   include/linux/llist.h:269:24: note: initialize the variable 'pos' to silence this warning
     269 |         struct llist_node *pos;
         |                               ^
         |                                = NULL
   In file included from lib/vsprintf.c:25:
   In file included from include/linux/module.h:19:
   In file included from include/linux/elf.h:6:
   In file included from arch/s390/include/asm/elf.h:181:
   In file included from arch/s390/include/asm/mmu_context.h:11:
   In file included from arch/s390/include/asm/pgalloc.h:18:
   In file included from include/linux/mm.h:2224:
   include/linux/vmstat.h:504:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     504 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     505 |                            item];
         |                            ~~~~
   include/linux/vmstat.h:511:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     511 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     512 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   include/linux/vmstat.h:524:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     524 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     525 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   In file included from lib/vsprintf.c:50:
   In file included from lib/../mm/internal.h:13:
   include/linux/mm_inline.h:47:41: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
      47 |         __mod_lruvec_state(lruvec, NR_LRU_BASE + lru, nr_pages);
         |                                    ~~~~~~~~~~~ ^ ~~~
   include/linux/mm_inline.h:49:22: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
      49 |                                 NR_ZONE_LRU_BASE + lru, nr_pages);
         |                                 ~~~~~~~~~~~~~~~~ ^ ~~~
   6 warnings generated.
--
   In file included from lib/test_bitops.c:10:
   In file included from include/linux/module.h:17:
   In file included from include/linux/kmod.h:9:
   In file included from include/linux/umh.h:4:
   In file included from include/linux/gfp.h:7:
   In file included from include/linux/mmzone.h:8:
   In file included from include/linux/spinlock.h:63:
   In file included from include/linux/lockdep.h:14:
   In file included from include/linux/smp.h:15:
   In file included from include/linux/smp_types.h:5:
>> include/linux/llist.h:283:7: error: variable 'pos' is uninitialized when used here [-Werror,-Wuninitialized]
     283 |                 if (pos->next == entry) {
         |                     ^~~
   include/linux/llist.h:269:24: note: initialize the variable 'pos' to silence this warning
     269 |         struct llist_node *pos;
         |                               ^
         |                                = NULL
   In file included from lib/test_bitops.c:10:
   In file included from include/linux/module.h:19:
   In file included from include/linux/elf.h:6:
   In file included from arch/s390/include/asm/elf.h:181:
   In file included from arch/s390/include/asm/mmu_context.h:11:
   In file included from arch/s390/include/asm/pgalloc.h:18:
   In file included from include/linux/mm.h:2224:
   include/linux/vmstat.h:504:43: error: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Werror,-Wenum-enum-conversion]
     504 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     505 |                            item];
         |                            ~~~~
   include/linux/vmstat.h:511:43: error: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Werror,-Wenum-enum-conversion]
     511 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     512 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   include/linux/vmstat.h:524:43: error: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Werror,-Wenum-enum-conversion]
     524 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     525 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   4 errors generated.
--
   In file included from lib/test_maple_tree.c:10:
   In file included from include/linux/maple_tree.h:12:
   In file included from include/linux/rcupdate.h:29:
   In file included from include/linux/lockdep.h:14:
   In file included from include/linux/smp.h:15:
   In file included from include/linux/smp_types.h:5:
>> include/linux/llist.h:283:7: warning: variable 'pos' is uninitialized when used here [-Wuninitialized]
     283 |                 if (pos->next == entry) {
         |                     ^~~
   include/linux/llist.h:269:24: note: initialize the variable 'pos' to silence this warning
     269 |         struct llist_node *pos;
         |                               ^
         |                                = NULL
   In file included from lib/test_maple_tree.c:11:
   In file included from include/linux/module.h:19:
   In file included from include/linux/elf.h:6:
   In file included from arch/s390/include/asm/elf.h:181:
   In file included from arch/s390/include/asm/mmu_context.h:11:
   In file included from arch/s390/include/asm/pgalloc.h:18:
   In file included from include/linux/mm.h:2224:
   include/linux/vmstat.h:504:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     504 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     505 |                            item];
         |                            ~~~~
   include/linux/vmstat.h:511:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     511 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     512 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   include/linux/vmstat.h:524:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     524 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     525 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   lib/test_maple_tree.c:212:26: warning: unused function 'not_empty' [-Wunused-function]
     212 | static inline __init int not_empty(struct maple_node *node)
         |                          ^~~~~~~~~
   5 warnings generated.
--
   In file included from lib/locking-selftest.c:14:
   In file included from include/linux/rwsem.h:15:
   In file included from include/linux/spinlock.h:63:
   In file included from include/linux/lockdep.h:14:
   In file included from include/linux/smp.h:15:
   In file included from include/linux/smp_types.h:5:
>> include/linux/llist.h:283:7: warning: variable 'pos' is uninitialized when used here [-Wuninitialized]
     283 |                 if (pos->next == entry) {
         |                     ^~~
   include/linux/llist.h:269:24: note: initialize the variable 'pos' to silence this warning
     269 |         struct llist_node *pos;
         |                               ^
         |                                = NULL
   In file included from lib/locking-selftest.c:22:
   In file included from include/linux/kallsyms.h:13:
   In file included from include/linux/mm.h:2224:
   include/linux/vmstat.h:504:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     504 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     505 |                            item];
         |                            ~~~~
   include/linux/vmstat.h:511:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     511 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     512 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   include/linux/vmstat.h:524:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     524 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     525 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   lib/locking-selftest.c:2510:1: warning: unused function 'class_HARDIRQ_lock_ptr' [-Wunused-function]
    2510 | DEFINE_LOCK_GUARD_0(HARDIRQ, HARDIRQ_ENTER(), HARDIRQ_EXIT())
         | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/cleanup.h:407:49: note: expanded from macro 'DEFINE_LOCK_GUARD_0'
     407 | __DEFINE_CLASS_IS_CONDITIONAL(_name, false);                            \
         |                                                                         ^
     408 | __DEFINE_UNLOCK_GUARD(_name, void, _unlock, __VA_ARGS__)                \
         | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/cleanup.h:378:21: note: expanded from macro '\
   __DEFINE_UNLOCK_GUARD'
     378 | static inline void *class_##_name##_lock_ptr(class_##_name##_t *_T)     \
         |                     ^~~~~~~~~~~~~~~~~~~~~~~~
   <scratch space>:93:1: note: expanded from here
      93 | class_HARDIRQ_lock_ptr
         | ^~~~~~~~~~~~~~~~~~~~~~
   lib/locking-selftest.c:2511:1: warning: unused function 'class_NOTTHREADED_HARDIRQ_lock_ptr' [-Wunused-function]
    2511 | DEFINE_LOCK_GUARD_0(NOTTHREADED_HARDIRQ,
         | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    2512 |         do {
         |         ~~~~
    2513 |                 local_irq_disable();
         |                 ~~~~~~~~~~~~~~~~~~~~
    2514 |                 __irq_enter();
         |                 ~~~~~~~~~~~~~~
    2515 |                 WARN_ON(!in_irq());
         |                 ~~~~~~~~~~~~~~~~~~~
    2516 |         } while(0), HARDIRQ_EXIT())
         |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/cleanup.h:407:49: note: expanded from macro 'DEFINE_LOCK_GUARD_0'
     407 | __DEFINE_CLASS_IS_CONDITIONAL(_name, false);                            \
         |                                                                         ^
     408 | __DEFINE_UNLOCK_GUARD(_name, void, _unlock, __VA_ARGS__)                \
         | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/cleanup.h:378:21: note: expanded from macro '\
   __DEFINE_UNLOCK_GUARD'
     378 | static inline void *class_##_name##_lock_ptr(class_##_name##_t *_T)     \
         |                     ^~~~~~~~~~~~~~~~~~~~~~~~
   <scratch space>:121:1: note: expanded from here
     121 | class_NOTTHREADED_HARDIRQ_lock_ptr
         | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   lib/locking-selftest.c:2517:1: warning: unused function 'class_SOFTIRQ_lock_ptr' [-Wunused-function]
    2517 | DEFINE_LOCK_GUARD_0(SOFTIRQ, SOFTIRQ_ENTER(), SOFTIRQ_EXIT())
         | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/cleanup.h:407:49: note: expanded from macro 'DEFINE_LOCK_GUARD_0'
     407 | __DEFINE_CLASS_IS_CONDITIONAL(_name, false);                            \
         |                                                                         ^
     408 | __DEFINE_UNLOCK_GUARD(_name, void, _unlock, __VA_ARGS__)                \
         | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/cleanup.h:378:21: note: expanded from macro '\
   __DEFINE_UNLOCK_GUARD'
     378 | static inline void *class_##_name##_lock_ptr(class_##_name##_t *_T)     \
         |                     ^~~~~~~~~~~~~~~~~~~~~~~~
   <scratch space>:141:1: note: expanded from here
     141 | class_SOFTIRQ_lock_ptr
         | ^~~~~~~~~~~~~~~~~~~~~~
   lib/locking-selftest.c:2520:1: warning: unused function 'class_RCU_lock_ptr' [-Wunused-function]
    2520 | DEFINE_LOCK_GUARD_0(RCU, rcu_read_lock(), rcu_read_unlock())
         | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/cleanup.h:407:49: note: expanded from macro 'DEFINE_LOCK_GUARD_0'
     407 | __DEFINE_CLASS_IS_CONDITIONAL(_name, false);                            \
         |                                                                         ^
     408 | __DEFINE_UNLOCK_GUARD(_name, void, _unlock, __VA_ARGS__)                \
         | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/cleanup.h:378:21: note: expanded from macro '\
   __DEFINE_UNLOCK_GUARD'
     378 | static inline void *class_##_name##_lock_ptr(class_##_name##_t *_T)     \
         |                     ^~~~~~~~~~~~~~~~~~~~~~~~
   <scratch space>:159:1: note: expanded from here
     159 | class_RCU_lock_ptr
         | ^~~~~~~~~~~~~~~~~~
   lib/locking-selftest.c:2521:1: warning: unused function 'class_RCU_BH_lock_ptr' [-Wunused-function]
    2521 | DEFINE_LOCK_GUARD_0(RCU_BH, rcu_read_lock_bh(), rcu_read_unlock_bh())
         | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/cleanup.h:407:49: note: expanded from macro 'DEFINE_LOCK_GUARD_0'
     407 | __DEFINE_CLASS_IS_CONDITIONAL(_name, false);                            \
         |                                                                         ^
--
   In file included from kernel/bpf/kmem_cache_iter.c:3:
   In file included from include/linux/bpf.h:10:
   In file included from include/linux/workqueue.h:9:
   In file included from include/linux/timer.h:8:
   In file included from include/linux/debugobjects.h:6:
   In file included from include/linux/spinlock.h:63:
   In file included from include/linux/lockdep.h:14:
   In file included from include/linux/smp.h:15:
   In file included from include/linux/smp_types.h:5:
>> include/linux/llist.h:283:7: warning: variable 'pos' is uninitialized when used here [-Wuninitialized]
     283 |                 if (pos->next == entry) {
         |                     ^~~
   include/linux/llist.h:269:24: note: initialize the variable 'pos' to silence this warning
     269 |         struct llist_node *pos;
         |                               ^
         |                                = NULL
   In file included from kernel/bpf/kmem_cache_iter.c:3:
   In file included from include/linux/bpf.h:20:
   In file included from include/linux/module.h:19:
   In file included from include/linux/elf.h:6:
   In file included from arch/s390/include/asm/elf.h:181:
   In file included from arch/s390/include/asm/mmu_context.h:11:
   In file included from arch/s390/include/asm/pgalloc.h:18:
   In file included from include/linux/mm.h:2224:
   include/linux/vmstat.h:504:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     504 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     505 |                            item];
         |                            ~~~~
   include/linux/vmstat.h:511:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     511 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     512 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   include/linux/vmstat.h:524:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     524 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     525 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   kernel/bpf/kmem_cache_iter.c:227:27: warning: bitwise operation between different enumeration types ('enum bpf_reg_type' and 'enum bpf_type_flag') [-Wenum-enum-conversion]
     227 |                   PTR_TO_BTF_ID_OR_NULL | PTR_TRUSTED },
         |                   ~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~
   5 warnings generated.
..


vim +/pos +283 include/linux/llist.h

   255	
   256	/**
   257	 * llist_del_entry - remove a particular entry from a lock-less list
   258	 * @head: head of the list to remove the entry from
   259	 * @entry: entry to be removed from the list
   260	 *
   261	 * Walk the list, find the given entry and remove it from the list.
   262	 * The caller must ensure that nothing can race in and change the
   263	 * list while this is running.
   264	 *
   265	 * Returns true if the entry was found and removed.
   266	 */
   267	static inline bool llist_del_entry(struct llist_head *head, struct llist_node *entry)
   268	{
   269		struct llist_node *pos;
   270	
   271		if (!head->first)
   272			return false;
   273	
   274		/* Is it the first entry? */
   275		if (head->first == entry) {
   276			head->first = entry->next;
   277			entry->next = entry;
   278			return true;
   279		}
   280	
   281		/* Find it in the list */
   282		llist_for_each(head->first, pos) {
 > 283			if (pos->next == entry) {
   284				pos->next = entry->next;
   285				entry->next = entry;
   286				return true;
   287			}
   288		}
   289		return false;
   290	}
   291	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/3] llist: add ability to remove a particular entry from the list

[kernel test robot]

Hi Olga,

kernel test robot noticed the following build warnings:

[auto build test WARNING on trondmy-nfs/linux-next]
[also build test WARNING on brauner-vfs/vfs.all linus/master v6.13-rc7 next-20250116]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Olga-Kornievskaia/llist-add-ability-to-remove-a-particular-entry-from-the-list/20250116-072951
base:   git://git.linux-nfs.org/projects/trondmy/linux-nfs.git linux-next
patch link:    https://lore.kernel.org/r/20250115232406.44815-2-okorniev%40redhat.com
patch subject: [PATCH 1/3] llist: add ability to remove a particular entry from the list
config: x86_64-rhel-9.4-rust (https://download.01.org/0day-ci/archive/20250117/202501171112.wcOmBz6B-lkp@intel.com/config)
compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250117/202501171112.wcOmBz6B-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202501171112.wcOmBz6B-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> clang diag: include/linux/llist.h:283:7: warning: variable 'pos' is uninitialized when used here [-Wuninitialized]
   clang diag: include/linux/vmstat.h:504:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
   clang diag: include/linux/vmstat.h:511:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
   clang diag: include/linux/vmstat.h:524:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/3] llist: add ability to remove a particular entry from the list

[kernel test robot]

Hi Olga,

kernel test robot noticed the following build errors:

[auto build test ERROR on trondmy-nfs/linux-next]
[also build test ERROR on brauner-vfs/vfs.all linus/master v6.13 next-20250123]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Olga-Kornievskaia/llist-add-ability-to-remove-a-particular-entry-from-the-list/20250116-072951
base:   git://git.linux-nfs.org/projects/trondmy/linux-nfs.git linux-next
patch link:    https://lore.kernel.org/r/20250115232406.44815-2-okorniev%40redhat.com
patch subject: [PATCH 1/3] llist: add ability to remove a particular entry from the list
config: riscv-allmodconfig (https://download.01.org/0day-ci/archive/20250124/202501240848.ZH81rjTD-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 19306351a2c45e266fa11b41eb1362b20b6ca56d)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250124/202501240848.ZH81rjTD-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202501240848.ZH81rjTD-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from lib/test_bitops.c:10:
   In file included from include/linux/module.h:17:
   In file included from include/linux/kmod.h:9:
   In file included from include/linux/umh.h:4:
   In file included from include/linux/gfp.h:7:
   In file included from include/linux/mmzone.h:8:
   In file included from include/linux/spinlock.h:63:
   In file included from include/linux/lockdep.h:14:
   In file included from include/linux/smp.h:15:
   In file included from include/linux/smp_types.h:5:
>> include/linux/llist.h:283:7: error: variable 'pos' is uninitialized when used here [-Werror,-Wuninitialized]
     283 |                 if (pos->next == entry) {
         |                     ^~~
   include/linux/llist.h:269:24: note: initialize the variable 'pos' to silence this warning
     269 |         struct llist_node *pos;
         |                               ^
         |                                = NULL
   1 error generated.


vim +/pos +283 include/linux/llist.h

   255	
   256	/**
   257	 * llist_del_entry - remove a particular entry from a lock-less list
   258	 * @head: head of the list to remove the entry from
   259	 * @entry: entry to be removed from the list
   260	 *
   261	 * Walk the list, find the given entry and remove it from the list.
   262	 * The caller must ensure that nothing can race in and change the
   263	 * list while this is running.
   264	 *
   265	 * Returns true if the entry was found and removed.
   266	 */
   267	static inline bool llist_del_entry(struct llist_head *head, struct llist_node *entry)
   268	{
   269		struct llist_node *pos;
   270	
   271		if (!head->first)
   272			return false;
   273	
   274		/* Is it the first entry? */
   275		if (head->first == entry) {
   276			head->first = entry->next;
   277			entry->next = entry;
   278			return true;
   279		}
   280	
   281		/* Find it in the list */
   282		llist_for_each(head->first, pos) {
 > 283			if (pos->next == entry) {
   284				pos->next = entry->next;
   285				entry->next = entry;
   286				return true;
   287			}
   288		}
   289		return false;
   290	}
   291	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH 2/3] SUNRPC: add ability to remove specific server transport

[Olga Kornievskaia]

nfsd needs to be able to remove a particular entry from its
list of transports.

Suggested-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
---
 include/linux/sunrpc/svc.h |  1 +
 net/sunrpc/svc_xprt.c      | 11 +++++++++++
 2 files changed, 12 insertions(+)

diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index 74658cca0f38..0bc0b9ead01e 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -444,6 +444,7 @@ int		   svc_register(const struct svc_serv *, struct net *, const int,
 				const unsigned short, const unsigned short);
 
 void		   svc_wake_up(struct svc_serv *);
+void		   svc_xprt_dequeue_entry(struct svc_xprt *xprt);
 void		   svc_reserve(struct svc_rqst *rqstp, int space);
 void		   svc_pool_wake_idle_thread(struct svc_pool *pool);
 struct svc_pool   *svc_pool_for_cpu(struct svc_serv *serv);
diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
index 06779b4cdd0a..7b86e69df08b 100644
--- a/net/sunrpc/svc_xprt.c
+++ b/net/sunrpc/svc_xprt.c
@@ -507,6 +507,17 @@ static struct svc_xprt *svc_xprt_dequeue(struct svc_pool *pool)
 	return xprt;
 }
 
+void svc_xprt_dequeue_entry(struct svc_xprt *xprt)
+{
+	struct svc_pool *pool;
+
+	pool = svc_pool_for_cpu(xprt->xpt_server);
+
+	WARN_ON_ONCE(pool->sp_xprts.ready);
+	llist_del_entry(&pool->sp_xprts.new, &xprt->xpt_ready.node);
+}
+EXPORT_SYMBOL_GPL(svc_xprt_dequeue_entry);
+
 /**
  * svc_reserve - change the space reserved for the reply to a request.
  * @rqstp:  The request in question
-- 
2.47.1

[PATCH 3/3] nfsd: fix management of listener transports

[Olga Kornievskaia]

When a particular listener is being removed we need to make sure
that we delete the entry from the list of permanent sockets
(sv_permsocks) as well as remove it from the listener transports
(sp_xprts). When adding back the leftover transports not being
removed we need to clear XPT_BUSY flag so that it can be used.

Fixes: 16a471177496 ("NFSD: add listener-{set,get} netlink command")
Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
---
 fs/nfsd/nfsctl.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 95ea4393305b..3deedd511e83 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -1988,7 +1988,7 @@ int nfsd_nl_listener_set_doit(struct sk_buff *skb, struct genl_info *info)
 	/* 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);
+		list_del_init(&xprt->xpt_list);
 
 		/*
 		 * Newly-created sockets are born with the BUSY bit set. Clear
@@ -2000,6 +2000,7 @@ int nfsd_nl_listener_set_doit(struct sk_buff *skb, struct genl_info *info)
 
 		set_bit(XPT_CLOSE, &xprt->xpt_flags);
 		spin_unlock_bh(&serv->sv_lock);
+		svc_xprt_dequeue_entry(xprt);
 		svc_xprt_close(xprt);
 		spin_lock_bh(&serv->sv_lock);
 	}
@@ -2031,6 +2032,7 @@ 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) {
+			clear_bit(XPT_BUSY, &xprt->xpt_flags);
 			svc_xprt_put(xprt);
 			continue;
 		}
-- 
2.47.1

Re: [PATCH 3/3] nfsd: fix management of listener transports

[Chuck Lever]

On 1/15/25 6:24 PM, Olga Kornievskaia wrote:
> When a particular listener is being removed we need to make sure
> that we delete the entry from the list of permanent sockets
> (sv_permsocks) as well as remove it from the listener transports
> (sp_xprts). When adding back the leftover transports not being
> removed we need to clear XPT_BUSY flag so that it can be used.
> 
> Fixes: 16a471177496 ("NFSD: add listener-{set,get} netlink command")
> Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
> ---
>   fs/nfsd/nfsctl.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index 95ea4393305b..3deedd511e83 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -1988,7 +1988,7 @@ int nfsd_nl_listener_set_doit(struct sk_buff *skb, struct genl_info *info)
>   	/* 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);
> +		list_del_init(&xprt->xpt_list);
>   
>   		/*
>   		 * Newly-created sockets are born with the BUSY bit set. Clear
> @@ -2000,6 +2000,7 @@ int nfsd_nl_listener_set_doit(struct sk_buff *skb, struct genl_info *info)
>   
>   		set_bit(XPT_CLOSE, &xprt->xpt_flags);
>   		spin_unlock_bh(&serv->sv_lock);
> +		svc_xprt_dequeue_entry(xprt);

The kdoc comment in front of llist_del_entry() says:

+ * The caller must ensure that nothing can race in and change the
+ * list while this is running.

In this caller, I don't see how such a race is prevented.


>   		svc_xprt_close(xprt);
>   		spin_lock_bh(&serv->sv_lock);
>   	}
> @@ -2031,6 +2032,7 @@ 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) {
> +			clear_bit(XPT_BUSY, &xprt->xpt_flags);
>   			svc_xprt_put(xprt);
>   			continue;
>   		}


-- 
Chuck Lever

Re: [PATCH 3/3] nfsd: fix management of listener transports

[Jeff Layton]

On Thu, 2025-01-16 at 09:27 -0500, Chuck Lever wrote:
> On 1/15/25 6:24 PM, Olga Kornievskaia wrote:
> > When a particular listener is being removed we need to make sure
> > that we delete the entry from the list of permanent sockets
> > (sv_permsocks) as well as remove it from the listener transports
> > (sp_xprts). When adding back the leftover transports not being
> > removed we need to clear XPT_BUSY flag so that it can be used.
> > 
> > Fixes: 16a471177496 ("NFSD: add listener-{set,get} netlink command")
> > Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
> > ---
> >   fs/nfsd/nfsctl.c | 4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> > index 95ea4393305b..3deedd511e83 100644
> > --- a/fs/nfsd/nfsctl.c
> > +++ b/fs/nfsd/nfsctl.c
> > @@ -1988,7 +1988,7 @@ int nfsd_nl_listener_set_doit(struct sk_buff *skb, struct genl_info *info)
> >   	/* 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);
> > +		list_del_init(&xprt->xpt_list);
> >   
> >   		/*
> >   		 * Newly-created sockets are born with the BUSY bit set. Clear
> > @@ -2000,6 +2000,7 @@ int nfsd_nl_listener_set_doit(struct sk_buff *skb, struct genl_info *info)
> >   
> >   		set_bit(XPT_CLOSE, &xprt->xpt_flags);
> >   		spin_unlock_bh(&serv->sv_lock);
> > +		svc_xprt_dequeue_entry(xprt);
> 
> The kdoc comment in front of llist_del_entry() says:
> 
> + * The caller must ensure that nothing can race in and change the
> + * list while this is running.
> 
> In this caller, I don't see how such a race is prevented.
> 

This caller holds the nfsd_mutex, and prior to this, it does:

        /* For now, no removing old sockets while server is running */
        if (serv->sv_nrthreads && !list_empty(&permsocks)) {
                list_splice_init(&permsocks, &serv->sv_permsocks);
                spin_unlock_bh(&serv->sv_lock);
                err = -EBUSY;
                goto out_unlock_mtx;
        }

No nfsd threads are running and none can be started, so nothing is
processing the queue at this time. Some comments explaining that would
be a welcome addition here.

> 
> >   		svc_xprt_close(xprt);
> >   		spin_lock_bh(&serv->sv_lock);
> >   	}
> > @@ -2031,6 +2032,7 @@ 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) {
> > +			clear_bit(XPT_BUSY, &xprt->xpt_flags);
> >   			svc_xprt_put(xprt);
> >   			continue;
> >   		}
> 
> 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 3/3] nfsd: fix management of listener transports

[Chuck Lever]

On 1/16/25 9:46 AM, Jeff Layton wrote:
> On Thu, 2025-01-16 at 09:27 -0500, Chuck Lever wrote:
>> On 1/15/25 6:24 PM, Olga Kornievskaia wrote:
>>> When a particular listener is being removed we need to make sure
>>> that we delete the entry from the list of permanent sockets
>>> (sv_permsocks) as well as remove it from the listener transports
>>> (sp_xprts). When adding back the leftover transports not being
>>> removed we need to clear XPT_BUSY flag so that it can be used.
>>>
>>> Fixes: 16a471177496 ("NFSD: add listener-{set,get} netlink command")
>>> Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
>>> ---
>>>    fs/nfsd/nfsctl.c | 4 +++-
>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
>>> index 95ea4393305b..3deedd511e83 100644
>>> --- a/fs/nfsd/nfsctl.c
>>> +++ b/fs/nfsd/nfsctl.c
>>> @@ -1988,7 +1988,7 @@ int nfsd_nl_listener_set_doit(struct sk_buff *skb, struct genl_info *info)
>>>    	/* 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);
>>> +		list_del_init(&xprt->xpt_list);
>>>    
>>>    		/*
>>>    		 * Newly-created sockets are born with the BUSY bit set. Clear
>>> @@ -2000,6 +2000,7 @@ int nfsd_nl_listener_set_doit(struct sk_buff *skb, struct genl_info *info)
>>>    
>>>    		set_bit(XPT_CLOSE, &xprt->xpt_flags);
>>>    		spin_unlock_bh(&serv->sv_lock);
>>> +		svc_xprt_dequeue_entry(xprt);
>>
>> The kdoc comment in front of llist_del_entry() says:
>>
>> + * The caller must ensure that nothing can race in and change the
>> + * list while this is running.
>>
>> In this caller, I don't see how such a race is prevented.
>>
> 
> This caller holds the nfsd_mutex, and prior to this, it does:
> 
>          /* For now, no removing old sockets while server is running */
>          if (serv->sv_nrthreads && !list_empty(&permsocks)) {
>                  list_splice_init(&permsocks, &serv->sv_permsocks);
>                  spin_unlock_bh(&serv->sv_lock);
>                  err = -EBUSY;
>                  goto out_unlock_mtx;
>          }
> 
> No nfsd threads are running and none can be started, so nothing is
> processing the queue at this time. Some comments explaining that would
> be a welcome addition here.

So this would also block incoming accepts on another (active) socket?

Yeah, that's not obvious.


>>>    		svc_xprt_close(xprt);
>>>    		spin_lock_bh(&serv->sv_lock);
>>>    	}
>>> @@ -2031,6 +2032,7 @@ 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) {
>>> +			clear_bit(XPT_BUSY, &xprt->xpt_flags);
>>>    			svc_xprt_put(xprt);
>>>    			continue;
>>>    		}
>>
>>
> 


-- 
Chuck Lever

Re: [PATCH 3/3] nfsd: fix management of listener transports

[Olga Kornievskaia]

On Thu, Jan 16, 2025 at 9:55 AM Chuck Lever <chuck.lever@oracle.com> wrote:
>
> On 1/16/25 9:46 AM, Jeff Layton wrote:
> > On Thu, 2025-01-16 at 09:27 -0500, Chuck Lever wrote:
> >> On 1/15/25 6:24 PM, Olga Kornievskaia wrote:
> >>> When a particular listener is being removed we need to make sure
> >>> that we delete the entry from the list of permanent sockets
> >>> (sv_permsocks) as well as remove it from the listener transports
> >>> (sp_xprts). When adding back the leftover transports not being
> >>> removed we need to clear XPT_BUSY flag so that it can be used.
> >>>
> >>> Fixes: 16a471177496 ("NFSD: add listener-{set,get} netlink command")
> >>> Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
> >>> ---
> >>>    fs/nfsd/nfsctl.c | 4 +++-
> >>>    1 file changed, 3 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> >>> index 95ea4393305b..3deedd511e83 100644
> >>> --- a/fs/nfsd/nfsctl.c
> >>> +++ b/fs/nfsd/nfsctl.c
> >>> @@ -1988,7 +1988,7 @@ int nfsd_nl_listener_set_doit(struct sk_buff *skb, struct genl_info *info)
> >>>     /* 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);
> >>> +           list_del_init(&xprt->xpt_list);
> >>>
> >>>             /*
> >>>              * Newly-created sockets are born with the BUSY bit set. Clear
> >>> @@ -2000,6 +2000,7 @@ int nfsd_nl_listener_set_doit(struct sk_buff *skb, struct genl_info *info)
> >>>
> >>>             set_bit(XPT_CLOSE, &xprt->xpt_flags);
> >>>             spin_unlock_bh(&serv->sv_lock);
> >>> +           svc_xprt_dequeue_entry(xprt);
> >>
> >> The kdoc comment in front of llist_del_entry() says:
> >>
> >> + * The caller must ensure that nothing can race in and change the
> >> + * list while this is running.
> >>
> >> In this caller, I don't see how such a race is prevented.
> >>
> >
> > This caller holds the nfsd_mutex, and prior to this, it does:
> >
> >          /* For now, no removing old sockets while server is running */
> >          if (serv->sv_nrthreads && !list_empty(&permsocks)) {
> >                  list_splice_init(&permsocks, &serv->sv_permsocks);
> >                  spin_unlock_bh(&serv->sv_lock);
> >                  err = -EBUSY;
> >                  goto out_unlock_mtx;
> >          }
> >
> > No nfsd threads are running and none can be started, so nothing is
> > processing the queue at this time. Some comments explaining that would
> > be a welcome addition here.
>
> So this would also block incoming accepts on another (active) socket?
>
> Yeah, that's not obvious.

I read these 2 comments as "more comments are needed" but I'm failing
to see what is not obvious about knowing that nothing can be running
because in the beginning of the function nfsd_mutex was acquired? And
there is already a comment in the quoted code.

I have contemplated that instead of introducing a new function into
code that isn't NFS (ie llist.h), perhaps we re-write the
nfsd_nl_listener_set_doit() to remove all from the existing transports
from lists (permsocks and sp_xprts) and create all new instead? But it
does seem an overkill for simply needing to remove something from the
list instead.

> >>>             svc_xprt_close(xprt);
> >>>             spin_lock_bh(&serv->sv_lock);
> >>>     }
> >>> @@ -2031,6 +2032,7 @@ 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) {
> >>> +                   clear_bit(XPT_BUSY, &xprt->xpt_flags);
> >>>                     svc_xprt_put(xprt);
> >>>                     continue;
> >>>             }
> >>
> >>
> >
>
>
> --
> Chuck Lever
>

Re: [PATCH 3/3] nfsd: fix management of listener transports

[Chuck Lever]

On 1/16/25 10:34 AM, Olga Kornievskaia wrote:
> On Thu, Jan 16, 2025 at 9:55 AM Chuck Lever <chuck.lever@oracle.com> wrote:
>>
>> On 1/16/25 9:46 AM, Jeff Layton wrote:
>>> On Thu, 2025-01-16 at 09:27 -0500, Chuck Lever wrote:
>>>> On 1/15/25 6:24 PM, Olga Kornievskaia wrote:
>>>>> When a particular listener is being removed we need to make sure
>>>>> that we delete the entry from the list of permanent sockets
>>>>> (sv_permsocks) as well as remove it from the listener transports
>>>>> (sp_xprts). When adding back the leftover transports not being
>>>>> removed we need to clear XPT_BUSY flag so that it can be used.
>>>>>
>>>>> Fixes: 16a471177496 ("NFSD: add listener-{set,get} netlink command")
>>>>> Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
>>>>> ---
>>>>>     fs/nfsd/nfsctl.c | 4 +++-
>>>>>     1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
>>>>> index 95ea4393305b..3deedd511e83 100644
>>>>> --- a/fs/nfsd/nfsctl.c
>>>>> +++ b/fs/nfsd/nfsctl.c
>>>>> @@ -1988,7 +1988,7 @@ int nfsd_nl_listener_set_doit(struct sk_buff *skb, struct genl_info *info)
>>>>>      /* 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);
>>>>> +           list_del_init(&xprt->xpt_list);
>>>>>
>>>>>              /*
>>>>>               * Newly-created sockets are born with the BUSY bit set. Clear
>>>>> @@ -2000,6 +2000,7 @@ int nfsd_nl_listener_set_doit(struct sk_buff *skb, struct genl_info *info)
>>>>>
>>>>>              set_bit(XPT_CLOSE, &xprt->xpt_flags);
>>>>>              spin_unlock_bh(&serv->sv_lock);
>>>>> +           svc_xprt_dequeue_entry(xprt);
>>>>
>>>> The kdoc comment in front of llist_del_entry() says:
>>>>
>>>> + * The caller must ensure that nothing can race in and change the
>>>> + * list while this is running.
>>>>
>>>> In this caller, I don't see how such a race is prevented.
>>>>
>>>
>>> This caller holds the nfsd_mutex, and prior to this, it does:
>>>
>>>           /* For now, no removing old sockets while server is running */
>>>           if (serv->sv_nrthreads && !list_empty(&permsocks)) {
>>>                   list_splice_init(&permsocks, &serv->sv_permsocks);
>>>                   spin_unlock_bh(&serv->sv_lock);
>>>                   err = -EBUSY;
>>>                   goto out_unlock_mtx;
>>>           }
>>>
>>> No nfsd threads are running and none can be started, so nothing is
>>> processing the queue at this time. Some comments explaining that would
>>> be a welcome addition here.
>>
>> So this would also block incoming accepts on another (active) socket?
>>
>> Yeah, that's not obvious.
> 
> I read these 2 comments as "more comments are needed" but I'm failing
> to see what is not obvious about knowing that nothing can be running
> because in the beginning of the function nfsd_mutex was acquired? And
> there is already a comment in the quoted code.

The patch appears to reviewers as a diff. There is no part of the
"For now, no removing" code that appears in this fix. Even when
going back to the source code and viewing the change in context,
the "For now" code block is far enough above the new dequeue_entry()
call site that it's simply not obvious what's going on.

A new comment here might read

	/* We've already corked new work above, so this is safe: */


> I have contemplated that instead of introducing a new function into
> code that isn't NFS (ie llist.h), perhaps we re-write the
> nfsd_nl_listener_set_doit() to remove all from the existing transports
> from lists (permsocks and sp_xprts) and create all new instead? But it
> does seem an overkill for simply needing to remove something from the
> list instead.

Or change the management of "permanent sockets" to use a different
data structure, possibly. The temporary sockets see high traffic and
benefit from the waitless list, but listeners can use something more
conventional, as long as the thread scheduling logic looks for work
there.


>>>>>              svc_xprt_close(xprt);
>>>>>              spin_lock_bh(&serv->sv_lock);
>>>>>      }
>>>>> @@ -2031,6 +2032,7 @@ 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) {
>>>>> +                   clear_bit(XPT_BUSY, &xprt->xpt_flags);
>>>>>                      svc_xprt_put(xprt);
>>>>>                      continue;
>>>>>              }
>>>>
>>>>
>>>
>>
>>
>> --
>> Chuck Lever
>>
> 


-- 
Chuck Lever

Re: [PATCH 0/3] fix removal of nfsd listeners

[Jeff Layton]

On Wed, 2025-01-15 at 18:24 -0500, Olga Kornievskaia wrote:
> Currently if a root user using nfsdctl command tries to remove a particular
> listener from the list of previously added ones, then starting the nfsd
> 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 svc_xprt_close() the specified listener but
> the other list of transports (server's sp_xprts list) would not be
> changed leading to the problem.
> 
> the other problem is that sp_xprt is a lwq structure of lockless
> list which does not have an ability to remove a single entry from
> the list.
> 
> this patch series addis a function to remove a single entry, then modifies
> nfsd_nl_listener_set_doit() to make sure the to-be-removed listener is
> removed from both lists and then it also ensures that the remaining
> listeners are added back in the correct state.
> 
> Olga Kornievskaia (3):
>   llist: add ability to remove a particular entry from the list
>   SUNRPC: add ability to remove specific server transport
>   nfsd: fix management of listener transports
> 
>  fs/nfsd/nfsctl.c           |  4 +++-
>  include/linux/llist.h      | 36 ++++++++++++++++++++++++++++++++++++
>  include/linux/sunrpc/svc.h |  1 +
>  net/sunrpc/svc_xprt.c      | 11 +++++++++++
>  4 files changed, 51 insertions(+), 1 deletion(-)
> 

Nice work, Olga.

Reviewed-by: Jeff Layton <jlayton@kernel.org>