net: l3mdev: Ignore non-L3 uppers in l3mdev_fib_table_rcu

PATCH Superseded Ao Zhou 4 participants 11 messages Apr 4 – Apr 19 1 message not in the archive

Subsystems (3)

Networking Network drivers L3MDEV

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Ao Zhou

author

2026-04-04 11:52 UTC #1 lore raw

PATCH 1/1 net: l3mdev: Ignore non-L3 uppers in l3mdev_fib_table_rcu

From: Haoze Xie <[email protected]>

l3mdev_fib_table_rcu() assumes that any upper device observed for an IFF_L3MDEV_SLAVE device is an L3 master and dereferences master->l3mdev_ops unconditionally.

VRF slave setup sets IFF_L3MDEV_SLAVE before the upper link is fully switched, so readers can transiently observe a non-L3 upper such as a bridge and follow a NULL l3mdev_ops pointer. Require the current upper to still be an L3 master before consulting its FIB table.

Fixes fee6d4c777a1 ("net: Add netif_is_l3_slave")

Reported-by Yifan Wu <[email protected]>

Reported-by Juefei Pu <[email protected]>

Co-developed-by Yuan Tan <[email protected]>

Signed-off-by Yuan Tan <[email protected]>

Suggested-by Xin Liu <[email protected]>

Signed-off-by Haoze Xie <[email protected]>

Signed-off-by Ao Zhou <[email protected]>

---
 net/l3mdev/l3mdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

net/l3mdev/l3mdev.c +1 -1

index 5432a5f2dfc8..b8a3030cb2c4 100644
@@ -177,7 +177,7 @@ u32 l3mdev_fib_table_rcu(const struct net_device *dev)
 		const struct net_device *master;
 
 		master = netdev_master_upper_dev_get_rcu(_dev);
-		if (master &&
+		if (master && netif_is_l3_master(master) &&
 		    master->l3mdev_ops->l3mdev_fib_table)
 			tb_id = master->l3mdev_ops->l3mdev_fib_table(master);
 	}

--
2.53.0

David Ahern

maintainer Reviewed-by on v1 1/1

2026-04-05 16:22 UTC #2 lore raw

Quoted 6 lines · Ao Zhou

From: Haoze Xie <[email protected]>
l3mdev_fib_table_rcu() assumes that any upper device observed for an IFF_L3MDEV_SLAVE device is an L3 master and dereferences master->l3mdev_ops unconditionally.

Quoted 6 lines

VRF slave setup sets IFF_L3MDEV_SLAVE before the upper link is fully switched, so readers can transiently observe a non-L3 upper such as a bridge and follow a NULL l3mdev_ops pointer. Require the current upper to still be an L3 master before consulting its FIB table.
Fixes fee6d4c777a1 ("net: Add netif_is_l3_slave")

Fixes should be:

commit fdeea7be88b12742bfd50d9e19a06c0d2e702400

Author: Ido Schimmel <[email protected]>
Date:   Thu Mar 16 09:08:15 2017 +0100

net: vrf: Set slave's private flag before linking

Quoted 16 lines

Reported-by Yifan Wu <[email protected]>
Reported-by Juefei Pu <[email protected]>
Co-developed-by Yuan Tan <[email protected]>
Signed-off-by Yuan Tan <[email protected]>
Suggested-by Xin Liu <[email protected]>
Signed-off-by Haoze Xie <[email protected]>
Signed-off-by Ao Zhou <[email protected]>
---
 net/l3mdev/l3mdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
net/l3mdev/l3mdev.c +0 -0
index 5432a5f2dfc8..b8a3030cb2c4 100644

net/l3mdev/l3mdev.c u32 l3mdev_fib_table_rcu(const struct net_device *dev) · line 180

Quoted 5 lines

net/l3mdev/l3mdev.c +1 -1

@@ -177,7 +177,7 @@ u32 l3mdev_fib_table_rcu(const struct net_device *dev)
 		const struct net_device *master;
 
 		master = netdev_master_upper_dev_get_rcu(_dev);
