public inbox for linux-usb@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH net v3 04/11] list: Move on_list_rcu() to list.h and add on_list() also
       [not found] <20260326131838.634095-1-dhowells@redhat.com>
@ 2026-03-26 13:18 ` David Howells
  2026-03-29 19:12   ` Jakub Kicinski
  2026-03-29 19:14   ` Jakub Kicinski
  0 siblings, 2 replies; 4+ messages in thread
From: David Howells @ 2026-03-26 13:18 UTC (permalink / raw)
  To: netdev
  Cc: David Howells, Marc Dionne, Jakub Kicinski, David S. Miller,
	Eric Dumazet, Paolo Abeni, linux-afs, linux-kernel,
	Mathieu Desnoyers, John Johansen, Minas Harutyunyan, Simon Horman,
	apparmor, linux-usb, stable

Unfortunately, list_empty() is not usable with an entry that has been
removed from a list with list_del_rcu() as ->next must be left pointing at
the following entry so as not to break traversal under RCU.

Solve this by moving on_list_rcu() from AppArmor to linux/list.h, and
turning it into an inline function.

Also add an on_list() counterpart (functionally, this is just an antonym
for list_empty()), but the name looks less awkward when applied to a
non-head element.  We probably don't want to use on_list_rcu() generally
because it requires an extra check as ->prev is set differently in the two
cases.

Further, rename the on_list() function in the Designware usb2 drd ip driver
to dwc2_on_list() to free up the original name.

Signed-off-by: David Howells <dhowells@redhat.com>
cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
cc: John Johansen <john.johansen@canonical.com>
cc: Minas Harutyunyan <hminas@synopsys.com>
cc: Marc Dionne <marc.dionne@auristor.com>
cc: Eric Dumazet <edumazet@google.com>
cc: "David S. Miller" <davem@davemloft.net>
cc: Jakub Kicinski <kuba@kernel.org>
cc: Paolo Abeni <pabeni@redhat.com>
cc: Simon Horman <horms@kernel.org>
cc: linux-afs@lists.infradead.org
cc: apparmor@lists.ubuntu.com
cc: linux-usb@vger.kernel.org
cc: netdev@vger.kernel.org
cc: stable@kernel.org
---
 drivers/usb/dwc2/gadget.c          |  6 +++---
 include/linux/list.h               | 26 ++++++++++++++++++++++++++
 security/apparmor/include/policy.h |  2 --
 3 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index d216e26c787b..04b6aef8ac13 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -4306,11 +4306,11 @@ static int dwc2_hsotg_ep_disable_lock(struct usb_ep *ep)
 }
 
 /**
- * on_list - check request is on the given endpoint
+ * dwc2_on_list - check request is on the given endpoint
  * @ep: The endpoint to check.
  * @test: The request to test if it is on the endpoint.
  */
-static bool on_list(struct dwc2_hsotg_ep *ep, struct dwc2_hsotg_req *test)
+static bool dwc2_on_list(struct dwc2_hsotg_ep *ep, struct dwc2_hsotg_req *test)
 {
 	struct dwc2_hsotg_req *req, *treq;
 
@@ -4338,7 +4338,7 @@ static int dwc2_hsotg_ep_dequeue(struct usb_ep *ep, struct usb_request *req)
 
 	spin_lock_irqsave(&hs->lock, flags);
 
-	if (!on_list(hs_ep, hs_req)) {
+	if (!dwc2_on_list(hs_ep, hs_req)) {
 		spin_unlock_irqrestore(&hs->lock, flags);
 		return -EINVAL;
 	}
diff --git a/include/linux/list.h b/include/linux/list.h
index 00ea8e5fb88b..d224e7210d1b 100644
--- a/include/linux/list.h
+++ b/include/linux/list.h
@@ -381,6 +381,32 @@ static inline int list_empty(const struct list_head *head)
 	return READ_ONCE(head->next) == head;
 }
 
+/**
+ * on_list - Test whether an entry is on a list.
+ * @entry: The entry to check
+ *
+ * Test whether an entry is on a list.  Safe to use on an entry initialised
+ * with INIT_LIST_HEAD() or LIST_HEAD() or removed with things like
+ * list_del_init().  Not safe for use with list_del() or list_del_rcu().
+ */
+static inline bool on_list(const struct list_head *entry)
+{
+	return !list_empty(entry);
+}
+
+/**
+ * on_list_rcu - Test whether an entry is on a list (RCU-del safe).
+ * @entry: The entry to check
+ *
+ * Test whether an entry is on a list.  Safe to use on an entry initialised
+ * with INIT_LIST_HEAD() or LIST_HEAD() or removed with things like
+ * list_del_init().  Also safe for use with list_del() or list_del_rcu().
+ */
+static inline bool on_list_rcu(const struct list_head *entry)
+{
+	return !list_empty(entry) && entry->prev != LIST_POISON2;
+}
+
 /**
  * list_del_init_careful - deletes entry from list and reinitialize it.
  * @entry: the element to delete from the list.
diff --git a/security/apparmor/include/policy.h b/security/apparmor/include/policy.h
index 3895f8774a3f..c3697c23bbed 100644
--- a/security/apparmor/include/policy.h
+++ b/security/apparmor/include/policy.h
@@ -57,8 +57,6 @@ extern const char *const aa_profile_mode_names[];
 
 #define profile_is_stale(_profile) (label_is_stale(&(_profile)->label))
 
-#define on_list_rcu(X) (!list_empty(X) && (X)->prev != LIST_POISON2)
-
 /* flags in the dfa accept2 table */
 enum dfa_accept_flags {
 	ACCEPT_FLAG_OWNER = 1,


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH net v3 04/11] list: Move on_list_rcu() to list.h and add on_list() also
  2026-03-26 13:18 ` [PATCH net v3 04/11] list: Move on_list_rcu() to list.h and add on_list() also David Howells
