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: <f5fdf3bf-a30f-50eb-c9d1-7f3776af1391@intel.com>
Date:   Mon, 5 Oct 2020 14:33:41 -0700
From:   Jacob Keller <jacob.e.keller@...el.com>
To:     Jakub Kicinski <kuba@...nel.org>,
        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 10/5/2020 12:31 PM, Jakub Kicinski wrote:
> 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,
> }
> 

Why can't .max_attr here just be derived from ARRAY_SIZE? In this
example, ARRAY_SIZE should return 2, so max_attr would be ARRAY_SIZE - 1.

Well, I guess HEADER/OTHER_ATTR could make this a sparse array... in
which case it might not work?

> ?
> 
> 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).
> 
Ahh.. Ok so I guess what you're doing here is defining the array size as
OTHER_ATTR + 1, so that ARRAY_SIZE() - 1 guarantees to equal
OTHER_ATTR.. so as long as OTHER_ATTR is the largest element in the array...

But wouldn't that be the case in the previous example where array size
is automatically determined... as long as you keep the index ordering in
ascending order, then the size of the array would be 1 larger than the
last element...