Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   "Tian, Kevin" <kevin.tian@intel.com>
To:     Alex Williamson <alex.williamson@redhat.com>,
        ruanjinjie <ruanjinjie@huawei.com>
CC:     "kwankhede@nvidia.com" <kwankhede@nvidia.com>,
        "kraxel@redhat.com" <kraxel@redhat.com>,
        "cjia@nvidia.com" <cjia@nvidia.com>,
        "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "akrowiak@linux.ibm.com" <akrowiak@linux.ibm.com>,
        "pasic@linux.ibm.com" <pasic@linux.ibm.com>,
        "jjherne@linux.ibm.com" <jjherne@linux.ibm.com>,
        "farman@linux.ibm.com" <farman@linux.ibm.com>,
        "mjrosato@linux.ibm.com" <mjrosato@linux.ibm.com>
Subject: RE: [PATCH] vfio/mdev: fix possible memory leak in module init funcs
Thread-Topic: [PATCH] vfio/mdev: fix possible memory leak in module init funcs
Thread-Index: AQHY+v5PNbG9jfSk/kCdM8m0sVUH4a5YKr0AgAAtZtA=
Date:   Thu, 1 Dec 2022 02:00:57 +0000
Message-ID: <BN9PR11MB5276BC0B7E656465950E3A558C149@BN9PR11MB5276.namprd11.prod.outlook.com>
References: <20221118032827.3725190-1-ruanjinjie@huawei.com>
 <20221130160622.0cf3e47d.alex.williamson@redhat.com>
In-Reply-To: <20221130160622.0cf3e47d.alex.williamson@redhat.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?srszEgWpSOoR4g6/xY7+blzRRjQ+qD/qFjiyB6u5IMEQrC4i7OwyZhKS6OW7?=
 =?us-ascii?Q?qhOC8nF5OYT3t2VmMDfd9mHytVWs69ecWC1U7IoDONqwEkOgUNG7QEBzr9U8?=
 =?us-ascii?Q?ju/Lg+nsfX9Lt+QmvkprGyPM70+e1LHcTNZRpUVSXkO9vDadFQ4KqKkMbHYJ?=
 =?us-ascii?Q?wGyUNDPspHZD2ZvJHzaE3HYT7llcgERQpIlr3RE4k6H4UhQEndBLZ34GuiPT?=
 =?us-ascii?Q?FtVFcED2mFlyQj95Pm/W2+X4H4jcPkSwGzzfan8H6ay/07K3P2Kf+hM5vj5t?=
 =?us-ascii?Q?c7wpQqhY/oUDvHRP8gTG3XZw8fINh1MQnTxeIu9EPgGVPGUfXIgv9B68G8Lf?=
 =?us-ascii?Q?vHVheaFOQ/memo7yN5Oy+9iHP2VMCEBPnE/DG3CL3d8g8cE4c05EplqQmeEE?=
 =?us-ascii?Q?vzvrvGaVMEec0iGuco0JSl3F/nwmUqhy+j7KmPbUuU3Gi/30b1D+dVHp19Aw?=
 =?us-ascii?Q?Elc9cdk0hgpLYVlRHOjaHNHjCvR1/ddkfQCc6n7XnYQNj/CMP9jtjzzt9hca?=
 =?us-ascii?Q?wLhhOyQLtKhIjNPJ7OL8Z7s23lQh6fDAkwb4AOADwLPXuWvmuQjfrkVVNDkC?=
 =?us-ascii?Q?uzSEbBBeR2RnTAQjL8pqauqZ6mG/ULRmeWT+DIY3fYIFILYjJkb4e5DxLroc?=
 =?us-ascii?Q?tIGMmqdzZuuku2elhS6tcUj8YnWXwTpV7YJkP6LgDIP+tIv1OBFw7SQNsDWo?=
 =?us-ascii?Q?miPaCRIROCEHVkY1pkMC0BpSWoH2qdxjEvOkhSqICySmp8iISwS0gb/JxUZd?=
 =?us-ascii?Q?0sgF9c/IRwLs9ytBQK8eZV7q+izgnsOOgUa4DrEFg/WkQz++SaEHhmSR+AxD?=
 =?us-ascii?Q?aOWjURdOANpu/79j+gSYQnH2oW+E8VPYqtCm3BvOEr+aYV6r5fd0iJUW77p2?=
 =?us-ascii?Q?xi+Fhp+xeQJ9bCYUTk+xw92E09MnNLq09eppzCbIMt2Rpz4Ne6U1e9ktC6WA?=
 =?us-ascii?Q?dB9W6L11gcRsnUSD1ikhW8WrUjZX5awNLKpG88ns9FOczyYKUAS9Xtnvm0+H?=
 =?us-ascii?Q?WgHkWZTMXzNnn7GRiVRxgyOmTNttyJpoFuGmHL4kAJOsVcjDgReZXOgt4xJJ?=
 =?us-ascii?Q?Xl2d339/cfBF9QttIL4XPC1orJwlAkKo9xtPBnUu4/wup/7KYgLHIw5SVvgZ?=
 =?us-ascii?Q?GL6k8qmHRHzNyoATrvugzVw0dWGOL5eoSerea/KZWwND/MXTpHHTEYCkvcro?=
 =?us-ascii?Q?2e1nPko4JahDaKEv47zrVAK1I77UZTaHG147jF848gP0Pgml+RW7btTDZkVN?=
 =?us-ascii?Q?w2D3VMHPvbkofz1QJZujvDeiSdxy+D5kxMOalwH1cIFDECoxsAaFeCQnJ5gT?=
 =?us-ascii?Q?IMb+ZZeOai186q9292rCDFzypvj2YnRPpo7borHoU4a1iu17YRqIFqxSd3uQ?=
 =?us-ascii?Q?hUmUd1I/cyW+uwSMM++f0hH7o3gDhDfdHNTbgHUpTcm2AdSw+cCIvahbSzAf?=
 =?us-ascii?Q?+65Fz5jUqIE8oqLZo3YuAcXDDco4k8bHMnFnhAKqcMxFu+sGGX0SWLylAwRP?=
 =?us-ascii?Q?PA9iiUtb6hROqlMD5/5Xe3/iZHXzJsZOdh4JLysTIF0YT0ipkoDj0ZTnVAfM?=
 =?us-ascii?Q?gAOodAk33V/USDylc7MxNGHGmHah8JkzEWF+tLXu?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BN9PR11MB5276.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 71e956f9-cdd1-4c19-48b1-08dad33fe30a
