| 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
| ||
|
Message-ID: <Y+vXhDvkFL3DBqJu@t14s.localdomain>
Date: Tue, 14 Feb 2023 10:48:36 -0800
From: Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
To: Oz Shlomo <ozsh@...dia.com>
Cc: Paul Blakey <paulb@...dia.com>, netdev@...r.kernel.org,
Saeed Mahameed <saeedm@...dia.com>,
Paolo Abeni <pabeni@...hat.com>,
Jakub Kicinski <kuba@...nel.org>,
Eric Dumazet <edumazet@...gle.com>,
Jamal Hadi Salim <jhs@...atatu.com>,
Cong Wang <xiyou.wangcong@...il.com>,
"David S. Miller" <davem@...emloft.net>,
Jiri Pirko <jiri@...dia.com>, Roi Dayan <roid@...dia.com>,
Vlad Buslov <vladbu@...dia.com>,
Simon Horman <simon.horman@...igine.com>
Subject: Re: [PATCH net-next v9 1/7] net/sched: cls_api: Support hardware
miss to tc action
On Tue, Feb 14, 2023 at 02:31:06PM +0200, Oz Shlomo wrote:
>
> On 14/02/2023 14:14, Paul Blakey wrote:
> > On 13/02/2023 20:43, Marcelo Ricardo Leitner wrote:
> > > On Mon, Feb 13, 2023 at 06:13:34PM +0200, Paul Blakey wrote:
> > > > On 10/02/2023 04:21, Marcelo Ricardo Leitner wrote:
> > > > > On Mon, Feb 06, 2023 at 07:43:57PM +0200, Paul Blakey wrote:
> > > > > > For drivers to support partial offload of a filter's action list,
> > > > > > add support for action miss to specify an action instance to
> > > > > > continue from in sw.
> > > > > >
> > > > > > CT action in particular can't be fully offloaded, as new connections
> > > > > > need to be handled in software. This imposes other limitations on
> > > > > > the actions that can be offloaded together with the CT action, such
> > > > > > as packet modifications.
> > > > > >
> > > > > > Assign each action on a filter's action list a unique miss_cookie
> > > > > > which drivers can then use to fill action_miss part of the tc skb
> > > > > > extension. On getting back this miss_cookie, find the action
> > > > > > instance with relevant cookie and continue classifying from there.
> > > > > >
> > > > > > Signed-off-by: Paul Blakey <paulb@...dia.com>
> > > > > > Reviewed-by: Jiri Pirko <jiri@...dia.com>
> > > > > > Reviewed-by: Simon Horman <simon.horman@...igine.com>
> > > > > > Acked-by: Jamal Hadi Salim <jhs@...atatu.com>
> > > > > > ---
> > > > > > include/linux/skbuff.h | 6 +-
> > > > > > include/net/flow_offload.h | 1 +
> > > > > > include/net/pkt_cls.h | 34 +++---
> > > > > > include/net/sch_generic.h | 2 +
> > > > > > net/openvswitch/flow.c | 3 +-
> > > > > > net/sched/act_api.c | 2 +-
> > > > > > net/sched/cls_api.c | 213 +++++++++++++++++++++++++++++++++++--
> > > > > > 7 files changed, 234 insertions(+), 27 deletions(-)
> > > > > >
> > > > > > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > > > > > index 1fa95b916342..9b9aa854068f 100644
> > > > > > --- a/include/linux/skbuff.h
> > > > > > +++ b/include/linux/skbuff.h
> > > > > > @@ -311,12 +311,16 @@ struct nf_bridge_info {
> > > > > > * and read by ovs to recirc_id.
> > > > > > */
> > > > > > struct tc_skb_ext {
> > > > > > - __u32 chain;
> > > > > > + union {
> > > > > > + u64 act_miss_cookie;
> > > > > > + __u32 chain;
> > > > > > + };
> > > > > > __u16 mru;
> > > > > > __u16 zone;
> > > > > > u8 post_ct:1;
> > > > > > u8 post_ct_snat:1;
> > > > > > u8 post_ct_dnat:1;
> > > > > > + u8 act_miss:1; /* Set if act_miss_cookie is used */
> > > > > > };
> > > > > > #endif
> > > > > > diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
> > > > > > index 0400a0ac8a29..88db7346eb7a 100644
> > > > > > --- a/include/net/flow_offload.h
> > > > > > +++ b/include/net/flow_offload.h
> > > > > > @@ -228,6 +228,7 @@ void flow_action_cookie_destroy(struct flow_action_cookie *cookie);
> > > > > > struct flow_action_entry {
> > > > > > enum flow_action_id id;
> > > > > > u32 hw_index;
> > > > > > + u64 miss_cookie;
> > > > > The per-action stats patchset is adding a cookie for the actions as
> > > > > well, and exactly on this struct:
> > > > >
> > > > > @@ -228,6 +228,7 @@ struct flow_action_cookie *flow_action_cookie_create(void *data,
> > > > > struct flow_action_entry {
> > > > > enum flow_action_id id;
> > > > > u32 hw_index;
> > > > > + unsigned long act_cookie;
> > > > > enum flow_action_hw_stats hw_stats;
> > > > > action_destr destructor;
> > > > > void *destructor_priv;
> > > > >
> > > > > There, it is a simple value: the act pointer itself. Here, it is already more
> > > > > complex. Can them be merged into only one maybe?
> > > > > If not, perhaps act_cookie should be renamed to stats_cookie then.
> > > > I don't think it can be shared, actions can be shared between multiple
> > > > filters, while the miss cookie would be different for each used instance
> > > > (takes the filter in to account).
> > > Good point. So it would at best be a masked value that part A works
> > > for the miss here and part B for the stats, which is pretty much what
> > > the two cookies are giving, just without having to do bit gymnasics,
> > > yes.
> > act cookie is using 64 bits (to store the pointer and void a mapping), and I'm using at least
> >
> > 32bits, so there is not simple type that will contain both.
> >
> > So I'll rename the act_cookie to stats_cookie once I rebase.
> >
> The current act_cookie uniquely identifies the action instance.
>
While miss_cookie also uniquely identifies the action, within a filter list.
Or, miss_cookie uniquely identifies an action configuration.
> I think it might be used in other use cases and not just for stats.
I don't disagree.
>
> Actually, I think the current naming scheme of act_cookie and miss_cookie
> makes sense.
act_cookie is actually already used (and differently) in tc_action itself here:
1045ba77a596 ("net sched actions: Add support for user cookies")
With this, IMO it is tough to know which one takes precedence where.
Then perhaps,
act_cookie here -> instance_cookie
miss_cookie -> config_cookie
Sorry for the bikeshedding, btw, but these cookies are getting
confusing. We need them to taste nice :-}
Powered by blists - more mailing lists