Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Jay Vosburgh <jay.vosburgh@canonical.com>
To:     Jonathan Toppins <jtoppins@redhat.com>
cc:     netdev@vger.kernel.org, liuhangbin@gmail.com,
        Veaceslav Falico <vfalico@gmail.com>,
        Andy Gospodarek <andy@greyhouse.net>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH net v3 2/2] bonding: 802.3ad: fix no transmission of LACPDUs
In-reply-to: <3d76f4c3-916a-9abe-745f-e2781fad1b24@redhat.com>
References: <cover.1660572700.git.jtoppins@redhat.com> <0639f1e3d366c5098d561a947fd416fa5277e7f4.1660572700.git.jtoppins@redhat.com> <17000.1660655501@famine> <3d55690a-8932-4560-4267-ab28816fdb47@redhat.com> <3d76f4c3-916a-9abe-745f-e2781fad1b24@redhat.com>
Comments: In-reply-to Jonathan Toppins <jtoppins@redhat.com>
   message dated "Tue, 16 Aug 2022 13:47:07 -0400."
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Date:   Wed, 17 Aug 2022 16:24:27 -0700
Message-ID: <7721.1660778667@famine>
Precedence: bulk

Jonathan Toppins <jtoppins@redhat.com> wrote:

>On 8/16/22 09:41, Jonathan Toppins wrote:
>> On 8/16/22 09:11, Jay Vosburgh wrote:
>>> Jonathan Toppins <jtoppins@redhat.com> wrote:
>>>
>>>> This is caused by the global variable ad_ticks_per_sec being zero as
>>>> demonstrated by the reproducer script discussed below. This causes
>>>> all timer values in __ad_timer_to_ticks to be zero, resulting
>>>> in the periodic timer to never fire.
>>>>
>>>> To reproduce:
>>>> Run the script in
>>>> `tools/testing/selftests/drivers/net/bonding/bond-break-lacpdu-tx.sh`
>>>> which
>>>> puts bonding into a state where it never transmits LACPDUs.
>>>>
>>>> line 44: ip link add fbond type bond mode 4 miimon 200 \
>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 xmi=
t_hash_policy 1 ad_actor_sys_prio 65535 lacp_rate fast
>>>> setting bond param: ad_actor_sys_prio
>>>> given:
>>>> =C2=A0=C2=A0=C2=A0 params.ad_actor_system =3D 0
>>>> call stack:
>>>> =C2=A0=C2=A0=C2=A0 bond_option_ad_actor_sys_prio()
>>>> =C2=A0=C2=A0=C2=A0 -> bond_3ad_update_ad_actor_settings()
>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 -> set ad.system.sys_priority =3D=
 bond->params.ad_actor_sys_prio
>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 -> ad.system.sys_mac_addr =3D bon=
d->dev->dev_addr; because
>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 par=
ams.ad_actor_system =3D=3D 0
>>>> results:
>>>> =C2=A0=C2=A0=C2=A0=C2=A0 ad.system.sys_mac_addr =3D bond->dev->dev_addr
>>>>
>>>> line 48: ip link set fbond address 52:54:00:3B:7C:A6
>>>> setting bond MAC addr
>>>> call stack:
>>>> =C2=A0=C2=A0=C2=A0 bond->dev->dev_addr =3D new_mac
>>>>
>>>> line 52: ip link set fbond type bond ad_actor_sys_prio 65535
>>>> setting bond param: ad_actor_sys_prio
>>>> given:
>>>> =C2=A0=C2=A0=C2=A0 params.ad_actor_system =3D 0
>>>> call stack:
>>>> =C2=A0=C2=A0=C2=A0 bond_option_ad_actor_sys_prio()
>>>> =C2=A0=C2=A0=C2=A0 -> bond_3ad_update_ad_actor_settings()
>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 -> set ad.system.sys_priority =3D=
 bond->params.ad_actor_sys_prio
>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 -> ad.system.sys_mac_addr =3D bon=
d->dev->dev_addr; because
>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 par=
ams.ad_actor_system =3D=3D 0
>>>> results:
>>>> =C2=A0=C2=A0=C2=A0=C2=A0 ad.system.sys_mac_addr =3D bond->dev->dev_addr
>>>>
>>>> line 60: ip link set veth1-bond down master fbond
>>>> given:
>>>> =C2=A0=C2=A0=C2=A0 params.ad_actor_system =3D 0
>>>> =C2=A0=C2=A0=C2=A0 params.mode =3D BOND_MODE_8023AD
>>>> =C2=A0=C2=A0=C2=A0 ad.system.sys_mac_addr =3D=3D bond->dev->dev_addr
>>>> call stack:
>>>> =C2=A0=C2=A0=C2=A0 bond_enslave
>>>> =C2=A0=C2=A0=C2=A0 -> bond_3ad_initialize(); because first slave
>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 -> if ad.system.sys_mac_addr !=3D=
 bond->dev->dev_addr
