Received-SPF: pass (google.com: domain of linux-kernel+bounces-28720-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
From: "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@intel.com>
To: Jiri Pirko <jiri@resnulli.us>
CC: "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"vadim.fedorenko@linux.dev" <vadim.fedorenko@linux.dev>,
	"davem@davemloft.net" <davem@davemloft.net>, "Olech, Milena"
	<milena.olech@intel.com>, "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>, "pabeni@redhat.com" <pabeni@redhat.com>,
	"kuba@kernel.org" <kuba@kernel.org>, mschmidt <mschmidt@redhat.com>, "Glaza,
 Jan" <jan.glaza@intel.com>, "Kitszel, Przemyslaw"
	<przemyslaw.kitszel@intel.com>
Subject: RE: [PATCH net v3 1/3] dpll: fix pin dump crash for rebound module
Thread-Topic: [PATCH net v3 1/3] dpll: fix pin dump crash for rebound module
Thread-Index: AQHaR5ClEEpG0cXcvUWhcYYw07aC5rDbQYMAgAJ8BdA=
Date: Wed, 17 Jan 2024 09:22:40 +0000
Message-ID: <DM6PR11MB4657B312964E35622709B1619B722@DM6PR11MB4657.namprd11.prod.outlook.com>
References: <20240115085241.312144-1-arkadiusz.kubalewski@intel.com>
 <20240115085241.312144-2-arkadiusz.kubalewski@intel.com>
 <ZaWGMrzj4wNC-wT3@nanopsycho>
In-Reply-To: <ZaWGMrzj4wNC-wT3@nanopsycho>
Accept-Language: pl-PL, en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?08vcbV12dyp72eId4uDmB0eDunMwunWm+FxLkDhZmwjw9AI4ptNKvfN7b76y?=
 =?us-ascii?Q?JiIA6H0HHVQ7RniOhCoosVDMqNrIlxA9FTcIy2r4254LzLLLPyT/HGwUqDrW?=
 =?us-ascii?Q?a3EZ6LkuRV1xuLxZljomFsuZ5FSR92grb/6G8FHKOMJnueOgbhPIA8cG15z1?=
 =?us-ascii?Q?xHIfq53kaCIyzj6vbR4/ndjRRXK3xzm1b2w9esxnLTQjfF49OsFLCv3tjSRl?=
 =?us-ascii?Q?XXhQZwmy54f/nSWef6RznQDi0mjisPEVLLnfEQjT4KaPuv9oB7sLMuTj96o0?=
 =?us-ascii?Q?vIHbZeSiHqbziMN/5Tjt9GDq2IrVkKwjM/EMQPAOpBJVkA+kCyRXf3vdShfw?=
 =?us-ascii?Q?YLKe9DkvBMe7C0di6WdpgsZPU99wzVxgkRJ/q2gXhMHfH78rK8pZpvgAelD8?=
 =?us-ascii?Q?OHZ/67mcyRz2G3hN3LYGHrMFaJRUE27gr1Xoi5yMwFtgYEgl9gU2yfOFTWLp?=
 =?us-ascii?Q?b349N/5a0ywp1On4lWp/G3g0Ki8Aj7S6Mg0yL1ChT90+n2kcLrlHKPt5Xdr8?=
 =?us-ascii?Q?TFiRx3oPcdRG+ODceGX3ex5rlAZOcNnQA/1/iEIxFO7tDtdSZxFtGx4qvz2c?=
 =?us-ascii?Q?QgJb0p3ABbJfQkPTFH1CGUqPHh10tdJCGWiIs7ms2B/zeU7USVqnTjtz4Yqb?=
 =?us-ascii?Q?iQWlt+3Ztukp185gEvANdWDHZQZCl5sDEN7KO19WYSTIBHFhpueKebZSPqEj?=
 =?us-ascii?Q?t/5E/GtKA5JcceGv41Fg1Befz+AcxQNUifXkcu1b2yMm/3oHCEZZlxJt0ZwL?=
 =?us-ascii?Q?831lCFsYWNZOPaSLCDgG3vKoDRfSeG2HOiBjtIqsCmkqJ4EZg9CNEpI9qjba?=
 =?us-ascii?Q?35quVoTBn0rV4T9SvlJOPqYeh45oeTUUeZ1Qb2/usnOePLW9f0iZVf+Yt7Ce?=
 =?us-ascii?Q?QVKyFJl0zhLnckcJNoWcSZRZ/xOz5N3U/6FuV8yOQvoo4vBaFfMOqO2RwVPe?=
 =?us-ascii?Q?JY5pulO1lmixnB30/NAO+DHVKt8tDS+nHTFhdFvQiUICtkLqhdupVbq8BXhi?=
 =?us-ascii?Q?YA/MI5tHLHnvx+sl8u/7idTWZfM6F2C7SVR2/m7FLBhn69Id//+JDD8Km1mJ?=
 =?us-ascii?Q?erfzs9rlwN+qe0U3vMKM5vnF8fBifTpUsndkFQdYnB2+oyoLE5qPup8mFh1y?=
 =?us-ascii?Q?cqm2/KFWrV1jOTxWN0Q8dAkX0sJjj1IAMRPblRz90JaTqi27yW+cxFG7DJ4K?=
 =?us-ascii?Q?44Obggv4duNHXc4WdMdOqptcFAv9b2TH43BqvwPh1uyfUKWxo2A3zRCUaoMB?=
 =?us-ascii?Q?NRlRh+zTSUpValBzwsBbxwJ0m2y+QIujC3F54y6+FCT27ToG1dfv+A4TUy6I?=
 =?us-ascii?Q?wijlDly87yviaJJYHcSrl1BmEmeME65I+gaNw1zAB9caHHRYGlcFRa26+h/Z?=
 =?us-ascii?Q?s00KYVCuAavoTmb9hhzkW0/tLUafvVWEPZA878anOWPbpewDrW/amGx0uOxC?=
 =?us-ascii?Q?sOQ1Bb5snEaipF+S/xLsT8gvgpKvTg7lRuGC0h0D3BoH35fHitTTwAzGmA1q?=
 =?us-ascii?Q?FWYiEIe+G77eO3pf7oyZD9LSFyJXH9X1ppOJhahseRT6IpBHLVQm5UiEae2b?=
 =?us-ascii?Q?tD0cSkdl7X3c+4+fNe231xPtkehVuNv8v61WlpSmXAjpe5tlAPMVjCNHpFms?=
 =?us-ascii?Q?hQ=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
X-Mailing-List: linux-kernel@vger.kernel.org
List-Id: <linux-kernel.vger.kernel.org>
List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB4657.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 476f2d63-1749-4f2b-cdf7-08dc173dda75
X-MS-Exchange-CrossTenant-originalarrivaltime: 17 Jan 2024 09:22:40.4816
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: aYxqnC2DSAmKYvuAs5ZVseT/sfX2rfDL4ACSfy1kU/5YckwqAgNTPgCApn0KDoM877HT9iLYtK60qc+cTlltl3x8wo3TE2i9mTneYPyjx+Q=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR11MB7894
X-OriginatorOrg: intel.com

>From: Jiri Pirko <jiri@resnulli.us>
>Sent: Monday, January 15, 2024 8:24 PM
>
>Mon, Jan 15, 2024 at 09:52:39AM CET, arkadiusz.kubalewski@intel.com 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@intel.com>
>>Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>
>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 =3D src->freq_supported_num *
>>+				   sizeof(*src->freq_supported);
>>+		dst->freq_supported =3D kmemdup(src->freq_supported,
>>+					      freq_size, GFP_KERNEL);
>>+		if (!src->freq_supported)
>>+			return -ENOMEM;
>>+	}
>>+	if (src->board_label) {
>>+		dst->board_label =3D kstrdup(src->board_label, GFP_KERNEL);
>>+		if (!dst->board_label)
>>+			goto free_freq_supp;
>>+	}
>>+	if (src->panel_label) {
>>+		dst->panel_label =3D kstrdup(src->panel_label, GFP_KERNEL);
>>+		if (!dst->panel_label)
>>+			goto free_board_label;
>>+	}
>>+	if (src->package_label) {
>>+		dst->package_label =3D 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 =3D -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 =3D prop;
>>+	ret =3D 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 !=3D DPLL_PIN_TYPE_MUX))
>>+	if (WARN_ON(parent->prop.type !=3D 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 =3D 0; fs < pin->prop->freq_supported_num; fs++) {
>>+	for (fs =3D 0; fs < pin->prop.freq_supported_num; fs++) {
>> 		nest =3D nla_nest_start(msg, DPLL_A_PIN_FREQUENCY_SUPPORTED);
>> 		if (!nest)
>> 			return -EMSGSIZE;
>>-		freq =3D pin->prop->freq_supported[fs].min;
>>+		freq =3D 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 =3D pin->prop->freq_supported[fs].max;
>>+		freq =3D 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_pi=
n
>>*pin, u32 freq)
>> {
>> 	int fs;
>>
>>-	for (fs =3D 0; fs < pin->prop->freq_supported_num; fs++)
>>-		if (freq >=3D pin->prop->freq_supported[fs].min &&
>>-		    freq <=3D pin->prop->freq_supported[fs].max)
>>+	for (fs =3D 0; fs < pin->prop.freq_supported_num; fs++)
>>+		if (freq >=3D pin->prop.freq_supported[fs].min &&
>>+		    freq <=3D 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 =3D pin->prop;
>>+	const struct dpll_pin_properties *prop =3D &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 =3D 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 =3D pin->prop;
>>+		prop =3D &pin->prop;
>> 		cid_match =3D clock_id ? pin->clock_id =3D=3D clock_id : true;
>> 		mod_match =3D mod_name_attr && module_name(pin->module) ?
>> 			!nla_strcmp(mod_name_attr,
>>--
>>2.38.1
>>