Received-SPF: pass (google.com: domain of linux-kernel+bounces-139427-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <22153526-f764-420e-84f1-851006c67e76@intel.com>
Date: Wed, 10 Apr 2024 15:20:15 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH net v2] net: sparx5: flower: fix fragment flags handling
To: =?UTF-8?Q?Asbj=C3=B8rn_Sloth_T=C3=B8nnesen?= <ast@fiberby.net>,
	<netdev@vger.kernel.org>
CC: Steen Hegelund <Steen.Hegelund@microchip.com>, Lars Povlsen
	<lars.povlsen@microchip.com>, Daniel Machon <daniel.machon@microchip.com>,
	<UNGLinuxDriver@microchip.com>, "David S. Miller" <davem@davemloft.net>,
	"Eric Dumazet" <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo
 Abeni <pabeni@redhat.com>, <linux-kernel@vger.kernel.org>
References: <20240410095224.6372-1-ast@fiberby.net>
Content-Language: en-US
From: Jacob Keller <jacob.e.keller@intel.com>
In-Reply-To: <20240410095224.6372-1-ast@fiberby.net>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?RklDeXQvK3RzamdNNkwyMll1aVEwaERlRTF5azQ1TitYREZqTVdVUktBL004?=
 =?utf-8?B?SEp5S3kzUTNwUTlRdGxyVEVyODM4eGMraVJNUTg1MG9OR3ZsSisrUHhBbDMy?=
 =?utf-8?B?cmUvWlBhZklGWFNhTGR3OE5iYWtrcjJOV3k5aHd2U2NsdW9iMkR6NlU3Umdt?=
 =?utf-8?B?eCtnb0ZmRGhEZHRxSVBLUlFqNjFZQTVxYVgwSmdEcEVQRFdNV3Bjcnc2NEJC?=
 =?utf-8?B?N0NGMTJKdmw5a1NPNEdKeXR2QnFIUzBTWExrQkRubkJGUEhtS3h5SDlCc3Vw?=
 =?utf-8?B?K2xWcFV2YjROUlV5cW95MW4zZnduWHZpVTA3bHZUZDl1cG1mTWdrOEo3VDl1?=
 =?utf-8?B?c3lmNDJKYnNIdFVCdUhjTWc2cVR1ZWVHa2EwWGRQaXQrMlhNZmh2WXNsOTlk?=
 =?utf-8?B?L3hoamQycElweGJGaWNDSmFuN0trekNEcUVxaDIyRUdkUUZvNnhyNVpzRDd6?=
 =?utf-8?B?c3doVWwwTEZSRUdrQ3dSbW1XR1htdFhEbFVycmROSHhFUlo1ZTM3bXJjcEsr?=
 =?utf-8?B?azlidHQvT0NMN3RmSCtySG1BbCtadXZwa0JoNWlkL091SW45d1RzS0hUSWhw?=
 =?utf-8?B?em5vZjRaMmN6YkIxRldmdUZBNHZ6S2dFTmpEaFZqVlR4TEdLYlJ1ZlhjRzJW?=
 =?utf-8?B?QnJJZUM3UDVNd3M0V0xkZS82MW04V0FRNGhTaGVRT2VMSU9ZYjhVWUFGbytJ?=
 =?utf-8?B?R2xDbzB0SldXQkV5bklaalQyZ1BBbTRLMXRQRDBVNTRBdGRpclRFdkNTbFdr?=
 =?utf-8?B?TDUwME9EbGRLMExlREkwM0o5UzkwRU9qTXpmSTM2WFpNYkVOaDVuV3Z5YjRy?=
 =?utf-8?B?aitCaUlNVTlYcFVKMmJod1JCVWJMUXQ1azdYQ3c4MHZDdHJZKzlsanRjSGdu?=
 =?utf-8?B?UU40dnV0Ym44MUJiNnBUR1p3dE0zVG82Tmt1Y2FJd3hlYzBFeFl1cXFoZ08y?=
 =?utf-8?B?YkVueC9qMFlLaHpkV3BWaDQzbW1GNHlXWlEzQS8vcmQ0dGlkTis5YjlJOFg0?=
 =?utf-8?B?UXVJS1M3UVVOZHM2b2hyK3RiWHphY1ZOazBuQ3RIR0FGSUNQNi9rMGs3SUJy?=
 =?utf-8?B?MHNEVk0xUmN3L0V6dFViQjFDRjBxeENMKzZ0ZGFrKzM4c2NuTjZiTWtCQ0gx?=
 =?utf-8?B?SWluM1htZC9sdzZZd3pnUDlTR3dGeXc0YTREN3daT012aVhDYXU0RWZnazRm?=
 =?utf-8?B?elA2MWNucllaVXFsUXVMR2NCYjlDWDZjT3hSVEVJeU0rTXd2MVlaTFV0V2c0?=
 =?utf-8?B?UFl3RWJMVnYxWWJ3T0ZlMkhEbFJFcGhxclBvUmhLUUVHWmRkSHhUcytLSzVi?=
 =?utf-8?B?R0NwSUY0UFI1TnYzUHZDZ1IxRmtEbEgvVHBUYU5KVDREYTJOa3UzK2loUGdx?=
 =?utf-8?B?VisrQTZFbURvSEtmZ01Zc2E4Sk1ocHByRFE1azlqMWQ3Vmd1cUxqU3pIVHJv?=
 =?utf-8?B?cWRGRkhpUVFIQ1pCT2JqaWhwR3J6UzFQejMwQWZRcXVCNFZTb29QKzRJOUY4?=
 =?utf-8?B?b0dtZ2M0anhsUi95c1VveCtFVGNrV1Nmb0RtS1ZISHJGSE1DblNMSFVCdVN6?=
 =?utf-8?B?N1lwU09LdnFibDBUYitCejZMRHl0ZThLbnZPS3VVYzRsVGxtWFdpWnRWNWl1?=
 =?utf-8?B?ZzFIWkxtcHVCaE9BRlBkY1huSWZFVG8yMGpYWDd0bmVlSUFEcGJnWE0wWGhP?=
 =?utf-8?B?eDFCaWd2cm9ZVkZCazI2N0wwc2FqSHNoMUpUL2xBZFBOYUNaUmYxZU5WWGt5?=
 =?utf-8?B?dDQ0c0NCT1BrYXVscHk0b2dLVGxCRU1nWVdySTlrYytmbDJRR2FGODhPRFBw?=
 =?utf-8?B?SmhWUjM1cHQxS0NaMGNVOVgyQlo0NUVjSk9EQlBiWXVjODIrYUREYXRZQ0VW?=
 =?utf-8?B?dGJuNnIyOXprblVLU0d2WThKbjdzZ2U2ZFFwekRJQ1JBZEZZODhsMG1GTVF3?=
 =?utf-8?B?T0Q3NFpyR0ZqOVRibFJ0bHNUelZzbW9EdzNOazJqZzNsSjRCT3hVanVVTEQ4?=
 =?utf-8?B?ZjNLVkh2NVJQR2l5azgzQ1gwUFFDV01FeU8vdDdUS1pBWVY0Rk9oRjR1dkxq?=
 =?utf-8?B?Vm96UGxkd2hmSHNvdW4xdjAyY3Rlb3R2eTUxTk9Hd2dnSXlsSllsNzFxWEVG?=
 =?utf-8?B?WDVmV2tHQ002enBvZ296SWJ5ZlE4bWFTTVRnWU5kaVdQVmFPMFNqR0hrbWxn?=
 =?utf-8?B?bFE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: c7267e43-9fdf-4ba7-8b85-08dc59ac6670
