netdev - Re: [PATCH net-next 07/10] genetlink: add genlmsg

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: <ZNUTdSE3CcQbK07D@nanopsycho>
Date: Thu, 10 Aug 2023 18:42:29 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: Jakub Kicinski <kuba@...nel.org>
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

Thu, Aug 10, 2023 at 06:13:36PM CEST, kuba@...nel.org wrote:
>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()?

Yep, looks fine to me. Thanks!