netdev - Re: [PATCH v4 2/3] thermal: netlink: Add genetlink bind/unbind notifications

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, 22 Feb 2024 16:47:18 +0100
From: Stanislaw Gruszka <stanislaw.gruszka@...ux.intel.com>
To: "Rafael J. Wysocki" <rafael@...nel.org>
Cc: linux-pm@...r.kernel.org,
	Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
	Ricardo Neri <ricardo.neri-calderon@...ux.intel.com>,
	Daniel Lezcano <daniel.lezcano@...aro.org>,
	"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>,
	Johannes Berg <johannes@...solutions.net>,
	Florian Westphal <fw@...len.de>, netdev@...r.kernel.org
Subject: Re: [PATCH v4 2/3] thermal: netlink: Add genetlink bind/unbind
 notifications

On Tue, Feb 13, 2024 at 02:24:56PM +0100, Rafael J. Wysocki wrote:
> On Mon, Feb 12, 2024 at 5:16 PM Stanislaw Gruszka
> <stanislaw.gruszka@...ux.intel.com> wrote:
> >
> > Introduce a new feature to the thermal netlink framework, enabling the
> > registration of sub drivers to receive events via a notifier mechanism.
> > Specifically, implement genetlink family bind and unbind callbacks to send
> > BIND and UNBIND events.
> >
> > The primary purpose of this enhancement is to facilitate the tracking of
> > user-space consumers by the intel_hif driver.
> 
> This should be intel_hfi.  Or better, Intel HFI.

Will change in next revision.

> > By leveraging these
> > notifications, the driver can determine when consumers are present
> > or absent.
> >
> > Suggested-by: Jakub Kicinski <kuba@...nel.org>
> > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@...ux.intel.com>
> > ---
> >  drivers/thermal/thermal_netlink.c | 40 +++++++++++++++++++++++++++----
> >  drivers/thermal/thermal_netlink.h | 26 ++++++++++++++++++++
> >  2 files changed, 61 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/thermal/thermal_netlink.c b/drivers/thermal/thermal_netlink.c
> > index 76a231a29654..86c7653a9530 100644
> > --- a/drivers/thermal/thermal_netlink.c
> > +++ b/drivers/thermal/thermal_netlink.c
> > @@ -7,17 +7,13 @@
> >   * Generic netlink for thermal management framework
> >   */
> >  #include <linux/module.h>
> > +#include <linux/notifier.h>
> >  #include <linux/kernel.h>
> >  #include <net/genetlink.h>
> >  #include <uapi/linux/thermal.h>
> >
> >  #include "thermal_core.h"
> >
> > -enum thermal_genl_multicast_groups {
> > -       THERMAL_GENL_SAMPLING_GROUP = 0,
> > -       THERMAL_GENL_EVENT_GROUP = 1,
> > -};
> > -
> >  static const struct genl_multicast_group thermal_genl_mcgrps[] = {
> 
> There are enough characters per code line to spell "multicast_groups"
> here (and analogously below).

Not sure what you mean, change thermal_genl_mcgrps to thermal_genl_multicast_groups ?

I could change that, but it's not really related to the changes in this patch,
so perhaps in separate patch.

Additionally "mcgrps" are more consistent with genl_family fields i.e:

      .mcgrps         = thermal_genl_mcgrps,
      .n_mcgrps       = ARRAY_SIZE(thermal_genl_mcgrps), 

> >         [THERMAL_GENL_SAMPLING_GROUP] = { .name = THERMAL_GENL_SAMPLING_GROUP_NAME, },
> >         [THERMAL_GENL_EVENT_GROUP]  = { .name = THERMAL_GENL_EVENT_GROUP_NAME,  },
> > @@ -75,6 +71,7 @@ struct param {
> >  typedef int (*cb_t)(struct param *);
> >
> >  static struct genl_family thermal_gnl_family;
> > +static BLOCKING_NOTIFIER_HEAD(thermal_gnl_chain);
> 
> thermal_genl_chain ?
> 
> It would be more consistent with the rest of the naming.

Ok, will change. Additionally in separate patch thermal_gnl_family for consistency.

> >  static int thermal_group_has_listeners(enum thermal_genl_multicast_groups group)
> >  {
> > @@ -645,6 +642,27 @@ static int thermal_genl_cmd_doit(struct sk_buff *skb,
> >         return ret;
> >  }
> >
> > +static int thermal_genl_bind(int mcgrp)
> > +{
> > +       struct thermal_genl_notify n = { .mcgrp = mcgrp };
> > +
> > +       if (WARN_ON_ONCE(mcgrp > THERMAL_GENL_MAX_GROUP))
> > +               return -EINVAL;
> 
> pr_warn_once() would be better IMO.  At least it would not crash the
> kernel configured with "panic on warn".

"panic on warn" is generic WARN_* issue at any place where WARN_* are used.
And I would say, crash is desired behaviour for those who use the option
to catch bugs. And mcgrp bigger than THERMAL_GENL_MAX_GROUP is definitely
a bug. Additionally pr_warn_once() does not print call trace, so I think
WARN_ON_ONCE() is more proper. But if really you prefer pr_warn_once()
I can change.

Regards
Stanislaw