linux-kernel - Re: [PATCH V4 03/11] vfio/pci: Prepare for dynamic interrupt context storage

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 28 Apr 2023 11:24:03 -0700
From:   Reinette Chatre <reinette.chatre@...el.com>
To:     "Tian, Kevin" <kevin.tian@...el.com>,
        "jgg@...dia.com" <jgg@...dia.com>,
        "yishaih@...dia.com" <yishaih@...dia.com>,
        "shameerali.kolothum.thodi@...wei.com" 
        <shameerali.kolothum.thodi@...wei.com>,
        "alex.williamson@...hat.com" <alex.williamson@...hat.com>
CC:     "tglx@...utronix.de" <tglx@...utronix.de>,
        "darwi@...utronix.de" <darwi@...utronix.de>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "Jiang, Dave" <dave.jiang@...el.com>,
        "Liu, Jing2" <jing2.liu@...el.com>,
        "Raj, Ashok" <ashok.raj@...el.com>,
        "Yu, Fenghua" <fenghua.yu@...el.com>,
        "tom.zanussi@...ux.intel.com" <tom.zanussi@...ux.intel.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V4 03/11] vfio/pci: Prepare for dynamic interrupt context
 storage

Hi Kevin,

On 4/27/2023 11:33 PM, Tian, Kevin wrote:
>> From: Chatre, Reinette <reinette.chatre@...el.com>
>> Sent: Friday, April 28, 2023 1:36 AM
>>
>> @@ -55,17 +80,28 @@ static void vfio_send_intx_eventfd(void *opaque,
>> void *unused)
>>  {
>>  	struct vfio_pci_core_device *vdev = opaque;
>>
>> -	if (likely(is_intx(vdev) && !vdev->virq_disabled))
>> -		eventfd_signal(vdev->ctx[0].trigger, 1);
>> +	if (likely(is_intx(vdev) && !vdev->virq_disabled)) {
>> +		struct vfio_pci_irq_ctx *ctx;
>> +
>> +		ctx = vfio_irq_ctx_get(vdev, 0);
>> +		if (!ctx)
>> +			return;
> 
> if this error happens it implies a kernel bug since the same check
> has been done in vfio_intx_enable(). Then should be a WARN_ON().

Sure. Considering that if these are triggered it may result
in many instances, so perhaps WARN_ON_ONCE()?
 
> ditto for other intx functions which can be called only after intx
> is enabled.

It seems the instances in this category can be identified as the places
where the array contents is currently used without any checks.

I am planning on the following changes:

diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
index d8dae99cf6d9..b549f5c97cb8 100644
--- a/drivers/vfio/pci/vfio_pci_intrs.c
+++ b/drivers/vfio/pci/vfio_pci_intrs.c
@@ -92,7 +92,7 @@ static void vfio_send_intx_eventfd(void *opaque, void *unused)
 		struct vfio_pci_irq_ctx *ctx;
 
 		ctx = vfio_irq_ctx_get(vdev, 0);
-		if (!ctx)
+		if (WARN_ON_ONCE(!ctx))
 			return;
 		eventfd_signal(ctx->trigger, 1);
 	}
@@ -107,7 +107,7 @@ bool vfio_pci_intx_mask(struct vfio_pci_core_device *vdev)
 	bool masked_changed = false;
 
 	ctx = vfio_irq_ctx_get(vdev, 0);
-	if (!ctx)
+	if (WARN_ON_ONCE(!ctx))
 		return masked_changed;
 
 	spin_lock_irqsave(&vdev->irqlock, flags);
@@ -154,7 +154,7 @@ static int vfio_pci_intx_unmask_handler(void *opaque, void *unused)
 	int ret = 0;
 
 	ctx = vfio_irq_ctx_get(vdev, 0);
-	if (!ctx)
+	if (WARN_ON_ONCE(!ctx))
 		return ret;
 
 	spin_lock_irqsave(&vdev->irqlock, flags);
@@ -200,7 +200,7 @@ static irqreturn_t vfio_intx_handler(int irq, void *dev_id)
 	int ret = IRQ_NONE;
 
 	ctx = vfio_irq_ctx_get(vdev, 0);
-	if (!ctx)
+	if (WARN_ON_ONCE(!ctx))
 		return ret;
 
 	spin_lock_irqsave(&vdev->irqlock, flags);
@@ -264,7 +264,7 @@ static int vfio_intx_set_signal(struct vfio_pci_core_device *vdev, int fd)
 	int ret;
 
 	ctx = vfio_irq_ctx_get(vdev, 0);
-	if (!ctx)
+	if (WARN_ON_ONCE(!ctx))
 		return -EINVAL;
 
 	if (ctx->trigger) {
@@ -320,6 +320,7 @@ static void vfio_intx_disable(struct vfio_pci_core_device *vdev)
 
 	dev_dbg(&vdev->pdev->dev, "%s:%d Disabling INTx\n", __func__, __LINE__);
 	ctx = vfio_irq_ctx_get(vdev, 0);
+	WARN_ON_ONCE(!ctx);
 	if (ctx) {
 		vfio_virqfd_disable(&ctx->unmask);
 		vfio_virqfd_disable(&ctx->mask);
@@ -586,7 +587,7 @@ static int vfio_pci_set_intx_unmask(struct vfio_pci_core_device *vdev,
 		struct vfio_pci_irq_ctx *ctx = vfio_irq_ctx_get(vdev, 0);
 		int32_t fd = *(int32_t *)data;
 
-		if (!ctx)
+		if (WARN_ON_ONCE(!ctx))
 			return -EINVAL;
 		if (fd >= 0)
 			return vfio_virqfd_enable((void *) vdev,