Skip to content

Commit

Permalink
rxrpc: Fix read-after-free in rxrpc_queue_local()
Browse files Browse the repository at this point in the history
rxrpc_queue_local() attempts to queue the local endpoint it is given and
then, if successful, prints a trace line.  The trace line includes the
current usage count - but we're not allowed to look at the local endpoint
at this point as we passed our ref on it to the workqueue.

Fix this by reading the usage count before queuing the work item.

Also fix the reading of local->debug_id for trace lines, which must be done
with the same consideration as reading the usage count.

Fixes: 09d2bf5 ("rxrpc: Add a tracepoint to track rxrpc_local refcounting")
Reported-by: [email protected]
Signed-off-by: David Howells <[email protected]>
  • Loading branch information
dhowells committed Aug 14, 2019
1 parent b00df84 commit 06d9532
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 12 deletions.
6 changes: 3 additions & 3 deletions include/trace/events/rxrpc.h
Original file line number Diff line number Diff line change
Expand Up @@ -498,10 +498,10 @@ rxrpc_tx_points;
#define E_(a, b) { a, b }

TRACE_EVENT(rxrpc_local,
TP_PROTO(struct rxrpc_local *local, enum rxrpc_local_trace op,
TP_PROTO(unsigned int local_debug_id, enum rxrpc_local_trace op,
int usage, const void *where),

TP_ARGS(local, op, usage, where),
TP_ARGS(local_debug_id, op, usage, where),

TP_STRUCT__entry(
__field(unsigned int, local )
Expand All @@ -511,7 +511,7 @@ TRACE_EVENT(rxrpc_local,
),

TP_fast_assign(
__entry->local = local->debug_id;
__entry->local = local_debug_id;
__entry->op = op;
__entry->usage = usage;
__entry->where = where;
Expand Down
19 changes: 10 additions & 9 deletions net/rxrpc/local_object.c
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ static struct rxrpc_local *rxrpc_alloc_local(struct rxrpc_net *rxnet,
local->debug_id = atomic_inc_return(&rxrpc_debug_id);
memcpy(&local->srx, srx, sizeof(*srx));
local->srx.srx_service = 0;
trace_rxrpc_local(local, rxrpc_local_new, 1, NULL);
trace_rxrpc_local(local->debug_id, rxrpc_local_new, 1, NULL);
}

_leave(" = %p", local);
Expand Down Expand Up @@ -321,7 +321,7 @@ struct rxrpc_local *rxrpc_get_local(struct rxrpc_local *local)
int n;

n = atomic_inc_return(&local->usage);
trace_rxrpc_local(local, rxrpc_local_got, n, here);
trace_rxrpc_local(local->debug_id, rxrpc_local_got, n, here);
return local;
}

Expand All @@ -335,24 +335,25 @@ struct rxrpc_local *rxrpc_get_local_maybe(struct rxrpc_local *local)
if (local) {
int n = atomic_fetch_add_unless(&local->usage, 1, 0);
if (n > 0)
trace_rxrpc_local(local, rxrpc_local_got, n + 1, here);
trace_rxrpc_local(local->debug_id, rxrpc_local_got,
n + 1, here);
else
local = NULL;
}
return local;
}

/*
* Queue a local endpoint unless it has become unreferenced and pass the
* caller's reference to the work item.
* Queue a local endpoint and pass the caller's reference to the work item.
*/
void rxrpc_queue_local(struct rxrpc_local *local)
{
const void *here = __builtin_return_address(0);
unsigned int debug_id = local->debug_id;
int n = atomic_read(&local->usage);

if (rxrpc_queue_work(&local->processor))
trace_rxrpc_local(local, rxrpc_local_queued,
atomic_read(&local->usage), here);
trace_rxrpc_local(debug_id, rxrpc_local_queued, n, here);
else
rxrpc_put_local(local);
}
Expand All @@ -367,7 +368,7 @@ void rxrpc_put_local(struct rxrpc_local *local)

if (local) {
n = atomic_dec_return(&local->usage);
trace_rxrpc_local(local, rxrpc_local_put, n, here);
trace_rxrpc_local(local->debug_id, rxrpc_local_put, n, here);

if (n == 0)
call_rcu(&local->rcu, rxrpc_local_rcu);
Expand Down Expand Up @@ -456,7 +457,7 @@ static void rxrpc_local_processor(struct work_struct *work)
container_of(work, struct rxrpc_local, processor);
bool again;

trace_rxrpc_local(local, rxrpc_local_processing,
trace_rxrpc_local(local->debug_id, rxrpc_local_processing,
atomic_read(&local->usage), NULL);

do {
Expand Down

0 comments on commit 06d9532

Please sign in to comment.