From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from lindbergh.monkeyblade.net (lindbergh.monkeyblade.net [23.128.96.19]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id CFC4A1DFC8 for ; Thu, 9 Nov 2023 13:19:11 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=resnulli-us.20230601.gappssmtp.com header.i=@resnulli-us.20230601.gappssmtp.com header.b="plnqCv5s" Received: from mail-ed1-x52c.google.com (mail-ed1-x52c.google.com [IPv6:2a00:1450:4864:20::52c]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 2383B30CD for ; Thu, 9 Nov 2023 05:19:11 -0800 (PST) Received: by mail-ed1-x52c.google.com with SMTP id 4fb4d7f45d1cf-543923af573so1377666a12.0 for ; Thu, 09 Nov 2023 05:19:11 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=resnulli-us.20230601.gappssmtp.com; s=20230601; t=1699535949; x=1700140749; darn=vger.kernel.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=66wh30Ew82ffDebJ18oPJoQua0dXNHmGRb6puz7dPeM=; b=plnqCv5shIwes3+RY3edDRIA9Eelhe/7JVHigwtTqcbCStd6K7ZZq2SQU+kSbH8PV8 e8vEnfi7pWTHaKgQLnefnBn9eMg4swMDE8ARGbYqlhipR32aOzCr3oNXSBdYqyVCNBd+ 4US6LH6p13tEiZKHvKMaqFOvoRPDf/xEkl+ZIuJ3TG9uIPJwd1wKmbgwWEBhqZ/Gd0Qx pbARUXK/exTASjKaguHI9LKL4fVRLt4XZ+CuJUur7Fq9dnrdgyHOQke0eResTew9ZRRO bOgrS9iPqYAtPGP4bCCmlbIHsrZHw5ULe1/oB+Y3dVC4cKAQdIf3bspfirY40n0TRSnW 38gQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1699535949; x=1700140749; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=66wh30Ew82ffDebJ18oPJoQua0dXNHmGRb6puz7dPeM=; b=OFlnm6zoc0i8vewN8mDB/z9ayTP7WeUDDn4cY1jV23zckdDhUHnbB5D3lR2gbtLQXZ bHn5w4P5AGO+NrHdv4ZHTlqsCPw7iUS1OcWfzFDzP6SXtuXkkc3VVYXvQo5KnxlRemdc oLIu+sP9izhJaG2Ye0DAd0heDDSocvYZUxqNC1t236wRJSfQYgwhxIZ2DI3s1fOWzFs7 UrOO6lZgw1lFwzg0qgrCxcfCxXa4+l+boDMnkYnZVtrCketqtTLBH0t9jb3Clckn7NIh yrnuYKrfLdsApwmn5vcimMDb1dpG+YapMsPde0mfUZcdvugELGGaJSVocBarpUcKgacP cZDQ== X-Gm-Message-State: AOJu0YyK7jfM0rBWL4ivD59vpZMWKV2AecmLnnw+YsOXbuPN5Kqm2pO7 ujHj4FEfHGDAXaqYY5ymCsUslw== X-Google-Smtp-Source: AGHT+IGW5rkFb5rbkhe30pmfhOx1tnnI7AwNRRoRWtnkaqrtsJ0+gptnPCAStqicmXldeDxXNgpXow== X-Received: by 2002:a17:907:7288:b0:9df:2fb1:1a83 with SMTP id dt8-20020a170907728800b009df2fb11a83mr4034677ejc.39.1699535949684; Thu, 09 Nov 2023 05:19:09 -0800 (PST) Received: from localhost ([86.61.181.4]) by smtp.gmail.com with ESMTPSA id m20-20020a17090607d400b009ce03057c4dsm2532546ejc.2.2023.11.09.05.19.08 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 09 Nov 2023 05:19:09 -0800 (PST) Date: Thu, 9 Nov 2023 14:19:08 +0100 From: Jiri Pirko To: "Kubalewski, Arkadiusz" Cc: "netdev@vger.kernel.org" , "vadim.fedorenko@linux.dev" , "Michalik, Michal" , "Olech, Milena" , "pabeni@redhat.com" , "kuba@kernel.org" Subject: Re: [PATCH net 2/3] dpll: fix pin dump crash for rebound module Message-ID: References: <20231108103226.1168500-1-arkadiusz.kubalewski@intel.com> <20231108103226.1168500-3-arkadiusz.kubalewski@intel.com> Precedence: bulk X-Mailing-List: netdev@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Thu, Nov 09, 2023 at 01:20:48PM CET, arkadiusz.kubalewski@intel.com wrote: >>From: Jiri Pirko >>Sent: Wednesday, November 8, 2023 3:30 PM >> >>Wed, Nov 08, 2023 at 11:32:25AM CET, arkadiusz.kubalewski@intel.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 >>>--- >>> 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. > >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 >>> >