Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
From:   Vishnu Dasa <vdasa@vmware.com>
To:     Christophe JAILLET <christophe.jaillet@wanadoo.fr>
CC:     Jorgen Hansen <jhansen@vmware.com>,
        "arnd@arndb.de" <arnd@arndb.de>,
        "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        "acking@vmware.com" <acking@vmware.com>,
        "dtor@vmware.com" <dtor@vmware.com>,
        Pv-drivers <Pv-drivers@vmware.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "kernel-janitors@vger.kernel.org" <kernel-janitors@vger.kernel.org>
Subject: Re: [PATCH 3/3] VMCI: Fix some error handling paths in
 vmci_guest_probe_device()
Thread-Topic: [PATCH 3/3] VMCI: Fix some error handling paths in
 vmci_guest_probe_device()
Thread-Index: AQHYHs1qh1rsU2MYvUCsLtTWA1uvXqyh8XYA
Date:   Thu, 24 Feb 2022 00:43:26 +0000
Message-ID: <864E509D-C980-4726-A3EF-18BCAD6CBB6F@vmware.com>
References: <cover.1644531317.git.christophe.jaillet@wanadoo.fr>
 <c5c1a5f3547bea3c4a1cfb8474db683d83c0ca1d.1644531317.git.christophe.jaillet@wanadoo.fr>
In-Reply-To: <c5c1a5f3547bea3c4a1cfb8474db683d83c0ca1d.1644531317.git.christophe.jaillet@wanadoo.fr>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?Wcy77WjUYfKr4Z7VLNYyQ7VmJxV9FlQxVDMOGkwuSl/vpgfwnUpovjZDHiPa?=
 =?us-ascii?Q?rx5moEj2hzBAvZJFW0zgJOkjo58TBB5eACvxvttvO0XFk9/5NwvkdHx83C4S?=
 =?us-ascii?Q?YvE/8JjfvDXE+flFVXg/5XglyhDC5fTvTKpRQZn9wlvhRfbb9pgToNZaFa1Y?=
 =?us-ascii?Q?ESEKDnC2hLRZLp5Ei69O8HB61dJJVfJO58tcesZanKSJipa9zGpjBUZTJlsB?=
 =?us-ascii?Q?G/FaqAcmKIYhjbyxW5X4wfcYgkRw0aNS1VsLcGAnEEgh3SgVOfUIjFJr8qwY?=
 =?us-ascii?Q?0SWtKCsYptvVQBtCFHUstP9pB2hmNktyxnalFsIz96SMfaotSNFUPdxPS/3H?=
 =?us-ascii?Q?HVUUda4vbGhMtKzio17gLtM/31ewUXxAt1S5CnhWvXpt710xBBYlIP6BfKSo?=
 =?us-ascii?Q?YgN6+aOaqwfU2TM7H8V3kS0L/kN2BMkbRP3rZ5n0RzECKaiPY51O14dfe5UX?=
 =?us-ascii?Q?v67L+9I0crtFMjYxu5HGcqwTFOSa6MGMqLGeuOITALIwTkVhfeqRd6uHVzcv?=
 =?us-ascii?Q?a5BiGmtmjqM6lYx/ZfgshtqvpdV7Ab44NMMShgOIzsjTYPNJCnwqxMGz0rTd?=
 =?us-ascii?Q?7Ey4lMTHHryRBR8R0XIkvDEu6zQdfl3+Sd37CE0AwA2VBiEvrLJEyHk5RIxQ?=
 =?us-ascii?Q?QWJfD+GYWlCtoYOmX03C/5/HSSm5MlquAdrcu0YAGaUDnhJr5sHT2Fg+JwAN?=
 =?us-ascii?Q?pYesBWKL53dBY/72xvJzrqfY2R7cLAgo/oxkT+KoP8VkAO/aYzn9YM6uotau?=
 =?us-ascii?Q?LTn3cjoga5IAIZ2ul85arQJWRyB7tZnvHgYYOeuZRXCq9FTkv6yDdbm6eJx8?=
 =?us-ascii?Q?XpmKn7C9cx4m/VilfCC1m11IiKNPubQEP0X3h6G7AUW21NvUKbxAXWKqrSeH?=
 =?us-ascii?Q?RE9pGWk/jTm7PIaADE0eM0QceG5C5M2P1mTUPjsZaRWqw3JYQb9Y+DR9Lxr8?=
 =?us-ascii?Q?MO6mqOcn5zwQl0JYqBHX33AsM+NAD5Y0lGKld4jFrQ52T38SPCUnenWVxwOI?=
 =?us-ascii?Q?lrATZC5zuAFp0yL/YmYWH0b/fkrDyrPztjXVDgcHmQnEa41GlWvfRxqfF01Q?=
 =?us-ascii?Q?qBANwPeq5MmXOiKHRiNBvURB9mhsWDd2gl+IZ++nC73SO29BAo5oPWKnIeM4?=
 =?us-ascii?Q?RTuVqPV/A6w/ExipN4uPlPyRRPJYPHfE8kVw7s5JDLcnvC4j8estSdTnCcZs?=
 =?us-ascii?Q?Ib5ReSlxMSfIzq0Z2CM1xdi5lRXHZnyHknIDpnRCwuulnV1tWXDUWYBvin53?=
 =?us-ascii?Q?ZQWW/v7fnXVJyXZAMHnRV5PYBB4tfX9FIlK/pmYuBfkOCT8L0uX90ZD1y1Ha?=
 =?us-ascii?Q?n9HiGCkeQ4YiE6xHzOvz1yQkSGFNGdztcYacKVjIuPvB29idRSMpQmkRq2Sf?=
 =?us-ascii?Q?m/cGMqrMcSWiYL2Pm5+98kQPcYpCkryXt89R2T8NIjSNjNYfIWu1jkM8Vfxo?=
 =?us-ascii?Q?d+Ef9EqbCcGZAx26CJcod4JVe3rRDkTgCIsnOdGIW5dMw1ZF/bEgR6QxEOyQ?=
 =?us-ascii?Q?ekA+KmTOe6Qf/hH3b3pFXcup5kaJ0mCuBgnkHh1pDv2hlgirDXv8nK/bCWnD?=
 =?us-ascii?Q?f8Hqz7EJqe8pATfHtrQ0oKRWrChE4usFfvqvib41APNcbBvqRuP9ubvj0P1P?=
 =?us-ascii?Q?3tDdxUVCLCsmR7+a7pzWENhUHOnlssxliuMAyH+ZYi+iJMpZPpNedxGF1R1U?=
 =?us-ascii?Q?cZrInYnHud3w95Rx6TpF1i2BSJk=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <892F96C4AE17DD45B7234AD3651A6D2E@namprd05.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: vmware.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: DM6PR05MB3962.namprd05.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 53b430ef-2e23-477f-850d-08d9f72eab98
