ynl - mutiple policies for one nested attr used in multiple cmds

ynl - mutiple policies for one nested attr used in multiple cmds

[Jiri Pirko]

Hi Kuba.

I need to have one nested attribute but according to what cmd it is used
with, there will be different nested policy.

If I'm looking at the codes correctly, that is not currenly supported,
correct?

If not, why idea how to format this in yaml file?
Pehaps something like:

      -
        name: dump-selector
        type: nest
        nested-attributes: dl-dump-selector

  -
    name: dl-dev-dump-selector
    subset-of: devlink
    attributes:
      -
        name: bus-name
        type: string
      -
        name: dev-name
        type: string

  -
    name: dl-port-dump-selector
    subset-of: devlink
    attributes:
      -
        name: bus-name
        type: string
      -
        name: dev-name
        type: string
      -
        name: port-index
	type: u32

And then:
      dump:
        pre: devlink-nl-start
        post: devlink-nl-done
        request:
          attributes:
            - dump-selector@dl-dev-dump-selector
...
      dump:
        pre: devlink-nl-start
        post: devlink-nl-done
        request:
          attributes:
            - dump-selector@dl-port-dump-selector

Idk, looks a bit odd though to parse the attr strings.

Any idea?

Re: ynl - mutiple policies for one nested attr used in multiple cmds

[Jakub Kicinski]

On Fri, 4 Aug 2023 19:29:31 +0200 Jiri Pirko wrote:
> I need to have one nested attribute but according to what cmd it is used
> with, there will be different nested policy.
> 
> If I'm looking at the codes correctly, that is not currenly supported,
> correct?
> 
> If not, why idea how to format this in yaml file?

I'm not sure if you'll like it but my first choice would be to skip
the selector attribute. Put the attributes directly into the message.
There is no functional purpose the wrapping serves, right?

Re: ynl - mutiple policies for one nested attr used in multiple cmds

[Jiri Pirko]

Fri, Aug 04, 2023 at 09:58:16PM CEST, kuba@kernel.org wrote:
>On Fri, 4 Aug 2023 19:29:31 +0200 Jiri Pirko wrote:
>> I need to have one nested attribute but according to what cmd it is used
>> with, there will be different nested policy.
>> 
>> If I'm looking at the codes correctly, that is not currenly supported,
>> correct?
>> 
>> If not, why idea how to format this in yaml file?
>
>I'm not sure if you'll like it but my first choice would be to skip
>the selector attribute. Put the attributes directly into the message.
>There is no functional purpose the wrapping serves, right?

Well, the only reason is backward compatibility.
Currently, there is no attr parsing during dump, which is ensured by
GENL_DONT_VALIDATE_DUMP flag. That means if user passes any attr, it is
ignored.

Now if we allow attrs to select, previously ignored attributes would be
processed now. User that passed crap with old kernel can gen different
results with new kernel.

That is why I decided to add selector attr and put attrs inside, doing
strict parsing, so if selector attr is not supported by kernel, user
gets message back.

So what do you suggest? Do per-dump strict parsing policy of root
attributes serving to do selection?

Re: ynl - mutiple policies for one nested attr used in multiple cmds

[Jakub Kicinski]

On Sat, 5 Aug 2023 08:33:28 +0200 Jiri Pirko wrote:
> >I'm not sure if you'll like it but my first choice would be to skip
> >the selector attribute. Put the attributes directly into the message.
> >There is no functional purpose the wrapping serves, right?  
> 
> Well, the only reason is backward compatibility.
> Currently, there is no attr parsing during dump, which is ensured by
> GENL_DONT_VALIDATE_DUMP flag. That means if user passes any attr, it is
> ignored.
> 
> Now if we allow attrs to select, previously ignored attributes would be
> processed now. User that passed crap with old kernel can gen different
> results with new kernel.
> 
> That is why I decided to add selector attr and put attrs inside, doing
> strict parsing, so if selector attr is not supported by kernel, user
> gets message back.
> 
> So what do you suggest? Do per-dump strict parsing policy of root
> attributes serving to do selection?

Even the selector attr comes with a risk, right? Not only have we
ignored all attributes, previously, we ignored the payload of the
message. So the payload of a devlink dump request could be entirely
uninitialized / random and it would work.

IOW we are operating on a scale of potential breakage here, unless
we do something very heavy handed.

How does the situation look with the known user apps? Is anyone
that we know of putting attributes into dump requests?

