Message-ID: <36538615-7768-bdea-7829-6349729ab7cc@intel.com>
Date:   Tue, 21 Feb 2023 13:35:12 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH bpf-next v5] bpf, test_run: fix &xdp_frame misplacement
 for LIVE_FRAMES
To:     "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Eric Dumazet <edumazet@google.com>,
        Paolo Abeni <pabeni@redhat.com>,
        "Martin KaFai Lau" <martin.lau@linux.dev>
CC:     Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>,
        =?UTF-8?Q?Toke_H=c3=b8iland-J=c3=b8rgensen?= <toke@redhat.com>,
        Song Liu <song@kernel.org>,
        Jesper Dangaard Brouer <hawk@kernel.org>,
        <bpf@vger.kernel.org>, <netdev@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>,
        Martin KaFai Lau <martin.lau@kernel.org>
References: <20230220154627.72267-1-aleksander.lobakin@intel.com>
Content-Language: en-US
From:   Alexander Lobakin <aleksander.lobakin@intel.com>
In-Reply-To: <20230220154627.72267-1-aleksander.lobakin@intel.com>
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?VFJadEZDaksrTDduN2Zhbkk0aTJEQ1Z0ZmpFdExKUEloTmUvdEtCTEtaU3Ni?=
 =?utf-8?B?alRrSWxnR3NQNmtSc3dXUUNibVdFOUhhRUtSMGcyS0cxcXg1T0ZJNGNKem5a?=
 =?utf-8?B?a1VxMGw1M3NhcHNYM2xjWk5ZbGltc2pBNHVGenA0TWZaM2c1Ry9kOFR3OWt1?=
 =?utf-8?B?T0dQT1ZNdjNrZUFkbmhFcHdQRUtzQjU4d3pMU2dUdVFZRkpxa0J1V1Y5MlR5?=
 =?utf-8?B?cHFkYUZsWkpLbEtzVFIzTGZ1dkxJUkVaTW4zOVlpS0lDbEJ0T0JGSDIweFJi?=
 =?utf-8?B?cFFNWFpERkhVNGI4RjhTd0g5SEppd1hRTTBIdkZwK2xoZjhyNU5UZTJnMi9x?=
 =?utf-8?B?L0J5T2dhdW9tQ3B2TTRZZy8zMURLRkxxd1FNNWQyZS9xS1kxemZta0tLd05W?=
 =?utf-8?B?Z21qRHU4U0h2NDBQZC9OSjVqVjNvNi9KdXdHa2FhanlBcCtRRzhSVkxUTEEv?=
 =?utf-8?B?SGlOS2kxTUE2VUVGTW9TNC9RaXQvZjRPb2JhUWNZZmJOemZQWlZIV1lHQktT?=
 =?utf-8?B?dk56NXZpeDV3eWNxdWhOczI2cFBlM2RPSUR5eUhHSlQ5ekJVTFl2aUlZNGZ5?=
 =?utf-8?B?bDBHNHVNWERkNmtmQ1V5NEpJSnpDWktuSGRFZzdrS1dsQ3A5UWdraFFmQjh5?=
 =?utf-8?B?d0R4T1BOSGRlN2tsVlVjbHgzTEE3aFlXalpNemZBZ3hoWnIwMjl1RDR0MnNC?=
 =?utf-8?B?ZkNSTDNpK2FTTngvZkVqakp2R3FRNnZjU01LakFaY295VEIzblpsRHE2bHdY?=
 =?utf-8?B?cmFPTzVncnRQZmNNS0pXdkpPcWgxcnlpUXNCSVNJYXlvaHVGOHMzemZqbGov?=
 =?utf-8?B?S3ZOVjlFd3pjc1RPSFk3dXdPQ0ZySEFPUk5zTE9xVlRBbUVEVW9xZnNzdlRH?=
 =?utf-8?B?N0JDL0FDdlNrNTBLMzFEeE5kNDYydkJIZzdsRmJUMUk2cDZZV0dObE1LQ3Fk?=
 =?utf-8?B?U3FDd2tWZzByS0hiMnJaU1d2LzlkMUV1aHRrVjdLWkRDSDZIR0tSZHBIWmhk?=
 =?utf-8?B?STdTV2Y0d3h2eXdlRStPS1F1VExtcjFTL2JOWXo2U1ZQaWlRd1R5KzF2RjI5?=
 =?utf-8?B?Z0xiNFRXbU9jR0I5aHFCZXJJNXd4VHAvSnRpVGJJOHRjOGxGNkt2YTRDY2hP?=
 =?utf-8?B?Sm1GRGc4RHVjS25sSnE5cVVMN3BQTzJCMnBGUDlnNjAyY1Z5RVNIS1VZcUxv?=
 =?utf-8?B?S2dKQmIwZ21GTFNtODhZTFBhTmp0Zlk2NTBlTWtETGJUTTJxRGZXYW0xL3dv?=
 =?utf-8?B?Z2JDYzVYNlQ1S0pWTjFGWHlBOXZ3eTRzS0szZ0FqZnkvTkd0OW1YcUtMY25n?=
 =?utf-8?B?OVJkS1B6cm44Q1dvbkxKdWd2bGRaLzdKakxvcGp1ODhmdmUxSGxDOWZxcjB5?=
 =?utf-8?B?ZmtUWXZNT2xpeGJ4eG52UzBrS29lT2pFbUh1M3VwZFZycmV1RzV3cVArdTJx?=
 =?utf-8?B?NkNPZW0rZVBJeHBXZmg0VWw4dVlYMWp1NXZBcW9pMzRNTDh4ZEJsOXJNbTha?=
 =?utf-8?B?cWRDdEEzQXp5bzBRMDRPYi9MaHY5MU5yZk55WGNjU1BmYlJwSTV0ZGFuY2dy?=
 =?utf-8?B?UlA5N1pVaUJYRVorVEVYR0FOR3ZUSllWNWd2RUR5TGZEbnZXc21IM0gxZHUx?=
 =?utf-8?B?QjgxLzlUcDhRMEF3Rlcrbm5XTlE0dXhuQ2hwcWNFZnZWa1hQODROR1Bkc2Fq?=
 =?utf-8?B?dVpvZnVLcm1rKzIzcnFnZkZQUTYvamRJNHZxb3gyZVFMOStHZExUYlgrNVVG?=
 =?utf-8?B?a202cG95c1VZTW5NVUN4UTc4YlhpQStXVFJKbWVTK05jRUdJVXo5OHdDV0tW?=
 =?utf-8?B?Tk8xblNaTUdnVjdzVVBlUFRSVklmcnBrZWFLZFFRT3plWjJzK0xaWko4T1FQ?=
 =?utf-8?B?clFNaW1FbFFYYUJOMm9LL1p6UnpqU1ZSVk9NS2c1YUEvMXhIMWhMOEJXamNl?=
 =?utf-8?B?NWgyM1FjbjdPSXBqazhoSmdFMFpzKzQ1QUFrb2xySmZ6M05qWU9TNHR5L1I1?=
 =?utf-8?B?NjFGZlk0SWEzeVJEMlgyV1VFOWFKNk9WVis0UkNEeTcraVo4blR5dnFkMlI1?=
 =?utf-8?B?ZzF5VFdHYWRmSkk3aHd0T01YM2h1TWxnbWJoWjh3eDlXZlRic3ZWcG9Vbnpp?=
 =?utf-8?B?TkgyM082eGtHS1hnSnpWQzQ4OHF3Y0pOdWFweXY5bWJ1VXJDZXFSbWhUYTZJ?=
 =?utf-8?Q?Vrg5XQv60RFB/WjW0WXPWEM=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 3187e92e-5b5a-4f4a-f675-08db1408294a
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3625.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Feb 2023 12:35:49.0473
 (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: eldm3JR0NslRk9lE9D3iSwSTlkIFyKY0F7c6a2B8tp4vuCy1mUu1E/srqOLhi8eYuXQwqLShoLBAqCNE8DzD4wGMUM/9h9JfTgJDStFvkb4=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR11MB7116
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

From: Alexander Lobakin <aleksander.lobakin@intel.com>
Date: Mon, 20 Feb 2023 16:46:27 +0100

> &xdp_buff and &xdp_frame are bound in a way that
> 
> xdp_buff->data_hard_start == xdp_frame
> 
> It's always the case and e.g. xdp_convert_buff_to_frame() relies on
> this.
> IOW, the following:
> 
> 	for (u32 i = 0; i < 0xdead; i++) {
> 		xdpf = xdp_convert_buff_to_frame(&xdp);
> 		xdp_convert_frame_to_buff(xdpf, &xdp);
> 	}
> 
> shouldn't ever modify @xdpf's contents or the pointer itself.
> However, "live packet" code wrongly treats &xdp_frame as part of its
> context placed *before* the data_hard_start. With such flow,
> data_hard_start is sizeof(*xdpf) off to the right and no longer points
> to the XDP frame.
> 
> Instead of replacing `sizeof(ctx)` with `offsetof(ctx, xdpf)` in several
> places and praying that there are no more miscalcs left somewhere in the
> code, unionize ::frm with ::data in a flex array, so that both starts
> pointing to the actual data_hard_start and the XDP frame actually starts
> being a part of it, i.e. a part of the headroom, not the context.
> A nice side effect is that the maximum frame size for this mode gets
> increased by 40 bytes, as xdp_buff::frame_sz includes everything from
> data_hard_start (-> includes xdpf already) to the end of XDP/skb shared
> info.
> Also update %MAX_PKT_SIZE accordingly in the selftests code. Leave it
> hardcoded for 64 bit && 4k pages, it can be made more flexible later on.
> 
> Minor: align `&head->data` with how `head->frm` is assigned for
> consistency.
> Minor #2: rename 'frm' to 'frame' in &xdp_page_head while at it for
> clarity.
> 
> (was found while testing XDP traffic generator on ice, which calls
>  xdp_convert_frame_to_buff() for each XDP frame)

Sorry, maybe this could be taken directly to net-next while it's still
open? It was tested and then reverted from bpf-next only due to not 100%
compile-time assertion, which I removed in this version. No more
changes. I doubt there'll be a second PR from bpf and would like this to
hit mainline before RC1 :s

> 
> Fixes: b530e9e1063e ("bpf: Add "live packet" mode for XDP in BPF_PROG_RUN")
> Acked-by: Toke Høiland-Jørgensen <toke@redhat.com>
> Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>
> Link: https://lore.kernel.org/r/20230215185440.4126672-1-aleksander.lobakin@intel.com
> Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
(>_< those two last tags are incorrect, lemme know if I should resubmit
 it without them or you could do it if ok with taking it now)

Thanks,
Olek