Message-ID: <cdf1ae24-eebb-a9f8-65d3-01876334a33c@intel.com>
Date:   Fri, 17 Mar 2023 14:38:44 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCH bpf-next 2/2] selftests/bpf: fix "metadata marker" getting
 overwritten by the netstack
Content-Language: en-US
To:     =?UTF-8?Q?Toke_H=c3=b8iland-J=c3=b8rgensen?= <toke@redhat.com>
CC:     Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>,
        Martin KaFai Lau <martin.lau@linux.dev>,
        Maciej Fijalkowski <maciej.fijalkowski@intel.com>,
        Larysa Zaremba <larysa.zaremba@intel.com>,
        Ilya Leoshkevich <iii@linux.ibm.com>,
        Song Liu <song@kernel.org>,
        Jesper Dangaard Brouer <hawk@kernel.org>,
        Jakub Kicinski <kuba@kernel.org>, <bpf@vger.kernel.org>,
        <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>
References: <20230316175051.922550-1-aleksander.lobakin@intel.com>
 <20230316175051.922550-3-aleksander.lobakin@intel.com>
 <875yb0a25h.fsf@toke.dk>
From:   Alexander Lobakin <aleksander.lobakin@intel.com>
In-Reply-To: <875yb0a25h.fsf@toke.dk>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?SUNNcTcyZjFMc0RNbG96RXRXdmhaNndjTXFPQis0NGIweEZjeXJVOG9BOGxS?=
 =?utf-8?B?eVprNXdlMzRlbkFucDhRTGkrOTF0a1RXOWdZVko3VFA0UDl4Q3o2c3d0M1lu?=
 =?utf-8?B?bHJRY0hlMm1wd2VkYmFsRWwrMDBYNVdZNjJNYTZmYmtjWUJWOGJERzQwQVlV?=
 =?utf-8?B?RG5iVnhzRzZVSjRBSFBmTGh4K1RJeFFGV01BaTFESVJBRDROMG96REhxVVJ0?=
 =?utf-8?B?OXovMS9ac29QSllSdWF6WUZ3L0dQWmNJZnprQ3pCWGtDL2lCWlZ1YWVEYjVq?=
 =?utf-8?B?QldMV3J6N0RnenZ2K3pvNktaUFl1SFJTSmxhSXY4VC9CSU15U1l1TkxOdWtS?=
 =?utf-8?B?MDdkWDNkRzcvdjRwOEE1RFRkbitxOUFOcTVvRS9iaGxkNkl4MWRNdVZtNGd6?=
 =?utf-8?B?dkQ2cG1EaHVMMTRPY0VGa1FzcXdEK3pLMFI4TmpjVFRjT1dUTTVtNzRmdGRI?=
 =?utf-8?B?dC92bGhOWThLRVdGdnJWVE05ZmxiUTZxak5sckpLT3k2MUdUOWg5aWZrTzND?=
 =?utf-8?B?ZFM2RGdpeUlaK3ZiOHVKSDl6c3RxNXdYYnVhcWNEWUpZck1ucEUvSHAzUWNQ?=
 =?utf-8?B?Z0kreVJvdm94WDJoUGF5ck1EeVljOW5rSm9DZ0x1azFFOENTdlNqSFlYKzEv?=
 =?utf-8?B?eUlvUkQxQ0M5Q25pTnM2NG0zWmN3N0J6THZjclFFNFZGQWhTSUtCMTRaWEZW?=
 =?utf-8?B?STJSUVJPUUs5MytMdXFJb2RoWVpNTllKZXZOTE5jZ21KeFhpSUJORTB2Q09X?=
 =?utf-8?B?Vzd5WXNMMHlkWXN3NEthRE14SVIrVkVNTms4dVNBL3F3UUNxQ3FUN3EyVmpk?=
 =?utf-8?B?Y3d4cVdKUGt4Uk9ZYmV4UkpUVmVZT0h6WUZVSTZPckJTUU5DNTlSUG1rV1dV?=
 =?utf-8?B?QXh4TEJkQ2hmaUJDeklOTkY3eVUzZlN5d1lmTjc2eFA0dEVpMUF4Rm1BMmlH?=
 =?utf-8?B?QmhBOCs0cmFicVkrY1JvV1FrNENXWVNHM1VIaHFQeFpoaDB1ZmVrRXlsbFRz?=
 =?utf-8?B?MnhzcmVUOTMra2JPRUJZMTcxV1IrejZWS1VsajhGNzR4RE1zMEh6MDZWcEhC?=
 =?utf-8?B?aEZEbXMydXdPV3VVbXFGbDV4RU1WK0o4aWtmVjlRdjlXZFZ1NnFtM1FrRjBv?=
 =?utf-8?B?VCtNUmNPWlNkOEpmNCtwa1B2TW1ZTnB2eGtScVNGWEJkM242b2RvSkhjQ294?=
 =?utf-8?B?SnlEeTJGemc1MDBuZjJlZVk5bndtQ01RMk0vN3FDVmVnSVQ5ckNlQlVWdUhE?=
 =?utf-8?B?N3hEWnhqOFRxNFJZaHJRTE42ekZhN0tCY2RoV0t6OUlwT1JqTFN0ZnJmY3Zl?=
 =?utf-8?B?TXgxSEtoYTFDejhEZ2t6U1JNa1MvaXRrM2czK0tES3MyZFU0MHNRVDJlaWdS?=
 =?utf-8?B?WWNuOFA1YVF0SFRiUHpZb2Q3K0JMaDNiUHdqeXNUbVUxWmtPSi9vSDRtZG5L?=
 =?utf-8?B?aXpYd1ZmZzE4VU9Ebk41ZDFMZFM4QlBxR0NxaUduOEZ2VWNVNFB5OE9NVTBW?=
 =?utf-8?B?U0hzSDJPKytpV3NEV0VWdzJHK3I4cFJWQUpwTTF2dVYwbzZaWFpUa1paT05z?=
 =?utf-8?B?UU1Relc0ZFBlZVZFalVySnBROFlOazR0MVh4VFhnTFE5NmNFa0ppbjFkYWht?=
 =?utf-8?B?Vkk1RVVXWnhRcXlxaHhUenNoUytDcDlFT2lWL3VDOGIwcnFGQmdkV2JaV2lV?=
 =?utf-8?B?V0VJem9uYUczMHpFclZkRHl2aXhVVU1rNDFKelVibVVtYy9lQkI4a1Q0dDc5?=
 =?utf-8?B?U25pVHRjTCtYYURTS2JjNDFkakZqTSs0djl2UW1rNHc1cGloNFFPMVlHWVRo?=
 =?utf-8?B?SmJPOUd1bTRCQk9HOW9MUzZMZVVwczVNOWhVQkhrbHJjbFE0YTVaTEtnQWRE?=
 =?utf-8?B?Qi9hTzd3R0srclkxMkdlc3BrRExCQnVtWFk0VHhjekF6ZlYxeHlKSDNzWEor?=
 =?utf-8?B?ZzE4WURKTEkrT0NtRDBMRlY0S29HMEVVMlNrRXRGOTlXSDVQeVdkZXhYSGdU?=
 =?utf-8?B?R3NyZkVOai91RkhuVGpMc3BGN2R3OGpTeitNUUNNQkFRNUwwTWhBM2JMZWk1?=
 =?utf-8?B?U3NjVDZFM0VLTGZWZGhnbHdneGpGNFpDeHd0c1ZFUEVSemtlNkNoNWwzcDJJ?=
 =?utf-8?B?Tkd4aFlpQTN4RnYzbTFhYXFnWXJQWUEzTVByamU4Q1h0VittNlJCdUFwV05H?=
 =?utf-8?Q?rxLwuQoJF0EWNNP58T3Ug2g=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 687543ee-a94c-4804-0044-08db26ed189c
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3625.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Mar 2023 13:39:56.8319
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: UyR9MzZPW/eYZmS4vaNxpkzgTDvh+gA/4TMDbUAnO/4Ac3EzHtAfZliXDAg/lrnsCJeIcnDIATLFI7IEFYYkuH+ha26wgb83mS1CiPAmZIM=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL1PR11MB5978
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

