* [PATCH 0/2] afs: Fixes
@ 2017-12-01 12:06 David Howells
2017-12-01 12:06 ` [PATCH 1/2] afs: Fix permit refcounting David Howells
2017-12-01 12:06 ` [PATCH 2/2] afs: Properly reset afs_vnode (inode) fields David Howells
0 siblings, 2 replies; 3+ messages in thread
From: David Howells @ 2017-12-01 12:06 UTC (permalink / raw)
To: torvalds; +Cc: dhowells, linux-fsdevel, linux-afs, linux-kernel
Hi Linus,
Here are two fix patches for the AFS filesystem:
(1) Fix the refcounting on permit caching.
(2) AFS inode (afs_vnode) fields need resetting after allocation because
they're only initialised when slab pages are obtained from the page
allocator.
The patches can be found here also:
http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=afs-fixes
tagged thusly:
afs-fixes-20171201
David
---
David Howells (2):
afs: Fix permit refcounting
afs: Properly reset afs_vnode (inode) fields
fs/afs/internal.h | 5 ++++-
fs/afs/security.c | 18 ++++++++++--------
fs/afs/super.c | 14 +++++++++++++-
3 files changed, 27 insertions(+), 10 deletions(-)
^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH 1/2] afs: Fix permit refcounting
2017-12-01 12:06 [PATCH 0/2] afs: Fixes David Howells
@ 2017-12-01 12:06 ` David Howells
2017-12-01 12:06 ` [PATCH 2/2] afs: Properly reset afs_vnode (inode) fields David Howells
1 sibling, 0 replies; 3+ messages in thread
From: David Howells @ 2017-12-01 12:06 UTC (permalink / raw)
To: torvalds; +Cc: Marc Dionne, dhowells, linux-fsdevel, linux-afs, linux-kernel
Fix four refcount bugs in afs_cache_permit():
(1) When checking the result of the kzalloc(), we can't just return, but
must put 'permits'.
(2) We shouldn't put permits immediately after hashing a new permit as we
need to keep the pointer stable so that we can check to see if
vnode->permit_cache has changed before we decide whether to assign to
it.
(3) 'permits' is being put twice.
(4) We need to put either the replacement or the thing replaced after the
assignment to vnode->permit_cache.
Without this, lots of the following are seen:
Kernel BUG at ffffffffa039857b [verbose debug info unavailable]
------------[ cut here ]------------
Kernel BUG at ffffffffa039858a [verbose debug info unavailable]
------------[ cut here ]------------
The addresses are in the .text..refcount section of the kafs.ko module.
Following the relocation records for the __ex_table section shows one to be
due to the decrement in afs_put_permits() and the other to be key_get() in
afs_cache_permit().
Occasionally, the following is seen:
refcount_t overflow at afs_cache_permit+0x57d/0x5c0 [kafs] in cc1[562], uid/euid: 0/0
WARNING: CPU: 0 PID: 562 at kernel/panic.c:657 refcount_error_report+0x9c/0xac
...
Reported-by: Marc Dionne <marc.dionne@auristor.com>
Signed-off-by: David Howells <dhowells@redhat.com>
Tested-by: Marc Dionne <marc.dionne@auristor.com>
---
fs/afs/security.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/fs/afs/security.c b/fs/afs/security.c
index 2b00097101b3..b88b7d45fdaa 100644
--- a/fs/afs/security.c
+++ b/fs/afs/security.c
@@ -120,7 +120,7 @@ static void afs_hash_permits(struct afs_permits *permits)
void afs_cache_permit(struct afs_vnode *vnode, struct key *key,
unsigned int cb_break)
{
- struct afs_permits *permits, *xpermits, *replacement, *new = NULL;
+ struct afs_permits *permits, *xpermits, *replacement, *zap, *new = NULL;
afs_access_t caller_access = READ_ONCE(vnode->status.caller_access);
size_t size = 0;
bool changed = false;
@@ -204,7 +204,7 @@ void afs_cache_permit(struct afs_vnode *vnode, struct key *key,
new = kzalloc(sizeof(struct afs_permits) +
sizeof(struct afs_permit) * size, GFP_NOFS);
if (!new)
- return;
+ goto out_put;
refcount_set(&new->usage, 1);
new->nr_permits = size;
@@ -229,8 +229,6 @@ void afs_cache_permit(struct afs_vnode *vnode, struct key *key,
afs_hash_permits(new);
- afs_put_permits(permits);
-
/* Now see if the permit list we want is actually already available */
spin_lock(&afs_permits_lock);
@@ -262,11 +260,15 @@ void afs_cache_permit(struct afs_vnode *vnode, struct key *key,
kfree(new);
spin_lock(&vnode->lock);
- if (cb_break != (vnode->cb_break + vnode->cb_interest->server->cb_s_break) ||
- permits != rcu_access_pointer(vnode->permit_cache))
- goto someone_else_changed_it_unlock;
- rcu_assign_pointer(vnode->permit_cache, replacement);
+ zap = rcu_access_pointer(vnode->permit_cache);
+ if (cb_break == (vnode->cb_break + vnode->cb_interest->server->cb_s_break) &&
+ zap == permits)
+ rcu_assign_pointer(vnode->permit_cache, replacement);
+ else
+ zap = replacement;
spin_unlock(&vnode->lock);
+ afs_put_permits(zap);
+out_put:
afs_put_permits(permits);
return;
^ permalink raw reply related [flat|nested] 3+ messages in thread
* [PATCH 2/2] afs: Properly reset afs_vnode (inode) fields
2017-12-01 12:06 [PATCH 0/2] afs: Fixes David Howells
2017-12-01 12:06 ` [PATCH 1/2] afs: Fix permit refcounting David Howells
@ 2017-12-01 12:06 ` David Howells
1 sibling, 0 replies; 3+ messages in thread
From: David Howells @ 2017-12-01 12:06 UTC (permalink / raw)
To: torvalds; +Cc: Marc Dionne, dhowells, linux-fsdevel, linux-afs, linux-kernel
When an AFS inode is allocated by afs_alloc_inode(), the allocated
afs_vnode struct isn't necessarily reset from the last time it was used as
an inode because the slab constructor is only invoked once when the memory
is obtained from the page allocator.
This means that information can leak from one inode to the next because
we're not calling kmem_cache_zalloc(). Some of the information isn't
reset, in particular the permit cache pointer.
Bring the clearances up to date.
Signed-off-by: David Howells <dhowells@redhat.com>
Tested-by: Marc Dionne <marc.dionne@auristor.com>
---
fs/afs/internal.h | 5 ++++-
fs/afs/super.c | 14 +++++++++++++-
2 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/fs/afs/internal.h b/fs/afs/internal.h
index e03910cebdd4..804d1f905622 100644
--- a/fs/afs/internal.h
+++ b/fs/afs/internal.h
@@ -441,7 +441,10 @@ enum afs_lock_state {
};
/*
- * AFS inode private data
+ * AFS inode private data.
+ *
+ * Note that afs_alloc_inode() *must* reset anything that could incorrectly
+ * leak from one inode to another.
*/
struct afs_vnode {
struct inode vfs_inode; /* the VFS's inode record */
diff --git a/fs/afs/super.c b/fs/afs/super.c
index d3f97da61bdf..1037dd41a622 100644
--- a/fs/afs/super.c
+++ b/fs/afs/super.c
@@ -536,7 +536,9 @@ static void afs_kill_super(struct super_block *sb)
}
/*
- * initialise an inode cache slab element prior to any use
+ * Initialise an inode cache slab element prior to any use. Note that
+ * afs_alloc_inode() *must* reset anything that could incorrectly leak from one
+ * inode to another.
*/
static void afs_i_init_once(void *_vnode)
{
@@ -568,11 +570,21 @@ static struct inode *afs_alloc_inode(struct super_block *sb)
atomic_inc(&afs_count_active_inodes);
+ /* Reset anything that shouldn't leak from one inode to the next. */
memset(&vnode->fid, 0, sizeof(vnode->fid));
memset(&vnode->status, 0, sizeof(vnode->status));
vnode->volume = NULL;
+ vnode->lock_key = NULL;
+ vnode->permit_cache = NULL;
+ vnode->cb_interest = NULL;
+#ifdef CONFIG_AFS_FSCACHE
+ vnode->cache = NULL;
+#endif
+
vnode->flags = 1 << AFS_VNODE_UNSET;
+ vnode->cb_type = 0;
+ vnode->lock_state = AFS_VNODE_LOCK_NONE;
_leave(" = %p", &vnode->vfs_inode);
return &vnode->vfs_inode;
^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-12-01 12:06 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-01 12:06 [PATCH 0/2] afs: Fixes David Howells
2017-12-01 12:06 ` [PATCH 1/2] afs: Fix permit refcounting David Howells
2017-12-01 12:06 ` [PATCH 2/2] afs: Properly reset afs_vnode (inode) fields David Howells
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).