>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return
>>>> results:
>>>> =C2=A0=C2=A0=C2=A0=C2=A0 Nothing is run in bond_3ad_initialize() becau=
se dev_add equals
>>>> =C2=A0=C2=A0=C2=A0=C2=A0 sys_mac_addr leaving the global ad_ticks_per_=
sec zero as it is
>>>> =C2=A0=C2=A0=C2=A0=C2=A0 never initialized anywhere else.
>>>>
>>>> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
>>>> Signed-off-by: Jonathan Toppins <jtoppins@redhat.com>
>>>> ---
>>>>
>>>> Notes:
>>>> =C2=A0=C2=A0=C2=A0 v2:
>>>> =C2=A0=C2=A0=C2=A0=C2=A0 * split this fix from the reproducer
>>>> =C2=A0=C2=A0=C2=A0 v3:
>>>> =C2=A0=C2=A0=C2=A0=C2=A0 * rebased to latest net/master
>>>>
>>>> drivers/net/bonding/bond_3ad.c | 3 ++-
>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/bonding/bond_3ad.c
>>>> b/drivers/net/bonding/bond_3ad.c
>>>> index d7fb33c078e8..957d30db6f95 100644
>>>> --- a/drivers/net/bonding/bond_3ad.c
>>>> +++ b/drivers/net/bonding/bond_3ad.c
>>>> @@ -84,7 +84,8 @@ enum ad_link_speed_type {
>>>> static const u8 null_mac_addr[ETH_ALEN + 2] __long_aligned =3D {
>>>> =C2=A0=C2=A0=C2=A0=C2=A00, 0, 0, 0, 0, 0
>>>> };
>>>> -static u16 ad_ticks_per_sec;
>>>> +
>>>> +static u16 ad_ticks_per_sec =3D 1000 / AD_TIMER_INTERVAL;
>>>> static const int ad_delta_in_ticks =3D (AD_TIMER_INTERVAL * HZ) / 1000;
>>>
>>> =C2=A0=C2=A0=C2=A0=C2=A0I still feel like this is kind of a hack, as it=
's not really
>>> fixing bond_3ad_initialize to actually work (which is the real problem
>>> as I understand it).=C2=A0 If it's ok to skip all that for this case, t=
hen
>>> why do we ever need to call bond_3ad_initialize?
>>>
>> The way it is currently written you still need to call
>> bond_3ad_initialize() just not for setting the tick resolution. The
>> issue here is ad_ticks_per_sec is used in several places to calculate
>> timer periods, __ad_timer_to_ticks(), for various timers in the 802.3ad
>> protocol. And if this variable, ad_ticks_per_sec, is left uninitialized
>> all of these timer periods go to zero. Since the value passed in
>> bond_3ad_initialize() is an immediate value I simply moved it off of the
>> call stack and set the static global variable instead.
>> To fix bond_3ad_initialize(), probably something like the below is
>> needed, but I do not understand why the guard if check was placed in
>> bond_3ad_initialize().
>
>I looked at the history of the if guard in bond_3ad_initialize and it has
>existed since the creation of the git tree. It appears since commit
>5ee14e6d336f ("bonding: 3ad: apply ad_actor settings changes immediately")
>the if guard is no longer needed and removing the if guard would also fix
>the problem, I have not tested yet.

	The logic in bond_3ad_initialize probably came that way when the
code was contributed sometime during 2.4.x.

	Curiosity got the better of me, and I looked at the 2.4.35 code.
I'm not sure what the point of the test was even then, since
bond_3ad_initialize is only called when adding a first interface to the
bond, and there wasn't a way to tweak the sys_mac_addr then.

	In the current code, I think the if test could fail if
sys_mac_addr is set manually to be equal to the bond's MAC prior to
adding the first interface to the bond.  As far as I can tell, the only
result of failing the MAC test would be that the agg selection timer
wouldn't be started, which is an optimization to reduce LACP aggregator
flailing when multiple interfaces are added at the same time (IEEE
802.1AX-2000 6.4.9 and 6.4.12.1.n).

>I think this patch series can be accepted as-is because, it does fix the
>issue as demonstrated by the kselftest accompanying the series and is the
>smallest change to fix the issue.
>
>Further, I don't see why we want to set the file-scoped variable,
>ad_ticks_per_sec, inside bond_3ad_initialize() as ad_ticks_per_sec is
>utilized across all bonds. It seems like ad_ticks_per_sec should be
>changed to a const and set at the top of the file. I see no value in
>passing the value as an unnamed constant on the stack when
>bond_3ad_initialize is called. These changes however could be done in the
>net-next tree as follow-on cleanup patches.
>
>Jay, how would you like to proceed?

	I don't have issue with moving ad_ticks_per_sec to a file scope
constant.  The minimal change here, though, is effectively making the
tick_resolution parameter to bond_3ad_initialize be ignored, even though
the compiler won't complain about it.

	Given that there is already mystery in how some of this works,
I'd prefer the patches to make the code clearer, so my vote is for the
"fix it right" method, i.e., make ad_ticks_per_sec a real constant,
remove the tick_resolution parameter from bond_3ad_initialize and drop
the "if MAC_ADDRESS_COMPARE" test therein.

	-J

---
	-Jay Vosburgh, jay.vosburgh@canonical.com