netdev - Re: [patch net-next 3/8] devlink: send notifications only if there are listeners

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 for Android: free password hash cracker in your pocket

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

Message-ID: <ZVXBUorPrSmd7UNl@nanopsycho>
Date: Thu, 16 Nov 2023 08:14:26 +0100
From: Jiri Pirko <jiri@...nulli.us>
To: Jacob Keller <jacob.e.keller@...el.com>
Cc: netdev@...r.kernel.org, kuba@...nel.org, pabeni@...hat.com,
	davem@...emloft.net, edumazet@...gle.com, jhs@...atatu.com,
	johannes@...solutions.net, andriy.shevchenko@...ux.intel.com,
	amritha.nambiar@...el.com, sdf@...gle.com
Subject: Re: [patch net-next 3/8] devlink: send notifications only if there
 are listeners

Wed, Nov 15, 2023 at 09:17:17PM CET, jacob.e.keller@...el.com wrote:
>
>
>On 11/15/2023 6:17 AM, Jiri Pirko wrote:
>> +static inline bool devlink_nl_notify_need(struct devlink *devlink)
>> +{
>> +	return genl_has_listeners(&devlink_nl_family, devlink_net(devlink),
>> +				  DEVLINK_MCGRP_CONFIG);
>> +}
>> +
>
>I assume following changes will add more checks here to filter here so
>it doesn't make sense to call this "devlink_has_listeners"? I feel like
>the devlink_nl_notify_need is a bit weird of a way to phrase this.
>
>I don't have a strong objection to the name overall, just found it a bit
>odd.

Right. I named it like this because eventually, this function is going
to check the filters as well return false if there is no msg passed to
any listening socket through any filter. That is the plan.


>
>>  /* Notify */
>>  void devlink_notify_register(struct devlink *devlink);
>>  void devlink_notify_unregister(struct devlink *devlink);
>> diff --git a/net/devlink/health.c b/net/devlink/health.c
>> index 695df61f8ac2..93eae8b5d2d3 100644
>> --- a/net/devlink/health.c
>> +++ b/net/devlink/health.c
>> @@ -496,6 +496,9 @@ static void devlink_recover_notify(struct devlink_health_reporter *reporter,
>>  	WARN_ON(cmd != DEVLINK_CMD_HEALTH_REPORTER_RECOVER);
>>  	ASSERT_DEVLINK_REGISTERED(devlink);
>>  
>> +	if (!devlink_nl_notify_need(devlink))
>> +		return;
>> +
>>  	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>>  	if (!msg)
>>  		return;
>> diff --git a/net/devlink/linecard.c b/net/devlink/linecard.c
>> index 9d080ac1734b..45b36975ee6f 100644
>> --- a/net/devlink/linecard.c
>> +++ b/net/devlink/linecard.c
>> @@ -136,7 +136,7 @@ static void devlink_linecard_notify(struct devlink_linecard *linecard,
>>  	WARN_ON(cmd != DEVLINK_CMD_LINECARD_NEW &&
>>  		cmd != DEVLINK_CMD_LINECARD_DEL);
>>  
>> -	if (!__devl_is_registered(devlink))
>> +	if (!__devl_is_registered(devlink) || !devlink_nl_notify_need(devlink))
>>  		return;
>>  
>
>A bunch of callers are checking both if its registered and a
>notification is needed, does it make sense to combine this? Or I guess
>at least a few places are notifying of removal after its no longer
>registered, so we can't inline the devl_is_registered into the
>devlink_nl_notify_need. Probably more clear to keep it separate too.

Yeah, it is more clear to have it separate. Plus there would have to be
2 locked and unlocked versions.


>
>Ok.