netdev - Re: [PATCH net-next v9 1/7] net/sched: cls_api: Support hardware miss to tc 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]

Date:   Tue, 14 Feb 2023 14:31:06 +0200
From:   Oz Shlomo <ozsh@...dia.com>
To:     Paul Blakey <paulb@...dia.com>,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
Cc:     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 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.

I think it might be used in other use cases and not just for stats.

Actually, I think the current naming scheme of act_cookie and 
miss_cookie makes sense.