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

[Jay Vosburgh <jv@jvosburgh.net>]

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.

	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