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

From: Jiri Pirko
Date: Mon Jan 15 2024 - 14:23:51 EST

Next message: Randy Dunlap: "[PATCH] tpm/tpm_ftpm_tee: fix all kernel-doc warnings"
Previous message: Bernd Edlinger: "[PATCH v14] exec: Fix dead-lock in de_thread with ptrace_attach"
In reply to: Arkadiusz Kubalewski: "[PATCH net v3 1/3] dpll: fix pin dump crash for rebound module"
Next in thread: Arkadiusz Kubalewski: "[PATCH net v3 2/3] dpll: fix register pin with unregistered parent pin"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Mon, Jan 15, 2024 at 09:52:39AM CET, arkadiusz.kubalewski@xxxxxxxxx wrote:
>When a kernel module is unbound but the pin resources were not entirely
>freed (other kernel module instance of the same PCI device 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")
>Reviewed-by: Jan Glaza <jan.glaza@xxxxxxxxx>
>Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@xxxxxxxxx>
>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@xxxxxxxxx>

Looks fine, small issue below.

>---
>v3:
>- introduce helper functions for easier pin properties handling
>- duplicate property labels only if not null, fail if duplicating fails
>
> drivers/dpll/dpll_core.c | 57 +++++++++++++++++++++++++++++++++++--
> drivers/dpll/dpll_core.h | 4 +--
> drivers/dpll/dpll_netlink.c | 28 +++++++++---------
> 3 files changed, 70 insertions(+), 19 deletions(-)
>
>diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
>index 3568149b9562..fbac32af78b7 100644
>--- a/drivers/dpll/dpll_core.c
>+++ b/drivers/dpll/dpll_core.c
>@@ -424,6 +424,53 @@ void dpll_device_unregister(struct dpll_device *dpll,
> }
> EXPORT_SYMBOL_GPL(dpll_device_unregister);
>
>+static void dpll_pin_prop_free(struct dpll_pin_properties *prop)
>+{
>+ kfree(prop->package_label);
>+ kfree(prop->panel_label);
>+ kfree(prop->board_label);
>+ kfree(prop->freq_supported);
>+}
>+
>+static int dpll_pin_prop_dup(const struct dpll_pin_properties *src,
>+ struct dpll_pin_properties *dst)
>+{
>+ memcpy(dst, src, sizeof(*dst));
>+ if (src->freq_supported && src->freq_supported_num) {
>+ size_t freq_size = src->freq_supported_num *
>+ sizeof(*src->freq_supported);
>+ dst->freq_supported = kmemdup(src->freq_supported,
>+ freq_size, GFP_KERNEL);
>+ if (!src->freq_supported)
>+ return -ENOMEM;
>+ }
>+ if (src->board_label) {
>+ dst->board_label = kstrdup(src->board_label, GFP_KERNEL);
>+ if (!dst->board_label)
>+ goto free_freq_supp;
>+ }
>+ if (src->panel_label) {
>+ dst->panel_label = kstrdup(src->panel_label, GFP_KERNEL);
>+ if (!dst->panel_label)
>+ goto free_board_label;
>+ }
>+ if (src->package_label) {
>+ dst->package_label = kstrdup(src->package_label, GFP_KERNEL);
>+ if (!dst->package_label)
>+ goto free_panel_label;
>+ }
>+
>+ return 0;
>+
>+free_panel_label:
>+ kfree(dst->panel_label);
>+free_board_label:
>+ kfree(dst->board_label);
>+free_freq_supp:
>+ kfree(dst->freq_supported);
>+ return -ENOMEM;
>+}
>+
> static struct dpll_pin *
> dpll_pin_alloc(u64 clock_id, u32 pin_idx, struct module *module,
> const struct dpll_pin_properties *prop)
>@@ -440,9 +487,11 @@ dpll_pin_alloc(u64 clock_id, u32 pin_idx, struct module *module,
> if (WARN_ON(prop->type < DPLL_PIN_TYPE_MUX ||
> prop->type > DPLL_PIN_TYPE_MAX)) {
> ret = -EINVAL;
>- goto err;
>+ goto pin_free;

This should be a separate patch, fix is unrelated to the rest of this
patch.

Also, user err_something consistently as err_pin_free, err_destroy_xa
etc.

pw-bot: cr

> }
>- pin->prop = prop;
>+ ret = dpll_pin_prop_dup(prop, &pin->prop);
>+ if (ret)
>+ goto pin_free;
> refcount_set(&pin->refcount, 1);
> xa_init_flags(&pin->dpll_refs, XA_FLAGS_ALLOC);
> xa_init_flags(&pin->parent_refs, XA_FLAGS_ALLOC);
>@@ -453,6 +502,7 @@ dpll_pin_alloc(u64 clock_id, u32 pin_idx, struct module *module,
> err:
> xa_destroy(&pin->dpll_refs);
> xa_destroy(&pin->parent_refs);
>+pin_free:
> kfree(pin);
> return ERR_PTR(ret);
> }
>@@ -512,6 +562,7 @@ void dpll_pin_put(struct dpll_pin *pin)
> xa_destroy(&pin->dpll_refs);
> xa_destroy(&pin->parent_refs);
> xa_erase(&dpll_pin_xa, pin->id);
>+ dpll_pin_prop_free(&pin->prop);
> kfree(pin);
> }
> mutex_unlock(&dpll_lock);
>@@ -634,7 +685,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 ce7cf736f020..4c64611d32ac 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;
> }
>@@ -396,7 +396,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;
>
>@@ -689,7 +689,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;
> }
>@@ -725,7 +725,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;
> }
>@@ -752,7 +752,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;
> }
>@@ -780,7 +780,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;
> }
>@@ -810,8 +810,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;
>@@ -995,7 +995,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
>

Next message: Randy Dunlap: "[PATCH] tpm/tpm_ftpm_tee: fix all kernel-doc warnings"
Previous message: Bernd Edlinger: "[PATCH v14] exec: Fix dead-lock in de_thread with ptrace_attach"
In reply to: Arkadiusz Kubalewski: "[PATCH net v3 1/3] dpll: fix pin dump crash for rebound module"
Next in thread: Arkadiusz Kubalewski: "[PATCH net v3 2/3] dpll: fix register pin with unregistered parent pin"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]