netdev - Re: [PATCH bpf-next 2/4] bpf: fix lockdep false positive in stackmap

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: <20190130201047.hscaxd7434hrznxx@ast-mbp.dhcp.thefacebook.com>
Date:   Wed, 30 Jan 2019 12:10:48 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Waiman Long <longman@...hat.com>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Alexei Starovoitov <ast@...nel.org>, davem@...emloft.net,
        daniel@...earbox.net, edumazet@...gle.com, jannh@...gle.com,
        netdev@...r.kernel.org, kernel-team@...com
Subject: Re: [PATCH bpf-next 2/4] bpf: fix lockdep false positive in stackmap

On Wed, Jan 30, 2019 at 02:42:23PM -0500, Waiman Long wrote:
> On 01/30/2019 02:30 PM, Alexei Starovoitov wrote:
> > On Wed, Jan 30, 2019 at 11:15:30AM +0100, Peter Zijlstra wrote:
> >> On Tue, Jan 29, 2019 at 08:04:56PM -0800, Alexei Starovoitov wrote:
> >>> Lockdep warns about false positive:
> >> This is not a false positive, and you probably also need to use
> >> down_read_non_owner() to match this up_read_non_owner().
> >>
> >> {up,down}_read() and {up,down}_read_non_owner() are not only different
> >> in the lockdep annotation; there is also optimistic spin stuff that
> >> relies on 'owner' tracking.
> > Can you point out in the code the spin bit?
> > As far as I can see sem->owner is debug only feature.
> > All owner checks are done under CONFIG_DEBUG_RWSEMS.
> 
> No, sem->owner is mainly for performing optimistic spinning which is a
> performance feature to make rwsem writer-lock performs similar to mutex.
> The debugging part is just an add-on. It is not the reason for the
> presence of sem->owner.

I see. Got it.

> > Also there is no down_read_trylock_non_owner() at the moment.
> > We can argue about it for -next, but I'd rather silence lockdep
> > with this patch today.
> >
> We can add down_read_trylock_non_owner() if there is a need for it. It
> should be easy to do.

Yes, but looking through the code it's not clear to me that it's safe
to mix non_owner() versions with regular.
bpf/stackmap.c does down_read_trylock + up_read.
If we add new down_read_trylock_non_owner that set the owner to
NULL | RWSEM_* bits is this safe with conccurent read/write
that do regular versions?
rwsem_can_spin_on_owner() does:
        if (owner) {
                ret = is_rwsem_owner_spinnable(owner) &&
                      owner_on_cpu(owner);
that looks correct.
For a second I thought there could be fault here due to non_owner.
But there could be other places where it's assumed that owner
is never null?

May be we should live with this lockdep warn in bpf tree
and fix it only in bpf-next?