Re: [PATCH net-next v11 1/4] net netlink: Add new type NLA_BITFIELD_32

[Jiri Pirko <jiri@resnulli.us>]

Fri, Jul 28, 2017 at 04:19:06PM CEST, dsahern@gmail.com wrote:
>On 7/28/17 7:51 AM, Jamal Hadi Salim wrote:
>> On 17-07-25 10:41 AM, David Ahern wrote:
>>> On 7/23/17 7:35 PM, Jamal Hadi Salim wrote:
>>>> In the most basic form, the user specifies the attribute policy as:
>>>> [ATTR_GOO] = { .type = NLA_BITFIELD_32, .validation_data =
>>>> &myvalidflags },
>>>>
>>>> where myvalidflags is the bit mask of the flags the kernel understands.
>>>>
>>>> If the user _does not_ provide myvalidflags then the attribute will
>>>> also be rejected.
>>>
>>> No other netlink attribute has this requirement. 
>> 
>> This is the first one where we have to inspect content. We add things
>> when we need them - as in this case.
>
>Sure, the validation is required. My argument is that the validation
>should be done where other attributes are validated -- inline with its
>use. Nothing about this new bitfield says it must have a generic
>validation code.
>
>> 
>>> Users of the attributes
>>> are the only ones that know if a value is valid or not (e.g, attribute
>>> passing a device index) and those are always checked in line.
>> 
>> It doesnt make sense that every user of the API has to repeat that
>> validation code. Same principle as someone specifying that a type is
>> u32 and have the nla validation check it. At some point we never had
>> the u32 validation code. Then it was factored out because everyone
>> repeats the same boilerplate code.
>
>Every user of an attribute that uses a device index must verify the
>device index is valid. The same code is repeated over and over.

This is something different. You don't have NLA_IFINDEX. If you'd have it,
might make sense to do validation on Netlink level. Ofc this is highly
hypothetical. But in Jamal's case, there is indeed NLA_BITFIELD32 and
this attribute type itself assumes some format. Therefore the validation
on Netlink level makes sense here. At least that is how I feel it.


>
>Now you are suggesting to have 1 attribute whose content is validated by
>generic infra and the rest are validated inline by the code using it. I
>believe it is wrong and going to lead to problems.