Message-ID: <1489683534.28631.231.camel@edumazet-glaptop3.roam.corp.google.com>
Subject: Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to
 refcount_t
From: Eric Dumazet <eric.dumazet@gmail.com>
To: Elena Reshetova <elena.reshetova@intel.com>
Cc: netdev@vger.kernel.org, bridge@lists.linux-foundation.org,
        linux-kernel@vger.kernel.org, kuznet@ms2.inr.ac.ru, jmorris@namei.org,
        kaber@trash.net, stephen@networkplumber.org, peterz@infradead.org,
        keescook@chromium.org, Hans Liljestrand <ishkamiel@gmail.com>,
        David Windsor <dwindsor@gmail.com>
Date: Thu, 16 Mar 2017 09:58:54 -0700
In-Reply-To: <1489678147-21404-8-git-send-email-elena.reshetova@intel.com>
References: <1489678147-21404-1-git-send-email-elena.reshetova@intel.com>
         <1489678147-21404-8-git-send-email-elena.reshetova@intel.com>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1129
Lines: 36

On Thu, 2017-03-16 at 17:28 +0200, Elena Reshetova wrote:
> refcount_t type and corresponding API should be
> used instead of atomic_t when the variable is used as
> a reference counter. This allows to avoid accidental
> refcounter overflows that might lead to use-after-free
> situations.


...

>  static __always_inline void sock_hold(struct sock *sk)
>  {
> -	atomic_inc(&sk->sk_refcnt);
> +	refcount_inc(&sk->sk_refcnt);
>  }
>  

While I certainly see the value of these refcount_t, we have a very
different behavior on these atomic_inc() which were doing a single
inlined LOCK RMW on x86.

We now call an external function performing a 
atomic_read(), various ops/tests, then atomic_cmpxchg_relaxed(), in a
loop, loosing the nice ability for x86 of preventing live locks.

Looks a lot of bloat, just to be able to chase hypothetical bugs in the
kernel.

I would love to have a way to enable extra debugging when I want a debug
kernel, like LOCKDEP or KASAN.

By adding all this bloat, we assert linux kernel is terminally buggy and
every atomic_inc() we did was suspicious, and need to be always
instrumented/validated.