[PATCH] ipvlan: no longer rely on RTNL in ipvlan_nl_fillinfo()

[PATCH] ipvlan: no longer rely on RTNL in ipvlan_nl_fillinfo()

[Kishore Gummadidala]

ipvlan_nl_fillinfo() currently relies on RTNL being held because it
calls ipvlan_port_get_rtnl(ipvlan->phy_dev) to retrieve the ipvl_port,
forcing "ip link show" dumps to hold RTNL.

However, ipvlan->port is already initialized in ipvlan_init() with a
reference on port->count and remains valid until ipvlan_uninit(), after
the net_device has been unregistered. Both ipvlan_nl_fillinfo() and
ipvlan_nl_changelink() can therefore use ipvlan->port directly without
dereferencing phy_dev->rx_handler_data or checking for a NULL port.

In addition, port->mode and port->flags are updated under RTNL (via
ipvlan_link_new(), ipvlan_nl_changelink(), and ipvlan_set_port_mode()),
while being read locklessly from the data path and from
ipvlan_nl_fillinfo():
- ipvlan_queue_xmit(), ipvlan_handle_frame(), and ipvlan_skb_to_addr()
  read port->mode,
- ipvlan_is_private() and ipvlan_is_vepa() read port->flags.

Furthermore, ipvlan_nl_changelink() previously updated port->flags via
separate read-modify-write calls for IPVLAN_F_PRIVATE and IPVLAN_F_VEPA,
which could publish an intermediate value to concurrent readers. Since
ipvlan_nl_validate() already validates that only those two flags exist
and are mutually exclusive, replace the bit-manipulation helpers with a
single WRITE_ONCE(port->flags, ...) in ipvlan_nl_changelink() (matching
ipvlan_link_new()) and remove the unused ipvlan_{mark,clear}_{private,
vepa}() helpers.

Annotate all remaining accesses to port->mode and port->flags with
READ_ONCE() and WRITE_ONCE(), caching READ_ONCE(port->mode) in a local
variable in ipvlan_queue_xmit() and ipvlan_handle_frame() so the switch
statement and fallback WARN_ONCE() observe a consistent value.

Finally, add const qualifiers to local pointers in ipvlan_nl_fillinfo().

Signed-off-by: Kishore Gummadidala <kishorg@google.com>
---
 drivers/net/ipvlan/ipvlan.h      | 24 ++------------------
 drivers/net/ipvlan/ipvlan_core.c | 12 ++++++----
 drivers/net/ipvlan/ipvlan_l3s.c  |  2 +-
 drivers/net/ipvlan/ipvlan_main.c | 38 ++++++++------------------------
 4 files changed, 20 insertions(+), 56 deletions(-)

diff --git a/drivers/net/ipvlan/ipvlan.h b/drivers/net/ipvlan/ipvlan.h
index 8d05ad480438..48176b546a46 100644
--- a/drivers/net/ipvlan/ipvlan.h
+++ b/drivers/net/ipvlan/ipvlan.h
@@ -125,32 +125,12 @@ static inline struct ipvl_port *ipvlan_port_get_rtnl(const struct net_device *d)
 
 static inline bool ipvlan_is_private(const struct ipvl_port *port)
 {
-	return !!(port->flags & IPVLAN_F_PRIVATE);
-}
-
-static inline void ipvlan_mark_private(struct ipvl_port *port)
-{
-	port->flags |= IPVLAN_F_PRIVATE;
-}
-
-static inline void ipvlan_clear_private(struct ipvl_port *port)
-{
-	port->flags &= ~IPVLAN_F_PRIVATE;
+	return !!(READ_ONCE(port->flags) & IPVLAN_F_PRIVATE);
 }
 
 static inline bool ipvlan_is_vepa(const struct ipvl_port *port)
 {
-	return !!(port->flags & IPVLAN_F_VEPA);
-}
-
-static inline void ipvlan_mark_vepa(struct ipvl_port *port)
-{
-	port->flags |= IPVLAN_F_VEPA;
-}
-
-static inline void ipvlan_clear_vepa(struct ipvl_port *port)
-{
-	port->flags &= ~IPVLAN_F_VEPA;
+	return !!(READ_ONCE(port->flags) & IPVLAN_F_VEPA);
 }
 
 void ipvlan_init_secret(void);
