netdev - Re: [PATCH net-next 1/6] ethtool: wire up get policies to ops

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: <20201005121622.55607210@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Mon, 5 Oct 2020 12:16:22 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Johannes Berg <johannes@...solutions.net>
Cc:     davem@...emloft.net, netdev@...r.kernel.org, kernel-team@...com,
        jiri@...nulli.us, andrew@...n.ch, mkubecek@...e.cz
Subject: Re: [PATCH net-next 1/6] ethtool: wire up get policies to ops

On Mon, 05 Oct 2020 20:56:29 +0200 Johannes Berg wrote:
> On Mon, 2020-10-05 at 08:57 -0700, Jakub Kicinski wrote:
> > @@ -783,6 +799,9 @@ static const struct genl_ops ethtool_genl_ops[] = {
> >  		.start	= ethnl_default_start,
> >  		.dumpit	= ethnl_default_dumpit,
> >  		.done	= ethnl_default_done,
> > +		.policy = ethnl_rings_get_policy,
> > +		.maxattr = ARRAY_SIZE(ethnl_rings_get_policy) - 1,
> > +
> >  	},  
> 
> If you find some other reason to respin, perhaps remove that blank line
> :)
> 
> Unrelated to that, it bothers me a bit that you put here the maxattr as
> the ARRAY_SIZE(), which is of course fine, but then still have
> 
> > @@ -127,7 +127,7 @@ const struct ethnl_request_ops ethnl_privflags_request_ops = {
> >  	.max_attr		= ETHTOOL_A_PRIVFLAGS_MAX,  
> 
> max_attr here, using the original define

Ah, another good catch, this is obviously no longer needed. I will
remove those members in v2.

> yes, mostly the policies use
> the define to size them, but they didn't really *need* to, and one might
> make an argument that on the policy arrays the size might as well be
> removed (and it be sized automatically based on the contents) since all
> the unspecified attrs are rejected anyway.
> 
> But with the difference it seems to me that it'd be possible to get this
> mixed up?

Right, I prefer not to have the unnecessary NLA_REJECTS, so my thinking
was - use the format I like for the new code, but leave the existing
rejects for a separate series / discussion.

If we remove the rejects we still need something like

extern struct nla_policy policy[lastattr + 1];

For array_size to work, but I think that's fine. And we'd get a
compiler errors if the sizes don't match up.

> I do see that you still need this to size the attrs for parsing them
> even after patch 2 where this:
> 
> >  	.req_info_size		= sizeof(struct privflags_req_info),
> >  	.reply_data_size	= sizeof(struct privflags_reply_data),
> > -	.request_policy		= privflags_get_policy,
> > +	.request_policy		= ethnl_privflags_get_policy,  
> 
> gets removed completely.
> 
> 
> Perhaps we can look up the genl_ops pointer, or add the ops pointer to
> struct genl_info (could point to the temporary full struct that gets
> populated, size of genl_info itself doesn't matter much since it's on
> the stack and temporary), and then use ops->maxattr instead of
> request_ops->max_attr in ethnl_default_parse()?

Hm, maybe my split of patches 1 and 2 hurts more than it helps.
Let me merge the two in v2.