X-MS-Exchange-CrossTenant-originalarrivaltime: 24 Feb 2022 00:43:26.7685
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b39138ca-3cee-4b4a-a4d6-cd83d9dd62f0
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: Uz7UbO4JiHmLQhh9Fz1P3TkyWxcHH409bfA4xW59CaHB2HAv2bxpaK8BpJfUDfF18maPPiez8+x2a3iKo8vSwQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO6PR05MB7506
X-Spam-Status: No, score=-2.0 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
	DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,RDNS_NONE,SPF_HELO_NONE,T_SCC_BODY_TEXT_LINE
	autolearn=no 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


> On Feb 10, 2022, at 2:27 PM, Christophe JAILLET <christophe.jaillet@wanad=
oo.fr> wrote:
>=20
> The 'err_remove_vmci_dev_g' error label is not at the right place.
> This could lead to un-released resource.
>=20
> There is also a missing label. If pci_alloc_irq_vectors() fails, the
> previous vmci_event_subscribe() call must be undone.
>=20
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>

Thanks for the patch!
Let's deal with the points which Dan brought up separately.

Acked-by: Vishnu Dasa <vdasa@vmware.com>

> ---
> Review with GREAT care.
>=20
> This patch is a recent rebase of an old patch that has never been
> submitted.
> This function is huge and modifying its error handling path is error
> prone (at least for me).
>=20
> The patch is compile-tested only.
> ---
> drivers/misc/vmw_vmci/vmci_guest.c | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>=20
> diff --git a/drivers/misc/vmw_vmci/vmci_guest.c b/drivers/misc/vmw_vmci/v=
mci_guest.c
> index 02d4722d8474..2e8ad36895ae 100644
> --- a/drivers/misc/vmw_vmci/vmci_guest.c
> +++ b/drivers/misc/vmw_vmci/vmci_guest.c
> @@ -765,7 +765,7 @@ static int vmci_guest_probe_device(struct pci_dev *pd=
ev,
> 	/* Check host capabilities. */
> 	error =3D vmci_check_host_caps(pdev);
> 	if (error)
> -		goto err_remove_bitmap;
> +		goto err_remove_vmci_dev_g;
>=20
> 	/* Enable device. */
>=20
> @@ -795,7 +795,7 @@ static int vmci_guest_probe_device(struct pci_dev *pd=
ev,
> 		error =3D pci_alloc_irq_vectors(pdev, 1, 1,
> 				PCI_IRQ_MSIX | PCI_IRQ_MSI | PCI_IRQ_LEGACY);
> 		if (error < 0)
> -			goto err_remove_bitmap;
> +			goto err_unsubscrive_event;
> 	} else {
> 		vmci_dev->exclusive_vectors =3D true;
> 	}
> @@ -871,13 +871,19 @@ static int vmci_guest_probe_device(struct pci_dev *=
pdev,
> err_disable_msi:
> 	pci_free_irq_vectors(pdev);
>=20
> +err_unsubscrive_event:

Please rename this to err_unsubscribe_event

> 	vmci_err =3D vmci_event_unsubscribe(ctx_update_sub_id);
> 	if (vmci_err < VMCI_SUCCESS)
> 		dev_warn(&pdev->dev,
> 			 "Failed to unsubscribe from event (type=3D%d) with subscriber (ID=3D0=
x%x): %d\n",
> 			 VMCI_EVENT_CTX_ID_UPDATE, ctx_update_sub_id, vmci_err);
>=20
> -err_remove_bitmap:
> +err_remove_vmci_dev_g:
> +	spin_lock_irq(&vmci_dev_spinlock);
> +	vmci_pdev =3D NULL;
> +	vmci_dev_g =3D NULL;
> +	spin_unlock_irq(&vmci_dev_spinlock);
> +
> 	if (vmci_dev->notification_bitmap) {
> 		vmci_write_reg(vmci_dev, VMCI_CONTROL_RESET, VMCI_CONTROL_ADDR);
> 		dma_free_coherent(&pdev->dev, PAGE_SIZE,
> @@ -885,12 +891,6 @@ static int vmci_guest_probe_device(struct pci_dev *p=
dev,
> 				  vmci_dev->notification_base);
> 	}
>=20
> -err_remove_vmci_dev_g:
> -	spin_lock_irq(&vmci_dev_spinlock);
> -	vmci_pdev =3D NULL;
> -	vmci_dev_g =3D NULL;
> -	spin_unlock_irq(&vmci_dev_spinlock);
> -
> err_free_data_buffers:
> 	vmci_free_dg_buffers(vmci_dev);
>=20
> --=20
> 2.32.0
>=20