[PATCH net v3] bonding: fix ad_actor_system option setting to default

[PATCH net v3] bonding: fix ad_actor_system option setting to default

[Fernando Fernandez Mancera]

When 802.3ad bond mode is configured the ad_actor_system option is set to
"00:00:00:00:00:00". But when trying to set the all-zeroes MAC as actors'
system address it was failing with EINVAL.

An all-zeroes ethernet address is valid, only multicast addresses are not
valid values.

Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
---
v2: added documentation changes and modified commit message
v3: fixed format warning on commit message
---
 Documentation/networking/bonding.rst | 11 ++++++-----
 drivers/net/bonding/bond_options.c   |  2 +-
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/Documentation/networking/bonding.rst b/Documentation/networking/bonding.rst
index 31cfd7d674a6..c0a789b00806 100644
--- a/Documentation/networking/bonding.rst
+++ b/Documentation/networking/bonding.rst
@@ -196,11 +196,12 @@ ad_actor_sys_prio
 ad_actor_system
 
 	In an AD system, this specifies the mac-address for the actor in
-	protocol packet exchanges (LACPDUs). The value cannot be NULL or
-	multicast. It is preferred to have the local-admin bit set for this
-	mac but driver does not enforce it. If the value is not given then
-	system defaults to using the masters' mac address as actors' system
-	address.
+	protocol packet exchanges (LACPDUs). The value cannot be a multicast
+	address. If the all-zeroes MAC is specified, bonding will internally
+	use the MAC of the bond itself. It is preferred to have the
+	local-admin bit set for this mac but driver does not enforce it. If
+	the value is not given then system defaults to using the masters'
+	mac address as actors' system address.
 
 	This parameter has effect only in 802.3ad mode and is available through
 	SysFs interface.
diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
index a8fde3bc458f..b93337b5a721 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -1526,7 +1526,7 @@ static int bond_option_ad_actor_system_set(struct bonding *bond,
 		mac = (u8 *)&newval->value;
 	}
 
-	if (!is_valid_ether_addr(mac))
+	if (is_multicast_ether_addr(mac))
 		goto err;
 
 	netdev_dbg(bond->dev, "Setting ad_actor_system to %pM\n", mac);
-- 
2.30.2

Re: [PATCH net v3] bonding: fix ad_actor_system option setting to default

[Jay Vosburgh]

Fernando Fernandez Mancera <ffmancera@riseup.net> wrote:

>When 802.3ad bond mode is configured the ad_actor_system option is set to
>"00:00:00:00:00:00". But when trying to set the all-zeroes MAC as actors'
>system address it was failing with EINVAL.
>
>An all-zeroes ethernet address is valid, only multicast addresses are not
>valid values.
>
>Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>

Acked-by: Jay Vosburgh <jay.vosburgh@canonical.com>

>---
>v2: added documentation changes and modified commit message
>v3: fixed format warning on commit message
>---
> Documentation/networking/bonding.rst | 11 ++++++-----
> drivers/net/bonding/bond_options.c   |  2 +-
> 2 files changed, 7 insertions(+), 6 deletions(-)
>
>diff --git a/Documentation/networking/bonding.rst b/Documentation/networking/bonding.rst
>index 31cfd7d674a6..c0a789b00806 100644
>--- a/Documentation/networking/bonding.rst
>+++ b/Documentation/networking/bonding.rst
>@@ -196,11 +196,12 @@ ad_actor_sys_prio
> ad_actor_system
> 
> 	In an AD system, this specifies the mac-address for the actor in
>-	protocol packet exchanges (LACPDUs). The value cannot be NULL or
>-	multicast. It is preferred to have the local-admin bit set for this
>-	mac but driver does not enforce it. If the value is not given then
>-	system defaults to using the masters' mac address as actors' system
>-	address.
>+	protocol packet exchanges (LACPDUs). The value cannot be a multicast
>+	address. If the all-zeroes MAC is specified, bonding will internally
>+	use the MAC of the bond itself. It is preferred to have the
>+	local-admin bit set for this mac but driver does not enforce it. If
>+	the value is not given then system defaults to using the masters'
>+	mac address as actors' system address.
> 
> 	This parameter has effect only in 802.3ad mode and is available through
> 	SysFs interface.
>diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
>index a8fde3bc458f..b93337b5a721 100644
>--- a/drivers/net/bonding/bond_options.c
>+++ b/drivers/net/bonding/bond_options.c
>@@ -1526,7 +1526,7 @@ static int bond_option_ad_actor_system_set(struct bonding *bond,
> 		mac = (u8 *)&newval->value;
> 	}
> 
>-	if (!is_valid_ether_addr(mac))
>+	if (is_multicast_ether_addr(mac))
> 		goto err;
> 
> 	netdev_dbg(bond->dev, "Setting ad_actor_system to %pM\n", mac);
>-- 
>2.30.2
>

