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

[netdev-bot+sashiko@kernel.org]

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