netdev - Re: [PATCH v5 net-next 02/12] net-shapers: implement NL get operation

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]

Message-ID: <57ef8eb8-9534-4061-ba6c-4dadaf790c45@redhat.com>
Date: Fri, 30 Aug 2024 12:55:05 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: netdev@...r.kernel.org, Jiri Pirko <jiri@...nulli.us>,
 Madhu Chittim <madhu.chittim@...el.com>,
 Sridhar Samudrala <sridhar.samudrala@...el.com>,
 Simon Horman <horms@...nel.org>, John Fastabend <john.fastabend@...il.com>,
 Sunil Kovvuri Goutham <sgoutham@...vell.com>,
 Jamal Hadi Salim <jhs@...atatu.com>, Donald Hunter
 <donald.hunter@...il.com>, anthony.l.nguyen@...el.com,
 przemyslaw.kitszel@...el.com, intel-wired-lan@...ts.osuosl.org,
 edumazet@...gle.com
Subject: Re: [PATCH v5 net-next 02/12] net-shapers: implement NL get operation

On 8/30/24 03:20, Jakub Kicinski wrote:>> +/* Initialize the context 
fetching the relevant device and
>> + * acquiring a reference to it.
>> + */
>> +static int net_shaper_ctx_init(const struct genl_info *info, int type,
>> +			       struct net_shaper_nl_ctx *ctx)
>> +{
>> +	struct net *ns = genl_info_net(info);
>> +	struct net_device *dev;
>> +	int ifindex;
>> +
>> +	memset(ctx, 0, sizeof(*ctx));
>> +	if (GENL_REQ_ATTR_CHECK(info, type))
>> +		return -EINVAL;
>> +
>> +	ifindex = nla_get_u32(info->attrs[type]);
> 
> Let's limit the 'binding' thing to just driver call sites, we can
> redo the rest easily later. This line and next pretends to take
> "arbitrary" type but clearly wants a ifindex/netdev, right?

There is a misunderstanding. This helper will be used in a following 
patch (7/12) with a different 'type' argument: 
NET_SHAPER_A_BINDING_IFINDEX. I've put a note in the commit message, but 
was unintentionally dropped in one of the recent refactors. I'll add 
that note back.

I hope you are ok with the struct net_shaper_binding * argument to most 
helpers? does not add complexity, will help to support devlink objects 
and swapping back and forth from/to struct net_device* can't be automated.

[...]
> Maybe send a patch like this, to avoid having to allocate this space,
> and special casing dump vs doit:
> 
> diff --git a/include/net/genetlink.h b/include/net/genetlink.h
> index 9ab49bfeae78..7658f0885178 100644
> --- a/include/net/genetlink.h
> +++ b/include/net/genetlink.h
> @@ -124,7 +124,8 @@ struct genl_family {
>    * @genlhdr: generic netlink message header
>    * @attrs: netlink attributes
>    * @_net: network namespace
> - * @user_ptr: user pointers
> + * @ctx: storage space for the use by the family
> + * @user_ptr: user pointers (deprecated, use ctx instead)
>    * @extack: extended ACK report struct
>    */
>   struct genl_info {
> @@ -135,7 +136,10 @@ struct genl_info {
>   	struct genlmsghdr *	genlhdr;
>   	struct nlattr **	attrs;
>   	possible_net_t		_net;
> -	void *			user_ptr[2];
> +	union {
> +		u8		ctx[48];
> +		void *		user_ptr[2];
> +	};
>   	struct netlink_ext_ack *extack;
>   };

Makes sense. Plus likely:

#define NETLINK_CTX_SIZE 48

and use such define above and in linux/netlink.h

Thanks,

Paolo