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

Message-ID: <20190506061631.GB2362@nanopsycho.orion>
Date:   Mon, 6 May 2019 08:16:31 +0200
From:   Jiri Pirko <jiri@...nulli.us>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
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

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.

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?

Thanks!