Re: [PATCH] fscache: fix GPF in fscache_free_cookie

[Jeff Layton]

On Wed, 2021-10-27 at 23:07 +0800, Dongliang Mu wrote:
> If fscache_alloc_cookie encounters memory allocation failure, it will
> go to nomem label and invoke fscache_free_cookie. However,
> fscache_alloc_cookie assumes current cookie is already linked into
> fscache_cookies and directly calls list_del. This assumption does not
> hold since list_add is not called in the above scenario. As a result, it
> will lead to Null Pointer Dereference. The stack trace is in the
> following.
> 
> Call Trace:
>  __list_del_entry include/linux/list.h:132 [inline]
>  list_del include/linux/list.h:146 [inline]
>  fscache_free_cookie fs/fscache/cookie.c:71 [inline]
>  fscache_free_cookie+0x3f/0x100 fs/fscache/cookie.c:66
>  fscache_alloc_cookie+0x2e2/0x300 fs/fscache/cookie.c:195
>  __fscache_acquire_cookie fs/fscache/cookie.c:296 [inline]
>  __fscache_acquire_cookie+0x132/0x380 fs/fscache/cookie.c:257
>  fscache_acquire_cookie include/linux/fscache.h:334 [inline]
>  v9fs_cache_session_get_cookie+0x74/0x120 fs/9p/cache.c:60
>  v9fs_session_init+0x724/0xa90 fs/9p/v9fs.c:471
>  v9fs_mount+0x56/0x450 fs/9p/vfs_super.c:126
>  legacy_get_tree+0x2b/0x90 fs/fs_context.c:610
>  vfs_get_tree+0x28/0x100 fs/super.c:1498
>  do_new_mount fs/namespace.c:2988 [inline]
>  path_mount+0xb92/0xfe0 fs/namespace.c:3318
>  do_mount+0xa1/0xc0 fs/namespace.c:3331
>  __do_sys_mount fs/namespace.c:3539 [inline]
>  __se_sys_mount fs/namespace.c:3516 [inline]
>  __x64_sys_mount+0xf4/0x160 fs/namespace.c:3516
> 
> Fix this by moving the list_add_tail before goto statements.
> 
> Fixes: 884a76881fc5 ("fscache: Procfile to display cookies")
> Signed-off-by: Dongliang Mu <mudongliangabcd@xxxxxxxxx>
> ---
>  fs/fscache/cookie.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/fscache/cookie.c b/fs/fscache/cookie.c
> index cd42be646ed3..d101e212db74 100644
> --- a/fs/fscache/cookie.c
> +++ b/fs/fscache/cookie.c
> @@ -150,6 +150,11 @@ struct fscache_cookie *fscache_alloc_cookie(
>  	if (!cookie)
>  		return NULL;
>  
> +	/* move list_add_tail before any error handling code */
> +	write_lock(&fscache_cookies_lock);
> +	list_add_tail(&cookie->proc_link, &fscache_cookies);
> +	write_unlock(&fscache_cookies_lock);
> +
>  	cookie->key_len = index_key_len;
>  	cookie->aux_len = aux_data_len;
>  
> @@ -186,9 +191,6 @@ struct fscache_cookie *fscache_alloc_cookie(
>  	 * told it may not wait */
>  	INIT_RADIX_TREE(&cookie->stores, GFP_NOFS & ~__GFP_DIRECT_RECLAIM);
>  
> -	write_lock(&fscache_cookies_lock);
> -	list_add_tail(&cookie->proc_link, &fscache_cookies);
> -	write_unlock(&fscache_cookies_lock);
>  	return cookie;
>  
>  nomem:

Nice catch!

Reviewed-by: Jeff Layton <jlayton@xxxxxxxxxx>