netdev - Re: [PATCH net-next v2 01/13] genetlink: refactor the cmd <> policy mapping dump

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, 2 Nov 2022 16:52:21 -0700
From:   Jacob Keller <jacob.e.keller@...el.com>
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>, <fw@...len.de>
Subject: Re: [PATCH net-next v2 01/13] genetlink: refactor the cmd <> policy
 mapping dump



On 11/2/2022 2:33 PM, Jakub Kicinski wrote:
> The code at the top of ctrl_dumppolicy() dumps mappings between
> ops and policies. It supports dumping both the entire family and
> single op if dump is filtered. But both of those cases are handled
> inside a loop, which makes the logic harder to follow and change.
> Refactor to split the two cases more clearly.
> 
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ---
> v2: bring the comment back
> ---
>   net/netlink/genetlink.c | 27 +++++++++++++++------------
>   1 file changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
> index 3e16527beb91..0a7a856e9ce0 100644
> --- a/net/netlink/genetlink.c
> +++ b/net/netlink/genetlink.c
> @@ -1319,21 +1319,24 @@ static int ctrl_dumppolicy(struct sk_buff *skb, struct netlink_callback *cb)
>   	void *hdr;
>   
>   	if (!ctx->policies) {
> -		while (ctx->opidx < genl_get_cmd_cnt(ctx->rt)) {
> -			struct genl_ops op;
> +		struct genl_ops op;
>   
> -			if (ctx->single_op) {
> -				int err;
> +		if (ctx->single_op) {
> +			int err;
>   
> -				err = genl_get_cmd(ctx->op, ctx->rt, &op);
> -				if (WARN_ON(err))
> -					return skb->len;
> +			err = genl_get_cmd(ctx->op, ctx->rt, &op);
> +			if (WARN_ON(err))
> +				return err;
>   
> -				/* break out of the loop after this one */
> -				ctx->opidx = genl_get_cmd_cnt(ctx->rt);
> -			} else {
> -				genl_get_cmd_by_index(ctx->opidx, ctx->rt, &op);
> -			}
> +			if (ctrl_dumppolicy_put_op(skb, cb, &op))
> +				return skb->len;
> +
> +			/* don't enter the loop below */
> +			ctx->opidx = genl_get_cmd_cnt(ctx->rt);
> +		}
> +
> +		while (ctx->opidx < genl_get_cmd_cnt(ctx->rt)) {
> +

Does the change to ctx->opidx have any other side effects we care about? 
if not it might be more legible to write this as:

/* don't modify ctx->opidx */
}

while (!ctx->single_op && ctx->opidx < genl_get_cmd_cnt(ctx->r)) {


That makes the intent a bit more clear and shouldn't need a comment 
about entering the loop. It also means we don't need to modify 
ctx->opidx, though I'm not sure if those other side effects matter or 
not.. we were modifying it before..

I don't know what else depends on the opidx.

Thanks,
Jake
			genl_get_cmd_by_index(ctx->opidx, ctx->rt, &op);
>   
>   			if (ctrl_dumppolicy_put_op(skb, cb, &op))
>   				return skb->len;