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: <20201001080426.vh4eccbylwsyimde@lion.mk-sys.cz>
Date:   Thu, 1 Oct 2020 10:04:26 +0200
From:   Michal Kubecek <mkubecek@...e.cz>
To:     Johannes Berg <johannes@...solutions.net>
Cc:     Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        andrew@...n.ch, jiri@...nulli.us, dsahern@...nel.org,
        pablo@...filter.org
Subject: Re: [RFC net-next 5/9] genetlink: add a structure for dump state

On Thu, Oct 01, 2020 at 09:48:31AM +0200, Johannes Berg wrote:
> 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.

Definitely. And it should rather be cb->ctx than cb->args as on 32-bit
architectures, cb->args is technically only 24 bytes while cb->ctx is
always 48 bytes long. After all, the comment in the structure definition
says args is deprecated.

Michal