netdev - Re: [PATCH net-next 03/13] genetlink: introduce split op representation

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: <93e9137fb80f63cd13fa226bcca3007c473a74d4.camel@sipsolutions.net>
Date:   Wed, 19 Oct 2022 09:59:24 +0200
From:   Johannes Berg <johannes@...solutions.net>
To:     Jakub Kicinski <kuba@...nel.org>, davem@...emloft.net
Cc:     netdev@...r.kernel.org, edumazet@...gle.com, pabeni@...hat.com,
        jiri@...nulli.us, razor@...ckwall.org, nicolas.dichtel@...nd.com,
        gnault@...hat.com, jacob.e.keller@...el.com, fw@...len.de,
        mareklindner@...mailbox.ch, sw@...onwunderlich.de, a@...table.cc,
        sven@...fation.org, jiri@...dia.com, nhorman@...driver.com,
        alex.aring@...il.com, stefan@...enfreihafen.org
Subject: Re: [PATCH net-next 03/13] genetlink: introduce split op
 representation

On Tue, 2022-10-18 at 16:07 -0700, Jakub Kicinski wrote:
> 
> +/**
> + * struct genl_split_ops - generic netlink operations (do/dump split version)
> + * @cmd: command identifier
> + * @internal_flags: flags used by the family
> + * @flags: GENL_* flags (%GENL_ADMIN_PERM or %GENL_UNS_ADMIN_PERM)
> + * @validate: validation flags from enum genl_validate_flags
> + * @policy: netlink policy (takes precedence over family policy)
> + * @maxattr: maximum number of attributes supported
> +  *

nit: extra space here

> + * Do callbacks:
> + * @pre_doit: called before an operation's @doit callback, it may
> + *	do additional, common, filtering and return an error
> + * @doit: standard command callback
> + * @post_doit: called after an operation's @doit callback, it may
> + *	undo operations done by pre_doit, for example release locks


Is that really worth it? I mean, if you need pre/post for a *specific*
op, you can just roll that into it.

Maybe the use case would be something like "groups" where some commands
need one set of pre/post, and some other commands need another set, and
then it's still simpler to do as pre/post rather than calling them
inside the doit()?

(and you also have space for the pointers given the dump part of the
union, so ...)

> +static void
> +genl_cmd_full_to_split(struct genl_split_ops *op,
> +		       const struct genl_family *family,
> +		       const struct genl_ops *full, u8 flags)
> +{

[...]


> +	op->flags		|= flags;


why |= ?

> @@ -776,8 +821,9 @@ static int genl_family_rcv_msg(const struct genl_family *family,
>  {
>  	struct net *net = sock_net(skb->sk);
>  	struct genlmsghdr *hdr = nlmsg_data(nlh);
> -	struct genl_ops op;
> +	struct genl_split_ops op;

it's not even initialized?


> +	flags = (nlh->nlmsg_flags & NLM_F_DUMP) == NLM_F_DUMP ?
> +		GENL_CMD_CAP_DUMP : GENL_CMD_CAP_DO;
> +	if (genl_get_cmd_split(hdr->cmd, flags, family, &op))
>  		return -EOPNOTSUPP;

before being used

or am I misreading something?

johannes