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: <63a83bf7-b2d3-4af6-b87b-4e166fa22744@intel.com>
Date: Wed, 15 Nov 2023 12:17:17 -0800
From: Jacob Keller <jacob.e.keller@...el.com>
To: Jiri Pirko <jiri@...nulli.us>, <netdev@...r.kernel.org>
CC: <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



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.

>  /* 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.

Ok.