diff --git a/drivers/net/ipvlan/ipvlan_core.c b/drivers/net/ipvlan/ipvlan_core.c
index 7ad12dc7845c..c08d2bf88503 100644
--- a/drivers/net/ipvlan/ipvlan_core.c
+++ b/drivers/net/ipvlan/ipvlan_core.c
@@ -676,6 +676,7 @@ int ipvlan_queue_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct ipvl_dev *ipvlan = netdev_priv(dev);
 	struct ipvl_port *port = ipvlan_port_get_rcu_bh(ipvlan->phy_dev);
+	u16 mode;
 
 	if (!port)
 		goto out;
@@ -683,7 +684,8 @@ int ipvlan_queue_xmit(struct sk_buff *skb, struct net_device *dev)
 	if (unlikely(!pskb_may_pull(skb, sizeof(struct ethhdr))))
 		goto out;
 
-	switch(port->mode) {
+	mode = READ_ONCE(port->mode);
+	switch (mode) {
 	case IPVLAN_MODE_L2:
 		return ipvlan_xmit_mode_l2(skb, dev);
 	case IPVLAN_MODE_L3:
@@ -694,7 +696,7 @@ int ipvlan_queue_xmit(struct sk_buff *skb, struct net_device *dev)
 	}
 
 	/* Should not reach here */
-	WARN_ONCE(true, "%s called for mode = [%x]\n", __func__, port->mode);
+	WARN_ONCE(true, "%s called for mode = [%x]\n", __func__, mode);
 out:
 	kfree_skb(skb);
 	return NET_XMIT_DROP;
@@ -782,11 +784,13 @@ rx_handler_result_t ipvlan_handle_frame(struct sk_buff **pskb)
 {
 	struct sk_buff *skb = *pskb;
 	struct ipvl_port *port = ipvlan_port_get_rcu(skb->dev);
+	u16 mode;
 
 	if (!port)
 		return RX_HANDLER_PASS;
 
-	switch (port->mode) {
+	mode = READ_ONCE(port->mode);
+	switch (mode) {
 	case IPVLAN_MODE_L2:
 		return ipvlan_handle_mode_l2(pskb, port);
 	case IPVLAN_MODE_L3:
@@ -798,7 +802,7 @@ rx_handler_result_t ipvlan_handle_frame(struct sk_buff **pskb)
 	}
 
 	/* Should not reach here */
-	WARN_ONCE(true, "%s called for mode = [%x]\n", __func__, port->mode);
+	WARN_ONCE(true, "%s called for mode = [%x]\n", __func__, mode);
 	kfree_skb(skb);
 	return RX_HANDLER_CONSUMED;
 }
diff --git a/drivers/net/ipvlan/ipvlan_l3s.c b/drivers/net/ipvlan/ipvlan_l3s.c
index 7c017fe35522..3e9f5f051d86 100644
--- a/drivers/net/ipvlan/ipvlan_l3s.c
+++ b/drivers/net/ipvlan/ipvlan_l3s.c
@@ -24,7 +24,7 @@ static struct ipvl_addr *ipvlan_skb_to_addr(struct sk_buff *skb,
 		goto out;
 
 	port = ipvlan_port_get_rcu(dev);
-	if (!port || port->mode != IPVLAN_MODE_L3S)
+	if (!port || READ_ONCE(port->mode) != IPVLAN_MODE_L3S)
 		goto out;
 
 	lyr3h = ipvlan_get_L3_hdr(port, skb, &addr_type);
diff --git a/drivers/net/ipvlan/ipvlan_main.c b/drivers/net/ipvlan/ipvlan_main.c
index 4939cf67b336..9c7c1a1f41c3 100644
--- a/drivers/net/ipvlan/ipvlan_main.c
+++ b/drivers/net/ipvlan/ipvlan_main.c
@@ -47,7 +47,7 @@ static int ipvlan_set_port_mode(struct ipvl_port *port, u16 nval,
 			/* Old mode was L3S */
 			ipvlan_l3s_unregister(port);
 		}
-		port->mode = nval;
+		WRITE_ONCE(port->mode, nval);
 
 		mutex_unlock(&port->pnodes_lock);
 	}
@@ -501,7 +501,7 @@ static int ipvlan_nl_changelink(struct net_device *dev,
 				struct netlink_ext_ack *extack)
 {
 	struct ipvl_dev *ipvlan = netdev_priv(dev);
-	struct ipvl_port *port = ipvlan_port_get_rtnl(ipvlan->phy_dev);
+	struct ipvl_port *port = ipvlan->port;
 	int err = 0;
 
 	if (!data)
@@ -516,17 +516,7 @@ static int ipvlan_nl_changelink(struct net_device *dev,
 	}
 
 	if (!err && data[IFLA_IPVLAN_FLAGS]) {
-		u16 flags = nla_get_u16(data[IFLA_IPVLAN_FLAGS]);
-
-		if (flags & IPVLAN_F_PRIVATE)
-			ipvlan_mark_private(port);
-		else
-			ipvlan_clear_private(port);
-
-		if (flags & IPVLAN_F_VEPA)
-			ipvlan_mark_vepa(port);
-		else
-			ipvlan_clear_vepa(port);
+		WRITE_ONCE(port->flags, nla_get_u16(data[IFLA_IPVLAN_FLAGS]));
 	}
 
 	return err;
@@ -570,23 +560,13 @@ static int ipvlan_nl_validate(struct nlattr *tb[], struct nlattr *data[],
 static int ipvlan_nl_fillinfo(struct sk_buff *skb,
 			      const struct net_device *dev)
 {
-	struct ipvl_dev *ipvlan = netdev_priv(dev);
-	struct ipvl_port *port = ipvlan_port_get_rtnl(ipvlan->phy_dev);
-	int ret = -EINVAL;
-
-	if (!port)
-		goto err;
-
-	ret = -EMSGSIZE;
-	if (nla_put_u16(skb, IFLA_IPVLAN_MODE, port->mode))
-		goto err;
-	if (nla_put_u16(skb, IFLA_IPVLAN_FLAGS, port->flags))
-		goto err;
+	const struct ipvl_dev *ipvlan = netdev_priv(dev);
+	const struct ipvl_port *port = ipvlan->port;
 
+	if (nla_put_u16(skb, IFLA_IPVLAN_MODE, READ_ONCE(port->mode)) ||
+	    nla_put_u16(skb, IFLA_IPVLAN_FLAGS, READ_ONCE(port->flags)))
+		return -EMSGSIZE;
 	return 0;
-
-err:
-	return ret;
 }
 
 int ipvlan_link_new(struct net_device *dev, struct rtnl_newlink_params *params,
@@ -682,7 +662,7 @@ int ipvlan_link_new(struct net_device *dev, struct rtnl_newlink_params *params,
 	 * to be consistent in setting it just like the mode attribute.
 	 */
 	if (data && data[IFLA_IPVLAN_FLAGS])
-		port->flags = nla_get_u16(data[IFLA_IPVLAN_FLAGS]);
+		WRITE_ONCE(port->flags, nla_get_u16(data[IFLA_IPVLAN_FLAGS]));
 
 	if (data && data[IFLA_IPVLAN_MODE])
 		mode = nla_get_u16(data[IFLA_IPVLAN_MODE]);
-- 
2.56.0.rc1.315.gc6ed9934b7-goog

Re: [PATCH] ipvlan: no longer rely on RTNL in ipvlan_nl_fillinfo()

[Eric Dumazet]

On Mon, Sep 28, 2026 at 11:05 PM Kishore Gummadidala <kishorg@google.com> wrote:
>
> ipvlan_nl_fillinfo() currently relies on RTNL being held because it
> calls ipvlan_port_get_rtnl(ipvlan->phy_dev) to retrieve the ipvl_port,
> forcing "ip link show" dumps to hold RTNL.
>
> However, ipvlan->port is already initialized in ipvlan_init() with a
> reference on port->count and remains valid until ipvlan_uninit(), after
> the net_device has been unregistered. Both ipvlan_nl_fillinfo() and
> ipvlan_nl_changelink() can therefore use ipvlan->port directly without
> dereferencing phy_dev->rx_handler_data or checking for a NULL port.
>
> In addition, port->mode and port->flags are updated under RTNL (via
> ipvlan_link_new(), ipvlan_nl_changelink(), and ipvlan_set_port_mode()),
> while being read locklessly from the data path and from
> ipvlan_nl_fillinfo():
> - ipvlan_queue_xmit(), ipvlan_handle_frame(), and ipvlan_skb_to_addr()
>   read port->mode,
> - ipvlan_is_private() and ipvlan_is_vepa() read port->flags.
>
> Furthermore, ipvlan_nl_changelink() previously updated port->flags via
> separate read-modify-write calls for IPVLAN_F_PRIVATE and IPVLAN_F_VEPA,
> which could publish an intermediate value to concurrent readers. Since
> ipvlan_nl_validate() already validates that only those two flags exist
> and are mutually exclusive, replace the bit-manipulation helpers with a
> single WRITE_ONCE(port->flags, ...) in ipvlan_nl_changelink() (matching
> ipvlan_link_new()) and remove the unused ipvlan_{mark,clear}_{private,
> vepa}() helpers.
>
> Annotate all remaining accesses to port->mode and port->flags with
> READ_ONCE() and WRITE_ONCE(), caching READ_ONCE(port->mode) in a local
> variable in ipvlan_queue_xmit() and ipvlan_handle_frame() so the switch
> statement and fallback WARN_ONCE() observe a consistent value.
>
> Finally, add const qualifiers to local pointers in ipvlan_nl_fillinfo().
>
> Signed-off-by: Kishore Gummadidala <kishorg@google.com>
> ---

Reviewed-by: Eric Dumazet <edumazet@google.com>

Please include the target tree in the subject ([PATCH net-next]) as
instructed in Documentation/process/maintainer-netdev.rst

As a follow-up, ipvlan_xmit_mode_l2() and ipvlan_xmit_mode_l3() could
read port->flags once, instead of calling ipvlan_is_vepa() then
ipvlan_is_private(), so that a concurrent changelink cannot be observed
half-way (vepa from the old value, private from the new one).

Thanks!

Re: [PATCH] ipvlan: no longer rely on RTNL in ipvlan_nl_fillinfo()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 (1) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The commit message says the single WRITE_ONCE(port->flags, ...)
  in ipvlan_nl_changelink() stops concurrent readers from seeing an…

Pre-existing issues:
- [Medium] ipvlan_link_new() in drivers/net/ipvlan/ipvlan_main.c writes
  the requested IFLA_IPVLAN_FLAGS into the shared port with…

--

ipvlan: no longer rely on RTNL in ipvlan_nl_fillinfo()

This commit makes ipvlan_nl_fillinfo() and ipvlan_nl_changelink() use
ipvlan->port directly, instead of looking the port up under RTNL. It
annotates the lockless accesses to port->mode and port->flags with
READ_ONCE()/WRITE_ONCE(). It also replaces the separate mark/clear helpers
in ipvlan_nl_changelink() with a single store to port->flags.

> diff --git a/drivers/net/ipvlan/ipvlan_main.c b/drivers/net/ipvlan/ipvlan_main.c
> index f29864db662aa..e8bd2f980d548 100644
> --- a/drivers/net/ipvlan/ipvlan_main.c
> +++ b/drivers/net/ipvlan/ipvlan_main.c

[ ... ]

> @@ -516,17 +516,7 @@ static int ipvlan_nl_changelink(struct net_device *dev,
>  	}
>  
>  	if (!err && data[IFLA_IPVLAN_FLAGS]) {
> -		u16 flags = nla_get_u16(data[IFLA_IPVLAN_FLAGS]);
> -
> -		if (flags & IPVLAN_F_PRIVATE)
> -			ipvlan_mark_private(port);
> -		else
> -			ipvlan_clear_private(port);
> -
> -		if (flags & IPVLAN_F_VEPA)
> -			ipvlan_mark_vepa(port);
> -		else
> -			ipvlan_clear_vepa(port);
> +		WRITE_ONCE(port->flags, nla_get_u16(data[IFLA_IPVLAN_FLAGS]));
>  	}
>  
>  	return err;

[Severity: High]

The commit message says the old read-modify-write sequence "could publish
an intermediate value to concurrent readers". Does the single store here
fully close that window?

The TX readers still load port->flags twice per packet.
ipvlan_is_vepa() and ipvlan_is_private() each do their own
READ_ONCE(port->flags), and ipvlan_addr_lookup() runs between them:

drivers/net/ipvlan/ipvlan_core.c:ipvlan_xmit_mode_l3() {
    ...
	if (!ipvlan_is_vepa(ipvlan->port)) {
		addr = ipvlan_addr_lookup(ipvlan->port, lyr3h, addr_type, true);
		if (addr) {
			if (ipvlan_is_private(ipvlan->port)) {
				consume_skb(skb);
				return NET_XMIT_DROP;
			}
			ipvlan_rcv_frame(addr, &skb, true);
    ...
}

ipvlan_xmit_mode_l2() uses the same pattern for local unicast.

Suppose the port changes from PRIVATE to VEPA while a packet is in flight:

TX CPU                                 RTNL
ipvlan_is_vepa() reads PRIVATE
  -> false, take the local branch
ipvlan_addr_lookup() finds addr
                                       ipvlan_nl_changelink()
                                         WRITE_ONCE(port->flags, VEPA)
ipvlan_is_private() reads VEPA
  -> false
ipvlan_rcv_frame()

Wouldn't the frame then go straight to another slave? That is bridge
behaviour (flags == 0), the same intermediate state the old
clear_private/mark_vepa sequence exposed. The old PRIVATE setting would
drop the frame, and the new VEPA setting would send it out via phy_dev.

Could the TX paths take one snapshot per packet, for example u16 flags =
READ_ONCE(port->flags), and test both bits on that value? Another option
is to have the helpers take a flags value instead of the port.

[ ... ]

> @@ -682,7 +662,7 @@ int ipvlan_link_new(struct net_device *dev, struct rtnl_newlink_params *params,
>  	 * to be consistent in setting it just like the mode attribute.
>  	 */
>  	if (data && data[IFLA_IPVLAN_FLAGS])
> -		port->flags = nla_get_u16(data[IFLA_IPVLAN_FLAGS]);
> +		WRITE_ONCE(port->flags, nla_get_u16(data[IFLA_IPVLAN_FLAGS]));
>  
>  	if (data && data[IFLA_IPVLAN_MODE])
>  		mode = nla_get_u16(data[IFLA_IPVLAN_MODE]);

[Severity: Medium]

This isn't a bug introduced by this patch, but ipvlan_link_new() writes
port->flags to the shared port before it calls ipvlan_set_port_mode(),
which can fail. If that call fails, is the old flags value ever restored?

drivers/net/ipvlan/ipvlan_main.c:ipvlan_link_new() {
    ...
	err = ipvlan_set_port_mode(port, mode, extack);
	if (err)
		goto unlink_netdev;
    ...
unlink_netdev:
	netdev_upper_dev_unlink(phy_dev, dev);
remove_ida:
	ida_free(&port->ida, dev->dev_id);
unregister_netdev:
	unregister_netdevice(dev);
	return err;
}

ipvlan_set_port_mode() fails if dev_change_flags() fails on an existing
slave, or if ipvlan_l3s_register() fails. With CONFIG_IPVLAN_L3S=n it
always fails for L3S mode. The stub returns -ENOTSUPP, and
ipvlan_nl_validate() only checks mode < IPVLAN_MODE_MAX:

static inline int ipvlan_l3s_register(struct ipvl_port *port)
{
	return -ENOTSUPP;
}

On a port that already has slaves, RTM_NEWLINK would then return an
error, but the PRIVATE/VEPA setting has already changed for every
existing slave.

ipvlan_nl_changelink() avoids this because it writes flags only after
ipvlan_set_port_mode() returns 0. Could ipvlan_link_new() use the same
order?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928210508.2645800-1-kishorg%40google.com