netdev - Re: [PATCH net-next] bonding: 3ad: send rtnl 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] [day] [month] [year] [list]

Date: Fri, 21 Jun 2024 13:58:54 +0300
From: Nikolay Aleksandrov <razor@...ckwall.org>
To: Hangbin Liu <liuhangbin@...il.com>
Cc: netdev@...r.kernel.org, Jay Vosburgh <j.vosburgh@...il.com>,
 Andy Gospodarek <andy@...yhouse.net>, "David S. Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Jiri Pirko <jiri@...nulli.us>, Daniel Borkmann <daniel@...earbox.net>,
 Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
 Lorenzo Bianconi <lorenzo@...nel.org>, Ido Schimmel <idosch@...dia.com>,
 Amit Cohen <amcohen@...dia.com>
Subject: Re: [PATCH net-next] bonding: 3ad: send rtnl ifinfo notify when mux
 state changed

On 6/20/24 12:08, Hangbin Liu wrote:
> Hi Nikolay,
> On Thu, Jun 20, 2024 at 11:47:46AM +0300, Nikolay Aleksandrov wrote:
>> On 6/20/24 09:10, Hangbin Liu wrote:
>>> Currently, administrators need to retrieve LACP mux state changes from
>>> the kernel DEBUG log using netdev_dbg and slave_dbg macros. To simplify
>>> this process, let's send the ifinfo notification whenever the mux state
>>> changes. This will enable users to directly access and monitor this
>>> information using the ip monitor command.
>>>
>>> To achieve this, add a new enum NETDEV_LACP_STATE_CHANGE in netdev_cmd.
>>>
>>> Signed-off-by: Hangbin Liu <liuhangbin@...il.com>
>>> ---
>>>  drivers/net/bonding/bond_3ad.c | 2 ++
>>>  include/linux/netdevice.h      | 1 +
>>>  net/core/dev.c                 | 2 +-
>>>  net/core/rtnetlink.c           | 1 +
>>>  4 files changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>>> index c6807e473ab7..bcd8b16173f2 100644
>>> --- a/drivers/net/bonding/bond_3ad.c
>>> +++ b/drivers/net/bonding/bond_3ad.c
>>> @@ -1185,6 +1185,8 @@ static void ad_mux_machine(struct port *port, bool *update_slave_arr)
>>>  		default:
>>>  			break;
>>>  		}
>>> +
>>> +		call_netdevice_notifiers(NETDEV_LACP_STATE_CHANGE, port->slave->dev);
>>>  	}
>>
>> This will cause sleeping while atomic because
>> ad_mux_machine() is called in atomic context (both rcu and bond mode
>> spinlock held with bh disabled) in bond_3ad_state_machine_handler().
> 
> Ah, that's why we call the bond_slave_state_notify() after spin_unlock_bh()?
> Where can I check if call_netdevice_notifiers() would sleep? So I can avoid
> this error next time.
> 
>> Minor (and rather more personal pref) I'd split the addition of the new
>> event and adding its first user (bond) for separate review.
> 
> Hmm, with out using call_netdevice_notifiers(). How about just call
> rtmsg_ifinfo() or rtmsg_ifinfo_event() directly?
> 
> Thanks
> Hangbin

Yep, I think that would be fine (w/ GFP_ATOMIC of course, if under the
locks).
Excuse me for the late response, have been having connectivity
issues the past few days.

Cheers,
 Nik