Skip to content

Commit 98dbec0

Browse files
committed
Merge branch 'rxrpc-fixes'
David Howells says: ==================== rxrpc: Fixes for I/O thread conversion/SACK table expansion Here are some fixes for AF_RXRPC: (1) Fix missing unlock in rxrpc's sendmsg. (2) Fix (lack of) propagation of security settings to rxrpc_call. (3) Fix NULL ptr deref in rxrpc_unuse_local(). (4) Fix problem with kthread_run() not invoking the I/O thread function if the kthread gets stopped first. Possibly this should actually be fixed in the kthread code. (5) Fix locking problem as putting a peer (which may be done from RCU) may now invoke kthread_stop(). (6) Fix switched parameters in a couple of trace calls. (7) Fix I/O thread's checking for kthread stop to make sure it completes all outstanding work before returning so that calls are cleaned up. (8) Fix an uninitialised var in the new rxperf test server. (9) Fix the return value of rxrpc_new_incoming_call() so that the checks on it work correctly. The patches fix at least one syzbot bug[1] and probably some others that don't have reproducers[2][3][4]. I think it also fixes another[5], but that showed another failure during testing that was different to the original. There's also an outstanding bug in rxrpc_put_peer()[6] that is fixed by a combination of several patches in my rxrpc-next branch, but I haven't included that here. ==================== Tested-by: Marc Dionne <[email protected]> Tested-by: [email protected] Signed-off-by: David S. Miller <[email protected]>
2 parents 9cd3fd2 + 31d35a0 commit 98dbec0

File tree

12 files changed

+40
-49
lines changed

12 files changed

+40
-49
lines changed

