netdev - Re: [PATCH net-next 08/13] genetlink: inline genl_get

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: <d3ef5be8-6fcb-3fe3-97f1-c0b8321230f4@intel.com>
Date:   Wed, 19 Oct 2022 14:46:06 -0700
From:   Jacob Keller <jacob.e.keller@...el.com>
To:     Jakub Kicinski <kuba@...nel.org>, <davem@...emloft.net>,
        <johannes@...solutions.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>, <fw@...len.de>
Subject: Re: [PATCH net-next 08/13] genetlink: inline genl_get_cmd()



On 10/18/2022 4:07 PM, Jakub Kicinski wrote:
> All callers go via genl_get_cmd_split() now,
> so merge genl_get_cmd() into it.
> 
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>

Ah nice. A lot of the time I would see code like this where the first
refactor and this merge were just done as a single thing. (it is easier
to manage but much harder to review). Thanks for keeping it split out.

Reviewed-by: Jacob Keller <jacob.e.keller@...el.com>

> ---
>  net/netlink/genetlink.c | 30 ++++++++++++------------------
>  1 file changed, 12 insertions(+), 18 deletions(-)
> 
> diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
> index 3e821c346577..9bfb110053c7 100644
> --- a/net/netlink/genetlink.c
> +++ b/net/netlink/genetlink.c
> @@ -158,14 +158,6 @@ static int genl_get_cmd_small(u32 cmd, const struct genl_family *family,
>  	return -ENOENT;
>  }
>  
> -static int genl_get_cmd(u32 cmd, const struct genl_family *family,
> -			struct genl_ops *op)
> -{
> -	if (!genl_get_cmd_full(cmd, family, op))
> -		return 0;
> -	return genl_get_cmd_small(cmd, family, op);
> -}
> -
>  static int
>  genl_cmd_full_to_split(struct genl_split_ops *op,
>  		       const struct genl_family *family,
> @@ -207,13 +199,15 @@ genl_cmd_full_to_split(struct genl_split_ops *op,
>  }
>  
>  static int
> -genl_get_cmd_split(u32 cmd, u8 flags, const struct genl_family *family,
> -		   struct genl_split_ops *op)
> +genl_get_cmd(u32 cmd, u8 flags, const struct genl_family *family,
> +	     struct genl_split_ops *op)
>  {
>  	struct genl_ops full;
>  	int err;
>  
> -	err = genl_get_cmd(cmd, family, &full);
> +	err = genl_get_cmd_full(cmd, family, &full);
> +	if (err == -ENOENT)
> +		err = genl_get_cmd_small(cmd, family, &full);
>  	if (err) {
>  		memset(op, 0, sizeof(*op));
>  		return err;
> @@ -841,7 +835,7 @@ static int genl_family_rcv_msg(const struct genl_family *family,
>  
>  	flags = (nlh->nlmsg_flags & NLM_F_DUMP) == NLM_F_DUMP ?
>  		GENL_CMD_CAP_DUMP : GENL_CMD_CAP_DO;

Ahhh this is where the GENL_CMD_CAP_DUMP vs GENL_CMD_CAP_DO is kept
separate. Either NLM_F_DUMP is set or not set so only one of these flags
will be set. Ok.

> -	if (genl_get_cmd_split(hdr->cmd, flags, family, &op))
> +	if (genl_get_cmd(hdr->cmd, flags, family, &op))
>  		return -EOPNOTSUPP;
>  
>  	if ((op.flags & GENL_ADMIN_PERM) &&
> @@ -1239,8 +1233,8 @@ static int ctrl_dumppolicy_start(struct netlink_callback *cb)
>  		ctx->single_op = true;
>  		ctx->op = nla_get_u32(tb[CTRL_ATTR_OP]);
>  
> -		if (genl_get_cmd_split(ctx->op, GENL_CMD_CAP_DO, rt, &doit) &&
> -		    genl_get_cmd_split(ctx->op, GENL_CMD_CAP_DUMP, rt, &dump)) {
> +		if (genl_get_cmd(ctx->op, GENL_CMD_CAP_DO, rt, &doit) &&
> +		    genl_get_cmd(ctx->op, GENL_CMD_CAP_DUMP, rt, &dump)) {
>  			NL_SET_BAD_ATTR(cb->extack, tb[CTRL_ATTR_OP]);
>  			return -ENOENT;
>  		}
> @@ -1380,10 +1374,10 @@ static int ctrl_dumppolicy(struct sk_buff *skb, struct netlink_callback *cb)
>  		struct genl_ops op;
>  
>  		if (ctx->single_op) {
> -			if (genl_get_cmd_split(ctx->op, GENL_CMD_CAP_DO,
> -					       ctx->rt, &doit) &&
> -			    genl_get_cmd_split(ctx->op, GENL_CMD_CAP_DUMP,
> -					       ctx->rt, &dumpit)) {
> +			if (genl_get_cmd(ctx->op, GENL_CMD_CAP_DO,
> +					 ctx->rt, &doit) &&
> +			    genl_get_cmd(ctx->op, GENL_CMD_CAP_DUMP,
> +					 ctx->rt, &dumpit)) {
>  				WARN_ON(1);
>  				return -ENOENT;
>  			}