Message-ID:
 <MW3PR15MB3913CB95AB53A926E253DF28FA0EA@MW3PR15MB3913.namprd15.prod.outlook.com>
Date: Wed, 10 Sep 2025 21:19:30 +0000
From: David Wilder <wilder@...ibm.com>
To: Nikolay Aleksandrov <razor@...ckwall.org>,
        "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>
CC: "jv@...sburgh.net" <jv@...sburgh.net>,
        "pradeeps@...ux.vnet.ibm.com"
	<pradeeps@...ux.vnet.ibm.com>,
        Pradeep Satyanarayana <pradeep@...ibm.com>,
        "i.maximets@....org" <i.maximets@....org>,
        Adrian Moreno Zapata
	<amorenoz@...hat.com>,
        Hangbin Liu <haliu@...hat.com>,
        "stephen@...workplumber.org" <stephen@...workplumber.org>,
        "horms@...nel.org"
	<horms@...nel.org>
Subject: RE: [PATCH net-next v10 4/7] bonding: Processing extended
 arp_ip_target from user space.




________________________________________
From: Nikolay Aleksandrov <razor@...ckwall.org>
Sent: Tuesday, September 9, 2025 11:42 AM
To: David Wilder; netdev@...r.kernel.org
Cc: jv@...sburgh.net; pradeeps@...ux.vnet.ibm.com; Pradeep Satyanarayana; i.maximets@....org; Adrian Moreno Zapata; Hangbin Liu; stephen@...workplumber.org; horms@...nel.org
Subject: [EXTERNAL] Re: [PATCH net-next v10 4/7] bonding: Processing extended arp_ip_target from user space.

Thank you Nikolay for you review and comments.
Responses inline.

> On 9/5/25 01:18, David Wilder wrote:
> > Changes to bond_netlink and bond_options to process extended
> > format arp_ip_target option sent from user space via the ip
> > command.
> >
> > The extended format adds a list of vlan tags to the ip target address.
> >
> > Signed-off-by: David Wilder <wilder@...ibm.com>
> > ---
> >   drivers/net/bonding/bond_netlink.c |   5 +-
> >   drivers/net/bonding/bond_options.c | 121 +++++++++++++++++++++++------
> >   2 files changed, 99 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/net/bonding/bond_netlink.c b/drivers/net/bonding/bond_netlink.c
> > index a5887254ff23..28ee50ddf4e2 100644
> > --- a/drivers/net/bonding/bond_netlink.c
> > +++ b/drivers/net/bonding/bond_netlink.c
> > @@ -291,9 +291,10 @@ static int bond_changelink(struct net_device *bond_dev, struct nlattr *tb[],
> >                       if (nla_len(attr) < sizeof(target))
> >                               return -EINVAL;
> >
> > -                     target = nla_get_be32(attr);
> > +                     bond_opt_initextra(&newval,
> > +                                        (__force void *)nla_data(attr),
> > +                                        nla_len(attr));
> >
> > -                     bond_opt_initval(&newval, (__force u64)target);
> >                       err = __bond_opt_set(bond, BOND_OPT_ARP_TARGETS,
> >                                            &newval,
> >                                            data[IFLA_BOND_ARP_IP_TARGET],
> > diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
> > index cf4cb301a738..61334633403d 100644
> > --- a/drivers/net/bonding/bond_options.c
> > +++ b/drivers/net/bonding/bond_options.c
> > @@ -31,8 +31,8 @@ static int bond_option_use_carrier_set(struct bonding *bond,
> >                                      const struct bond_opt_value *newval);
> >   static int bond_option_arp_interval_set(struct bonding *bond,
> >                                       const struct bond_opt_value *newval);
> > -static int bond_option_arp_ip_target_add(struct bonding *bond, __be32 target);
> > -static int bond_option_arp_ip_target_rem(struct bonding *bond, __be32 target);
> > +static int bond_option_arp_ip_target_add(struct bonding *bond, struct bond_arp_target target);
> > +static int bond_option_arp_ip_target_rem(struct bonding *bond, struct bond_arp_target target);
> >   static int bond_option_arp_ip_targets_set(struct bonding *bond,
> >                                         const struct bond_opt_value *newval);
> >   static int bond_option_ns_ip6_targets_set(struct bonding *bond,
> > @@ -1133,7 +1133,7 @@ static int bond_option_arp_interval_set(struct bonding *bond,
> >   }
> >
> >   static void _bond_options_arp_ip_target_set(struct bonding *bond, int slot,
> > -                                         __be32 target,
> > +                                         struct bond_arp_target target,
> >                                           unsigned long last_rx)
> >   {
> >       struct bond_arp_target *targets = bond->params.arp_targets;
> > @@ -1143,24 +1143,25 @@ static void _bond_options_arp_ip_target_set(struct bonding *bond, int slot,
> >       if (slot >= 0 && slot < BOND_MAX_ARP_TARGETS) {
> >               bond_for_each_slave(bond, slave, iter)
> >                       slave->target_last_arp_rx[slot] = last_rx;
> > -             targets[slot].target_ip = target;
> > +             memcpy(&targets[slot], &target, sizeof(target));
> >       }
> >   }
> >
> > -static int _bond_option_arp_ip_target_add(struct bonding *bond, __be32 target)
> > +static int _bond_option_arp_ip_target_add(struct bonding *bond, struct bond_arp_target target)
> >   {
> >       struct bond_arp_target *targets = bond->params.arp_targets;
> > +     char pbuf[BOND_OPTION_STRING_MAX_SIZE];
> >       int ind;
> >
> > -     if (!bond_is_ip_target_ok(target)) {
> > +     if (!bond_is_ip_target_ok(target.target_ip)) {
> >               netdev_err(bond->dev, "invalid ARP target %pI4 specified for addition\n",
> > -                        &target);
> > +                        &target.target_ip);
> >               return -EINVAL;
> >       }
> >
> > -     if (bond_get_targets_ip(targets, target) != -1) { /* dup */
> > +     if (bond_get_targets_ip(targets, target.target_ip) != -1) { /* dup */
> >               netdev_err(bond->dev, "ARP target %pI4 is already present\n",
> > -                        &target);
> > +                        &target.target_ip);
> >               return -EINVAL;
> >       }
> >
> > @@ -1170,43 +1171,44 @@ static int _bond_option_arp_ip_target_add(struct bonding *bond, __be32 target)
> >               return -EINVAL;
> >       }
> >
> > @@ -1170,43 +1171,44 @@ static int _bond_option_arp_ip_target_add(struct bonding *bond, __be32 target)
> >               return -EINVAL;
> >       }
> >
> > -     netdev_dbg(bond->dev, "Adding ARP target %pI4\n", &target);
> > +     netdev_dbg(bond->dev, "Adding ARP target %s\n",
> > +                bond_arp_target_to_string(&target, pbuf, sizeof(pbuf)));
> >
> >       _bond_options_arp_ip_target_set(bond, ind, target, jiffies);
> >
> >       return 0;
> >   }
> >
> > -static int bond_option_arp_ip_target_add(struct bonding *bond, __be32 target)
> > +static int bond_option_arp_ip_target_add(struct bonding *bond, struct bond_arp_target target)
> >   {
> >       return _bond_option_arp_ip_target_add(bond, target);
> >   }
> >
> > -static int bond_option_arp_ip_target_rem(struct bonding *bond, __be32 target)
> > +static int bond_option_arp_ip_target_rem(struct bonding *bond, struct bond_arp_target target)
> >   {
> >       struct bond_arp_target *targets = bond->params.arp_targets;
> > +     unsigned long *targets_rx;
> >       struct list_head *iter;
> >       struct slave *slave;
> > -     unsigned long *targets_rx;
> >       int ind, i;
> >
> > -     if (!bond_is_ip_target_ok(target)) {
> > +     if (!bond_is_ip_target_ok(target.target_ip)) {
> >               netdev_err(bond->dev, "invalid ARP target %pI4 specified for removal\n",
> > -                        &target);
> > +                        &target.target_ip);
> >               return -EINVAL;
> >       }
> >
> > -     ind = bond_get_targets_ip(targets, target);
> > +     ind = bond_get_targets_ip(targets, target.target_ip);
> >       if (ind == -1) {
> >               netdev_err(bond->dev, "unable to remove nonexistent ARP target %pI4\n",
> > -                        &target);
> > +                        &target.target_ip);
> >               return -EINVAL;
> >       }
> >
> >       if (ind == 0 && !targets[1].target_ip && bond->params.arp_interval)
> >               netdev_warn(bond->dev, "Removing last arp target with arp_interval on\n");
> >
> > -     netdev_dbg(bond->dev, "Removing ARP target %pI4\n", &target);
> > +     netdev_dbg(bond->dev, "Removing ARP target %pI4\n", &target.target_ip);
> >
> >       bond_for_each_slave(bond, slave, iter) {
> >               targets_rx = slave->target_last_arp_rx;
> > @@ -1214,30 +1216,77 @@ static int bond_option_arp_ip_target_rem(struct bonding *bond, __be32 target)
> >                       targets_rx[i] = targets_rx[i+1];
> >               targets_rx[i] = 0;
> >       }
> > -     for (i = ind; (i < BOND_MAX_ARP_TARGETS - 1) && targets[i + 1].target_ip; i++)
> > -             targets[i] = targets[i+1];
> > +
> > +     bond_free_vlan_tag(&targets[ind]);
> > +
> > +     for (i = ind; (i < BOND_MAX_ARP_TARGETS - 1) && targets[i + 1].target_ip; i++) {
> > +             targets[i].target_ip = targets[i + 1].target_ip;
> > +             targets[i].tags = targets[i + 1].tags;
> > +             targets[i].flags = targets[i + 1].flags;
> > +     }
> >       targets[i].target_ip = 0;
> > +     targets[i].flags = 0;
> > +     targets[i].tags = NULL;
>
> This looks wrong, bond_free_vlan_tag() is kfree(tags) and then it is set to NULL but
> these tags can be used with only RCU held (change is in patch 05) in a few places, there is
> no synchronization and nothing to keep that code from either a use-after-free or a null
> ptr dereference.
>
> Check bond_loadbalance_arp_mon -> bond_send_validate.
>
  I understand your concern, see the discussion about patch 5.
  If the setting of user supplied tags is performed during
  the creation of the bond with the RTNL held then this should not
  be an issue.  I need to think about what happens if the user attempts to
  create or change or delete an arp_ip_taget on an existing bond. The existing code
  had no special handling of that case.

> >
> >       return 0;
> >   }
> >
> >   void bond_option_arp_ip_targets_clear(struct bonding *bond)
> >   {
> > +     struct bond_arp_target empty_target;
>
> empty_target = {} ...
>
> >       int i;
> >
> > +     empty_target.target_ip = 0;
> > +     empty_target.flags = 0;
> > +     empty_target.tags = NULL;
>
> ... and you can drop these lines

Thanks,  I will make that change.

>
> > +
> >       for (i = 0; i < BOND_MAX_ARP_TARGETS; i++)
> > -             _bond_options_arp_ip_target_set(bond, i, 0, 0);
> > +             _bond_options_arp_ip_target_set(bond, i, empty_target, 0);
> > +}
> > +
> > +/**
> > + * bond_validate_tags - validate an array of bond_vlan_tag.
> > + * @tags: the array to validate
> > + * @len: the length in bytes of @tags
> > + *
> > + * Validate that @tags points to a valid array of struct bond_vlan_tag.
> > + * Returns: the length of the validated bytes in the array or -1 if no
> > + * valid list is found.
> > + */
> > +static int bond_validate_tags(struct bond_vlan_tag *tags, size_t len)
> > +{
> > +     size_t i, ntags = 0;
> > +
> > +     if (len == 0 || !tags)
> > +             return 0;
> > +
> > +     for (i = 0; i <= len; i = i + sizeof(struct bond_vlan_tag)) {
> > +             if (ntags > BOND_MAX_VLAN_TAGS)
> > +                     break;
> > +
> > +             if (tags->vlan_proto == BOND_VLAN_PROTO_NONE)
> > +                     return i + sizeof(struct bond_vlan_tag);
>
> You suppose that there is at least sizeof(struct bond_vlan_tag) in tags
> but there shouldn't be, it could be target_ip + 1 byte and here you will
> be out of bounds.

I will add a check for len < sizeof(struct bond_vlan_tag) .

>
> > +> +          if (tags->vlan_id > 4094)
>
> vlan tag 0 is invalid as well, and this should be using the vlan definitions
> e.g. !tags->vlan_id || tags->vlan_id >= VLAN_VID_MASK

I will make that change as well.

>
> > +                     break;
> > +             tags++;
> > +             ntags++;
> > +     }
> > +     return -1;
> >   }
> >
> >   static int bond_option_arp_ip_targets_set(struct bonding *bond,
> >                                         const struct bond_opt_value *newval)
> >   {
> > -     int ret = -EPERM;
> > -     __be32 target;
> > +     size_t len = (size_t)newval->extra_len;
> > +     char *extra = (char *)newval->extra;
> > +     struct bond_arp_target target;
> > +     int size, ret = -EPERM;
> >
> >       if (newval->string) {
> > +             /* Adding or removing arp_ip_target from sysfs */
> >               if (strlen(newval->string) < 1 ||
> > -                 !in4_pton(newval->string + 1, -1, (u8 *)&target, -1, NULL)) {
> > +                 !in4_pton(newval->string + 1, -1, (u8 *)&target.target_ip, -1, NULL)) {
> >                       netdev_err(bond->dev, "invalid ARP target specified\n");
> >                       return ret;
> >               }
> > @@ -1248,7 +1297,29 @@ static int bond_option_arp_ip_targets_set(struct bonding *bond,
> >               else
> >                       netdev_err(bond->dev, "no command found in arp_ip_targets file - use +<addr> or -<addr>\n");
> >       } else {
> > -             target = newval->value;
> > +             /* Adding arp_ip_target from netlink. aka: ip command */
> > +             if (len < sizeof(target.target_ip)) {
>
> this check is redundant, we already validate it has at least sizeof(__be32) in bond_changelink()

Check removed.

>
> > +                     netdev_err(bond->dev, "invalid ARP target specified\n");
> > +                     return ret;
> > +             }
> > +             memcpy(&target.target_ip, newval->extra, sizeof(__be32));
> > +             len = len - sizeof(target.target_ip);
> > +             extra = extra + sizeof(target.target_ip);
>
> len could be < sizeof(struct bond_vlan_tag), e.g. it could be 1
> (see above my comment about tags len)
>

 The new check in bond_validate_tags() should do it.

> > +
> > +             size = bond_validate_tags((struct bond_vlan_tag *)extra, len);
> > +
> > +             if (size > 0) {
> > +                     target.tags = kmalloc((size_t)size, GFP_ATOMIC);
> > +                     if (!target.tags)
> > +                             return -ENOMEM;
> > +                     memcpy(target.tags, extra, size);
> > +                     target.flags |= BOND_TARGET_USERTAGS;
> > +             }
>
> else if (size == -1)
>
> > +
> > +             if (size == -1)
> > +                     netdev_warn(bond->dev, "Invalid list of vlans provided with %pI4\n",
> > +                                 &target.target_ip);
> > +
> >               ret = bond_option_arp_ip_target_add(bond, target);
>
>
> I don't see target.tags freed if bond_option_arp_ip_target_add() results in an error.

Adding the kfree if (ret).

Powered by blists - more mailing lists