Re: [PATCH net-next 07/10] genetlink: add genlmsg_iput() API

[Jakub Kicinski <kuba@kernel.org>]

On Thu, 10 Aug 2023 11:07:41 +0200 Jiri Pirko wrote:
> >+#ifdef __LITTLE_ENDIAN
> >+#define __GENL_PTR_LOW(byte)	((void *)(unsigned long)(byte))
> >+#else
> >+#define __GENL_PTR_LOW(byte)	\
> >+	((void *)((unsigned long)(byte) << (BITS_PER_LONG - 8)))
> >+#endif
> >+
> >+/**
> >+ * GENL_INFO_NTF() - define genl_info for notifications
> >+ * @__name: name of declared variable
> >+ * @__family: pointer to the genetlink family
> >+ * @__cmd: command to be used in the notification
> >+ */
> >+#define GENL_INFO_NTF(__name, __family, __cmd)			\
> >+	struct genl_info __name = {				\
> >+		.family = (__family),				\
> >+		.genlhdr = (void *)&(__name.user_ptr[0]),	\
> >+		.user_ptr[0] = __GENL_PTR_LOW(__cmd),		\  
> 
> Ugh. Took me some time to decypher what you do here. Having endian
> specific code here seems quite odd to me. Why don't you have this as
> static inline initializer function instead and use struct genlmsghdr
> pointer to store cmd where it belong?
> 
> static inline void genl_info_ntf(struct genl_info *info,
> 				 const struct genl_family *family, u8 cmd)
> }
> 	struct genlmsghdr *hdr = (void *) &info->user_ptr[0];
> 
> 	info->family = family;
> 	info->genlhdr = hdr;
> 	hdr->cmd = cmd;
> }

Nice! The endian magic is easily the nastiest part of this series.
I considered making genlhdr a struct (rather than a pointer) because
it's actually smaller than a pointer on 64b. But dunno, feels kinda
weird to have a copy of the struct and a pointer to nlh. Hence the
magic.

And I was trying to save the 2 LoC and provide the DEFINE_ style macro
but on second thought your init helper is cleaner. I don't like the
DEFINE_ shit myself. I'll just throw in a memset(0) in there, and maybe
add a verb to the name - genl_info_init_nft()?

diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index 53f403b8efa9..e18a4c0d69ee 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -270,25 +270,26 @@ genl_info_dump(struct netlink_callback *cb)
 	return &genl_dumpit_info(cb)->info;
 }
 
-#ifdef __LITTLE_ENDIAN
-#define __GENL_PTR_LOW(byte)	((void *)(unsigned long)(byte))
-#else
-#define __GENL_PTR_LOW(byte)	\
-	((void *)((unsigned long)(byte) << (BITS_PER_LONG - 8)))
-#endif
-
 /**
- * GENL_INFO_NTF() - define genl_info for notifications
- * @__name: name of declared variable
- * @__family: pointer to the genetlink family
- * @__cmd: command to be used in the notification
+ * genl_info_init_ntf() - initialize genl_info for notifications
+ * @info:   genl_info struct to set up
+ * @family: pointer to the genetlink family
+ * @cmd:    command to be used in the notification
+ *
+ * Initialize a locally declared struct genl_info to pass to various APIs.
+ * Intended to be used when creating notifications.
  */
-#define GENL_INFO_NTF(__name, __family, __cmd)			\
-	struct genl_info __name = {				\
-		.family = (__family),				\
-		.genlhdr = (void *)&(__name.user_ptr[0]),	\
-		.user_ptr[0] = __GENL_PTR_LOW(__cmd),		\
-	}
+static inline void
+genl_info_init_ntf(struct genl_info *info, const struct genl_family *family,
+		   u8 cmd)
+{
+	struct genlmsghdr *hdr = (void *) &info->user_ptr[0];
+
+	memset(info, 0, sizeof(*info));
+	info->family = family;
+	info->genlhdr = hdr;
+	hdr->cmd = cmd;
+}
 
 static inline bool genl_info_is_ntf(const struct genl_info *info)
 {
@@ -316,7 +317,10 @@ __genlmsg_iput(struct sk_buff *skb, const struct genl_info *info, int flags)
  * @info: genl_info as provided to do/dump handlers
  *
  * Convenience wrapper which starts a genetlink message based on
- * information in user request (or info constructed with GENL_INFO_NTF()).
+ * information in user request. @info should be either the struct passed
+ * by genetlink core to do/dump handlers (when constructing replies to
+ * such requests) or a struct initialized by genl_info_init_ntf()
+ * when constructing notifications.
  *
  * Returns pointer to new genetlink header.
  */
diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
index 8e4e78bbd71a..c1aea8b756b6 100644
--- a/net/core/netdev-genl.c
+++ b/net/core/netdev-genl.c
@@ -41,13 +41,15 @@ netdev_nl_dev_fill(struct net_device *netdev, struct sk_buff *rsp,
 static void
 netdev_genl_dev_notify(struct net_device *netdev, int cmd)
 {
-	GENL_INFO_NTF(info, &netdev_nl_family, cmd);
+	struct genl_info info;
 	struct sk_buff *ntf;
 
 	if (!genl_has_listeners(&netdev_nl_family, dev_net(netdev),
 				NETDEV_NLGRP_MGMT))
 		return;
 
+	genl_info_init_ntf(&info, &netdev_nl_family, cmd);
+
 	ntf = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL);
 	if (!ntf)
 		return;
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index 5aa319c4279c..3bbd5afb7b31 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -644,15 +644,17 @@ ethnl_default_notify_ops[ETHTOOL_MSG_KERNEL_MAX + 1] = {
 static void ethnl_default_notify(struct net_device *dev, unsigned int cmd,
 				 const void *data)
 {
-	GENL_INFO_NTF(info, &ethtool_genl_family, cmd);
 	struct ethnl_reply_data *reply_data;
 	const struct ethnl_request_ops *ops;
 	struct ethnl_req_info *req_info;
+	struct genl_info info;
 	struct sk_buff *skb;
 	void *reply_payload;
 	int reply_len;
 	int ret;
 
+	genl_info_init_ntf(&info, &ethtool_genl_family, cmd);
+
 	if (WARN_ONCE(cmd > ETHTOOL_MSG_KERNEL_MAX ||
 		      !ethnl_default_notify_ops[cmd],
 		      "unexpected notification type %u\n", cmd))