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]

Date:   Mon, 05 Oct 2020 21:21:36 +0200
From:   Johannes Berg <johannes@...solutions.net>
To:     Jakub Kicinski <kuba@...nel.org>
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, 2020-10-05 at 12:16 -0700, Jakub Kicinski wrote:
> 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.

Good point, I misread/misunderstood the code and thought it was still
being used to size the parsing array, but that's of course no longer
there since the genl core now does it.

> > 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];

Not sure I understand? You're using strict validation (I think), so
attrs that are out of range will be rejected same as NLA_REJECT (well,
with a different message) in __nla_validate_parse():

        nla_for_each_attr(nla, head, len, rem) {
                u16 type = nla_type(nla);

                if (type == 0 || type > maxtype) {
                        if (validate & NL_VALIDATE_MAXTYPE) {
                                NL_SET_ERR_MSG_ATTR(extack, nla,
                                                    "Unknown attribute type");
                                return -EINVAL;
                        }


In fact, if you're using strict validation even the default
(0==NLA_UNSPEC) will be rejected, just like NLA_REJECT.


Or am I confused somewhere?

johannes