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

[Michal Kubiak <michal.kubiak@intel.com>]

On Mon, Jul 01, 2024 at 12:56:43PM +0000, Adrián Moreno wrote:
> On Mon, Jul 01, 2024 at 01:40:39PM GMT, Michal Kubiak wrote:
> > 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@redhat.com>
> > > Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> > > ---
> > >  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(-)
> > >

[...]

> > > @@ -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
> >
> 
> Hi Michal,
> 
> Thanks for sharing your thoughts.
> 
> I tried to keep the dependency between both cookie sizes loose.
> 
> I don't want a change in TC_COOKIE_MAX_SIZE to inadvertently modify
> OVS_PSAMPLE_COOKIE_MAX_SIZE because OVS might not need a bigger cookie
> and even if it does, backwards compatibility needs to be guaranteed:
> meaning OVS userspace will have to detect the new size and use it or
> fall back to a smaller cookie for older kernels. All this needs to be
> known and worked on in userspace.
> 
> On the other hand, I intentionally made OVS's "psample" action as
> similar as possible to act_psample, including setting the same cookie
> size to begin with. The reason is that I think we should try to implement
> tc-flower offloading of this action using act_sample, plus 16 seemed a
> very reasonable max value.
> 
> When we decide to support offloading the "psample" action, this must
> be done entirely in userspace. OVS must create a act_sample action
> (instead of the OVS "psample" one) via netlink. In no circumstances the
> openvswitch kmod interacts with tc directly.
> 
> Now, back to your concern. If OVS_PSAMPLE_COOKIE_MAX_SIZE grows and
> TC_COOKIE_MAX_SIZE does not *and* we already support offloading this
> action to tc, the only consequence is that OVS userspace has a
> problem because the tc's netlink interface will reject cookies larger
> than TC_COOKIE_MAX_SIZE [1].
> This guarantees that the array in patch #2 is never overflown.
> 
> OVS will have to deal with the different sizes and try to squeeze the
> data into TC_COOKIE_MAX_SIZE or fail to offload the action altogether.
> 
> Psample does not have a size limit so different parts of the kernel can
> use psample with different internal max-sizes without any restriction.
> 
> I hope this clears your concerns.
> 
> [1] https://github.com/torvalds/linux/blob/master/net/sched/act_api.c#L1299
> 
> Thanks.
> Adrián
> 

Thank you, Adrian, for your detailed explanation. I wasn't aware of the
internal validation of that parameter using the mechanism from [1].
Sorry for asking the questions I should have answered by studying the
code more carefully.

I have no concerns about it now.

Thanks,
Reviewed-by: Michal Kubiak <michal.kubiak@intel.com>