@ 2026-03-29 19:12   ` Jakub Kicinski
  2026-03-29 20:01     ` Linus Torvalds
  2026-03-29 19:14   ` Jakub Kicinski
  1 sibling, 1 reply; 4+ messages in thread
From: Jakub Kicinski @ 2026-03-29 19:12 UTC (permalink / raw)
  To: David Howells
  Cc: netdev, Marc Dionne, David S. Miller, Eric Dumazet, Paolo Abeni,
	linux-afs, linux-kernel, Mathieu Desnoyers, John Johansen,
	Minas Harutyunyan, Simon Horman, apparmor, linux-usb, stable,
	Linus Torvalds

On Thu, 26 Mar 2026 13:18:29 +0000 David Howells wrote:
> diff --git a/include/linux/list.h b/include/linux/list.h
> index 00ea8e5fb88b..d224e7210d1b 100644
> --- a/include/linux/list.h
> +++ b/include/linux/list.h
> @@ -381,6 +381,32 @@ static inline int list_empty(const struct list_head *head)
>  	return READ_ONCE(head->next) == head;
>  }
>  
> +/**
> + * on_list - Test whether an entry is on a list.
> + * @entry: The entry to check
> + *
> + * Test whether an entry is on a list.  Safe to use on an entry initialised
> + * with INIT_LIST_HEAD() or LIST_HEAD() or removed with things like
> + * list_del_init().  Not safe for use with list_del() or list_del_rcu().
> + */
> +static inline bool on_list(const struct list_head *entry)
> +{
> +	return !list_empty(entry);
> +}
> +
> +/**
> + * on_list_rcu - Test whether an entry is on a list (RCU-del safe).
> + * @entry: The entry to check
> + *
> + * Test whether an entry is on a list.  Safe to use on an entry initialised
> + * with INIT_LIST_HEAD() or LIST_HEAD() or removed with things like
> + * list_del_init().  Also safe for use with list_del() or list_del_rcu().
> + */
> +static inline bool on_list_rcu(const struct list_head *entry)
> +{
> +	return !list_empty(entry) && entry->prev != LIST_POISON2;
> +}

Could someone with sufficient weight to their name ack this?

The non-RCU version of on_list() does not sit well with me.
It provides no additional semantics above list_empty() and
the uninit / poison-related gotchas more obvious when typing
!list_empty(&entry->list).

I can believe the RCU version is more useful. It could probably
be used on both RCU and non-RCU entries?

Last minor nit - the list API consistently uses list_ as a prefix.
I have no better name to suggest but it's sad that on_list_rcu()
breaks that.

I think you're missing a READ_ONCE(), BTW.

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH net v3 04/11] list: Move on_list_rcu() to list.h and add on_list() also
  2026-03-26 13:18 ` [PATCH net v3 04/11] list: Move on_list_rcu() to list.h and add on_list() also David Howells
  2026-03-29 19:12   ` Jakub Kicinski
@ 2026-03-29 19:14   ` Jakub Kicinski
  1 sibling, 0 replies; 4+ messages in thread
