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]

Date: Thu, 27 Jun 2024 18:05:40 +0800
From: Hangbin Liu <liuhangbin@...il.com>
To: Nikolay Aleksandrov <razor@...ckwall.org>
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 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