* [Race] data race between eth_heder_cache_update() and neigh_hh_output()
@ 2020-11-30 15:40 Gong, Sishuai
2020-11-30 15:53 ` Florian Westphal
0 siblings, 1 reply; 3+ messages in thread
From: Gong, Sishuai @ 2020-11-30 15:40 UTC (permalink / raw)
To: davem@davemloft.net; +Cc: netdev@vger.kernel.org
Hi,
We found a data race in linux kernel 5.3.11 that we are able to reproduce in x86 under specific interleavings. We are not sure about the consequence of this race now but it seems that the two memcpy() can lead to some inconsistency. We also noticed that both the writer and reader are protected by locks, but the writer is protected using seqlock while the reader is protected by rculock.
------------------------------------------
Write site
/tmp/tmp.B7zb7od2zE-5.3.11/extract/linux-5.3.11/net/ethernet/eth.c:264
252 /**
253 * eth_header_cache_update - update cache entry
254 * @hh: destination cache entry
255 * @dev: network device
256 * @haddr: new hardware address
257 *
258 * Called by Address Resolution module to notify changes in address.
259 */
260 void eth_header_cache_update(struct hh_cache *hh,
261 const struct net_device *dev,
262 const unsigned char *haddr)
263 {
==> 264 memcpy(((u8 *) hh->hh_data) + HH_DATA_OFF(sizeof(struct ethhdr)),
265 haddr, ETH_ALEN);
266 }
267 EXPORT_SYMBOL(eth_header_cache_update);
------------------------------------------
Reader site
/tmp/tmp.B7zb7od2zE-5.3.11/extract/linux-5.3.11/include/net/neighbour.h:481
463 static inline int neigh_hh_output(const struct hh_cache *hh, struct sk_buff *skb)
464 {
465 unsigned int hh_alen = 0;
466 unsigned int seq;
467 unsigned int hh_len;
468
469 do {
470 seq = read_seqbegin(&hh->hh_lock);
471 hh_len = hh->hh_len;
472 if (likely(hh_len <= HH_DATA_MOD)) {
473 hh_alen = HH_DATA_MOD;
474
475 /* skb_push() would proceed silently if we have room for
476 * the unaligned size but not for the aligned size:
477 * check headroom explicitly.
478 */
479 if (likely(skb_headroom(skb) >= HH_DATA_MOD)) {
480 /* this is inlined by gcc */
==> 481 memcpy(skb->data - HH_DATA_MOD, hh->hh_data,
482 HH_DATA_MOD);
483 }
484 } else {
485 hh_alen = HH_DATA_ALIGN(hh_len);
486
487 if (likely(skb_headroom(skb) >= hh_alen)) {
488 memcpy(skb->data - hh_alen, hh->hh_data,
489 hh_alen);
490 }
491 }
492 } while (read_seqretry(&hh->hh_lock, seq));
493
494 if (WARN_ON_ONCE(skb_headroom(skb) < hh_alen)) {
495 kfree_skb(skb);
496 return NET_XMIT_DROP;
497 }
498
499 __skb_push(skb, hh_len);
500 return dev_queue_xmit(skb);
501 }
------------------------------------------
Writer calling trace
- ksys_ioctl
-- do_vfs_ioctl
--- vfs_ioctl
---- arp_ioctl
----- arp_req_set
------ neigh_update
------- __neigh_update
------------------------------------------
Reader calling trace
- __sys_sendto
-- sock_sendmsg
--- inet_sendmsg
---- ip_push_pending_frames
----- ip_send_skb
------ ip_local_out
------- ip_finish_output
-------- __ip_finish_output
--------- ip_finish_output2
Thanks,
Sishuai
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Race] data race between eth_heder_cache_update() and neigh_hh_output()
2020-11-30 15:40 [Race] data race between eth_heder_cache_update() and neigh_hh_output() Gong, Sishuai
@ 2020-11-30 15:53 ` Florian Westphal
2020-11-30 15:56 ` Gong, Sishuai
0 siblings, 1 reply; 3+ messages in thread
From: Florian Westphal @ 2020-11-30 15:53 UTC (permalink / raw)
To: Gong, Sishuai; +Cc: davem@davemloft.net, netdev@vger.kernel.org
Gong, Sishuai <sishuai@purdue.edu> wrote:
> Hi,
>
> We found a data race in linux kernel 5.3.11 that we are able to reproduce in x86 under specific interleavings. We are not sure about the consequence of this race now but it seems that the two memcpy() can lead to some inconsistency. We also noticed that both the writer and reader are protected by locks, but the writer is protected using seqlock while the reader is protected by rculock.
AFAICS reader uses same seqlock as the writer.
> ------------------------------------------
> Write site
>
> /tmp/tmp.B7zb7od2zE-5.3.11/extract/linux-5.3.11/net/ethernet/eth.c:264
> 252 /**
> 253 * eth_header_cache_update - update cache entry
> 254 * @hh: destination cache entry
> 255 * @dev: network device
> 256 * @haddr: new hardware address
> 257 *
> 258 * Called by Address Resolution module to notify changes in address.
> 259 */
> 260 void eth_header_cache_update(struct hh_cache *hh,
> 261 const struct net_device *dev,
> 262 const unsigned char *haddr)
> 263 {
> ==> 264 memcpy(((u8 *) hh->hh_data) + HH_DATA_OFF(sizeof(struct ethhdr)),
> 265 haddr, ETH_ALEN);
This is called under write_seqlock_bh(hh->hh_lock)
> /tmp/tmp.B7zb7od2zE-5.3.11/extract/linux-5.3.11/include/net/neighbour.h:481
> 463 static inline int neigh_hh_output(const struct hh_cache *hh, struct sk_buff *skb)
> 464 {
> 465 unsigned int hh_alen = 0;
> 466 unsigned int seq;
> 467 unsigned int hh_len;
> 468
> 469 do {
> 470 seq = read_seqbegin(&hh->hh_lock);
This samples the seqcount.
> 471 hh_len = hh->hh_len;
> 472 if (likely(hh_len <= HH_DATA_MOD)) {
> 473 hh_alen = HH_DATA_MOD;
> 474
> 475 /* skb_push() would proceed silently if we have room for
> 476 * the unaligned size but not for the aligned size:
> 477 * check headroom explicitly.
> 478 */
> 479 if (likely(skb_headroom(skb) >= HH_DATA_MOD)) {
> 480 /* this is inlined by gcc */
> ==> 481 memcpy(skb->data - HH_DATA_MOD, hh->hh_data,
> 482 HH_DATA_MOD);
[..]
> 492 } while (read_seqretry(&hh->hh_lock, seq));
... so this retries unless seqcount was stable.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Race] data race between eth_heder_cache_update() and neigh_hh_output()
2020-11-30 15:53 ` Florian Westphal
@ 2020-11-30 15:56 ` Gong, Sishuai
0 siblings, 0 replies; 3+ messages in thread
From: Gong, Sishuai @ 2020-11-30 15:56 UTC (permalink / raw)
To: Florian Westphal; +Cc: davem@davemloft.net, netdev@vger.kernel.org
Hi,
Thanks for clarifying our confusion and we are sorry if we caused any trouble.
> On Nov 30, 2020, at 10:53 AM, Florian Westphal <fw@strlen.de> wrote:
>
> Gong, Sishuai <sishuai@purdue.edu> wrote:
>> Hi,
>>
>> We found a data race in linux kernel 5.3.11 that we are able to reproduce in x86 under specific interleavings. We are not sure about the consequence of this race now but it seems that the two memcpy() can lead to some inconsistency. We also noticed that both the writer and reader are protected by locks, but the writer is protected using seqlock while the reader is protected by rculock.
>
> AFAICS reader uses same seqlock as the writer.
>
>> ------------------------------------------
>> Write site
>>
>> /tmp/tmp.B7zb7od2zE-5.3.11/extract/linux-5.3.11/net/ethernet/eth.c:264
>> 252 /**
>> 253 * eth_header_cache_update - update cache entry
>> 254 * @hh: destination cache entry
>> 255 * @dev: network device
>> 256 * @haddr: new hardware address
>> 257 *
>> 258 * Called by Address Resolution module to notify changes in address.
>> 259 */
>> 260 void eth_header_cache_update(struct hh_cache *hh,
>> 261 const struct net_device *dev,
>> 262 const unsigned char *haddr)
>> 263 {
>> ==> 264 memcpy(((u8 *) hh->hh_data) + HH_DATA_OFF(sizeof(struct ethhdr)),
>> 265 haddr, ETH_ALEN);
>
> This is called under write_seqlock_bh(hh->hh_lock)
>
>> /tmp/tmp.B7zb7od2zE-5.3.11/extract/linux-5.3.11/include/net/neighbour.h:481
>> 463 static inline int neigh_hh_output(const struct hh_cache *hh, struct sk_buff *skb)
>> 464 {
>> 465 unsigned int hh_alen = 0;
>> 466 unsigned int seq;
>> 467 unsigned int hh_len;
>> 468
>> 469 do {
>> 470 seq = read_seqbegin(&hh->hh_lock);
>
> This samples the seqcount.
>
>> 471 hh_len = hh->hh_len;
>> 472 if (likely(hh_len <= HH_DATA_MOD)) {
>> 473 hh_alen = HH_DATA_MOD;
>> 474
>> 475 /* skb_push() would proceed silently if we have room for
>> 476 * the unaligned size but not for the aligned size:
>> 477 * check headroom explicitly.
>> 478 */
>> 479 if (likely(skb_headroom(skb) >= HH_DATA_MOD)) {
>> 480 /* this is inlined by gcc */
>> ==> 481 memcpy(skb->data - HH_DATA_MOD, hh->hh_data,
>> 482 HH_DATA_MOD);
>
> [..]
>
>> 492 } while (read_seqretry(&hh->hh_lock, seq));
>
> ... so this retries unless seqcount was stable.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-11-30 15:57 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-11-30 15:40 [Race] data race between eth_heder_cache_update() and neigh_hh_output() Gong, Sishuai
2020-11-30 15:53 ` Florian Westphal
2020-11-30 15:56 ` Gong, Sishuai
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.