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

Fri, Nov 10, 2023 at 12:32:21AM CET, arkadiusz.kubalewski@intel.com wrote:

quoted

From: Jiri Pirko <jiri@resnulli.us>
Sent: Thursday, November 9, 2023 7:06 PM

Thu, Nov 09, 2023 at 05:30:20PM CET, arkadiusz.kubalewski@intel.com wrote:

quoted

quoted

From: Jiri Pirko <jiri@resnulli.us>
Sent: Thursday, November 9, 2023 2:19 PM

Thu, Nov 09, 2023 at 01:20:48PM CET, arkadiusz.kubalewski@intel.com
wrote:

quoted

quoted

From: Jiri Pirko <jiri@resnulli.us>
Sent: Wednesday, November 8, 2023 3:30 PM

Wed, Nov 08, 2023 at 11:32:25AM CET, arkadiusz.kubalewski@intel.com
wrote:

quoted

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 <arkadiusz.kubalewski@intel.com>
---
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

quoted

quoted

Thank you!
Arkadiusz

quoted

quoted

Thank you!
Arkadiusz

quoted

quoted

	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