* [PATCH net] rxrpc: Call state should be read with READ_ONCE() under some circumstances
@ 2021-01-12 15:59 David Howells
2021-01-13 2:25 ` Jakub Kicinski
2021-01-13 8:23 ` David Howells
0 siblings, 2 replies; 6+ messages in thread
From: David Howells @ 2021-01-12 15:59 UTC (permalink / raw)
To: netdev; +Cc: Baptiste Lepers, linux-afs, linux-kernel, dhowells
From: Baptiste Lepers <baptiste.lepers@gmail.com>
The call state may be changed at any time by the data-ready routine in
response to received packets, so if the call state is to be read and acted
upon several times in a function, READ_ONCE() must be used unless the call
state lock is held.
As it happens, we used READ_ONCE() to read the state a few lines above the
unmarked read in rxrpc_input_data(), so use that value rather than
re-reading it.
Signed-off-by: Baptiste Lepers <baptiste.lepers@gmail.com>
Signed-off-by: David Howells <dhowells@redhat.com>
---
net/rxrpc/input.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/rxrpc/input.c b/net/rxrpc/input.c
index 667c44aa5a63..dc201363f2c4 100644
--- a/net/rxrpc/input.c
+++ b/net/rxrpc/input.c
@@ -430,7 +430,7 @@ static void rxrpc_input_data(struct rxrpc_call *call, struct sk_buff *skb)
return;
}
- if (call->state == RXRPC_CALL_SERVER_RECV_REQUEST) {
+ if (state == RXRPC_CALL_SERVER_RECV_REQUEST) {
unsigned long timo = READ_ONCE(call->next_req_timo);
unsigned long now, expect_req_by;
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net] rxrpc: Call state should be read with READ_ONCE() under some circumstances
2021-01-12 15:59 [PATCH net] rxrpc: Call state should be read with READ_ONCE() under some circumstances David Howells
@ 2021-01-13 2:25 ` Jakub Kicinski
2021-01-13 8:23 ` David Howells
1 sibling, 0 replies; 6+ messages in thread
From: Jakub Kicinski @ 2021-01-13 2:25 UTC (permalink / raw)
To: David Howells; +Cc: netdev, Baptiste Lepers, linux-afs, linux-kernel
On Tue, 12 Jan 2021 15:59:15 +0000 David Howells wrote:
> From: Baptiste Lepers <baptiste.lepers@gmail.com>
>
> The call state may be changed at any time by the data-ready routine in
> response to received packets, so if the call state is to be read and acted
> upon several times in a function, READ_ONCE() must be used unless the call
> state lock is held.
>
> As it happens, we used READ_ONCE() to read the state a few lines above the
> unmarked read in rxrpc_input_data(), so use that value rather than
> re-reading it.
>
> Signed-off-by: Baptiste Lepers <baptiste.lepers@gmail.com>
> Signed-off-by: David Howells <dhowells@redhat.com>
Fixes: a158bdd3247b ("rxrpc: Fix call timeouts")
maybe?
> diff --git a/net/rxrpc/input.c b/net/rxrpc/input.c
> index 667c44aa5a63..dc201363f2c4 100644
> --- a/net/rxrpc/input.c
> +++ b/net/rxrpc/input.c
> @@ -430,7 +430,7 @@ static void rxrpc_input_data(struct rxrpc_call *call, struct sk_buff *skb)
> return;
> }
>
> - if (call->state == RXRPC_CALL_SERVER_RECV_REQUEST) {
> + if (state == RXRPC_CALL_SERVER_RECV_REQUEST) {
> unsigned long timo = READ_ONCE(call->next_req_timo);
> unsigned long now, expect_req_by;
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] rxrpc: Call state should be read with READ_ONCE() under some circumstances
2021-01-12 15:59 [PATCH net] rxrpc: Call state should be read with READ_ONCE() under some circumstances David Howells
2021-01-13 2:25 ` Jakub Kicinski
@ 2021-01-13 8:23 ` David Howells
2021-01-13 18:41 ` Jakub Kicinski
1 sibling, 1 reply; 6+ messages in thread
From: David Howells @ 2021-01-13 8:23 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: dhowells, netdev, Baptiste Lepers, linux-afs, linux-kernel
Jakub Kicinski <kuba@kernel.org> wrote:
> On Tue, 12 Jan 2021 15:59:15 +0000 David Howells wrote:
> > From: Baptiste Lepers <baptiste.lepers@gmail.com>
> >
> > The call state may be changed at any time by the data-ready routine in
> > response to received packets, so if the call state is to be read and acted
> > upon several times in a function, READ_ONCE() must be used unless the call
> > state lock is held.
> >
> > As it happens, we used READ_ONCE() to read the state a few lines above the
> > unmarked read in rxrpc_input_data(), so use that value rather than
> > re-reading it.
> >
> > Signed-off-by: Baptiste Lepers <baptiste.lepers@gmail.com>
> > Signed-off-by: David Howells <dhowells@redhat.com>
>
> Fixes: a158bdd3247b ("rxrpc: Fix call timeouts")
>
> maybe?
Ah, yes. I missed there wasn't a Fixes line. Can you add that one in, or do
I need to resubmit the patch?
David
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] rxrpc: Call state should be read with READ_ONCE() under some circumstances
2021-01-13 8:23 ` David Howells
@ 2021-01-13 18:41 ` Jakub Kicinski
0 siblings, 0 replies; 6+ messages in thread
From: Jakub Kicinski @ 2021-01-13 18:41 UTC (permalink / raw)
To: David Howells; +Cc: netdev, Baptiste Lepers, linux-afs, linux-kernel
On Wed, 13 Jan 2021 08:23:54 +0000 David Howells wrote:
> Jakub Kicinski <kuba@kernel.org> wrote:
>
> > On Tue, 12 Jan 2021 15:59:15 +0000 David Howells wrote:
> > > From: Baptiste Lepers <baptiste.lepers@gmail.com>
> > >
> > > The call state may be changed at any time by the data-ready routine in
> > > response to received packets, so if the call state is to be read and acted
> > > upon several times in a function, READ_ONCE() must be used unless the call
> > > state lock is held.
> > >
> > > As it happens, we used READ_ONCE() to read the state a few lines above the
> > > unmarked read in rxrpc_input_data(), so use that value rather than
> > > re-reading it.
> > >
> > > Signed-off-by: Baptiste Lepers <baptiste.lepers@gmail.com>
> > > Signed-off-by: David Howells <dhowells@redhat.com>
> >
> > Fixes: a158bdd3247b ("rxrpc: Fix call timeouts")
> >
> > maybe?
>
> Ah, yes. I missed there wasn't a Fixes line. Can you add that one in, or do
> I need to resubmit the patch?
Sure, added, just checking if you didn't leave it out on purpose.
Applied, thanks!
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] rxrpc: Call state should be read with READ_ONCE() under some circumstances
2017-03-04 0:01 David Howells
@ 2017-03-07 21:59 ` David Miller
0 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2017-03-07 21:59 UTC (permalink / raw)
To: dhowells; +Cc: netdev, linux-afs, linux-kernel
From: David Howells <dhowells@redhat.com>
Date: Sat, 04 Mar 2017 00:01:41 +0000
> The call state may be changed at any time by the data-ready routine in
> response to received packets, so if the call state is to be read and acted
> upon several times in a function, READ_ONCE() must be used unless the call
> state lock is held.
>
> Signed-off-by: David Howells <dhowells@redhat.com>
Applied.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH net] rxrpc: Call state should be read with READ_ONCE() under some circumstances
@ 2017-03-04 0:01 David Howells
2017-03-07 21:59 ` David Miller
0 siblings, 1 reply; 6+ messages in thread
From: David Howells @ 2017-03-04 0:01 UTC (permalink / raw)
To: netdev; +Cc: dhowells, linux-afs, linux-kernel
The call state may be changed at any time by the data-ready routine in
response to received packets, so if the call state is to be read and acted
upon several times in a function, READ_ONCE() must be used unless the call
state lock is held.
Signed-off-by: David Howells <dhowells@redhat.com>
---
net/rxrpc/input.c | 12 +++++++-----
net/rxrpc/recvmsg.c | 4 ++--
net/rxrpc/sendmsg.c | 48 ++++++++++++++++++++++++++++++------------------
3 files changed, 39 insertions(+), 25 deletions(-)
diff --git a/net/rxrpc/input.c b/net/rxrpc/input.c
index 9f4cfa25af7c..d74921c4969b 100644
--- a/net/rxrpc/input.c
+++ b/net/rxrpc/input.c
@@ -420,6 +420,7 @@ static void rxrpc_input_data(struct rxrpc_call *call, struct sk_buff *skb,
u16 skew)
{
struct rxrpc_skb_priv *sp = rxrpc_skb(skb);
+ enum rxrpc_call_state state;
unsigned int offset = sizeof(struct rxrpc_wire_header);
unsigned int ix;
rxrpc_serial_t serial = sp->hdr.serial, ack_serial = 0;
@@ -434,14 +435,15 @@ static void rxrpc_input_data(struct rxrpc_call *call, struct sk_buff *skb,
_proto("Rx DATA %%%u { #%u f=%02x }",
sp->hdr.serial, seq, sp->hdr.flags);
- if (call->state >= RXRPC_CALL_COMPLETE)
+ state = READ_ONCE(call->state);
+ if (state >= RXRPC_CALL_COMPLETE)
return;
/* Received data implicitly ACKs all of the request packets we sent
* when we're acting as a client.
*/
- if ((call->state == RXRPC_CALL_CLIENT_SEND_REQUEST ||
- call->state == RXRPC_CALL_CLIENT_AWAIT_REPLY) &&
+ if ((state == RXRPC_CALL_CLIENT_SEND_REQUEST ||
+ state == RXRPC_CALL_CLIENT_AWAIT_REPLY) &&
!rxrpc_receiving_reply(call))
return;
@@ -799,7 +801,7 @@ static void rxrpc_input_ack(struct rxrpc_call *call, struct sk_buff *skb,
return rxrpc_proto_abort("AK0", call, 0);
/* Ignore ACKs unless we are or have just been transmitting. */
- switch (call->state) {
+ switch (READ_ONCE(call->state)) {
case RXRPC_CALL_CLIENT_SEND_REQUEST:
case RXRPC_CALL_CLIENT_AWAIT_REPLY:
case RXRPC_CALL_SERVER_SEND_REPLY:
@@ -940,7 +942,7 @@ static void rxrpc_input_call_packet(struct rxrpc_call *call,
static void rxrpc_input_implicit_end_call(struct rxrpc_connection *conn,
struct rxrpc_call *call)
{
- switch (call->state) {
+ switch (READ_ONCE(call->state)) {
case RXRPC_CALL_SERVER_AWAIT_ACK:
rxrpc_call_completed(call);
break;
diff --git a/net/rxrpc/recvmsg.c b/net/rxrpc/recvmsg.c
index 22447dbcc380..46b1a93be03a 100644
--- a/net/rxrpc/recvmsg.c
+++ b/net/rxrpc/recvmsg.c
@@ -525,7 +525,7 @@ int rxrpc_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
msg->msg_namelen = len;
}
- switch (call->state) {
+ switch (READ_ONCE(call->state)) {
case RXRPC_CALL_SERVER_ACCEPTING:
ret = rxrpc_recvmsg_new_call(rx, call, msg, flags);
break;
@@ -638,7 +638,7 @@ int rxrpc_kernel_recv_data(struct socket *sock, struct rxrpc_call *call,
mutex_lock(&call->user_mutex);
- switch (call->state) {
+ switch (READ_ONCE(call->state)) {
case RXRPC_CALL_CLIENT_RECV_REPLY:
case RXRPC_CALL_SERVER_RECV_REQUEST:
case RXRPC_CALL_SERVER_ACK_REQUEST:
diff --git a/net/rxrpc/sendmsg.c b/net/rxrpc/sendmsg.c
index 27685d8cba1a..9c2e443de811 100644
--- a/net/rxrpc/sendmsg.c
+++ b/net/rxrpc/sendmsg.c
@@ -486,6 +486,7 @@ rxrpc_new_client_call_for_sendmsg(struct rxrpc_sock *rx, struct msghdr *msg,
int rxrpc_do_sendmsg(struct rxrpc_sock *rx, struct msghdr *msg, size_t len)
__releases(&rx->sk.sk_lock.slock)
{
+ enum rxrpc_call_state state;
enum rxrpc_command cmd;
struct rxrpc_call *call;
unsigned long user_call_ID = 0;
@@ -524,13 +525,17 @@ int rxrpc_do_sendmsg(struct rxrpc_sock *rx, struct msghdr *msg, size_t len)
return PTR_ERR(call);
/* ... and we have the call lock. */
} else {
- ret = -EBUSY;
- if (call->state == RXRPC_CALL_UNINITIALISED ||
- call->state == RXRPC_CALL_CLIENT_AWAIT_CONN ||
- call->state == RXRPC_CALL_SERVER_PREALLOC ||
- call->state == RXRPC_CALL_SERVER_SECURING ||
- call->state == RXRPC_CALL_SERVER_ACCEPTING)
+ switch (READ_ONCE(call->state)) {
+ case RXRPC_CALL_UNINITIALISED:
+ case RXRPC_CALL_CLIENT_AWAIT_CONN:
+ case RXRPC_CALL_SERVER_PREALLOC:
+ case RXRPC_CALL_SERVER_SECURING:
+ case RXRPC_CALL_SERVER_ACCEPTING:
+ ret = -EBUSY;
goto error_release_sock;
+ default:
+ break;
+ }
ret = mutex_lock_interruptible(&call->user_mutex);
release_sock(&rx->sk);
@@ -540,10 +545,11 @@ int rxrpc_do_sendmsg(struct rxrpc_sock *rx, struct msghdr *msg, size_t len)
}
}
+ state = READ_ONCE(call->state);
_debug("CALL %d USR %lx ST %d on CONN %p",
- call->debug_id, call->user_call_ID, call->state, call->conn);
+ call->debug_id, call->user_call_ID, state, call->conn);
- if (call->state >= RXRPC_CALL_COMPLETE) {
+ if (state >= RXRPC_CALL_COMPLETE) {
/* it's too late for this call */
ret = -ESHUTDOWN;
} else if (cmd == RXRPC_CMD_SEND_ABORT) {
@@ -553,12 +559,12 @@ int rxrpc_do_sendmsg(struct rxrpc_sock *rx, struct msghdr *msg, size_t len)
} else if (cmd != RXRPC_CMD_SEND_DATA) {
ret = -EINVAL;
} else if (rxrpc_is_client_call(call) &&
- call->state != RXRPC_CALL_CLIENT_SEND_REQUEST) {
+ state != RXRPC_CALL_CLIENT_SEND_REQUEST) {
/* request phase complete for this client call */
ret = -EPROTO;
} else if (rxrpc_is_service_call(call) &&
- call->state != RXRPC_CALL_SERVER_ACK_REQUEST &&
- call->state != RXRPC_CALL_SERVER_SEND_REPLY) {
+ state != RXRPC_CALL_SERVER_ACK_REQUEST &&
+ state != RXRPC_CALL_SERVER_SEND_REPLY) {
/* Reply phase not begun or not complete for service call. */
ret = -EPROTO;
} else {
@@ -603,14 +609,20 @@ int rxrpc_kernel_send_data(struct socket *sock, struct rxrpc_call *call,
_debug("CALL %d USR %lx ST %d on CONN %p",
call->debug_id, call->user_call_ID, call->state, call->conn);
- if (call->state >= RXRPC_CALL_COMPLETE) {
- ret = -ESHUTDOWN; /* it's too late for this call */
- } else if (call->state != RXRPC_CALL_CLIENT_SEND_REQUEST &&
- call->state != RXRPC_CALL_SERVER_ACK_REQUEST &&
- call->state != RXRPC_CALL_SERVER_SEND_REPLY) {
- ret = -EPROTO; /* request phase complete for this client call */
- } else {
+ switch (READ_ONCE(call->state)) {
+ case RXRPC_CALL_CLIENT_SEND_REQUEST:
+ case RXRPC_CALL_SERVER_ACK_REQUEST:
+ case RXRPC_CALL_SERVER_SEND_REPLY:
ret = rxrpc_send_data(rxrpc_sk(sock->sk), call, msg, len);
+ break;
+ case RXRPC_CALL_COMPLETE:
+ /* It's too late for this call */
+ ret = -ESHUTDOWN;
+ break;
+ default:
+ /* Request phase complete for this client call */
+ ret = -EPROTO;
+ break;
}
mutex_unlock(&call->user_mutex);
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-01-13 18:42 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-12 15:59 [PATCH net] rxrpc: Call state should be read with READ_ONCE() under some circumstances David Howells
2021-01-13 2:25 ` Jakub Kicinski
2021-01-13 8:23 ` David Howells
2021-01-13 18:41 ` Jakub Kicinski
-- strict thread matches above, loose matches on Subject: below --
2017-03-04 0:01 David Howells
2017-03-07 21:59 ` David Miller
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).