linux-kernel - Re: [PATCH] mm/shmem: Do not wait for lock_page() in shmem_unused_huge

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, 16 Mar 2018 14:20:23 +0100
From:   Michal Hocko <mhocko@...nel.org>
To:     Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
Cc:     kirill@...temov.name, kirill.shutemov@...ux.intel.com,
        akpm@...ux-foundation.org, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, stable@...r.kernel.org,
        linux-mm@...ts.ewheeler.net
Subject: Re: [PATCH] mm/shmem: Do not wait for lock_page() in
 shmem_unused_huge_shrink()

On Fri 16-03-18 22:14:24, Tetsuo Handa wrote:
> f2fs is doing
> 
>   page = f2fs_pagecache_get_page(inode->i_mapping, 0, FGP_LOCK|FGP_NOWAIT, 0);
> 
> which calls
> 
>   struct page *pagecache_get_page(inode->i_mapping, 0, FGP_LOCK|FGP_NOWAIT, 0);
> 
> . Then, can't we define
> 
>   static inline struct page *find_trylock_page(struct address_space *mapping,
>   					     pgoff_t offset)
>   {
>   	return pagecache_get_page(mapping, offset, FGP_LOCK|FGP_NOWAIT, 0);
>   }
> 
> and replace find_lock_page() with find_trylock_page() ?

I haven't checked whether we have enough users of this pattern to create
a helper.

> Also, won't
> 
> ----------
> diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
> index 34ce3ebf..0cfc329 100644
> --- a/include/linux/pagemap.h
> +++ b/include/linux/pagemap.h
> @@ -479,6 +479,8 @@ static inline int trylock_page(struct page *page)
>  static inline void lock_page(struct page *page)
>  {
>  	might_sleep();
> +	WARN_ONCE(current->flags & PF_MEMALLOC,
> +		  "lock_page() from reclaim context might deadlock");
>  	if (!trylock_page(page))
>  		__lock_page(page);
>  }

lock_page is called from many (semi)hot paths so I wouldn't add
additional code there. Maybe we can hide it in VM_WARN. I would have
to think much more to be sure this won't lead to some strange false
positives. I suspect it won't but wouldn't bet my head on that.

In any case, you can try to send a patch and we can stick it into mmotm
and have it there for few cycles to see what falls out...
-- 
Michal Hocko
SUSE Labs