Received-SPF: pass (google.com: domain of linux-kernel+bounces-10003-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
From: "Register, Scott" <scott.register@intel.com>
To: "Lobakin, Aleksander" <aleksander.lobakin@intel.com>,
	"intel-wired-lan@lists.osuosl.org" <intel-wired-lan@lists.osuosl.org>
CC: "Fijalkowski, Maciej" <maciej.fijalkowski@intel.com>, "Kitszel,
 Przemyslaw" <przemyslaw.kitszel@intel.com>, "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>, "Lobakin, Aleksander"
	<aleksander.lobakin@intel.com>, Eric Dumazet <edumazet@google.com>, "Kubiak,
 Michal" <michal.kubiak@intel.com>, Simon Horman <horms@kernel.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>, Jakub Kicinski
	<kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, "David S. Miller"
	<davem@davemloft.net>
Subject: RE: [Intel-wired-lan] [PATCH iwl-net v2] idpf: fix corrupted frames
 and skb leaks in singleq mode
Thread-Topic: [Intel-wired-lan] [PATCH iwl-net v2] idpf: fix corrupted frames
 and skb leaks in singleq mode
Thread-Index: AQHaLC5PZQq0fbfFDE2hLm0GJTRZsbC1sm8g
Date: Fri, 22 Dec 2023 18:45:06 +0000
Message-ID: <CY8PR11MB7195FEFA59AF60D8054C04C99794A@CY8PR11MB7195.namprd11.prod.outlook.com>
References: <20231211123144.3759488-1-aleksander.lobakin@intel.com>
In-Reply-To: <20231211123144.3759488-1-aleksander.lobakin@intel.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?jl9HC/FIPdur3GnyhQtl+3vUegmtPsIm9f/FByPjf7gCYtn+BIOBLhGZCeVl?=
 =?us-ascii?Q?CCYuuQcamzYSUkZw37W+MPE6NDNPgLgN7JRuZIsLZuwmAxIeQ927a3w+kihh?=
 =?us-ascii?Q?Fsq4+C6Xd6E+tgrvJ81pQlrwne0hwJwsh7JAUnI4mzo+tAkivVz3MvQw4k3S?=
 =?us-ascii?Q?0sdPVAY4gS9djHqr41tUBYRLDhQrJhfgqYhb8Ze8t0zpjQgFQoF51hoYPyzf?=
 =?us-ascii?Q?mvpI8CfWejEIw8eVBIZsKoqYv4qYqFc9+dlKXfbxIdg9Q9cpQBl+rcWMMC49?=
 =?us-ascii?Q?osU8ys6MzwjvczGkwDRZThlmeY825gsnf82xw0niYY+1kx/po5+p4B9R5uCz?=
 =?us-ascii?Q?WUn6ynFZQCXcK12rMHepmDpjWWx9UkXc4RTModcbmrygbb82XWHvg80AP0HF?=
 =?us-ascii?Q?TAvTlKbXNVtay74JN+Z59JkUDBH06LbP12vC5pa8JNjkYhzPa5VP879hODy3?=
 =?us-ascii?Q?9ts/LSb37LN5y8SqN65jwpoyaHMEMSBxEpVC/BThLSLqxRBZPGvQJulWn8sA?=
 =?us-ascii?Q?kqPY7Fx4xGnHcpgH15aqOY1/Nd3m0ikKWc/bC2XoG12fcygAHskG9rPCd9Up?=
 =?us-ascii?Q?IhF2gpOBy9MwBRQsU/fInKdY3MKk9ov5Wuz9t5iBgu11s1z5IlL91hOpP0RH?=
 =?us-ascii?Q?+UrZtV9LOuJIannbBo6A/y1oDjJL3FtSZkluR1phOqVnCgix37iiok8G94p5?=
 =?us-ascii?Q?pBHDxxW/4IwTxKruASo3g91zDWBTqnXt1EMNArGZDH28yIwTmKyjHXtXMHwV?=
 =?us-ascii?Q?GFU6GbOBGmTZ0enS6BK/p0gxvZ+J9OHP3UgY9yJtQ1PSt/rqhiM0l/WpSWZq?=
 =?us-ascii?Q?Terre8/dyMD+v3j14u7hi2EzBVhbfC8/85wbW9ct5RjRA9cJsDUwDEbn43B+?=
 =?us-ascii?Q?VMx7QrW60PcEGdqh+w+mQrNvanqssfGiJCARUZDcxEi63/sPaSMQolZp2+QL?=
 =?us-ascii?Q?6P4++XUfLXy/I1BRK4KClo+AHYiyIn1PwD+LuIZyGUmbMFUESQBS99uBCI4o?=
 =?us-ascii?Q?j0IFzgf6DwPoDfubKG8B7AcKG4u9bLvD5gtOs4lGZgsLzN1NseOi22iZ5mVu?=
 =?us-ascii?Q?e4Ha0+TyXn9a4nPOn855bF9fc7IPZvEEof9NGMyXeCjl3wQvniCz/OKIZNQx?=
 =?us-ascii?Q?siwxk/14DDte0M04dBEU5cjIoZNQqtke7Fd75uZKCVEFF8yUWRLqVc8sUC11?=
 =?us-ascii?Q?GXCpAxDRnZ0gQssoOyuzp5FlkYsU3AHqYxcM2VLnYJEcYq0uAMTPmWaN+zkU?=
 =?us-ascii?Q?ZPg6jUrIvunu4wwheR1SKHwykMcDAsZxZrXMSPXSwAjecRosB481hHpH4nHz?=
 =?us-ascii?Q?2X7qVare+/fu0fBk7cx0VP2TReyCVHwMne2U5YKHLaA/bBxqmq3xoyhNMpgb?=
 =?us-ascii?Q?gUsr60D5GyREKh9OjW8Kao/XT4G3ZgmOUWPf62JkgdbOhch8EtXK3JTYxZvZ?=
 =?us-ascii?Q?Gx0qYGtastq8zRcRBOHpZv3y7dEsJltLnaQgZG3GtjVWeIFs6aip2rZg0ltv?=
 =?us-ascii?Q?V0LX0C3TGBml6pRONcI2xVMxUGPt9Fo2di2+V7qH6QMwgDCUv4nZXPNYjr4Z?=
 =?us-ascii?Q?5P0KXR2+ONcnHUUu4YmSaBdM0h4qUGWI7t6LnjlRueqCcHSr3Ti6v3mJ5KEL?=
 =?us-ascii?Q?agmE3teCLkTp1zp+t3dE+oCaHaadhb0CaVXDu2teqrIq?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
X-Mailing-List: linux-kernel@vger.kernel.org
List-Id: <linux-kernel.vger.kernel.org>
List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: CY8PR11MB7195.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 213712d9-528e-4b03-17be-08dc031e1e29
X-MS-Exchange-CrossTenant-originalarrivaltime: 22 Dec 2023 18:45:06.9297
 (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: KtOTBpF+rQb0mdLtDQp64U8CUlNr9piqG/wM6sNu5CREpPzJTnE+Wid10sI5I1ilSLY9DLm6AV77ZNgy0727qgHeni604SWli3bnwuFCBMs=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ2PR11MB8401
X-OriginatorOrg: intel.com

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Alexander Lobakin
> Sent: Monday, December 11, 2023 4:32 AM
> To: intel-wired-lan@lists.osuosl.org
> Cc: Fijalkowski, Maciej <maciej.fijalkowski@intel.com>; Kitszel, Przemysl=
aw
> <przemyslaw.kitszel@intel.com>; linux-kernel@vger.kernel.org; Lobakin,
> Aleksander <aleksander.lobakin@intel.com>; Eric Dumazet
> <edumazet@google.com>; Kubiak, Michal <michal.kubiak@intel.com>;
> Simon Horman <horms@kernel.org>; netdev@vger.kernel.org; Jakub
> Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; David S.
> Miller <davem@davemloft.net>
> Subject: [Intel-wired-lan] [PATCH iwl-net v2] idpf: fix corrupted frames =
and
> skb leaks in singleq mode
>=20
> idpf_ring::skb serves only for keeping an incomplete frame between
> several NAPI Rx polling cycles, as one cycle may end up before
> processing the end of packet descriptor. The pointer is taken from
> the ring onto the stack before entering the loop and gets written
> there after the loop exits. When inside the loop, only the onstack
> pointer is used.
> For some reason, the logics is broken in the singleq mode, where the
> pointer is taken from the ring each iteration. This means that if a
> frame got fragmented into several descriptors, each fragment will have
> its own skb, but only the last one will be passed up the stack
> (containing garbage), leaving the rest leaked.
> Then, on ifdown, rxq::skb is being freed only in the splitq mode, while
> it can point to a valid skb in singleq as well. This can lead to a yet
> another skb leak.
> Just don't touch the ring skb field inside the polling loop, letting
> the onstack skb pointer work as expected: build a new skb if it's the
> first frame descriptor and attach a frag otherwise. On ifdown, free
> rxq::skb unconditionally if the pointer is non-NULL.
>=20
> Fixes: a5ab9ee0df0b ("idpf: add singleq start_xmit and napi poll")
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Reviewed-by: Michal Kubiak <michal.kubiak@intel.com>
> Reviewed-by: Simon Horman <horms@kernel.org>
> Reviewed-by: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>
> ---
> Tony, please add it to dev-queue instead of the first revision.
>=20
> From v1[0]:
> * fix the related skb leak on ifdown;
> * fix subject prefix;
> * pick Reviewed-bys.
>=20
> [0] https://lore.kernel.org/all/20231201143821.1091005-1-
> aleksander.lobakin@intel.com
> ---
>  drivers/net/ethernet/intel/idpf/idpf_singleq_txrx.c | 1 -
>  drivers/net/ethernet/intel/idpf/idpf_txrx.c         | 2 +-
>  2 files changed, 1 insertion(+), 2 deletions(-)

Tested-by: Scott Register <scott.register@intel.com>