linux-kernel - Re: [PATCH v4 1/3] misc: pci_endpoint_test: Fix the return value of IOCTL

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <Z3Qii43XSnxvc4pp@ryzen>
Date: Tue, 31 Dec 2024 17:57:47 +0100
From: Niklas Cassel <cassel@...nel.org>
To: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
Cc: kw@...ux.com, gregkh@...uxfoundation.org, arnd@...db.de,
	lpieralisi@...nel.org, shuah@...nel.org, kishon@...nel.org,
	linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
	bhelgaas@...gle.com, linux-arm-msm@...r.kernel.org, robh@...nel.org,
	linux-kselftest@...r.kernel.org,
	Damien Le Moal <dlemoal@...nel.org>
Subject: Re: [PATCH v4 1/3] misc: pci_endpoint_test: Fix the return value of
 IOCTL

On Tue, Dec 31, 2024 at 06:43:39PM +0530, Manivannan Sadhasivam wrote:
> IOCTLs are supposed to return 0 for success and negative error codes for
> failure. Currently, this driver is returning 0 for failure and 1 for
> success, that's not correct. Hence, fix it!
> 
> Reported-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> Closes: https://lore.kernel.org/all/YvzNg5ROnxEApDgS@kroah.com
> Fixes: 2c156ac71c6b ("misc: Add host side PCI driver for PCI test function device")
> Reviewed-by: Damien Le Moal <dlemoal@...nel.org>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
> ---
>  drivers/misc/pci_endpoint_test.c | 250 +++++++++++++++----------------
>  tools/pci/pcitest.c              |  51 +++----
>  2 files changed, 148 insertions(+), 153 deletions(-)
> 
> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> index 5c99da952b7a..7d3f78b6f854 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -169,43 +169,47 @@ static void pci_endpoint_test_free_irq_vectors(struct pci_endpoint_test *test)
>  	test->irq_type = IRQ_TYPE_UNDEFINED;
>  }
>  
> -static bool pci_endpoint_test_alloc_irq_vectors(struct pci_endpoint_test *test,
> +static int pci_endpoint_test_alloc_irq_vectors(struct pci_endpoint_test *test,
>  						int type)
>  {
> -	int irq = -1;
> +	int irq;
>  	struct pci_dev *pdev = test->pdev;
>  	struct device *dev = &pdev->dev;
> -	bool res = true;
>  
>  	switch (type) {
>  	case IRQ_TYPE_INTX:
>  		irq = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_INTX);
> -		if (irq < 0)
> +		if (irq < 0) {
>  			dev_err(dev, "Failed to get Legacy interrupt\n");
> +			return -ENOSPC;
> +		}
> +
>  		break;
>  	case IRQ_TYPE_MSI:
>  		irq = pci_alloc_irq_vectors(pdev, 1, 32, PCI_IRQ_MSI);
> -		if (irq < 0)
> +		if (irq < 0) {
>  			dev_err(dev, "Failed to get MSI interrupts\n");
> +			return -ENOSPC;
> +		}
> +
>  		break;
>  	case IRQ_TYPE_MSIX:
>  		irq = pci_alloc_irq_vectors(pdev, 1, 2048, PCI_IRQ_MSIX);
> -		if (irq < 0)
> +		if (irq < 0) {
>  			dev_err(dev, "Failed to get MSI-X interrupts\n");
> +			return -ENOSPC;

>From the pci_alloc_irq_vectors() kdoc:
 * Return: number of allocated vectors (which might be smaller than
 * @max_vecs), -ENOSPC if less than @min_vecs interrupt vectors are
 * available, other errnos otherwise.

So pci_alloc_irq_vectors() can return errors different than ENOSPC,
and in that case, you will overwrite that error.


> @@ -442,9 +444,12 @@ static bool pci_endpoint_test_msi_irq(struct pci_endpoint_test *test,
>  	val = wait_for_completion_timeout(&test->irq_raised,
>  					  msecs_to_jiffies(1000));
>  	if (!val)
> -		return false;
> +		return -ETIMEDOUT;
>  
> -	return pci_irq_vector(pdev, msi_num - 1) == test->last_irq;
> +	if (!(pci_irq_vector(pdev, msi_num - 1) == test->last_irq))

if (pci_irq_vector(pdev, msi_num - 1) != test->last_irq) ?

Or perhaps even:

ret = pci_irq_vector();
if (ret < 0)
	return ret;

if (ret != test->last_irq)
	return -EIO;


Otherwise, this looks good to me:
Reviewed-by: Niklas Cassel <cassel@...nel.org>