netdev - Re: [PATCH net-next 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]

Message-ID: <68eaa8749d8ad971b34cce82f4306e77ccccbf3a.camel@sipsolutions.net>
Date:   Wed, 19 Oct 2022 09:50:16 +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
Subject: Re: [PATCH net-next 01/13] genetlink: refactor the cmd <> policy
 mapping dump

On Tue, 2022-10-18 at 16:07 -0700, 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.

Hmm. Yeah, fair, it's nicer now :)

However,

>  	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;
> +
> +			ctx->opidx = genl_get_cmd_cnt(ctx->rt);

This (now without a comment that you removed rather than changed), still
strikes me as odd.

I guess if we add a comment /* don't enter the loop below */ that'd be
nicer, but I feel maybe putting the loop into the else instead would be
nicer?


johannes