-		if (master &&
+		if (master && netif_is_l3_master(master) &&

Reviewed-by David Ahern <[email protected]>

Ido Schimmel

2026-04-06 10:34 UTC #3 lore raw

In reply to Ao Zhou

Quoted 10 lines · Ao Zhou

From: Haoze Xie <[email protected]>
l3mdev_fib_table_rcu() assumes that any upper device observed for an IFF_L3MDEV_SLAVE device is an L3 master and dereferences master->l3mdev_ops unconditionally.
VRF slave setup sets IFF_L3MDEV_SLAVE before the upper link is fully switched, so readers can transiently observe a non-L3 upper such as a bridge and follow a NULL l3mdev_ops pointer. Require the current upper to still be an L3 master before consulting its FIB table.

Do you have a reproducer? I don't see how that can happen.

do_set_master() ensures that the device doesn't have a master when ndo_add_slave() is called. Meaning, if netif_is_l3_slave() is true and netdev_master_upper_dev_get_rcu() resolved a master device, it's guaranteed to be a VRF.

Quoted 29 lines

Fixes fee6d4c777a1 ("net: Add netif_is_l3_slave")
Reported-by Yifan Wu <[email protected]>
Reported-by Juefei Pu <[email protected]>
Co-developed-by Yuan Tan <[email protected]>
Signed-off-by Yuan Tan <[email protected]>
Suggested-by Xin Liu <[email protected]>
Signed-off-by Haoze Xie <[email protected]>
Signed-off-by Ao Zhou <[email protected]>
---
 net/l3mdev/l3mdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
net/l3mdev/l3mdev.c +1 -2
index 5432a5f2dfc8..b8a3030cb2c4 100644
@@ -177,7 +177,7 @@ u32 l3mdev_fib_table_rcu(const struct net_device *dev)
 		const struct net_device *master;
 
 		master = netdev_master_upper_dev_get_rcu(_dev);
-		if (master &&
+		if (master && netif_is_l3_master(master) &&
 		    master->l3mdev_ops->l3mdev_fib_table)
 			tb_id = master->l3mdev_ops->l3mdev_fib_table(master);
 	}
--
2.53.0

Haoze Xie

2026-04-19 03:46 UTC #4 lore raw

Quoted 12 lines · Ido Schimmel

Quoted 10 lines · Ao Zhou
From: Haoze Xie <[email protected]>
l3mdev_fib_table_rcu() assumes that any upper device observed for an IFF_L3MDEV_SLAVE device is an L3 master and dereferences master->l3mdev_ops unconditionally.
VRF slave setup sets IFF_L3MDEV_SLAVE before the upper link is fully switched, so readers can transiently observe a non-L3 upper such as a bridge and follow a NULL l3mdev_ops pointer. Require the current upper to still be an L3 master before consulting its FIB table.

Quoted 7 lines

Do you have a reproducer? I don't see how that can happen.
do_set_master() ensures that the device doesn't have a master when ndo_add_slave() is called. Meaning, if netif_is_l3_slave() is true and netdev_master_upper_dev_get_rcu() resolved a master device, it's guaranteed to be a VRF.

I agree that this is correct in the synchronous do_set_master() control flow.

My concern is narrower: an RCU reader can still resolve a stale upper after the unlink, so netif_is_l3_slave(dev) alone is not enough to prove that the resolved upper is still an L3 master at the point where master->l3mdev_ops is dereferenced.

Quoted 29 lines

Quoted 29 lines
Fixes fee6d4c777a1 ("net: Add netif_is_l3_slave")
Reported-by Yifan Wu <[email protected]>
Reported-by Juefei Pu <[email protected]>
Co-developed-by Yuan Tan <[email protected]>
Signed-off-by Yuan Tan <[email protected]>
Suggested-by Xin Liu <[email protected]>
Signed-off-by Haoze Xie <[email protected]>
Signed-off-by Ao Zhou <[email protected]>
---
 net/l3mdev/l3mdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
net/l3mdev/l3mdev.c +1 -2
index 5432a5f2dfc8..b8a3030cb2c4 100644
@@ -177,7 +177,7 @@ u32 l3mdev_fib_table_rcu(const struct net_device *dev)
 		const struct net_device *master;
 
 		master = netdev_master_upper_dev_get_rcu(_dev);
-		if (master &&
+		if (master && netif_is_l3_master(master) &&
 		    master->l3mdev_ops->l3mdev_fib_table)
 			tb_id = master->l3mdev_ops->l3mdev_fib_table(master);
 	}
--
2.53.0

Ao Zhou

author

2026-04-06 13:28 UTC #5 lore raw

PATCH v2 1/1 net: l3mdev: Ignore non-L3 uppers in l3mdev_fib_table_rcu

In reply to a message not in the archive

From: Haoze Xie <[email protected]>

l3mdev_fib_table_rcu() assumes that any upper device observed for an IFF_L3MDEV_SLAVE device is an L3 master and dereferences master->l3mdev_ops unconditionally.

Fixes fdeea7be88b1 ("net: vrf: Set slave's private flag before linking")

Reported-by Yifan Wu <[email protected]>

Reported-by Juefei Pu <[email protected]>

Co-developed-by Yuan Tan <[email protected]>

Signed-off-by Yuan Tan <[email protected]>

Suggested-by Xin Liu <[email protected]>

Reviewed-by David Ahern <[email protected]>

Signed-off-by Haoze Xie <[email protected]>

Signed-off-by Ao Zhou <[email protected]>

---
changes in v2:
- point Fixes to the VRF slave ordering change identified by David Ahern
- add David Ahern's Reviewed-by trailer

 net/l3mdev/l3mdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

net/l3mdev/l3mdev.c +1 -1

index 5432a5f2dfc8..b8a3030cb2c4 100644
@@ -177,7 +177,7 @@ u32 l3mdev_fib_table_rcu(const struct net_device *dev)
 		const struct net_device *master;
 
 		master = netdev_master_upper_dev_get_rcu(_dev);
-		if (master &&
+		if (master && netif_is_l3_master(master) &&
 		    master->l3mdev_ops->l3mdev_fib_table)
 			tb_id = master->l3mdev_ops->l3mdev_fib_table(master);
 	}

Signature · 1 line

2.53.0

Ido Schimmel

2026-04-06 15:48 UTC #6 lore raw

Quoted 33 lines · Ao Zhou

From: Haoze Xie <[email protected]>
l3mdev_fib_table_rcu() assumes that any upper device observed for an IFF_L3MDEV_SLAVE device is an L3 master and dereferences master->l3mdev_ops unconditionally.
VRF slave setup sets IFF_L3MDEV_SLAVE before the upper link is fully switched, so readers can transiently observe a non-L3 upper such as a bridge and follow a NULL l3mdev_ops pointer. Require the current upper to still be an L3 master before consulting its FIB table.
Fixes fdeea7be88b1 ("net: vrf: Set slave's private flag before linking")
Reported-by Yifan Wu <[email protected]>
Reported-by Juefei Pu <[email protected]>
Co-developed-by Yuan Tan <[email protected]>
Signed-off-by Yuan Tan <[email protected]>
Suggested-by Xin Liu <[email protected]>
Reviewed-by David Ahern <[email protected]>
Signed-off-by Haoze Xie <[email protected]>
Signed-off-by Ao Zhou <[email protected]>
---
changes in v2:
- point Fixes to the VRF slave ordering change identified by David Ahern
- add David Ahern's Reviewed-by trailer
 net/l3mdev/l3mdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
net/l3mdev/l3mdev.c +0 -0
index 5432a5f2dfc8..b8a3030cb2c4 100644

net/l3mdev/l3mdev.c u32 l3mdev_fib_table_rcu(const struct net_device *dev) · line 181

Quoted 6 lines

net/l3mdev/l3mdev.c +1 -1

@@ -177,7 +177,7 @@ u32 l3mdev_fib_table_rcu(const struct net_device *dev)
 		const struct net_device *master;
 
 		master = netdev_master_upper_dev_get_rcu(_dev);
-		if (master &&
+		if (master && netif_is_l3_master(master) &&
 		    master->l3mdev_ops->l3mdev_fib_table)

Don't we have the same problem in l3mdev_l3_rcv() and l3mdev_l3_out()? If so, please check if I missed more places and include them in v3.

And I think that the part that I was missing earlier is that we don't have RCU synchronization in the unslaving path, so an RCU reader can either see the original master, NULL or a new master (e.g., bridge instead of the original VRF master).

Quoted 5 lines

net/l3mdev/l3mdev.c +0 -0
 			tb_id = master->l3mdev_ops->l3mdev_fib_table(master);
 	}
Signature · 1 line
2.53.0

David Ahern

maintainer

2026-04-06 18:14 UTC #7 lore raw

Quoted 8 lines · Ido Schimmel

Don't we have the same problem in l3mdev_l3_rcv() and l3mdev_l3_out()? If so, please check if I missed more places and include them in v3.
And I think that the part that I was missing earlier is that we don't have RCU synchronization in the unslaving path, so an RCU reader can either see the original master, NULL or a new master (e.g., bridge instead of the original VRF master).

synchronize_rcu after the unlink (control path) seems like a better, more robust option than adding more checks to the datapath.

Ido Schimmel

2026-04-07 08:03 UTC #8 lore raw

Quoted 5 lines · David Ahern

Quoted 4 lines · Ido Schimmel
Don't we have the same problem in l3mdev_l3_rcv() and l3mdev_l3_out()? If so, please check if I missed more places and include them in v3.

Quoted 7 lines

Quoted 4 lines
And I think that the part that I was missing earlier is that we don't have RCU synchronization in the unslaving path, so an RCU reader can either see the original master, NULL or a new master (e.g., bridge instead of the original VRF master).
synchronize_rcu after the unlink (control path) seems like a better, more robust option than adding more checks to the datapath.

IMO it would be better to proceed with the original approach and look into adding RCU synchronization in net-next. The original approach is more surgical and the pattern of first checking netif_is_l3_master() already exists in other places.

Haoze Xie

2026-04-19 03:51 UTC #9 lore raw

Quoted 11 lines · Ido Schimmel

Quoted 10 lines · David Ahern
Quoted 8 lines · Ido Schimmel
Don't we have the same problem in l3mdev_l3_rcv() and l3mdev_l3_out()? If so, please check if I missed more places and include them in v3.
And I think that the part that I was missing earlier is that we don't have RCU synchronization in the unslaving path, so an RCU reader can either see the original master, NULL or a new master (e.g., bridge instead of the original VRF master).

Quoted 7 lines

Quoted 2 lines
synchronize_rcu after the unlink (control path) seems like a better, more robust option than adding more checks to the datapath.
IMO it would be better to proceed with the original approach and look into adding RCU synchronization in net-next. The original approach is more surgical and the pattern of first checking netif_is_l3_master() already exists in other places.

I followed that direction for v3.

The patch keeps the more surgical datapath fix for net and extends the same check to the other same-pattern slave-side helpers.

I am sending v3 as a separate public thread. Further feedback is welcome.

Haoze Xie

2026-04-19 03:49 UTC #10 lore raw

In reply to Ido Schimmel

Quoted 36 lines · Ido Schimmel

Quoted 35 lines · Ao Zhou
From: Haoze Xie <[email protected]>
l3mdev_fib_table_rcu() assumes that any upper device observed for an IFF_L3MDEV_SLAVE device is an L3 master and dereferences master->l3mdev_ops unconditionally.
VRF slave setup sets IFF_L3MDEV_SLAVE before the upper link is fully switched, so readers can transiently observe a non-L3 upper such as a bridge and follow a NULL l3mdev_ops pointer. Require the current upper to still be an L3 master before consulting its FIB table.
Fixes fdeea7be88b1 ("net: vrf: Set slave's private flag before linking")
Reported-by Yifan Wu <[email protected]>
Reported-by Juefei Pu <[email protected]>
Co-developed-by Yuan Tan <[email protected]>
Signed-off-by Yuan Tan <[email protected]>
Suggested-by Xin Liu <[email protected]>
Reviewed-by David Ahern <[email protected]>
Signed-off-by Haoze Xie <[email protected]>
Signed-off-by Ao Zhou <[email protected]>
---
changes in v2:
- point Fixes to the VRF slave ordering change identified by David Ahern
- add David Ahern's Reviewed-by trailer
 net/l3mdev/l3mdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
net/l3mdev/l3mdev.c +0 -0
index 5432a5f2dfc8..b8a3030cb2c4 100644
@@ -177,7 +177,7 @@ u32 l3mdev_fib_table_rcu(const struct net_device *dev)
 		const struct net_device *master;
 

Quoted 8 lines

net/l3mdev/l3mdev.c u32 l3mdev_fib_table_rcu(const struct net_device *dev) · line 181
Quoted 4 lines
net/l3mdev/l3mdev.c +1 -1
 		master = netdev_master_upper_dev_get_rcu(_dev);
-		if (master &&
+		if (master && netif_is_l3_master(master) &&
 		    master->l3mdev_ops->l3mdev_fib_table)
Don't we have the same problem in l3mdev_l3_rcv() and l3mdev_l3_out()? If so, please check if I missed more places and include them in v3.

I checked the same pattern in the other slave-side helpers, and v3 now extends the fix to both l3mdev_l3_rcv() and l3mdev_l3_out() in addition to l3mdev_fib_table_rcu().

All three helpers resolve the current upper with netdev_master_upper_dev_get_rcu() and then use master->l3mdev_ops. So v3 consistently requires the resolved upper to still satisfy netif_is_l3_master(master) before dereferencing l3mdev_ops.

Quoted 10 lines

And I think that the part that I was missing earlier is that we don't have RCU synchronization in the unslaving path, so an RCU reader can either see the original master, NULL or a new master (e.g., bridge instead of the original VRF master).
Quoted 5 lines
net/l3mdev/l3mdev.c +0 -0
 			tb_id = master->l3mdev_ops->l3mdev_fib_table(master);
 	}
Signature · 1 line
2.53.0

Haoze Xie

2026-04-19 14:38 UTC #11 lore raw

Quoted 47 lines · Haoze Xie

Quoted 36 lines · Ido Schimmel
Quoted 35 lines · Ao Zhou
From: Haoze Xie <[email protected]>
l3mdev_fib_table_rcu() assumes that any upper device observed for an IFF_L3MDEV_SLAVE device is an L3 master and dereferences master->l3mdev_ops unconditionally.
VRF slave setup sets IFF_L3MDEV_SLAVE before the upper link is fully switched, so readers can transiently observe a non-L3 upper such as a bridge and follow a NULL l3mdev_ops pointer. Require the current upper to still be an L3 master before consulting its FIB table.
Fixes fdeea7be88b1 ("net: vrf: Set slave's private flag before linking")
Reported-by Yifan Wu <[email protected]>
Reported-by Juefei Pu <[email protected]>
Co-developed-by Yuan Tan <[email protected]>
Signed-off-by Yuan Tan <[email protected]>
Suggested-by Xin Liu <[email protected]>
Reviewed-by David Ahern <[email protected]>
Signed-off-by Haoze Xie <[email protected]>
Signed-off-by Ao Zhou <[email protected]>
---
changes in v2:
- point Fixes to the VRF slave ordering change identified by David Ahern
- add David Ahern's Reviewed-by trailer
 net/l3mdev/l3mdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
net/l3mdev/l3mdev.c +0 -0
index 5432a5f2dfc8..b8a3030cb2c4 100644
@@ -177,7 +177,7 @@ u32 l3mdev_fib_table_rcu(const struct net_device *dev)
 		const struct net_device *master;
 
Quoted 8 lines
net/l3mdev/l3mdev.c u32 l3mdev_fib_table_rcu(const struct net_device *dev) · line 181
Quoted 4 lines
net/l3mdev/l3mdev.c +1 -1
 		master = netdev_master_upper_dev_get_rcu(_dev);
-		if (master &&
+		if (master && netif_is_l3_master(master) &&
 		    master->l3mdev_ops->l3mdev_fib_table)
Don't we have the same problem in l3mdev_l3_rcv() and l3mdev_l3_out()? If so, please check if I missed more places and include them in v3.

Quoted 9 lines

I checked the same pattern in the other slave-side helpers, and v3 now extends the fix to both l3mdev_l3_rcv() and l3mdev_l3_out() in addition to l3mdev_fib_table_rcu().
All three helpers resolve the current upper with netdev_master_upper_dev_get_rcu() and then use master->l3mdev_ops. So v3 consistently requires the resolved upper to still satisfy netif_is_l3_master(master) before dereferencing l3mdev_ops.

While updating the patch, I found that l3mdev_l3_rcv() must keep working for existing IFF_L3MDEV_RX_HANDLER users such as ipvlan_l3s. The v3 patch keeps that direct RX-handler path intact and applies the extra master check only to the slave-resolved upper case.

the smoke test, it should ping succeffully:

===> BEGIN smoke test cmd <===
ip netns add ipvl_ns
ip link add ipvl_host type veth peer name ipvl_peer ip link set ipvl_peer netns ipvl_ns

ip link set ipvl_host up
ip link add link ipvl_host name ipvl0 type ipvlan mode l3s ip addr add 198.51.100.1/24 dev ipvl0
ip link set ipvl0 up

ip netns exec ipvl_ns ip link set lo up
ip netns exec ipvl_ns ip link set ipvl_peer up
ip netns exec ipvl_ns ip addr add 198.51.100.2/24 dev ipvl_peer ip netns exec ipvl_ns ping -c 3 198.51.100.1
===> END smoke test cmd <===

Quoted 11 lines

Quoted 10 lines
And I think that the part that I was missing earlier is that we don't have RCU synchronization in the unslaving path, so an RCU reader can either see the original master, NULL or a new master (e.g., bridge instead of the original VRF master).
Quoted 5 lines
net/l3mdev/l3mdev.c +0 -0
 			tb_id = master->l3mdev_ops->l3mdev_fib_table(master);
 	}
Signature · 1 line
2.53.0