[RFC,iproute2,0/6] devlink: add policy check for all attributes

Message

Keller, Jacob E Aug. 5, 2022, 11:41 p.m. UTC

This series implements code to check the kernel policy for the devlink
commands to determine whether or not attributes are supported before adding
them to netlink messages.

It implements a new mnlu_gen_get_op_policy to extract the policy
information, and uses it to implement checks when parsing option arguments.
This is intended to eventually go along with improvements to the policy
reporting in devlink kernel code to report separate policy for each command.

I think checking every attribute makes sense and is easier to follow than
only checking specific attributes. This will help ensure that future
attributes don't accidentally get sent to commands when they aren't
supported (once the devlink kernel policy is improved to report correct
information for each command separately).

Cc: Jacob Keller <[e-mail]>
Cc: Jonathan Corbet <[e-mail]>
Cc: Jiri Pirko <[e-mail]>
Cc: "David S. Miller" <[e-mail]>
Cc: Eric Dumazet <[e-mail]>
Cc: Jakub Kicinski <[e-mail]>
Cc: Paolo Abeni <[e-mail]>
Cc: David Ahern <[e-mail]>
Cc: Stephen Hemminger <[e-mail]>
Cc: [e-mail]

Jacob Keller (6):
  mnlg: remove unnused mnlg_socket structure
  utils: extract CTRL_ATTR_MAXATTR and save it
  mnl_utils: add function to dump command policy
  devlink: use dl_no_arg instead of checking dl_argc == 0
  devlink: remove dl_argv_parse_put
  devlink: check attributes against policy

 devlink/devlink.c   | 846 ++++++++++++++++++++++++++++++--------------
 devlink/mnlg.c      |   8 -
 include/mnl_utils.h |  28 ++
 lib/mnl_utils.c     | 258 +++++++++++++-
 4 files changed, 858 insertions(+), 282 deletions(-)

Comments

Jiri Pirko Aug. 8, 2022, 10:32 a.m. UTC | #1

Sat, Aug 06, 2022 at 01:41:49AM CEST, [e-mail] wrote:


[...]

>This is intended to eventually go along with improvements to the policy
>reporting in devlink kernel code to report separate policy for each command.

Can you explain this a bit please?

Keller, Jacob E Aug. 8, 2022, 5:02 p.m. UTC | #2

> -----Original Message-----
> From: Jiri Pirko <[e-mail]>
> Sent: Monday, August 08, 2022 3:32 AM
> To: Keller, Jacob E <[e-mail]>
> Cc: [e-mail]; Jonathan Corbet <[e-mail]>; David S. Miller
> <[e-mail]>; Eric Dumazet <[e-mail]>; Jakub Kicinski
> <[e-mail]>; Paolo Abeni <[e-mail]>; David Ahern
> <[e-mail]>; Stephen Hemminger <[e-mail]>
> Subject: Re: [RFC iproute2 0/6] devlink: add policy check for all attributes
> 
> Sat, Aug 06, 2022 at 01:41:49AM CEST, [e-mail] wrote:
> 
> 
> [...]
> 
> >This is intended to eventually go along with improvements to the policy
> >reporting in devlink kernel code to report separate policy for each command.
> 
> Can you explain this a bit please?

Currently devlink only reports a global policy which is the same for every command. This means that commands like DEVLINK_CMD_FLASH report the same attributes as valid as other commands like DEVLINK_CMD_INFO_GET. The policy (if the command is strict) would only require that attributes be one of the known attributes according to the general devlink policy.

However, none of the commands accept or honor all the attributes. The netlink policy support allows each command to report an individual policy that would only include the attributes which the command uses and would honor the meaning of.

Without per-command policy, there is no way for user space to tell when the kernel changed support for some attribute (such as the DEVLINK_ATTR_DRY_RUN I recently proposed). Yes, you can use maxattr to determine of the kernel knows about the attribute, but that doesn't guarantee that every command supports it.

The per-command policy would report only the subset of attributes supported by the command. In strict validation, this would also make the kernel reject commands which tried to send other attributes. Ideally we would also have nested attribute policy, so each nested attribute would express the policy for the subset of attributes which are valid within that nest as well.

By adding policy checking support to user space, we can make sure that at least iproute2 userspace won't accidentally send an unsupported attribute to a command, but that only works once the policy for the command in the kernel is updated to list the specific policy. Right now, this will effectively get the generic policy and indicate that all known attributes in the policy table are accepted.

