| 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
| ||
|
Message-ID: <20230810091336.40951430@kernel.org>
Date: Thu, 10 Aug 2023 09:13:36 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Jiri Pirko <jiri@...nulli.us>
Cc: davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com,
pabeni@...hat.com, johannes@...solutions.net
Subject: Re: [PATCH net-next 07/10] genetlink: add genlmsg_iput() API
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, ð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))
Powered by blists - more mailing lists