Re: [PATCH net-next 07/10] genetlink: add genlmsg_iput() API
From: Jakub Kicinski <kuba@kernel.org>
Date: 2023-08-10 16:13:37
Subsystem:
networking [ethtool], networking [general], the rest · Maintainers:
Andrew Lunn, Jakub Kicinski, "David S. Miller", Eric Dumazet, Paolo Abeni, Linus Torvalds
On Thu, 10 Aug 2023 11:07:41 +0200 Jiri Pirko wrote:
quoted
+#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, ðtool_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, ðtool_genl_family, cmd); + if (WARN_ONCE(cmd > ETHTOOL_MSG_KERNEL_MAX || !ethnl_default_notify_ops[cmd], "unexpected notification type %u\n", cmd))