Received-SPF: pass (google.com: domain of linux-kernel+bounces-154942-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
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: AQHakpL1PxeMlLmpYEyEiHJJwvzNZLF0TZAAgAFoi2A=
Date: Tue, 23 Apr 2024 11:04:22 +0000
Message-ID: <DM6PR11MB465781953B5600F67C96AA809B112@DM6PR11MB4657.namprd11.prod.outlook.com>
References: <20240419194711.1075349-1-arkadiusz.kubalewski@intel.com>
 <ZiZmpg7GF99Ihxk0@nanopsycho>
In-Reply-To: <ZiZmpg7GF99Ihxk0@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?thlJUYVlrZbCAhIKCwTet1k3aUVf3XQvLKi2VF657r5v5M3mWwkhneooMQn2?=
 =?us-ascii?Q?j6GwT/btaUcEzJn/QMBrtcMxY31T6sdC+bHIhk7rEj+sjI16SPHKtbeblKBl?=
 =?us-ascii?Q?+7Y2L1v9/E4T8GhLziiXqMeLtjHUePg6FTvpT8tbkqby13eOvyY+kZxvWiao?=
 =?us-ascii?Q?45Okp2MwI2DcOWL7Mznbdo28qO7gz6d6AOEvfbD08F6hl0OUy3zdHv9mr+9J?=
 =?us-ascii?Q?kJuqymjlOc7E5hvcYDmluGYUi7IGfEiCs3AP754z0EPrn8gChy6Gw1ZfgSVR?=
 =?us-ascii?Q?/lT67jh+xe5Xa2ef/PKfn8EpBkV20gvdH9kPogjQ6QU5wgYwM08d0j6UR4gf?=
 =?us-ascii?Q?sn6FkXSTGacEfvl9kS7ndl3YU5gFMi6ZE6S9qiWdYp3UxPDgMAsZ1mN91AbP?=
 =?us-ascii?Q?yNllO9mpYByYHeCT8IqMMfDhy5RI8O3KDDv8FY/OMlX3x8eegnW7rl1X5dzX?=
 =?us-ascii?Q?/wd2y+FMSdc1cu9rTl9tukUT2CHh5mQdU6CXXgA9+X9hdZ3BqZHyhmcDIyXa?=
 =?us-ascii?Q?iWOAslRMYD/gO+weBBcK6IVp2IU0t1inpcy6qYP/UB/kWDOqnDFh1oKyTZP3?=
 =?us-ascii?Q?VnOgCwQNZN3ZG6pGRWhm7OwZA9iNXOZ+SWEKqNN0OjvHKBRHDQ6V3AD1RN1w?=
 =?us-ascii?Q?pUFeQZqZuRZm4V7u7EQKldx0Tt1sv7D5STDR2mIKVae4wQhY6GREcmD0PxxM?=
 =?us-ascii?Q?C0pltwhgP9qjicHD0PK55NwkK+TOsDmMSYNUCa/7cM8YKy4F0uXQOmqufuy9?=
 =?us-ascii?Q?A1kfQm6lcmelb7Ckk77UgBpTU062SbtPj7Dz54AJbZ6J2YJ7KCrS0hrgOb+t?=
 =?us-ascii?Q?iJ2sk6nzszdJGYMZAiITbYAkRfPsPOf7KD9csFZO9u3kJ4WBlZmwJ8T+kmy1?=
 =?us-ascii?Q?vc6jcA3MgIoAPMB7rUUZryUQ/zzOOK9GbHbzuMF2TnQVjOjEQiMnc77RzouN?=
 =?us-ascii?Q?icy48h5fr4QPhi/uJgZjmgMh3TYhyEvkmJMFWJgCT79Gg33b1uZOciK6YyIR?=
 =?us-ascii?Q?vKl0vYcPAAWtUGU6XGReAjVLo4on9tKaKltTDIle4gqibMkvdY3OWvt3G3VT?=
 =?us-ascii?Q?ntNy+L1Ka30+qjwAxQR8UTloybviVKZFZKwo1qG+uj+OfW2yXwvLxZkrcc++?=
 =?us-ascii?Q?t1Y8FFNiOXd5GrGCZoUX02ln5kciPUyZ2PYexBKudYWqpV8b1pa5pgio/aHf?=
 =?us-ascii?Q?0lURG2Q0mY1rk7N+BXp8vsprKPvvsq0cg+4oHcqOUYHJ6uq7+ETowEgOT1yO?=
 =?us-ascii?Q?W+yrQJOrlT7GJpBAkVRGm7xAAeDDg/Zj/t3qefqZW9w8pClLQTQ6Zlg6dFMA?=
 =?us-ascii?Q?HH8s5A4jeIDciMS5emDdO1iUIKR/jawVAU6przNgSbkKEt1AP05t0l8YZl6M?=
 =?us-ascii?Q?gO6P+nf4locM2jnRFzz8U4EZeumFsuOmKKdEZHofu2ylUCPFYPlJ1HN+4Apa?=
 =?us-ascii?Q?6AXeN772C3/3sqVgze/e6w5Ht1TsukrzwdRXoeNuFEu+6cwJtaHTotj5yNEC?=
 =?us-ascii?Q?iy6dOTilO1MHfP2VIM9Uh7WMSeeK4i8q+pB2wqHKc1cEYYxm3z0dOxNfybDS?=
 =?us-ascii?Q?FRFKA2CPieP0h7FAhj6MZAVKg/xvhH6JrFHgGmcrPQhJ2tVAZZG1PiX1MmsF?=
 =?us-ascii?Q?7w=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: 1b2da3bc-9d12-49dc-aa1a-08dc6385219e
X-MS-Exchange-CrossTenant-originalarrivaltime: 23 Apr 2024 11:04:22.5537
 (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: hTJSP7GTRMED4syRrPgJ5Zw5akncXMd9eR8it57lXyT4MtWhPxt/rANDzmJChtd6Eazff9enqGt4LiS7Wg2QMKB+pb9zGA2BQVGH2WyURiY=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY8PR11MB7084
X-OriginatorOrg: intel.com

>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 created
>>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 data
>>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 differ=
ent
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 releasing
>>dpll_pin_registration resources.
>>
>>Currently, the following crash/call trace is produced when ice driver is
>>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+0=
x20/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 registra=
tions")
>>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 coo=
kie)
>
>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.

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?

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, struct
>>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, struct
>>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
>>