Re: ynl - mutiple policies for one nested attr used in multiple cmds

[Jiri Pirko]

Mon, Aug 07, 2023 at 07:03:13PM CEST, kuba@kernel.org wrote:
>On Sat, 5 Aug 2023 08:33:28 +0200 Jiri Pirko wrote:
>> >I'm not sure if you'll like it but my first choice would be to skip
>> >the selector attribute. Put the attributes directly into the message.
>> >There is no functional purpose the wrapping serves, right?  
>> 
>> Well, the only reason is backward compatibility.
>> Currently, there is no attr parsing during dump, which is ensured by
>> GENL_DONT_VALIDATE_DUMP flag. That means if user passes any attr, it is
>> ignored.
>> 
>> Now if we allow attrs to select, previously ignored attributes would be
>> processed now. User that passed crap with old kernel can gen different
>> results with new kernel.
>> 
>> That is why I decided to add selector attr and put attrs inside, doing
>> strict parsing, so if selector attr is not supported by kernel, user
>> gets message back.
>> 
>> So what do you suggest? Do per-dump strict parsing policy of root
>> attributes serving to do selection?
>
>Even the selector attr comes with a risk, right? Not only have we

Yep, however, the odds are quite low. That's why I went that direction.


>ignored all attributes, previously, we ignored the payload of the
>message. So the payload of a devlink dump request could be entirely
>uninitialized / random and it would work.

Yep.

>
>IOW we are operating on a scale of potential breakage here, unless
>we do something very heavy handed.

True. I can easily imagine an app having one function to create both do
and dump message, putting in crap as bus_name/dev_name attrs
in case of dump.


>
>How does the situation look with the known user apps? Is anyone
>that we know of putting attributes into dump requests?

I'm not aware of that.

Re: ynl - mutiple policies for one nested attr used in multiple cmds

[Jakub Kicinski]

On Mon, 7 Aug 2023 19:12:30 +0200 Jiri Pirko wrote:
> >How does the situation look with the known user apps? Is anyone
> >that we know of putting attributes into dump requests?  
> 
> I'm not aware of that.

I vote to risk it and skip the nest, then :)

Re: ynl - mutiple policies for one nested attr used in multiple cmds

[Jiri Pirko]

Mon, Aug 07, 2023 at 07:24:06PM CEST, kuba@kernel.org wrote:
>On Mon, 7 Aug 2023 19:12:30 +0200 Jiri Pirko wrote:
>> >How does the situation look with the known user apps? Is anyone
>> >that we know of putting attributes into dump requests?  
>> 
>> I'm not aware of that.
>
>I vote to risk it and skip the nest, then :)

Okay, will implement strict parsing for dumps, treating attributes as
selectors. Cool.

Re: ynl - mutiple policies for one nested attr used in multiple cmds

[Jiri Pirko]

Fri, Aug 04, 2023 at 09:58:16PM CEST, kuba@kernel.org wrote:
>On Fri, 4 Aug 2023 19:29:31 +0200 Jiri Pirko wrote:
>> I need to have one nested attribute but according to what cmd it is used
>> with, there will be different nested policy.
>> 
>> If I'm looking at the codes correctly, that is not currenly supported,
>> correct?
>> 
>> If not, why idea how to format this in yaml file?
>
>I'm not sure if you'll like it but my first choice would be to skip
>the selector attribute. Put the attributes directly into the message.
>There is no functional purpose the wrapping serves, right?

I have another variation of a similar problem.
There might be a different policy for nested attribute for get and set.
Example nest: DEVLINK_ATTR_PORT_FUNCTION

Any suggestion how to resolve this?

Thanks!

Re: ynl - mutiple policies for one nested attr used in multiple cmds

[Jakub Kicinski]

On Fri, 18 Aug 2023 10:37:19 +0200 Jiri Pirko wrote:
> >I'm not sure if you'll like it but my first choice would be to skip
> >the selector attribute. Put the attributes directly into the message.
> >There is no functional purpose the wrapping serves, right?  
> 
> I have another variation of a similar problem.
> There might be a different policy for nested attribute for get and set.
> Example nest: DEVLINK_ATTR_PORT_FUNCTION
> 
> Any suggestion how to resolve this?

You mean something like:

GET:
 [NEST_X]
   [ATTR_A]
   [ATTR_B]

