[{"id":3687339,"web_url":"http://patchwork.ozlabs.org/comment/3687339/","msgid":"<20260506192129.A1E18C2BCB0@smtp.kernel.org>","list_archive_url":null,"date":"2026-05-06T19:21:29","subject":"Re: [PATCH v6 4/6] vfio/pci: Add PCIe TPH enable/disable support","submitter":{"id":93311,"url":"http://patchwork.ozlabs.org/api/people/93311/","name":null,"email":"sashiko-bot@kernel.org"},"content":"Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:\n- [High] Incorrect `argsz` validation in `vfio_pci_tph_enable()` incorrectly adds the structure size twice, breaking the UAPI.\n- [High] Missing locking in `vfio_pci_ioctl_tph()` leads to config space and `pci_dev` bitfield data corruption.\n- [High] Disabling TPH after registering the device exposes it to userspace races.\n- [High] TPH state is not explicitly disabled when the VFIO device is closed, leading to state inconsistencies and subsequent failures.\n--\n\ncommit c8b020c06b71ba6da131d464466e50c2b03f622b\nAuthor: Chengwen Feng \n\nvfio/pci: Add PCIe TPH enable/disable support\n\nThis patch adds support to enable and disable the TPH function with mode\nselection. It restricts the unsafe device-specific TPH mode via a module\nparameter and disables TPH when taking over device ownership.\n\n> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c\n> index d98d04bad4a39..69f666d20c4af 100644\n> --- a/drivers/vfio/pci/vfio_pci_core.c\n> +++ b/drivers/vfio/pci/vfio_pci_core.c\n\n[ ... ]\n\n> @@ -1492,6 +1492,43 @@ static int vfio_pci_tph_get_cap(struct vfio_pci_core_device *vdev,\n> \treturn 0;\n> }\n> \n> +static int vfio_pci_tph_enable(struct vfio_pci_core_device *vdev,\n> +\t\t\t struct vfio_device_pci_tph_op *op,\n> +\t\t\t void __user *uarg)\n> +{\n> +\tstruct pci_dev *pdev = vdev->pdev;\n> +\tstruct vfio_pci_tph_ctrl ctrl;\n> +\tint mode;\n> +\n> +\tif (op->argsz < offsetofend(struct vfio_device_pci_tph_op, ctrl) +\n> +\t\t\tsizeof(struct vfio_pci_tph_ctrl))\n> +\t\treturn -EINVAL;\n\nSince offsetofend(TYPE, MEMBER) already includes the size of the ctrl member,\ndoes adding sizeof(struct vfio_pci_tph_ctrl) a second time require more\nbytes than the actual structure size?\n\nCould this cause the check to reject valid userspace calls that provide the\nexact structure size?\n\n> +\n> +\tif (copy_from_user(&ctrl, uarg, sizeof(ctrl)))\n> +\t\treturn -EFAULT;\n> +\n> +\tif (ctrl.mode != VFIO_PCI_TPH_MODE_IV &&\n> +\t ctrl.mode != VFIO_PCI_TPH_MODE_DS)\n> +\t\treturn -EINVAL;\n> +\n> +\tif (ctrl.mode == VFIO_PCI_TPH_MODE_DS && !enable_unsafe_tph_ds_mode)\n> +\t\treturn -EOPNOTSUPP;\n> +\n> +\t/* Reserved must be zero */\n> +\tif (memchr_inv(ctrl.reserved, 0, sizeof(ctrl.reserved)))\n> +\t\treturn -EINVAL;\n> +\n> +\tmode = (ctrl.mode == VFIO_PCI_TPH_MODE_IV) ? PCI_TPH_ST_IV_MODE :\n> +\t\t\t\t\t\t PCI_TPH_ST_DS_MODE;\n> +\treturn pcie_enable_tph(pdev, mode);\n> +}\n\nIf userspace enables TPH but crashes or closes the file descriptor without\nmanually disabling it, is the TPH state left enabled since the release path\ndoes not call pcie_disable_tph()?\n\nIf the device is opened again, could subsequent attempts to enable TPH fail\nwith -EBUSY because pcie_enable_tph() sees it already enabled?\n\n> +\n> +static int vfio_pci_tph_disable(struct vfio_pci_core_device *vdev)\n> +{\n> +\tpcie_disable_tph(vdev->pdev);\n> +\treturn 0;\n> +}\n> +\n> static int vfio_pci_ioctl_tph(struct vfio_pci_core_device *vdev,\n> \t\t\t void __user *uarg)\n> {\n> @@ -1504,6 +1541,10 @@ static int vfio_pci_ioctl_tph(struct vfio_pci_core_device *vdev,\n> \tswitch (op.op) {\n> \tcase VFIO_PCI_TPH_GET_CAP:\n> \t\treturn vfio_pci_tph_get_cap(vdev, &op, uarg + minsz);\n> +\tcase VFIO_PCI_TPH_ENABLE:\n> +\t\treturn vfio_pci_tph_enable(vdev, &op, uarg + minsz);\n> +\tcase VFIO_PCI_TPH_DISABLE:\n> +\t\treturn vfio_pci_tph_disable(vdev);\n\nBecause vfio_pci_ioctl_tph() handles these ioctls locklessly without\nholding vdev->igate, does concurrently calling pcie_enable_tph() cause a\ndata race on the pdev->tph_enabled bitfield?\n\nCould this concurrently overwrite adjacent critical bitfields in\nstruct pci_dev like msi_enabled or is_managed?\n\n> \tdefault:\n> \t\t/* Other ops are not implemented yet */\n> \t\treturn -EINVAL;\n\n[ ... ]\n\n> @@ -2259,6 +2300,8 @@ int vfio_pci_core_register_device(struct vfio_pci_core_device *vdev)\n> \tret = vfio_register_group_dev(&vdev->vdev);\n> \tif (ret)\n> \t\tgoto out_power;\n> +\t/* Disable TPH when taking over ownership of the device */\n> +\tpcie_disable_tph(pdev);\n\nIs it safe to call pcie_disable_tph() after vfio_register_group_dev()?\n\nSince the device becomes visible to userspace once registered, could a\nuserspace process instantly open the device and enable TPH, racing with this\ninitialization thread and overwriting the user's configuration?\n\nShould hardware state initialization happen before publishing the device to\nuserspace?\n\n> \treturn 0;\n> \n> out_power:","headers":{"Return-Path":"\n ","X-Original-To":["incoming@patchwork.ozlabs.org","linux-pci@vger.kernel.org"],"Delivered-To":"patchwork-incoming@legolas.ozlabs.org","Authentication-Results":["legolas.ozlabs.org;\n\tdkim=pass (2048-bit key;\n unprotected) header.d=kernel.org header.i=@kernel.org header.a=rsa-sha256\n header.s=k20201202 header.b=qux6WVDN;\n\tdkim-atps=neutral","legolas.ozlabs.org;\n spf=pass (sender SPF authorized) smtp.mailfrom=vger.kernel.org\n (client-ip=2600:3c0a:e001:db::12fc:5321; helo=sea.lore.kernel.org;\n envelope-from=linux-pci+bounces-53941-incoming=patchwork.ozlabs.org@vger.kernel.org;\n receiver=patchwork.ozlabs.org)","smtp.subspace.kernel.org;\n\tdkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org\n header.b=\"qux6WVDN\"","smtp.subspace.kernel.org;\n arc=none smtp.client-ip=10.30.226.201"],"Received":["from sea.lore.kernel.org (sea.lore.kernel.org\n [IPv6:2600:3c0a:e001:db::12fc:5321])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\t key-exchange x25519 server-signature ECDSA (secp384r1) server-digest SHA384)\n\t(No client certificate requested)\n\tby legolas.ozlabs.org (Postfix) with ESMTPS id 4g9lcd5k6zz1yJq\n\tfor ; Thu, 07 May 2026 05:21:53 +1000 (AEST)","from smtp.subspace.kernel.org (conduit.subspace.kernel.org\n [100.90.174.1])\n\tby sea.lore.kernel.org (Postfix) with ESMTP id BD9CD3008A58\n\tfor ; Wed, 6 May 2026 19:21:30 +0000 (UTC)","from localhost.localdomain (localhost.localdomain [127.0.0.1])\n\tby smtp.subspace.kernel.org (Postfix) with ESMTP id 4D1BE314A8D;\n\tWed, 6 May 2026 19:21:30 +0000 (UTC)","from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org\n [10.30.226.201])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby smtp.subspace.kernel.org (Postfix) with ESMTPS id 2A3B630AAD8\n\tfor ; Wed, 6 May 2026 19:21:29 +0000 (UTC)","by smtp.kernel.org (Postfix) with ESMTPSA id A1E18C2BCB0;\n\tWed, 6 May 2026 19:21:29 +0000 (UTC)"],"ARC-Seal":"i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116;\n\tt=1778095290; cv=none;\n b=nUss9p+K1TQJ5du+89aIJwzPzTd8bJcKm6Vq2npQgMtVcJ1LJBo6+RxD7eWBXhJruRV2qPX+PQMaI6TYIF+pPMNoGiaYSH7/YPyoAgqKmajblC0TYRf9LRkXeoxdzCCvCgwhSEqRDBzdg1QNht+XmOvsYPMwcvoEoxqfGwBne1A=","ARC-Message-Signature":"i=1; a=rsa-sha256; d=subspace.kernel.org;\n\ts=arc-20240116; t=1778095290; c=relaxed/simple;\n\tbh=VCjxNZC5KsDGrQRuvPvDWXpl9BQzPFlVMO9L0XAf7j8=;\n\th=From:Subject:To:Cc:In-Reply-To:References:Content-Type:Date:\n\t Message-Id;\n b=BUSBUBPXOAgdax5rLUWb1lI4P709OgM47HMYDxcVANJEBn4yVL1KgYR9s2uq5c/SGXwMb407AoHa3/4Ma62u5lKMuLQyTkWzAScGTtjHX50hpO90tRoJgnC+BRiNAXmqEEIjm0Oqmg0kLgkC3ksMQnHZO4aZLipDvgntiey14xY=","ARC-Authentication-Results":"i=1; smtp.subspace.kernel.org;\n dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org\n header.b=qux6WVDN; arc=none smtp.client-ip=10.30.226.201","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org;\n\ts=k20201202; t=1778095289;\n\tbh=VCjxNZC5KsDGrQRuvPvDWXpl9BQzPFlVMO9L0XAf7j8=;\n\th=From:Subject:Reply-To:To:Cc:In-Reply-To:References:Date:From;\n\tb=qux6WVDNaJXjyh2oqABszrnulXMGpj4SEhE6we+NA+03yAtN6k8YIE7oWc2tZopDC\n\t roJeomhUr/LuSBZRNtSXcARhUoT3lddNMbA7miTti0dWQN2jooDjPG5iEZk/1KuYl9\n\t 676CqHrQCbtV2CqO+KTiE+oemDNzenbCYfwNtCWkmcFDdHWWUUE2wnHWNGi6hKM4IZ\n\t N0kj4sFEU0Rd/8oV313i9zFc8dy5n+aSfZ2MnRYinkpH5HVfxYaCvoyUjBy2eM7Osu\n\t guZiwLI35KKRvCbzj2Cn14Z2Pb1c6wfGNHbUp0FQuMdSfLNne8y7ILNJrBDgKPoLlK\n\t ltEeq1M/6aa5Q==","From":"sashiko-bot@kernel.org","Subject":"Re: [PATCH v6 4/6] vfio/pci: Add PCIe TPH enable/disable support","Reply-To":"sashiko@lists.linux.dev","To":"\"Chengwen Feng\" ","Cc":"linux-pci@vger.kernel.org","In-Reply-To":"<20260506094623.29327-5-fengchengwen@huawei.com>","References":"<20260506094623.29327-5-fengchengwen@huawei.com>","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","Date":"Wed, 06 May 2026 19:21:29 +0000","Message-Id":"<20260506192129.A1E18C2BCB0@smtp.kernel.org>","Precedence":"bulk","X-Mailing-List":"linux-pci@vger.kernel.org","List-Id":"","List-Subscribe":"","List-Unsubscribe":""}}]