Subject: Re: [PATCH net 2/3] dpll: fix pin dump crash for rebound module

From: Jiri Pirko <[email-masked]>
Date: Fri, 10 Nov 2023 07:45:41 +0100

Fri, Nov 10, 2023 at 12:32:21AM CET, [email-masked] wrote:
>>From: Jiri Pirko <[email-masked]>
>>Sent: Thursday, November 9, 2023 7:06 PM
>>
>>Thu, Nov 09, 2023 at 05:30:20PM CET, [email-masked] wrote:
>>>>From: Jiri Pirko <[email-masked]>
>>>>Sent: Thursday, November 9, 2023 2:19 PM
>>>>
>>>>Thu, Nov 09, 2023 at 01:20:48PM CET, [email-masked]
>>>>wrote:
>>>>>>From: Jiri Pirko <[email-masked]>
>>>>>>Sent: Wednesday, November 8, 2023 3:30 PM
>>>>>>
>>>>>>Wed, Nov 08, 2023 at 11:32:25AM CET, [email-masked]
>>>>>>wrote:
>>>>>>>When a kernel module is unbound but the pin resources were not
>>>>>>>entirely
>>>>>>>freed (other kernel module instance have had kept the reference to
>>>>>>>that
>>>>>>>pin), and kernel module is again bound, the pin properties would not
>>>>>>>be
>>>>>>>updated (the properties are only assigned when memory for the pin is
>>>>>>>allocated), prop pointer still points to the kernel module memory of
>>>>>>>the kernel module which was deallocated on the unbind.
>>>>>>>
>>>>>>>If the pin dump is invoked in this state, the result is a kernel
>>>>>>>crash.
>>>>>>>Prevent the crash by storing persistent pin properties in dpll
>>>>>>>subsystem,
>>>>>>>copy the content from the kernel module when pin is allocated, instead
>>>>>>>of
>>>>>>>using memory of the kernel module.
>>>>>>>
>>>>>>>Fixes: 9431063ad323 ("dpll: core: Add DPLL framework base functions")
>>>>>>>Fixes: 9d71b54b65b1 ("dpll: netlink: Add DPLL framework base
>>>>>>>functions")
>>>>>>>Signed-off-by: Arkadiusz Kubalewski <[email-masked]>
>>>>>>>---
>>>>>>> drivers/dpll/dpll_core.c    |  4 ++--
>>>>>>> drivers/dpll/dpll_core.h    |  4 ++--
>>>>>>> drivers/dpll/dpll_netlink.c | 28 ++++++++++++++--------------
>>>>>>> 3 files changed, 18 insertions(+), 18 deletions(-)
>>>>>>>
>>>>>>>diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
>>>>>>>index 3568149b9562..4077b562ba3b 100644
>>>>>>>--- a/drivers/dpll/dpll_core.c
>>>>>>>+++ b/drivers/dpll/dpll_core.c
>>>>>>>@@ -442,7 +442,7 @@ dpll_pin_alloc(u64 clock_id, u32 pin_idx, struct
>>>>>>>module *module,
>>>>>>> 		ret = -EINVAL;
>>>>>>> 		goto err;
>>>>>>> 	}
>>>>>>>-	pin->prop = prop;
>>>>>>>+	memcpy(&pin->prop, prop, sizeof(pin->prop));
>>>>>>
>>>>>>Odd, you don't care about the pointer within this structure?
>>>>>>
>>>>>
>>>>>Well, true. Need a fix.
>>>>>Wondering if copying idea is better than just assigning prop pointer on
>>>>>each call to dpll_pin_get(..) function (when pin already exists)?
>>>>
>>>>Not sure what do you mean. Examples please.
>>>>
>>>
>>>Sure,
>>>
>>>Basically this change:
>>>
>>>diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
>>>index ae884b92d68c..06b72d5877c3 100644
>>>--- a/drivers/dpll/dpll_core.c
>>>+++ b/drivers/dpll/dpll_core.c
>>>@@ -483,6 +483,7 @@ dpll_pin_get(u64 clock_id, u32 pin_idx, struct module
>>>*module,
>>>                    pos->pin_idx == pin_idx &&
>>>                    pos->module == module) {
>>>                        ret = pos;
>>>+                       pos->prop = prop;
>>>                        refcount_inc(&ret->refcount);
>>>                        break;
>>>                }
>>>
>>>would replace whole of this patch changes, although seems a bit hacky.
>>
>>Or event better, as I suggested in the other patch reply, resolve this
>>internally in the driver registering things only when they are valid.
>>Much better then to hack anything in dpll core.
>>
>
>This approach seemed to me hacky, that is why started with coping the
>data.
>It is not about registering, rather about unregistering on driver
>unbind, which brakes things, and currently cannot be recovered in
>described case.

Sure it can. PF0 unbind-> internal notification-> PF1 unregisters all
related object. Very clean and simple.

>
>Thank you!
>Arkadiusz
>
>>
>>>
>>>Thank you!
>>>Arkadiusz
>>>
>>>>
>>>>>
>>>>>Thank you!
>>>>>Arkadiusz
>>>>>
>>>>>>
>>>>>>> 	refcount_set(&pin->refcount, 1);
>>>>>>> 	xa_init_flags(&pin->dpll_refs, XA_FLAGS_ALLOC);
>>>>>>> 	xa_init_flags(&pin->parent_refs, XA_FLAGS_ALLOC);
>>>>>>>@@ -634,7 +634,7 @@ int dpll_pin_on_pin_register(struct dpll_pin
>>>>>>>*parent,
>>>>>>>struct dpll_pin *pin,
>>>>>>> 	unsigned long i, stop;
>>>>>>> 	int ret;
>>>>>>>
>>>>>>>-	if (WARN_ON(parent->prop->type != DPLL_PIN_TYPE_MUX))
>>>>>>>+	if (WARN_ON(parent->prop.type != DPLL_PIN_TYPE_MUX))
>>>>>>> 		return -EINVAL;
>>>>>>>
>>>>>>> 	if (WARN_ON(!ops) ||
>>>>>>>diff --git a/drivers/dpll/dpll_core.h b/drivers/dpll/dpll_core.h
>>>>>>>index 5585873c5c1b..717f715015c7 100644
>>>>>>>--- a/drivers/dpll/dpll_core.h
>>>>>>>+++ b/drivers/dpll/dpll_core.h
>>>>>>>@@ -44,7 +44,7 @@ struct dpll_device {
>>>>>>>  * @module:		module of creator
>>>>>>>  * @dpll_refs:		hold referencees to dplls pin was registered
>>>>>>>with
>>>>>>>  * @parent_refs:	hold references to parent pins pin was registered
>>>>>>>with
>>>>>>>- * @prop:		pointer to pin properties given by registerer
>>>>>>>+ * @prop:		pin properties copied from the registerer
>>>>>>>  * @rclk_dev_name:	holds name of device when pin can recover
>>>>>>>clock
>>>>>>>from it
>>>>>>>  * @refcount:		refcount
>>>>>>>  **/
>>>>>>>@@ -55,7 +55,7 @@ struct dpll_pin {
>>>>>>> 	struct module *module;
>>>>>>> 	struct xarray dpll_refs;
>>>>>>> 	struct xarray parent_refs;
>>>>>>>-	const struct dpll_pin_properties *prop;
>>>>>>>+	struct dpll_pin_properties prop;
>>>>>>> 	refcount_t refcount;
>>>>>>> };
>>>>>>>
>>>>>>>diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
>>>>>>>index 93fc6c4b8a78..963bbbbe6660 100644
>>>>>>>--- a/drivers/dpll/dpll_netlink.c
>>>>>>>+++ b/drivers/dpll/dpll_netlink.c
>>>>>>>@@ -278,17 +278,17 @@ dpll_msg_add_pin_freq(struct sk_buff *msg,
>>>>>>>struct
>>>>>>>dpll_pin *pin,
>>>>>>> 	if (nla_put_64bit(msg, DPLL_A_PIN_FREQUENCY, sizeof(freq),
>>>>>>>&freq,
>>>>>>> 			  DPLL_A_PIN_PAD))
>>>>>>> 		return -EMSGSIZE;
>>>>>>>-	for (fs = 0; fs < pin->prop->freq_supported_num; fs++) {
>>>>>>>+	for (fs = 0; fs < pin->prop.freq_supported_num; fs++) {
>>>>>>> 		nest = nla_nest_start(msg,
>>>>>>>DPLL_A_PIN_FREQUENCY_SUPPORTED);
>>>>>>> 		if (!nest)
>>>>>>> 			return -EMSGSIZE;
>>>>>>>-		freq = pin->prop->freq_supported[fs].min;
>>>>>>>+		freq = pin->prop.freq_supported[fs].min;
>>>>>>> 		if (nla_put_64bit(msg, DPLL_A_PIN_FREQUENCY_MIN,
>>>>>>>sizeof(freq),
>>>>>>> 				  &freq, DPLL_A_PIN_PAD)) {
>>>>>>> 			nla_nest_cancel(msg, nest);
>>>>>>> 			return -EMSGSIZE;
>>>>>>> 		}
>>>>>>>-		freq = pin->prop->freq_supported[fs].max;
>>>>>>>+		freq = pin->prop.freq_supported[fs].max;
>>>>>>> 		if (nla_put_64bit(msg, DPLL_A_PIN_FREQUENCY_MAX,
>>>>>>>sizeof(freq),
>>>>>>> 				  &freq, DPLL_A_PIN_PAD)) {
>>>>>>> 			nla_nest_cancel(msg, nest);
>>>>>>>@@ -304,9 +304,9 @@ static bool dpll_pin_is_freq_supported(struct
>>>>>>>dpll_pin
>>>>>>>*pin, u32 freq)
>>>>>>> {
>>>>>>> 	int fs;
>>>>>>>
>>>>>>>-	for (fs = 0; fs < pin->prop->freq_supported_num; fs++)
>>>>>>>-		if (freq >= pin->prop->freq_supported[fs].min &&
>>>>>>>-		    freq <= pin->prop->freq_supported[fs].max)
>>>>>>>+	for (fs = 0; fs < pin->prop.freq_supported_num; fs++)
>>>>>>>+		if (freq >= pin->prop.freq_supported[fs].min &&
>>>>>>>+		    freq <= pin->prop.freq_supported[fs].max)
>>>>>>> 			return true;
>>>>>>> 	return false;
>>>>>>> }
>>>>>>>@@ -403,7 +403,7 @@ static int
>>>>>>> dpll_cmd_pin_get_one(struct sk_buff *msg, struct dpll_pin *pin,
>>>>>>> 		     struct netlink_ext_ack *extack)
>>>>>>> {
>>>>>>>-	const struct dpll_pin_properties *prop = pin->prop;
>>>>>>>+	const struct dpll_pin_properties *prop = &pin->prop;
>>>>>>> 	struct dpll_pin_ref *ref;
>>>>>>> 	int ret;
>>>>>>>
>>>>>>>@@ -696,7 +696,7 @@ dpll_pin_on_pin_state_set(struct dpll_pin *pin,
>>>>>>>u32
>>>>>>>parent_idx,
>>>>>>> 	int ret;
>>>>>>>
>>>>>>> 	if (!(DPLL_PIN_CAPABILITIES_STATE_CAN_CHANGE &
>>>>>>>-	      pin->prop->capabilities)) {
>>>>>>>+	      pin->prop.capabilities)) {
>>>>>>> 		NL_SET_ERR_MSG(extack, "state changing is not allowed");
>>>>>>> 		return -EOPNOTSUPP;
>>>>>>> 	}
>>>>>>>@@ -732,7 +732,7 @@ dpll_pin_state_set(struct dpll_device *dpll,
>>>>>>>struct
>>>>>>>dpll_pin *pin,
>>>>>>> 	int ret;
>>>>>>>
>>>>>>> 	if (!(DPLL_PIN_CAPABILITIES_STATE_CAN_CHANGE &
>>>>>>>-	      pin->prop->capabilities)) {
>>>>>>>+	      pin->prop.capabilities)) {
>>>>>>> 		NL_SET_ERR_MSG(extack, "state changing is not allowed");
>>>>>>> 		return -EOPNOTSUPP;
>>>>>>> 	}
>>>>>>>@@ -759,7 +759,7 @@ dpll_pin_prio_set(struct dpll_device *dpll, struct
>>>>>>>dpll_pin *pin,
>>>>>>> 	int ret;
>>>>>>>
>>>>>>> 	if (!(DPLL_PIN_CAPABILITIES_PRIORITY_CAN_CHANGE &
>>>>>>>-	      pin->prop->capabilities)) {
>>>>>>>+	      pin->prop.capabilities)) {
>>>>>>> 		NL_SET_ERR_MSG(extack, "prio changing is not allowed");
>>>>>>> 		return -EOPNOTSUPP;
>>>>>>> 	}
>>>>>>>@@ -787,7 +787,7 @@ dpll_pin_direction_set(struct dpll_pin *pin,
>>>>>>>struct
>>>>>>>dpll_device *dpll,
>>>>>>> 	int ret;
>>>>>>>
>>>>>>> 	if (!(DPLL_PIN_CAPABILITIES_DIRECTION_CAN_CHANGE &
>>>>>>>-	      pin->prop->capabilities)) {
>>>>>>>+	      pin->prop.capabilities)) {
>>>>>>> 		NL_SET_ERR_MSG(extack, "direction changing is not
>>>>>>>allowed");
>>>>>>> 		return -EOPNOTSUPP;
>>>>>>> 	}
>>>>>>>@@ -817,8 +817,8 @@ dpll_pin_phase_adj_set(struct dpll_pin *pin,
>>>>>>>struct
>>>>>>>nlattr *phase_adj_attr,
>>>>>>> 	int ret;
>>>>>>>
>>>>>>> 	phase_adj = nla_get_s32(phase_adj_attr);
>>>>>>>-	if (phase_adj > pin->prop->phase_range.max ||
>>>>>>>-	    phase_adj < pin->prop->phase_range.min) {
>>>>>>>+	if (phase_adj > pin->prop.phase_range.max ||
>>>>>>>+	    phase_adj < pin->prop.phase_range.min) {
>>>>>>> 		NL_SET_ERR_MSG_ATTR(extack, phase_adj_attr,
>>>>>>> 				    "phase adjust value not supported");
>>>>>>> 		return -EINVAL;
>>>>>>>@@ -999,7 +999,7 @@ dpll_pin_find(u64 clock_id, struct nlattr
>>>>>>>*mod_name_attr,
>>>>>>> 	unsigned long i;
>>>>>>>
>>>>>>> 	xa_for_each_marked(&dpll_pin_xa, i, pin, DPLL_REGISTERED) {
>>>>>>>-		prop = pin->prop;
>>>>>>>+		prop = &pin->prop;
>>>>>>> 		cid_match = clock_id ? pin->clock_id == clock_id : true;
>>>>>>> 		mod_match = mod_name_attr && module_name(pin->module) ?
>>>>>>> 			!nla_strcmp(mod_name_attr,
>>>>>>>--
>>>>>>>2.38.1
>>>>>>>
>>>>>
>