netdev - Re: [PATCH net-next 07/13] genetlink: support split policies in ctrl_dumppolicy_put

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]

Date:   Wed, 19 Oct 2022 14:38:41 -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 07/13] genetlink: support split policies in
 ctrl_dumppolicy_put_op()



On 10/18/2022 4:07 PM, Jakub Kicinski wrote:
> Pass do and dump versions of the op to ctrl_dumppolicy_put_op()
> so that it can provide a different policy index for the two.
> 
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>

Makes sense to me.

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

> ---
>  net/netlink/genetlink.c | 55 ++++++++++++++++++++++++-----------------
>  1 file changed, 33 insertions(+), 22 deletions(-)
> 
> diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
> index 234b27977013..3e821c346577 100644
> --- a/net/netlink/genetlink.c
> +++ b/net/netlink/genetlink.c
> @@ -1319,7 +1319,8 @@ static void *ctrl_dumppolicy_prep(struct sk_buff *skb,
>  
>  static int ctrl_dumppolicy_put_op(struct sk_buff *skb,
>  				  struct netlink_callback *cb,
> -			          struct genl_ops *op)
> +			          struct genl_split_ops *doit,
> +				  struct genl_split_ops *dumpit)
>  {
>  	struct ctrl_dump_policy_ctx *ctx = (void *)cb->ctx;
>  	struct nlattr *nest_pol, *nest_op;
> @@ -1327,10 +1328,7 @@ static int ctrl_dumppolicy_put_op(struct sk_buff *skb,
>  	int idx;
>  
>  	/* skip if we have nothing to show */
> -	if (!op->policy)
> -		return 0;
> -	if (!op->doit &&
> -	    (!op->dumpit || op->validate & GENL_DONT_VALIDATE_DUMP))
> +	if (!doit->policy && !dumpit->policy)
>  		return 0;
>  

We don't need to check the GENL_DONT_VALIDATE_DUMP because the previous
code for getting the split op checked this and set some other fields, right?

>  	hdr = ctrl_dumppolicy_prep(skb, cb);
> @@ -1341,21 +1339,26 @@ static int ctrl_dumppolicy_put_op(struct sk_buff *skb,
>  	if (!nest_pol)
>  		goto err;
>  
> -	nest_op = nla_nest_start(skb, op->cmd);
> +	nest_op = nla_nest_start(skb, doit->cmd);

We always use doit->cmd, but that shouldn't matter since the dump and
doit are always the same command here. Ok.

>  	if (!nest_op)
>  		goto err;
>  
> -	/* for now both do/dump are always the same */
> -	idx = netlink_policy_dump_get_policy_idx(ctx->state,
> -						 op->policy,
> -						 op->maxattr);
> +	if (doit->policy) {
> +		idx = netlink_policy_dump_get_policy_idx(ctx->state,
> +							 doit->policy,
> +							 doit->maxattr);
>  
> -	if (op->doit && nla_put_u32(skb, CTRL_ATTR_POLICY_DO, idx))
> -		goto err;
> +		if (nla_put_u32(skb, CTRL_ATTR_POLICY_DO, idx))
> +			goto err;
> +	}
> +	if (dumpit->policy) {
> +		idx = netlink_policy_dump_get_policy_idx(ctx->state,
> +							 dumpit->policy,
> +							 dumpit->maxattr);
>  
> -	if (op->dumpit && !(op->validate & GENL_DONT_VALIDATE_DUMP) &&
> -	    nla_put_u32(skb, CTRL_ATTR_POLICY_DUMP, idx))
> -		goto err;
> +		if (nla_put_u32(skb, CTRL_ATTR_POLICY_DUMP, idx))
> +			goto err;
> +	}
>  
>  	nla_nest_end(skb, nest_op);
>  	nla_nest_end(skb, nest_pol);
> @@ -1373,16 +1376,19 @@ static int ctrl_dumppolicy(struct sk_buff *skb, struct netlink_callback *cb)
>  	void *hdr;
>  
>  	if (!ctx->policies) {
> +		struct genl_split_ops doit, dumpit;
>  		struct genl_ops op;
>  
>  		if (ctx->single_op) {
> -			int err;
> -
> -			err = genl_get_cmd(ctx->op, ctx->rt, &op);
> -			if (WARN_ON(err))
> -				return err;
> +			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)) {
> +				WARN_ON(1);
> +				return -ENOENT;
> +			}
>  
> -			if (ctrl_dumppolicy_put_op(skb, cb, &op))
> +			if (ctrl_dumppolicy_put_op(skb, cb, &doit, &dumpit))
>  				return skb->len;
>  
>  			ctx->opidx = genl_get_cmd_cnt(ctx->rt);
> @@ -1391,7 +1397,12 @@ static int ctrl_dumppolicy(struct sk_buff *skb, struct netlink_callback *cb)
>  		while (ctx->opidx < genl_get_cmd_cnt(ctx->rt)) {
>  			genl_get_cmd_by_index(ctx->opidx, ctx->rt, &op);
>  
> -			if (ctrl_dumppolicy_put_op(skb, cb, &op))
> +			genl_cmd_full_to_split(&doit, ctx->rt,
> +					       &op, GENL_CMD_CAP_DO);
> +			genl_cmd_full_to_split(&dumpit, ctx->rt,
> +					       &op, GENL_CMD_CAP_DUMP);
> +
> +			if (ctrl_dumppolicy_put_op(skb, cb, &doit, &dumpit))
>  				return skb->len;
>  
>  			ctx->opidx++;