include/trace/events/rxrpc.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -471,7 +471,7 @@ TRACE_EVENT(rxrpc_peer,
471471
TP_STRUCT__entry(
472472
__field(unsigned int, peer )
473473
__field(int, ref )
474-
__field(int, why )
474+
__field(enum rxrpc_peer_trace, why )
475475
),
476476

477477
TP_fast_assign(

net/rxrpc/ar-internal.h

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -287,6 +287,7 @@ struct rxrpc_local {
287287
struct hlist_node link;
288288
struct socket *socket; /* my UDP socket */
289289
struct task_struct *io_thread;
290+
struct completion io_thread_ready; /* Indication that the I/O thread started */
290291
struct rxrpc_sock __rcu *service; /* Service(s) listening on this endpoint */
291292
struct rw_semaphore defrag_sem; /* control re-enablement of IP DF bit */
292293
struct sk_buff_head rx_queue; /* Received packets */
@@ -811,9 +812,9 @@ extern struct workqueue_struct *rxrpc_workqueue;
811812
*/
812813
int rxrpc_service_prealloc(struct rxrpc_sock *, gfp_t);
813814
void rxrpc_discard_prealloc(struct rxrpc_sock *);
814-
bool rxrpc_new_incoming_call(struct rxrpc_local *, struct rxrpc_peer *,
815-
struct rxrpc_connection *, struct sockaddr_rxrpc *,
816-
struct sk_buff *);
815+
int rxrpc_new_incoming_call(struct rxrpc_local *, struct rxrpc_peer *,
816+
struct rxrpc_connection *, struct sockaddr_rxrpc *,
817+
struct sk_buff *);
817818
void rxrpc_accept_incoming_calls(struct rxrpc_local *);
818819
int rxrpc_user_charge_accept(struct rxrpc_sock *, unsigned long);
819820

@@ -1072,7 +1073,6 @@ void rxrpc_destroy_all_peers(struct rxrpc_net *);
10721073
struct rxrpc_peer *rxrpc_get_peer(struct rxrpc_peer *, enum rxrpc_peer_trace);
10731074
struct rxrpc_peer *rxrpc_get_peer_maybe(struct rxrpc_peer *, enum rxrpc_peer_trace);
10741075
void rxrpc_put_peer(struct rxrpc_peer *, enum rxrpc_peer_trace);
1075-
void rxrpc_put_peer_locked(struct rxrpc_peer *, enum rxrpc_peer_trace);
10761076

10771077
/*
10781078
* proc.c

net/rxrpc/call_accept.c

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -326,11 +326,11 @@ static struct rxrpc_call *rxrpc_alloc_incoming_call(struct rxrpc_sock *rx,
326326
* If we want to report an error, we mark the skb with the packet type and
327327
* abort code and return false.
328328
*/
329-
bool rxrpc_new_incoming_call(struct rxrpc_local *local,
330-
struct rxrpc_peer *peer,
331-
struct rxrpc_connection *conn,
332-
struct sockaddr_rxrpc *peer_srx,
333-
struct sk_buff *skb)
329+
int rxrpc_new_incoming_call(struct rxrpc_local *local,
330+
struct rxrpc_peer *peer,
331+
struct rxrpc_connection *conn,
332+
struct sockaddr_rxrpc *peer_srx,
333+
struct sk_buff *skb)
334334
{
335335
const struct rxrpc_security *sec = NULL;
336336
struct rxrpc_skb_priv *sp = rxrpc_skb(skb);
@@ -342,7 +342,7 @@ bool rxrpc_new_incoming_call(struct rxrpc_local *local,
342342
/* Don't set up a call for anything other than the first DATA packet. */
343343
if (sp->hdr.seq != 1 ||
344344
sp->hdr.type != RXRPC_PACKET_TYPE_DATA)
345-
return true; /* Just discard */
345+
return 0; /* Just discard */
346346

347347
rcu_read_lock();
348348

@@ -413,7 +413,7 @@ bool rxrpc_new_incoming_call(struct rxrpc_local *local,
413413
_leave(" = %p{%d}", call, call->debug_id);
414414
rxrpc_input_call_event(call, skb);
415415
rxrpc_put_call(call, rxrpc_call_put_input);
416-
return true;
416+
return 0;
417417

418418
unsupported_service:
419419
trace_rxrpc_abort(0, "INV", sp->hdr.cid, sp->hdr.callNumber, sp->hdr.seq,
@@ -425,10 +425,10 @@ bool rxrpc_new_incoming_call(struct rxrpc_local *local,
425425
reject:
426426
rcu_read_unlock();
427427
_leave(" = f [%u]", skb->mark);
428-
return false;
428+
return -EPROTO;
429429
discard:
430430
rcu_read_unlock();
431-
return true;
431+
return 0;
432432
}
433433

434434
/*

net/rxrpc/call_object.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -217,6 +217,7 @@ static struct rxrpc_call *rxrpc_alloc_client_call(struct rxrpc_sock *rx,
217217
call->tx_total_len = p->tx_total_len;
218218
call->key = key_get(cp->key);
219219
call->local = rxrpc_get_local(cp->local, rxrpc_local_get_call);
220+
call->security_level = cp->security_level;
220221
if (p->kernel)
221222
__set_bit(RXRPC_CALL_KERNEL, &call->flags);
222223
if (cp->upgrade)

net/rxrpc/conn_client.c

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -551,8 +551,6 @@ static void rxrpc_activate_one_channel(struct rxrpc_connection *conn,
551551
call->conn = rxrpc_get_connection(conn, rxrpc_conn_get_activate_call);
552552
call->cid = conn->proto.cid | channel;
553553
call->call_id = call_id;
554-
call->security = conn->security;
555-
call->security_ix = conn->security_ix;
556554
call->dest_srx.srx_service = conn->service_id;
557555

558556
trace_rxrpc_connect_call(call);

net/rxrpc/io_thread.c

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -292,7 +292,7 @@ static int rxrpc_input_packet(struct rxrpc_local *local, struct sk_buff **_skb)
292292
skb->mark = RXRPC_SKB_MARK_REJECT_ABORT;
293293
reject_packet:
294294
rxrpc_reject_packet(local, skb);
295-
return ret;
295+
return 0;
296296
}
297297

298298
/*
@@ -384,7 +384,7 @@ static int rxrpc_input_packet_on_conn(struct rxrpc_connection *conn,
384384
if (rxrpc_to_client(sp))
385385
goto bad_message;
386386
if (rxrpc_new_incoming_call(conn->local, conn->peer, conn,
387-
peer_srx, skb))
387+
peer_srx, skb) == 0)
388388
return 0;
389389
goto reject_packet;
390390
}
@@ -425,6 +425,9 @@ int rxrpc_io_thread(void *data)
425425
struct rxrpc_local *local = data;
426426
struct rxrpc_call *call;
427427
struct sk_buff *skb;
428+
bool should_stop;
429+
430+
complete(&local->io_thread_ready);
428431

429432
skb_queue_head_init(&rx_queue);
430433

@@ -476,13 +479,14 @@ int rxrpc_io_thread(void *data)
476479
}
477480

478481
set_current_state(TASK_INTERRUPTIBLE);
482+
should_stop = kthread_should_stop();
479483
if (!skb_queue_empty(&local->rx_queue) ||
480484
!list_empty(&local->call_attend_q)) {
481485
__set_current_state(TASK_RUNNING);
482486
continue;
483487
}
484488

485-
if (kthread_should_stop())
489+
if (should_stop)
486490
break;
487491
schedule();
488492
}

net/rxrpc/local_object.c

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,7 @@ static struct rxrpc_local *rxrpc_alloc_local(struct rxrpc_net *rxnet,
9797
local->rxnet = rxnet;
9898
INIT_HLIST_NODE(&local->link);
9999
init_rwsem(&local->defrag_sem);
100+
init_completion(&local->io_thread_ready);
100101
skb_queue_head_init(&local->rx_queue);
101102
INIT_LIST_HEAD(&local->call_attend_q);
102103
local->client_bundles = RB_ROOT;
@@ -189,6 +190,7 @@ static int rxrpc_open_socket(struct rxrpc_local *local, struct net *net)
189190
goto error_sock;
190191
}
191192

193+
wait_for_completion(&local->io_thread_ready);
192194
local->io_thread = io_thread;
193195
_leave(" = 0");
194196
return 0;
@@ -357,10 +359,11 @@ struct rxrpc_local *rxrpc_use_local(struct rxrpc_local *local,
357359
*/
358360
void rxrpc_unuse_local(struct rxrpc_local *local, enum rxrpc_local_trace why)
359361
{
360-
unsigned int debug_id = local->debug_id;
362+
unsigned int debug_id;
361363
int r, u;
362364

363365
if (local) {
366+
debug_id = local->debug_id;
364367
r = refcount_read(&local->ref);
365368
u = atomic_dec_return(&local->active_users);
366369
trace_rxrpc_local(debug_id, why, r, u);

net/rxrpc/peer_event.c

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -235,6 +235,7 @@ static void rxrpc_peer_keepalive_dispatch(struct rxrpc_net *rxnet,
235235
struct rxrpc_peer *peer;
236236
const u8 mask = ARRAY_SIZE(rxnet->peer_keepalive) - 1;
237237
time64_t keepalive_at;
238+
bool use;
238239
int slot;
239240

240241
spin_lock(&rxnet->peer_hash_lock);
@@ -247,9 +248,10 @@ static void rxrpc_peer_keepalive_dispatch(struct rxrpc_net *rxnet,
247248
if (!rxrpc_get_peer_maybe(peer, rxrpc_peer_get_keepalive))
248249
continue;
249250

250-
if (__rxrpc_use_local(peer->local, rxrpc_local_use_peer_keepalive)) {
251-
spin_unlock(&rxnet->peer_hash_lock);
251+
use = __rxrpc_use_local(peer->local, rxrpc_local_use_peer_keepalive);
252+
spin_unlock(&rxnet->peer_hash_lock);
252253

254+
if (use) {
253255
keepalive_at = peer->last_tx_at + RXRPC_KEEPALIVE_TIME;
254256
slot = keepalive_at - base;
255257
_debug("%02x peer %u t=%d {%pISp}",
@@ -270,9 +272,11 @@ static void rxrpc_peer_keepalive_dispatch(struct rxrpc_net *rxnet,
270272
spin_lock(&rxnet->peer_hash_lock);
271273
list_add_tail(&peer->keepalive_link,
272274
&rxnet->peer_keepalive[slot & mask]);
275+
spin_unlock(&rxnet->peer_hash_lock);
273276
rxrpc_unuse_local(peer->local, rxrpc_local_unuse_peer_keepalive);
274277
}
275-
rxrpc_put_peer_locked(peer, rxrpc_peer_put_keepalive);
278+
rxrpc_put_peer(peer, rxrpc_peer_put_keepalive);
279+
spin_lock(&rxnet->peer_hash_lock);
276280
}
277281

278282
spin_unlock(&rxnet->peer_hash_lock);

net/rxrpc/peer_object.c

Lines changed: 2 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -226,7 +226,7 @@ struct rxrpc_peer *rxrpc_alloc_peer(struct rxrpc_local *local, gfp_t gfp,
226226
rxrpc_peer_init_rtt(peer);
227227

228228
peer->cong_ssthresh = RXRPC_TX_MAX_WINDOW;
229-
trace_rxrpc_peer(peer->debug_id, why, 1);
229+
trace_rxrpc_peer(peer->debug_id, 1, why);
230230
}
231231

232232
_leave(" = %p", peer);
@@ -382,7 +382,7 @@ struct rxrpc_peer *rxrpc_get_peer(struct rxrpc_peer *peer, enum rxrpc_peer_trace
382382
int r;
383383

384384
__refcount_inc(&peer->ref, &r);
385-
trace_rxrpc_peer(peer->debug_id, why, r + 1);
385+
trace_rxrpc_peer(peer->debug_id, r + 1, why);
386386
return peer;
387387
}
388388

@@ -438,25 +438,6 @@ void rxrpc_put_peer(struct rxrpc_peer *peer, enum rxrpc_peer_trace why)
438438
}
439439
}
440440

441-
/*
442-
* Drop a ref on a peer record where the caller already holds the
443-
* peer_hash_lock.
444-
*/
445-
void rxrpc_put_peer_locked(struct rxrpc_peer *peer, enum rxrpc_peer_trace why)
446-
{
447-
unsigned int debug_id = peer->debug_id;
448-
bool dead;
449-
int r;
450-
451-
dead = __refcount_dec_and_test(&peer->ref, &r);
452-
trace_rxrpc_peer(debug_id, r - 1, why);
453-
if (dead) {
454-
hash_del_rcu(&peer->hash_link);
455-
list_del_init(&peer->keepalive_link);
456-
rxrpc_free_peer(peer);
457-
}
458-
}
459-
460441
/*
461442
* Make sure all peer records have been discarded.
462443
*/

net/rxrpc/rxperf.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -275,7 +275,7 @@ static void rxperf_deliver_to_call(struct work_struct *work)
275275
struct rxperf_call *call = container_of(work, struct rxperf_call, work);
276276
enum rxperf_call_state state;
277277
u32 abort_code, remote_abort = 0;
278-
int ret;
278+
int ret = 0;
279279

280280
if (call->state == RXPERF_CALL_COMPLETE)
281281
return;

net/rxrpc/security.c

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -67,13 +67,13 @@ const struct rxrpc_security *rxrpc_security_lookup(u8 security_index)
6767
*/
6868
int rxrpc_init_client_call_security(struct rxrpc_call *call)
6969
{
70-
const struct rxrpc_security *sec;
70+
const struct rxrpc_security *sec = &rxrpc_no_security;
7171
struct rxrpc_key_token *token;
7272
struct key *key = call->key;
7373
int ret;
7474

7575
if (!key)
76-
return 0;
76+
goto found;
7777

7878
ret = key_validate(key);
7979
if (ret < 0)
@@ -88,7 +88,7 @@ int rxrpc_init_client_call_security(struct rxrpc_call *call)
8888

8989
found:
9090
call->security = sec;
91-
_leave(" = 0");
91+
call->security_ix = sec->security_index;
9292
return 0;
9393
}
9494

net/rxrpc/sendmsg.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -625,7 +625,7 @@ int rxrpc_do_sendmsg(struct rxrpc_sock *rx, struct msghdr *msg, size_t len)
625625
if (call->tx_total_len != -1 ||
626626
call->tx_pending ||
627627
call->tx_top != 0)
628-
goto error_put;
628+
goto out_put_unlock;
629629
call->tx_total_len = p.call.tx_total_len;
630630
}
631631
}

0 commit comments

Comments
 (0)