Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20200131164308.GA5175@willie-the-truck> <CANn89i+CnezK81gZSLOy0w7MaZy0uT=xyxoKSTyZU3aMpzifOA@mail.gmail.com>
 <20200131184322.GA11457@worktop.programming.kicks-ass.net>
In-Reply-To: <20200131184322.GA11457@worktop.programming.kicks-ass.net>
From:   Eric Dumazet <edumazet@google.com>
Date:   Fri, 31 Jan 2020 10:48:21 -0800
Message-ID: <CANn89iLBL1qbrEucm2FU02oNbf=x3_4K93TmZ3yS2ZNWm8Qrsg@mail.gmail.com>
Subject: Re: Confused about hlist_unhashed_lockless()
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     Will Deacon <will@kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        "Paul E. McKenney" <paulmck@kernel.org>,
        "the arch/x86 maintainers" <x86@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Marco Elver <elver@google.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Jan 31, 2020 at 10:43 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Fri, Jan 31, 2020 at 08:48:05AM -0800, Eric Dumazet wrote:
> >     BUG: KCSAN: data-race in del_timer / detach_if_pending
>
> > diff --git a/include/linux/timer.h b/include/linux/timer.h
> > index 1e6650ed066d5d28251b0bd385fc37ef94c96532..0dc19a8c39c9e49a7cde3d34bfa4be8871cbc1c2
> > 100644
> > --- a/include/linux/timer.h
> > +++ b/include/linux/timer.h
> > @@ -164,7 +164,7 @@ static inline void destroy_timer_on_stack(struct
> > timer_list *timer) { }
> >   */
> >  static inline int timer_pending(const struct timer_list * timer)
> >  {
> > - return timer->entry.pprev != NULL;
> > + return !hlist_unhashed_lockless(&timer->entry);
> >  }
>
> That's just completely wrong.
>
> Aside from any memory barrier issues that might or might not be there
> (I'm waaaay to tired atm to tell), the above code is perfectly fine.
>
> In fact, this is a KCSAN compiler infrastructure 'bug'.
>
> Any load that is only compared to zero is immune to load-tearing issues.
>
> The correct thing to do here is something like:
>
> static inline int timer_pending(const struct timer_list *timer)
> {
>         /*
>          * KCSAN compiler infrastructure is insuffiently clever to
>          * realize that a 'load compared to zero' is immune to
>          * load-tearing.
>          */
>         return data_race(timer->entry.pprev != NULL);
> }


This is nice, now with have data_race()

Remember these patches were sent 2 months ago, at a time we were
trying to sort out things.

data_race() was merged a few days ago.