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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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.
> On Feb 3, 2020, at 2:42 PM, Eric Dumazet <[email protected]> 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?
On 2/3/20 12:19 PM, Qian Cai wrote:
>
>
>> On Feb 3, 2020, at 2:42 PM, Eric Dumazet <[email protected]> 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)
> On Feb 3, 2020, at 3:28 PM, Eric Dumazet <[email protected]> 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.