* [PATCH] skbuff: fix a data race in skb_queue_len()
@ 2020-02-03 18:56 Qian Cai
2020-02-03 19:41 ` Eric Dumazet
0 siblings, 1 reply; 5+ messages in thread
From: Qian Cai @ 2020-02-03 18:56 UTC (permalink / raw)
To: davem; +Cc: kuba, elver, netdev, linux-kernel, Qian Cai
sk_buff.qlen can be accessed concurrently as noticed by KCSAN,
BUG: KCSAN: data-race in __skb_try_recv_from_queue / unix_dgram_sendmsg
read to 0xffff8a1b1d8a81c0 of 4 bytes by task 5371 on cpu 96:
unix_dgram_sendmsg+0x9a9/0xb70 include/linux/skbuff.h:1821
net/unix/af_unix.c:1761
____sys_sendmsg+0x33e/0x370
___sys_sendmsg+0xa6/0xf0
__sys_sendmsg+0x69/0xf0
__x64_sys_sendmsg+0x51/0x70
do_syscall_64+0x91/0xb47
entry_SYSCALL_64_after_hwframe+0x49/0xbe
write to 0xffff8a1b1d8a81c0 of 4 bytes by task 1 on cpu 99:
__skb_try_recv_from_queue+0x327/0x410 include/linux/skbuff.h:2029
__skb_try_recv_datagram+0xbe/0x220
unix_dgram_recvmsg+0xee/0x850
____sys_recvmsg+0x1fb/0x210
___sys_recvmsg+0xa2/0xf0
__sys_recvmsg+0x66/0xf0
__x64_sys_recvmsg+0x51/0x70
do_syscall_64+0x91/0xb47
entry_SYSCALL_64_after_hwframe+0x49/0xbe
Since only the read is operating as lockless, it could introduce a logic
bug in unix_recvq_full() due to the load tearing. Fix it by adding
a READ_ONCE() there.
Signed-off-by: Qian Cai <cai@lca.pw>
---
include/linux/skbuff.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 3d13a4b717e9..4b5157164f3e 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1818,7 +1818,7 @@ static inline struct sk_buff *skb_peek_tail(const struct sk_buff_head *list_)
*/
static inline __u32 skb_queue_len(const struct sk_buff_head *list_)
{
- return list_->qlen;
+ return READ_ONCE(list_->qlen);
}
/**
--
1.8.3.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] skbuff: fix a data race in skb_queue_len()
2020-02-03 18:56 [PATCH] skbuff: fix a data race in skb_queue_len() Qian Cai
@ 2020-02-03 19:41 ` Eric Dumazet
2020-02-03 20:19 ` Qian Cai
0 siblings, 1 reply; 5+ messages in thread
From: Eric Dumazet @ 2020-02-03 19:41 UTC (permalink / raw)
To: Qian Cai, davem; +Cc: kuba, elver, netdev, linux-kernel
On 2/3/20 10:56 AM, Qian Cai wrote:
> sk_buff.qlen can be accessed concurrently as noticed by KCSAN,
>
> BUG: KCSAN: data-race in __skb_try_recv_from_queue / unix_dgram_sendmsg
>
> read to 0xffff8a1b1d8a81c0 of 4 bytes by task 5371 on cpu 96:
> unix_dgram_sendmsg+0x9a9/0xb70 include/linux/skbuff.h:1821
> net/unix/af_unix.c:1761
> ____sys_sendmsg+0x33e/0x370
> ___sys_sendmsg+0xa6/0xf0
> __sys_sendmsg+0x69/0xf0
> __x64_sys_sendmsg+0x51/0x70
> do_syscall_64+0x91/0xb47
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> write to 0xffff8a1b1d8a81c0 of 4 bytes by task 1 on cpu 99:
> __skb_try_recv_from_queue+0x327/0x410 include/linux/skbuff.h:2029
> __skb_try_recv_datagram+0xbe/0x220
> unix_dgram_recvmsg+0xee/0x850
> ____sys_recvmsg+0x1fb/0x210
> ___sys_recvmsg+0xa2/0xf0
> __sys_recvmsg+0x66/0xf0
> __x64_sys_recvmsg+0x51/0x70
> do_syscall_64+0x91/0xb47
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> Since only the read is operating as lockless, it could introduce a logic
> bug in unix_recvq_full() due to the load tearing. Fix it by adding
> a READ_ONCE() there.
>
> Signed-off-by: Qian Cai <cai@lca.pw>
> ---
> include/linux/skbuff.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 3d13a4b717e9..4b5157164f3e 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -1818,7 +1818,7 @@ static inline struct sk_buff *skb_peek_tail(const struct sk_buff_head *list_)
> */
> static inline __u32 skb_queue_len(const struct sk_buff_head *list_)
> {
> - return list_->qlen;
> + return READ_ONCE(list_->qlen);
> }
We do not want to add READ_ONCE() for all uses of skb_queue_len()
This could hide some real bugs, and could generate slightly less
efficient code in the cases we have the lock held.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] skbuff: fix a data race in skb_queue_len()
2020-02-03 19:41 ` Eric Dumazet
@ 2020-02-03 20:19 ` Qian Cai
2020-02-03 20:28 ` Eric Dumazet
0 siblings, 1 reply; 5+ messages in thread
From: Qian Cai @ 2020-02-03 20:19 UTC (permalink / raw)
To: Eric Dumazet; +Cc: davem, kuba, elver, netdev, linux-kernel
> On Feb 3, 2020, at 2:42 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> We do not want to add READ_ONCE() for all uses of skb_queue_len()
>
> This could hide some real bugs, and could generate slightly less
> efficient code in the cases we have the lock held.
Good point. I should have thought about that. How about introducing 2 new helpers.
skb_queue_len_once()
unix_recvq_full_once()
which will have a READ_ONCE() there, and then unix_dgram_sendmsg() could use that instead?
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] skbuff: fix a data race in skb_queue_len()
2020-02-03 20:19 ` Qian Cai
@ 2020-02-03 20:28 ` Eric Dumazet
2020-02-03 22:34 ` Qian Cai
0 siblings, 1 reply; 5+ messages in thread
From: Eric Dumazet @ 2020-02-03 20:28 UTC (permalink / raw)
To: Qian Cai, Eric Dumazet; +Cc: davem, kuba, elver, netdev, linux-kernel
On 2/3/20 12:19 PM, Qian Cai wrote:
>
>
>> On Feb 3, 2020, at 2:42 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>
>> We do not want to add READ_ONCE() for all uses of skb_queue_len()
>>
>> This could hide some real bugs, and could generate slightly less
>> efficient code in the cases we have the lock held.
>
> Good point. I should have thought about that. How about introducing 2 new helpers.
>
> skb_queue_len_once()
> unix_recvq_full_once()
>
> which will have a READ_ONCE() there, and then unix_dgram_sendmsg() could use that instead?
>
We added recently skb_queue_empty_lockless() helper, to use in these contexts.
The fact that we use READ_ONCE() is more of an implementation detail I think.
Also, addressing load-stearing issues without making sure the write side
is using WRITE_ONCE() might be not enough (even if KCSAN warnings disappear)
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] skbuff: fix a data race in skb_queue_len()
2020-02-03 20:28 ` Eric Dumazet
@ 2020-02-03 22:34 ` Qian Cai
0 siblings, 0 replies; 5+ messages in thread
From: Qian Cai @ 2020-02-03 22:34 UTC (permalink / raw)
To: Eric Dumazet; +Cc: davem, kuba, elver, netdev, linux-kernel
> On Feb 3, 2020, at 3:28 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> We added recently skb_queue_empty_lockless() helper, to use in these contexts.
>
> The fact that we use READ_ONCE() is more of an implementation detail I think.
>
Make sense. I’ll use lockless in naming instead.
> Also, addressing load-stearing issues without making sure the write side
> is using WRITE_ONCE() might be not enough (even if KCSAN warnings disappear)
I suppose that could be a case. I’ll have,
WRITE_ONCE(list->qlen, list->qlen - 1);
in __skb_unlink() where it had already had a few WRITE_ONCE() for other variables.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-02-03 22:34 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-03 18:56 [PATCH] skbuff: fix a data race in skb_queue_len() Qian Cai
2020-02-03 19:41 ` Eric Dumazet
2020-02-03 20:19 ` Qian Cai
2020-02-03 20:28 ` Eric Dumazet
2020-02-03 22:34 ` Qian Cai
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.