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

From: Kubalewski, Arkadiusz
Date: Wed Jan 17 2024 - 04:24:11 EST

Next message: Krzysztof Kozlowski: "Re: [PATCH] dt-bindings: serial: stm32: add power-domains property"
Previous message: UTTKARSH AGGARWAL: "Re: [RFC PATCH] usb: dwc3: gadget: Fix NULL pointer dereference in dwc3_gadget_suspend"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

>From: Jiri Pirko <jiri@xxxxxxxxxxx>
>Sent: Monday, January 15, 2024 8:24 PM
>
>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.
>

Ok

>
>>---
>>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.
>

Sure, fixed in v4.

Thank you!
Arkadiusz

>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: Krzysztof Kozlowski: "Re: [PATCH] dt-bindings: serial: stm32: add power-domains property"
Previous message: UTTKARSH AGGARWAL: "Re: [RFC PATCH] usb: dwc3: gadget: Fix NULL pointer dereference in dwc3_gadget_suspend"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]