netdev - Re: [RFC net-next 5/9] genetlink: add a structure for dump state

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: <f035af6008cdca32c84f13bc3f38614fa0b535ac.camel@sipsolutions.net>
Date:   Thu, 01 Oct 2020 09:48:31 +0200
From:   Johannes Berg <johannes@...solutions.net>
To:     Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org
Cc:     andrew@...n.ch, jiri@...nulli.us, mkubecek@...e.cz,
        dsahern@...nel.org, pablo@...filter.org
Subject: Re: [RFC net-next 5/9] genetlink: add a structure for dump state

On Wed, 2020-09-30 at 17:05 -0700, Jakub Kicinski wrote:
> Whenever netlink dump uses more than 2 cb->args[] entries
> code gets hard to read. We're about to add more state to
> ctrl_dumppolicy() so create a structure.
> 
> Since the structure is typed and clearly named we can remove
> the local fam_id variable and use ctx->fam_id directly.
> 
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ---
>  net/netlink/genetlink.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
> index 38d8f353dba1..a8001044d8cd 100644
> --- a/net/netlink/genetlink.c
> +++ b/net/netlink/genetlink.c
> @@ -1102,13 +1102,18 @@ static int genl_ctrl_event(int event, const struct genl_family *family,
>  	return 0;
>  }
>  
> +struct ctrl_dump_policy_ctx {
> +	unsigned long state;

Maybe if we do this, also make a "struct netlink_policy_dump_state" in
include/net/netlink.h for the policy dump to use as a state? Right now
it just uses an "unsigned long *state" there.

I feel that would more clearly show what this "state" actually is.

Alternatively, perhaps just rename it to "policy_dump_state"? Yeah,
that's longer, but at least would be very obvious?

> +	unsigned int fam_id;

You could make this a u16 I guess, but it doesn't really matter.

>  static int ctrl_dumppolicy(struct sk_buff *skb, struct netlink_callback *cb)
>  {
> +	struct ctrl_dump_policy_ctx *ctx = (void *)cb->args;


I'd also prefer if you stuck a

	BUILD_BUG_ON(sizeof(*ctx) > sizeof(cb->args));

here. It's not likely we'll need so much more state here, but would
still be good to check IMHO.

But in general looks good :)

johannes