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]

Date:   Fri, 19 Nov 2021 07:04:07 -0800
From:   Dave Hansen <dave.hansen@...el.com>
To:     Muchun Song <songmuchun@...edance.com>,
        dave.hansen@...ux.intel.com, luto@...nel.org, peterz@...radead.org,
        tglx@...utronix.de, mingo@...hat.com, bp@...en8.de, hpa@...or.com
Cc:     x86@...nel.org, linux-kernel@...r.kernel.org,
        duanxiongchun@...edance.com, zhengqi.arch@...edance.com
Subject: Re: [RFC PATCH] x86/fault: move might_sleep() out of mmap read lock

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.

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.

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.