Re: [alsa-devel] [PATCH v6 1/6] ASoC: amd:Create multiple I2S platform device endpoint

[Dan Carpenter <dan.carpenter@oracle.com>]

I'm sorry but the probe error handling is totally broken.  It has a
bunch of double frees.  Here is how it should work:

On Fri, Nov 15, 2019 at 05:40:18PM +0530, Ravulapati Vishnu vardhan rao wrote:
>  static int snd_acp3x_probe(struct pci_dev *pci,
>  			   const struct pci_device_id *pci_id)
>  {
> -	int ret;
> -	u32 addr, val;
>  	struct acp3x_dev_data *adata;
> -	struct platform_device_info pdevinfo;
> +	struct platform_device_info pdevinfo[ACP3x_DEVS];
>  	unsigned int irqflags;
> +	int ret;
> +	u32 addr, val, i;

Make i an int.  Loop iterators should be int unless we are going to
loop more than INT_MAX times (which hopefully is seldom in the kernel).

>  
>  	if (pci_enable_device(pci)) {

Ideally this should preserve the error code from pci_enable_device().

	ret = pci_enable_device(pci);
	if (ret)
		return ret;

But 1) you didn't introduce this.  2) This code is basically fine.
This is the first resource allocation so there is no error handling, we
just return ret.  But after this the most recently allocated resource is
'pci enable' if we hit an error now we will want to do
"goto err_pci_disable;"

>  		dev_err(&pci->dev, "pci_enable_device failed\n");
> @@ -43,7 +43,7 @@ static int snd_acp3x_probe(struct pci_dev *pci,

There is a bit where we request regions so now that's the most recent
resource.

>  			     GFP_KERNEL);
>  	if (!adata) {
>  		ret = -ENOMEM;
> -		goto release_regions;
> +		goto adata_free;

We failed to allocate "adata" so we have to release the most recent
resource "goto err_release_regions;".  "adata" is allocated with
devm_kzalloc() so it gets freed automatically after probe().  If we
free it ourselves with kfree() that will lead to a double free.  So
let's remember requet regions still as our most recent allocation.


>  	}
>  
>  	/* check for msi interrupt support */
> @@ -68,11 +68,11 @@ static int snd_acp3x_probe(struct pci_dev *pci,
>  	switch (val) {
>  	case I2S_MODE:
>  		adata->res = devm_kzalloc(&pci->dev,
> -					  sizeof(struct resource) * 2,
> +					  sizeof(struct resource) * 4,
>  					  GFP_KERNEL);
>  		if (!adata->res) {
>  			ret = -ENOMEM;
> -			goto unmap_mmio;
> +			goto release_regions;


unmap_mmio is still the most recent so the labal was correct.  The
advantage of this style of error handling is that when we add new
allocations, we don't have to change a lot of labels.  adata->res uses
devm_kzalloc() again so we keep unmap_mmio in our head as the most
recent allocation.

>  		}
>  
>  		adata->res[0].name = "acp3x_i2s_iomem";
> @@ -80,28 +80,52 @@ static int snd_acp3x_probe(struct pci_dev *pci,
>  		adata->res[0].start = addr;
>  		adata->res[0].end = addr + (ACP3x_REG_END - ACP3x_REG_START);
>  
> -		adata->res[1].name = "acp3x_i2s_irq";
> -		adata->res[1].flags = IORESOURCE_IRQ;
> -		adata->res[1].start = pci->irq;
> -		adata->res[1].end = pci->irq;
> +		adata->res[1].name = "acp3x_i2s_sp";
> +		adata->res[1].flags = IORESOURCE_MEM;
> +		adata->res[1].start = addr + ACP3x_I2STDM_REG_START;
> +		adata->res[1].end = addr + ACP3x_I2STDM_REG_END;
> +
> +		adata->res[2].name = "acp3x_i2s_bt";
> +		adata->res[2].flags = IORESOURCE_MEM;
> +		adata->res[2].start = addr + ACP3x_BT_TDM_REG_START;
> +		adata->res[2].end = addr + ACP3x_BT_TDM_REG_END;
> +
> +		adata->res[3].name = "acp3x_i2s_irq";
> +		adata->res[3].flags = IORESOURCE_IRQ;
> +		adata->res[3].start = pci->irq;
> +		adata->res[3].end = adata->res[3].start;
>  
>  		adata->acp3x_audio_mode = ACP3x_I2S_MODE;
>  
>  		memset(&pdevinfo, 0, sizeof(pdevinfo));
> -		pdevinfo.name = "acp3x_rv_i2s";
> -		pdevinfo.id = 0;
> -		pdevinfo.parent = &pci->dev;
> -		pdevinfo.num_res = 2;
> -		pdevinfo.res = adata->res;
> -		pdevinfo.data = &irqflags;
> -		pdevinfo.size_data = sizeof(irqflags);
> -
> -		adata->pdev = platform_device_register_full(&pdevinfo);
> -		if (IS_ERR(adata->pdev)) {
> -			dev_err(&pci->dev, "cannot register %s device\n",
> -				pdevinfo.name);
> -			ret = PTR_ERR(adata->pdev);
> -			goto unmap_mmio;
> +		pdevinfo[0].name = "acp3x_rv_i2s_dma";
> +		pdevinfo[0].id = 0;
> +		pdevinfo[0].parent = &pci->dev;
> +		pdevinfo[0].num_res = 4;
> +		pdevinfo[0].res = &adata->res[0];
> +		pdevinfo[0].data = &irqflags;
> +		pdevinfo[0].size_data = sizeof(irqflags);
> +
> +		pdevinfo[1].name = "acp3x_i2s_playcap";
> +		pdevinfo[1].id = 0;
> +		pdevinfo[1].parent = &pci->dev;
> +		pdevinfo[1].num_res = 1;
> +		pdevinfo[1].res = &adata->res[1];
> +
> +		pdevinfo[2].name = "acp3x_i2s_playcap";
> +		pdevinfo[2].id = 1;
> +		pdevinfo[2].parent = &pci->dev;
> +		pdevinfo[2].num_res = 1;
> +		pdevinfo[2].res = &adata->res[2];
> +		for (i = 0; i < ACP3x_DEVS ; i++) {
> +			adata->pdev[i] =
> +				platform_device_register_full(&pdevinfo[i]);
> +			if (IS_ERR(adata->pdev[i])) {
> +				dev_err(&pci->dev, "cannot register %s device\n",
> +					pdevinfo[i].name);
> +				ret = PTR_ERR(adata->pdev[i]);
> +				goto unmap_mmio;
> +			}

Loops are a bit complicated.  What we do is if we allocate more than one
thing inside the loop, then we free until the start of the most recent
iteration:
		for (i = 0; i < end; i++) {
			foo->a = alloc();
			if (!foo->a)
				goto unwind_loop;
			foo->b = alloc();
			if (!foo->b) {
				free(foo->a);
				goto unwind_loop;
			}
			foo->c = alloc();
			if (!foo->c) {
				free(foo->b);
				free(foo->a);
				goto unwind_loop;
			}
		}

But this loop only allocates one thing so it's fine.  We can just
goto unwind_loop;

>  		}
>  		break;
>  	default:
> @@ -112,10 +136,22 @@ static int snd_acp3x_probe(struct pci_dev *pci,
>  	return 0;
>  

Then at the end it looks like:

   136          return 0;
   137  
   138  unwind_loop:
   139          if (val == I2S_MODE) {
   140                  while (--i >= 0)
   141                          platform_device_unregister(adata->pdev[i]);
   142          }
   143  unmap_mmio:
   144          iounmap(adata->acp3x_base);
   145  disable_msi:
   146          pci_disable_msi(pci);
   147  release_regions:
   148          pci_release_regions(pci);
   149  disable_pci:
   150          pci_disable_device(pci);
   151  
   152          return ret;
   153  }

Note, that I have an if (val == I2S_MODE).  It's not required but the
unwind code should be a mirror image of the allocation code and it's
better for future proofing.

Also if "i" is unsigned this error handling will break.  We see that
bug with unsigned loop iterators pretty frequently.

regards,
dan carpenter

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel