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.
The patches are tagged here:
git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git
rxrpc-fixes-20221215
and can also be found on the following branch:
https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=rxrpc-fixes
David
Link: https://syzkaller.appspot.com/bug?extid=3538a6a72efa8b059c38 [1]
Link: https://syzkaller.appspot.com/bug?extid=2a99eae8dc7c754bc16b [2]
Link: https://syzkaller.appspot.com/bug?extid=e1391a5bf3f779e31237 [3]
Link: https://syzkaller.appspot.com/bug?extid=2aea8e1c8e20cb27a01f [4]
Link: https://syzkaller.appspot.com/bug?extid=1eb4232fca28c0a6d1c2 [5]
Link: https://syzkaller.appspot.com/bug?extid=c22650d2844392afdcfd [6]
---
David Howells (9):
rxrpc: Fix missing unlock in rxrpc_do_sendmsg()
rxrpc: Fix security setting propagation
rxrpc: Fix NULL deref in rxrpc_unuse_local()
rxrpc: Fix I/O thread startup getting skipped
rxrpc: Fix locking issues in rxrpc_put_peer_locked()
rxrpc: Fix switched parameters in peer tracing
rxrpc: Fix I/O thread stop
rxrpc: rxperf: Fix uninitialised variable
rxrpc: Fix the return value of rxrpc_new_incoming_call()
include/trace/events/rxrpc.h | 2 +-
net/rxrpc/ar-internal.h | 8 ++++----
net/rxrpc/call_accept.c | 18 +++++++++---------
net/rxrpc/call_object.c | 1 +
net/rxrpc/conn_client.c | 2 --
net/rxrpc/io_thread.c | 10 +++++++---
net/rxrpc/local_object.c | 5 ++++-
net/rxrpc/peer_event.c | 10 +++++++---
net/rxrpc/peer_object.c | 23 ++---------------------
net/rxrpc/rxperf.c | 2 +-
net/rxrpc/security.c | 6 +++---
net/rxrpc/sendmsg.c | 2 +-
12 files changed, 40 insertions(+), 49 deletions(-)
Dan Carpenter sayeth[1]:
The patch 75bfdbf2fca3: "rxrpc: Implement an in-kernel rxperf server
for testing purposes" from Nov 3, 2022, leads to the following Smatch
static checker warning:
net/rxrpc/rxperf.c:337 rxperf_deliver_to_call()
error: uninitialized symbol 'ret'.
Fix this by initialising ret to 0. The value is only used for tracing
purposes in the rxperf server.
Fixes: 75bfdbf2fca3 ("rxrpc: Implement an in-kernel rxperf server for testing purposes")
Reported-by: Dan Carpenter <[email protected]>
Signed-off-by: David Howells <[email protected]>
cc: Marc Dionne <[email protected]>
cc: [email protected]
Link: http://lists.infradead.org/pipermail/linux-afs/2022-December/006124.html [1]
---
net/rxrpc/rxperf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/rxrpc/rxperf.c b/net/rxrpc/rxperf.c
index 66f5eea291ff..d33a109e846c 100644
--- a/net/rxrpc/rxperf.c
+++ b/net/rxrpc/rxperf.c
@@ -275,7 +275,7 @@ static void rxperf_deliver_to_call(struct work_struct *work)
struct rxperf_call *call = container_of(work, struct rxperf_call, work);
enum rxperf_call_state state;
u32 abort_code, remote_abort = 0;
- int ret;
+ int ret = 0;
if (call->state == RXPERF_CALL_COMPLETE)
return;
The rxrpc I/O thread checks to see if there's any work it needs to do, and
if not, checks kthread_should_stop() before scheduling, and if it should
stop, breaks out of the loop and tries to clean up and exit.
This can, however, race with socket destruction, wherein outstanding calls
are aborted and released from the socket and then the socket unuses the
local endpoint, causing kthread_stop() to be issued. The abort is deferred
to the I/O thread and the event can by issued between the I/O thread
checking if there's any work to be done (such as processing call aborts)
and the stop being seen.
This results in the I/O thread stopping processing of events whilst call
cleanup events are still outstanding, leading to connections or other
objects still being around and uncleaned up, which can result in assertions
being triggered, e.g.:
rxrpc: AF_RXRPC: Leaked client conn 00000000e8009865 {2}
------------[ cut here ]------------
kernel BUG at net/rxrpc/conn_client.c:64!
Fix this by retrieving the kthread_should_stop() indication, then checking
to see if there's more work to do, and going back round the loop if there
is, and breaking out of the loop only if there wasn't.
This was triggered by a syzbot test that produced some other symptom[1].
Fixes: a275da62e8c1 ("rxrpc: Create a per-local endpoint receive queue and I/O thread")
Signed-off-by: David Howells <[email protected]>
cc: Marc Dionne <[email protected]>
cc: [email protected]
Link: https://lore.kernel.org/r/[email protected]/ [1]
---
net/rxrpc/io_thread.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/net/rxrpc/io_thread.c b/net/rxrpc/io_thread.c
index e460e4151c16..e6b9f0ceae17 100644
--- a/net/rxrpc/io_thread.c
+++ b/net/rxrpc/io_thread.c
@@ -425,6 +425,7 @@ int rxrpc_io_thread(void *data)
struct rxrpc_local *local = data;
struct rxrpc_call *call;
struct sk_buff *skb;
+ bool should_stop;
complete(&local->io_thread_ready);
@@ -478,13 +479,14 @@ int rxrpc_io_thread(void *data)
}
set_current_state(TASK_INTERRUPTIBLE);
+ should_stop = kthread_should_stop();
if (!skb_queue_empty(&local->rx_queue) ||
!list_empty(&local->call_attend_q)) {
__set_current_state(TASK_RUNNING);
continue;
}
- if (kthread_should_stop())
+ if (should_stop)
break;
schedule();
}
Dan Carpenter sayeth[1]:
The patch 5e6ef4f1017c: "rxrpc: Make the I/O thread take over the
call and local processor work" from Jan 23, 2020, leads to the
following Smatch static checker warning:
net/rxrpc/io_thread.c:283 rxrpc_input_packet()
warn: bool is not less than zero.
Fix this (for now) by changing rxrpc_new_incoming_call() to return an int
with 0 or error code rather than bool. Note that the actual return value
of rxrpc_input_packet() is currently ignored. I have a separate patch to
clean that up.
Fixes: 5e6ef4f1017c ("rxrpc: Make the I/O thread take over the call and local processor work")
Reported-by: Dan Carpenter <[email protected]>
Signed-off-by: David Howells <[email protected]>
cc: Marc Dionne <[email protected]>
cc: [email protected]
Link: http://lists.infradead.org/pipermail/linux-afs/2022-December/006123.html [1]
---
net/rxrpc/ar-internal.h | 6 +++---
net/rxrpc/call_accept.c | 18 +++++++++---------
net/rxrpc/io_thread.c | 4 ++--
3 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/net/rxrpc/ar-internal.h b/net/rxrpc/ar-internal.h
index 5b732a4af009..18092526d3c8 100644
--- a/net/rxrpc/ar-internal.h
+++ b/net/rxrpc/ar-internal.h
@@ -812,9 +812,9 @@ extern struct workqueue_struct *rxrpc_workqueue;
*/
int rxrpc_service_prealloc(struct rxrpc_sock *, gfp_t);
void rxrpc_discard_prealloc(struct rxrpc_sock *);
-bool rxrpc_new_incoming_call(struct rxrpc_local *, struct rxrpc_peer *,
- struct rxrpc_connection *, struct sockaddr_rxrpc *,
- struct sk_buff *);
+int rxrpc_new_incoming_call(struct rxrpc_local *, struct rxrpc_peer *,
+ struct rxrpc_connection *, struct sockaddr_rxrpc *,
+ struct sk_buff *);
void rxrpc_accept_incoming_calls(struct rxrpc_local *);
int rxrpc_user_charge_accept(struct rxrpc_sock *, unsigned long);
diff --git a/net/rxrpc/call_accept.c b/net/rxrpc/call_accept.c
index d1850863507f..c02401656fa9 100644
--- a/net/rxrpc/call_accept.c
+++ b/net/rxrpc/call_accept.c
@@ -326,11 +326,11 @@ static struct rxrpc_call *rxrpc_alloc_incoming_call(struct rxrpc_sock *rx,
* If we want to report an error, we mark the skb with the packet type and
* abort code and return false.
*/
-bool rxrpc_new_incoming_call(struct rxrpc_local *local,
- struct rxrpc_peer *peer,
- struct rxrpc_connection *conn,
- struct sockaddr_rxrpc *peer_srx,
- struct sk_buff *skb)
+int rxrpc_new_incoming_call(struct rxrpc_local *local,
+ struct rxrpc_peer *peer,
+ struct rxrpc_connection *conn,
+ struct sockaddr_rxrpc *peer_srx,
+ struct sk_buff *skb)
{
const struct rxrpc_security *sec = NULL;
struct rxrpc_skb_priv *sp = rxrpc_skb(skb);
@@ -342,7 +342,7 @@ bool rxrpc_new_incoming_call(struct rxrpc_local *local,
/* Don't set up a call for anything other than the first DATA packet. */
if (sp->hdr.seq != 1 ||
sp->hdr.type != RXRPC_PACKET_TYPE_DATA)
- return true; /* Just discard */
+ return 0; /* Just discard */
rcu_read_lock();
@@ -413,7 +413,7 @@ bool rxrpc_new_incoming_call(struct rxrpc_local *local,
_leave(" = %p{%d}", call, call->debug_id);
rxrpc_input_call_event(call, skb);
rxrpc_put_call(call, rxrpc_call_put_input);
- return true;
+ return 0;
unsupported_service:
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,
reject:
rcu_read_unlock();
_leave(" = f [%u]", skb->mark);
- return false;
+ return -EPROTO;
discard:
rcu_read_unlock();
- return true;
+ return 0;
}
/*
diff --git a/net/rxrpc/io_thread.c b/net/rxrpc/io_thread.c
index e6b9f0ceae17..1ad067d66fb6 100644
--- a/net/rxrpc/io_thread.c
+++ b/net/rxrpc/io_thread.c
@@ -292,7 +292,7 @@ static int rxrpc_input_packet(struct rxrpc_local *local, struct sk_buff **_skb)
skb->mark = RXRPC_SKB_MARK_REJECT_ABORT;
reject_packet:
rxrpc_reject_packet(local, skb);
- return ret;
+ return 0;
}
/*
@@ -384,7 +384,7 @@ static int rxrpc_input_packet_on_conn(struct rxrpc_connection *conn,
if (rxrpc_to_client(sp))
goto bad_message;
if (rxrpc_new_incoming_call(conn->local, conn->peer, conn,
- peer_srx, skb))
+ peer_srx, skb) == 0)
return 0;
goto reject_packet;
}
One of the error paths in rxrpc_do_sendmsg() doesn't unlock the call mutex
before returning. Fix it to do this.
Note that this still doesn't get rid of the checker warning:
../net/rxrpc/sendmsg.c:617:5: warning: context imbalance in 'rxrpc_do_sendmsg' - wrong count at exit
I think the interplay between the socket lock and the call's user_mutex may
be too complicated for checker to analyse, especially as
rxrpc_new_client_call_for_sendmsg(), which it calls, returns with the
call's user_mutex if successful but unconditionally drops the socket lock.
Fixes: e754eba685aa ("rxrpc: Provide a cmsg to specify the amount of Tx data for a call")
Signed-off-by: David Howells <[email protected]>
cc: Marc Dionne <[email protected]>
cc: [email protected]
---
net/rxrpc/sendmsg.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/rxrpc/sendmsg.c b/net/rxrpc/sendmsg.c
index 9fa7e37f7155..cde1e65f16b4 100644
--- a/net/rxrpc/sendmsg.c
+++ b/net/rxrpc/sendmsg.c
@@ -625,7 +625,7 @@ int rxrpc_do_sendmsg(struct rxrpc_sock *rx, struct msghdr *msg, size_t len)
if (call->tx_total_len != -1 ||
call->tx_pending ||
call->tx_top != 0)
- goto error_put;
+ goto out_put_unlock;
call->tx_total_len = p.call.tx_total_len;
}
}
Now that rxrpc_put_local() may call kthread_stop(), it can't be called
under spinlock as it might sleep. This can cause a problem in the peer
keepalive code in rxrpc as it tries to avoid dropping the peer_hash_lock
from the point it needs to re-add peer->keepalive_link to going round the
loop again in rxrpc_peer_keepalive_dispatch().
Fix this by just dropping the lock when we don't need it and accepting that
we'll have to take it again. This code is only called about every 20s for
each peer, so not very often.
This allows rxrpc_put_peer_unlocked() to be removed also.
If triggered, this bug produces an oops like the following, as reproduced
by a syzbot reproducer for a different oops[1]:
BUG: sleeping function called from invalid context at kernel/sched/completion.c:101
...
RCU nest depth: 0, expected: 0
3 locks held by kworker/u9:0/50:
#0: ffff88810e74a138 ((wq_completion)krxrpcd){+.+.}-{0:0}, at: process_one_work+0x294/0x636
#1: ffff8881013a7e20 ((work_completion)(&rxnet->peer_keepalive_work)){+.+.}-{0:0}, at: process_one_work+0x294/0x636
#2: ffff88817d366390 (&rxnet->peer_hash_lock){+.+.}-{2:2}, at: rxrpc_peer_keepalive_dispatch+0x2bd/0x35f
...
Call Trace:
<TASK>
dump_stack_lvl+0x4c/0x5f
__might_resched+0x2cf/0x2f2
__wait_for_common+0x87/0x1e8
kthread_stop+0x14d/0x255
rxrpc_peer_keepalive_dispatch+0x333/0x35f
rxrpc_peer_keepalive_worker+0x2e9/0x449
process_one_work+0x3c1/0x636
worker_thread+0x25f/0x359
kthread+0x1a6/0x1b5
ret_from_fork+0x1f/0x30
Fixes: a275da62e8c1 ("rxrpc: Create a per-local endpoint receive queue and I/O thread")
Signed-off-by: David Howells <[email protected]>
cc: Marc Dionne <[email protected]>
cc: [email protected]
Link: https://lore.kernel.org/r/[email protected]/ [1]
---
net/rxrpc/ar-internal.h | 1 -
net/rxrpc/peer_event.c | 10 +++++++---
net/rxrpc/peer_object.c | 19 -------------------
3 files changed, 7 insertions(+), 23 deletions(-)
diff --git a/net/rxrpc/ar-internal.h b/net/rxrpc/ar-internal.h
index 37f3aec784cc..5b732a4af009 100644
--- a/net/rxrpc/ar-internal.h
+++ b/net/rxrpc/ar-internal.h
@@ -1073,7 +1073,6 @@ void rxrpc_destroy_all_peers(struct rxrpc_net *);
struct rxrpc_peer *rxrpc_get_peer(struct rxrpc_peer *, enum rxrpc_peer_trace);
struct rxrpc_peer *rxrpc_get_peer_maybe(struct rxrpc_peer *, enum rxrpc_peer_trace);
void rxrpc_put_peer(struct rxrpc_peer *, enum rxrpc_peer_trace);
-void rxrpc_put_peer_locked(struct rxrpc_peer *, enum rxrpc_peer_trace);
/*
* proc.c
diff --git a/net/rxrpc/peer_event.c b/net/rxrpc/peer_event.c
index 6685bf917aa6..552ba84a255c 100644
--- a/net/rxrpc/peer_event.c
+++ b/net/rxrpc/peer_event.c
@@ -235,6 +235,7 @@ static void rxrpc_peer_keepalive_dispatch(struct rxrpc_net *rxnet,
struct rxrpc_peer *peer;
const u8 mask = ARRAY_SIZE(rxnet->peer_keepalive) - 1;
time64_t keepalive_at;
+ bool use;
int slot;
spin_lock(&rxnet->peer_hash_lock);
@@ -247,9 +248,10 @@ static void rxrpc_peer_keepalive_dispatch(struct rxrpc_net *rxnet,
if (!rxrpc_get_peer_maybe(peer, rxrpc_peer_get_keepalive))
continue;
- if (__rxrpc_use_local(peer->local, rxrpc_local_use_peer_keepalive)) {
- spin_unlock(&rxnet->peer_hash_lock);
+ use = __rxrpc_use_local(peer->local, rxrpc_local_use_peer_keepalive);
+ spin_unlock(&rxnet->peer_hash_lock);
+ if (use) {
keepalive_at = peer->last_tx_at + RXRPC_KEEPALIVE_TIME;
slot = keepalive_at - base;
_debug("%02x peer %u t=%d {%pISp}",
@@ -270,9 +272,11 @@ static void rxrpc_peer_keepalive_dispatch(struct rxrpc_net *rxnet,
spin_lock(&rxnet->peer_hash_lock);
list_add_tail(&peer->keepalive_link,
&rxnet->peer_keepalive[slot & mask]);
+ spin_unlock(&rxnet->peer_hash_lock);
rxrpc_unuse_local(peer->local, rxrpc_local_unuse_peer_keepalive);
}
- rxrpc_put_peer_locked(peer, rxrpc_peer_put_keepalive);
+ rxrpc_put_peer(peer, rxrpc_peer_put_keepalive);
+ spin_lock(&rxnet->peer_hash_lock);
}
spin_unlock(&rxnet->peer_hash_lock);
diff --git a/net/rxrpc/peer_object.c b/net/rxrpc/peer_object.c
index 608946dcc505..82de295393a0 100644
--- a/net/rxrpc/peer_object.c
+++ b/net/rxrpc/peer_object.c
@@ -438,25 +438,6 @@ void rxrpc_put_peer(struct rxrpc_peer *peer, enum rxrpc_peer_trace why)
}
}
-/*
- * Drop a ref on a peer record where the caller already holds the
- * peer_hash_lock.
- */
-void rxrpc_put_peer_locked(struct rxrpc_peer *peer, enum rxrpc_peer_trace why)
-{
- unsigned int debug_id = peer->debug_id;
- bool dead;
- int r;
-
- dead = __refcount_dec_and_test(&peer->ref, &r);
- trace_rxrpc_peer(debug_id, r - 1, why);
- if (dead) {
- hash_del_rcu(&peer->hash_link);
- list_del_init(&peer->keepalive_link);
- rxrpc_free_peer(peer);
- }
-}
-
/*
* Make sure all peer records have been discarded.
*/
Fix the switched parameters on rxrpc_alloc_peer() and rxrpc_get_peer().
The ref argument and the why argument got mixed.
Fixes: 47c810a79844 ("rxrpc: trace: Don't use __builtin_return_address for rxrpc_peer tracing")
Signed-off-by: David Howells <[email protected]>
cc: Marc Dionne <[email protected]>
cc: [email protected]
---
include/trace/events/rxrpc.h | 2 +-
net/rxrpc/peer_object.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/include/trace/events/rxrpc.h b/include/trace/events/rxrpc.h
index 049b52e7aa6a..c6cfed00d0c6 100644
--- a/include/trace/events/rxrpc.h
+++ b/include/trace/events/rxrpc.h
@@ -471,7 +471,7 @@ TRACE_EVENT(rxrpc_peer,
TP_STRUCT__entry(
__field(unsigned int, peer )
__field(int, ref )
- __field(int, why )
+ __field(enum rxrpc_peer_trace, why )
),
TP_fast_assign(
diff --git a/net/rxrpc/peer_object.c b/net/rxrpc/peer_object.c
index 82de295393a0..4eecea2be307 100644
--- a/net/rxrpc/peer_object.c
+++ b/net/rxrpc/peer_object.c
@@ -226,7 +226,7 @@ struct rxrpc_peer *rxrpc_alloc_peer(struct rxrpc_local *local, gfp_t gfp,
rxrpc_peer_init_rtt(peer);
peer->cong_ssthresh = RXRPC_TX_MAX_WINDOW;
- trace_rxrpc_peer(peer->debug_id, why, 1);
+ trace_rxrpc_peer(peer->debug_id, 1, why);
}
_leave(" = %p", peer);
@@ -382,7 +382,7 @@ struct rxrpc_peer *rxrpc_get_peer(struct rxrpc_peer *peer, enum rxrpc_peer_trace
int r;
__refcount_inc(&peer->ref, &r);
- trace_rxrpc_peer(peer->debug_id, why, r + 1);
+ trace_rxrpc_peer(peer->debug_id, r + 1, why);
return peer;
}
When starting a kthread, the __kthread_create_on_node() function, as called
from kthread_run(), waits for a completion to indicate that the task_struct
(or failure state) of the new kernel thread is available before continuing.
This does not wait, however, for the thread function to be invoked and,
indeed, will skip it if kthread_stop() gets called before it gets there.
If this happens, though, kthread_run() will have returned successfully,
indicating that the thread was started and returning the task_struct
pointer. The actual error indication is returned by kthread_stop().
Note that this is ambiguous, as the caller cannot tell whether the -EINTR
error code came from kthread() or from the thread function.
This was encountered in the new rxrpc I/O thread, where if the system is
being pounded hard by, say, syzbot, the check of KTHREAD_SHOULD_STOP can be
delayed long enough for kthread_stop() to get called when rxrpc releases a
socket - and this causes an oops because the I/O thread function doesn't
get started and thus doesn't remove the rxrpc_local struct from the
local_endpoints list.
Fix this by using a completion to wait for the thread to actually enter
rxrpc_io_thread(). This makes sure the thread can't be prematurely
stopped and makes sure the relied-upon cleanup is done.
Fixes: a275da62e8c1 ("rxrpc: Create a per-local endpoint receive queue and I/O thread")
Reported-by: [email protected]
Signed-off-by: David Howells <[email protected]>
cc: Marc Dionne <[email protected]>
cc: Hillf Danton <[email protected]>
Link: https://lore.kernel.org/r/[email protected]/
---
net/rxrpc/ar-internal.h | 1 +
net/rxrpc/io_thread.c | 2 ++
net/rxrpc/local_object.c | 2 ++
3 files changed, 5 insertions(+)
diff --git a/net/rxrpc/ar-internal.h b/net/rxrpc/ar-internal.h
index e7dccab7b741..37f3aec784cc 100644
--- a/net/rxrpc/ar-internal.h
+++ b/net/rxrpc/ar-internal.h
@@ -287,6 +287,7 @@ struct rxrpc_local {
struct hlist_node link;
struct socket *socket; /* my UDP socket */
struct task_struct *io_thread;
+ struct completion io_thread_ready; /* Indication that the I/O thread started */
struct rxrpc_sock __rcu *service; /* Service(s) listening on this endpoint */
struct rw_semaphore defrag_sem; /* control re-enablement of IP DF bit */
struct sk_buff_head rx_queue; /* Received packets */
diff --git a/net/rxrpc/io_thread.c b/net/rxrpc/io_thread.c
index d83ae3193032..e460e4151c16 100644
--- a/net/rxrpc/io_thread.c
+++ b/net/rxrpc/io_thread.c
@@ -426,6 +426,8 @@ int rxrpc_io_thread(void *data)
struct rxrpc_call *call;
struct sk_buff *skb;
+ complete(&local->io_thread_ready);
+
skb_queue_head_init(&rx_queue);
set_user_nice(current, MIN_NICE);
diff --git a/net/rxrpc/local_object.c b/net/rxrpc/local_object.c
index 24ee585d9aaf..270b63d8f37a 100644
--- a/net/rxrpc/local_object.c
+++ b/net/rxrpc/local_object.c
@@ -97,6 +97,7 @@ static struct rxrpc_local *rxrpc_alloc_local(struct rxrpc_net *rxnet,
local->rxnet = rxnet;
INIT_HLIST_NODE(&local->link);
init_rwsem(&local->defrag_sem);
+ init_completion(&local->io_thread_ready);
skb_queue_head_init(&local->rx_queue);
INIT_LIST_HEAD(&local->call_attend_q);
local->client_bundles = RB_ROOT;
@@ -189,6 +190,7 @@ static int rxrpc_open_socket(struct rxrpc_local *local, struct net *net)
goto error_sock;
}
+ wait_for_completion(&local->io_thread_ready);
local->io_thread = io_thread;
_leave(" = 0");
return 0;
On Thu, Dec 15, 2022 at 12:20 PM David Howells <[email protected]> wrote:
>
>
> 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.
>
> The patches are tagged here:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git
> rxrpc-fixes-20221215
>
> and can also be found on the following branch:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=rxrpc-fixes
>
> David
>
> Link: https://syzkaller.appspot.com/bug?extid=3538a6a72efa8b059c38 [1]
> Link: https://syzkaller.appspot.com/bug?extid=2a99eae8dc7c754bc16b [2]
> Link: https://syzkaller.appspot.com/bug?extid=e1391a5bf3f779e31237 [3]
> Link: https://syzkaller.appspot.com/bug?extid=2aea8e1c8e20cb27a01f [4]
> Link: https://syzkaller.appspot.com/bug?extid=1eb4232fca28c0a6d1c2 [5]
> Link: https://syzkaller.appspot.com/bug?extid=c22650d2844392afdcfd [6]
>
> ---
> David Howells (9):
> rxrpc: Fix missing unlock in rxrpc_do_sendmsg()
> rxrpc: Fix security setting propagation
> rxrpc: Fix NULL deref in rxrpc_unuse_local()
> rxrpc: Fix I/O thread startup getting skipped
> rxrpc: Fix locking issues in rxrpc_put_peer_locked()
> rxrpc: Fix switched parameters in peer tracing
> rxrpc: Fix I/O thread stop
> rxrpc: rxperf: Fix uninitialised variable
> rxrpc: Fix the return value of rxrpc_new_incoming_call()
>
>
> include/trace/events/rxrpc.h | 2 +-
> net/rxrpc/ar-internal.h | 8 ++++----
> net/rxrpc/call_accept.c | 18 +++++++++---------
> net/rxrpc/call_object.c | 1 +
> net/rxrpc/conn_client.c | 2 --
> net/rxrpc/io_thread.c | 10 +++++++---
> net/rxrpc/local_object.c | 5 ++++-
> net/rxrpc/peer_event.c | 10 +++++++---
> net/rxrpc/peer_object.c | 23 ++---------------------
> net/rxrpc/rxperf.c | 2 +-
> net/rxrpc/security.c | 6 +++---
> net/rxrpc/sendmsg.c | 2 +-
> 12 files changed, 40 insertions(+), 49 deletions(-)
For the series:
Tested-by: Marc Dionne <[email protected]>
Tested-by: [email protected]
Marc
Hillf Danton <[email protected]> wrote:
> > @@ -478,13 +479,14 @@ int rxrpc_io_thread(void *data)
> > }
> >
> > set_current_state(TASK_INTERRUPTIBLE);
> > + should_stop = kthread_should_stop();
> > if (!skb_queue_empty(&local->rx_queue) ||
> > !list_empty(&local->call_attend_q)) {
> > __set_current_state(TASK_RUNNING);
> > continue;
> > }
> >
> > - if (kthread_should_stop())
> > + if (should_stop)
> > break;
> > schedule();
> > }
>
> At the second glance still fail to see the difference this change can
> make.
There is a window here:
if (!skb_queue_empty(&local->rx_queue) ...)
continue;
--->
if (kthread_should_stop())
break;
in which an event can happen that should be attended to. For instance the
AF_RXRPC socket being closed, aborting all its calls and stopping the kthread
by doing the final unuse on its rxrpc_local struct - in that order. The
window can be expanded by an interrupt or softirq handler running.
So once we've observed that we've been asked to stop, we need to check if
there's more work to be done and, if so, do that work first.
So by doing:
should_stop = kthread_should_stop();
if (!skb_queue_empty(&local->rx_queue) ...)
continue;
if (should_stop)
break;
we do all outstanding cleanup work before exiting the loop to stop the thread.
David
Hillf Danton <[email protected]> wrote:
> In line with
>
> if (condition)
> return;
> add to wait queue
> if (!condition)
> schedule();
>
> this change should look like
>
> if (!skb_queue_empty(&local->rx_queue) ...)
> continue;
>
> if (kthread_should_stop())
> if (!skb_queue_empty(&local->rx_queue) ...)
> continue;
> else
> break;
>
> as checking condition once barely makes sense.
Really, no. The condition is going to expand to have a whole bunch of things
in it and I don't want to have it twice, e.g.:
if (!skb_queue_empty(&local->rx_queue) ||
READ_ONCE(local->events) ||
!list_empty(&local->call_attend_q) ||
!list_empty(&local->conn_attend_q) ||
!list_empty(&local->new_client_calls) ||
test_bit(RXRPC_CLIENT_CONN_REAP_TIMER,
&local->client_conn_flags)) {
...
Hmmm... I wonder if kthread_should_stop() needs a barrier associated with
it. It's just a test_bit(), so the compiler can cache the results of all
these tests - or reorder them.
David
Hillf Danton <[email protected]> wrote:
> > So once we've observed that we've been asked to stop, we need to check if
> > there's more work to be done and, if so, do that work first.
>
> In line with
>
> if (condition)
> return;
> add to wait queue
> if (!condition)
> schedule();
>
> this change should look like
>
> if (!skb_queue_empty(&local->rx_queue) ...)
> continue;
>
> if (kthread_should_stop())
> if (!skb_queue_empty(&local->rx_queue) ...)
> continue;
> else
> break;
>
> as checking condition once barely makes sense.
Note that these are not really analogous. The add-to-wait-queue step is
significantly more expensive than kthread_should_stop() and requires removal
in the event that the condition becomes true in the window.
In the case of kthread_should_stop(), it's just a test_bit() of a word that's
in a cacheline not going to get changed until the thread is stopped. Testing
the value first and then checking the condition should be fine as the stop
flag can be shared in the cpu's data cache until it's set.
Also from a code-maintenance PoV, I don't want to write the condition twice if
I can avoid it. That allows for the two copies to get out of sync.
> Because of a bit complex condition does not mean checking it once is neither
> sane nor correct.
So you agree with me, I think?
David
Hello:
This series was applied to netdev/net.git (master)
by David S. Miller <[email protected]>:
On Thu, 15 Dec 2022 16:19:38 +0000 you wrote:
> 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().
>
> [...]
Here is the summary with links:
- [net,1/9] rxrpc: Fix missing unlock in rxrpc_do_sendmsg()
https://git.kernel.org/netdev/net/c/4feb2c44629e
- [net,2/9] rxrpc: Fix security setting propagation
https://git.kernel.org/netdev/net/c/fdb99487b018
- [net,3/9] rxrpc: Fix NULL deref in rxrpc_unuse_local()
https://git.kernel.org/netdev/net/c/eaa02390adb0
- [net,4/9] rxrpc: Fix I/O thread startup getting skipped
https://git.kernel.org/netdev/net/c/8fbcc83334a7
- [net,5/9] rxrpc: Fix locking issues in rxrpc_put_peer_locked()
https://git.kernel.org/netdev/net/c/608aecd16a31
- [net,6/9] rxrpc: Fix switched parameters in peer tracing
https://git.kernel.org/netdev/net/c/c838f1a73d77
- [net,7/9] rxrpc: Fix I/O thread stop
https://git.kernel.org/netdev/net/c/743d1768a008
- [net,8/9] rxrpc: rxperf: Fix uninitialised variable
https://git.kernel.org/netdev/net/c/11e1706bc84f
- [net,9/9] rxrpc: Fix the return value of rxrpc_new_incoming_call()
https://git.kernel.org/netdev/net/c/31d35a02ad5b
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html