* [PATCH] fscache: fix GPF in fscache_free_cookie
@ 2021-10-27 15:07 Dongliang Mu
2021-10-27 15:16 ` Jeff Layton
0 siblings, 1 reply; 5+ messages in thread
From: Dongliang Mu @ 2021-10-27 15:07 UTC (permalink / raw)
To: David Howells, Jeff Layton; +Cc: Dongliang Mu, linux-cachefs, linux-kernel
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:
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] fscache: fix GPF in fscache_free_cookie 2021-10-27 15:07 [PATCH] fscache: fix GPF in fscache_free_cookie Dongliang Mu @ 2021-10-27 15:16 ` Jeff Layton 2021-10-28 2:20 ` Dongliang Mu 0 siblings, 1 reply; 5+ messages in thread From: Jeff Layton @ 2021-10-27 15:16 UTC (permalink / raw) To: Dongliang Mu, David Howells; +Cc: linux-cachefs, linux-kernel 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> ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] fscache: fix GPF in fscache_free_cookie 2021-10-27 15:16 ` Jeff Layton @ 2021-10-28 2:20 ` Dongliang Mu 2021-10-28 10:39 ` Jeff Layton 0 siblings, 1 reply; 5+ messages in thread From: Dongliang Mu @ 2021-10-28 2:20 UTC (permalink / raw) To: Jeff Layton; +Cc: David Howells, linux-cachefs, linux-kernel 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. > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] fscache: fix GPF in fscache_free_cookie 2021-10-28 2:20 ` Dongliang Mu @ 2021-10-28 10:39 ` Jeff Layton 2021-10-28 12:06 ` Dongliang Mu 0 siblings, 1 reply; 5+ messages in thread From: Jeff Layton @ 2021-10-28 10:39 UTC (permalink / raw) To: Dongliang Mu; +Cc: David Howells, linux-cachefs, linux-kernel 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> ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] fscache: fix GPF in fscache_free_cookie 2021-10-28 10:39 ` Jeff Layton @ 2021-10-28 12:06 ` Dongliang Mu 0 siblings, 0 replies; 5+ messages in thread From: Dongliang Mu @ 2021-10-28 12:06 UTC (permalink / raw) To: Jeff Layton; +Cc: David Howells, linux-cachefs, linux-kernel On Thu, Oct 28, 2021 at 6:39 PM Jeff Layton <jlayton@kernel.org> wrote: > > 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. Sure, sound good. > > -- > Jeff Layton <jlayton@kernel.org> > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-10-28 12:06 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-27 15:07 [PATCH] fscache: fix GPF in fscache_free_cookie Dongliang Mu 2021-10-27 15:16 ` Jeff Layton 2021-10-28 2:20 ` Dongliang Mu 2021-10-28 10:39 ` Jeff Layton 2021-10-28 12:06 ` Dongliang Mu
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.