From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 19DE145C4B; Fri, 1 Nov 2024 14:42:25 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C1A744027C; Fri, 1 Nov 2024 14:42:24 +0100 (CET) Received: from FR6P281CU001.outbound.protection.outlook.com (mail-germanywestcentralazon11020072.outbound.protection.outlook.com [52.101.171.72]) by mails.dpdk.org (Postfix) with ESMTP id 495514003C for ; Fri, 1 Nov 2024 14:42:23 +0100 (CET) ARC-Seal: i=1; a=rsa-sha256; s=arcselector10001; d=microsoft.com; cv=none; b=JFbAfu7/45lqWBdEOiFSTY2sfL8bEDEZAORM+h1p0TPZB7RpL3F4KYg9ScvXRRCCvQCplNQKEJ8yf9dzW+5TDyke93Pop+4ao7nibXH1pZJJ8auZcu3XBAIDUKHNE5QqGQ2aLxcPfeskRRSmmM3MbhqMH8e6HDqN/k6XH6vGe0hLVBhAztCIvM+l9Vw2OalLA+WFHrTsYbkRcEAd2eg2W06CAZIicAj6QMXmOnb0sa3U2+kwSgSqkF7oo7y47RRRItLC99MGlZXNAfE6JKd3lVn8U2llYauTyhytoBkUScPvpOlodQJgS5Ts2OBxp3P/6X5kxcLnbOGYr7KgsYRZ2Q== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector10001; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=Tu5tHradhti8PuTloBmb9opo9VfCBuCZcrNfFpkcu24=; b=S1opULs12uKN3qeJbc1ZHHSr+NHcTGfu4xxFdwLc2zHgVygRfqKupb1JiSr4ptpbEp1BAGBOCBSL4cNl6TQX0jPuhyEF/Y9UUN7DwWf2fqmjSMbWa6sZAzwzZ8CvQn82NB+c6Fcv1g2PzSenneL85KkzVFVTzZvEZ8hdVbRzoSe56Dk3LFuN1vOMtGB0RoGgQ94PLXMPFyoFYJm5DbyzA5FULd3xuVoEpQHRxtzCEb1N7HzsOPZpvDBczxrEAd8AOJ5zlqfKnSQwGwPG6Fjh3xdNusuHl7u4hZQA6SlQIaD8BZtmr17sGzaWM5UiOHBLNa3yYfSiaKKRjqRImSsfvQ== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=allegro-packets.com; dmarc=pass action=none header.from=allegro-packets.com; dkim=pass header.d=allegro-packets.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=allegro-packets.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=Tu5tHradhti8PuTloBmb9opo9VfCBuCZcrNfFpkcu24=; b=Qiqgqxa2cwlt3UNcLfaS7k7ld3A2ZqZaN53Owx+6mynrHALG/Yejw0OS9tnorAuJm8DF79xTlp7MTgnG30Bqu1gl41nWcq2qyB/jnJF+SzYyzG17UsYYrDFvfACZRDO5mGrgTga9i4ciKsizSJa3JuPx5Y2MOuRUmZfXwY8JMi1QqzszfErUG0LxeyzXUnWWLxWD2qSyeK8sam655iRKgAmLzAvp9hPxx6rrDXdLXdOB7SO2MAXIK14/xnaF+Z9UeRkZFxogmxLCmCxds+oee+DMko0AZb19ztHUHPn1I/XQzHzlIHaYrragcj0fsrMj0SuB66JO1/F45NO+1sxlOQ== Authentication-Results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=allegro-packets.com; Received: from BE1P281MB3111.DEUP281.PROD.OUTLOOK.COM (2603:10a6:b10:60::13) by FR2PPFB885F590A.DEUP281.PROD.OUTLOOK.COM (2603:10a6:d18:2::82) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.8114.25; Fri, 1 Nov 2024 13:42:21 +0000 Received: from BE1P281MB3111.DEUP281.PROD.OUTLOOK.COM ([fe80::e877:d319:1b52:4c53]) by BE1P281MB3111.DEUP281.PROD.OUTLOOK.COM ([fe80::e877:d319:1b52:4c53%7]) with mapi id 15.20.8114.015; Fri, 1 Nov 2024 13:42:21 +0000 Message-ID: <14d31c7f-1a67-4b18-9e9f-d0aadeec49c2@allegro-packets.com> Date: Fri, 1 Nov 2024 14:42:20 +0100 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] igc: fix invalid length and corrupted multi-segment mbufs To: Bruce Richardson Cc: "dev@dpdk.org" References: Content-Language: en-US From: Martin Weiser In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-ClientProxiedBy: FR0P281CA0098.DEUP281.PROD.OUTLOOK.COM (2603:10a6:d10:a9::8) To BE1P281MB3111.DEUP281.PROD.OUTLOOK.COM (2603:10a6:b10:60::13) MIME-Version: 1.0 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: BE1P281MB3111:EE_|FR2PPFB885F590A:EE_ X-MS-Office365-Filtering-Correlation-Id: 423acc1a-2527-4b86-ed20-08dcfa7b02a4 X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0; ARA:13230040|1800799024|366016|10070799003|376014; X-Microsoft-Antispam-Message-Info: =?utf-8?B?aHdYYWNZVWF0WEplNEFyN3M3NVV4ck0zbDhYL29vRnA2U3J0QXZ6WW9uRTZa?= =?utf-8?B?eTlMemxNaW1KeVJ0cWIwd0Q0RTVRSzcwTnJmN3FSZ3VnQnZjRmJsUVU3Sk9L?= =?utf-8?B?WjQ2M0FXM1ZCQlJiNDNUZ3VQNUFiUWhWbjQwM0p5bENDRXhQMlZUMGJuRlE2?= =?utf-8?B?VXFIYy9DMWQ0NE41T0lGYmtHemFia0t2VTlmczRDSUVQZ2ZNeDhraXc2UDFX?= =?utf-8?B?Smp1OERmUXBwNHBOblI2WXg2dHgxMDVrWXlLRnJQVG5ubGlqY01CQ3dBTEFy?= =?utf-8?B?RkpFQTlEUEM0YlhkR1MxUk1Bbk14alg5T0RKMFB4QW1LMzlsRGZxcitVZDNl?= =?utf-8?B?akdEenNwVTV2MDRqOE5wbU5UTkgxTEFOdG44WENpWERST04wc3pFZVZDNXdh?= =?utf-8?B?UUdyclNrdVlscHVBelRoN2FydXFtd0tMTEVlbXQyeXJPd2dpSVczSUVpMFVC?= =?utf-8?B?TjJaNkI1U3k1RlRyOGZWQ1d4QnU0eVp1aVFuajM0VTVRaGsrRmZaZVhkYXVL?= =?utf-8?B?bDJlV3RQNEVNRHpPcm9uM1ZqVUJCdi9LNElIc09QQjE2cmFhWEhPUm9qa3cx?= =?utf-8?B?VmJTd3hXbkxLbU5INVA4WUtwT1d0NTk0cWcybDNKRm9rUHJQVysycmtYdHpo?= =?utf-8?B?Y3Q5aWFOQ1YyZGRuQlVXMktIYzFxTjRoMklRM2s4WEFYazB6bjdYMXN2WlNF?= =?utf-8?B?c1U0SExXTEhEMFgveGU3K2RmVnRQV1VnYW5JTWtyTmllK1lTUndHa0h3NG5D?= =?utf-8?B?Mm04dktKZlNQc0ZsOXVHY1VIdzBmZ0xSdnJxNnRFYXFnOW5TeWFlWUtscGFR?= =?utf-8?B?WmppY3hZMi9SbjFxSDd2cXg3aGw4SEx6d1lRZHIraVJ2K3V5MkFJOTU4aUNn?= =?utf-8?B?OXFaWksxOGpPZEpKcFVZQUxFbkorb3RZN0JMamFmdEdvODhjUFV4aGFqVDl0?= =?utf-8?B?bGxRZmlhVndybFFJeFg0djZHV1hxZnk1MEp0TytCSEJDMFc4UjZ3aEZ5YjJt?= =?utf-8?B?RGpJWERyVDFNMkNWMldtYVFpbWRncW9BNlViR1lFOGNpZGUxNkxBNGRJSmxW?= =?utf-8?B?bm5Qb0M5VHNuQVRvVjZocDhuR2VOcDY4a1cwQ1ZnY2RIeFd1S0tpbFdSYkdM?= =?utf-8?B?V2xrRlYxMWkwdXltZG5qMVh2Vm0xeGNDVWxJNnVId1pweXk1Sy9UYVRlSWYv?= =?utf-8?B?VWJTdGkva0FSVHhBVk1lYnZDbW9Ib1Z6L2FNTDR5cjZaZDY2bVRjeFB5Q2JM?= =?utf-8?B?ZHM2cDU2Z1lXbzJVa25xRERDcGQzS1JuQ2VHN2lzOTBoNGwvOGlyN05zNHh1?= =?utf-8?B?RFhFWkNWOXcwTmxGWHkvLzdwWExWRG80ZnZOckFzaUFWUE1UeXV6dlhHUWow?= =?utf-8?B?eFZpbEhsTVVwaEFVS0wzSzV1VnlqclJ5RUZ0eDVKaUhCcnFrTUhPbnBQNEFM?= =?utf-8?B?MnRsc25uVDNaa2ZVcDlrT1U5U2pXNWJ4NUFxTmRVN1lCK1EwbjlrRmUzRjA0?= =?utf-8?B?b3dKRDU2WWtxV1RKWExMTG1aci9PeTBjU2VRd1ZZTDVRcXdVbDVFbUVEeHdX?= =?utf-8?B?YmpJVWhNcmtlU09TYUpXNHc0RmoxOE5KTkd5MWdjeFhXK1VsOXF1aGFrTjc0?= =?utf-8?B?TkNTalFzc2VRbWNibHpTU1I5WmdnYU5Ra2hBZVRUNGNsdzZyVk8rTXF1dldH?= =?utf-8?B?L3A0dE8zNjEzYWt3MkpnSFN5QjI4NW13aGhySHdaNDF3d2o4a3NqM2tUdkdE?= =?utf-8?Q?oNzjHV65Sos8oQfhQggXeu3EKn59JSn/Xnp7q6e?= X-Forefront-Antispam-Report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:BE1P281MB3111.DEUP281.PROD.OUTLOOK.COM; PTR:; CAT:NONE; SFS:(13230040)(1800799024)(366016)(10070799003)(376014); DIR:OUT; SFP:1102; X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?TDdEanZGRFFoM01Id1JOREJEN05UN00vT1lCSi83WDR3MkNLZ293M1FpVVBi?= =?utf-8?B?aFZEWjFEVnViZzFlb0s5TjhRMndLeFlNUDBFTyttUk9VYVI0TWNVSVM0OXgz?= =?utf-8?B?SldQc3I2Qm0rRERid3NrYWllUWI3YU15KzNJWVRpOHQxQUFUR05VWHR1K3Ar?= =?utf-8?B?ZzB4WXhUN0hDMWp6b3JCRURXQStzOUJSSzM3aGgza3JGL3o4RDdpWjkzM255?= =?utf-8?B?L1dWNkhlSC8xYXlNSG9aL3VhZzErTEliVnZ3Zy96VGlYWFhJV1NBREozSm9Q?= =?utf-8?B?bW1Zakh3OVJvc2FZaUp2dUtWL0ZySTFxMmh3REZxWStOZ3c5SzZvMXBuMXlk?= =?utf-8?B?NzhXMUlJZHo5RVRKMkl0YmYxL25mVnJMSG5JSlhuS2dSZlFxbitKRmZhc2hs?= =?utf-8?B?TXhFdWlaSDliZkt3KzRSQUxGOERteFp1a3dwK2hvMGsxYTZ6bFRRdW9mR1Bo?= =?utf-8?B?Y0dNMkowSWZBNW9Ib0ZvakxQVFcrTTRuaDBxbDRXMUhLb1d3elJRSTJ1YnVs?= =?utf-8?B?UnBzbFk3YjNQdzExODJGWmtwenliWVowMWlxNEMrdlFZWXRySk9UL1JNd3dq?= =?utf-8?B?VTJ4ZTN4bTE1ZEtRSC9RODdnUkpoWXBNZjFPb0lRQ284QzJiSHJkbmdYWHUr?= =?utf-8?B?cnp3QUVEekRQNkE0UXdTV1NGQ3FCUTR6VjY4Q3d3emZsRU9YM09VVHdaRSs0?= =?utf-8?B?NEUzWGdOcllCaFo3a2oxY3VxVXRLbGJ4NThKVk8xNDBLa3RzaElOTVRYcFRL?= =?utf-8?B?N2VJeU1rSTlqK0tPN044UWxYMnVDU3YwWVVCczJvMGVEdFgxZDQrVDFUd21B?= =?utf-8?B?UW1EZ3B0U01RTFpxRjB3NkNqeXU4KzFYbTRsVW1OYWt5dGpXbzBkUGZJNEpH?= =?utf-8?B?Z2MyTkRzaVp3WU0xQTlmNlA3L3h6MndvaVRXRGlwWExKd3h2SmlLN3NqYi9q?= =?utf-8?B?R0FNRTRTL05sOU45V3dKNzRmTUxXeTRHeDFNNlNKOS9YZ0NPZGJaVFpIRXBZ?= =?utf-8?B?eUZGRTdjcXduYjNkMUU5R21FLzhWSU8zUnRUeCtxZmprN3RKZFExTE41dE54?= =?utf-8?B?S2YxVnp1NlJZZEpQbGdXcWtiaDl0d0VwSW1rRm4wbW5lOGxscm5USE1hMTUx?= =?utf-8?B?eW9RMUp0UThWaDRFSlRobExwN3JXQjBBZ3dKelVDMm9tUHZWMUY4VEI4YWt2?= =?utf-8?B?cnpJR01zMGJRYk1pOXMyR0dzQXV1emMzZzAwWVZqRjdZcVdCVHlra2IyOUpy?= =?utf-8?B?RkhhRERaQ0o2MW9XZzNDSnBKdkVRRFYxMTRSNkd6M3o1aXdRSTBnR2xxZWFT?= =?utf-8?B?OEVrSlBmZU9icVlITjdxaFdYdjN0S2ozc1Y3YU5kYzdpVk9yM3BYZFI0bFcr?= =?utf-8?B?ZTlyR01GZFNpRk1OZDZlZDU4c1Q5a1hqQTBWdEw5STlydVRjaU5CSjR2U2RK?= =?utf-8?B?YjZDbGVOSEMwWExrbG1OQnVTTHpUcnlDMlhyNlFROHI4ZVpNR2E3UVFDK01T?= =?utf-8?B?amxheUcyWkYyekxVcGpFQitSYUdETG51ekVaUmRidkFiQVp2U1FaZ0xTdXJW?= =?utf-8?B?N05HekhHV3NteDhPTEtKMFpTbDlDRUU0bnNGYU1VYXQvanRwYUg4RC9zekJj?= =?utf-8?B?d1k0c2dDd3JTNjkvaEZSek5QbHdHK0MyazVBWTh5SGlscW9ueFErQy9kaGda?= =?utf-8?B?K2VCNEZIVTlrZXoyaGRtMldGcWduSGJKV1ppVS9uYjZWOXhGQklmcDVRUDl0?= =?utf-8?B?TUhNajlmUHpaZkVNdDZ2bGdkZ1IyYlQyenpMejJOZTFGWXFiT0dGa1dLWlQ0?= =?utf-8?B?Z05MVjhpNnlXSkcvU0IwNVc2TEx0Ymd6aGViZlR1YlVyUmJtc3dpaFRTM2R3?= =?utf-8?B?VWtOVWNZQ1VFK29sa0lOQjZiUVFSajZFV3lUUk8xOXkraUtDWkNYWGwzY0Vp?= =?utf-8?B?NTAvTzd4dFZKcnBWV2VlVDRuWVJhSFZlRGNJRk4xdG1Bai9wZWFJRmxUOFN3?= =?utf-8?B?MFQzRjh4UHh3M3NyNUI0VGtuOElBeWlGaExYUkNpUktXdEFwRG11T0RCT21I?= =?utf-8?B?empQdXVNa3c3cy90SklyTThhR29Tb09qRmF4U2RmV2o1STNxS3FZZ3N2d0J1?= =?utf-8?B?eG1ieXdSdXM4YU1oZDVHakxQU2MyY0lNemZiMUFiaEduRFYwbHdYayt0NU9Y?= =?utf-8?B?cjRFb2tLV2hCYTVrZnhxWlFaZHV0enczWFY3NzNISnRLUjhtMDRHbVZEOFNF?= =?utf-8?B?R1dRaDMvbDk5OC9jL2lZV3BxSW9nPT0=?= X-OriginatorOrg: allegro-packets.com X-MS-Exchange-CrossTenant-Network-Message-Id: 423acc1a-2527-4b86-ed20-08dcfa7b02a4 X-MS-Exchange-CrossTenant-AuthSource: BE1P281MB3111.DEUP281.PROD.OUTLOOK.COM X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Nov 2024 13:42:21.3378 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: c65709ba-d173-4248-a752-2f88437ee1ad X-MS-Exchange-CrossTenant-MailboxType: HOSTED X-MS-Exchange-CrossTenant-UserPrincipalName: pfbDiTlhQdG6xbMSsVs37HVDvha/L0l8eJwsEnlEhSXCilXEcM/kKRHmWM7xoqXjq6eGVQAAqNImuC25HbanaP+Hv/wHDhVOUPT8oHjhAmt0dYeR6P4/K09OetPDKHZA X-MS-Exchange-Transport-CrossTenantHeadersStamped: FR2PPFB885F590A X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Hi Bruce, thank you very much for your feedback. Please see my answers inline below. I will send a v2 of the patch. Best regards, Martin Am 29.10.24 um 18:42 schrieb Bruce Richardson: > On Mon, Oct 28, 2024 at 03:17:07PM +0100, Martin Weiser wrote: >> >> The issue only appeared with hardware-timestamping enabled >> (RTE_ETH_RX_OFFLOAD_TIMESTAMP). >> >> The length of the prepended hardware timestamp was not subtracted from >> the data length so that received packets were 16 bytes longer than >> expected. >> >> In scatter-gather mode only the first mbuf has a timestamp but the >> data offset of the follow-up mbufs was not adjusted accordingly. >> This caused 16 bytes of packet data to be missing between >> the segments. >> >> Signed-off-by: Martin Weiser >> --- > > Hi, > > thanks for the patch. Some comments inline below. > > /Bruce > >> drivers/net/igc/igc_txrx.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/drivers/net/igc/igc_txrx.c b/drivers/net/igc/igc_txrx.c >> index d0cee1b016..2fafa91bd5 100644 >> --- a/drivers/net/igc/igc_txrx.c >> +++ b/drivers/net/igc/igc_txrx.c >> @@ -347,6 +347,8 @@ igc_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts) >> >> rxm->data_off = RTE_PKTMBUF_HEADROOM; >> data_len = rte_le_to_cpu_16(rxd.wb.upper.length) - rxq->crc_len; >> + if (rxq->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP) >> + data_len -= IGC_TS_HDR_LEN; >> rxm->data_len = data_len; >> rxm->pkt_len = data_len; >> rxm->nb_segs = 1; >> @@ -509,6 +511,12 @@ igc_recv_scattered_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, >> */ >> rxm->data_off = RTE_PKTMBUF_HEADROOM; >> data_len = rte_le_to_cpu_16(rxd.wb.upper.length); >> + if (rxq->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP) { >> + if (first_seg == NULL) >> + data_len -= IGC_TS_HDR_LEN; >> + else >> + rxm->data_off -= IGC_TS_HDR_LEN; > > This initially confused me, because I was assuming that data_off was a typo > for data_len. However, then I realised on closer examination that, when > timestamp offload is enabled, the actual buffer addresses sent down to the > hardware are offset by IGC_TS_HDR_LEN (meaning the first buffer of a pkt > has the start of data at "normal" data_offset in mbuf, but subsequent > buffers need adjustment). Is my understanding of the issue correct? This is exactly right. Took me a bit to understand what was happening here and I do not know if there might be a better way do this than to change the start offset of all descriptors. But there is probably a reason why it was done this way. Initially the issue was detected as forwarding of packets that maxed out the MTU failed since they now, with the increased length, exceeded the MTU. Only then it became apparent that the scatter-gather handling was also broken. > > In either case, I have two small bits of feedback on this: > * Firstly, I think this needs a comment explaining the logic here, to avoid > others being confused as I was. This will be part of patch v2. > * Secondly, a very minor point, but is the code clearer or shorter, if you > merge this extra code down into the next block which is already checking > for the first_segment of a packet or not? I would actually prefer to keep it that way as not to mix up the the data buffer address and length handling with the mbuf chain construction. > >> + } >> rxm->data_len = data_len; >> >> /* >> @@ -557,6 +565,7 @@ igc_recv_scattered_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, >> last_seg->data_len = last_seg->data_len - >> (RTE_ETHER_CRC_LEN - data_len); >> last_seg->next = NULL; >> + rxm = last_seg; >> } else { >> rxm->data_len = (uint16_t) >> (data_len - RTE_ETHER_CRC_LEN); >> -- >> 2.47.0