lkml.org 
[lkml]   [2021]   [Oct]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] fscache: fix GPF in fscache_free_cookie
From
Date
On Thu, 2021-10-28 at 10:20 +0800, Dongliang Mu wrote:
> On Wed, Oct 27, 2021 at 11:16 PM Jeff Layton <jlayton@kernel.org> wrote:
> >
> > 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@gmail.com>
> > > ---
> > > 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@kernel.org>
>
> Hi Jeff,
>
> fscache_free_cookie also has an issue in cookie->backing_objects, but
> it does not affect the execution. The reason is in the following:
>
> At first, I observed that the cookie->backing_objects in
> fscache_alloc_cookie is not initialized with INIT_HLIST_HEAD when an
> error occurs. It may lead to some issues in the fscache_free_cookie,
> e.g., WARN_ON.
>
> Actually, it does not due to the zero initialization of
> kmem_cache_zalloc before. cookie->backing_objects is already with two
> null pointers. It does not need INIT_HLIST_HEAD.
>
> And in the fscache_free_cookie, it actually does not trigger
> WARN_ON(!hlist_empty()).
>
> So I wonder if we need to explicitly move INIT_HLIST_HEAD before any
> error handling code.
>
>

I don't think so. INIT_HLIST_HEAD just does this:

#define INIT_HLIST_HEAD(ptr) ((ptr)->first = NULL)

...so I think it's unnecessary in this case, since the thing is
zalloc'ed (like you said), it's already initialized. Probably we could
just skip the INIT_HLIST_HEAD call altogether in the
fscache_cookie_alloc, but David has a pile of patches in flight that
rework this code substantially, so I wouldn't worry about it at the
moment.

--
Jeff Layton <jlayton@kernel.org>

\
 
 \ /
  Last update: 2021-10-28 12:40    [W:0.056 / U:0.224 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site