Re: [PATCH net v1 1/2] netlink: let len field used to parse type-not-care nested attrs

[Jakub Kicinski]

On Tue, 1 Aug 2023 10:00:01 +0800 (GMT+08:00) Lin Ma wrote:
> > > However, this is tedious and just like Leon said: add another layer of
> > > cabal knowledge. The better solution should leverage the nla_policy and
> > > discard nlattr whose length is invalid when doing parsing. That is, we
> > > should defined a nested_policy for the X above like  
> > 
> > Hard no. Putting array index into attr type is an advanced case and the
> > parsing code has to be able to deal with low level netlink details.  
> 
> Well, I just known that the type field for those attributes is used as array
> index.
> Hence, for this advanced case, could we define another NLA type, maybe 
> NLA_NESTED_IDXARRAY enum? That may be much clearer against modifying existing
> code.

What if the value is of a complex type (nest)?  For 10th time, if
someone does "interesting things" they must know what they're doing.

> > Higher level API should remove the nla_for_each_nested() completely
> > which is rather hard to achieve here.  
> 
> By investigating the code uses nla_for_each_nested macro. There are basically
> two scenarios:
> 
> 1. manually parse nested attributes whose type is not cared (the advance case
>    use type as index here).
> 2. manually parse nested attributes for *one* specific type. Such code do
>    nla_type check.
> 
> From the API side, to completely remove nla_for_each_nested and avoid the
> manual  parsing. I think we can choose two solutions.
> 
> Solution-1: add a parsing helper that receives a function pointer as an
>             argument, it will call this pointer after carefully verify the
>             type and length of an attribute.
> 
> Solution-2: add a parsing helper that traverses this nested twice, the first
>             time  to do counting size for allocating heap buffer (or stack
>             buffer from the caller if the max count is known). The second
>             time to fill this buffer with attribute pointers.
> 
> Which one is preferred? Please enlighten me about this and I can try to propose
> a fix. (I personally like the solution-2 as it works like the existing parsers
> like nla_parse) 

Your initial fix was perfectly fine.

We do need a solution for a normal multi-attr parse, but that's not 
the case here.