Re: [PATCH net] dpll: fix dpll_pin_registration missing refcount

[Jiri Pirko]

Fri, Apr 19, 2024 at 09:47:11PM CEST, arkadiusz.kubalewski@xxxxxxxxx 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?


>
>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+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@xxxxxxxxx>
>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@xxxxxxxxx>
>---
> 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 = 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.

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 == ops && reg->priv == priv && reg->cookie == cookie)

This way, we will create separate "sub-registration" for each parent.

Makes sense?

> 			return 0;
> 		}
> 		ref_exists = true;
>@@ -113,6 +115,7 @@ dpll_xa_ref_pin_add(struct xarray *xa_pins, struct dpll_pin *pin,
> 	reg->priv = 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 = 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 = dpll_pin_registration_find(ref, ops, priv);
> 		if (reg) {
> 			refcount_inc(&ref->refcount);
>+			refcount_inc(&reg->refcount);
> 			return 0;
> 		}
> 		ref_exists = true;
>@@ -192,6 +198,7 @@ dpll_xa_ref_dpll_add(struct xarray *xa_dplls, struct dpll_device *dpll,
> 	reg->priv = 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 = 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
>