netdev - Re: [PATCHv3 net-next] bonding: 3ad: send ifinfo notify when mux state changed

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <89249184-41ac-42f6-b5af-4a46f9b28247@blackwall.org>
Date: Thu, 27 Jun 2024 13:33:10 +0300
From: Nikolay Aleksandrov <razor@...ckwall.org>
To: Hangbin Liu <liuhangbin@...il.com>
Cc: Jay Vosburgh <jay.vosburgh@...onical.com>,
 Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
 Andy Gospodarek <andy@...yhouse.net>, "David S. Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Paolo Abeni <pabeni@...hat.com>, Ido Schimmel <idosch@...dia.com>,
 Jiri Pirko <jiri@...nulli.us>, Amit Cohen <amcohen@...dia.com>
Subject: Re: [PATCHv3 net-next] bonding: 3ad: send ifinfo notify when mux
 state changed

On 27/06/2024 13:05, Hangbin Liu wrote:
> On Thu, Jun 27, 2024 at 11:29:21AM +0300, Nikolay Aleksandrov wrote:
>> On 27/06/2024 11:26, Hangbin Liu wrote:
>>> On Wed, Jun 26, 2024 at 05:06:00PM -0700, Jay Vosburgh wrote:
>>>>> Hits:
>>>>>
>>>>> RTNL: assertion failed at net/core/rtnetlink.c (1823)
>>>
>>> Thanks for this hits...
>>>
>>>>>
>>>>> On two selftests. Please run the selftests on a debug kernel..
>>>
>>> OK, I will try run my tests on debug kernel in future.
>>>
>>>>
>>>> 	Oh, I forgot about needing RTNL.
>>>>
>>
>> +1 & facepalm, completely forgot it was running without rtnl
>>
>>>> 	We cannot simply acquire RTNL in ad_mux_machine(), as the
>>>> bond->mode_lock is already held, and the lock ordering must be RTNL
>>>> first, then mode_lock, lest we deadlock.
>>>>
>>>> 	Hangbin, I'd suggest you look at how bond_netdev_notify_work()
>>>> complies with the lock ordering (basically, doing the actual work out of
>>>> line in a workqueue event), or how the "should_notify" flag is used in
>>>> bond_3ad_state_machine_handler().  The first is more complicated, but
>>>> won't skip events; the second may miss intermediate state transitions if
>>>> it cannot acquire RTNL and has to delay the notification.
>>>
>>> I think the administer doesn't want to loose the state change info. So how
>>> about something like:
>>>
>>
>> You can (and will) miss events with the below code. It is kind of best effort,
>> but if the notification is not run before the next state change, you will
>> lose the intermediate changes.
> 
> Yes, but at least the admin could get the latest state. With the following
> code the admin may not get the latest update if lock rtnl failed.
> 
>         if (should_notify_rtnl && rtnl_trylock()) {
>                 bond_slave_lacp_notify(bond);
>                 rtnl_unlock();
> 	}
> 
> Thanks
> Hangbin

Well, you mentioned administrators want to see the state changes, please
better clarify the exact end goal. Note that technically may even not be
the last state as the state change itself happens in parallel (different
locks) and any update could be delayed depending on rtnl availability
and workqueue re-scheduling. But sure, they will get some update at some point. :)

It all depends on what are the requirements.

An uglier but lockless alternative would be to poll the slave's sysfs oper state,
that doesn't require any locks and would be up-to-date.

Cheers,
 Nik