Message-ID: <m2jznuwv7g.fsf@gmail.com>
Date: Sat, 27 Jan 2024 17:18:59 +0000
From: Donald Hunter <donald.hunter@...il.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: netdev@...r.kernel.org,  "David S. Miller" <davem@...emloft.net>,  Eric
 Dumazet <edumazet@...gle.com>,  Paolo Abeni <pabeni@...hat.com>,  Jonathan
 Corbet <corbet@....net>,  linux-doc@...r.kernel.org,  Jacob Keller
 <jacob.e.keller@...el.com>,  Breno Leitao <leitao@...ian.org>,  Jiri Pirko
 <jiri@...nulli.us>,  Alessandro Marcolini
 <alessandromarcolini99@...il.com>,  donald.hunter@...hat.com
Subject: Re: [PATCH net-next v1 02/12] tools/net/ynl: Support sub-messages
 in nested attribute spaces

Jakub Kicinski <kuba@...nel.org> writes:

> On Fri, 26 Jan 2024 12:44:57 +0000 Donald Hunter wrote:
>> I was quite pleased with how simple the patch turned out to be when I
>> used ChainMap, but it does have this weakness.
>
> It is very neat, no question about it :(
>
>> In practice, the only place this could be a problem is with
>> tc-act-attrs which has the same attribute name 'kind' in the nest and
>> in tc-attrs at the top level. If you send a create message with ynl,
>> you could omit the 'kind' attr in the 'act' nest and ynl would
>> incorrectly resolve to the top level 'kind'. The kernel would reject
>> the action with a missing 'kind' but the rest of payload would be
>> encoded wrongly and/or could break ynl.
>
> We can detect the problem post-fact and throw an exception. I primarily
> care about removing the ambiguity.

Agreed.

> Is it possible to check at which "level" of the chainmap the key was
> found? If so we can also construct a 'chainmap of attr sets' and make
> sure that the key level == attr set level. I.e. that we got a hit at
> the first level which declares a key of that name.
>
> More crude option - we could construct a list of dicts (the levels
> within the chainmap) and keys they can't contain. Once we got a hit
> for a sub-message key at level A, all dicts currently on top of A
> are not allowed to add that key. Once we're done with the message we
> scan thru the list and make sure the keys haven't appeared?
>
> Another random thought, should we mark the keys which can "descend"
> somehow? IDK, put a ~ in front?
>
> 	selector: ~kind
>
> or some other char?

Okay, so I think the behaviour we need is to either search current scope
or search the outermost scope. My suggestion would be to replace the
ChainMap approach with just choosing between current and outermost
scope. The unusual case is needing to search the outermost scope so
using a prefix e.g. '/' for that would work.

We can have 'selector: kind' continue to refer to current scope and then
have 'selector: /kind' refer to the outermost scope.

If we run into a case that requires something other than current or
outermost then we could add e.g. '../kind' so that the scope to search
is always explicitly identified.

>> My initial thought is that this might be better handled as input
>> validation, e.g. adding 'required: true' to the spec for 'act/kind'.
>> After using ynl for a while, I think it would help to specify required
>> attributes for messages, nests and sub-messsages. It's very hard to
>> discover the required attributes for families that don't provide
>> extack responses for errors.
>
> Hah, required attrs. I have been sitting on patches for the kernel for
> over a year - https://github.com/kuba-moo/linux/tree/req-args
> Not sure if they actually work but for the kernel I was curious if it's
> possible to do the validation in constant time (in relation to the
> policy size, i.e. without scanning the entire policy at the end to
> confirm that all required attrs are present). And that's what I came up
> with.

Interesting. It's definitely a thorny problem with varying sets of
'required' attributes. It could be useful to report the absolutely
required attributes in policy responses, without any actual enforcement.
Would it be possible to report policy for legacy netlink-raw families?

Thinking about it, usability would probably be most improved by adding
extack messages to more of the tc error paths.

> I haven't posted it because I was a tiny bit worried that required args
> will cause bugs (people forgetting to null check attrs) and may cause
> uAPI breakage down the line (we should clearly state that "required"
> status is just advisory, and can go away in future kernel release).
> But that was more of a on-the-fence situation. If you find them useful
> feel free to move forward!
>
> I do think that's a separate story, tho. For sub-message selector
> - isn't the key _implicitly_ required, in the first attr set where
> it is defined? Conversely if the sub-message isn't present the key
> isn't required any more either?

Yes, the key is implicitly required for sub-messages. The toplevel key
is probably required regardless of the presence of a sub-message.

Powered by blists - more mailing lists