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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZoPgLfosO5e160nO@localhost.localdomain>
Date: Tue, 2 Jul 2024 13:10:37 +0200
From: Michal Kubiak <michal.kubiak@...el.com>
To: Adrián Moreno <amorenoz@...hat.com>
CC: <netdev@...r.kernel.org>, <aconole@...hat.com>, <echaudro@...hat.com>,
	<horms@...nel.org>, <i.maximets@....org>, <dev@...nvswitch.org>, "Donald
 Hunter" <donald.hunter@...il.com>, Jakub Kicinski <kuba@...nel.org>, "David
 S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, "Paolo
 Abeni" <pabeni@...hat.com>, Pravin B Shelar <pshelar@....org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next v7 05/10] net: openvswitch: add psample action

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@...hat.com>
> > > Signed-off-by: Adrian Moreno <amorenoz@...hat.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@...el.com>