linux-kernel - Re: [RFC PATCH] x86/fault: move might

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: <CAMZfGtVDgbLAS3uyB0QPHpDsA8Mam0sVYFdq9HhU4rSjqZG0qw@mail.gmail.com>
Date:   Mon, 22 Nov 2021 14:59:54 +0800
From:   Muchun Song <songmuchun@...edance.com>
To:     Dave Hansen <dave.hansen@...el.com>
Cc:     Dave Hansen <dave.hansen@...ux.intel.com>,
        Andrew Lutomirski <luto@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Peter Anvin <hpa@...or.com>, X86 ML <x86@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Xiongchun duan <duanxiongchun@...edance.com>,
        Qi Zheng <zhengqi.arch@...edance.com>
Subject: Re: [RFC PATCH] x86/fault: move might_sleep() out of mmap read lock

On Fri, Nov 19, 2021 at 11:04 PM Dave Hansen <dave.hansen@...el.com> wrote:
>
> On 11/18/21 10:58 PM, Muchun Song wrote:
> > The mmap lock is supposed to be a contended lock sometimes, scheduling
> > to other task with holding mmap read lock does not seems to be a wise
> > choice. So move it to the front of mmap_read_trylock(). Although
> > mmap_read_lock() implies a might_sleep(), I think redundant check is
> > not a problem since this task is about to sleep and it is not a hot
> > path.
>
> This justification doesn't really do it for me.  There are a billion
> ways to sleep in the fault path while the mmap lock is held.  Adding one
> more cond_resched() doesn't seem like it would do much.

I agree with you that there are lots of ways to sleep in the
fault path. Just try my best to not sleep with mmap lock.

>
> In other words, I don't think there's a performance justification here.
>
> That said...
>
> > diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> > index 4bfed53e210e..22fd1dfafa3d 100644
> > --- a/arch/x86/mm/fault.c
> > +++ b/arch/x86/mm/fault.c
> > @@ -1323,6 +1323,8 @@ void do_user_addr_fault(struct pt_regs *regs,
> >       }
> >  #endif
> >
> > +     might_sleep();
> > +
> >       /*
> >        * Kernel-mode access to the user address space should only occur
> >        * on well-defined single instructions listed in the exception
> > @@ -1346,13 +1348,6 @@ void do_user_addr_fault(struct pt_regs *regs,
> >               }
> >  retry:
> >               mmap_read_lock(mm);
> > -     } else {
> > -             /*
> > -              * The above down_read_trylock() might have succeeded in
> > -              * which case we'll have missed the might_sleep() from
> > -              * down_read():
> > -              */
> > -             might_sleep();
> >       }
> >
> >       vma = find_vma(mm, address);
>
> The comment is stale, which isn't great.  The might_sleep() is already
> in the fast path.  So, moving it up above makes a lot of sense just in
> terms of simplicity.

Without this patch, I didn't see the might_sleep() in the fast path. What
am I missing here?

Thanks.

>
> The might_sleep() does need a comment, though, about what it's _doing_
> there.
>
> So, right idea, good result, but for the wrong reasons.
>
> If you want to revise the justification and add a comment, I think this
> is something we can take.