GET:
 [NEST_X]
   [ATTR_A]
   [ATTR_C]

Where ATTR_A, ATTR_B and ATTR_C are from the same set but depending 
on the command the nest can either contain A,C or A,B?

That can happen in legit ways :( I don't have a good solution for it.

Re: ynl - mutiple policies for one nested attr used in multiple cmds

[Jiri Pirko]

Fri, Aug 18, 2023 at 05:55:35PM CEST, kuba@kernel.org wrote:
>On Fri, 18 Aug 2023 10:37:19 +0200 Jiri Pirko wrote:
>> >I'm not sure if you'll like it but my first choice would be to skip
>> >the selector attribute. Put the attributes directly into the message.
>> >There is no functional purpose the wrapping serves, right?  
>> 
>> I have another variation of a similar problem.
>> There might be a different policy for nested attribute for get and set.
>> Example nest: DEVLINK_ATTR_PORT_FUNCTION
>> 
>> Any suggestion how to resolve this?
>
>You mean something like:
>
>GET:
> [NEST_X]
>   [ATTR_A]
>   [ATTR_B]
>
>GET:
> [NEST_X]
>   [ATTR_A]
>   [ATTR_C]
>
>Where ATTR_A, ATTR_B and ATTR_C are from the same set but depending 
>on the command the nest can either contain A,C or A,B?
>
>That can happen in legit ways :( I don't have a good solution for it.

More precisely, it is:
GET:
 [NEST_X]
   [ATTR_A]
   [ATTR_B]
   [ATTR_C]
   [ATTR_D]

SET:
 [NEST_X]
   [ATTR_A]
   [ATTR_C]

Okay, you don't have good solution, do you have at least the least bad
one? :)

Re: ynl - mutiple policies for one nested attr used in multiple cmds

[Jakub Kicinski]

On Fri, 18 Aug 2023 20:11:16 +0200 Jiri Pirko wrote:
> Okay, you don't have good solution, do you have at least the least bad
> one? :)

I was pondering this for the recent pp work:
https://lore.kernel.org/all/20230816234303.3786178-13-kuba@kernel.org/
search for NL_SET_ERR_MSG_ATTR.

I ended up hand-rejecting the attrs which I didn't want.
It's not great because the policy (netdev_page_pool_info_nl_policy)
is shared so if someone adds stuff there they'll need to know
to update all the rejects :[

I guess a better way to code up the same idea would be to check if tb[]
is NULL outside of expected attrs.

Option #2 is to not use the auto-generated policy, and write the policy
by hand in the kernel with the right members.

Option #3 is to add support for this to the YAML. With the existing
concepts we would have to redefine all levels as subsets, and then
we can override nested-attributes. A lot of typing. The YAML is really
just a slightly decorated version of the policy tables. The policy
tables in this case have to be separate.

Re: ynl - mutiple policies for one nested attr used in multiple cmds

[Jiri Pirko]

Fri, Aug 18, 2023 at 10:24:47PM CEST, kuba@kernel.org wrote:
>On Fri, 18 Aug 2023 20:11:16 +0200 Jiri Pirko wrote:
>> Okay, you don't have good solution, do you have at least the least bad
>> one? :)
>
>I was pondering this for the recent pp work:
>https://lore.kernel.org/all/20230816234303.3786178-13-kuba@kernel.org/
>search for NL_SET_ERR_MSG_ATTR.
>
>I ended up hand-rejecting the attrs which I didn't want.
>It's not great because the policy (netdev_page_pool_info_nl_policy)
>is shared so if someone adds stuff there they'll need to know
>to update all the rejects :[
>
>I guess a better way to code up the same idea would be to check if tb[]
>is NULL outside of expected attrs.

The problem is that with devlink, there no nostrict parsing. So the
like-to-be-ignored attrs if passed might error out during validation.

>
>Option #2 is to not use the auto-generated policy, and write the policy
>by hand in the kernel with the right members.

I'll go with this option for now I think.

>
>Option #3 is to add support for this to the YAML. With the existing
>concepts we would have to redefine all levels as subsets, and then
>we can override nested-attributes. A lot of typing. The YAML is really
>just a slightly decorated version of the policy tables. The policy
>tables in this case have to be separate.

Yeah. But eventually, I think this would be needed anyway to make yaml
to handle all the cases. Relying on the developer to do option #1 or #2
kinda defeats the inital yaml goal to avoid people mistakes, I think.