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 <[email protected]> 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/[email protected]?part=14
