| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <DM6PR11MB4657B61E86D5DFFAF83BC1E59BAEA@DM6PR11MB4657.namprd11.prod.outlook.com>
Date: Fri, 10 Nov 2023 09:01:50 +0000
From: "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com>
To: Jiri Pirko <jiri@...nulli.us>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"vadim.fedorenko@...ux.dev" <vadim.fedorenko@...ux.dev>, "Michalik, Michal"
<michal.michalik@...el.com>, "Olech, Milena" <milena.olech@...el.com>,
"pabeni@...hat.com" <pabeni@...hat.com>, "kuba@...nel.org" <kuba@...nel.org>
Subject: RE: [PATCH net 2/3] dpll: fix pin dump crash for rebound module
>From: Jiri Pirko <jiri@...nulli.us>
>Sent: Friday, November 10, 2023 7:46 AM
>
>Fri, Nov 10, 2023 at 12:32:21AM CET, arkadiusz.kubalewski@...el.com wrote:
>>>From: Jiri Pirko <jiri@...nulli.us>
>>>Sent: Thursday, November 9, 2023 7:06 PM
>>>
>>>Thu, Nov 09, 2023 at 05:30:20PM CET, arkadiusz.kubalewski@...el.com
>>>wrote:
>>>>>From: Jiri Pirko <jiri@...nulli.us>
>>>>>Sent: Thursday, November 9, 2023 2:19 PM
>>>>>
>>>>>Thu, Nov 09, 2023 at 01:20:48PM CET, arkadiusz.kubalewski@...el.com
>>>>>wrote:
>>>>>>>From: Jiri Pirko <jiri@...nulli.us>
>>>>>>>Sent: Wednesday, November 8, 2023 3:30 PM
>>>>>>>
>>>>>>>Wed, Nov 08, 2023 at 11:32:25AM CET, arkadiusz.kubalewski@...el.com
>>>>>>>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 <arkadiusz.kubalewski@...el.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.
>
What you are suggesting is:
- special purpose bus in the driver,
- dpll-related,
- not needed,
- prone for errors.
The dpll subsystem is here to make driver life easier.
Thank you!
Arkadiusz
>
>>
>>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
>>>>>>>>
>>>>>>
>>
Powered by blists - more mailing lists