netdev - Re: [PATCH v2 2/3] net: Add BUG_ON() to get

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: <1515578288.131759.17.camel@gmail.com>
Date:   Wed, 10 Jan 2018 01:58:08 -0800
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Kirill Tkhai <ktkhai@...tuozzo.com>, netdev@...r.kernel.org,
        davem@...emloft.net
Cc:     ebiederm@...ssion.com
Subject: Re: [PATCH v2 2/3] net: Add BUG_ON() to get_net()

On Wed, 2018-01-10 at 10:37 +0300, Kirill Tkhai wrote:
> On 09.01.2018 21:52, Eric Dumazet wrote:
> > On Tue, 2018-01-09 at 18:00 +0300, Kirill Tkhai wrote:
> > > Since people may mistakenly obtain destroying net
> > > from net_namespace_list and from net::netns_ids
> > > without checking for its net::counter, let's protect
> > > against such situations and insert BUG_ON() to stop
> > > move on after this.
> > > 
> > > Panic is better, than memory corruption and undefined
> > > behavior.
> > > 
> > > Signed-off-by: Kirill Tkhai <ktkhai@...tuozzo.com>
> > > ---
> > >  include/net/net_namespace.h |    2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
> > > index 10f99dafd5ac..ff0e47471d5b 100644
> > > --- a/include/net/net_namespace.h
> > > +++ b/include/net/net_namespace.h
> > > @@ -195,7 +195,7 @@ void __put_net(struct net *net);
> > >  
> > >  static inline struct net *get_net(struct net *net)
> > >  {
> > > -	atomic_inc(&net->count);
> > > +	BUG_ON(atomic_inc_return(&net->count) <= 1);
> > >  	return net;
> > >  }
> > 
> > 
> > Why not simply use refcount_t instead of duplicating its logic?
> 
> The main goal of the change is to catch rare races happening on production nodes
> with real load and to prevent memory corruption. You can't simply use refcount_t
> primitives, as there is no appropriate primitive with BUG_ON() among them. WARN_ON()
> from the primitives doesn't protect from memory corruption.
> 
> Also, keep in mind, that CONFIG_REFCOUNT_FULL is usually disabled on no-debug kernel.
> I've checked both Fedora and Debian. So, the only possibility to catch such the races,
> if someone really happy meets them on test kernel and test workload, which is usually
> is very unlikely.
> 

Keep in mind that most of these bugs are found by syzkaller or other
fuzzer bots, that have CONFIG_REFCOUNT_FULL enabled.

Do not rely on production workload to find such bug for you, coverage
is very very low.

Resistance is futile, because this refcount will eventually be
converted some day.