Re: [PATCH net-next v7 05/10] net: openvswitch: add psample action

[Michal Kubiak]

On Sun, Jun 30, 2024 at 09:57:26PM +0200, Adrian Moreno wrote:
> Add support for a new action: psample.
> 
> This action accepts a u32 group id and a variable-length cookie and uses
> the psample multicast group to make the packet available for
> observability.
> 
> The maximum length of the user-defined cookie is set to 16, same as
> tc_cookie, to discourage using cookies that will not be offloadable.
> 
> Acked-by: Eelco Chaudron <echaudro@xxxxxxxxxx>
> Signed-off-by: Adrian Moreno <amorenoz@xxxxxxxxxx>
> ---
>  Documentation/netlink/specs/ovs_flow.yaml | 17 ++++++++
>  include/uapi/linux/openvswitch.h          | 28 ++++++++++++++
>  net/openvswitch/Kconfig                   |  1 +
>  net/openvswitch/actions.c                 | 47 +++++++++++++++++++++++
>  net/openvswitch/flow_netlink.c            | 32 ++++++++++++++-
>  5 files changed, 124 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/netlink/specs/ovs_flow.yaml b/Documentation/netlink/specs/ovs_flow.yaml
> index 4fdfc6b5cae9..46f5d1cd8a5f 100644
> --- a/Documentation/netlink/specs/ovs_flow.yaml
> +++ b/Documentation/netlink/specs/ovs_flow.yaml
> @@ -727,6 +727,12 @@ attribute-sets:
>          name: dec-ttl
>          type: nest
>          nested-attributes: dec-ttl-attrs
> +      -
> +        name: psample
> +        type: nest
> +        nested-attributes: psample-attrs
> +        doc: |
> +          Sends a packet sample to psample for external observation.
>    -
>      name: tunnel-key-attrs
>      enum-name: ovs-tunnel-key-attr
> @@ -938,6 +944,17 @@ attribute-sets:
>        -
>          name: gbp
>          type: u32
> +  -
> +    name: psample-attrs
> +    enum-name: ovs-psample-attr
> +    name-prefix: ovs-psample-attr-
> +    attributes:
> +      -
> +        name: group
> +        type: u32
> +      -
> +        name: cookie
> +        type: binary
>  
>  operations:
>    name-prefix: ovs-flow-cmd-
> diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
> index efc82c318fa2..3dd653748725 100644
> --- a/include/uapi/linux/openvswitch.h
> +++ b/include/uapi/linux/openvswitch.h
> @@ -914,6 +914,31 @@ struct check_pkt_len_arg {
>  };
>  #endif
>  
> +#define OVS_PSAMPLE_COOKIE_MAX_SIZE 16

In your patch #2 you use "TC_COOKIE_MAX_SIZE" as an array size for your
cookie. I know that now OVS_PSAMPLE_COOKIE_MAX_SIZE == TC_COOKIE_MAX_SIZE,
so this size will be validated correctly.
But how likely is that those 2 constants will have different values in the
future?
Would it be reasonable to create more strict dependency between those
macros, e.g.:

#define OVS_PSAMPLE_COOKIE_MAX_SIZE TC_COOKIE_MAX_SIZE

or, at least, add a comment that the size shouldn't be bigger than
TC_COOKIE_MAX_SIZE?
I'm just considering the risk of exceeding the array from the patch #2 when
somebody increases OVS_PSAMPLE_COOKIE_MAX_SIZE in the future.

Thanks,
Michal