netdev - Re: [PATCH net-next 10/13] net/sched: add block pointer to tc_cls_common

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] [day] [month] [year] [list]

Date:   Mon, 6 May 2019 11:11:13 -0700
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     davem@...emloft.net, netdev@...r.kernel.org,
        oss-drivers@...ronome.com, xiyou.wangcong@...il.com,
        idosch@...lanox.com, f.fainelli@...il.com, andrew@...n.ch,
        vivien.didelot@...il.com, gerlitz.or@...il.com,
        simon.horman@...ronome.com,
        Pieter Jansen van Vuuren 
        <pieter.jansenvanvuuren@...ronome.com>
Subject: Re: [PATCH net-next 10/13] net/sched: add block pointer to
 tc_cls_common_offload structure

On Mon, 6 May 2019 08:16:31 +0200, Jiri Pirko wrote:
> Sun, May 05, 2019 at 07:34:32PM CEST, jakub.kicinski@...ronome.com wrote:
> >On Sat, 4 May 2019 15:16:54 +0200, Jiri Pirko wrote:  
> >> Sat, May 04, 2019 at 01:46:25PM CEST, jakub.kicinski@...ronome.com wrote:  
> >> >From: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@...ronome.com>
> >> >
> >> >Some actions like the police action are stateful and could share state
> >> >between devices. This is incompatible with offloading to multiple devices
> >> >and drivers might want to test for shared blocks when offloading.
> >> >Store a pointer to the tcf_block structure in the tc_cls_common_offload
> >> >structure to allow drivers to determine when offloads apply to a shared
> >> >block.    
> >> 
> >> I don't this this is good idea. If your driver supports shared blocks,
> >> you should register the callback accordingly. See:
> >> mlxsw_sp_setup_tc_block_flower_bind() where tcf_block_cb_lookup() and
> >> __tcf_block_cb_register() are used to achieve that.  
> >
> >Right, in some ways.  Unfortunately we don't support shared blocks
> >fully, i.e. we register multiple callbacks and get the rules
> >replicated.  It's a FW limitation, but I don't think we have shared
> >blocks on the roadmap, since rule storage is not an issue for our HW.
> >
> >But even if we did support sharing blocks, we'd have to teach TC that
> >some rules can only be offloaded if there is only a single callback
> >registered, right?  In case the block is shared between different ASICs.  
> 
> I don't see why sharing block between different ASICs is a problem. The
> sharing implementation is totally up to the driver. It can duplicate the
> rules even within one ASIC. According to that, it registers one or more
> callbacks.

If we want to replicate software semantics for act_police all ports
sharing the port should count against the same rate limit.  This is
pretty much impossible unless the rule is offloaded to a single ASIC
and the ASIC/FW supports proper block/action sharing.

> In this patchset, you use the block only to see if it is shared or not.
> When TC calls the driver to bind, it provides the block struct:
> ndo_setup_tc
>    type == TC_SETUP_BLOCK
>       f->command == TC_BLOCK_BIND
> You can check for sharing there and remember it for the future check in
> filter insertion.
> 
> I would like to avoid passing block pointer during filter insertion. It
> is misleading and I'm pretty sure it would lead to misuse by drivers.
> 
> I see that Dave already applied this patchset. Could you please send
> follow-up removing the block pointer from filter offload struct?

Makes sense, we'll follow up shortly!