Re: List iterator used after loop in mlxsw_sp_fid_port_vid_list_add?

[Ido Schimmel <idosch@nvidia.com>]

On Fri, May 22, 2026 at 11:41:25PM +0800, Maoyi Xie wrote:
> Hi all,
> 
> I came across what looks like an iterator used after the loop
> ends in drivers/net/ethernet/mellanox/mlxsw/spectrum_fid.c
> (linux-7.1-rc1), in mlxsw_sp_fid_port_vid_list_add(). I would
> appreciate your input on whether this is worth fixing or whether
> I am misreading the pattern.
> 
>   list_for_each_entry(tmp_port_vid, &fid->port_vid_list, list) {
>       if (tmp_port_vid->local_port > local_port)
>           break;
>   }
> 
>   list_add_tail(&port_vid->list, &tmp_port_vid->list);
> 
> When the loop walks the whole list without break (the new
> local_port is larger than every existing one), `tmp_port_vid`
> walks past the end of the list and `&tmp_port_vid->list` aliases
> the list head via container_of() offset cancellation, so
> list_add_tail() resolves to inserting at the tail. That is the
> intended behaviour for the case where the loop falls through.
> The dereference of the iterator after the loop ends is the part
> I am unsure about. Same shape as the Koschel cleanups from 2022
> (99d8ae4ec8a tracing, 2966a9918df clockevents, dc1acd5c946 dlm,
> and others) and the "controlled container confusion" pattern
> described in [1].

By "`&tmp_port_vid->list` aliases the list head via container_of()
offset cancellation" you mean that when we don't find an entry we get:

tmp_port_vid = list_entry(&fid->port_vid_list, typeof(*tmp_port_vid), list) = 
	container_of(&fid->port_vid_list, struct mlxsw_sp_fid_port_vid, list) =
	&fid->port_vid_list - offsetof(struct mlxsw_sp_fid_port_vid, list)

&tmp_port_vid->list =
	&fid->port_vid_list - offsetof(struct mlxsw_sp_fid_port_vid, list) +
	offsetof(struct mlxsw_sp_fid_port_vid, list) =
	&fid->port_vid_list

?

> 
> I drafted a candidate fix that initialises an explicit
> `insert_before` pointer to &fid->port_vid_list (the list head)
> and overwrites it to &tmp_port_vid->list only on early break,
> then passes insert_before to list_add_tail(). The iterator is
> no longer dereferenced after the loop and the diff is 5+/2-.
> 
> I built spectrum_fid.o on x86_64 with MLXSW_CORE + MLXSW_PCI +
> MLXSW_SPECTRUM + NET_SWITCHDEV + VLAN_8021Q at W=1 and the
> object compiles clean with no warnings. I also ran a small
> userspace mock of the two versions across seven scenarios:
> empty list, single entry with the new local_port above, below,
> and equal to the existing one, and multi-entry insertion at
> head, middle, and tail (fall through). The final list ordering
> matches in every case.
> 
> Does this look like something worth a [PATCH]? Happy to send
> one if so, or to drop it if the shape here is fine.

Yes, please send the patch to net-next. AFAICT, the code is currently
correct, but fragile. If we ever tried to dereference 'tmp_port_vid' we
would have a problem.

Thanks!