Re: [PATCH net-next v5 6/7] bonding: Update for extended arp_ip_target format.

[Simon Horman <horms@kernel.org>]

On Tue, Jul 15, 2025 at 06:22:37PM +0000, David Wilder wrote:
> 
> 
> 
> ________________________________________
> From: Simon Horman <horms@kernel.org>
> Sent: Tuesday, July 15, 2025 6:58 AM
> To: David Wilder
> Cc: netdev@vger.kernel.org; jv@jvosburgh.net; pradeeps@linux.vnet.ibm.com; Pradeep Satyanarayana; i.maximets@ovn.org; Adrian Moreno Zapata; Hangbin Liu
> Subject: [EXTERNAL] Re: [PATCH net-next v5 6/7] bonding: Update for extended arp_ip_target format.
> 
> >On Mon, Jul 14, 2025 at 03:54:51PM -0700, David Wilder wrote:
> >> Updated bond_fill_info() to support extended arp_ip_target format.
> >>
> >> Forward and backward compatibility between the kernel and iprout2 is
> >> preserved.
> >>
> >> Signed-off-by: David Wilder <wilder@us.ibm.com>
> >> ---
> >>  drivers/net/bonding/bond_netlink.c | 28 ++++++++++++++++++++++++++--
> >>  include/net/bonding.h              |  1 +
> >>  2 files changed, 27 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/net/bonding/bond_netlink.c b/drivers/net/bonding/bond_netlink.c
> >> index 5486ef40907e..6e8aebe5629f 100644
> >> --- a/drivers/net/bonding/bond_netlink.c
> >> +++ b/drivers/net/bonding/bond_netlink.c
> >> @@ -701,8 +701,32 @@ static int bond_fill_info(struct sk_buff *skb,
> >>
> >>       targets_added = 0;
> >>       for (i = 0; i < BOND_MAX_ARP_TARGETS; i++) {
> >> -             if (bond->params.arp_targets[i].target_ip) {
> >> -                     if (nla_put_be32(skb, i, bond->params.arp_targets[i].target_ip))
> >> +             struct bond_arp_target *target = &bond->params.arp_targets[i];
> >> +             struct Data {
> >> +                     __u32 addr;
> >> +                     struct bond_vlan_tag vlans[BOND_MAX_VLAN_TAGS + 1];
> >> +             } data;
> >> +             int size = 0;
> >> +
> >> +             if (target->target_ip) {
> >> +                     data.addr = target->target_ip;
> >
> >Hi David,
> >
> >There appears to be an endian mismatch here. Sparse says:
> >
>   >.../bond_netlink.c:712:35: warning: incorrect type in assignment (different base types)
>   >.../bond_netlink.c:712:35:    expected unsigned int [usertype] addr
>   >.../bond_netlink.c:712:35:    got restricted __be32 [usertype] target_ip
> >
> >> +                     size = sizeof(target->target_ip);
> >> +             }
> >
> >It seems that data.addr may be used uninitialised below
> >if the if condition above is not met.
> 
> >Flagged by Smatch.
> 
> Hi Simon
> 
> Thanks for catching this,  I will make the following change in the next version.
> 
> @@ -703,15 +703,14 @@ static int bond_fill_info(struct sk_buff *skb,
>         for (i = 0; i < BOND_MAX_ARP_TARGETS; i++) {
>                 struct bond_arp_target *target = &bond->params.arp_targets[i];
>                 struct Data {
> -                       __u32 addr;
> +                       __be32 addr;
>                         struct bond_vlan_tag vlans[BOND_MAX_VLAN_TAGS + 1];
>                 } data;
>                 int size = 0;
> 
> -               if (target->target_ip) {
> -                       data.addr = target->target_ip;
> -                       size = sizeof(target->target_ip);
> -               }
> +               BUG_ON(!target->target_ip);

Hi David,

I think this addresses the issues I raised, thanks!

But please don't use BUG_ON() like this, it will crash the kernel,
which seems disproportionate to the problem at hand.

I'm not particularly familiar with this code. But it seems
that it is filling in netlink attributes. And does not make
any resource allocations. So perhaps this is sufficient?

		if (!target->target_ip)
			return -EINVAL;

> +               data.addr = target->target_ip;
> +               size = sizeof(target->target_ip);
> 
>                 for (int level = 0; target->flags & BOND_TARGET_USERTAGS && target->tags; level++) {
>                         if (level > BOND_MAX_VLAN_TAGS)
> 
> David Wilder