Received-SPF: pass (google.com: domain of linux-kernel+bounces-175056-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
From: "Singh, Krishneil K" <krishneil.k.singh@intel.com>
To: "Zaremba, Larysa" <larysa.zaremba@intel.com>, "Nguyen, Anthony L"
	<anthony.l.nguyen@intel.com>, "intel-wired-lan@lists.osuosl.org"
	<intel-wired-lan@lists.osuosl.org>, "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>, "netdev@vger.kernel.org"
	<netdev@vger.kernel.org>
CC: "Zaremba, Larysa" <larysa.zaremba@intel.com>, "Brady, Alan"
	<alan.brady@intel.com>, "Fijalkowski, Maciej" <maciej.fijalkowski@intel.com>,
	"Brandeburg, Jesse" <jesse.brandeburg@intel.com>, "Tantilov, Emil S"
	<emil.s.tantilov@intel.com>, "Linga, Pavan Kumar"
	<pavan.kumar.linga@intel.com>, "Hay, Joshua A" <joshua.a.hay@intel.com>,
	"Kitszel, Przemyslaw" <przemyslaw.kitszel@intel.com>, "Bagnucki, Igor"
	<igor.bagnucki@intel.com>
Subject: RE: [PATCH iwl-net] idpf: Interpret .set_channels() input differently
Thread-Topic: [PATCH iwl-net] idpf: Interpret .set_channels() input
 differently
Thread-Index: AQHal/EVpUvpxq7QukW7Rz83X0cLl7GPmKKA
Date: Thu, 9 May 2024 23:01:58 +0000
Message-ID: <MW4PR11MB591177888AFF2E9EFF199E8ABAE62@MW4PR11MB5911.namprd11.prod.outlook.com>
References: <20240426154125.235977-1-larysa.zaremba@intel.com>
In-Reply-To: <20240426154125.235977-1-larysa.zaremba@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?wqCt8myj7s194RrHTGvRsvzIOGXxvgcmizQyYu0ipNb5l6ILdQ8iqFew9/kJ?=
 =?us-ascii?Q?4lYMyWXp0b18yeDs/g2e2WGn9mm3MhqntRLewj1mFJg3hDMTOeUsoX+woTvc?=
 =?us-ascii?Q?q23cWTJDhdHK58HftdBuGIUNJHr44psNV3Tsj4ixxWrCBRLHLAYcSSxGGTBx?=
 =?us-ascii?Q?Yk9wrDF2wphBeQw3V3TF2enh5RzFy4s36LHNar/XFCANjo+RnuPpuR2Jz+u5?=
 =?us-ascii?Q?VdKX6pf0uVoeW2PadrCwqjxDeXHNsw2ddw/SCRuXKl8w2KMO9/tniEETo3xS?=
 =?us-ascii?Q?Jj33RUnZRCdP4fXeePO2lCOQUcTfhkRaMZ25yndrf6JxlcfTmPSX8uIuXlq+?=
 =?us-ascii?Q?fZ7R9t2drD1TyAlHkw2gTxFP6pBRjimwkquFj+8EOJBKnptaFB591bydfQFF?=
 =?us-ascii?Q?4MKQgiuTvkuvfU6EqKC3OaZulrRSbO/cAB+wBTUQyr+8TxgThUxQOhk/Ar+G?=
 =?us-ascii?Q?K/HcS3ue8jCpxavc4uwZGJHroi0KdtPANHIYa/5dQUUpaYog87IquT2zKCsk?=
 =?us-ascii?Q?aywbVXT8zzCRPtpbzqbe5k3dB8x0qdBSnp1IRivfR2OSjpP3rZMBdm4aM5AO?=
 =?us-ascii?Q?EBaEIXyET/0UfFDT8BNegjfoUg1zDvUFUzBLCN3LlNu7iuNXaMOvQ7/jmWvc?=
 =?us-ascii?Q?eNPwSUId4cGdnvGqPRhbpGEhMeEUQ0vGhRZ5ulG23hAMBV9oaCev47bnnJDn?=
 =?us-ascii?Q?tUHEjOct8dFiAVkbcKAkRJ17ZyYUnFMEBTWctvslnJZMN/YBdFaN83KSJWGn?=
 =?us-ascii?Q?8XvNUIvL3iU0/2UbiFuOz4oBGeXdYHN3OXw/S7bAd0Sx5f9xExdX1PclZXD8?=
 =?us-ascii?Q?UmoYon2qkUJMo0vfuN0R3jS8r0IsPAuHPT2SQHpJDPOe0Qe0LYANW4IaKWhh?=
 =?us-ascii?Q?jeRoAhUznn2kErog1cdsP674FtWslx5mHm8UVgPTJ3YNKjaJfWizf0fNQx9l?=
 =?us-ascii?Q?dMmBkEh4i3Ppp43Tds2yX/PUL2yqtNoYmeX3YuyQIAKwtAe8Zp9cwaBNLmkR?=
 =?us-ascii?Q?hv4NxUo1iWJEb/G9ABcDxhBQCNKeCnbsSkWH6ZGNHqJU465fGDja+I01eKVn?=
 =?us-ascii?Q?MYsP5N6NU3B1DePZ85U67C3ogXjkCoIHT+iJoLbHYPht0iYeDHYHd1DZaILL?=
 =?us-ascii?Q?XWxzK1T9lcTcqe3MzNrZrcHFhmG011Q0tkdLC8PDsQ0cTjZb282s8s0VQLzN?=
 =?us-ascii?Q?eJXEhPg554d192of2GgRZey3I22R5yNdzKfirn0B2n+0Noe7cIy6Ip9uFdvm?=
 =?us-ascii?Q?NZ3AAulVz3gBEm8e8iTZdbbXbynSNchOzDAfFnmwc7J98tFTabETN0R81RBR?=
 =?us-ascii?Q?FzcdznrGgpgqhhn5x1tvdeBBWZsxiJTSK/qlQrlN76jx3LVLgp2Dui1NFrub?=
 =?us-ascii?Q?I4TE0rjxk3cjaYBT7MuKElKCZNE9smakelkjSSh23PiHZ1rZCsmS4If0Tecb?=
 =?us-ascii?Q?bPJ9aO45PmXxgZLnSIjA4kBcW1usaupFUwg+evVUPgstk5Ck5rimLR+e5H+B?=
 =?us-ascii?Q?r5M/SZ9Ni0RZTHj7n6keT37abTqPPwy4xpvohhBeNBbl6EfmJzv+AchOogUY?=
 =?us-ascii?Q?LPzyvkBB5RtVSElDJ80Yv5sgFUxhdgJHSLPGkeDtx+wCEBQhattN+jsoj4So?=
 =?us-ascii?Q?yg=3D=3D?=
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: MW4PR11MB5911.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: f9f4cc34-249d-44dd-419a-08dc707c079f
X-MS-Exchange-CrossTenant-originalarrivaltime: 09 May 2024 23:01:58.5665
 (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: md8VV/lr/f3ht5vQuD4efqxxMmknan5rFw1/6eSGXbJFGxILeoT/a4JP2zo/oWUvK1x4JNQOSCEteQKIR0n+f7mPz3gTexHe09FMAndF6g4=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR11MB4706
X-OriginatorOrg: intel.com

> -----Original Message-----
> From: Larysa Zaremba <larysa.zaremba@intel.com>
> Sent: Friday, April 26, 2024 8:41 AM
> To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; intel-wired-
> lan@lists.osuosl.org; linux-kernel@vger.kernel.org; netdev@vger.kernel.or=
g
> Cc: Zaremba, Larysa <larysa.zaremba@intel.com>; Brady, Alan
> <alan.brady@intel.com>; Fijalkowski, Maciej <maciej.fijalkowski@intel.com=
>;
> Brandeburg, Jesse <jesse.brandeburg@intel.com>; Tantilov, Emil S
> <emil.s.tantilov@intel.com>; Linga, Pavan Kumar
> <pavan.kumar.linga@intel.com>; Hay, Joshua A <joshua.a.hay@intel.com>;
> Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>; Bagnucki, Igor
> <igor.bagnucki@intel.com>
> Subject: [PATCH iwl-net] idpf: Interpret .set_channels() input differentl=
y
>=20
> Unlike ice, idpf does not check, if user has requested at least 1 combine=
d
> channel. Instead, it relies on a check in the core code. Unfortunately, t=
he
> check does not trigger for us because of the hacky .set_channels()
> interpretation logic that is not consistent with the core code.
>=20
> This naturally leads to user being able to trigger a crash with an invali=
d
> input. This is how:
>=20
> 1. ethtool -l <IFNAME> -> combined: 40
> 2. ethtool -L <IFNAME> rx 0 tx 0
>    combined number is not specified, so command becomes {rx_count =3D 0,
>    tx_count =3D 0, combined_count =3D 40}.
> 3. ethnl_set_channels checks, if there is at least 1 RX and 1 TX channel,
>    comparing (combined_count + rx_count) and (combined_count + tx_count)
>    to zero. Obviously, (40 + 0) is greater than zero, so the core code
>    deems the input OK.
> 4. idpf interprets `rx 0 tx 0` as 0 channels and tries to proceed with su=
ch
>    configuration.
>=20
> The issue has to be solved fundamentally, as current logic is also known =
to
> cause AF_XDP problems in ice [0].
>=20
> Interpret the command in a way that is more consistent with ethtool
> manual [1] (--show-channels and --set-channels) and new ice logic.
>=20
> Considering that in the idpf driver only the difference between RX and TX
> queues forms dedicated channels, change the correct way to set number of
> channels to:
>=20
> ethtool -L <IFNAME> combined 10 /* For symmetric queues */
> ethtool -L <IFNAME> combined 8 tx 2 rx 0 /* For asymmetric queues */
>=20
> [0] https://lore.kernel.org/netdev/20240418095857.2827-1-
> larysa.zaremba@intel.com/
> [1] https://man7.org/linux/man-pages/man8/ethtool.8.html
>=20
> Fixes: 02cbfba1add5 ("idpf: add ethtool callbacks")
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Reviewed-by: Igor Bagnucki <igor.bagnucki@intel.com>
> Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com>
> ---
>  .../net/ethernet/intel/idpf/idpf_ethtool.c    | 21 ++++++-------------
>  1 file changed, 6 insertions(+), 15 deletions(-)
>=20
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_ethtool.c
> b/drivers/net/ethernet/intel/idpf/idpf_ethtool.c
> index 986d429d1175..1cf3067a9c31 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_ethtool.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_ethtool.c

Tested-by: Krishneil Singh <krishneil.k.singh@intel.com>