Note that this means strict validation on its own is not enough.  It doesn't really matter if a command is set to strict validation when the validation still allows every attribute in the general policy, regardless of whether the command currently does anything with the attribute. Any of the unsupported ones get silently ignored, with no warning to the user.

Related to this, also think we should determine a plan for how to migrate devlink to strict validation, once the per-command policy is defined and implemented. However, I am not sure what the backwards-compatibility issues exist for switching.

Hope this explains things,
Jake

Jiri Pirko Aug. 9, 2022, 7:07 a.m. UTC | #3

Mon, Aug 08, 2022 at 07:02:49PM CEST, [e-mail] wrote:
>
>
>> -----Original Message-----
>> From: Jiri Pirko <[e-mail]>
>> Sent: Monday, August 08, 2022 3:32 AM
>> To: Keller, Jacob E <[e-mail]>
>> Cc: [e-mail]; Jonathan Corbet <[e-mail]>; David S. Miller
>> <[e-mail]>; Eric Dumazet <[e-mail]>; Jakub Kicinski
>> <[e-mail]>; Paolo Abeni <[e-mail]>; David Ahern
>> <[e-mail]>; Stephen Hemminger <[e-mail]>
>> Subject: Re: [RFC iproute2 0/6] devlink: add policy check for all attributes
>> 
>> Sat, Aug 06, 2022 at 01:41:49AM CEST, [e-mail] wrote:
>> 
>> 
>> [...]
>> 
>> >This is intended to eventually go along with improvements to the policy
>> >reporting in devlink kernel code to report separate policy for each command.
>> 
>> Can you explain this a bit please?
>
>Currently devlink only reports a global policy which is the same for every command. This means that commands like DEVLINK_CMD_FLASH report the same attributes as valid as other commands like DEVLINK_CMD_INFO_GET. The policy (if the command is strict) would only require that attributes be one of the known attributes according to the general devlink policy.
>
>However, none of the commands accept or honor all the attributes. The netlink policy support allows each command to report an individual policy that would only include the attributes which the command uses and would honor the meaning of.
>
>Without per-command policy, there is no way for user space to tell when the kernel changed support for some attribute (such as the DEVLINK_ATTR_DRY_RUN I recently proposed). Yes, you can use maxattr to determine of the kernel knows about the attribute, but that doesn't guarantee that every command supports it.
>
>The per-command policy would report only the subset of attributes supported by the command. In strict validation, this would also make the kernel reject commands which tried to send other attributes. Ideally we would also have nested attribute policy, so each nested attribute would express the policy for the subset of attributes which are valid within that nest as well.
>
>By adding policy checking support to user space, we can make sure that at least iproute2 userspace won't accidentally send an unsupported attribute to a command, but that only works once the policy for the command in the kernel is updated to list the specific policy. Right now, this will effectively get the generic policy and indicate that all known attributes in the policy table are accepted.
>
>Note that this means strict validation on its own is not enough.  It doesn't really matter if a command is set to strict validation when the validation still allows every attribute in the general policy, regardless of whether the command currently does anything with the attribute. Any of the unsupported ones get silently ignored, with no warning to the user.
>
>Related to this, also think we should determine a plan for how to migrate devlink to strict validation, once the per-command policy is defined and implemented. However, I am not sure what the backwards-compatibility issues exist for switching.
>
>Hope this explains things,

It is, thanks.

Do you have patches for the per-cmd policy?


>Jake

Jiri Pirko Aug. 9, 2022, 9:50 a.m. UTC | #4

Sat, Aug 06, 2022 at 01:41:49AM CEST, [e-mail] wrote:
>This series implements code to check the kernel policy for the devlink
>commands to determine whether or not attributes are supported before adding
>them to netlink messages.
>
>It implements a new mnlu_gen_get_op_policy to extract the policy
>information, and uses it to implement checks when parsing option arguments.
>This is intended to eventually go along with improvements to the policy
>reporting in devlink kernel code to report separate policy for each command.
>
>I think checking every attribute makes sense and is easier to follow than
>only checking specific attributes. This will help ensure that future
>attributes don't accidentally get sent to commands when they aren't
>supported (once the devlink kernel policy is improved to report correct
>information for each command separately).

This patchset looks good to me. Thanks!