From: Toke Høiland-Jørgensen <toke@redhat.com>
Date: Thu, 16 Mar 2023 21:10:02 +0100

> Alexander Lobakin <aleksander.lobakin@intel.com> writes:
> 
>> Alexei noticed xdp_do_redirect test on BPF CI started failing on
>> BE systems after skb PP recycling was enabled:
>>
>> test_xdp_do_redirect:PASS:prog_run 0 nsec
>> test_xdp_do_redirect:PASS:pkt_count_xdp 0 nsec
>> test_xdp_do_redirect:PASS:pkt_count_zero 0 nsec
>> test_xdp_do_redirect:FAIL:pkt_count_tc unexpected pkt_count_tc: actual
>> 220 != expected 9998
>> test_max_pkt_size:PASS:prog_run_max_size 0 nsec
>> test_max_pkt_size:PASS:prog_run_too_big 0 nsec
>> close_netns:PASS:setns 0 nsec
>>  #289 xdp_do_redirect:FAIL
>> Summary: 270/1674 PASSED, 30 SKIPPED, 1 FAILED
>>
>> and it doesn't happen on LE systems.
>> Ilya then hunted it down to:
>>
>>  #0  0x0000000000aaeee6 in neigh_hh_output (hh=0x83258df0,
>> skb=0x88142200) at linux/include/net/neighbour.h:503
>>  #1  0x0000000000ab2cda in neigh_output (skip_cache=false,
>> skb=0x88142200, n=<optimized out>) at linux/include/net/neighbour.h:544
>>  #2  ip6_finish_output2 (net=net@entry=0x88edba00, sk=sk@entry=0x0,
>> skb=skb@entry=0x88142200) at linux/net/ipv6/ip6_output.c:134
>>  #3  0x0000000000ab4cbc in __ip6_finish_output (skb=0x88142200, sk=0x0,
>> net=0x88edba00) at linux/net/ipv6/ip6_output.c:195
>>  #4  ip6_finish_output (net=0x88edba00, sk=0x0, skb=0x88142200) at
>> linux/net/ipv6/ip6_output.c:206
>>
>> xdp_do_redirect test places a u32 marker (0x42) right before the Ethernet
>> header to check it then in the XDP program and return %XDP_ABORTED if it's
>> not there. Neigh xmit code likes to round up hard header length to speed
>> up copying the header, so it overwrites two bytes in front of the Eth
>> header. On LE systems, 0x42 is one byte at `data - 4`, while on BE it's
>> `data - 1`, what explains why it happens only there.
>> It didn't happen previously due to that %XDP_PASS meant the page will be
>> discarded and replaced by a new one, but now it can be recycled as well,
>> while bpf_test_run code doesn't reinitialize the content of recycled
>> pages. This mark is limited to this particular test and its setup though,
>> so there's no need to predict 1000 different possible cases. Just move
>> it 4 bytes to the left, still keeping it 32 bit to match on more
>> bytes.
> 
> Wow, this must have been annoying to track down - nice work :)

I just blinked my eyes once and Ilya already came back with the detailed
stacktrace, so it's almost entirely his work <O

> 
>> Fixes: 9c94bbf9a87b ("xdp: recycle Page Pool backed skbs built from XDP frames")
>> Reported-by: Alexei Starovoitov <ast@kernel.org>
>> Link: https://lore.kernel.org/bpf/CAADnVQ+B_JOU+EpP=DKhbY9yXdN6GiRPnpTTXfEZ9sNkUeb-yQ@mail.gmail.com
>> Reported-by: Ilya Leoshkevich <iii@linux.ibm.com> # + debugging
>> Link: https://lore.kernel.org/bpf/8341c1d9f935f410438e79d3bd8a9cc50aefe105.camel@linux.ibm.com
>> Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>
> 
> Acked-by: Toke Høiland-Jørgensen <toke@redhat.com>
> 

Thanks! That was quick :D

Olek