Re: [PATCH RFC nf-next 10/12] netfilter: ipset: use correct lockdep annotation in ipset_dereference

Hi Florian,

On Tue, 14 Jul 2026, Florian Westphal wrote:

> Avoid always-true arguments where possible, they defeat lockdep.
>
> ip_set_comment_free() is problematic: called from different contexts,
> some hold set->lock spinlock (safe), some do not hold a lock but have other
> means of mutual exclusion (e.g., entire set torn down).

Wouldn't something like the following be sufficient?

- Add a bool "deleted" element to struct ip_set.
- The set-specific destroy function would set it true before
   doing anything else.
- Then it'd be safe to use

#define ipset_dereference_locked(p, set)	\
 	rcu_dereference_protected(p, lockdep_is_held(&set->lock) || \
 				     set->deleted))

> Other callers need investigation: ip_set_comment_free() alters
> set->ext_size in a non-atomic way.  I don't see how this is safe except
> for "entire set is destroyed" case: parallel usage would be a bug.

Maybe we should convert ext_size to atomic64_t?

Best regards,
Jozsef
> Add a few lockdep assertions to ip_set_init_comment() callpaths to have
> more confidence in the correctness of the
> "Called from uadd only, protected by the set spinlock." comment at the
> start of ip_set_init_comment().
>
> Assisted-by: Claude:claude-opus-4-6
> Signed-off-by: Florian Westphal <[email protected]>
> ---
> include/linux/netfilter/ipset/ip_set.h | 3 +++
> net/netfilter/ipset/ip_set_core.c      | 3 +--
> net/netfilter/ipset/ip_set_hash_gen.h  | 6 ++----
> net/netfilter/ipset/ip_set_list_set.c  | 2 ++
> 4 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/netfilter/ipset/ip_set.h b/include/linux/netfilter/ipset/ip_set.h
> index f9003ec21259..99bc997914f4 100644
> --- a/include/linux/netfilter/ipset/ip_set.h
> +++ b/include/linux/netfilter/ipset/ip_set.h
> @@ -282,6 +282,9 @@ struct ip_set {
> 	void *data;
> };
>
> +#define ipset_dereference_locked(p, set)		\
> +	rcu_dereference_protected(p, lockdep_is_held(&set->lock))
> +
> static inline void
> __ip_set_destroy_comment(struct ip_set *set, void *data)
> {
> diff --git a/net/netfilter/ipset/ip_set_core.c b/net/netfilter/ipset/ip_set_core.c
> index 6ece5cf305fe..a5f77f639d2a 100644
> --- a/net/netfilter/ipset/ip_set_core.c
> +++ b/net/netfilter/ipset/ip_set_core.c
> @@ -346,8 +346,7 @@ void
> ip_set_init_comment(struct ip_set *set, struct ip_set_comment *comment,
> 		    const struct ip_set_ext *ext)
> {
> -	struct ip_set_comment_rcu *c = rcu_dereference_protected(comment->c,
> -								 lockdep_is_held(&set->lock));
> +	struct ip_set_comment_rcu *c = ipset_dereference_locked(comment->c, set);
> 	size_t len = ext->comment ? strlen(ext->comment) : 0;
>
> 	if (unlikely(c)) {
> diff --git a/net/netfilter/ipset/ip_set_hash_gen.h b/net/netfilter/ipset/ip_set_hash_gen.h
> index a0f2cd481b82..e615de2e616b 100644
> --- a/net/netfilter/ipset/ip_set_hash_gen.h
> +++ b/net/netfilter/ipset/ip_set_hash_gen.h
> @@ -11,8 +11,6 @@
> #include <linux/netfilter/nfnetlink.h>
> #include <linux/netfilter/ipset/ip_set.h>
>
> -#define __ipset_dereference(p)		\
> -	rcu_dereference_protected(p, 1)
> #define ipset_dereference_nfnl(p)	\
> 	rcu_dereference_protected(p,	\
> 		lockdep_nfnl_is_held(NFNL_SUBSYS_IPSET))
> @@ -271,7 +269,7 @@ mtype_add_cidr(struct ip_set *set, struct htype *h, u8 cidr, u8 n)
> 	int i, j, found, len = 0, ret = 0;
>
> 	spin_lock_bh(&set->lock);
> -	nets = __ipset_dereference(h->rnets[n]);
> +	nets = ipset_dereference_locked(h->rnets[n], set);
> 	/* Add in increasing prefix order, so larger cidr first */
> 	for (i = 0, found = -1; i < nets->len; i++) {
> 		if (nets->nets[i].count)
> @@ -323,7 +321,7 @@ mtype_del_cidr(struct ip_set *set, struct htype *h, u8 cidr, u8 n)
> 	int found;
>
> 	spin_lock_bh(&set->lock);
> -	nets = __ipset_dereference(h->rnets[n]);
> +	nets = ipset_dereference_locked(h->rnets[n], set);
> 	for (i = 0, found = -1; i < nets->len; i++) {
> 		if (nets->nets[i].count)
> 			len++;
> diff --git a/net/netfilter/ipset/ip_set_list_set.c b/net/netfilter/ipset/ip_set_list_set.c
> index d7ddc57a4eca..27bc96458e13 100644
> --- a/net/netfilter/ipset/ip_set_list_set.c
> +++ b/net/netfilter/ipset/ip_set_list_set.c
> @@ -326,6 +326,8 @@ list_set_udel(struct ip_set *set, void *value, const struct ip_set_ext *ext,
> 	struct set_adt_elem *d = value;
> 	struct set_elem *e, *n, *next, *prev = NULL;
>
> +	lockdep_assert_held(&set->lock);
> +
> 	list_for_each_entry_safe(e, n, &map->members, list) {
> 		if (SET_WITH_TIMEOUT(set) &&
> 		    ip_set_timeout_expired(ext_timeout(e, set)))
> -- 
> 2.54.0
>
>
>