Attached are another couple of miscellaneous fixes for FS-Cache and
CacheFiles:
(1) Fix a race between object burial in cachefiles and external rmdir.
(2) Fix a race from a split atomic op.
(3) Fix incomplete initialisation of cookie key space.
(4) Fix out-of-bounds read.
The patches are tagged here:
git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git
fscache-fixes-20181017
and can also be found on the following branch:
http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=fscache-fixes
David
---
Al Viro (1):
cachefiles: fix the race between cachefiles_bury_object() and rmdir(2)
David Howells (1):
fscache: Fix incomplete initialisation of inline key space
Eric Sandeen (1):
fscache: Fix out of bound read in long cookie keys
kiran.modukuri (1):
fscache: Fix race in fscache_op_complete() due to split atomic_sub & read
fs/cachefiles/namei.c | 2 +-
fs/fscache/cookie.c | 31 ++++++++++---------------------
fs/fscache/internal.h | 1 -
fs/fscache/main.c | 4 +---
include/linux/fscache-cache.h | 4 ++--
5 files changed, 14 insertions(+), 28 deletions(-)
From: Al Viro <[email protected]>
the victim might've been rmdir'ed just before the lock_rename();
unlike the normal callers, we do not look the source up after the
parents are locked - we know it beforehand and just recheck that it's
still the child of what used to be its parent. Unfortunately,
the check is too weak - we don't spot a dead directory since its
->d_parent is unchanged, dentry is positive, etc. So we sail all
the way to ->rename(), with hosting filesystems _not_ expecting
to be asked renaming an rmdir'ed subdirectory.
The fix is easy, fortunately - the lock on parent is sufficient for
making IS_DEADDIR() on child safe.
Cc: [email protected]
Fixes: 9ae326a69004 (CacheFiles: A cache that backs onto a mounted filesystem)
Signed-off-by: Al Viro <[email protected]>
Signed-off-by: David Howells <[email protected]>
---
fs/cachefiles/namei.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c
index af2b17b21b94..95983c744164 100644
--- a/fs/cachefiles/namei.c
+++ b/fs/cachefiles/namei.c
@@ -343,7 +343,7 @@ static int cachefiles_bury_object(struct cachefiles_cache *cache,
trap = lock_rename(cache->graveyard, dir);
/* do some checks before getting the grave dentry */
- if (rep->d_parent != dir) {
+ if (rep->d_parent != dir || IS_DEADDIR(d_inode(rep))) {
/* the entry was probably culled when we dropped the parent dir
* lock */
unlock_rename(cache->graveyard, dir);
From: kiran.modukuri <[email protected]>
The code in fscache_retrieval_complete is using atomic_sub followed by an
atomic_read:
atomic_sub(n_pages, &op->n_pages);
if (atomic_read(&op->n_pages) <= 0)
fscache_op_complete(&op->op, true);
This causes two threads doing a decrement of n_pages to race with each
other seeing the op->refcount 0 at same time - and they end up calling
fscache_op_complete() in both the threads leading to an assertion failure.
Fix this by using atomic_sub_return() instead of two calls.
The oops looks something like:
FS-Cache: Assertion failed
FS-Cache: 0 > 0 is false
...
kernel BUG at /usr/src/linux-4.4.0/fs/fscache/operation.c:449!
...
Workqueue: fscache_operation fscache_op_work_func [fscache]
...
RIP: 0010:[<ffffffffc037eacd>] fscache_op_complete+0x10d/0x180 [fscache]
...
Call Trace:
[<ffffffffc1464cf9>] cachefiles_read_copier+0x3a9/0x410 [cachefiles]
[<ffffffffc037e272>] fscache_op_work_func+0x22/0x50 [fscache]
[<ffffffff81096da0>] process_one_work+0x150/0x3f0
[<ffffffff8109751a>] worker_thread+0x11a/0x470
[<ffffffff81808e59>] ? __schedule+0x359/0x980
[<ffffffff81097400>] ? rescuer_thread+0x310/0x310
[<ffffffff8109cdd6>] kthread+0xd6/0xf0
[<ffffffff8109cd00>] ? kthread_park+0x60/0x60
[<ffffffff8180d0cf>] ret_from_fork+0x3f/0x70
[<ffffffff8109cd00>] ? kthread_park+0x60/0x60
This seen this in 4.4.x kernels and the same bug affects fscache in latest
upstreams kernels.
Fixes: 1bb4b7f98f36 ("FS-Cache: The retrieval remaining-pages counter needs to be atomic_t")
Signed-off-by: Kiran Kumar Modukuri <[email protected]>
Signed-off-by: David Howells <[email protected]>
---
include/linux/fscache-cache.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/linux/fscache-cache.h b/include/linux/fscache-cache.h
index 34cf0fdd7dc7..bf98ed803af2 100644
--- a/include/linux/fscache-cache.h
+++ b/include/linux/fscache-cache.h
@@ -196,11 +196,11 @@ static inline void fscache_enqueue_retrieval(struct fscache_retrieval *op)
static inline void fscache_retrieval_complete(struct fscache_retrieval *op,
int n_pages)
{
- atomic_sub(n_pages, &op->n_pages);
- if (atomic_read(&op->n_pages) <= 0)
+ if (atomic_sub_return(n_pages, &op->n_pages) <= 0)
fscache_op_complete(&op->op, false);
}
+
/**
* fscache_put_retrieval - Drop a reference to a retrieval operation
* @op: The retrieval operation affected
From: Eric Sandeen <[email protected]>
fscache_set_key() can incur an out-of-bounds read, reported by KASAN:
BUG: KASAN: slab-out-of-bounds in fscache_alloc_cookie+0x5b3/0x680 [fscache]
Read of size 4 at addr ffff88084ff056d4 by task mount.nfs/32615
and also reported by syzbot at https://lkml.org/lkml/2018/7/8/236
BUG: KASAN: slab-out-of-bounds in fscache_set_key fs/fscache/cookie.c:120 [inline]
BUG: KASAN: slab-out-of-bounds in fscache_alloc_cookie+0x7a9/0x880 fs/fscache/cookie.c:171
Read of size 4 at addr ffff8801d3cc8bb4 by task syz-executor907/4466
This happens for any index_key_len which is not divisible by 4 and is
larger than the size of the inline key, because the code allocates exactly
index_key_len for the key buffer, but the hashing loop is stepping through
it 4 bytes (u32) at a time in the buf[] array.
Fix this by calculating how many u32 buffers we'll need by using
DIV_ROUND_UP, and then using kcalloc() to allocate a precleared allocation
buffer to hold the index_key, then using that same count as the hashing
index limit.
Fixes: ec0328e46d6e ("fscache: Maintain a catalogue of allocated cookies")
Reported-by: [email protected]
Signed-off-by: Eric Sandeen <[email protected]>
Signed-off-by: David Howells <[email protected]>
---
fs/fscache/cookie.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/fs/fscache/cookie.c b/fs/fscache/cookie.c
index b52f1dcd5dea..c550512ce335 100644
--- a/fs/fscache/cookie.c
+++ b/fs/fscache/cookie.c
@@ -70,7 +70,7 @@ void fscache_free_cookie(struct fscache_cookie *cookie)
}
/*
- * Set the index key in a cookie. The cookie struct has space for a 12-byte
+ * Set the index key in a cookie. The cookie struct has space for a 16-byte
* key plus length and hash, but if that's not big enough, it's instead a
* pointer to a buffer containing 3 bytes of hash, 1 byte of length and then
* the key data.
@@ -80,10 +80,13 @@ static int fscache_set_key(struct fscache_cookie *cookie,
{
unsigned long long h;
u32 *buf;
+ int bufs;
int i;
+ bufs = DIV_ROUND_UP(index_key_len, sizeof(*buf));
+
if (index_key_len > sizeof(cookie->inline_key)) {
- buf = kzalloc(index_key_len, GFP_KERNEL);
+ buf = kcalloc(bufs, sizeof(*buf), GFP_KERNEL);
if (!buf)
return -ENOMEM;
cookie->key = buf;
@@ -98,7 +101,8 @@ static int fscache_set_key(struct fscache_cookie *cookie,
*/
h = (unsigned long)cookie->parent;
h += index_key_len + cookie->type;
- for (i = 0; i < (index_key_len + sizeof(u32) - 1) / sizeof(u32); i++)
+
+ for (i = 0; i < bufs; i++)
h += buf[i];
cookie->key_hash = h ^ (h >> 32);
The inline key in struct rxrpc_cookie is insufficiently initialized,
zeroing only 3 of the 4 slots, therefore an index_key_len between 13 and 15
bytes will end up hashing uninitialized memory because the memcpy only
partially fills the last buf[] element.
Fix this by clearing fscache_cookie objects on allocation rather than using
the slab constructor to initialise them. We're going to pretty much fill
in the entire struct anyway, so bringing it into our dcache writably
shouldn't incur much overhead.
This removes the need to do clearance in fscache_set_key() (where we aren't
doing it correctly anyway).
Also, we don't need to set cookie->key_len in fscache_set_key() as we
already did it in the only caller, so remove that.
Fixes: ec0328e46d6e ("fscache: Maintain a catalogue of allocated cookies")
Reported-by: [email protected]
Reported-by: Eric Sandeen <[email protected]>
Signed-off-by: David Howells <[email protected]>
---
fs/fscache/cookie.c | 23 ++++-------------------
fs/fscache/internal.h | 1 -
fs/fscache/main.c | 4 +---
3 files changed, 5 insertions(+), 23 deletions(-)
diff --git a/fs/fscache/cookie.c b/fs/fscache/cookie.c
index 83bfe04456b6..b52f1dcd5dea 100644
--- a/fs/fscache/cookie.c
+++ b/fs/fscache/cookie.c
@@ -69,19 +69,6 @@ void fscache_free_cookie(struct fscache_cookie *cookie)
}
}
-/*
- * initialise an cookie jar slab element prior to any use
- */
-void fscache_cookie_init_once(void *_cookie)
-{
- struct fscache_cookie *cookie = _cookie;
-
- memset(cookie, 0, sizeof(*cookie));
- spin_lock_init(&cookie->lock);
- spin_lock_init(&cookie->stores_lock);
- INIT_HLIST_HEAD(&cookie->backing_objects);
-}
-
/*
* Set the index key in a cookie. The cookie struct has space for a 12-byte
* key plus length and hash, but if that's not big enough, it's instead a
@@ -95,8 +82,6 @@ static int fscache_set_key(struct fscache_cookie *cookie,
u32 *buf;
int i;
- cookie->key_len = index_key_len;
-
if (index_key_len > sizeof(cookie->inline_key)) {
buf = kzalloc(index_key_len, GFP_KERNEL);
if (!buf)
@@ -104,9 +89,6 @@ static int fscache_set_key(struct fscache_cookie *cookie,
cookie->key = buf;
} else {
buf = (u32 *)cookie->inline_key;
- buf[0] = 0;
- buf[1] = 0;
- buf[2] = 0;
}
memcpy(buf, index_key, index_key_len);
@@ -161,7 +143,7 @@ struct fscache_cookie *fscache_alloc_cookie(
struct fscache_cookie *cookie;
/* allocate and initialise a cookie */
- cookie = kmem_cache_alloc(fscache_cookie_jar, GFP_KERNEL);
+ cookie = kmem_cache_zalloc(fscache_cookie_jar, GFP_KERNEL);
if (!cookie)
return NULL;
@@ -192,6 +174,9 @@ struct fscache_cookie *fscache_alloc_cookie(
cookie->netfs_data = netfs_data;
cookie->flags = (1 << FSCACHE_COOKIE_NO_DATA_YET);
cookie->type = def->type;
+ spin_lock_init(&cookie->lock);
+ spin_lock_init(&cookie->stores_lock);
+ INIT_HLIST_HEAD(&cookie->backing_objects);
/* radix tree insertion won't use the preallocation pool unless it's
* told it may not wait */
diff --git a/fs/fscache/internal.h b/fs/fscache/internal.h
index f83328a7f048..d6209022e965 100644
--- a/fs/fscache/internal.h
+++ b/fs/fscache/internal.h
@@ -51,7 +51,6 @@ extern struct fscache_cache *fscache_select_cache_for_object(
extern struct kmem_cache *fscache_cookie_jar;
extern void fscache_free_cookie(struct fscache_cookie *);
-extern void fscache_cookie_init_once(void *);
extern struct fscache_cookie *fscache_alloc_cookie(struct fscache_cookie *,
const struct fscache_cookie_def *,
const void *, size_t,
diff --git a/fs/fscache/main.c b/fs/fscache/main.c
index 7dce110bf17d..30ad89db1efc 100644
--- a/fs/fscache/main.c
+++ b/fs/fscache/main.c
@@ -143,9 +143,7 @@ static int __init fscache_init(void)
fscache_cookie_jar = kmem_cache_create("fscache_cookie_jar",
sizeof(struct fscache_cookie),
- 0,
- 0,
- fscache_cookie_init_once);
+ 0, 0, NULL);
if (!fscache_cookie_jar) {
pr_notice("Failed to allocate a cookie jar\n");
ret = -ENOMEM;
Hi David,
On Wed, Oct 17, 2018 at 03:23:38PM +0100, David Howells wrote:
> From: kiran.modukuri <[email protected]>
>
> The code in fscache_retrieval_complete is using atomic_sub followed by an
> atomic_read:
>
> atomic_sub(n_pages, &op->n_pages);
> if (atomic_read(&op->n_pages) <= 0)
> fscache_op_complete(&op->op, true);
>
> This causes two threads doing a decrement of n_pages to race with each
> other seeing the op->refcount 0 at same time - and they end up calling
> fscache_op_complete() in both the threads leading to an assertion failure.
>
> Fix this by using atomic_sub_return() instead of two calls.
Seems a case for atomic_sub_return_relaxed()... why not?
Andrea
Andrea Parri <[email protected]> wrote:
> > Fix this by using atomic_sub_return() instead of two calls.
>
> Seems a case for atomic_sub_return_relaxed()... why not?
Ummm... In that case, should it be atomic_sub_return_release()?
David
On Wed, Oct 17, 2018 at 04:32:13PM +0100, David Howells wrote:
> Andrea Parri <[email protected]> wrote:
>
> > > Fix this by using atomic_sub_return() instead of two calls.
> >
> > Seems a case for atomic_sub_return_relaxed()... why not?
>
> Ummm... In that case, should it be atomic_sub_return_release()?
Hard to tell for me: your diff./changelog is all I know about fs-cache
... (and this suggests -no-, given that atomic_sub() and atomic_read()
provide no ordering...); good question though. ;-)
Andrea
>
> David
On Wed, Oct 17, 2018 at 03:23:14PM +0100, David Howells wrote:
>
> Attached are another couple of miscellaneous fixes for FS-Cache and
> CacheFiles:
>
> (1) Fix a race between object burial in cachefiles and external rmdir.
>
> (2) Fix a race from a split atomic op.
>
> (3) Fix incomplete initialisation of cookie key space.
>
> (4) Fix out-of-bounds read.
Patches 1, 3, and 4 are now merged, thanks. I didn't pull from you as
patch 2 needed to be dropped according to the thread.
thanks,
greg k-h
Andrea Parri <[email protected]> wrote:
> > > > Fix this by using atomic_sub_return() instead of two calls.
> > >
> > > Seems a case for atomic_sub_return_relaxed()... why not?
> >
> > Ummm... In that case, should it be atomic_sub_return_release()?
>
> Hard to tell for me: your diff./changelog is all I know about fs-cache
> ... (and this suggests -no-, given that atomic_sub() and atomic_read()
> provide no ordering...); good question though. ;-)
Yeah, that doesn't mean that it shouldn't be stricter than 'relaxed'. It's
kind of like an unlock/release operation, so I think 'release' is probably the
minimum requirement.
David
On Mon, Nov 26, 2018 at 04:26:36PM +0000, David Howells wrote:
> Andrea Parri <[email protected]> wrote:
>
> > > > > Fix this by using atomic_sub_return() instead of two calls.
> > > >
> > > > Seems a case for atomic_sub_return_relaxed()... why not?
> > >
> > > Ummm... In that case, should it be atomic_sub_return_release()?
> >
> > Hard to tell for me: your diff./changelog is all I know about fs-cache
> > ... (and this suggests -no-, given that atomic_sub() and atomic_read()
> > provide no ordering...); good question though. ;-)
>
> Yeah, that doesn't mean that it shouldn't be stricter than 'relaxed'. It's
> kind of like an unlock/release operation, so I think 'release' is probably the
> minimum requirement.
Sure. My point was: those operations are currently not atomic _and_
they provide no ordering; I think that the above commit message does
a good work in explaining *why* we need atomicity, but can't say the
same for the memory-ordering requirement.
Andrea
>
> David
Andrea Parri <[email protected]> wrote:
> > > > > > Fix this by using atomic_sub_return() instead of two calls.
> > > > >
> > > > > Seems a case for atomic_sub_return_relaxed()... why not?
> > > >
> > > > Ummm... In that case, should it be atomic_sub_return_release()?
> > >
> > > Hard to tell for me: your diff./changelog is all I know about fs-cache
> > > ... (and this suggests -no-, given that atomic_sub() and atomic_read()
> > > provide no ordering...); good question though. ;-)
> >
> > Yeah, that doesn't mean that it shouldn't be stricter than 'relaxed'.
> > It's kind of like an unlock/release operation, so I think 'release' is
> > probably the minimum requirement.
>
> Sure. My point was: those operations are currently not atomic _and_
> they provide no ordering; I think that the above commit message does
> a good work in explaining *why* we need atomicity, but can't say the
> same for the memory-ordering requirement.
Having discussed it with Paul McKenney and thought about it some more, I think
relaxed is probably okay since there isn't a pair of variables that need
ordering.
David
> > Sure. My point was: those operations are currently not atomic _and_
> > they provide no ordering; I think that the above commit message does
> > a good work in explaining *why* we need atomicity, but can't say the
> > same for the memory-ordering requirement.
>
> Having discussed it with Paul McKenney and thought about it some more, I think
> relaxed is probably okay since there isn't a pair of variables that need
> ordering.
Count several troubled, and exiting!, weekends spent "processing" (my)
conversations with Paul... so been there! ;-)
Makes all sense to me of course, thank you for the clarification.
Andrea
>
> David