netdev - Re: [PATCH] net/mlx5e: fix high stack usage

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: <7d1bb1476459a16343fa0c0846d9181b667ad002.camel@mellanox.com>
Date:   Sat, 3 Nov 2018 00:52:12 +0000
From:   Saeed Mahameed <saeedm@...lanox.com>
To:     "arnd@...db.de" <arnd@...db.de>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Moshe Shemesh <moshe@...lanox.com>,
        "linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
        Boris Pismenny <borisp@...lanox.com>,
        Tariq Toukan <tariqt@...lanox.com>,
        "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
        Eran Ben Elisha <eranbe@...lanox.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Ilya Lesokhin <ilyal@...lanox.com>,
        "eric.dumazet@...il.com" <eric.dumazet@...il.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "leon@...nel.org" <leon@...nel.org>,
        Kamal Heib <kamalh@...lanox.com>
Subject: Re: [PATCH] net/mlx5e: fix high stack usage

On Fri, 2018-11-02 at 23:15 +0100, Arnd Bergmann wrote:
> On 11/2/18, Saeed Mahameed <saeedm@...lanox.com> wrote:
> > On Fri, 2018-11-02 at 14:39 -0700, Eric Dumazet wrote:
> > > 
> > > On 11/02/2018 02:05 PM, Saeed Mahameed wrote:
> > > 
> > > > temp will be mem copied to priv->stats.sw at the end,
> > > > memcpy(&priv->stats.sw, &s, sizeof(s));
> > > > 
> > > > one other way to solve this as suggested by Andrew, is to get
> > > > rid
> > > > of
> > > > the temp var and make it point directly to priv->stats.sw
> > > > 
> > > 
> > > What about concurrency ?
> > > 
> > > This temp variable is there to make sure concurrent readers of
> > > stats
> > > might
> > > not see mangle data (because another 'reader' just did a memset()
> > > and
> > > is doing the folding)
> > > 
> > > 
> > > mlx5e_get_stats() can definitely be run at the same time by
> > > multiple
> > > threads.
> > > 
> > 
> > hmm, you are right, i was thinking that mlx5e_get_Stats will
> > trigger a
> > work to update stats and grab the state_lock, but for sw stats this
> > is
> > not the case it is done in place.
> > 
> > BTW memcpy itself is not thread safe.
> 
> Before commit 6c63efe4cfab ("net/mlx5e: Remove redundant
> active_channels
> indication"), there was a read_lock() in the function apparently
> intended to
> made it thread safe. This got removed with the comment
> 
> commit 6c63efe4cfabf230a8ed4b1d880249875ffdac13
> Author: Eran Ben Elisha <eranbe@...lanox.com>
> Date:   Tue May 29 11:06:31 2018 +0300
> 
>     net/mlx5e: Remove redundant active_channels indication
> 
>     Now, when all channels stats are saved regardless of the
> channel's state
>     {open, closed}, we can safely remove this indication and the
> stats spin
>     lock which protects it.
> 
>     Fixes: 76c3810bade3 ("net/mlx5e: Avoid reset netdev stats on
> configuration changes")
> 
> I don't really understand the reasoning, but maybe we can remove
> the memcpy() if the code is thread safe, or we need the lock back if
> it's not.
> 

this lock was needed for a whole different purpose, it wasn't meant to
synchronize between two reader threads, it was meant to synchronize
between driver restarts and the reader for loop which ran over the open
channels, while they could be going through a destruction process.

I think all we need is to maintain two priv->stats.sw copies and use
them as temp for each reader thread, when can only have two concurrent
readers (mlx5e_ethtool_get_ethtool_stats and ndo_get_stats64) .. 

>          Arnd