From: Jakub Kicinski @ 2026-03-29 19:14 UTC (permalink / raw)
  To: David Howells
  Cc: netdev, Marc Dionne, David S. Miller, Eric Dumazet, Paolo Abeni,
	linux-afs, linux-kernel, Mathieu Desnoyers, John Johansen,
	Minas Harutyunyan, Simon Horman, apparmor, linux-usb, stable

On Thu, 26 Mar 2026 13:18:29 +0000 David Howells wrote:
> Unfortunately, list_empty() is not usable with an entry that has been
> removed from a list with list_del_rcu() as ->next must be left pointing at
> the following entry so as not to break traversal under RCU.

This seems to break build for jffs.
Someone already marked this as rejected in PW.
Whoever it was PLEASE STOP.

Quoting documentation:

  Updating patch status
  ~~~~~~~~~~~~~~~~~~~~~
  
  Contributors and reviewers do not have the permissions to update patch
  state directly in patchwork. Patchwork doesn't expose much information
  about the history of the state of patches, therefore having multiple
  people update the state leads to confusion.
  
  Instead of delegating patchwork permissions netdev uses a simple mail
  bot which looks for special commands/lines within the emails sent to
  the mailing list. For example to mark a series as Changes Requested
  one needs to send the following line anywhere in the email thread::
  
    pw-bot: changes-requested
  
  As a result the bot will set the entire series to Changes Requested.
  This may be useful when author discovers a bug in their own series
  and wants to prevent it from getting applied.
  
  The use of the bot is entirely optional, if in doubt ignore its
  existence completely. Maintainers will classify and update the state
  of the patches themselves. No email should ever be sent to the list
  with the main purpose of communicating with the bot, the bot commands
  should be seen as metadata. 
  The use of the bot is restricted to authors of the patches (the
  ``From:`` header on patch submission and command must match!),
  maintainers of the modified code according to the MAINTAINERS file
  (again, ``From:`` must match the MAINTAINERS entry) and a handful of
  senior reviewers. 
  Bot records its activity here:
  
    https://netdev.bots.linux.dev/pw-bot.html
  
See:
  https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#updating-patch-status

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH net v3 04/11] list: Move on_list_rcu() to list.h and add on_list() also
  2026-03-29 19:12   ` Jakub Kicinski
@ 2026-03-29 20:01     ` Linus Torvalds
  0 siblings, 0 replies; 4+ messages in thread
From: Linus Torvalds @ 2026-03-29 20:01 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: David Howells, netdev, Marc Dionne, David S. Miller, Eric Dumazet,
	Paolo Abeni, linux-afs, linux-kernel, Mathieu Desnoyers,
	John Johansen, Minas Harutyunyan, Simon Horman, apparmor,
	linux-usb, stable

On Sun, 29 Mar 2026 at 12:12, Jakub Kicinski <kuba@kernel.org> wrote:
>
> Could someone with sufficient weight to their name ack this?

I don't particularly like it. I think the name is too generic, and
it's all wrong anyway. Whether something is on a list or not ends up
being much too specific to the use-case, and I do *not* see a huge
advantage to a helper function that just wraps "list_empty()" with
another name that is actually *less* descriptive.

So no. NAK.

As you mention, the RCU version at least does something else, but
honestly, it looks pretty damn questionable too. And no, it does not
work for non-RCU lists in any kind of generic sense, since iut's
perfectly valid to remove list entries without poisoning them.,

For example, some places that want a simple "am I on a list" will use
__list_del_clearprev(), which does *not* poison the prev pointer, but
just clears it instead. And then the "am I on a list" is just checking
prev for NULL or not.

In other words: all of this is wrong. Whether you are on a list or not
is simply not a generic operation. It depends on the user.

The name is also *MUCH* too generic anyway.

               Linus

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2026-03-29 20:02 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <20260326131838.634095-1-dhowells@redhat.com>
2026-03-26 13:18 ` [PATCH net v3 04/11] list: Move on_list_rcu() to list.h and add on_list() also David Howells
2026-03-29 19:12   ` Jakub Kicinski
2026-03-29 20:01     ` Linus Torvalds
2026-03-29 19:14   ` Jakub Kicinski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox