Message-ID: <0cf528b8-7d6a-4e50-b91c-d83b4cb65e2b@blackwall.org>
Date: Thu, 16 Oct 2025 14:48:00 +0300
From: Nikolay Aleksandrov <razor@...ckwall.org>
To: David Wilder <wilder@...ibm.com>, netdev@...r.kernel.org
Cc: jv@...sburgh.net, pradeep@...ibm.com, i.maximets@....org,
 amorenoz@...hat.com, haliu@...hat.com, stephen@...workplumber.org,
 horms@...nel.org, kuba@...nel.org, pabeni@...hat.com, andrew+netdev@...n.ch,
 edumazet@...gle.com
Subject: Re: [PATCH net-next v13 0/7] bonding: Extend arp_ip_target format to
 allow for a list of vlan tags.

On 10/14/25 02:52, David Wilder wrote:
> The current implementation of the arp monitor builds a list of vlan-tags by
> following the chain of net_devices above the bond. See bond_verify_device_path().
> Unfortunately, with some configurations, this is not possible. One example is
> when an ovs switch is configured above the bond.
> 
> This change extends the "arp_ip_target" parameter format to allow for a list of
> vlan tags to be included for each arp target. This new list of tags is optional
> and may be omitted to preserve the current format and process of discovering
> vlans.
> 
> The new format for arp_ip_target is:
> arp_ip_target ipv4-address[vlan-tag\...],...
> 
> For example:
> arp_ip_target 10.0.0.1[10/20]
> arp_ip_target 10.0.0.1[] (used to disable vlan discovery)
> 
> Changes since V12
> Fixed uninitialized variable in bond_option_arp_ip_targets_set() (patch 4)
> causing a CI failure.
> 
> Changes since V11
> No Change.
> 
> Changes since V10
> Thanks Paolo:
> - 1/7 Changed the layout of struct bond_arp_target to reduce size of the struct.
> - 3/7 Fixed format 'size-num' -> 'size - num'
> - 7/7 Updated selftest (bond-arp-ip-target.sh). Removed sleep 10 in check_failure_count().
>       Added call to tc to verify arp probes are reaching the target interface. Then I verify that
>       the Link Failure counts are not increasing over "time".  Arp probes are sent every 100ms,
>       two missed probes will trigger a Link failure. A one second wait between checking counts
>       should be be more than sufficient.  This speeds up the execution of the test.
> 
> Thanks Nikolay:
> - 4/7 In bond_option_arp_ip_targets_clear() I changed the definition of empty_target to empty_target = {}.
> -     bond_validate_tags() now verifies input is a multiple of sizeof(struct bond_vlan_tag).
>       Updated VID validity check to use: !tags->vlan_id || tags->vlan_id >= VLAN_VID_MASK) as suggested.
> -     In bond_option_arp_ip_targets_set() removed the redundant length check of target.target_ip.
> -     Added kfree(target.tags) when bond_option_arp_ip_target_add() results in an error.
> -     Removed the caching of struct bond_vlan_tag returned by bond_verify_device_path(), Nikolay
>       pointed out that caching tags prevented the detection of VLAN configuration changes. 
>       Added a kfree(tags) for tags allocated in bond_verify_device_path().
> 
> Jay, Nikolay and I had a discussion regarding locking when adding, deleting or changing vlan tags.
> Jay pointed out that user supplied tags that are stashed in the bond configuration and can only be
> changed via user space this can be done safely in an RCU manner as netlink always operates with RTNL
> held. If user space provided tags and then replumbs things, it'll be on user space to update the tags
> in a safe manor.  
> 
> I was concerned about changing options on a configured bond,  I found that attempting to change
> a bonds configuration (using "ip set") will abort the attempt to make a change if the bond's state is
> "UP" or has slaves configured. Therefor the configuration and operational side of a bond is separated.
> I agree with Jay that the existing locking scheme is sufficient.
> 
> Change since V9
> Fix kdoc build error.
> 
> Changes since V8:
> Moved the #define BOND_MAX_VLAN_TAGS from patch 6 to patch 3.
> Thanks Simon for catching the bisection break.
> 
> Changes since V7:
> These changes should eliminate the CI failures I have been seeing.
> 1) patch 2, changed type of bond_opt_value.extra_len to size_t.
> 2) Patch 4, added bond_validate_tags() to validate the array of bond_vlan_tag provided by
>  the user.
> 
> Changes since V6:
> 1) I made a number of changes to fix the failure seen in the
> kernel CI.  I am still unable to reproduce the this failure, hopefully I
> have fixed it.  These change are in patch #4 to functions:
> bond_option_arp_ip_targets_clear() and
> bond_option_arp_ip_targets_set()
> 
> Changes since V5: Only the last 2 patches have changed since V5.
> 1) Fixed sparse warning in bond_fill_info().
> 2) Also in bond_fill_info() I resolved data.addr uninitialized when if condition is not met.
> Thank you Simon for catching this. Note: The change is different that what I shared earlier.
> 3) Fixed shellcheck warnings in test script: Blocked source warning, Ignored specific unassigned
> references and exported ALL_TESTS to resolve a reference warning.
> 
> Changes since V4:
> 1)Dropped changes to proc and sysfs APIs to bonding.  These APIs 
> do not need to be updated to support new functionality.  Netlink
> and iproute2 have been updated to do the right thing, but the
> other APIs are more or less frozen in the past.
> 
> 2)Jakub reported a warning triggered in bond_info_seq_show() during
> testing.  I was unable to reproduce this warning or identify
> it with code inspection.  However, all my changes to bond_info_seq_show()
> have been dropped as unnecessary (see above).
> Hopefully this will resolve the issue. 
> 
> 3)Selftest script has been updated based on the results of shellcheck.
> Two unresolved references that are not possible to resolve are all
> that remain.
> 
> 4)A patch was added updating bond_info_fill()
> to support "ip -d show <bond-device>" command.
> 
> The inclusion of a list of vlan tags is optional. The new logic
> preserves both forward and backward compatibility with the kernel
> and iproute2 versions.
> 
> Changes since V3:
> 1) Moved the parsing of the extended arp_ip_target out of the kernel and into
>    userspace (ip command). A separate patch to iproute2 to follow shortly.
> 2) Split up the patch set to make review easier.
> 
> Please see iproute changes in a separate posting.
> 
> Thank you for your time and reviews.
> 
> Signed-off-by: David Wilder <wilder@...ibm.com>
> David Wilder (7):
>   bonding: Adding struct bond_arp_target
>   bonding: Adding extra_len field to struct bond_opt_value.
>   bonding: arp_ip_target helpers.
>   bonding: Processing extended arp_ip_target from user space.
>   bonding: Update to bond_arp_send_all() to use supplied vlan tags
>   bonding: Update for extended arp_ip_target format.
>   bonding: Selftest and documentation for the arp_ip_target parameter.
> 
>  Documentation/networking/bonding.rst          |  11 +
>  drivers/net/bonding/bond_main.c               |  48 ++--
>  drivers/net/bonding/bond_netlink.c            |  35 ++-
>  drivers/net/bonding/bond_options.c            | 140 +++++++++---
>  drivers/net/bonding/bond_procfs.c             |   4 +-
>  drivers/net/bonding/bond_sysfs.c              |   4 +-
>  include/net/bond_options.h                    |  29 ++-
>  include/net/bonding.h                         |  65 +++++-
>  .../selftests/drivers/net/bonding/Makefile    |   1 +
>  .../drivers/net/bonding/bond-arp-ip-target.sh | 205 ++++++++++++++++++
>  10 files changed, 467 insertions(+), 75 deletions(-)
>  create mode 100755 tools/testing/selftests/drivers/net/bonding/bond-arp-ip-target.sh
> 

I remember reviewing previous versions of this patch-set, please CC all
reviewers for future versions.

Thanks,
 Nik

Powered by blists - more mailing lists