Re: dm-vdo: iterator after loop end in mark_slab_journal_dirty?

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/
>