Received-SPF: pass (google.com: domain of linux-kernel+bounces-30042-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
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 v4 1/4] dpll: fix pin dump crash for rebound module
Thread-Topic: [PATCH net v4 1/4] dpll: fix pin dump crash for rebound module
Thread-Index: AQHaSSYIaKVjAh3jtkyFeoA+JDsxBrDeD0mAgAFcfIA=
Date: Thu, 18 Jan 2024 11:12:45 +0000
Message-ID: <DM6PR11MB465715FB1297FE557BD492669B712@DM6PR11MB4657.namprd11.prod.outlook.com>
References: <20240117091416.504096-1-arkadiusz.kubalewski@intel.com>
 <20240117091416.504096-2-arkadiusz.kubalewski@intel.com>
 <Zafi9wid9HlbLM0z@nanopsycho>
In-Reply-To: <Zafi9wid9HlbLM0z@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?x4u5dqX35KX+uFaM4lNALf0j/dBeXoTnQrfqDMVQoQHl011PSzvwfZoFx5Nn?=
 =?us-ascii?Q?URoOYnG9K0iiZj/DpFVqUVwTKyw8j+glTQA00oEYG22HWNvDAxgQQ75ATuAP?=
 =?us-ascii?Q?AumSw1IakrPn1lNFQvYab2w+gDi2jfEeIwJjgTPD4A2Q0MoQV1tE4BX+g1sa?=
 =?us-ascii?Q?v7y4XGkUj1329YaU/O9CFivDacFzSF8xFHQQ5Qga7FXkkYw4+0YRThvcjO76?=
 =?us-ascii?Q?FvVj72QCClSiVt8dKD1oCRe2TyKFxWhAfXVPcjYWFR6sN+UpJAOl5rxiRnws?=
 =?us-ascii?Q?XOuN7SpQ0y8Tb3L0BWFbpxAl+0Ws+rmoDaoK6uuxzwfrcq9d8xeBHTOtEqG/?=
 =?us-ascii?Q?y28Jw1BtcxTv4QVOMm+DsU4ZPsxVYqY+sthqC8JgUYSFo/P2qlllnmotJuVH?=
 =?us-ascii?Q?ydXJgdPQeD1p4mHy+ZoTtTVg2j+qbTE3EDIHPL+j9WkL5GPDFC2eyXcmRfV8?=
 =?us-ascii?Q?4Fl2YclApi0RCW4ylsJdk/SfdA0b2GA0dsZlH6d1hAHeuySWwnzdh2pJ32MT?=
 =?us-ascii?Q?b5aaX5GRL+XXHKepO+om7PW/D4vnFvG/WQU82vy2GcitAluWE5csBmXWOjOj?=
 =?us-ascii?Q?S/FBwPjDiSwG/3yxT7sCP9YTZF/eMBjjhiqWveg7WirsyTrsBp+MzjmRTFaz?=
 =?us-ascii?Q?uG1mNjzGxcmsPvwI8MqD16kdWJAbSxdXR2kpn1ZzRkgb3tNmWTxnqKI3ojxP?=
 =?us-ascii?Q?49vcZZd0TnXoxNUJ/din3qFRbz2cN0+ra2QBAUjtt7X+IvxR3O8HFzknhvbO?=
 =?us-ascii?Q?JGPVc+HcY9BfVZqLeCpUIaObZJ++bMZ9ifk4gnhlFYp6mOptNjP0Lup+jHD0?=
 =?us-ascii?Q?F6Rxl4p9wH+MCtpfZt1AUcCFoIEqEa4Axn6YCuBb/K53uXA8C1cqE+x/G06E?=
 =?us-ascii?Q?6bNvvcc8LyVyz1W4ob+rMrghjANDXgs3MIqNaKXTFunANnXZagtl/Abz7fRq?=
 =?us-ascii?Q?atquF68BCPsfXmcFcT2Xce7qMEASirJPDLJvhYHqp7kgML9JCRbvv6z2v2jI?=
 =?us-ascii?Q?R/FPBhzWE9XX5kIKSCnGxTX+9gWyPAmatlRyGxmseLl8/lyUnd6T22JHmPKc?=
 =?us-ascii?Q?9lBgZ1Io3GT1aHE2S4NN1Kwa/MM5mWtb7S+GHbn70wOVQ4W0p0l5woLs46NH?=
 =?us-ascii?Q?hJxw/ihw38ZNN3pNvWg+upve6uK72pO7ozBjE9sIUk6g3xjLE4YYPoQD7kGF?=
 =?us-ascii?Q?dtYN8bzGsY3ZGjsE2zOGOBc6nzRWbcSVPQMhoxKcrjQnfZYzRYuW1BBuMvO+?=
 =?us-ascii?Q?tBNdNODWJSjHrbFFv5sKSGkyZtk+t68F0pPifkzszVU0E1d5koVK7NagEs5x?=
 =?us-ascii?Q?bkE/wPclswmrCSb6WZSTkCrW2daKsUMOrgM20Lb4NhP6zcsU9L3LWTeWCinz?=
 =?us-ascii?Q?+Mj6qR0OLRSjhDsedh2F5PVoTQWpnams3OuC1LnNncD6pDlU0Cfv/n71ZjEI?=
 =?us-ascii?Q?Sh8nSMq02DTpPyFrb3Q49iueNgCR/vWuzmYdSS0XROwUE1FwsYkPrjE7vYjX?=
 =?us-ascii?Q?L+0+Chmxre0eWLR4n0jEahZD2ZLTA7+QOClwKbzrQxiGc46/RyPq8/RRFUDI?=
 =?us-ascii?Q?zlkuK1O3OKHhrubSpEDv4obPykI0fIeudSGJw7MVC+1LQrTXUCcU8MhxYsLA?=
 =?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: da5258de-c050-407d-3b75-08dc181665e8
X-MS-Exchange-CrossTenant-originalarrivaltime: 18 Jan 2024 11:12:45.7307
 (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: jYi6uiXx2NsyDMtuQ/Gy9O+emP0ZExKUrYf7oFDRmRfvtqLh/wy5+d2aZm074fcCufb165Qk1DYfnExS1oe4mgCy2SSRttmmilnIPoTAdyE=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN9PR11MB5337
X-OriginatorOrg: intel.com

>From: Jiri Pirko <jiri@resnulli.us>
>Sent: Wednesday, January 17, 2024 3:24 PM
>
>Wed, Jan 17, 2024 at 10:14:13AM 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>
>
>[...]
>
>
>>@@ -443,7 +490,9 @@ dpll_pin_alloc(u64 clock_id, u32 pin_idx, struct
>>module *module,
>> 		ret =3D -EINVAL;
>> 		goto err;
>> 	}
>>-	pin->prop =3D prop;
>>+	ret =3D dpll_pin_prop_dup(prop, &pin->prop);
>>+	if (ret)
>>+		goto pin_free;
>
>This does not compile, does it? Please fix the error path first (patch
>4 should be patch 1) and then use properly named label right away there.
>

Yes, I messed up it, haven't check if each patch compiles separately.
Fixed in v5.

Thank you!
Arkadiusz

>pw-bot: cr
>
>
>> 	refcount_set(&pin->refcount, 1);
>> 	xa_init_flags(&pin->dpll_refs, XA_FLAGS_ALLOC);
>> 	xa_init_flags(&pin->parent_refs, XA_FLAGS_ALLOC);
>
>[...]