linux-kernel - Re: [PATCH-tip v2 02/10] locking/rwsem: Move owner setting code from rwsem.c to rwsem.h

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: <5f58af2f-b333-d3c3-a1e6-099dddd91f89@redhat.com>
Date:   Thu, 21 Feb 2019 22:37:35 -0500
From:   Waiman Long <longman@...hat.com>
To:     Will Deacon <will.deacon@....com>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        linux-kernel@...r.kernel.org, x86@...nel.org,
        Arnd Bergmann <arnd@...db.de>, Borislav Petkov <bp@...en8.de>,
        "H. Peter Anvin" <hpa@...or.com>,
        Davidlohr Bueso <dave@...olabs.net>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Tim Chen <tim.c.chen@...ux.intel.com>
Subject: Re: [PATCH-tip v2 02/10] locking/rwsem: Move owner setting code from
 rwsem.c to rwsem.h

On 02/21/2019 09:15 AM, Will Deacon wrote:
> Hi Waiman,
>
> On Fri, Feb 15, 2019 at 03:50:02PM -0500, Waiman Long wrote:
>> Moves all the owner setting code closer to the rwsem-xadd fast paths
>> directly within rwsem.h file.
>>
>> For __down_read() and __down_read_killable(), it is assumed that
>> rwsem will be marked as reader-owned when the functions return. That
>> is currently true except in the transient case that the waiter queue
>> just becomes empty. So a rwsem_set_reader_owned() call is added for
>> this case.
> Sorry, but I'm having a really hard time understanding the paragraph
> above. You make it sound like you're fixing a bug here, but that doesn't
> appear to be the case. Please can you elaborate?

Sorry for the confusion. I am referring to the code

if (list_empty(&sem->wait_list)) {
                /*
                 * In case the wait queue is empty and the lock isn't owned
                 * by a writer, this reader can exit the slowpath and return
                 * immediately as its RWSEM_ACTIVE_READ_BIAS has already
                 * been set in the count.
                 */
                if (atomic_long_read(&sem->count) >= 0) {
                        raw_spin_unlock_irq(&sem->wait_lock);
                        return sem;
                }
                adjustment += RWSEM_WAITING_BIAS;
        }

Here, a reader will return without going through the wakeup path. It is
here that I need to insert an explicit owner setting call.

>> The __rwsem_set_reader_owned() call in __rwsem_mark_wake()
>> is now necessary.
>>
>> Signed-off-by: Waiman Long <longman@...hat.com>
>> ---
>>  kernel/locking/rwsem-xadd.c |  6 +++---
>>  kernel/locking/rwsem.c      | 19 ++-----------------
>>  kernel/locking/rwsem.h      | 17 +++++++++++++++--
>>  3 files changed, 20 insertions(+), 22 deletions(-)
> I personally find the existing code easier to read, but I don't know if
> that's just because I'm used to it...

The check reason for this patch is because I want to add a RWSEM_DEBUG
check when a reader returns from the slowpath to make sure that the
wakeup path is being call and because of some kind race condition. This
check may be able to spot the wake_q race condition that was fixed
earlier. I will update the commit log to make my intention clearer.

Cheers,
Longman