Re: [PATCH] futex: Remove unnecessary warning from get_futex_key

[Mark Rutland]

On Wed, Aug 09, 2017 at 08:27:11AM +0100, Mel Gorman wrote:
> Commit 65d8fc777f6d ("futex: Remove requirement for lock_page() in
> get_futex_key()") removed an unnecessary lock_page() with the side-effect
> that page->mapping needed to be treated very carefully. Two defensive
> warnings were added in case any assumption was missed and the first warning
> assumed a correct application would not alter a mapping backing a futex key.
> Since merging, it has not triggered for any unexpected case but Mark
> Rutland reported the following bug triggering due to the first warning.

[...]

> Reported-by: Mark Rutland <mark.rutland@xxxxxxx>
> Signed-off-by: Mel Gorman <mgorman@xxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx # 4.7+
> ---
>  kernel/futex.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/futex.c b/kernel/futex.c
> index 16dbe4c93895..f50b434756c1 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -670,13 +670,14 @@ get_futex_key(u32 __user *uaddr, int fshared, union futex_key *key, int rw)
>  		 * this reference was taken by ihold under the page lock
>  		 * pinning the inode in place so i_lock was unnecessary. The
>  		 * only way for this check to fail is if the inode was
> -		 * truncated in parallel so warn for now if this happens.
> +		 * truncated in parallel which is almost certainly an
> +		 * application bug. In such a case, just retry.
>  		 *
>  		 * We are not calling into get_futex_key_refs() in file-backed
>  		 * cases, therefore a successful atomic_inc return below will
>  		 * guarantee that get_futex_key() will still imply smp_mb(); (B).
>  		 */
> -		if (WARN_ON_ONCE(!atomic_inc_not_zero(&inode->i_count))) {
> +		if (!atomic_inc_not_zero(&inode->i_count)) {

I applied the same diff yesterday, and haven't seen anything go wrong
with my test case and/or with Syzkaller running, so FWIW:

Tested-by: Mark Rutland <mark.rutland@xxxxxxx>

Thanks for putting this together!

Mark.