X-MS-Exchange-CrossTenant-originalarrivaltime: 01 Dec 2022 02:00:57.1693
 (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: /pebHyhWdyVD3j9baF89bcJhyViRr6WfHUxiY1lRudxIjANmu+lM+S/oMs1nrxL7HM0r/5ZsPaIWh/TH7dEkhQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB5914
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> From: Alex Williamson <alex.williamson@redhat.com>
> Sent: Thursday, December 1, 2022 7:06 AM
>=20
> [Cc +vfio-ap, vfio-ccw]
>=20
> On Fri, 18 Nov 2022 11:28:27 +0800
> ruanjinjie <ruanjinjie@huawei.com> wrote:
>=20
> > Inject fault while probing module, if device_register() fails,
> > but the refcount of kobject is not decreased to 0, the name
> > allocated in dev_set_name() is leaked. Fix this by calling
> > put_device(), so that name can be freed in callback function
> > kobject_cleanup().

It's not just about the name. The problem of kboject not being
released is a bigger one.

put_device() is always required no matter device_register()
succeeds or not:

* NOTE: _Never_ directly free @dev after calling this function, even
 * if it returned an error! Always use put_device() to give up the
 * reference initialized in this function instead.
 */
int device_register(struct device *dev)

> > @@ -1430,8 +1430,10 @@ static int __init mbochs_dev_init(void)
> >  	dev_set_name(&mbochs_dev, "%s", MBOCHS_NAME);
> >
> >  	ret =3D device_register(&mbochs_dev);
> > -	if (ret)
> > +	if (ret) {
> > +		put_device(&mbochs_dev);
> >  		goto err_class;
> > +	}
> >
> >  	ret =3D mdev_register_parent(&mbochs_parent, &mbochs_dev,
> &mbochs_driver,
> >  				   mbochs_mdev_types,
>=20
>=20
> vfio-ap has a similar unwind as the sample drivers, but actually makes
> an attempt to catch this ex:
>=20
> 	...
>         ret =3D device_register(&matrix_dev->device);
>         if (ret)
>                 goto matrix_reg_err;
>=20
>         ret =3D driver_register(&matrix_driver);
>         if (ret)
>                 goto matrix_drv_err;
>=20
>         return 0;
>=20
> matrix_drv_err:
>         device_unregister(&matrix_dev->device);
> matrix_reg_err:
>         put_device(&matrix_dev->device);
> 	...
>=20
> So of the vfio drivers calling device_register(), vfio-ap is the only
> one that does a put_device() if device_register() fails, but it also
> seems sketchy to call both device_unregister() and put_device() in the
> case that we exit via matrix_drv_err.
>=20
> I wonder if all of these shouldn't adopt a flow like:
>=20
> 	ret =3D device_register(&dev);
> 	if (ret)
> 		goto err1;
>=20
> 	....
>=20
> 	return 0;
>=20
> err2:
> 	device_del(&dev);
> err1:
> 	put_device(&dev);
>=20

It's kind of a mixed model.

With above unwind it's clearer to use device_initialize() and device_add() =
instead.

Otherwise what this patch does looks better IMHO:

	ret =3D device_register(&dev);
	if (ret) {
		put_device(&dev);
		goto err1;
	}

	...

	return 0;

err2:
	device_unregister(&dev);
err1:
	earlier_unwind();