netdev - Re: [PATCH net-next 10/13] genetlink: use iterator in the op to policy map dumping

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: <c22b7ce0-a1a2-f9b1-b878-bdddb948d687@intel.com>
Date:   Wed, 19 Oct 2022 14:53:11 -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 10/13] genetlink: use iterator in the op to
 policy map dumping



On 10/18/2022 4:07 PM, Jakub Kicinski wrote:
> We can't put the full iterator in the struct ctrl_dump_policy_ctx
> because dump context is statically sized by netlink core.
> Allocate it dynamically.
> 
> Rename policy to dump_map to make the logic a little easier to follow.
> 
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ---
>  net/netlink/genetlink.c | 48 ++++++++++++++++++++++-------------------
>  1 file changed, 26 insertions(+), 22 deletions(-)
> 
> diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
> index c5fcb7b9c383..63807204805a 100644
> --- a/net/netlink/genetlink.c
> +++ b/net/netlink/genetlink.c
> @@ -1225,10 +1225,10 @@ static int genl_ctrl_event(int event, const struct genl_family *family,
>  struct ctrl_dump_policy_ctx {
>  	struct netlink_policy_dump_state *state;
>  	const struct genl_family *rt;
> -	unsigned int opidx;
> +	struct genl_op_iter *op_iter;
>  	u32 op;
>  	u16 fam_id;
> -	u8 policies:1,
> +	u8 dump_map:1,
>  	   single_op:1;
>  };

It might be easier to review this if the change to dump_map was done
separately, but I can understand the difficulty in splitting this.

Overall I think this makes sense.

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

>  
> @@ -1298,9 +1298,16 @@ static int ctrl_dumppolicy_start(struct netlink_callback *cb)
>  
>  		if (!ctx->state)
>  			return -ENODATA;
> +
> +		ctx->dump_map = 1;
>  		return 0;
>  	}
>  
> +	ctx->op_iter = kmalloc(sizeof(*ctx->op_iter), GFP_KERNEL);
> +	if (!ctx->op_iter)
> +		return -ENOMEM;
> +	ctx->dump_map = genl_op_iter_init(rt, ctx->op_iter);
> +
>  	for (genl_op_iter_init(rt, &i); genl_op_iter_next(&i); ) {
>  		if (i.doit.policy) {
>  			err = netlink_policy_dump_add_policy(&ctx->state,
> @@ -1318,12 +1325,16 @@ static int ctrl_dumppolicy_start(struct netlink_callback *cb)
>  		}
>  	}
>  
> -	if (!ctx->state)
> -		return -ENODATA;
> +	if (!ctx->state) {
> +		err = -ENODATA;
> +		goto err_free_op_iter;
> +	}
>  	return 0;
>  
>  err_free_state:
>  	netlink_policy_dump_free(ctx->state);
> +err_free_op_iter:
> +	kfree(ctx->op_iter);
>  	return err;
>  }
>  
> @@ -1403,11 +1414,10 @@ static int ctrl_dumppolicy(struct sk_buff *skb, struct netlink_callback *cb)
>  	struct ctrl_dump_policy_ctx *ctx = (void *)cb->ctx;
>  	void *hdr;
>  
> -	if (!ctx->policies) {
> -		struct genl_split_ops doit, dumpit;
> -		struct genl_ops op;
> -
> +	if (ctx->dump_map) {
>  		if (ctx->single_op) {
> +			struct genl_split_ops doit, dumpit;
> +
>  			if (genl_get_cmd(ctx->op, GENL_CMD_CAP_DO,
>  					 ctx->rt, &doit) &&
>  			    genl_get_cmd(ctx->op, GENL_CMD_CAP_DUMP,
> @@ -1419,25 +1429,18 @@ static int ctrl_dumppolicy(struct sk_buff *skb, struct netlink_callback *cb)
>  			if (ctrl_dumppolicy_put_op(skb, cb, &doit, &dumpit))
>  				return skb->len;
>  
> -			ctx->opidx = genl_get_cmd_cnt(ctx->rt);
> +			/* done with the per-op policy index list */
> +			ctx->dump_map = 0;
>  		}
>  
> -		while (ctx->opidx < genl_get_cmd_cnt(ctx->rt)) {
> -			genl_get_cmd_by_index(ctx->opidx, ctx->rt, &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))
> +		while (ctx->dump_map) {
> +			if (ctrl_dumppolicy_put_op(skb, cb,
> +						   &ctx->op_iter->doit,
> +						   &ctx->op_iter->dumpit))
>  				return skb->len;
>  
> -			ctx->opidx++;
> +			ctx->dump_map = genl_op_iter_next(ctx->op_iter);
>  		}
> -
> -		/* completed with the per-op policy index list */
> -		ctx->policies = true;
>  	}
>  
>  	while (netlink_policy_dump_loop(ctx->state)) {
> @@ -1470,6 +1473,7 @@ static int ctrl_dumppolicy_done(struct netlink_callback *cb)
>  {
>  	struct ctrl_dump_policy_ctx *ctx = (void *)cb->ctx;
>  
> +	kfree(ctx->op_iter);
>  	netlink_policy_dump_free(ctx->state);
>  	return 0;
>  }