Re: [PATCH v2 14/14] list: Cache cursors in entry iterators

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Modifying the standard list iterators to pre-cache the `next` pointer fundamentally changes their traversal semantics from dynamic to static evaluation, causing use-after-free (UAF) vulnerabilities and silently breaking loops that modify the list ahead of the cursor or skip elements by advancing `pos`.
- [High] Eliminating the distinction between safe and non-safe list iterators obscures code intent, removes a vital visual guardrail for destructive operations, and introduces severe backporting risks.
--

commit e1c09f66ccd28cca66865681febeb6f8233f0a36
Author: Kaitao Cheng <chengkaitao@kylinos.cn>

list: Cache cursors in entry iterators

This commit introduces private next and previous cursor variables into the
standard list iteration macros. It changes the semantics of the
list_for_each_entry() family to cache the next pointer before executing the
loop body. It also updates comments to recommend the non-safe variants over
the safe ones when a temporary cursor is not needed.

> diff --git a/include/linux/list.h b/include/linux/list.h
> index 09d979976b3b8..9df84a56a789b 100644
> --- a/include/linux/list.h
> +++ b/include/linux/list.h
> @@ -809,6 +809,29 @@ static inline size_t list_count_nodes(struct list_head *head)
>  #define list_entry_is_head(pos, head, member)				\
>  	list_is_head(&pos->member, (head))
>  
> +#define __list_for_each_entry(pos, next, head, member)			\
> +	for (typeof(pos) next = list_next_entry(pos =			\
> +		list_first_entry(head, typeof(*pos), member), member);	\
> +	     !list_entry_is_head(pos, head, member);			\
> +	     pos = next, next = list_next_entry(next, member))

[ ... ]

> @@ -816,9 +839,7 @@ static inline size_t list_count_nodes(struct list_head *head)
>   * @member:	the name of the list_head within the struct.
>   */
>  #define list_for_each_entry(pos, head, member)				\
> -	for (pos = list_first_entry(head, typeof(*pos), member);	\
> -	     !list_entry_is_head(pos, head, member);			\
> -	     pos = list_next_entry(pos, member))
> +	__list_for_each_entry(pos, __UNIQUE_ID(next), head, member)

[Severity: Critical]
Does caching the next pointer fundamentally change the dynamic traversal
semantics of the standard list iterators kernel-wide?

A common kernel pattern is pinning the current entry (pos), dropping the
lock to perform a blocking operation, and reacquiring the lock. With
dynamic evaluation, pos->next is safely fetched under the lock.

With the new cached next pointer, if the next element was removed and freed
by a concurrent thread while the lock was dropped, next becomes a dangling
pointer, which could lead to a use-after-free when the loop steps.

Will this also silently skip dynamically inserted elements? If a loop body
inserts a new element immediately after pos, the old macro would naturally
visit it next. The new macro appears to silently skip it because it advances
using the pre-cached next.

Could this also overwrite manual cursor updates? If the loop body explicitly
alters pos to skip elements, the loop increment (pos = next) might overwrite
the updated cursor with the pre-cached next, negating the skip and potentially
causing incorrect traversal behavior.

[ ... ]

> @@ -901,6 +917,8 @@ static inline size_t list_count_nodes(struct list_head *head)
>   * @n:		another type * to use as temporary storage
>   * @head:	the head for your list.
>   * @member:	the name of the list_head within the struct.
> + *
> + * Prefer list_for_each_entry() unless the temporary cursor is needed.
>   */
>  #define list_for_each_entry_safe(pos, n, head, member)			\

[Severity: High]
Does advising developers to prefer list_for_each_entry() over the _safe
variant for removals obscure the intent of the code?

The _safe variant explicitly signals to reviewers and static analysis tools
that the loop body may mutate the list. Conflating variable-scoping
convenience with traversal safety constraints removes a visual guardrail for
destructive operations.

Additionally, does this introduce a backporting hazard? If developers write
new code that unlinks elements using list_for_each_entry(), backporting those
patches to older stable kernels (which lack this caching patch) will compile
cleanly but might reliably introduce use-after-free vulnerabilities, as older
kernels evaluate pos->next after pos has been unlinked.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609064122.95825-1-kaitao.cheng@linux.dev?part=14