Received-SPF: pass (google.com: domain of linux-kernel+bounces-156722-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: "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@intel.com>
To: Jiri Pirko <jiri@resnulli.us>
CC: "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"vadim.fedorenko@linux.dev" <vadim.fedorenko@linux.dev>,
	"davem@davemloft.net" <davem@davemloft.net>, "rrameshbabu@nvidia.com"
	<rrameshbabu@nvidia.com>, "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>, "pabeni@redhat.com" <pabeni@redhat.com>,
	"kuba@kernel.org" <kuba@kernel.org>, mschmidt <mschmidt@redhat.com>,
	"Kitszel, Przemyslaw" <przemyslaw.kitszel@intel.com>
Subject: RE: [PATCH net] dpll: fix dpll_pin_registration missing refcount
Thread-Topic: [PATCH net] dpll: fix dpll_pin_registration missing refcount
Thread-Index: AQHakpL1PxeMlLmpYEyEiHJJwvzNZLF0TZAAgAFoi2CAAAQgAIABguYA
Date: Wed, 24 Apr 2024 10:26:11 +0000
Message-ID: <DM6PR11MB46573DA3B0B44BAA25F2CF759B102@DM6PR11MB4657.namprd11.prod.outlook.com>
References: <20240419194711.1075349-1-arkadiusz.kubalewski@intel.com>
 <ZiZmpg7GF99Ihxk0@nanopsycho>
 <DM6PR11MB465781953B5600F67C96AA809B112@DM6PR11MB4657.namprd11.prod.outlook.com>
 <ZieYjuoRl61WCVZg@nanopsycho>
In-Reply-To: <ZieYjuoRl61WCVZg@nanopsycho>
Accept-Language: pl-PL, en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?7p73h/dbQUT70jcE3hGW8GOuXVCYwgn1t/rLZxH46YIguDdMRsW2Mu218HbR?=
 =?us-ascii?Q?QQ6MsssRAd7/19CAJrmRLyX4HN5MTgz0lLweClm7a6HWG0RvYvid91/eP1w7?=
 =?us-ascii?Q?Erzxp9jidSu2uxp3gaGI1ALf3/wrpfwPd+4Idq1Jho7QID8dI2IOVVKt5IZw?=
 =?us-ascii?Q?LnDLRueFVmnsGlaUFXI5cQutfkJZjtWQXLGk0oBLQ6lvheumWjOJXiOxVBf0?=
 =?us-ascii?Q?EO1rteZLCOPsRa0dqNFUk6BwQLexBvJSNk6HH4DFl0Qr/RPUee6+rU8uwg2e?=
 =?us-ascii?Q?YYtGZti9l3pwfAO7RCC/fto4MUupLX5mPlcMb5pLXrtiVnW37F9Zqjg++ZQF?=
 =?us-ascii?Q?sziKeWQdbGo+e8xHLXPXExHNjxpC6PtMCtzRedIeMFSnRSmyWl5NmVQMU5pd?=
 =?us-ascii?Q?a19GykEqiSURfBjmcN9nIK9VhMMpvDGN4QQCw8Zb8heUpVCkGbbjHeQ/j5uN?=
 =?us-ascii?Q?WlDXRXv6Lx6MNT28TuZ8G3ik8bk/MqGAnr5BSSt7LxabQuXyWF1eFsdBjS1q?=
 =?us-ascii?Q?TaYYAGgMQCRv1UVSTgl2oqtxG8lzJ9JBmOO3YA20UfZF1rGc2fT89zlG9JKI?=
 =?us-ascii?Q?NUNCOiTx0rP8zm4Lnlo/axjPemC+hkk8+K2DgdVMirmH1oeUyuMWMDjn9gSL?=
 =?us-ascii?Q?Vw5AMMqvPraaiIS/QmpRmOXw7ApaMEHcKRNaoAzGFE1CojWW/A3/hXqOHfLH?=
 =?us-ascii?Q?6r+G3nWFbAHm/DSxXmHoC8aPHH6NGjEr6diTegu7eR0h3Z3JOElfOCgxUXzb?=
 =?us-ascii?Q?YiL/A181XEviDBh0/RzzylgL093WW4pwly4rWFwy3n9GwSVpCx022xiqMj3J?=
 =?us-ascii?Q?ZxN9770dHtpZ0k2lvgT7iorTLYXqDfpeo9EqKS7vhFi5Fxv0sIARxLYGeEmy?=
 =?us-ascii?Q?quxpoia5/u+7fXn+UqxEcTI8GLcePk9RGE8vajaGJ+yZkQkBSdypmfGglqRc?=
 =?us-ascii?Q?U5LtRULAQO9ZXWM51DoLVcocCxOQeusmYbBbVZU2xyeG1TUcagwQM6CEMyDF?=
 =?us-ascii?Q?QROZBtIdnML31xu0YcmmH6mMJkEreXmiVzQMcEmAMrUwwgN5eH0pN/h/AEW7?=
 =?us-ascii?Q?MLIGRGNGipMewUxFv5XBaqw0RE55oTc0THxTjO3/VdI3y8gZ6bZJEmAqfA3E?=
 =?us-ascii?Q?w+flz/XMS9ztroQTfK0CXr7IsFW4Z73yowcCD+sVNgLahUijvYzv6aSf6zSh?=
 =?us-ascii?Q?Vu4pQoZnd/BD+CW8grlvuWsJDzoriDYFApfxExCjACd8ClkTbi71I9dtPeno?=
 =?us-ascii?Q?qN/wy+sAwbH6RXiiaZG70Ggn1Zu9Jl9N0ZZioZSPOdwvpCSXXPbgX1AgqGFA?=
 =?us-ascii?Q?VpFnn/7f44Wzp0O2ORWqQHKDHN7Rw4Pw7j73fW64M5dzEWueLUe9mlEM9Fv3?=
 =?us-ascii?Q?S2+a0yUa4un4fQ/oFUKftk2Bjv45eDDGt1vWddY1t4RhAAExnTZfvpqpL8nF?=
 =?us-ascii?Q?fxEypoU8PFRcWLZkgN1neJdJlkx8+5Q70EGXsHh1VMveMnZ9FxyYROWb0fw2?=
 =?us-ascii?Q?yPbV8D59ZTs1i6DQK5v0qZF1Y5w9Ps2yff/RmRoQbl0D361Xql1S0NlRuOsd?=
 =?us-ascii?Q?nLElJqSNwtl1AxEXfyeK9pP+rthQPxW/RxwpUDjvYQMtnw2ngXP0CZd05aIP?=
 =?us-ascii?Q?lQ=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: DM6PR11MB4657.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 7ced1df6-d2b4-4d11-7a70-08dc6448f6cb
X-MS-Exchange-CrossTenant-originalarrivaltime: 24 Apr 2024 10:26:12.0504
 (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: iuGvlr4uxxG1XuGYpsUXEF56lU/948BqhzZFibb9v2AwMt6RdGrikz3A8bCLeYMOOTyY+YsGg8MY53muVX0kA4/RfkqGMxs583pgAStp0Ak=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB6813
X-OriginatorOrg: intel.com

>From: Jiri Pirko <jiri@resnulli.us>
>Sent: Tuesday, April 23, 2024 1:17 PM
>
>Tue, Apr 23, 2024 at 01:04:22PM CEST, arkadiusz.kubalewski@intel.com wrote=
:
>>>From: Jiri Pirko <jiri@resnulli.us>
>>>Sent: Monday, April 22, 2024 3:31 PM
>>>
>>>Fri, Apr 19, 2024 at 09:47:11PM CEST, arkadiusz.kubalewski@intel.com
>>>wrote:
>>>>In scenario where pin is registered with multiple parent pins via
>>>>dpll_pin_on_pin_register(..), belonging to the same dpll device,
>>>>and each time with the same set of ops/priv data, a reference
>>>>between a pin and dpll is created once and then refcounted, at the same
>>>>time the dpll_pin_registration is only checked for existence and create=
d
>>>>if does not exist. This is wrong, as for the same ops/priv data a
>>>>registration shall be also refcounted, a child pin is also registered
>>>>with dpll device, until each child is unregistered the registration dat=
a
>>>>shall exist.
>>>
>>>I read this 3 time, don't undestand clearly the matter of the problem.
>>>Could you perhaps make it somehow visual?
>>>
>>
>>Many thanks for all your insights on this!
>>
>>Register child pin twice (via dpll_pin_on_pin_register(..)) with two
>>different
>>parents but the same ops/priv. Then, a single
>>dpll_pin_on_pin_unregister(..) will
>>cause below stack trace.
>>
>>It was good to add a fix in b446631f355e, but the fix did not cover a
>>multi-parent
>>registration case, here I am fixing it.
>>
>>>
>>>>
>>>>Add refcount and check if all registrations are dropped before releasin=
g
>>>>dpll_pin_registration resources.
>>>>
>>>>Currently, the following crash/call trace is produced when ice driver i=
s
>>>>removed on the system with installed NIC which includes dpll device:
>>>>
>>>>WARNING: CPU: 51 PID: 9155 at drivers/dpll/dpll_core.c:809
>>>>dpll_pin_ops+0x20/0x30
>>>>Call Trace:
>>>> dpll_msg_add_pin_freq+0x37/0x1d0
>>>> dpll_cmd_pin_get_one+0x1c0/0x400
>>>> ? __nlmsg_put+0x63/0x80
>>>> dpll_pin_event_send+0x93/0x140
>>>> dpll_pin_on_pin_unregister+0x3f/0x100
>>>> ice_dpll_deinit_pins+0xa1/0x230 [ice]
>>>> ice_remove+0xf1/0x210 [ice]
>>>>
>>>>Fixes: b446631f355e ("dpll: fix dpll_xa_ref_*_del() for multiple
>>>>registrations")
>>>>Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>>>>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>>>>---
>>>> drivers/dpll/dpll_core.c | 17 +++++++++++++----
>>>> 1 file changed, 13 insertions(+), 4 deletions(-)
>>>>
>>>>diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
>>>>index 64eaca80d736..7ababa327c0c 100644
>>>>--- a/drivers/dpll/dpll_core.c
>>>>+++ b/drivers/dpll/dpll_core.c
>>>>@@ -40,6 +40,7 @@ struct dpll_device_registration {
>>>>
>>>> struct dpll_pin_registration {
>>>> 	struct list_head list;
>>>>+	refcount_t refcount;
>>>> 	const struct dpll_pin_ops *ops;
>>>> 	void *priv;
>>>> };
>>>>@@ -81,6 +82,7 @@ dpll_xa_ref_pin_add(struct xarray *xa_pins, struct
>>>>dpll_pin *pin,
>>>> 		reg =3D dpll_pin_registration_find(ref, ops, priv);
>>>> 		if (reg) {
>>>> 			refcount_inc(&ref->refcount);
>>>>+			refcount_inc(&reg->refcount);
>>>
>>>I don't like this. Registration is supposed to be created for a single
>>>registration. Not you create one for many and refcount it.
>>>
>>
>>If register function is called with the same priv/ops, why to do all you
>>suggested below instead of just refcounting?
>>
>>>Instead of this, I suggest to extend __dpll_pin_register() for a
>>>"void *cookie" arg. That would be NULL for dpll_pin_register() caller.
>>>For dpll_pin_on_pin_register() caller, it would pass "parent" pointer.
>>>
>>>Than dpll_xa_ref_pin_add() can pass this cookie value to
>>>dpll_pin_registration_find(). The if case there would look like:
>>>if (reg->ops =3D=3D ops && reg->priv =3D=3D priv && reg->cookie =3D=3D c=
ookie)
>>>
>>>This way, we will create separate "sub-registration" for each parent.
>>>
>>>Makes sense?
>>>
>>
>>It would do, but only if the code would anyhow use that new parent
>>sub-registration explicitly for anything else later.
>>
>>Creating a sub-registration with additional parent cookie just to create =
a
>>second registration with only difference parent cookie and not using the
>>cookie even once after, seems overshot for a fix.
>
>Well, we have ref with multiple references and refcount, single
>registration instance per registration. Now you make that multiple
>references and refcounted as well, just because the parent is different.
>That is why I suggested to add the parent to the registration look-up
>if. Makes things a bit cleaner to read in already quite complex code.
>
>
>>
>>What you suggest is rather a refactor, but again needed only after we
>>would
>>make use of the parent cooking somewhere else.
>>And such refactor shall target next-tree, right?
>
>Not sure what refactor you refer to. Couple of lines, similar to your
>version.
>

Just sent v2 with your proposal, please take a look.

Thank you!
Arkadiusz.

>
>>
>>Thank you!
>>Arkadiusz
>>
>>>> 			return 0;
>>>> 		}
>>>> 		ref_exists =3D true;
>>>>@@ -113,6 +115,7 @@ dpll_xa_ref_pin_add(struct xarray *xa_pins, struct
>>>>dpll_pin *pin,
>>>> 	reg->priv =3D priv;
>>>> 	if (ref_exists)
>>>> 		refcount_inc(&ref->refcount);
>>>>+	refcount_set(&reg->refcount, 1);
>>>> 	list_add_tail(&reg->list, &ref->registration_list);
>>>>
>>>> 	return 0;
>>>>@@ -131,8 +134,10 @@ static int dpll_xa_ref_pin_del(struct xarray
>>>>*xa_pins, struct dpll_pin *pin,
>>>> 		reg =3D dpll_pin_registration_find(ref, ops, priv);
>>>> 		if (WARN_ON(!reg))
>>>> 			return -EINVAL;
>>>>-		list_del(&reg->list);
>>>>-		kfree(reg);
>>>>+		if (refcount_dec_and_test(&reg->refcount)) {
>>>>+			list_del(&reg->list);
>>>>+			kfree(reg);
>>>>+		}
>>>> 		if (refcount_dec_and_test(&ref->refcount)) {
>>>> 			xa_erase(xa_pins, i);
>>>> 			WARN_ON(!list_empty(&ref->registration_list));
>>>>@@ -160,6 +165,7 @@ dpll_xa_ref_dpll_add(struct xarray *xa_dplls, struc=
t
>>>>dpll_device *dpll,
>>>> 		reg =3D dpll_pin_registration_find(ref, ops, priv);
>>>> 		if (reg) {
>>>> 			refcount_inc(&ref->refcount);
>>>>+			refcount_inc(&reg->refcount);
>>>> 			return 0;
>>>> 		}
>>>> 		ref_exists =3D true;
>>>>@@ -192,6 +198,7 @@ dpll_xa_ref_dpll_add(struct xarray *xa_dplls, struc=
t
>>>>dpll_device *dpll,
>>>> 	reg->priv =3D priv;
>>>> 	if (ref_exists)
>>>> 		refcount_inc(&ref->refcount);
>>>>+	refcount_set(&reg->refcount, 1);
>>>> 	list_add_tail(&reg->list, &ref->registration_list);
>>>>
>>>> 	return 0;
>>>>@@ -211,8 +218,10 @@ dpll_xa_ref_dpll_del(struct xarray *xa_dplls,
>>>>struct
>>>>dpll_device *dpll,
>>>> 		reg =3D dpll_pin_registration_find(ref, ops, priv);
>>>> 		if (WARN_ON(!reg))
>>>> 			return;
>>>>-		list_del(&reg->list);
>>>>-		kfree(reg);
>>>>+		if (refcount_dec_and_test(&reg->refcount)) {
>>>>+			list_del(&reg->list);
>>>>+			kfree(reg);
>>>>+		}
>>>> 		if (refcount_dec_and_test(&ref->refcount)) {
>>>> 			xa_erase(xa_dplls, i);
>>>> 			WARN_ON(!list_empty(&ref->registration_list));
>>>>
>>>>base-commit: ac1a21db32eda8a09076bad025d7b848dd086d28
>>>>--
>>>>2.38.1
>>>>