Re: [PATCH v2 1/5] include/linux/rculist_nulls: add hlist_nulls_add_behind_rcu()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] rculist_nulls: missing next->pprev update in hlist_nulls_add_behind_rcu()
--

commit 32c8749135647f4c1c26a14e364aff3736e91e2e
Author: Max Kellermann <max.kellermann@ionos.com>

include/linux/rculist_nulls: add hlist_nulls_add_behind_rcu()

This commit factors the non-empty tail insertion logic out of
hlist_nulls_add_tail_rcu() into a new hlist_nulls_add_behind_rcu() helper
so callers tracking the last node can append in constant time.

> diff --git a/include/linux/rculist_nulls.h b/include/linux/rculist_nulls.h
> index a97c3bcb1656e..054e477dd0937 100644
> --- a/include/linux/rculist_nulls.h
> +++ b/include/linux/rculist_nulls.h
> @@ -115,6 +115,33 @@ static inline void hlist_nulls_add_head_rcu(struct hlist_nulls_node *n,
>  		WRITE_ONCE(first->pprev, &n->next);
>  }
>  
> +/**
> + * hlist_nulls_add_behind_rcu - append after the last list entry
> + * @n: the new element to add to the hash list.
> + * @last: the last element in the list.
> + *
> + * Adds the specified element after @last while permitting racing RCU
> + * traversals.  @last must be the last list entry, with its next
> + * pointer holding the terminating nulls marker.
> + *
> + * The caller must take whatever precautions are necessary
> + * (such as holding appropriate locks) to avoid racing
> + * with another list-mutation primitive, such as hlist_nulls_add_head_rcu()
> + * or hlist_nulls_del_rcu(), running on this same list.
> + * However, it is perfectly legal to run concurrently with
> + * the _rcu list-traversal primitives, such as
> + * hlist_nulls_for_each_entry_rcu(), used to prevent memory-consistency
> + * problems on Alpha CPUs.  Regardless of the type of CPU, the
> + * list-traversal primitive must be guarded by rcu_read_lock().
> + */
> +static inline void hlist_nulls_add_behind_rcu(struct hlist_nulls_node *n,
> +					      struct hlist_nulls_node *last)
> +{
> +	WRITE_ONCE(n->next, last->next);
> +	WRITE_ONCE(n->pprev, &last->next);
> +	rcu_assign_pointer(hlist_nulls_next_rcu(last), n);
> +}

[Severity: Medium]
Should hlist_nulls_add_behind_rcu() in include/linux/rculist_nulls.h also
update n->next->pprev when !is_a_nulls(n->next), matching
hlist_add_behind_rcu() in include/linux/rculist.h?

In include/linux/rculist.h, hlist_add_behind_rcu() supports inserting behind
any existing node prev in the list by updating the successor's pprev pointer:

include/linux/rculist.h:hlist_add_behind_rcu() {
	n->next = prev->next;
	WRITE_ONCE(n->pprev, &prev->next);
	rcu_assign_pointer(hlist_next_rcu(prev), n);
	if (n->next)
		WRITE_ONCE(n->next->pprev, &n->next);
}

The other *_add_behind() primitives, hlist_add_behind() in
include/linux/list.h and hlist_bl_add_behind() in include/linux/list_bl.h,
similarly update n->next->pprev so they can be used on interior nodes as well
as the tail.

With hlist_nulls_add_behind_rcu() omitting that update, if a caller passes a
node whose next pointer is not a nulls marker (!is_a_nulls(last->next)),
n->next->pprev remains pointing to &last->next instead of &n->next. Removing
the successor via hlist_nulls_del_rcu() -> __hlist_nulls_del() or replacing
it via hlist_nulls_replace_rcu() would then overwrite last->next and silently
drop n from the list, or cause a use-after-free write if last was removed and
freed first.

Could hlist_nulls_add_behind_rcu() check if (!is_a_nulls(n->next)) and set
WRITE_ONCE(n->next->pprev, &n->next) just like hlist_nulls_add_head_rcu() and
hlist_nulls_replace_rcu() in the same file?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103113711.1832834-1-max.kellermann@ionos.com?part=1