On 5/19/26 5:41 AM, Maoyi Xie wrote:
Hi all,
While reading drivers/md/dm-vdo/slab-depot.c I noticed
something that looks like a past the end iterator pattern.
I would appreciate it if you could take a look and let me
know whether this is a real issue, and whether it is worth
fixing.
The site is mark_slab_journal_dirty() (linux-7.1-rc1, around
line 156):
list_for_each_entry_reverse(dirty_journal, dirty_list,
dirty_entry) {
if (dirty_journal->recovery_lock <=
journal->recovery_lock)
break;
}
list_move_tail(&journal->dirty_entry,
dirty_journal->dirty_entry.next);
When the loop walks all entries without break (the new
journal's recovery_lock is smaller than every existing
entry), dirty_journal is past the end.
dirty_journal->dirty_entry then aliases dirty_list (the
list head) via container_of offset cancellation. So
dirty_journal->dirty_entry.next is head->next, the first
real entry. list_move_tail then inserts journal just
before the first entry, i.e. at the front of the list.
That is the intended behaviour for a sorted insertion
where the new entry has the smallest recovery_lock. The
dereference of the past the end iterator is undefined per
C11.
I don't quite follow your train of thought here. Which dereference do
you think produces undefined behavior, and why? Everything you describe
sounds valid.
Jakob Koschel cleaned up many such sites in 2022, for
example commits 99d8ae4ec8a (tracing: Remove usage of list
iterator variable after the loop), 2966a9918df (clockevents:
Use dedicated list iterator variable) and dc1acd5c946 (dlm:
replace usage of found with dedicated list iterator
variable). This site was not covered.
Jakob's series did not address this driver, because this driver was
added to the Linux kernel after 2022.
I think I understand your point that the code really shouldn't use
dirty_journal outside the loop, since it doesn't point to a valid struct
slab_journal object. I certainly wouldn't mind a patch to clean this up
a bit (perhaps by doing the list insertion inside the loop when
possible). But as a practical matter it doesn't seem to matter all that
much, since this function is only manipulating the list heads.
If you do decide to patch this, it would be nice to clean up the list
insertion, too. It's more indirect than it needs to be.
A candidate fix would track an explicit insertion target.
Declare `struct list_head *insert_after = dirty_list` before
the loop. Overwrite it to `&dirty_journal->dirty_entry` only
when the loop breaks early. The final list_move_tail then
reads `insert_after->next`. On break that points to the
entry right after the insertion position. On fall-through
it points to `head->next`, the first real entry. The
behaviour is unchanged in all cases, including an empty
list and a list with one entry.
Please don't fix it like this, this would just makes the code messier.
Matt
If this is intentional or already known, please disregard.
Otherwise, I am happy to send a [PATCH] or to leave the fix
to you. Thank you for your time, and sorry for the noise if
this is not actually worth fixing or has already been
spotted.
Thanks,
Maoyi Xie
https://maoyixie.com/