Re: [PATCH net v3] bonding: fix ad_actor_system option setting to default

[Fernando F. Mancera]

On 12/18/21 03:34, Jay Vosburgh wrote:
> Fernando Fernandez Mancera <ffmancera@riseup.net> wrote:
> 
>> When 802.3ad bond mode is configured the ad_actor_system option is set to
>> "00:00:00:00:00:00". But when trying to set the all-zeroes MAC as actors'
>> system address it was failing with EINVAL.
>>
>> An all-zeroes ethernet address is valid, only multicast addresses are not
>> valid values.
>>
>> Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
> 
> Acked-by: Jay Vosburgh <jay.vosburgh@canonical.com>
> 
>> ---
>> v2: added documentation changes and modified commit message
>> v3: fixed format warning on commit message
>> ---
>> Documentation/networking/bonding.rst | 11 ++++++-----
>> drivers/net/bonding/bond_options.c   |  2 +-
>> 2 files changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/Documentation/networking/bonding.rst b/Documentation/networking/bonding.rst
>> index 31cfd7d674a6..c0a789b00806 100644
>> --- a/Documentation/networking/bonding.rst
>> +++ b/Documentation/networking/bonding.rst
>> @@ -196,11 +196,12 @@ ad_actor_sys_prio
>> ad_actor_system
>>
>> 	In an AD system, this specifies the mac-address for the actor in
>> -	protocol packet exchanges (LACPDUs). The value cannot be NULL or
>> -	multicast. It is preferred to have the local-admin bit set for this
>> -	mac but driver does not enforce it. If the value is not given then
>> -	system defaults to using the masters' mac address as actors' system
>> -	address.
>> +	protocol packet exchanges (LACPDUs). The value cannot be a multicast
>> +	address. If the all-zeroes MAC is specified, bonding will internally
>> +	use the MAC of the bond itself. It is preferred to have the
>> +	local-admin bit set for this mac but driver does not enforce it. If
>> +	the value is not given then system defaults to using the masters'
>> +	mac address as actors' system address.
>>
>> 	This parameter has effect only in 802.3ad mode and is available through
>> 	SysFs interface.
>> diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
>> index a8fde3bc458f..b93337b5a721 100644
>> --- a/drivers/net/bonding/bond_options.c
>> +++ b/drivers/net/bonding/bond_options.c
>> @@ -1526,7 +1526,7 @@ static int bond_option_ad_actor_system_set(struct bonding *bond,
>> 		mac = (u8 *)&newval->value;
>> 	}
>>
>> -	if (!is_valid_ether_addr(mac))
>> +	if (is_multicast_ether_addr(mac))
>> 		goto err;
>>
>> 	netdev_dbg(bond->dev, "Setting ad_actor_system to %pM\n", mac);
>> -- 
>> 2.30.2
>>

I noticed this patch state in patchwork is "changes requested"[1]. But I 
didn't get any reply or request. Is the state wrong? Should I ignore it? 
If not, what is missing?

Thanks,
Fernando.

[1] 
https://patchwork.kernel.org/project/netdevbpf/patch/20211218015001.1740-1-ffmancera@riseup.net/

Re: [PATCH net v3] bonding: fix ad_actor_system option setting to default

[Jakub Kicinski]

On Mon, 20 Dec 2021 15:53:27 +0100 Fernando F. Mancera wrote:
> I noticed this patch state in patchwork is "changes requested"[1]. But I 
> didn't get any reply or request. Is the state wrong? Should I ignore it? 

Hm, unclear why. Could you send a v4 with a Fixes tag included and CCing
all the maintainers suggested by get_maintainer.pl?

Re: [PATCH net v3] bonding: fix ad_actor_system option setting to default

[Fernando F. Mancera]

On 12/20/21 17:27, Jakub Kicinski wrote:
> On Mon, 20 Dec 2021 15:53:27 +0100 Fernando F. Mancera wrote:
>> I noticed this patch state in patchwork is "changes requested"[1]. But I
>> didn't get any reply or request. Is the state wrong? Should I ignore it?
> 
> Hm, unclear why. Could you send a v4 with a Fixes tag included and CCing
> all the maintainers suggested by get_maintainer.pl?
> 

Thanks Jakub, will do.