X-MS-Exchange-CrossTenant-AuthSource: CO1PR11MB5089.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Apr 2024 22:20:16.8247
 (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: EbYyZ29/jiX3dxjP0j4tLSn0POdYLpCVhFwBuLAK20DYdhCY8e+m0oLsLc/GA18izC/kyWC3tTo2XfYx0p2GS5Qi2mO5MEcgtBCICUn0rj0=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB6349
X-OriginatorOrg: intel.com


On 4/10/2024 2:52 AM, Asbjørn Sloth Tønnesen wrote:
> I noticed that only 3 out of the 4 input bits were used,
> mt.key->flags & FLOW_DIS_IS_FRAGMENT was never checked.
> 
> In order to avoid a complicated maze, I converted it to
> use a 16 byte mapping table.
> 
> As shown in the table below the old heuristics doesn't
> always do the right thing, ie. when FLOW_DIS_IS_FRAGMENT=1/1
> then it used to only match follow-up fragment packets.
> 
> Here are all the combinations, and their resulting new/old
> VCAP key/mask filter:
> 
>   /- FLOW_DIS_IS_FRAGMENT (key/mask)
>   |    /- FLOW_DIS_FIRST_FRAG (key/mask)
>   |    |    /-- new VCAP fragment (key/mask)
>   v    v    v    v- old VCAP fragment (key/mask)
> 
>  0/0  0/0  -/-  -/-     impossible (due to entry cond. on mask)
>  0/0  0/1  -/-  0/3 !!  invalid (can't match non-fragment + follow-up frag)
>  0/0  1/0  -/-  -/-     impossible (key > mask)
>  0/0  1/1  1/3  1/3     first fragment
> 
>  0/1  0/0  0/3  3/3 !!  not fragmented
>  0/1  0/1  0/3  3/3 !!  not fragmented (+ not first fragment)
>  0/1  1/0  -/-  -/-     impossible (key > mask)
>  0/1  1/1  -/-  1/3 !!  invalid (non-fragment and first frag)
> 
>  1/0  0/0  -/-  -/-     impossible (key > mask)
>  1/0  0/1  -/-  -/-     impossible (key > mask)
>  1/0  1/0  -/-  -/-     impossible (key > mask)
>  1/0  1/1  -/-  -/-     impossible (key > mask)
> 
>  1/1  0/0  1/1  3/3 !!  some fragment
>  1/1  0/1  3/3  3/3     follow-up fragment
>  1/1  1/0  -/-  -/-     impossible (key > mask)
>  1/1  1/1  1/3  1/3     first fragment
> 
> In the datasheet the VCAP fragment values are documented as:
>  0 = no fragment
>  1 = initial fragment
>  2 = suspicious fragment
>  3 = valid follow-up fragment
> 
> Result: 3 combinations match the old behavior,
>         3 combinations have been corrected,
>         2 combinations are now invalid, and fail,
>         8 combinations are impossible.
> 

Appreciate the detailed analysis with the lookup table. This is a bit
more opaque but a maze of combinations would be even less readable and
maintainable.

Makes sense.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>