* [PATCH 0/2] afs: Fix ref-put functions
@ 2022-07-29 16:39 David Howells
2022-07-29 16:39 ` [PATCH 1/2] afs: Use refcount_t rather than atomic_t David Howells
2022-07-29 16:39 ` [PATCH 2/2] afs: Fix access after dec in put functions David Howells
0 siblings, 2 replies; 8+ messages in thread
From: David Howells @ 2022-07-29 16:39 UTC (permalink / raw)
To: linux-afs; +Cc: Marc Dionne, dhowells, Marc Dionne, linux-fsdevel, linux-kernel
Here's a pair of patches: the first converts afs to use refcount_t instead
of atomic_t for its refcounts; the second fixes a number of afs ref-putting
functions to make sure they don't access the target object after the
decrement unless the refcount was reduced to 0.
The patches can be found here:
https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=afs-fixes
David
---
David Howells (2):
afs: Use refcount_t rather than atomic_t
afs: Fix access after dec in put functions
fs/afs/cell.c | 61 ++++++++++++++++++--------------------
fs/afs/cmservice.c | 4 +--
fs/afs/internal.h | 16 +++++-----
fs/afs/proc.c | 6 ++--
fs/afs/rxrpc.c | 31 ++++++++++---------
fs/afs/server.c | 46 +++++++++++++++++-----------
fs/afs/vl_list.c | 19 ++++--------
fs/afs/volume.c | 21 ++++++++-----
include/trace/events/afs.h | 36 +++++++++++-----------
9 files changed, 124 insertions(+), 116 deletions(-)
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH 1/2] afs: Use refcount_t rather than atomic_t 2022-07-29 16:39 [PATCH 0/2] afs: Fix ref-put functions David Howells @ 2022-07-29 16:39 ` David Howells 2022-08-02 14:27 ` Marc Dionne 2022-07-29 16:39 ` [PATCH 2/2] afs: Fix access after dec in put functions David Howells 1 sibling, 1 reply; 8+ messages in thread From: David Howells @ 2022-07-29 16:39 UTC (permalink / raw) To: linux-afs; +Cc: Marc Dionne, dhowells, Marc Dionne, linux-fsdevel, linux-kernel Use refcount_t rather than atomic_t in afs to make use of the count checking facilities provided. Signed-off-by: David Howells <dhowells@redhat.com> cc: Marc Dionne <marc.dionne@auristor.com> cc: linux-afs@lists.infradead.org --- fs/afs/cell.c | 61 +++++++++++++++++++++----------------------- fs/afs/cmservice.c | 2 + fs/afs/internal.h | 16 ++++++------ fs/afs/proc.c | 6 ++-- fs/afs/rxrpc.c | 26 ++++++++++--------- fs/afs/server.c | 40 +++++++++++++++++------------ fs/afs/vl_list.c | 19 ++++---------- fs/afs/volume.c | 21 +++++++++------ include/trace/events/afs.h | 26 +++++++++---------- 9 files changed, 110 insertions(+), 107 deletions(-) diff --git a/fs/afs/cell.c b/fs/afs/cell.c index 07ad744eef77..988c2ac7cece 100644 --- a/fs/afs/cell.c +++ b/fs/afs/cell.c @@ -158,7 +158,7 @@ static struct afs_cell *afs_alloc_cell(struct afs_net *net, cell->name[i] = tolower(name[i]); cell->name[i] = 0; - atomic_set(&cell->ref, 1); + refcount_set(&cell->ref, 1); atomic_set(&cell->active, 0); INIT_WORK(&cell->manager, afs_manage_cell_work); cell->volumes = RB_ROOT; @@ -287,7 +287,7 @@ struct afs_cell *afs_lookup_cell(struct afs_net *net, cell = candidate; candidate = NULL; atomic_set(&cell->active, 2); - trace_afs_cell(cell->debug_id, atomic_read(&cell->ref), 2, afs_cell_trace_insert); + trace_afs_cell(cell->debug_id, refcount_read(&cell->ref), 2, afs_cell_trace_insert); rb_link_node_rcu(&cell->net_node, parent, pp); rb_insert_color(&cell->net_node, &net->cells); up_write(&net->cells_lock); @@ -295,7 +295,7 @@ struct afs_cell *afs_lookup_cell(struct afs_net *net, afs_queue_cell(cell, afs_cell_trace_get_queue_new); wait_for_cell: - trace_afs_cell(cell->debug_id, atomic_read(&cell->ref), atomic_read(&cell->active), + trace_afs_cell(cell->debug_id, refcount_read(&cell->ref), atomic_read(&cell->active), afs_cell_trace_wait); _debug("wait_for_cell"); wait_var_event(&cell->state, @@ -490,13 +490,13 @@ static void afs_cell_destroy(struct rcu_head *rcu) { struct afs_cell *cell = container_of(rcu, struct afs_cell, rcu); struct afs_net *net = cell->net; - int u; + int r; _enter("%p{%s}", cell, cell->name); - u = atomic_read(&cell->ref); - ASSERTCMP(u, ==, 0); - trace_afs_cell(cell->debug_id, u, atomic_read(&cell->active), afs_cell_trace_free); + r = refcount_read(&cell->ref); + ASSERTCMP(r, ==, 0); + trace_afs_cell(cell->debug_id, r, atomic_read(&cell->active), afs_cell_trace_free); afs_put_vlserverlist(net, rcu_access_pointer(cell->vl_servers)); afs_unuse_cell(net, cell->alias_of, afs_cell_trace_unuse_alias); @@ -539,13 +539,10 @@ void afs_cells_timer(struct timer_list *timer) */ struct afs_cell *afs_get_cell(struct afs_cell *cell, enum afs_cell_trace reason) { - int u; + int r; - if (atomic_read(&cell->ref) <= 0) - BUG(); - - u = atomic_inc_return(&cell->ref); - trace_afs_cell(cell->debug_id, u, atomic_read(&cell->active), reason); + __refcount_inc(&cell->ref, &r); + trace_afs_cell(cell->debug_id, r + 1, atomic_read(&cell->active), reason); return cell; } @@ -556,12 +553,14 @@ void afs_put_cell(struct afs_cell *cell, enum afs_cell_trace reason) { if (cell) { unsigned int debug_id = cell->debug_id; - unsigned int u, a; + unsigned int a; + bool zero; + int r; a = atomic_read(&cell->active); - u = atomic_dec_return(&cell->ref); - trace_afs_cell(debug_id, u, a, reason); - if (u == 0) { + zero = __refcount_dec_and_test(&cell->ref, &r); + trace_afs_cell(debug_id, r - 1, a, reason); + if (zero) { a = atomic_read(&cell->active); WARN(a != 0, "Cell active count %u > 0\n", a); call_rcu(&cell->rcu, afs_cell_destroy); @@ -574,14 +573,12 @@ void afs_put_cell(struct afs_cell *cell, enum afs_cell_trace reason) */ struct afs_cell *afs_use_cell(struct afs_cell *cell, enum afs_cell_trace reason) { - int u, a; - - if (atomic_read(&cell->ref) <= 0) - BUG(); + int r, a; - u = atomic_read(&cell->ref); + r = refcount_read(&cell->ref); + WARN_ON(r == 0); a = atomic_inc_return(&cell->active); - trace_afs_cell(cell->debug_id, u, a, reason); + trace_afs_cell(cell->debug_id, r, a, reason); return cell; } @@ -593,7 +590,7 @@ void afs_unuse_cell(struct afs_net *net, struct afs_cell *cell, enum afs_cell_tr { unsigned int debug_id; time64_t now, expire_delay; - int u, a; + int r, a; if (!cell) return; @@ -607,9 +604,9 @@ void afs_unuse_cell(struct afs_net *net, struct afs_cell *cell, enum afs_cell_tr expire_delay = afs_cell_gc_delay; debug_id = cell->debug_id; - u = atomic_read(&cell->ref); + r = refcount_read(&cell->ref); a = atomic_dec_return(&cell->active); - trace_afs_cell(debug_id, u, a, reason); + trace_afs_cell(debug_id, r, a, reason); WARN_ON(a == 0); if (a == 1) /* 'cell' may now be garbage collected. */ @@ -621,11 +618,11 @@ void afs_unuse_cell(struct afs_net *net, struct afs_cell *cell, enum afs_cell_tr */ void afs_see_cell(struct afs_cell *cell, enum afs_cell_trace reason) { - int u, a; + int r, a; - u = atomic_read(&cell->ref); + r = refcount_read(&cell->ref); a = atomic_read(&cell->active); - trace_afs_cell(cell->debug_id, u, a, reason); + trace_afs_cell(cell->debug_id, r, a, reason); } /* @@ -739,7 +736,7 @@ static void afs_manage_cell(struct afs_cell *cell) active = 1; if (atomic_try_cmpxchg_relaxed(&cell->active, &active, 0)) { rb_erase(&cell->net_node, &net->cells); - trace_afs_cell(cell->debug_id, atomic_read(&cell->ref), 0, + trace_afs_cell(cell->debug_id, refcount_read(&cell->ref), 0, afs_cell_trace_unuse_delete); smp_store_release(&cell->state, AFS_CELL_REMOVED); } @@ -866,7 +863,7 @@ void afs_manage_cells(struct work_struct *work) bool sched_cell = false; active = atomic_read(&cell->active); - trace_afs_cell(cell->debug_id, atomic_read(&cell->ref), + trace_afs_cell(cell->debug_id, refcount_read(&cell->ref), active, afs_cell_trace_manage); ASSERTCMP(active, >=, 1); @@ -874,7 +871,7 @@ void afs_manage_cells(struct work_struct *work) if (purging) { if (test_and_clear_bit(AFS_CELL_FL_NO_GC, &cell->flags)) { active = atomic_dec_return(&cell->active); - trace_afs_cell(cell->debug_id, atomic_read(&cell->ref), + trace_afs_cell(cell->debug_id, refcount_read(&cell->ref), active, afs_cell_trace_unuse_pin); } } diff --git a/fs/afs/cmservice.c b/fs/afs/cmservice.c index a3f5de28be79..cedd627e1fae 100644 --- a/fs/afs/cmservice.c +++ b/fs/afs/cmservice.c @@ -213,7 +213,7 @@ static void SRXAFSCB_CallBack(struct work_struct *work) */ if (call->server) { trace_afs_server(call->server, - atomic_read(&call->server->ref), + refcount_read(&call->server->ref), atomic_read(&call->server->active), afs_server_trace_callback); afs_break_callbacks(call->server, call->count, call->request); diff --git a/fs/afs/internal.h b/fs/afs/internal.h index a6f25d9e75b5..64ad55494349 100644 --- a/fs/afs/internal.h +++ b/fs/afs/internal.h @@ -122,7 +122,7 @@ struct afs_call { }; struct afs_operation *op; unsigned int server_index; - atomic_t usage; + refcount_t ref; enum afs_call_state state; spinlock_t state_lock; int error; /* error code */ @@ -365,7 +365,7 @@ struct afs_cell { struct hlist_node proc_link; /* /proc cell list link */ time64_t dns_expiry; /* Time AFSDB/SRV record expires */ time64_t last_inactive; /* Time of last drop of usage count */ - atomic_t ref; /* Struct refcount */ + refcount_t ref; /* Struct refcount */ atomic_t active; /* Active usage counter */ unsigned long flags; #define AFS_CELL_FL_NO_GC 0 /* The cell was added manually, don't auto-gc */ @@ -410,7 +410,7 @@ struct afs_vlserver { #define AFS_VLSERVER_FL_IS_YFS 2 /* Server is YFS not AFS */ #define AFS_VLSERVER_FL_RESPONDING 3 /* VL server is responding */ rwlock_t lock; /* Lock on addresses */ - atomic_t usage; + refcount_t ref; unsigned int rtt; /* Server's current RTT in uS */ /* Probe state */ @@ -446,7 +446,7 @@ struct afs_vlserver_entry { struct afs_vlserver_list { struct rcu_head rcu; - atomic_t usage; + refcount_t ref; u8 nr_servers; u8 index; /* Server currently in use */ u8 preferred; /* Preferred server */ @@ -517,7 +517,7 @@ struct afs_server { #define AFS_SERVER_FL_NO_IBULK 17 /* Fileserver doesn't support FS.InlineBulkStatus */ #define AFS_SERVER_FL_NO_RM2 18 /* Fileserver doesn't support YFS.RemoveFile2 */ #define AFS_SERVER_FL_HAS_FS64 19 /* Fileserver supports FS.{Fetch,Store}Data64 */ - atomic_t ref; /* Object refcount */ + refcount_t ref; /* Object refcount */ atomic_t active; /* Active user count */ u32 addr_version; /* Address list version */ unsigned int rtt; /* Server's current RTT in uS */ @@ -571,7 +571,7 @@ struct afs_volume { struct rcu_head rcu; afs_volid_t vid; /* volume ID */ }; - atomic_t usage; + refcount_t ref; time64_t update_at; /* Time at which to next update */ struct afs_cell *cell; /* Cell to which belongs (pins ref) */ struct rb_node cell_node; /* Link in cell->volumes */ @@ -1493,14 +1493,14 @@ extern int afs_end_vlserver_operation(struct afs_vl_cursor *); */ static inline struct afs_vlserver *afs_get_vlserver(struct afs_vlserver *vlserver) { - atomic_inc(&vlserver->usage); + refcount_inc(&vlserver->ref); return vlserver; } static inline struct afs_vlserver_list *afs_get_vlserverlist(struct afs_vlserver_list *vllist) { if (vllist) - atomic_inc(&vllist->usage); + refcount_inc(&vllist->ref); return vllist; } diff --git a/fs/afs/proc.c b/fs/afs/proc.c index e1b863449296..2a0c83d71565 100644 --- a/fs/afs/proc.c +++ b/fs/afs/proc.c @@ -47,7 +47,7 @@ static int afs_proc_cells_show(struct seq_file *m, void *v) /* display one cell per line on subsequent lines */ seq_printf(m, "%3u %3u %6lld %2u %2u %s\n", - atomic_read(&cell->ref), + refcount_read(&cell->ref), atomic_read(&cell->active), cell->dns_expiry - ktime_get_real_seconds(), vllist ? vllist->nr_servers : 0, @@ -217,7 +217,7 @@ static int afs_proc_cell_volumes_show(struct seq_file *m, void *v) } seq_printf(m, "%3d %08llx %s %s\n", - atomic_read(&vol->usage), vol->vid, + refcount_read(&vol->ref), vol->vid, afs_vol_types[vol->type], vol->name); @@ -388,7 +388,7 @@ static int afs_proc_servers_show(struct seq_file *m, void *v) alist = rcu_dereference(server->addresses); seq_printf(m, "%pU %3d %3d\n", &server->uuid, - atomic_read(&server->ref), + refcount_read(&server->ref), atomic_read(&server->active)); seq_printf(m, " - info: fl=%lx rtt=%u brk=%x\n", server->flags, server->rtt, server->cb_s_break); diff --git a/fs/afs/rxrpc.c b/fs/afs/rxrpc.c index a5434f3e57c6..d9acc43cb6f0 100644 --- a/fs/afs/rxrpc.c +++ b/fs/afs/rxrpc.c @@ -145,7 +145,7 @@ static struct afs_call *afs_alloc_call(struct afs_net *net, call->type = type; call->net = net; call->debug_id = atomic_inc_return(&rxrpc_debug_id); - atomic_set(&call->usage, 1); + refcount_set(&call->ref, 1); INIT_WORK(&call->async_work, afs_process_async_call); init_waitqueue_head(&call->waitq); spin_lock_init(&call->state_lock); @@ -163,14 +163,15 @@ 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; - int n = atomic_dec_return(&call->usage); - int o = atomic_read(&net->nr_outstanding_calls); + bool zero; + int r, o; - trace_afs_call(call, afs_call_trace_put, n, 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, __builtin_return_address(0)); - ASSERTCMP(n, >=, 0); - if (n == 0) { + if (zero) { ASSERT(!work_pending(&call->async_work)); ASSERT(call->type->name != NULL); @@ -198,9 +199,11 @@ 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); + int r; - trace_afs_call(call, why, u, + __refcount_inc(&call->ref, &r); + + trace_afs_call(call, why, r + 1, atomic_read(&call->net->nr_outstanding_calls), __builtin_return_address(0)); return call; @@ -668,14 +671,13 @@ static void afs_wake_up_async_call(struct sock *sk, struct rxrpc_call *rxcall, unsigned long call_user_ID) { struct afs_call *call = (struct afs_call *)call_user_ID; - int u; + int r; trace_afs_notify_call(rxcall, call); call->need_attention = true; - u = atomic_fetch_add_unless(&call->usage, 1, 0); - if (u != 0) { - trace_afs_call(call, afs_call_trace_wake, u + 1, + if (__refcount_inc_not_zero(&call->ref, &r)) { + trace_afs_call(call, 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 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); diff --git a/fs/afs/vl_list.c b/fs/afs/vl_list.c index 38b2ba1d9ec0..acc48216136a 100644 --- a/fs/afs/vl_list.c +++ b/fs/afs/vl_list.c @@ -17,7 +17,7 @@ struct afs_vlserver *afs_alloc_vlserver(const char *name, size_t name_len, vlserver = kzalloc(struct_size(vlserver, name, name_len + 1), GFP_KERNEL); if (vlserver) { - atomic_set(&vlserver->usage, 1); + refcount_set(&vlserver->ref, 1); rwlock_init(&vlserver->lock); init_waitqueue_head(&vlserver->probe_wq); spin_lock_init(&vlserver->probe_lock); @@ -39,13 +39,9 @@ static void afs_vlserver_rcu(struct rcu_head *rcu) void afs_put_vlserver(struct afs_net *net, struct afs_vlserver *vlserver) { - if (vlserver) { - unsigned int u = atomic_dec_return(&vlserver->usage); - //_debug("VL PUT %p{%u}", vlserver, u); - - if (u == 0) - call_rcu(&vlserver->rcu, afs_vlserver_rcu); - } + if (vlserver && + refcount_dec_and_test(&vlserver->ref)) + call_rcu(&vlserver->rcu, afs_vlserver_rcu); } struct afs_vlserver_list *afs_alloc_vlserver_list(unsigned int nr_servers) @@ -54,7 +50,7 @@ struct afs_vlserver_list *afs_alloc_vlserver_list(unsigned int nr_servers) vllist = kzalloc(struct_size(vllist, servers, nr_servers), GFP_KERNEL); if (vllist) { - atomic_set(&vllist->usage, 1); + refcount_set(&vllist->ref, 1); rwlock_init(&vllist->lock); } @@ -64,10 +60,7 @@ struct afs_vlserver_list *afs_alloc_vlserver_list(unsigned int nr_servers) void afs_put_vlserverlist(struct afs_net *net, struct afs_vlserver_list *vllist) { if (vllist) { - unsigned int u = atomic_dec_return(&vllist->usage); - - //_debug("VLLS PUT %p{%u}", vllist, u); - if (u == 0) { + if (refcount_dec_and_test(&vllist->ref)) { int i; for (i = 0; i < vllist->nr_servers; i++) { diff --git a/fs/afs/volume.c b/fs/afs/volume.c index cc665cef0abe..f4937029dcd7 100644 --- a/fs/afs/volume.c +++ b/fs/afs/volume.c @@ -52,7 +52,7 @@ static void afs_remove_volume_from_cell(struct afs_volume *volume) struct afs_cell *cell = volume->cell; if (!hlist_unhashed(&volume->proc_link)) { - trace_afs_volume(volume->vid, atomic_read(&volume->usage), + trace_afs_volume(volume->vid, refcount_read(&cell->ref), afs_volume_trace_remove); write_seqlock(&cell->volume_lock); hlist_del_rcu(&volume->proc_link); @@ -87,7 +87,7 @@ static struct afs_volume *afs_alloc_volume(struct afs_fs_context *params, volume->type_force = params->force; volume->name_len = vldb->name_len; - atomic_set(&volume->usage, 1); + refcount_set(&volume->ref, 1); INIT_HLIST_NODE(&volume->proc_link); rwlock_init(&volume->servers_lock); rwlock_init(&volume->cb_v_break_lock); @@ -228,7 +228,7 @@ static void afs_destroy_volume(struct afs_net *net, struct afs_volume *volume) afs_remove_volume_from_cell(volume); afs_put_serverlist(net, rcu_access_pointer(volume->servers)); afs_put_cell(volume->cell, afs_cell_trace_put_vol); - trace_afs_volume(volume->vid, atomic_read(&volume->usage), + trace_afs_volume(volume->vid, refcount_read(&volume->ref), afs_volume_trace_free); kfree_rcu(volume, rcu); @@ -242,8 +242,10 @@ struct afs_volume *afs_get_volume(struct afs_volume *volume, enum afs_volume_trace reason) { if (volume) { - int u = atomic_inc_return(&volume->usage); - trace_afs_volume(volume->vid, u, reason); + int r; + + __refcount_inc(&volume->ref, &r); + trace_afs_volume(volume->vid, r + 1, reason); } return volume; } @@ -257,9 +259,12 @@ void afs_put_volume(struct afs_net *net, struct afs_volume *volume, { if (volume) { afs_volid_t vid = volume->vid; - int u = atomic_dec_return(&volume->usage); - trace_afs_volume(vid, u, reason); - if (u == 0) + bool zero; + int r; + + zero = __refcount_dec_and_test(&volume->ref, &r); + trace_afs_volume(vid, r - 1, reason); + if (zero) afs_destroy_volume(net, volume); } } diff --git a/include/trace/events/afs.h b/include/trace/events/afs.h index 499f5fabd20f..aa60f42a9763 100644 --- a/include/trace/events/afs.h +++ b/include/trace/events/afs.h @@ -728,14 +728,14 @@ TRACE_EVENT(afs_cb_call, TRACE_EVENT(afs_call, TP_PROTO(struct afs_call *call, enum afs_call_trace op, - int usage, int outstanding, const void *where), + int ref, int outstanding, const void *where), - TP_ARGS(call, op, usage, outstanding, where), + TP_ARGS(call, op, ref, outstanding, where), TP_STRUCT__entry( __field(unsigned int, call ) __field(int, op ) - __field(int, usage ) + __field(int, ref ) __field(int, outstanding ) __field(const void *, where ) ), @@ -743,15 +743,15 @@ TRACE_EVENT(afs_call, TP_fast_assign( __entry->call = call->debug_id; __entry->op = op; - __entry->usage = usage; + __entry->ref = ref; __entry->outstanding = outstanding; __entry->where = where; ), - TP_printk("c=%08x %s u=%d o=%d sp=%pSR", + TP_printk("c=%08x %s r=%d o=%d sp=%pSR", __entry->call, __print_symbolic(__entry->op, afs_call_traces), - __entry->usage, + __entry->ref, __entry->outstanding, __entry->where) ); @@ -1476,36 +1476,36 @@ TRACE_EVENT(afs_volume, __entry->reason = reason; ), - TP_printk("V=%llx %s u=%d", + TP_printk("V=%llx %s ur=%d", __entry->vid, __print_symbolic(__entry->reason, afs_volume_traces), __entry->ref) ); TRACE_EVENT(afs_cell, - TP_PROTO(unsigned int cell_debug_id, int usage, int active, + TP_PROTO(unsigned int cell_debug_id, int ref, int active, enum afs_cell_trace reason), - TP_ARGS(cell_debug_id, usage, active, reason), + TP_ARGS(cell_debug_id, ref, active, reason), TP_STRUCT__entry( __field(unsigned int, cell ) - __field(int, usage ) + __field(int, ref ) __field(int, active ) __field(int, reason ) ), TP_fast_assign( __entry->cell = cell_debug_id; - __entry->usage = usage; + __entry->ref = ref; __entry->active = active; __entry->reason = reason; ), - TP_printk("L=%08x %s u=%d a=%d", + TP_printk("L=%08x %s r=%d a=%d", __entry->cell, __print_symbolic(__entry->reason, afs_cell_traces), - __entry->usage, + __entry->ref, __entry->active) ); ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] afs: Use refcount_t rather than atomic_t 2022-07-29 16:39 ` [PATCH 1/2] afs: Use refcount_t rather than atomic_t David Howells @ 2022-08-02 14:27 ` Marc Dionne 0 siblings, 0 replies; 8+ messages in thread From: Marc Dionne @ 2022-08-02 14:27 UTC (permalink / raw) To: David Howells; +Cc: linux-afs, linux-fsdevel, Linux Kernel Mailing List On Fri, Jul 29, 2022 at 1:40 PM David Howells <dhowells@redhat.com> wrote: > > Use refcount_t rather than atomic_t in afs to make use of the count > checking facilities provided. > > Signed-off-by: David Howells <dhowells@redhat.com> > cc: Marc Dionne <marc.dionne@auristor.com> > cc: linux-afs@lists.infradead.org > --- > > fs/afs/cell.c | 61 +++++++++++++++++++++----------------------- > fs/afs/cmservice.c | 2 + > fs/afs/internal.h | 16 ++++++------ > fs/afs/proc.c | 6 ++-- > fs/afs/rxrpc.c | 26 ++++++++++--------- > fs/afs/server.c | 40 +++++++++++++++++------------ > fs/afs/vl_list.c | 19 ++++---------- > fs/afs/volume.c | 21 +++++++++------ > include/trace/events/afs.h | 26 +++++++++---------- > 9 files changed, 110 insertions(+), 107 deletions(-) > > diff --git a/fs/afs/cell.c b/fs/afs/cell.c > index 07ad744eef77..988c2ac7cece 100644 > --- a/fs/afs/cell.c > +++ b/fs/afs/cell.c > @@ -158,7 +158,7 @@ static struct afs_cell *afs_alloc_cell(struct afs_net *net, > cell->name[i] = tolower(name[i]); > cell->name[i] = 0; > > - atomic_set(&cell->ref, 1); > + refcount_set(&cell->ref, 1); > atomic_set(&cell->active, 0); > INIT_WORK(&cell->manager, afs_manage_cell_work); > cell->volumes = RB_ROOT; > @@ -287,7 +287,7 @@ struct afs_cell *afs_lookup_cell(struct afs_net *net, > cell = candidate; > candidate = NULL; > atomic_set(&cell->active, 2); > - trace_afs_cell(cell->debug_id, atomic_read(&cell->ref), 2, afs_cell_trace_insert); > + trace_afs_cell(cell->debug_id, refcount_read(&cell->ref), 2, afs_cell_trace_insert); > rb_link_node_rcu(&cell->net_node, parent, pp); > rb_insert_color(&cell->net_node, &net->cells); > up_write(&net->cells_lock); > @@ -295,7 +295,7 @@ struct afs_cell *afs_lookup_cell(struct afs_net *net, > afs_queue_cell(cell, afs_cell_trace_get_queue_new); > > wait_for_cell: > - trace_afs_cell(cell->debug_id, atomic_read(&cell->ref), atomic_read(&cell->active), > + trace_afs_cell(cell->debug_id, refcount_read(&cell->ref), atomic_read(&cell->active), > afs_cell_trace_wait); > _debug("wait_for_cell"); > wait_var_event(&cell->state, > @@ -490,13 +490,13 @@ static void afs_cell_destroy(struct rcu_head *rcu) > { > struct afs_cell *cell = container_of(rcu, struct afs_cell, rcu); > struct afs_net *net = cell->net; > - int u; > + int r; > > _enter("%p{%s}", cell, cell->name); > > - u = atomic_read(&cell->ref); > - ASSERTCMP(u, ==, 0); > - trace_afs_cell(cell->debug_id, u, atomic_read(&cell->active), afs_cell_trace_free); > + r = refcount_read(&cell->ref); > + ASSERTCMP(r, ==, 0); > + trace_afs_cell(cell->debug_id, r, atomic_read(&cell->active), afs_cell_trace_free); > > afs_put_vlserverlist(net, rcu_access_pointer(cell->vl_servers)); > afs_unuse_cell(net, cell->alias_of, afs_cell_trace_unuse_alias); > @@ -539,13 +539,10 @@ void afs_cells_timer(struct timer_list *timer) > */ > struct afs_cell *afs_get_cell(struct afs_cell *cell, enum afs_cell_trace reason) > { > - int u; > + int r; > > - if (atomic_read(&cell->ref) <= 0) > - BUG(); > - > - u = atomic_inc_return(&cell->ref); > - trace_afs_cell(cell->debug_id, u, atomic_read(&cell->active), reason); > + __refcount_inc(&cell->ref, &r); > + trace_afs_cell(cell->debug_id, r + 1, atomic_read(&cell->active), reason); > return cell; > } > > @@ -556,12 +553,14 @@ void afs_put_cell(struct afs_cell *cell, enum afs_cell_trace reason) > { > if (cell) { > unsigned int debug_id = cell->debug_id; > - unsigned int u, a; > + unsigned int a; > + bool zero; > + int r; > > a = atomic_read(&cell->active); > - u = atomic_dec_return(&cell->ref); > - trace_afs_cell(debug_id, u, a, reason); > - if (u == 0) { > + zero = __refcount_dec_and_test(&cell->ref, &r); > + trace_afs_cell(debug_id, r - 1, a, reason); > + if (zero) { > a = atomic_read(&cell->active); > WARN(a != 0, "Cell active count %u > 0\n", a); > call_rcu(&cell->rcu, afs_cell_destroy); > @@ -574,14 +573,12 @@ void afs_put_cell(struct afs_cell *cell, enum afs_cell_trace reason) > */ > struct afs_cell *afs_use_cell(struct afs_cell *cell, enum afs_cell_trace reason) > { > - int u, a; > - > - if (atomic_read(&cell->ref) <= 0) > - BUG(); > + int r, a; > > - u = atomic_read(&cell->ref); > + r = refcount_read(&cell->ref); > + WARN_ON(r == 0); > a = atomic_inc_return(&cell->active); > - trace_afs_cell(cell->debug_id, u, a, reason); > + trace_afs_cell(cell->debug_id, r, a, reason); > return cell; > } > > @@ -593,7 +590,7 @@ void afs_unuse_cell(struct afs_net *net, struct afs_cell *cell, enum afs_cell_tr > { > unsigned int debug_id; > time64_t now, expire_delay; > - int u, a; > + int r, a; > > if (!cell) > return; > @@ -607,9 +604,9 @@ void afs_unuse_cell(struct afs_net *net, struct afs_cell *cell, enum afs_cell_tr > expire_delay = afs_cell_gc_delay; > > debug_id = cell->debug_id; > - u = atomic_read(&cell->ref); > + r = refcount_read(&cell->ref); > a = atomic_dec_return(&cell->active); > - trace_afs_cell(debug_id, u, a, reason); > + trace_afs_cell(debug_id, r, a, reason); > WARN_ON(a == 0); > if (a == 1) > /* 'cell' may now be garbage collected. */ > @@ -621,11 +618,11 @@ void afs_unuse_cell(struct afs_net *net, struct afs_cell *cell, enum afs_cell_tr > */ > void afs_see_cell(struct afs_cell *cell, enum afs_cell_trace reason) > { > - int u, a; > + int r, a; > > - u = atomic_read(&cell->ref); > + r = refcount_read(&cell->ref); > a = atomic_read(&cell->active); > - trace_afs_cell(cell->debug_id, u, a, reason); > + trace_afs_cell(cell->debug_id, r, a, reason); > } > > /* > @@ -739,7 +736,7 @@ static void afs_manage_cell(struct afs_cell *cell) > active = 1; > if (atomic_try_cmpxchg_relaxed(&cell->active, &active, 0)) { > rb_erase(&cell->net_node, &net->cells); > - trace_afs_cell(cell->debug_id, atomic_read(&cell->ref), 0, > + trace_afs_cell(cell->debug_id, refcount_read(&cell->ref), 0, > afs_cell_trace_unuse_delete); > smp_store_release(&cell->state, AFS_CELL_REMOVED); > } > @@ -866,7 +863,7 @@ void afs_manage_cells(struct work_struct *work) > bool sched_cell = false; > > active = atomic_read(&cell->active); > - trace_afs_cell(cell->debug_id, atomic_read(&cell->ref), > + trace_afs_cell(cell->debug_id, refcount_read(&cell->ref), > active, afs_cell_trace_manage); > > ASSERTCMP(active, >=, 1); > @@ -874,7 +871,7 @@ void afs_manage_cells(struct work_struct *work) > if (purging) { > if (test_and_clear_bit(AFS_CELL_FL_NO_GC, &cell->flags)) { > active = atomic_dec_return(&cell->active); > - trace_afs_cell(cell->debug_id, atomic_read(&cell->ref), > + trace_afs_cell(cell->debug_id, refcount_read(&cell->ref), > active, afs_cell_trace_unuse_pin); > } > } > diff --git a/fs/afs/cmservice.c b/fs/afs/cmservice.c > index a3f5de28be79..cedd627e1fae 100644 > --- a/fs/afs/cmservice.c > +++ b/fs/afs/cmservice.c > @@ -213,7 +213,7 @@ static void SRXAFSCB_CallBack(struct work_struct *work) > */ > if (call->server) { > trace_afs_server(call->server, > - atomic_read(&call->server->ref), > + refcount_read(&call->server->ref), > atomic_read(&call->server->active), > afs_server_trace_callback); > afs_break_callbacks(call->server, call->count, call->request); > diff --git a/fs/afs/internal.h b/fs/afs/internal.h > index a6f25d9e75b5..64ad55494349 100644 > --- a/fs/afs/internal.h > +++ b/fs/afs/internal.h > @@ -122,7 +122,7 @@ struct afs_call { > }; > struct afs_operation *op; > unsigned int server_index; > - atomic_t usage; > + refcount_t ref; > enum afs_call_state state; > spinlock_t state_lock; > int error; /* error code */ > @@ -365,7 +365,7 @@ struct afs_cell { > struct hlist_node proc_link; /* /proc cell list link */ > time64_t dns_expiry; /* Time AFSDB/SRV record expires */ > time64_t last_inactive; /* Time of last drop of usage count */ > - atomic_t ref; /* Struct refcount */ > + refcount_t ref; /* Struct refcount */ > atomic_t active; /* Active usage counter */ > unsigned long flags; > #define AFS_CELL_FL_NO_GC 0 /* The cell was added manually, don't auto-gc */ > @@ -410,7 +410,7 @@ struct afs_vlserver { > #define AFS_VLSERVER_FL_IS_YFS 2 /* Server is YFS not AFS */ > #define AFS_VLSERVER_FL_RESPONDING 3 /* VL server is responding */ > rwlock_t lock; /* Lock on addresses */ > - atomic_t usage; > + refcount_t ref; > unsigned int rtt; /* Server's current RTT in uS */ > > /* Probe state */ > @@ -446,7 +446,7 @@ struct afs_vlserver_entry { > > struct afs_vlserver_list { > struct rcu_head rcu; > - atomic_t usage; > + refcount_t ref; > u8 nr_servers; > u8 index; /* Server currently in use */ > u8 preferred; /* Preferred server */ > @@ -517,7 +517,7 @@ struct afs_server { > #define AFS_SERVER_FL_NO_IBULK 17 /* Fileserver doesn't support FS.InlineBulkStatus */ > #define AFS_SERVER_FL_NO_RM2 18 /* Fileserver doesn't support YFS.RemoveFile2 */ > #define AFS_SERVER_FL_HAS_FS64 19 /* Fileserver supports FS.{Fetch,Store}Data64 */ > - atomic_t ref; /* Object refcount */ > + refcount_t ref; /* Object refcount */ > atomic_t active; /* Active user count */ > u32 addr_version; /* Address list version */ > unsigned int rtt; /* Server's current RTT in uS */ > @@ -571,7 +571,7 @@ struct afs_volume { > struct rcu_head rcu; > afs_volid_t vid; /* volume ID */ > }; > - atomic_t usage; > + refcount_t ref; > time64_t update_at; /* Time at which to next update */ > struct afs_cell *cell; /* Cell to which belongs (pins ref) */ > struct rb_node cell_node; /* Link in cell->volumes */ > @@ -1493,14 +1493,14 @@ extern int afs_end_vlserver_operation(struct afs_vl_cursor *); > */ > static inline struct afs_vlserver *afs_get_vlserver(struct afs_vlserver *vlserver) > { > - atomic_inc(&vlserver->usage); > + refcount_inc(&vlserver->ref); > return vlserver; > } > > static inline struct afs_vlserver_list *afs_get_vlserverlist(struct afs_vlserver_list *vllist) > { > if (vllist) > - atomic_inc(&vllist->usage); > + refcount_inc(&vllist->ref); > return vllist; > } > > diff --git a/fs/afs/proc.c b/fs/afs/proc.c > index e1b863449296..2a0c83d71565 100644 > --- a/fs/afs/proc.c > +++ b/fs/afs/proc.c > @@ -47,7 +47,7 @@ static int afs_proc_cells_show(struct seq_file *m, void *v) > > /* display one cell per line on subsequent lines */ > seq_printf(m, "%3u %3u %6lld %2u %2u %s\n", > - atomic_read(&cell->ref), > + refcount_read(&cell->ref), > atomic_read(&cell->active), > cell->dns_expiry - ktime_get_real_seconds(), > vllist ? vllist->nr_servers : 0, > @@ -217,7 +217,7 @@ static int afs_proc_cell_volumes_show(struct seq_file *m, void *v) > } > > seq_printf(m, "%3d %08llx %s %s\n", > - atomic_read(&vol->usage), vol->vid, > + refcount_read(&vol->ref), vol->vid, > afs_vol_types[vol->type], > vol->name); > > @@ -388,7 +388,7 @@ static int afs_proc_servers_show(struct seq_file *m, void *v) > alist = rcu_dereference(server->addresses); > seq_printf(m, "%pU %3d %3d\n", > &server->uuid, > - atomic_read(&server->ref), > + refcount_read(&server->ref), > atomic_read(&server->active)); > seq_printf(m, " - info: fl=%lx rtt=%u brk=%x\n", > server->flags, server->rtt, server->cb_s_break); > diff --git a/fs/afs/rxrpc.c b/fs/afs/rxrpc.c > index a5434f3e57c6..d9acc43cb6f0 100644 > --- a/fs/afs/rxrpc.c > +++ b/fs/afs/rxrpc.c > @@ -145,7 +145,7 @@ static struct afs_call *afs_alloc_call(struct afs_net *net, > call->type = type; > call->net = net; > call->debug_id = atomic_inc_return(&rxrpc_debug_id); > - atomic_set(&call->usage, 1); > + refcount_set(&call->ref, 1); > INIT_WORK(&call->async_work, afs_process_async_call); > init_waitqueue_head(&call->waitq); > spin_lock_init(&call->state_lock); > @@ -163,14 +163,15 @@ 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; > - int n = atomic_dec_return(&call->usage); > - int o = atomic_read(&net->nr_outstanding_calls); > + bool zero; > + int r, o; > > - trace_afs_call(call, afs_call_trace_put, n, 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, > __builtin_return_address(0)); > > - ASSERTCMP(n, >=, 0); > - if (n == 0) { > + if (zero) { > ASSERT(!work_pending(&call->async_work)); > ASSERT(call->type->name != NULL); > > @@ -198,9 +199,11 @@ 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); > + int r; > > - trace_afs_call(call, why, u, > + __refcount_inc(&call->ref, &r); > + > + trace_afs_call(call, why, r + 1, > atomic_read(&call->net->nr_outstanding_calls), > __builtin_return_address(0)); > return call; > @@ -668,14 +671,13 @@ static void afs_wake_up_async_call(struct sock *sk, struct rxrpc_call *rxcall, > unsigned long call_user_ID) > { > struct afs_call *call = (struct afs_call *)call_user_ID; > - int u; > + int r; > > trace_afs_notify_call(rxcall, call); > call->need_attention = true; > > - u = atomic_fetch_add_unless(&call->usage, 1, 0); > - if (u != 0) { > - trace_afs_call(call, afs_call_trace_wake, u + 1, > + if (__refcount_inc_not_zero(&call->ref, &r)) { > + trace_afs_call(call, 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 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); > diff --git a/fs/afs/vl_list.c b/fs/afs/vl_list.c > index 38b2ba1d9ec0..acc48216136a 100644 > --- a/fs/afs/vl_list.c > +++ b/fs/afs/vl_list.c > @@ -17,7 +17,7 @@ struct afs_vlserver *afs_alloc_vlserver(const char *name, size_t name_len, > vlserver = kzalloc(struct_size(vlserver, name, name_len + 1), > GFP_KERNEL); > if (vlserver) { > - atomic_set(&vlserver->usage, 1); > + refcount_set(&vlserver->ref, 1); > rwlock_init(&vlserver->lock); > init_waitqueue_head(&vlserver->probe_wq); > spin_lock_init(&vlserver->probe_lock); > @@ -39,13 +39,9 @@ static void afs_vlserver_rcu(struct rcu_head *rcu) > > void afs_put_vlserver(struct afs_net *net, struct afs_vlserver *vlserver) > { > - if (vlserver) { > - unsigned int u = atomic_dec_return(&vlserver->usage); > - //_debug("VL PUT %p{%u}", vlserver, u); > - > - if (u == 0) > - call_rcu(&vlserver->rcu, afs_vlserver_rcu); > - } > + if (vlserver && > + refcount_dec_and_test(&vlserver->ref)) > + call_rcu(&vlserver->rcu, afs_vlserver_rcu); > } > > struct afs_vlserver_list *afs_alloc_vlserver_list(unsigned int nr_servers) > @@ -54,7 +50,7 @@ struct afs_vlserver_list *afs_alloc_vlserver_list(unsigned int nr_servers) > > vllist = kzalloc(struct_size(vllist, servers, nr_servers), GFP_KERNEL); > if (vllist) { > - atomic_set(&vllist->usage, 1); > + refcount_set(&vllist->ref, 1); > rwlock_init(&vllist->lock); > } > > @@ -64,10 +60,7 @@ struct afs_vlserver_list *afs_alloc_vlserver_list(unsigned int nr_servers) > void afs_put_vlserverlist(struct afs_net *net, struct afs_vlserver_list *vllist) > { > if (vllist) { > - unsigned int u = atomic_dec_return(&vllist->usage); > - > - //_debug("VLLS PUT %p{%u}", vllist, u); > - if (u == 0) { > + if (refcount_dec_and_test(&vllist->ref)) { > int i; > > for (i = 0; i < vllist->nr_servers; i++) { > diff --git a/fs/afs/volume.c b/fs/afs/volume.c > index cc665cef0abe..f4937029dcd7 100644 > --- a/fs/afs/volume.c > +++ b/fs/afs/volume.c > @@ -52,7 +52,7 @@ static void afs_remove_volume_from_cell(struct afs_volume *volume) > struct afs_cell *cell = volume->cell; > > if (!hlist_unhashed(&volume->proc_link)) { > - trace_afs_volume(volume->vid, atomic_read(&volume->usage), > + trace_afs_volume(volume->vid, refcount_read(&cell->ref), > afs_volume_trace_remove); > write_seqlock(&cell->volume_lock); > hlist_del_rcu(&volume->proc_link); > @@ -87,7 +87,7 @@ static struct afs_volume *afs_alloc_volume(struct afs_fs_context *params, > volume->type_force = params->force; > volume->name_len = vldb->name_len; > > - atomic_set(&volume->usage, 1); > + refcount_set(&volume->ref, 1); > INIT_HLIST_NODE(&volume->proc_link); > rwlock_init(&volume->servers_lock); > rwlock_init(&volume->cb_v_break_lock); > @@ -228,7 +228,7 @@ static void afs_destroy_volume(struct afs_net *net, struct afs_volume *volume) > afs_remove_volume_from_cell(volume); > afs_put_serverlist(net, rcu_access_pointer(volume->servers)); > afs_put_cell(volume->cell, afs_cell_trace_put_vol); > - trace_afs_volume(volume->vid, atomic_read(&volume->usage), > + trace_afs_volume(volume->vid, refcount_read(&volume->ref), > afs_volume_trace_free); > kfree_rcu(volume, rcu); > > @@ -242,8 +242,10 @@ struct afs_volume *afs_get_volume(struct afs_volume *volume, > enum afs_volume_trace reason) > { > if (volume) { > - int u = atomic_inc_return(&volume->usage); > - trace_afs_volume(volume->vid, u, reason); > + int r; > + > + __refcount_inc(&volume->ref, &r); > + trace_afs_volume(volume->vid, r + 1, reason); > } > return volume; > } > @@ -257,9 +259,12 @@ void afs_put_volume(struct afs_net *net, struct afs_volume *volume, > { > if (volume) { > afs_volid_t vid = volume->vid; > - int u = atomic_dec_return(&volume->usage); > - trace_afs_volume(vid, u, reason); > - if (u == 0) > + bool zero; > + int r; > + > + zero = __refcount_dec_and_test(&volume->ref, &r); > + trace_afs_volume(vid, r - 1, reason); > + if (zero) > afs_destroy_volume(net, volume); > } > } > diff --git a/include/trace/events/afs.h b/include/trace/events/afs.h > index 499f5fabd20f..aa60f42a9763 100644 > --- a/include/trace/events/afs.h > +++ b/include/trace/events/afs.h > @@ -728,14 +728,14 @@ TRACE_EVENT(afs_cb_call, > > TRACE_EVENT(afs_call, > TP_PROTO(struct afs_call *call, enum afs_call_trace op, > - int usage, int outstanding, const void *where), > + int ref, int outstanding, const void *where), > > - TP_ARGS(call, op, usage, outstanding, where), > + TP_ARGS(call, op, ref, outstanding, where), > > TP_STRUCT__entry( > __field(unsigned int, call ) > __field(int, op ) > - __field(int, usage ) > + __field(int, ref ) > __field(int, outstanding ) > __field(const void *, where ) > ), > @@ -743,15 +743,15 @@ TRACE_EVENT(afs_call, > TP_fast_assign( > __entry->call = call->debug_id; > __entry->op = op; > - __entry->usage = usage; > + __entry->ref = ref; > __entry->outstanding = outstanding; > __entry->where = where; > ), > > - TP_printk("c=%08x %s u=%d o=%d sp=%pSR", > + TP_printk("c=%08x %s r=%d o=%d sp=%pSR", > __entry->call, > __print_symbolic(__entry->op, afs_call_traces), > - __entry->usage, > + __entry->ref, > __entry->outstanding, > __entry->where) > ); > @@ -1476,36 +1476,36 @@ TRACE_EVENT(afs_volume, > __entry->reason = reason; > ), > > - TP_printk("V=%llx %s u=%d", > + TP_printk("V=%llx %s ur=%d", > __entry->vid, > __print_symbolic(__entry->reason, afs_volume_traces), > __entry->ref) > ); > > TRACE_EVENT(afs_cell, > - TP_PROTO(unsigned int cell_debug_id, int usage, int active, > + TP_PROTO(unsigned int cell_debug_id, int ref, int active, > enum afs_cell_trace reason), > > - TP_ARGS(cell_debug_id, usage, active, reason), > + TP_ARGS(cell_debug_id, ref, active, reason), > > TP_STRUCT__entry( > __field(unsigned int, cell ) > - __field(int, usage ) > + __field(int, ref ) > __field(int, active ) > __field(int, reason ) > ), > > TP_fast_assign( > __entry->cell = cell_debug_id; > - __entry->usage = usage; > + __entry->ref = ref; > __entry->active = active; > __entry->reason = reason; > ), > > - TP_printk("L=%08x %s u=%d a=%d", > + TP_printk("L=%08x %s r=%d a=%d", > __entry->cell, > __print_symbolic(__entry->reason, afs_cell_traces), > - __entry->usage, > + __entry->ref, > __entry->active) > ); Reviewed-by: Marc Dionne <marc.dionne@auristor.com> Marc ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] afs: Fix access after dec in put functions 2022-07-29 16:39 [PATCH 0/2] afs: Fix ref-put functions David Howells 2022-07-29 16:39 ` [PATCH 1/2] afs: Use refcount_t rather than atomic_t David Howells @ 2022-07-29 16:39 ` David Howells 2022-08-02 14:54 ` Marc Dionne ` (2 more replies) 1 sibling, 3 replies; 8+ messages in thread From: David Howells @ 2022-07-29 16:39 UTC (permalink / raw) To: linux-afs; +Cc: Marc Dionne, dhowells, Marc Dionne, linux-fsdevel, linux-kernel 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. 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 <dhowells@redhat.com> cc: Marc Dionne <marc.dionne@auristor.com> cc: linux-afs@lists.infradead.org --- 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(-) 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..bca4b4c55c14 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; 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(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; ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] afs: Fix access after dec in put functions 2022-07-29 16:39 ` [PATCH 2/2] afs: Fix access after dec in put functions David Howells @ 2022-08-02 14:54 ` Marc Dionne 2022-08-02 15:49 ` David Howells 2022-08-02 17:14 ` David Howells 2 siblings, 0 replies; 8+ messages in thread From: Marc Dionne @ 2022-08-02 14:54 UTC (permalink / raw) To: David Howells; +Cc: linux-afs, linux-fsdevel, Linux Kernel Mailing List On Fri, Jul 29, 2022 at 1:40 PM David Howells <dhowells@redhat.com> wrote: > > 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. > > 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 <dhowells@redhat.com> > cc: Marc Dionne <marc.dionne@auristor.com> > cc: linux-afs@lists.infradead.org > --- > > 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(-) > > 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..bca4b4c55c14 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; > 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(server->debug_id, r - 1, a, reason); Don't you also want to copy server->debug_id into a local variable here? > 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; > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] afs: Fix access after dec in put functions 2022-07-29 16:39 ` [PATCH 2/2] afs: Fix access after dec in put functions David Howells 2022-08-02 14:54 ` Marc Dionne @ 2022-08-02 15:49 ` David Howells 2022-08-02 17:14 ` David Howells 2 siblings, 0 replies; 8+ messages in thread From: David Howells @ 2022-08-02 15:49 UTC (permalink / raw) To: Marc Dionne; +Cc: dhowells, linux-afs, linux-fsdevel, Linux Kernel Mailing List Marc Dionne <marc.dionne@auristor.com> wrote: > > - trace_afs_server(server, r - 1, atomic_read(&server->active), reason); > > + trace_afs_server(server->debug_id, r - 1, a, reason); > > Don't you also want to copy server->debug_id into a local variable here? Bah. Yes. David ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] afs: Fix access after dec in put functions 2022-07-29 16:39 ` [PATCH 2/2] afs: Fix access after dec in put functions David Howells 2022-08-02 14:54 ` Marc Dionne 2022-08-02 15:49 ` David Howells @ 2022-08-02 17:14 ` David Howells 2022-08-02 17:27 ` Marc Dionne 2 siblings, 1 reply; 8+ messages in thread From: David Howells @ 2022-08-02 17:14 UTC (permalink / raw) To: Marc Dionne; +Cc: dhowells, linux-afs, linux-fsdevel, Linux Kernel Mailing List Marc Dionne <marc.dionne@auristor.com> wrote: > > - trace_afs_server(server, r - 1, atomic_read(&server->active), reason); > > + trace_afs_server(server->debug_id, r - 1, a, reason); > > Don't you also want to copy server->debug_id into a local variable here? Okay, how about the attached change? David --- diff --git a/fs/afs/server.c b/fs/afs/server.c index bca4b4c55c14..4981baf97835 100644 --- a/fs/afs/server.c +++ b/fs/afs/server.c @@ -399,7 +399,7 @@ 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; + unsigned int a, debug_id = server->debug_id; bool zero; int r; @@ -408,7 +408,7 @@ void afs_put_server(struct afs_net *net, struct afs_server *server, a = atomic_inc_return(&server->active); zero = __refcount_dec_and_test(&server->ref, &r); - trace_afs_server(server->debug_id, r - 1, a, reason); + trace_afs_server(debug_id, r - 1, a, reason); if (unlikely(zero)) __afs_put_server(net, server); } ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] afs: Fix access after dec in put functions 2022-08-02 17:14 ` David Howells @ 2022-08-02 17:27 ` Marc Dionne 0 siblings, 0 replies; 8+ messages in thread From: Marc Dionne @ 2022-08-02 17:27 UTC (permalink / raw) To: David Howells; +Cc: linux-afs, linux-fsdevel, Linux Kernel Mailing List On Tue, Aug 2, 2022 at 2:15 PM David Howells <dhowells@redhat.com> wrote: > > Marc Dionne <marc.dionne@auristor.com> wrote: > > > > - trace_afs_server(server, r - 1, atomic_read(&server->active), reason); > > > + trace_afs_server(server->debug_id, r - 1, a, reason); > > > > Don't you also want to copy server->debug_id into a local variable here? > > Okay, how about the attached change? > > David > --- > diff --git a/fs/afs/server.c b/fs/afs/server.c > index bca4b4c55c14..4981baf97835 100644 > --- a/fs/afs/server.c > +++ b/fs/afs/server.c > @@ -399,7 +399,7 @@ 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; > + unsigned int a, debug_id = server->debug_id; > bool zero; > int r; > > @@ -408,7 +408,7 @@ void afs_put_server(struct afs_net *net, struct afs_server *server, > > a = atomic_inc_return(&server->active); > zero = __refcount_dec_and_test(&server->ref, &r); > - trace_afs_server(server->debug_id, r - 1, a, reason); > + trace_afs_server(debug_id, r - 1, a, reason); > if (unlikely(zero)) > __afs_put_server(net, server); > } Looks fine with that change. Marc ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-08-02 17:27 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-07-29 16:39 [PATCH 0/2] afs: Fix ref-put functions David Howells 2022-07-29 16:39 ` [PATCH 1/2] afs: Use refcount_t rather than atomic_t David Howells 2022-08-02 14:27 ` Marc Dionne 2022-07-29 16:39 ` [PATCH 2/2] afs: Fix access after dec in put functions David Howells 2022-08-02 14:54 ` Marc Dionne 2022-08-02 15:49 ` David Howells 2022-08-02 17:14 ` David Howells 2022-08-02 17:27 ` Marc Dionne
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).