netdev - Re: [PATCH RFC net] net/mlx5e: Add preemption enable/disable around TC statistics upcall

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: <20170210101416.1c537cec@cakuba.netronome.com>
Date:   Fri, 10 Feb 2017 10:14:16 -0800
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Or Gerlitz <gerlitz.or@...il.com>
Cc:     Or Gerlitz <ogerlitz@...lanox.com>,
        "David S. Miller" <davem@...emloft.net>,
        Amir Vadai <amir@...ai.me>, Ido Schimmel <idosch@...lanox.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Roi Dayan <roid@...lanox.com>,
        Linux Netdev List <netdev@...r.kernel.org>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Jamal Hadi Salim <jhs@...atatu.com>
Subject: Re: [PATCH RFC net] net/mlx5e: Add preemption enable/disable around
 TC statistics upcall

On Fri, 10 Feb 2017 18:21:25 +0200, Or Gerlitz wrote:
> On Fri, Feb 10, 2017 at 3:34 AM, Jakub Kicinski wrote:
> > On Thu,  9 Feb 2017 17:38:43 +0200, Or Gerlitz wrote:  
> >> Running with CONFIG_PREEMPT set, I get a
> >>
> >> BUG: using smp_processor_id() in preemptible [00000000] code: tc/3793
> >>
> >> asserion from the TC action (mirred) stats_update callback, when the do
> >>
> >>       _bstats_cpu_update(this_cpu_ptr(a->cpu_bstats), bytes, packets)
> >>
> >> As done by commit 66860be "nfp: bpf: allow offloaded filters to update stats",
> >> disabling/enabling preemption around the TC upcall solves that.
> >>
> >> Fixes: aad7e08d39bd ('net/mlx5e: Hardware offloaded flower filter statistics support')
> >> Signed-off-by: Or Gerlitz <ogerlitz@...lanox.com>
> >> ---
> >>
> >> I marked it as RFC, since I wasn't fully sure on the nature of the
> >> problem, nor if this is the direction we should take to the fix.  
> 
> > I think it's the right fix  
> 
> Do you under the problem? what's wrong with the call done in the TC
> action code w.r.t preemption?
> 
> does it make sense to do this (say) 100K times/sec?

TC actions have pre-cpu stats, referencing them has to be done with
preemption disabled.  Let's CC Jamal and Cong - maybe there are some
more clever things we could do here?  The situation in a nutshell is
that the offload drivers read the stats from HW and want to write them
back to the TC action stats.  The writeback happens in process context
when user requests stats dump (potentially for multiple actions but we
currently would just iterate over all actions in driver code).