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: <20201005123120.7e8caa84@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Mon, 5 Oct 2020 12:31:20 -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 21:21:36 +0200 Johannes Berg wrote:
> > > 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?

Yea, I think we're both confused. Agreed with the above.

Are you suggesting:

const struct nla_policy policy[/* no size */] = {
	[HEADER]	= NLA_POLICY(...)
	[OTHER_ATTR]	= NLA_POLICY(...)
};

extern const struct nla_policy policy[/* no size */];

op = {
	.policy = policy,
	.max_attr = OTHER_ATTR,
}

?

What I'm saying is that my preference would be:

const struct nla_policy policy[OTHER_ATTR + 1] = {
	[HEADER]	= NLA_POLICY(...)
	[OTHER_ATTR]	= NLA_POLICY(...)
};

extern const struct nla_policy policy[OTHER_ATTR + 1];

op = {
	.policy = policy,
	.max_attr = ARRAY_SIZE(policy) - 1,
}

Since it's harder to forget to update the op (you don't have to update
op, and compiler will complain about the extern out of sync).