* [PATCH 1/6] afs: Use struct_size() in kzalloc()
2019-01-17 15:27 [PATCH 0/6] AFS fixes David Howells
@ 2019-01-17 15:27 ` David Howells
2019-01-17 15:27 ` [PATCH 2/6] afs: Set correct lock type for the yfs CreateFile David Howells
` (5 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: David Howells @ 2019-01-17 15:27 UTC (permalink / raw)
To: torvalds; +Cc: Gustavo A. R. Silva, dhowells, linux-afs, linux-kernel
From: Gustavo A. R. Silva <gustavo@embeddedor.com>
One of the more common cases of allocation size calculations is finding the
size of a structure that has a zero-sized array at the end, along with
memory for some number of elements for that array. For example:
struct foo {
int stuff;
void *entry[];
};
instance = kzalloc(sizeof(struct foo) + sizeof(void *) * count, GFP_KERNEL);
Instead of leaving these open-coded and prone to type mistakes, we can now
use the new struct_size() helper:
instance = kzalloc(struct_size(instance, entry, count), GFP_KERNEL);
This code was detected with the help of Coccinelle.
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
Signed-off-by: David Howells <dhowells@redhat.com>
---
fs/afs/server_list.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/fs/afs/server_list.c b/fs/afs/server_list.c
index 95d0761cdb34..155dc14caef9 100644
--- a/fs/afs/server_list.c
+++ b/fs/afs/server_list.c
@@ -42,9 +42,7 @@ struct afs_server_list *afs_alloc_server_list(struct afs_cell *cell,
if (vldb->fs_mask[i] & type_mask)
nr_servers++;
- slist = kzalloc(sizeof(struct afs_server_list) +
- sizeof(struct afs_server_entry) * nr_servers,
- GFP_KERNEL);
+ slist = kzalloc(struct_size(slist, servers, nr_servers), GFP_KERNEL);
if (!slist)
goto error;
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 2/6] afs: Set correct lock type for the yfs CreateFile
2019-01-17 15:27 [PATCH 0/6] AFS fixes David Howells
2019-01-17 15:27 ` [PATCH 1/6] afs: Use struct_size() in kzalloc() David Howells
@ 2019-01-17 15:27 ` David Howells
2019-01-17 15:27 ` [PATCH 3/6] afs: Don't set vnode->cb_s_break in afs_validate() David Howells
` (4 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: David Howells @ 2019-01-17 15:27 UTC (permalink / raw)
To: torvalds; +Cc: Marc Dionne, dhowells, linux-afs, linux-kernel
From: Marc Dionne <marc.dionne@auristor.com>
A lock type of 0 is "LockRead", which makes the fileserver record an
unintentional read lock on the new file. This will cause problems
later on if the file is the subject of locking operations.
The correct default value should be -1 ("LockNone").
Fix the operation marshalling code to set the value and provide an enum to
symbolise the values whilst we're at it.
Fixes: 30062bd13e36 ("afs: Implement YFS support in the fs client")
Signed-off-by: Marc Dionne <marc.dionne@auristor.com>
Signed-off-by: David Howells <dhowells@redhat.com>
---
fs/afs/protocol_yfs.h | 11 +++++++++++
fs/afs/yfsclient.c | 2 +-
2 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/fs/afs/protocol_yfs.h b/fs/afs/protocol_yfs.h
index 07bc10f076aa..d443e2bfa094 100644
--- a/fs/afs/protocol_yfs.h
+++ b/fs/afs/protocol_yfs.h
@@ -161,3 +161,14 @@ struct yfs_xdr_YFSStoreVolumeStatus {
struct yfs_xdr_u64 max_quota;
struct yfs_xdr_u64 file_quota;
} __packed;
+
+enum yfs_lock_type {
+ yfs_LockNone = -1,
+ yfs_LockRead = 0,
+ yfs_LockWrite = 1,
+ yfs_LockExtend = 2,
+ yfs_LockRelease = 3,
+ yfs_LockMandatoryRead = 0x100,
+ yfs_LockMandatoryWrite = 0x101,
+ yfs_LockMandatoryExtend = 0x102,
+};
diff --git a/fs/afs/yfsclient.c b/fs/afs/yfsclient.c
index 12658c1363ae..5aa57929e8c2 100644
--- a/fs/afs/yfsclient.c
+++ b/fs/afs/yfsclient.c
@@ -803,7 +803,7 @@ int yfs_fs_create_file(struct afs_fs_cursor *fc,
bp = xdr_encode_YFSFid(bp, &vnode->fid);
bp = xdr_encode_string(bp, name, namesz);
bp = xdr_encode_YFSStoreStatus_mode(bp, mode);
- bp = xdr_encode_u32(bp, 0); /* ViceLockType */
+ bp = xdr_encode_u32(bp, yfs_LockNone); /* ViceLockType */
yfs_check_req(call, bp);
afs_use_fs_server(call, fc->cbi);
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 3/6] afs: Don't set vnode->cb_s_break in afs_validate()
2019-01-17 15:27 [PATCH 0/6] AFS fixes David Howells
2019-01-17 15:27 ` [PATCH 1/6] afs: Use struct_size() in kzalloc() David Howells
2019-01-17 15:27 ` [PATCH 2/6] afs: Set correct lock type for the yfs CreateFile David Howells
@ 2019-01-17 15:27 ` David Howells
2019-01-17 15:27 ` [PATCH 4/6] afs: Fix key refcounting in file locking code David Howells
` (3 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: David Howells @ 2019-01-17 15:27 UTC (permalink / raw)
To: torvalds; +Cc: Marc Dionne, dhowells, linux-afs, linux-kernel
From: Marc Dionne <marc.dionne@auristor.com>
A cb_interest record is not necessarily attached to the vnode on entry to
afs_validate(), which can cause an oops when we try to bring the vnode's
cb_s_break up to date in the default case (ie. no current callback promise
and the vnode has not been deleted).
Fix this by simply removing the line, as vnode->cb_s_break will be set when
needed by afs_register_server_cb_interest() when we next get a callback
promise from RPC call.
The oops looks something like:
BUG: unable to handle kernel NULL pointer dereference at 0000000000000018
...
RIP: 0010:afs_validate+0x66/0x250 [kafs]
...
Call Trace:
afs_d_revalidate+0x8d/0x340 [kafs]
? __d_lookup+0x61/0x150
lookup_dcache+0x44/0x70
? lookup_dcache+0x44/0x70
__lookup_hash+0x24/0xa0
do_unlinkat+0x11d/0x2c0
__x64_sys_unlink+0x23/0x30
do_syscall_64+0x4d/0xf0
entry_SYSCALL_64_after_hwframe+0x44/0xa9
Fixes: ae3b7361dc0e ("afs: Fix validation/callback interaction")
Signed-off-by: Marc Dionne <marc.dionne@auristor.com>
Signed-off-by: David Howells <dhowells@redhat.com>
---
fs/afs/inode.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/fs/afs/inode.c b/fs/afs/inode.c
index 6b17d3620414..211343831c30 100644
--- a/fs/afs/inode.c
+++ b/fs/afs/inode.c
@@ -414,7 +414,6 @@ int afs_validate(struct afs_vnode *vnode, struct key *key)
} else if (test_bit(AFS_VNODE_DELETED, &vnode->flags)) {
valid = true;
} else {
- vnode->cb_s_break = vnode->cb_interest->server->cb_s_break;
vnode->cb_v_break = vnode->volume->cb_v_break;
valid = false;
}
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 4/6] afs: Fix key refcounting in file locking code
2019-01-17 15:27 [PATCH 0/6] AFS fixes David Howells
` (2 preceding siblings ...)
2019-01-17 15:27 ` [PATCH 3/6] afs: Don't set vnode->cb_s_break in afs_validate() David Howells
@ 2019-01-17 15:27 ` David Howells
2019-01-17 15:28 ` [PATCH 5/6] afs: Provide a function to get a ref on a call David Howells
` (2 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: David Howells @ 2019-01-17 15:27 UTC (permalink / raw)
To: torvalds; +Cc: dhowells, linux-afs, linux-kernel
Fix the refcounting of the authentication keys in the file locking code.
The vnode->lock_key member points to a key on which it expects to be
holding a ref, but it isn't always given an extra ref, however.
Fixes: 0fafdc9f888b ("afs: Fix file locking")
Signed-off-by: David Howells <dhowells@redhat.com>
---
fs/afs/flock.c | 4 ++--
fs/afs/inode.c | 2 ++
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/fs/afs/flock.c b/fs/afs/flock.c
index 0568fd986821..e432bd27a2e7 100644
--- a/fs/afs/flock.c
+++ b/fs/afs/flock.c
@@ -208,7 +208,7 @@ void afs_lock_work(struct work_struct *work)
/* The new front of the queue now owns the state variables. */
next = list_entry(vnode->pending_locks.next,
struct file_lock, fl_u.afs.link);
- vnode->lock_key = afs_file_key(next->fl_file);
+ vnode->lock_key = key_get(afs_file_key(next->fl_file));
vnode->lock_type = (next->fl_type == F_RDLCK) ? AFS_LOCK_READ : AFS_LOCK_WRITE;
vnode->lock_state = AFS_VNODE_LOCK_WAITING_FOR_CB;
goto again;
@@ -413,7 +413,7 @@ static void afs_dequeue_lock(struct afs_vnode *vnode, struct file_lock *fl)
/* The new front of the queue now owns the state variables. */
next = list_entry(vnode->pending_locks.next,
struct file_lock, fl_u.afs.link);
- vnode->lock_key = afs_file_key(next->fl_file);
+ vnode->lock_key = key_get(afs_file_key(next->fl_file));
vnode->lock_type = (next->fl_type == F_RDLCK) ? AFS_LOCK_READ : AFS_LOCK_WRITE;
vnode->lock_state = AFS_VNODE_LOCK_WAITING_FOR_CB;
afs_lock_may_be_available(vnode);
diff --git a/fs/afs/inode.c b/fs/afs/inode.c
index 211343831c30..1a4ce07fb406 100644
--- a/fs/afs/inode.c
+++ b/fs/afs/inode.c
@@ -545,6 +545,8 @@ void afs_evict_inode(struct inode *inode)
#endif
afs_put_permits(rcu_access_pointer(vnode->permit_cache));
+ key_put(vnode->lock_key);
+ vnode->lock_key = NULL;
_leave("");
}
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 5/6] afs: Provide a function to get a ref on a call
2019-01-17 15:27 [PATCH 0/6] AFS fixes David Howells
` (3 preceding siblings ...)
2019-01-17 15:27 ` [PATCH 4/6] afs: Fix key refcounting in file locking code David Howells
@ 2019-01-17 15:28 ` David Howells
2019-01-17 15:28 ` [PATCH 6/6] afs: Fix race in async call refcounting David Howells
2019-01-17 18:29 ` [PATCH 0/6] AFS fixes Linus Torvalds
6 siblings, 0 replies; 10+ messages in thread
From: David Howells @ 2019-01-17 15:28 UTC (permalink / raw)
To: torvalds; +Cc: dhowells, linux-afs, linux-kernel
Provide a function to get a reference on an afs_call struct.
Signed-off-by: David Howells <dhowells@redhat.com>
---
fs/afs/rxrpc.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/fs/afs/rxrpc.c b/fs/afs/rxrpc.c
index a7b44863d502..4830e0a6bf1d 100644
--- a/fs/afs/rxrpc.c
+++ b/fs/afs/rxrpc.c
@@ -203,20 +203,26 @@ void afs_put_call(struct afs_call *call)
}
}
+static struct afs_call *afs_get_call(struct afs_call *call,
+ enum afs_call_trace why)
+{
+ int u = atomic_inc_return(&call->usage);
+
+ trace_afs_call(call, why, u,
+ atomic_read(&call->net->nr_outstanding_calls),
+ __builtin_return_address(0));
+ return call;
+}
+
/*
* Queue the call for actual work.
*/
static void afs_queue_call_work(struct afs_call *call)
{
if (call->type->work) {
- int u = atomic_inc_return(&call->usage);
-
- trace_afs_call(call, afs_call_trace_work, u,
- atomic_read(&call->net->nr_outstanding_calls),
- __builtin_return_address(0));
-
INIT_WORK(&call->work, call->type->work);
+ afs_get_call(call, afs_call_trace_work);
if (!queue_work(afs_wq, &call->work))
afs_put_call(call);
}
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 6/6] afs: Fix race in async call refcounting
2019-01-17 15:27 [PATCH 0/6] AFS fixes David Howells
` (4 preceding siblings ...)
2019-01-17 15:28 ` [PATCH 5/6] afs: Provide a function to get a ref on a call David Howells
@ 2019-01-17 15:28 ` David Howells
2019-01-17 18:29 ` [PATCH 0/6] AFS fixes Linus Torvalds
6 siblings, 0 replies; 10+ messages in thread
From: David Howells @ 2019-01-17 15:28 UTC (permalink / raw)
To: torvalds; +Cc: dhowells, linux-afs, linux-kernel
There's a race between afs_make_call() and afs_wake_up_async_call() in the
case that an error is returned from rxrpc_kernel_send_data() after it has
queued the final packet.
afs_make_call() will try and clean up the mess, but the call state may have
been moved on thereby causing afs_process_async_call() to also try and to
delete the call.
Fix this by:
(1) Getting an extra ref for an asynchronous call for the call itself to
hold. This makes sure the call doesn't evaporate on us accidentally
and will allow the call to be retained by the caller in a future
patch. The ref is released on leaving afs_make_call() or
afs_wait_for_call_to_complete().
(2) In the event of an error from rxrpc_kernel_send_data():
(a) Don't set the call state to AFS_CALL_COMPLETE until *after* the
call has been aborted and ended. This prevents
afs_deliver_to_call() from doing anything with any notifications
it gets.
(b) Explicitly end the call immediately to prevent further callbacks.
(c) Cancel any queued async_work and wait for the work if it's
executing. This allows us to be sure the race won't recur when we
change the state. We put the work queue's ref on the call if we
managed to cancel it.
(d) Put the call's ref that we got in (1). This belongs to us as long
as the call is in state AFS_CALL_CL_REQUESTING.
Fixes: 341f741f04be ("afs: Refcount the afs_call struct")
Signed-off-by: David Howells <dhowells@redhat.com>
---
fs/afs/rxrpc.c | 35 ++++++++++++++++++++++++++++++-----
include/trace/events/afs.h | 2 ++
2 files changed, 32 insertions(+), 5 deletions(-)
diff --git a/fs/afs/rxrpc.c b/fs/afs/rxrpc.c
index 4830e0a6bf1d..2c588f9bbbda 100644
--- a/fs/afs/rxrpc.c
+++ b/fs/afs/rxrpc.c
@@ -23,6 +23,7 @@ struct workqueue_struct *afs_async_calls;
static void afs_wake_up_call_waiter(struct sock *, struct rxrpc_call *, unsigned long);
static long afs_wait_for_call_to_complete(struct afs_call *, struct afs_addr_cursor *);
static void afs_wake_up_async_call(struct sock *, struct rxrpc_call *, unsigned long);
+static void afs_delete_async_call(struct work_struct *);
static void afs_process_async_call(struct work_struct *);
static void afs_rx_new_call(struct sock *, struct rxrpc_call *, unsigned long);
static void afs_rx_discard_new_call(struct rxrpc_call *, unsigned long);
@@ -404,6 +405,12 @@ long afs_make_call(struct afs_addr_cursor *ac, struct afs_call *call,
}
}
+ /* If the call is going to be asynchronous, we need an extra ref for
+ * the call to hold itself so the caller need not hang on to its ref.
+ */
+ if (call->async)
+ afs_get_call(call, afs_call_trace_get);
+
/* create a call */
rxcall = rxrpc_kernel_begin_call(call->net->socket, srx, call->key,
(unsigned long)call,
@@ -444,15 +451,17 @@ long afs_make_call(struct afs_addr_cursor *ac, struct afs_call *call,
goto error_do_abort;
}
- /* at this point, an async call may no longer exist as it may have
- * already completed */
- if (call->async)
+ /* Note that at this point, we may have received the reply or an abort
+ * - and an asynchronous call may already have completed.
+ */
+ if (call->async) {
+ afs_put_call(call);
return -EINPROGRESS;
+ }
return afs_wait_for_call_to_complete(call, ac);
error_do_abort:
- call->state = AFS_CALL_COMPLETE;
if (ret != -ECONNABORTED) {
rxrpc_kernel_abort_call(call->net->socket, rxcall,
RX_USER_ABORT, ret, "KSD");
@@ -469,8 +478,24 @@ long afs_make_call(struct afs_addr_cursor *ac, struct afs_call *call,
error_kill_call:
if (call->type->done)
call->type->done(call);
- afs_put_call(call);
+
+ /* We need to dispose of the extra ref we grabbed for an async call.
+ * The call, however, might be queued on afs_async_calls and we need to
+ * make sure we don't get any more notifications that might requeue it.
+ */
+ if (call->rxcall) {
+ rxrpc_kernel_end_call(call->net->socket, call->rxcall);
+ call->rxcall = NULL;
+ }
+ if (call->async) {
+ if (cancel_work_sync(&call->async_work))
+ afs_put_call(call);
+ afs_put_call(call);
+ }
+
ac->error = ret;
+ call->state = AFS_CALL_COMPLETE;
+ afs_put_call(call);
_leave(" = %d", ret);
return ret;
}
diff --git a/include/trace/events/afs.h b/include/trace/events/afs.h
index 33d291888ba9..e3f005eae1f7 100644
--- a/include/trace/events/afs.h
+++ b/include/trace/events/afs.h
@@ -25,6 +25,7 @@
enum afs_call_trace {
afs_call_trace_alloc,
afs_call_trace_free,
+ afs_call_trace_get,
afs_call_trace_put,
afs_call_trace_wake,
afs_call_trace_work,
@@ -159,6 +160,7 @@ enum afs_file_error {
#define afs_call_traces \
EM(afs_call_trace_alloc, "ALLOC") \
EM(afs_call_trace_free, "FREE ") \
+ EM(afs_call_trace_get, "GET ") \
EM(afs_call_trace_put, "PUT ") \
EM(afs_call_trace_wake, "WAKE ") \
E_(afs_call_trace_work, "WORK ")
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 0/6] AFS fixes
2019-01-17 15:27 [PATCH 0/6] AFS fixes David Howells
` (5 preceding siblings ...)
2019-01-17 15:28 ` [PATCH 6/6] afs: Fix race in async call refcounting David Howells
@ 2019-01-17 18:29 ` Linus Torvalds
2019-01-17 18:31 ` Linus Torvalds
6 siblings, 1 reply; 10+ messages in thread
From: Linus Torvalds @ 2019-01-17 18:29 UTC (permalink / raw)
To: David Howells
Cc: Gustavo A. R. Silva, Marc Dionne, linux-afs, Linux List Kernel Mailing
On Fri, Jan 18, 2019 at 3:27 AM David Howells <dhowells@redhat.com> wrote:
>
> Here's a set of fixes for AFS:
Can you please use the proper pull request format?
Not only didn't this show in my usual "git pull" search, but this:
> http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git
> tag afs-fixes-20190117
is not valid pull address.. Yes, I can fix up the thing (and have
done so), but a normal git pull-request wouldn't have had that issue.
Linus
^ permalink raw reply [flat|nested] 10+ messages in thread