Re: [PATCH net-next v10 5/7] bonding: Update to bond_arp_send_all() to use supplied vlan tags

[Jay Vosburgh <jv@jvosburgh.net>]

Jay Vosburgh <jv@jvosburgh.net> wrote:

>Nikolay Aleksandrov <razor@blackwall.org> wrote:
>
>>On 9/5/25 01:18, David Wilder wrote:
>>> bond_arp_send_all() will pass the vlan tags supplied by
>>> the user to bond_arp_send(). If vlan tags have not been
>>> supplied the vlans in the path to the target will be
>>> discovered by bond_verify_device_path(). The discovered
>>> vlan tags are then saved to be used on future calls to
>>> bond_arp_send().
>>> bond_uninit() is also updated to free vlan tags when a
>>> bond is destroyed.
>>> Signed-off-by: David Wilder <wilder@us.ibm.com>
>>> ---
>>>   drivers/net/bonding/bond_main.c | 22 +++++++++++++---------
>>>   1 file changed, 13 insertions(+), 9 deletions(-)
>>> diff --git a/drivers/net/bonding/bond_main.c
>>> b/drivers/net/bonding/bond_main.c
>>> index 7548119ca0f3..7288f8a5f1a5 100644
>>> --- a/drivers/net/bonding/bond_main.c
>>> +++ b/drivers/net/bonding/bond_main.c
>>> @@ -3063,18 +3063,19 @@ struct bond_vlan_tag *bond_verify_device_path(struct net_device *start_dev,
>>>     static void bond_arp_send_all(struct bonding *bond, struct slave
>>> *slave)
>>>   {
>>> -	struct rtable *rt;
>>> -	struct bond_vlan_tag *tags;
>>>   	struct bond_arp_target *targets = bond->params.arp_targets;
>>> +	char pbuf[BOND_OPTION_STRING_MAX_SIZE];
>>> +	struct bond_vlan_tag *tags;
>>>   	__be32 target_ip, addr;
>>> +	struct rtable *rt;
>>>   	int i;
>>>     	for (i = 0; i < BOND_MAX_ARP_TARGETS && targets[i].target_ip; i++)
>>> {
>>>   		target_ip = targets[i].target_ip;
>>>   		tags = targets[i].tags;
>>>   -		slave_dbg(bond->dev, slave->dev, "%s: target %pI4\n",
>>> -			  __func__, &target_ip);
>>> +		slave_dbg(bond->dev, slave->dev, "%s: target %s\n", __func__,
>>> +			  bond_arp_target_to_string(&targets[i], pbuf, sizeof(pbuf)));
>>>     		/* Find out through which dev should the packet go */
>>>   		rt = ip_route_output(dev_net(bond->dev), target_ip, 0, 0, 0,
>>> @@ -3096,9 +3097,13 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
>>>   		if (rt->dst.dev == bond->dev)
>>>   			goto found;
>>>   -		rcu_read_lock();
>>> -		tags = bond_verify_device_path(bond->dev, rt->dst.dev, 0);
>>> -		rcu_read_unlock();
>>> +		if (!tags) {
>>> +			rcu_read_lock();
>>> +			tags = bond_verify_device_path(bond->dev, rt->dst.dev, 0);
>>> +			/* cache the tags */
>>> +			targets[i].tags = tags;
>>> +			rcu_read_unlock();
>>
>>Surely you must be joking. You cannot overwrite the tags pointer without any synchronization.
>
>	Agreed, I think this will race with at least bond_fill_info,
>_bond_options_arp_ip_target_set, and bond_option_arp_ip_target_rem.
>
>	Also, pretending for the moment that the above isn't an issue,
>does this cache handle changes in real time?  I.e., if the VLAN above
>the bond is replumbed without dismantling the bond, will the above
>notice and do the right thing?
>
>	The current code checks the device path on every call, and I
>don't see how it's feasible to skip that.

	Ok, thinking this through a little more... the point of the
patch set is to permit the user to supply the tags via option setting
for cases that bond_verify_device_path can't figure things out.  So the
tags stashed as part of the bond (i.e., provided as option settings from
user space) should only be changable from user space.

	So, I think the way it'll have to work is, if user space
provided tags then use them, otherwise call bond_verify_device_path and
use whatever it says, but throw that away after each pass.

	If user space provided tags and then replumbs things, then it'll
be on user space to update the tags, as the option is essentially
overriding the automatic lookup provided by bond_verify_device_path.

	If the tags stashed in the bond configuration can only be
changed via user space option settings, I think that can be done safely
in an RCU manner (as netlink always operates with RTNL held, if memory
serves).

	-J

>	Separately, a random thought while looking at the code, I feel
>like there ought to be a way to replace the GFP_ATOMIC memory allocation
>in bond_verify_device_path with storage local to its caller (since it's
>always immediately freed), but that's probably not something to get into
>for this patch set.
>
>	-J
>
>>> +		}
>>>     		if (!IS_ERR_OR_NULL(tags))
>>>   			goto found;
>>> @@ -3114,7 +3119,6 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
>>>   		addr = bond_confirm_addr(rt->dst.dev, target_ip, 0);
>>>   		ip_rt_put(rt);
>>>   		bond_arp_send(slave, ARPOP_REQUEST, target_ip, addr, tags);
>>> -		kfree(tags);
>>>   	}
>>>   }
>>>   @@ -6047,6 +6051,7 @@ static void bond_uninit(struct net_device
>>> *bond_dev)
>>>   	bond_for_each_slave(bond, slave, iter)
>>>   		__bond_release_one(bond_dev, slave->dev, true, true);
>>>   	netdev_info(bond_dev, "Released all slaves\n");
>>> +	bond_free_vlan_tags(bond->params.arp_targets);
>>>     #ifdef CONFIG_XFRM_OFFLOAD
>>>   	mutex_destroy(&bond->ipsec_lock);
>>> @@ -6633,7 +6638,6 @@ static void __exit bonding_exit(void)
>>>     	bond_netlink_fini();
>>>   	unregister_pernet_subsys(&bond_net_ops);
>>> -
>>>   	bond_destroy_debugfs();
>>>     #ifdef CONFIG_NET_POLL_CONTROLLER
>>
>
>---
>	-Jay Vosburgh, jv@jvosburgh.net
>

---
	-Jay Vosburgh, jv@jvosburgh.net