From c56f9ec8b20f931014574b943590c4d830109380 Mon Sep 17 00:00:00 2001 From: David Howells Date: Wed, 6 Jul 2022 10:52:14 +0100 Subject: afs: Use refcount_t rather than atomic_t Use refcount_t rather than atomic_t in afs to make use of the count checking facilities provided. Signed-off-by: David Howells Reviewed-by: Marc Dionne cc: linux-afs@lists.infradead.org Link: https://lore.kernel.org/r/165911277768.3745403.423349776836296452.stgit@warthog.procyon.org.uk/ # v1 --- fs/afs/server.c | 40 +++++++++++++++++++++++----------------- 1 file changed, 23 insertions(+), 17 deletions(-) (limited to 'fs/afs/server.c') diff --git a/fs/afs/server.c b/fs/afs/server.c index 6e5b9a19b234..ffed828622b6 100644 --- a/fs/afs/server.c +++ b/fs/afs/server.c @@ -228,7 +228,7 @@ static struct afs_server *afs_alloc_server(struct afs_cell *cell, if (!server) goto enomem; - atomic_set(&server->ref, 1); + refcount_set(&server->ref, 1); atomic_set(&server->active, 1); server->debug_id = atomic_inc_return(&afs_server_debug_id); RCU_INIT_POINTER(server->addresses, alist); @@ -352,9 +352,10 @@ void afs_servers_timer(struct timer_list *timer) struct afs_server *afs_get_server(struct afs_server *server, enum afs_server_trace reason) { - unsigned int u = atomic_inc_return(&server->ref); + int r; - trace_afs_server(server, u, atomic_read(&server->active), reason); + __refcount_inc(&server->ref, &r); + trace_afs_server(server, r + 1, atomic_read(&server->active), reason); return server; } @@ -364,14 +365,14 @@ struct afs_server *afs_get_server(struct afs_server *server, static struct afs_server *afs_maybe_use_server(struct afs_server *server, enum afs_server_trace reason) { - unsigned int r = atomic_fetch_add_unless(&server->ref, 1, 0); unsigned int a; + int r; - if (r == 0) + if (!__refcount_inc_not_zero(&server->ref, &r)) return NULL; a = atomic_inc_return(&server->active); - trace_afs_server(server, r, a, reason); + trace_afs_server(server, r + 1, a, reason); return server; } @@ -380,10 +381,13 @@ static struct afs_server *afs_maybe_use_server(struct afs_server *server, */ struct afs_server *afs_use_server(struct afs_server *server, enum afs_server_trace reason) { - unsigned int r = atomic_inc_return(&server->ref); - unsigned int a = atomic_inc_return(&server->active); + unsigned int a; + int r; + + __refcount_inc(&server->ref, &r); + a = atomic_inc_return(&server->active); - trace_afs_server(server, r, a, reason); + trace_afs_server(server, r + 1, a, reason); return server; } @@ -393,14 +397,15 @@ struct afs_server *afs_use_server(struct afs_server *server, enum afs_server_tra void afs_put_server(struct afs_net *net, struct afs_server *server, enum afs_server_trace reason) { - unsigned int usage; + bool zero; + int r; if (!server) return; - usage = atomic_dec_return(&server->ref); - trace_afs_server(server, usage, atomic_read(&server->active), reason); - if (unlikely(usage == 0)) + zero = __refcount_dec_and_test(&server->ref, &r); + trace_afs_server(server, r - 1, atomic_read(&server->active), reason); + if (unlikely(zero)) __afs_put_server(net, server); } @@ -436,7 +441,7 @@ static void afs_server_rcu(struct rcu_head *rcu) { struct afs_server *server = container_of(rcu, struct afs_server, rcu); - trace_afs_server(server, atomic_read(&server->ref), + trace_afs_server(server, refcount_read(&server->ref), atomic_read(&server->active), afs_server_trace_free); afs_put_addrlist(rcu_access_pointer(server->addresses)); kfree(server); @@ -487,7 +492,7 @@ static void afs_gc_servers(struct afs_net *net, struct afs_server *gc_list) active = atomic_read(&server->active); if (active == 0) { - trace_afs_server(server, atomic_read(&server->ref), + trace_afs_server(server, refcount_read(&server->ref), active, afs_server_trace_gc); next = rcu_dereference_protected( server->uuid_next, lockdep_is_held(&net->fs_lock.lock)); @@ -553,7 +558,7 @@ void afs_manage_servers(struct work_struct *work) _debug("manage %pU %u", &server->uuid, active); if (purging) { - trace_afs_server(server, atomic_read(&server->ref), + trace_afs_server(server, refcount_read(&server->ref), active, afs_server_trace_purging); if (active != 0) pr_notice("Can't purge s=%08x\n", server->debug_id); @@ -633,7 +638,8 @@ static noinline bool afs_update_server_record(struct afs_operation *op, _enter(""); - trace_afs_server(server, atomic_read(&server->ref), atomic_read(&server->active), + trace_afs_server(server, refcount_read(&server->ref), + atomic_read(&server->active), afs_server_trace_update); alist = afs_vl_lookup_addrs(op->volume->cell, op->key, &server->uuid); -- cgit v1.2.3-58-ga151 From 2757a4dc184997c66ef1de32636f73b9f21aac14 Mon Sep 17 00:00:00 2001 From: David Howells Date: Wed, 6 Jul 2022 11:26:14 +0100 Subject: afs: Fix access after dec in put functions Reference-putting functions should not access the object being put after decrementing the refcount unless they reduce the refcount to zero. Fix a couple of instances of this in afs by copying the information to be logged by tracepoint to local variables before doing the decrement. [Fixed a bit in afs_put_server() that I'd missed but Marc caught] Fixes: 341f741f04be ("afs: Refcount the afs_call struct") Fixes: 452181936931 ("afs: Trace afs_server usage") Fixes: 977e5f8ed0ab ("afs: Split the usage count on struct afs_server") Signed-off-by: David Howells cc: Marc Dionne cc: linux-afs@lists.infradead.org Link: https://lore.kernel.org/r/165911278430.3745403.16526310736054780645.stgit@warthog.procyon.org.uk/ # v1 --- fs/afs/cmservice.c | 2 +- fs/afs/rxrpc.c | 11 ++++++----- fs/afs/server.c | 22 +++++++++++++--------- include/trace/events/afs.h | 12 ++++++------ 4 files changed, 26 insertions(+), 21 deletions(-) (limited to 'fs/afs/server.c') diff --git a/fs/afs/cmservice.c b/fs/afs/cmservice.c index cedd627e1fae..0a090d614e76 100644 --- a/fs/afs/cmservice.c +++ b/fs/afs/cmservice.c @@ -212,7 +212,7 @@ static void SRXAFSCB_CallBack(struct work_struct *work) * to maintain cache coherency. */ if (call->server) { - trace_afs_server(call->server, + trace_afs_server(call->server->debug_id, refcount_read(&call->server->ref), atomic_read(&call->server->active), afs_server_trace_callback); diff --git a/fs/afs/rxrpc.c b/fs/afs/rxrpc.c index d9acc43cb6f0..d5c4785c862d 100644 --- a/fs/afs/rxrpc.c +++ b/fs/afs/rxrpc.c @@ -152,7 +152,7 @@ static struct afs_call *afs_alloc_call(struct afs_net *net, call->iter = &call->def_iter; o = atomic_inc_return(&net->nr_outstanding_calls); - trace_afs_call(call, afs_call_trace_alloc, 1, o, + trace_afs_call(call->debug_id, afs_call_trace_alloc, 1, o, __builtin_return_address(0)); return call; } @@ -163,12 +163,13 @@ static struct afs_call *afs_alloc_call(struct afs_net *net, void afs_put_call(struct afs_call *call) { struct afs_net *net = call->net; + unsigned int debug_id = call->debug_id; bool zero; int r, o; zero = __refcount_dec_and_test(&call->ref, &r); o = atomic_read(&net->nr_outstanding_calls); - trace_afs_call(call, afs_call_trace_put, r - 1, o, + trace_afs_call(debug_id, afs_call_trace_put, r - 1, o, __builtin_return_address(0)); if (zero) { @@ -186,7 +187,7 @@ void afs_put_call(struct afs_call *call) afs_put_addrlist(call->alist); kfree(call->request); - trace_afs_call(call, afs_call_trace_free, 0, o, + trace_afs_call(call->debug_id, afs_call_trace_free, 0, o, __builtin_return_address(0)); kfree(call); @@ -203,7 +204,7 @@ static struct afs_call *afs_get_call(struct afs_call *call, __refcount_inc(&call->ref, &r); - trace_afs_call(call, why, r + 1, + trace_afs_call(call->debug_id, why, r + 1, atomic_read(&call->net->nr_outstanding_calls), __builtin_return_address(0)); return call; @@ -677,7 +678,7 @@ static void afs_wake_up_async_call(struct sock *sk, struct rxrpc_call *rxcall, call->need_attention = true; if (__refcount_inc_not_zero(&call->ref, &r)) { - trace_afs_call(call, afs_call_trace_wake, r + 1, + trace_afs_call(call->debug_id, afs_call_trace_wake, r + 1, atomic_read(&call->net->nr_outstanding_calls), __builtin_return_address(0)); diff --git a/fs/afs/server.c b/fs/afs/server.c index ffed828622b6..4981baf97835 100644 --- a/fs/afs/server.c +++ b/fs/afs/server.c @@ -243,7 +243,7 @@ static struct afs_server *afs_alloc_server(struct afs_cell *cell, server->rtt = UINT_MAX; afs_inc_servers_outstanding(net); - trace_afs_server(server, 1, 1, afs_server_trace_alloc); + trace_afs_server(server->debug_id, 1, 1, afs_server_trace_alloc); _leave(" = %p", server); return server; @@ -352,10 +352,12 @@ void afs_servers_timer(struct timer_list *timer) struct afs_server *afs_get_server(struct afs_server *server, enum afs_server_trace reason) { + unsigned int a; int r; __refcount_inc(&server->ref, &r); - trace_afs_server(server, r + 1, atomic_read(&server->active), reason); + a = atomic_read(&server->active); + trace_afs_server(server->debug_id, r + 1, a, reason); return server; } @@ -372,7 +374,7 @@ static struct afs_server *afs_maybe_use_server(struct afs_server *server, return NULL; a = atomic_inc_return(&server->active); - trace_afs_server(server, r + 1, a, reason); + trace_afs_server(server->debug_id, r + 1, a, reason); return server; } @@ -387,7 +389,7 @@ struct afs_server *afs_use_server(struct afs_server *server, enum afs_server_tra __refcount_inc(&server->ref, &r); a = atomic_inc_return(&server->active); - trace_afs_server(server, r + 1, a, reason); + trace_afs_server(server->debug_id, r + 1, a, reason); return server; } @@ -397,14 +399,16 @@ struct afs_server *afs_use_server(struct afs_server *server, enum afs_server_tra void afs_put_server(struct afs_net *net, struct afs_server *server, enum afs_server_trace reason) { + unsigned int a, debug_id = server->debug_id; bool zero; int r; if (!server) return; + a = atomic_inc_return(&server->active); zero = __refcount_dec_and_test(&server->ref, &r); - trace_afs_server(server, r - 1, atomic_read(&server->active), reason); + trace_afs_server(debug_id, r - 1, a, reason); if (unlikely(zero)) __afs_put_server(net, server); } @@ -441,7 +445,7 @@ static void afs_server_rcu(struct rcu_head *rcu) { struct afs_server *server = container_of(rcu, struct afs_server, rcu); - trace_afs_server(server, refcount_read(&server->ref), + trace_afs_server(server->debug_id, refcount_read(&server->ref), atomic_read(&server->active), afs_server_trace_free); afs_put_addrlist(rcu_access_pointer(server->addresses)); kfree(server); @@ -492,7 +496,7 @@ static void afs_gc_servers(struct afs_net *net, struct afs_server *gc_list) active = atomic_read(&server->active); if (active == 0) { - trace_afs_server(server, refcount_read(&server->ref), + trace_afs_server(server->debug_id, refcount_read(&server->ref), active, afs_server_trace_gc); next = rcu_dereference_protected( server->uuid_next, lockdep_is_held(&net->fs_lock.lock)); @@ -558,7 +562,7 @@ void afs_manage_servers(struct work_struct *work) _debug("manage %pU %u", &server->uuid, active); if (purging) { - trace_afs_server(server, refcount_read(&server->ref), + trace_afs_server(server->debug_id, refcount_read(&server->ref), active, afs_server_trace_purging); if (active != 0) pr_notice("Can't purge s=%08x\n", server->debug_id); @@ -638,7 +642,7 @@ static noinline bool afs_update_server_record(struct afs_operation *op, _enter(""); - trace_afs_server(server, refcount_read(&server->ref), + trace_afs_server(server->debug_id, refcount_read(&server->ref), atomic_read(&server->active), afs_server_trace_update); diff --git a/include/trace/events/afs.h b/include/trace/events/afs.h index aa60f42a9763..e9d412d19dbb 100644 --- a/include/trace/events/afs.h +++ b/include/trace/events/afs.h @@ -727,10 +727,10 @@ TRACE_EVENT(afs_cb_call, ); TRACE_EVENT(afs_call, - TP_PROTO(struct afs_call *call, enum afs_call_trace op, + TP_PROTO(unsigned int call_debug_id, enum afs_call_trace op, int ref, int outstanding, const void *where), - TP_ARGS(call, op, ref, outstanding, where), + TP_ARGS(call_debug_id, op, ref, outstanding, where), TP_STRUCT__entry( __field(unsigned int, call ) @@ -741,7 +741,7 @@ TRACE_EVENT(afs_call, ), TP_fast_assign( - __entry->call = call->debug_id; + __entry->call = call_debug_id; __entry->op = op; __entry->ref = ref; __entry->outstanding = outstanding; @@ -1433,10 +1433,10 @@ TRACE_EVENT(afs_cb_miss, ); TRACE_EVENT(afs_server, - TP_PROTO(struct afs_server *server, int ref, int active, + TP_PROTO(unsigned int server_debug_id, int ref, int active, enum afs_server_trace reason), - TP_ARGS(server, ref, active, reason), + TP_ARGS(server_debug_id, ref, active, reason), TP_STRUCT__entry( __field(unsigned int, server ) @@ -1446,7 +1446,7 @@ TRACE_EVENT(afs_server, ), TP_fast_assign( - __entry->server = server->debug_id; + __entry->server = server_debug_id; __entry->ref = ref; __entry->active = active; __entry->reason = reason; -- cgit v1.2.3-58-ga151