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]

Date:   Fri, 10 Feb 2017 18:21:25 +0200
From:   Or Gerlitz <gerlitz.or@...il.com>
To:     Jakub Kicinski <jakub.kicinski@...ronome.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>
Subject: Re: [PATCH RFC net] net/mlx5e: Add preemption enable/disable around
 TC statistics upcall

On Fri, Feb 10, 2017 at 3:34 AM, Jakub Kicinski
<jakub.kicinski@...ronome.com> 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?

> for net-next we could perhaps redo the
> tcf_action_stats_update() helper so that it takes care of preemption and
> the iteration so more people don't trip over this?

maybe, lets 1st understand that deeper, hopefully you can assist..