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 <f...@strlen.de> wrote:
>
> Gong, Sishuai <sish...@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.
