List iterator used after loop in mlxsw_sp_fid_port_vid_list_add?

List iterator used after loop in mlxsw_sp_fid_port_vid_list_add?

[Maoyi Xie]

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].

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.

Thanks,
Maoyi
https://maoyixie.com/

[1] Jakob Koschel et al., "UNCONTAINED: Uncovering Container
    Confusion in the Linux Kernel", USENIX Security 2023.
    https://www.usenix.org/conference/usenixsecurity23/presentation/koschel

Re: List iterator used after loop in mlxsw_sp_fid_port_vid_list_add?

[Ido Schimmel]

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!