linux-kernel - Re: [tip: timers/core] hrtimer: Annotate lockless access to timer->state

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:   Thu, 7 Nov 2019 08:54:28 -0800
From:   "Paul E. McKenney" <paulmck@...nel.org>
To:     Eric Dumazet <edumazet@...gle.com>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        linux-tip-commits@...r.kernel.org,
        syzbot <syzkaller@...glegroups.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...nel.org>, Borislav Petkov <bp@...en8.de>
Subject: Re: [tip: timers/core] hrtimer: Annotate lockless access to
 timer->state

On Thu, Nov 07, 2019 at 08:39:42AM -0800, Eric Dumazet wrote:
> On Thu, Nov 7, 2019 at 8:35 AM Eric Dumazet <edumazet@...gle.com> wrote:
> >
> > On Thu, Nov 7, 2019 at 8:11 AM Paul E. McKenney <paulmck@...nel.org> wrote:
> > >
> > > OK, so this is due to timer_pending() lockless access to ->entry.pprev
> > > to determine whether or not the timer is on the list.  New one on me!
> > >
> > > Given that use case, I don't have an objection to your patch to list.h.
> > >
> > > Except...
> > >
> > > Would it make sense to add a READ_ONCE() to hlist_unhashed()
> > > and to then make timer_pending() invoke hlist_unhashed()?  That
> > > would better confine the needed uses of READ_ONCE().
> >
> > Sounds good to me, I had the same idea but was too lazy to look at the
> > history of timer_pending()
> > to check if the pprev pointer check was really the same underlying idea.
> 
> Note that forcing READ_ONCE() in hlist_unhashed() might force the compiler
> to read the pprev pointer twice in some cases.
> 
> This was one of the reason for me to add skb_queue_empty_lockless()
> variant in include/linux/skbuff.h

Ouch!

> /**
>  * skb_queue_empty_lockless - check if a queue is empty
>  * @list: queue head
>  *
>  * Returns true if the queue is empty, false otherwise.
>  * This variant can be used in lockless contexts.
>  */
> static inline bool skb_queue_empty_lockless(const struct sk_buff_head *list)
> {
> return READ_ONCE(list->next) == (const struct sk_buff *) list;
> }
> 
> So maybe add a hlist_unhashed_lockless() to clearly document why
> callers are using the lockless variant ?

That sounds like a reasonable approach to me.  There aren't all that
many uses of hlist_unhashed(), so a name change should not be a problem.

							Thanx, Paul