LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] staging: rts5208: add check on NULL before dereference
@ 2018-06-09 16:38 Anton Vasilyev
  2018-06-09 16:58 ` okaya
  0 siblings, 1 reply; 16+ messages in thread
From: Anton Vasilyev @ 2018-06-09 16:38 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Anton Vasilyev, Johannes Thumshirn, Gaurav Pathak,
	Hannes Reinecke, Sinan Kaya, devel, linux-kernel, ldv-project

If rtsx_probe fails to allocate dev->chip, then NULL pointer
dereference occurs at rtsx_release_resources().

Patch adds checks chip on NULL before its dereference at
rtsx_release_resources and passing with dereference inside
rtsx_release_chip.

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Anton Vasilyev <vasilyev@ispras.ru>
---
 drivers/staging/rts5208/rtsx.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/rts5208/rtsx.c b/drivers/staging/rts5208/rtsx.c
index 70e0b8623110..952dd0d580cf 100644
--- a/drivers/staging/rts5208/rtsx.c
+++ b/drivers/staging/rts5208/rtsx.c
@@ -623,12 +623,13 @@ static void rtsx_release_resources(struct rtsx_dev *dev)
 
 	if (dev->irq > 0)
 		free_irq(dev->irq, (void *)dev);
-	if (dev->chip->msi_en)
+	if (dev->chip && dev->chip->msi_en)
 		pci_disable_msi(dev->pci);
 	if (dev->remap_addr)
 		iounmap(dev->remap_addr);
+	if (dev->chip)
+		rtsx_release_chip(dev->chip);
 
-	rtsx_release_chip(dev->chip);
 	kfree(dev->chip);
 }
 
-- 
2.17.1

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] staging: rts5208: add check on NULL before dereference
  2018-06-09 16:38 [PATCH] staging: rts5208: add check on NULL before dereference Anton Vasilyev
@ 2018-06-09 16:58 ` okaya
  2018-06-09 19:34   ` Andy Shevchenko
  0 siblings, 1 reply; 16+ messages in thread
From: okaya @ 2018-06-09 16:58 UTC (permalink / raw)
  To: Anton Vasilyev
  Cc: Greg Kroah-Hartman, Johannes Thumshirn, Gaurav Pathak,
	Hannes Reinecke, devel, linux-kernel, ldv-project

On 2018-06-09 12:38, Anton Vasilyev wrote:
> If rtsx_probe fails to allocate dev->chip, then NULL pointer
> dereference occurs at rtsx_release_resources().
> 
> Patch adds checks chip on NULL before its dereference at
> rtsx_release_resources and passing with dereference inside
> rtsx_release_chip.
> 
> Found by Linux Driver Verification project (linuxtesting.org).
> 
> Signed-off-by: Anton Vasilyev <vasilyev@ispras.ru>
> ---
>  drivers/staging/rts5208/rtsx.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/rts5208/rtsx.c 
> b/drivers/staging/rts5208/rtsx.c
> index 70e0b8623110..952dd0d580cf 100644
> --- a/drivers/staging/rts5208/rtsx.c
> +++ b/drivers/staging/rts5208/rtsx.c
> @@ -623,12 +623,13 @@ static void rtsx_release_resources(struct 
> rtsx_dev *dev)
> 

I think you should bail out if dev->chip is null rather than adding 
conditiinals.


>  	if (dev->irq > 0)
>  		free_irq(dev->irq, (void *)dev);
> -	if (dev->chip->msi_en)
> +	if (dev->chip && dev->chip->msi_en)
>  		pci_disable_msi(dev->pci);
>  	if (dev->remap_addr)
>  		iounmap(dev->remap_addr);
> +	if (dev->chip)
> +		rtsx_release_chip(dev->chip);
> 
> -	rtsx_release_chip(dev->chip);
>  	kfree(dev->chip);
>  }

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] staging: rts5208: add check on NULL before dereference
  2018-06-09 16:58 ` okaya
@ 2018-06-09 19:34   ` Andy Shevchenko
  2018-06-09 22:22     ` okaya
  2018-06-12 13:06     ` Dan Carpenter
  0 siblings, 2 replies; 16+ messages in thread
From: Andy Shevchenko @ 2018-06-09 19:34 UTC (permalink / raw)
  To: Sinan Kaya
  Cc: Anton Vasilyev, Greg Kroah-Hartman, Johannes Thumshirn,
	Gaurav Pathak, Hannes Reinecke, devel, Linux Kernel Mailing List,
	ldv-project

On Sat, Jun 9, 2018 at 7:58 PM,  <okaya@codeaurora.org> wrote:
> On 2018-06-09 12:38, Anton Vasilyev wrote:
>>
>> If rtsx_probe fails to allocate dev->chip, then NULL pointer
>> dereference occurs at rtsx_release_resources().
>>
>> Patch adds checks chip on NULL before its dereference at
>> rtsx_release_resources and passing with dereference inside
>> rtsx_release_chip.
>>
>> Found by Linux Driver Verification project (linuxtesting.org).

> I think you should bail out if dev->chip is null rather than adding
> conditiinals.

I'm wondering if it's false positive. At which circumstances that may happen?

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] staging: rts5208: add check on NULL before dereference
  2018-06-09 19:34   ` Andy Shevchenko
@ 2018-06-09 22:22     ` okaya
  2018-06-12 13:06     ` Dan Carpenter
  1 sibling, 0 replies; 16+ messages in thread
From: okaya @ 2018-06-09 22:22 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Anton Vasilyev, Greg Kroah-Hartman, Johannes Thumshirn,
	Gaurav Pathak, Hannes Reinecke, devel, Linux Kernel Mailing List,
	ldv-project

On 2018-06-09 15:34, Andy Shevchenko wrote:
> On Sat, Jun 9, 2018 at 7:58 PM,  <okaya@codeaurora.org> wrote:
>> On 2018-06-09 12:38, Anton Vasilyev wrote:
>>> 
>>> If rtsx_probe fails to allocate dev->chip, then NULL pointer
>>> dereference occurs at rtsx_release_resources().
>>> 
>>> Patch adds checks chip on NULL before its dereference at
>>> rtsx_release_resources and passing with dereference inside
>>> rtsx_release_chip.
>>> 
>>> Found by Linux Driver Verification project (linuxtesting.org).
> 
>> I think you should bail out if dev->chip is null rather than adding
>> conditiinals.
> 
> I'm wondering if it's false positive. At which circumstances that may 
> happen?

Only if dev->chip allocation fails. Code tries to cleanup prior 
resources by calling clean_everything() function which ends up in 
rtsx_release_resources()

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] staging: rts5208: add check on NULL before dereference
  2018-06-09 19:34   ` Andy Shevchenko
  2018-06-09 22:22     ` okaya
@ 2018-06-12 13:06     ` Dan Carpenter
  2018-06-13 16:55       ` [PATCH v2] " Anton Vasilyev
  1 sibling, 1 reply; 16+ messages in thread
From: Dan Carpenter @ 2018-06-12 13:06 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Sinan Kaya, devel, ldv-project, Greg Kroah-Hartman,
	Johannes Thumshirn, Linux Kernel Mailing List, Hannes Reinecke,
	Gaurav Pathak, Anton Vasilyev

On Sat, Jun 09, 2018 at 10:34:43PM +0300, Andy Shevchenko wrote:
> On Sat, Jun 9, 2018 at 7:58 PM,  <okaya@codeaurora.org> wrote:
> > On 2018-06-09 12:38, Anton Vasilyev wrote:
> >>
> >> If rtsx_probe fails to allocate dev->chip, then NULL pointer
> >> dereference occurs at rtsx_release_resources().
> >>
> >> Patch adds checks chip on NULL before its dereference at
> >> rtsx_release_resources and passing with dereference inside
> >> rtsx_release_chip.
> >>
> >> Found by Linux Driver Verification project (linuxtesting.org).
> 
> > I think you should bail out if dev->chip is null rather than adding
> > conditiinals.
> 
> I'm wondering if it's false positive. At which circumstances that may happen?
> 


Here's how the code looks like in rtsx_probe().

   972          /* We come here if there are any problems */
   973  errout:
   974          dev_err(&pci->dev, "%s failed\n", __func__);
   975          release_everything(dev);
   976  
   977          return err;
   978  }

Do everything error handling is error prone because you're trying to
free some things which haven't been allocated.  It's also more
complicated so it leads to leaks.

The correct way to do error handling is to have a series of labels which
undo one thing at a time.  The labels should be named properly so that
you can tell what the goto does such as "goto err_release_foo;".  That
way you never have to worry about freeing things which haven't been
allocated.  As you read the code, you just have to track the most
recently allocated resource and verify that the goto does what is
expected so you avoid leaks.

In this example:

   857          dev->chip = kzalloc(sizeof(*dev->chip), GFP_KERNEL);
   858          if (!dev->chip) {
   859                  err = -ENOMEM;
   860                  goto errout;
   861          }
   862  
   863          spin_lock_init(&dev->reg_lock);
   864          mutex_init(&dev->dev_mutex);
   865          init_completion(&dev->cmnd_ready);
   866          init_completion(&dev->control_exit);
   867          init_completion(&dev->polling_exit);
   868          init_completion(&dev->notify);
   869          init_completion(&dev->scanning_done);
   870          init_waitqueue_head(&dev->delay_wait);

If the kzalloc() fails, then we call release_everything() with
"dev->chip" NULL.  But we'll actually crash before we hit the NULL
dereference because we didn't do the init_completion(&dev->cmnd_ready);

So this patch doesn't really fix anything because do everything error
handling is a hopeless approach.

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v2] staging: rts5208: add check on NULL before dereference
  2018-06-12 13:06     ` Dan Carpenter
@ 2018-06-13 16:55       ` Anton Vasilyev
  2018-06-13 17:00         ` Andy Shevchenko
  0 siblings, 1 reply; 16+ messages in thread
From: Anton Vasilyev @ 2018-06-13 16:55 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Anton Vasilyev, Andy Shevchenko, Sinan Kaya, Johannes Thumshirn,
	Gaurav Pathak, Hannes Reinecke, devel, linux-kernel, ldv-project

If rtsx_probe() fails to allocate dev->chip, then NULL pointer
dereference occurs at release_everything()->rtsx_release_resources().

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Anton Vasilyev <vasilyev@ispras.ru>
---
v2: Add error handling into rtsx_probe based on Dan Carpenter's comment.
I do not have corresponding hardware, so patch was tested by compilation only.

I faced with inaccuracy at rtsx_remove() and original rtsx_probe():
there is quiesce_and_remove_host() call with scsi_remove_host() inside,
whereas release_everything() calls scsi_host_put() after this
scsi_remove_host() call. This is strange for me.

Also I do not know is it require to check result value of
rtsx_init_chip() call on rtsx_probe().
---
 drivers/staging/rts5208/rtsx.c | 38 +++++++++++++++++++++++-----------
 1 file changed, 26 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/rts5208/rtsx.c b/drivers/staging/rts5208/rtsx.c
index 70e0b8623110..69e6abe14abf 100644
--- a/drivers/staging/rts5208/rtsx.c
+++ b/drivers/staging/rts5208/rtsx.c
@@ -857,7 +857,7 @@ static int rtsx_probe(struct pci_dev *pci,
 	dev->chip = kzalloc(sizeof(*dev->chip), GFP_KERNEL);
 	if (!dev->chip) {
 		err = -ENOMEM;
-		goto errout;
+		goto chip_alloc_fail;
 	}
 
 	spin_lock_init(&dev->reg_lock);
@@ -879,7 +879,7 @@ static int rtsx_probe(struct pci_dev *pci,
 	if (!dev->remap_addr) {
 		dev_err(&pci->dev, "ioremap error\n");
 		err = -ENXIO;
-		goto errout;
+		goto ioremap_fail;
 	}
 
 	/*
@@ -894,7 +894,7 @@ static int rtsx_probe(struct pci_dev *pci,
 	if (!dev->rtsx_resv_buf) {
 		dev_err(&pci->dev, "alloc dma buffer fail\n");
 		err = -ENXIO;
-		goto errout;
+		goto dma_alloc_fail;
 	}
 	dev->chip->host_cmds_ptr = dev->rtsx_resv_buf;
 	dev->chip->host_cmds_addr = dev->rtsx_resv_buf_addr;
@@ -915,7 +915,7 @@ static int rtsx_probe(struct pci_dev *pci,
 
 	if (rtsx_acquire_irq(dev) < 0) {
 		err = -EBUSY;
-		goto errout;
+		goto irq_acquire_fail;
 	}
 
 	pci_set_master(pci);
@@ -935,14 +935,14 @@ static int rtsx_probe(struct pci_dev *pci,
 	if (IS_ERR(th)) {
 		dev_err(&pci->dev, "Unable to start control thread\n");
 		err = PTR_ERR(th);
-		goto errout;
+		goto control_thread_fail;
 	}
 	dev->ctl_thread = th;
 
 	err = scsi_add_host(host, &pci->dev);
 	if (err) {
 		dev_err(&pci->dev, "Unable to add the scsi host\n");
-		goto errout;
+		goto scsi_add_host_fail;
 	}
 
 	/* Start up the thread for delayed SCSI-device scanning */
@@ -950,18 +950,16 @@ static int rtsx_probe(struct pci_dev *pci,
 	if (IS_ERR(th)) {
 		dev_err(&pci->dev, "Unable to start the device-scanning thread\n");
 		complete(&dev->scanning_done);
-		quiesce_and_remove_host(dev);
 		err = PTR_ERR(th);
-		goto errout;
+		goto scan_thread_fail;
 	}
 
 	/* Start up the thread for polling thread */
 	th = kthread_run(rtsx_polling_thread, dev, "rtsx-polling");
 	if (IS_ERR(th)) {
 		dev_err(&pci->dev, "Unable to start the device-polling thread\n");
-		quiesce_and_remove_host(dev);
 		err = PTR_ERR(th);
-		goto errout;
+		goto scan_thread_fail;
 	}
 	dev->polling_thread = th;
 
@@ -970,9 +968,25 @@ static int rtsx_probe(struct pci_dev *pci,
 	return 0;
 
 	/* We come here if there are any problems */
-errout:
+scan_thread_fail:
+	quiesce_and_remove_host(dev);
+scsi_add_host_fail:
+	complete(&dev->cmnd_ready);
+	wait_for_completion(&dev->control_exit);
+control_thread_fail:
+	free_irq(dev->irq, (void *)dev);
+	rtsx_release_chip(dev->chip);
+irq_acquire_fail:
+	dev->chip->host_cmds_ptr = NULL;
+	dev->chip->host_sg_tbl_ptr = NULL;
+	if (dev->chip->msi_en)
+		pci_disable_msi(dev->pci);
+dma_alloc_fail:
+	iounmap(dev->remap_addr);
+ioremap_fail:
+	kfree(dev->chip);
+chip_alloc_fail:
 	dev_err(&pci->dev, "%s failed\n", __func__);
-	release_everything(dev);
 
 	return err;
 }
-- 
2.17.1


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] staging: rts5208: add check on NULL before dereference
  2018-06-13 16:55       ` [PATCH v2] " Anton Vasilyev
@ 2018-06-13 17:00         ` Andy Shevchenko
  2018-06-13 17:34           ` [PATCH v3] staging: rts5208: add error handling into rtsx_probe Anton Vasilyev
       [not found]           ` <20180613173128.32384-1-vasilyev@ispras.ru>
  0 siblings, 2 replies; 16+ messages in thread
From: Andy Shevchenko @ 2018-06-13 17:00 UTC (permalink / raw)
  To: Anton Vasilyev
  Cc: Dan Carpenter, Sinan Kaya, Johannes Thumshirn, Gaurav Pathak,
	Hannes Reinecke, devel, Linux Kernel Mailing List, ldv-project

On Wed, Jun 13, 2018 at 7:55 PM, Anton Vasilyev <vasilyev@ispras.ru> wrote:
> If rtsx_probe() fails to allocate dev->chip, then NULL pointer
> dereference occurs at release_everything()->rtsx_release_resources().
>
> Found by Linux Driver Verification project (linuxtesting.org).
>

You forgot to adjust subject and commit message to the new reality
which is brought by this change.

> Signed-off-by: Anton Vasilyev <vasilyev@ispras.ru>
> ---
> v2: Add error handling into rtsx_probe based on Dan Carpenter's comment.
> I do not have corresponding hardware, so patch was tested by compilation only.
>
> I faced with inaccuracy at rtsx_remove() and original rtsx_probe():
> there is quiesce_and_remove_host() call with scsi_remove_host() inside,
> whereas release_everything() calls scsi_host_put() after this
> scsi_remove_host() call. This is strange for me.
>
> Also I do not know is it require to check result value of
> rtsx_init_chip() call on rtsx_probe().
> ---
>  drivers/staging/rts5208/rtsx.c | 38 +++++++++++++++++++++++-----------
>  1 file changed, 26 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/staging/rts5208/rtsx.c b/drivers/staging/rts5208/rtsx.c
> index 70e0b8623110..69e6abe14abf 100644
> --- a/drivers/staging/rts5208/rtsx.c
> +++ b/drivers/staging/rts5208/rtsx.c
> @@ -857,7 +857,7 @@ static int rtsx_probe(struct pci_dev *pci,
>         dev->chip = kzalloc(sizeof(*dev->chip), GFP_KERNEL);
>         if (!dev->chip) {
>                 err = -ENOMEM;
> -               goto errout;
> +               goto chip_alloc_fail;
>         }
>
>         spin_lock_init(&dev->reg_lock);
> @@ -879,7 +879,7 @@ static int rtsx_probe(struct pci_dev *pci,
>         if (!dev->remap_addr) {
>                 dev_err(&pci->dev, "ioremap error\n");
>                 err = -ENXIO;
> -               goto errout;
> +               goto ioremap_fail;
>         }
>
>         /*
> @@ -894,7 +894,7 @@ static int rtsx_probe(struct pci_dev *pci,
>         if (!dev->rtsx_resv_buf) {
>                 dev_err(&pci->dev, "alloc dma buffer fail\n");
>                 err = -ENXIO;
> -               goto errout;
> +               goto dma_alloc_fail;
>         }
>         dev->chip->host_cmds_ptr = dev->rtsx_resv_buf;
>         dev->chip->host_cmds_addr = dev->rtsx_resv_buf_addr;
> @@ -915,7 +915,7 @@ static int rtsx_probe(struct pci_dev *pci,
>
>         if (rtsx_acquire_irq(dev) < 0) {
>                 err = -EBUSY;
> -               goto errout;
> +               goto irq_acquire_fail;
>         }
>
>         pci_set_master(pci);
> @@ -935,14 +935,14 @@ static int rtsx_probe(struct pci_dev *pci,
>         if (IS_ERR(th)) {
>                 dev_err(&pci->dev, "Unable to start control thread\n");
>                 err = PTR_ERR(th);
> -               goto errout;
> +               goto control_thread_fail;
>         }
>         dev->ctl_thread = th;
>
>         err = scsi_add_host(host, &pci->dev);
>         if (err) {
>                 dev_err(&pci->dev, "Unable to add the scsi host\n");
> -               goto errout;
> +               goto scsi_add_host_fail;
>         }
>
>         /* Start up the thread for delayed SCSI-device scanning */
> @@ -950,18 +950,16 @@ static int rtsx_probe(struct pci_dev *pci,
>         if (IS_ERR(th)) {
>                 dev_err(&pci->dev, "Unable to start the device-scanning thread\n");
>                 complete(&dev->scanning_done);
> -               quiesce_and_remove_host(dev);
>                 err = PTR_ERR(th);
> -               goto errout;
> +               goto scan_thread_fail;
>         }
>
>         /* Start up the thread for polling thread */
>         th = kthread_run(rtsx_polling_thread, dev, "rtsx-polling");
>         if (IS_ERR(th)) {
>                 dev_err(&pci->dev, "Unable to start the device-polling thread\n");
> -               quiesce_and_remove_host(dev);
>                 err = PTR_ERR(th);
> -               goto errout;
> +               goto scan_thread_fail;
>         }
>         dev->polling_thread = th;
>
> @@ -970,9 +968,25 @@ static int rtsx_probe(struct pci_dev *pci,
>         return 0;
>
>         /* We come here if there are any problems */
> -errout:
> +scan_thread_fail:
> +       quiesce_and_remove_host(dev);
> +scsi_add_host_fail:
> +       complete(&dev->cmnd_ready);
> +       wait_for_completion(&dev->control_exit);
> +control_thread_fail:
> +       free_irq(dev->irq, (void *)dev);
> +       rtsx_release_chip(dev->chip);
> +irq_acquire_fail:
> +       dev->chip->host_cmds_ptr = NULL;
> +       dev->chip->host_sg_tbl_ptr = NULL;
> +       if (dev->chip->msi_en)
> +               pci_disable_msi(dev->pci);
> +dma_alloc_fail:
> +       iounmap(dev->remap_addr);
> +ioremap_fail:
> +       kfree(dev->chip);
> +chip_alloc_fail:
>         dev_err(&pci->dev, "%s failed\n", __func__);
> -       release_everything(dev);
>
>         return err;
>  }
> --
> 2.17.1
>



-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v3] staging: rts5208: add error handling into rtsx_probe
  2018-06-13 17:00         ` Andy Shevchenko
@ 2018-06-13 17:34           ` Anton Vasilyev
       [not found]           ` <20180613173128.32384-1-vasilyev@ispras.ru>
  1 sibling, 0 replies; 16+ messages in thread
From: Anton Vasilyev @ 2018-06-13 17:34 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Anton Vasilyev, Dan Carpenter, Sinan Kaya, Johannes Thumshirn,
	Gaurav Pathak, Hannes Reinecke, devel, linux-kernel, ldv-project

If rtsx_probe() fails to allocate dev->chip, then release_everything()
will crash on uninitialized dev->cmnd_ready complete.

Patch adds error handling into rtsx_probe.

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Anton Vasilyev <vasilyev@ispras.ru>
---
v3: fix subject and commit message
v2: Add error handling into rtsx_probe based on Dan Carpenter's comment.
I do not have corresponding hardware, so patch was tested by compilation only.

I faced with inaccuracy at rtsx_remove() and original rtsx_probe():
there is quiesce_and_remove_host() call with scsi_remove_host() inside,
whereas release_everything() calls scsi_host_put() after this
scsi_remove_host() call. This is strange for me.
Also I do not know is it require to check result value of
rtsx_init_chip() call on rtsx_probe().
---
 drivers/staging/rts5208/rtsx.c | 38 +++++++++++++++++++++++-----------
 1 file changed, 26 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/rts5208/rtsx.c b/drivers/staging/rts5208/rtsx.c
index 70e0b8623110..69e6abe14abf 100644
--- a/drivers/staging/rts5208/rtsx.c
+++ b/drivers/staging/rts5208/rtsx.c
@@ -857,7 +857,7 @@ static int rtsx_probe(struct pci_dev *pci,
 	dev->chip = kzalloc(sizeof(*dev->chip), GFP_KERNEL);
 	if (!dev->chip) {
 		err = -ENOMEM;
-		goto errout;
+		goto chip_alloc_fail;
 	}
 
 	spin_lock_init(&dev->reg_lock);
@@ -879,7 +879,7 @@ static int rtsx_probe(struct pci_dev *pci,
 	if (!dev->remap_addr) {
 		dev_err(&pci->dev, "ioremap error\n");
 		err = -ENXIO;
-		goto errout;
+		goto ioremap_fail;
 	}
 
 	/*
@@ -894,7 +894,7 @@ static int rtsx_probe(struct pci_dev *pci,
 	if (!dev->rtsx_resv_buf) {
 		dev_err(&pci->dev, "alloc dma buffer fail\n");
 		err = -ENXIO;
-		goto errout;
+		goto dma_alloc_fail;
 	}
 	dev->chip->host_cmds_ptr = dev->rtsx_resv_buf;
 	dev->chip->host_cmds_addr = dev->rtsx_resv_buf_addr;
@@ -915,7 +915,7 @@ static int rtsx_probe(struct pci_dev *pci,
 
 	if (rtsx_acquire_irq(dev) < 0) {
 		err = -EBUSY;
-		goto errout;
+		goto irq_acquire_fail;
 	}
 
 	pci_set_master(pci);
@@ -935,14 +935,14 @@ static int rtsx_probe(struct pci_dev *pci,
 	if (IS_ERR(th)) {
 		dev_err(&pci->dev, "Unable to start control thread\n");
 		err = PTR_ERR(th);
-		goto errout;
+		goto control_thread_fail;
 	}
 	dev->ctl_thread = th;
 
 	err = scsi_add_host(host, &pci->dev);
 	if (err) {
 		dev_err(&pci->dev, "Unable to add the scsi host\n");
-		goto errout;
+		goto scsi_add_host_fail;
 	}
 
 	/* Start up the thread for delayed SCSI-device scanning */
@@ -950,18 +950,16 @@ static int rtsx_probe(struct pci_dev *pci,
 	if (IS_ERR(th)) {
 		dev_err(&pci->dev, "Unable to start the device-scanning thread\n");
 		complete(&dev->scanning_done);
-		quiesce_and_remove_host(dev);
 		err = PTR_ERR(th);
-		goto errout;
+		goto scan_thread_fail;
 	}
 
 	/* Start up the thread for polling thread */
 	th = kthread_run(rtsx_polling_thread, dev, "rtsx-polling");
 	if (IS_ERR(th)) {
 		dev_err(&pci->dev, "Unable to start the device-polling thread\n");
-		quiesce_and_remove_host(dev);
 		err = PTR_ERR(th);
-		goto errout;
+		goto scan_thread_fail;
 	}
 	dev->polling_thread = th;
 
@@ -970,9 +968,25 @@ static int rtsx_probe(struct pci_dev *pci,
 	return 0;
 
 	/* We come here if there are any problems */
-errout:
+scan_thread_fail:
+	quiesce_and_remove_host(dev);
+scsi_add_host_fail:
+	complete(&dev->cmnd_ready);
+	wait_for_completion(&dev->control_exit);
+control_thread_fail:
+	free_irq(dev->irq, (void *)dev);
+	rtsx_release_chip(dev->chip);
+irq_acquire_fail:
+	dev->chip->host_cmds_ptr = NULL;
+	dev->chip->host_sg_tbl_ptr = NULL;
+	if (dev->chip->msi_en)
+		pci_disable_msi(dev->pci);
+dma_alloc_fail:
+	iounmap(dev->remap_addr);
+ioremap_fail:
+	kfree(dev->chip);
+chip_alloc_fail:
 	dev_err(&pci->dev, "%s failed\n", __func__);
-	release_everything(dev);
 
 	return err;
 }
-- 
2.17.1


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: your mail
       [not found]           ` <20180613173128.32384-1-vasilyev@ispras.ru>
@ 2018-06-19  7:42             ` Dan Carpenter
  2018-06-19 15:25               ` [PATCH v4] staging: rts5208: add error handling into rtsx_probe Anton Vasilyev
  0 siblings, 1 reply; 16+ messages in thread
From: Dan Carpenter @ 2018-06-19  7:42 UTC (permalink / raw)
  To: Anton Vasilyev
  Cc: Andy Shevchenko, devel, ldv-project, Johannes Thumshirn,
	linux-kernel, Sinan Kaya, Hannes Reinecke, Gaurav Pathak

Thanks for this.  This is a lot of work.

On Wed, Jun 13, 2018 at 08:31:28PM +0300, Anton Vasilyev wrote:
> diff --git a/drivers/staging/rts5208/rtsx.c b/drivers/staging/rts5208/rtsx.c
> index 70e0b8623110..69e6abe14abf 100644
> --- a/drivers/staging/rts5208/rtsx.c
> +++ b/drivers/staging/rts5208/rtsx.c
> @@ -857,7 +857,7 @@ static int rtsx_probe(struct pci_dev *pci,
>  	dev->chip = kzalloc(sizeof(*dev->chip), GFP_KERNEL);
>  	if (!dev->chip) {
>  		err = -ENOMEM;
> -		goto errout;
> +		goto chip_alloc_fail;

The most recent successful allocation is scsi_host_alloc().  I was
really hoping this would say something like "goto err_free_host;" or
something.  The naming style here is a "come from" label which doesn't
say if it's going to free the scsi host or not...  It turns out we don't
free the the host, but we should:

err_put_host:
	scsi_host_put(host);

The kzalloc() has it's own error message built in, and all the other
error paths as well so the dev_err() is not super important to me...

Killing the threads seems actually really complicated so maybe we should
just have a separate error paths for that.  I'm not sure...

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v4] staging: rts5208: add error handling into rtsx_probe
  2018-06-19  7:42             ` your mail Dan Carpenter
@ 2018-06-19 15:25               ` Anton Vasilyev
  2018-06-19 17:13                 ` Andy Shevchenko
  0 siblings, 1 reply; 16+ messages in thread
From: Anton Vasilyev @ 2018-06-19 15:25 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Anton Vasilyev, Greg Kroah-Hartman, Andy Shevchenko, Sinan Kaya,
	Johannes Thumshirn, Gaurav Pathak, Hannes Reinecke, devel,
	linux-kernel, ldv-project

If rtsx_probe() fails to allocate dev->chip, then release_everything()
will crash on uninitialized dev->cmnd_ready complete

Patch adds error handling into rtsx_probe.

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Anton Vasilyev <vasilyev@ispras.ru>
---
v4: rename labels baced on Dan Carpenter's recommendation
v3: fix subject and commit message
v2: Add error handling into rtsx_probe based on Dan Carpenter's comment.
I do not have corresponding hardware, so patch was tested by compilation only.

I faced with inaccuracy at rtsx_remove() and original rtsx_probe():
there is quiesce_and_remove_host() call with scsi_remove_host() inside,
whereas release_everything() calls scsi_host_put() after this
scsi_remove_host() call. This is strange for me.
Also I do not know is it require to check result value of
rtsx_init_chip() call on rtsx_probe().
---
 drivers/staging/rts5208/rtsx.c | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/rts5208/rtsx.c b/drivers/staging/rts5208/rtsx.c
index 70e0b8623110..233026ee5dd4 100644
--- a/drivers/staging/rts5208/rtsx.c
+++ b/drivers/staging/rts5208/rtsx.c
@@ -879,7 +879,7 @@ static int rtsx_probe(struct pci_dev *pci,
 	if (!dev->remap_addr) {
 		dev_err(&pci->dev, "ioremap error\n");
 		err = -ENXIO;
-		goto errout;
+		goto err_chip_free;
 	}
 
 	/*
@@ -894,7 +894,7 @@ static int rtsx_probe(struct pci_dev *pci,
 	if (!dev->rtsx_resv_buf) {
 		dev_err(&pci->dev, "alloc dma buffer fail\n");
 		err = -ENXIO;
-		goto errout;
+		goto err_addr_unmap;
 	}
 	dev->chip->host_cmds_ptr = dev->rtsx_resv_buf;
 	dev->chip->host_cmds_addr = dev->rtsx_resv_buf_addr;
@@ -915,7 +915,7 @@ static int rtsx_probe(struct pci_dev *pci,
 
 	if (rtsx_acquire_irq(dev) < 0) {
 		err = -EBUSY;
-		goto errout;
+		goto err_disable_msi;
 	}
 
 	pci_set_master(pci);
@@ -935,14 +935,14 @@ static int rtsx_probe(struct pci_dev *pci,
 	if (IS_ERR(th)) {
 		dev_err(&pci->dev, "Unable to start control thread\n");
 		err = PTR_ERR(th);
-		goto errout;
+		goto err_rtsx_release;
 	}
 	dev->ctl_thread = th;
 
 	err = scsi_add_host(host, &pci->dev);
 	if (err) {
 		dev_err(&pci->dev, "Unable to add the scsi host\n");
-		goto errout;
+		goto err_complete_control_thread;
 	}
 
 	/* Start up the thread for delayed SCSI-device scanning */
@@ -950,18 +950,16 @@ static int rtsx_probe(struct pci_dev *pci,
 	if (IS_ERR(th)) {
 		dev_err(&pci->dev, "Unable to start the device-scanning thread\n");
 		complete(&dev->scanning_done);
-		quiesce_and_remove_host(dev);
 		err = PTR_ERR(th);
-		goto errout;
+		goto err_stop_host;
 	}
 
 	/* Start up the thread for polling thread */
 	th = kthread_run(rtsx_polling_thread, dev, "rtsx-polling");
 	if (IS_ERR(th)) {
 		dev_err(&pci->dev, "Unable to start the device-polling thread\n");
-		quiesce_and_remove_host(dev);
 		err = PTR_ERR(th);
-		goto errout;
+		goto err_stop_host;
 	}
 	dev->polling_thread = th;
 
@@ -970,9 +968,25 @@ static int rtsx_probe(struct pci_dev *pci,
 	return 0;
 
 	/* We come here if there are any problems */
+err_stop_host:
+	quiesce_and_remove_host(dev);
+err_complete_control_thread:
+	complete(&dev->cmnd_ready);
+	wait_for_completion(&dev->control_exit);
+err_rtsx_release:
+	free_irq(dev->irq, (void *)dev);
+	rtsx_release_chip(dev->chip);
+err_disable_msi:
+	dev->chip->host_cmds_ptr = NULL;
+	dev->chip->host_sg_tbl_ptr = NULL;
+	if (dev->chip->msi_en)
+		pci_disable_msi(dev->pci);
+err_addr_unmap:
+	iounmap(dev->remap_addr);
+err_chip_free:
+	kfree(dev->chip);
 errout:
 	dev_err(&pci->dev, "%s failed\n", __func__);
-	release_everything(dev);
 
 	return err;
 }
-- 
2.17.1


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v4] staging: rts5208: add error handling into rtsx_probe
  2018-06-19 15:25               ` [PATCH v4] staging: rts5208: add error handling into rtsx_probe Anton Vasilyev
@ 2018-06-19 17:13                 ` Andy Shevchenko
  2018-08-01 11:55                   ` [PATCH v5] " Anton Vasilyev
  0 siblings, 1 reply; 16+ messages in thread
From: Andy Shevchenko @ 2018-06-19 17:13 UTC (permalink / raw)
  To: Anton Vasilyev
  Cc: Dan Carpenter, Greg Kroah-Hartman, Sinan Kaya,
	Johannes Thumshirn, Gaurav Pathak, Hannes Reinecke, devel,
	Linux Kernel Mailing List, ldv-project

On Tue, Jun 19, 2018 at 6:25 PM, Anton Vasilyev <vasilyev@ispras.ru> wrote:
> If rtsx_probe() fails to allocate dev->chip, then release_everything()
> will crash on uninitialized dev->cmnd_ready complete

Period is missed at the end.

>
> Patch adds error handling into rtsx_probe.

an error

>
> Found by Linux Driver Verification project (linuxtesting.org).
>

Reviewed-by: Andy Shevchenko <andy.shevhchenko@gmail.com>

Couple of comments below though.

> Signed-off-by: Anton Vasilyev <vasilyev@ispras.ru>
> ---
> v4: rename labels baced on Dan Carpenter's recommendation
> v3: fix subject and commit message
> v2: Add error handling into rtsx_probe based on Dan Carpenter's comment.
> I do not have corresponding hardware, so patch was tested by compilation only.
>
> I faced with inaccuracy at rtsx_remove() and original rtsx_probe():
> there is quiesce_and_remove_host() call with scsi_remove_host() inside,
> whereas release_everything() calls scsi_host_put() after this
> scsi_remove_host() call. This is strange for me.
> Also I do not know is it require to check result value of
> rtsx_init_chip() call on rtsx_probe().
> ---
>  drivers/staging/rts5208/rtsx.c | 34 ++++++++++++++++++++++++----------
>  1 file changed, 24 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/staging/rts5208/rtsx.c b/drivers/staging/rts5208/rtsx.c
> index 70e0b8623110..233026ee5dd4 100644
> --- a/drivers/staging/rts5208/rtsx.c
> +++ b/drivers/staging/rts5208/rtsx.c
> @@ -879,7 +879,7 @@ static int rtsx_probe(struct pci_dev *pci,
>         if (!dev->remap_addr) {
>                 dev_err(&pci->dev, "ioremap error\n");
>                 err = -ENXIO;
> -               goto errout;
> +               goto err_chip_free;
>         }
>
>         /*
> @@ -894,7 +894,7 @@ static int rtsx_probe(struct pci_dev *pci,
>         if (!dev->rtsx_resv_buf) {
>                 dev_err(&pci->dev, "alloc dma buffer fail\n");
>                 err = -ENXIO;
> -               goto errout;
> +               goto err_addr_unmap;
>         }
>         dev->chip->host_cmds_ptr = dev->rtsx_resv_buf;
>         dev->chip->host_cmds_addr = dev->rtsx_resv_buf_addr;
> @@ -915,7 +915,7 @@ static int rtsx_probe(struct pci_dev *pci,
>
>         if (rtsx_acquire_irq(dev) < 0) {
>                 err = -EBUSY;
> -               goto errout;
> +               goto err_disable_msi;
>         }
>
>         pci_set_master(pci);
> @@ -935,14 +935,14 @@ static int rtsx_probe(struct pci_dev *pci,
>         if (IS_ERR(th)) {
>                 dev_err(&pci->dev, "Unable to start control thread\n");
>                 err = PTR_ERR(th);
> -               goto errout;
> +               goto err_rtsx_release;
>         }
>         dev->ctl_thread = th;
>
>         err = scsi_add_host(host, &pci->dev);
>         if (err) {
>                 dev_err(&pci->dev, "Unable to add the scsi host\n");
> -               goto errout;
> +               goto err_complete_control_thread;
>         }
>
>         /* Start up the thread for delayed SCSI-device scanning */
> @@ -950,18 +950,16 @@ static int rtsx_probe(struct pci_dev *pci,
>         if (IS_ERR(th)) {
>                 dev_err(&pci->dev, "Unable to start the device-scanning thread\n");
>                 complete(&dev->scanning_done);
> -               quiesce_and_remove_host(dev);
>                 err = PTR_ERR(th);
> -               goto errout;
> +               goto err_stop_host;
>         }
>
>         /* Start up the thread for polling thread */
>         th = kthread_run(rtsx_polling_thread, dev, "rtsx-polling");
>         if (IS_ERR(th)) {
>                 dev_err(&pci->dev, "Unable to start the device-polling thread\n");
> -               quiesce_and_remove_host(dev);
>                 err = PTR_ERR(th);
> -               goto errout;
> +               goto err_stop_host;
>         }
>         dev->polling_thread = th;
>
> @@ -970,9 +968,25 @@ static int rtsx_probe(struct pci_dev *pci,
>         return 0;
>
>         /* We come here if there are any problems */
> +err_stop_host:
> +       quiesce_and_remove_host(dev);
> +err_complete_control_thread:
> +       complete(&dev->cmnd_ready);
> +       wait_for_completion(&dev->control_exit);
> +err_rtsx_release:
> +       free_irq(dev->irq, (void *)dev);
> +       rtsx_release_chip(dev->chip);
> +err_disable_msi:

> +       dev->chip->host_cmds_ptr = NULL;
> +       dev->chip->host_sg_tbl_ptr = NULL;

This seems superfluous, you are freeing entire struct few calls after.

> +       if (dev->chip->msi_en)
> +               pci_disable_msi(dev->pci);

This might be material for another improvement, PCI core tracks
MSI/MSI-X enable state.
So, the conditional with the local variable looks superfluous as well
(in some, rare, cases we indeed need something like this to basically
distinguish MSI from MSI-X, though I don't think it's a case here)

> +err_addr_unmap:
> +       iounmap(dev->remap_addr);
> +err_chip_free:
> +       kfree(dev->chip);
>  errout:
>         dev_err(&pci->dev, "%s failed\n", __func__);
> -       release_everything(dev);
>
>         return err;
>  }
> --
> 2.17.1
>



-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v5] staging: rts5208: add error handling into rtsx_probe
  2018-06-19 17:13                 ` Andy Shevchenko
@ 2018-08-01 11:55                   ` Anton Vasilyev
  2018-08-01 12:18                     ` Andy Shevchenko
  0 siblings, 1 reply; 16+ messages in thread
From: Anton Vasilyev @ 2018-08-01 11:55 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Anton Vasilyev, Dan Carpenter, Greg Kroah-Hartman, Sinan Kaya,
	Johannes Thumshirn, Gaurav Pathak, Hannes Reinecke, devel,
	linux-kernel, ldv-project

If rtsx_probe() fails to allocate dev->chip, then release_everything()
will crash on uninitialized dev->cmnd_ready complete.

Patch adds an error handling into rtsx_probe.
Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Anton Vasilyev <vasilyev@ispras.ru>
---
v5: fix mistype and remove superfluous pointers zeroing
v4: rename labels baced on Dan Carpenter's recommendation
v3: fix subject and commit message
v2: Add error handling into rtsx_probe based on Dan Carpenter's comment.
I do not have corresponding hardware, so patch was tested by compilation only.

I faced with inaccuracy at rtsx_remove() and original rtsx_probe():
there is quiesce_and_remove_host() call with scsi_remove_host() inside,
whereas release_everything() calls scsi_host_put() after this
scsi_remove_host() call. This is strange for me.
Also I do not know is it require to check result value of
rtsx_init_chip() call on rtsx_probe().
---
 drivers/staging/rts5208/rtsx.c | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/rts5208/rtsx.c b/drivers/staging/rts5208/rtsx.c
index 70e0b8623110..6a5c670634b1 100644
--- a/drivers/staging/rts5208/rtsx.c
+++ b/drivers/staging/rts5208/rtsx.c
@@ -879,7 +879,7 @@ static int rtsx_probe(struct pci_dev *pci,
 	if (!dev->remap_addr) {
 		dev_err(&pci->dev, "ioremap error\n");
 		err = -ENXIO;
-		goto errout;
+		goto err_chip_free;
 	}
 
 	/*
@@ -894,7 +894,7 @@ static int rtsx_probe(struct pci_dev *pci,
 	if (!dev->rtsx_resv_buf) {
 		dev_err(&pci->dev, "alloc dma buffer fail\n");
 		err = -ENXIO;
-		goto errout;
+		goto err_addr_unmap;
 	}
 	dev->chip->host_cmds_ptr = dev->rtsx_resv_buf;
 	dev->chip->host_cmds_addr = dev->rtsx_resv_buf_addr;
@@ -915,7 +915,7 @@ static int rtsx_probe(struct pci_dev *pci,
 
 	if (rtsx_acquire_irq(dev) < 0) {
 		err = -EBUSY;
-		goto errout;
+		goto err_disable_msi;
 	}
 
 	pci_set_master(pci);
@@ -935,14 +935,14 @@ static int rtsx_probe(struct pci_dev *pci,
 	if (IS_ERR(th)) {
 		dev_err(&pci->dev, "Unable to start control thread\n");
 		err = PTR_ERR(th);
-		goto errout;
+		goto err_rtsx_release;
 	}
 	dev->ctl_thread = th;
 
 	err = scsi_add_host(host, &pci->dev);
 	if (err) {
 		dev_err(&pci->dev, "Unable to add the scsi host\n");
-		goto errout;
+		goto err_complete_control_thread;
 	}
 
 	/* Start up the thread for delayed SCSI-device scanning */
@@ -950,18 +950,16 @@ static int rtsx_probe(struct pci_dev *pci,
 	if (IS_ERR(th)) {
 		dev_err(&pci->dev, "Unable to start the device-scanning thread\n");
 		complete(&dev->scanning_done);
-		quiesce_and_remove_host(dev);
 		err = PTR_ERR(th);
-		goto errout;
+		goto err_stop_host;
 	}
 
 	/* Start up the thread for polling thread */
 	th = kthread_run(rtsx_polling_thread, dev, "rtsx-polling");
 	if (IS_ERR(th)) {
 		dev_err(&pci->dev, "Unable to start the device-polling thread\n");
-		quiesce_and_remove_host(dev);
 		err = PTR_ERR(th);
-		goto errout;
+		goto err_stop_host;
 	}
 	dev->polling_thread = th;
 
@@ -970,9 +968,23 @@ static int rtsx_probe(struct pci_dev *pci,
 	return 0;
 
 	/* We come here if there are any problems */
+err_stop_host:
+	quiesce_and_remove_host(dev);
+err_complete_control_thread:
+	complete(&dev->cmnd_ready);
+	wait_for_completion(&dev->control_exit);
+err_rtsx_release:
+	free_irq(dev->irq, (void *)dev);
+	rtsx_release_chip(dev->chip);
+err_disable_msi:
+	if (dev->chip->msi_en)
+		pci_disable_msi(dev->pci);
+err_addr_unmap:
+	iounmap(dev->remap_addr);
+err_chip_free:
+	kfree(dev->chip);
 errout:
 	dev_err(&pci->dev, "%s failed\n", __func__);
-	release_everything(dev);
 
 	return err;
 }
-- 
2.18.0


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v5] staging: rts5208: add error handling into rtsx_probe
  2018-08-01 11:55                   ` [PATCH v5] " Anton Vasilyev
@ 2018-08-01 12:18                     ` Andy Shevchenko
  2018-08-01 14:08                       ` Anton Vasilyev
  0 siblings, 1 reply; 16+ messages in thread
From: Andy Shevchenko @ 2018-08-01 12:18 UTC (permalink / raw)
  To: Anton Vasilyev
  Cc: Dan Carpenter, Greg Kroah-Hartman, Sinan Kaya,
	Johannes Thumshirn, Gaurav Pathak, Hannes Reinecke, devel,
	Linux Kernel Mailing List, ldv-project

On Wed, Aug 1, 2018 at 2:55 PM, Anton Vasilyev <vasilyev@ispras.ru> wrote:
> If rtsx_probe() fails to allocate dev->chip, then release_everything()
> will crash on uninitialized dev->cmnd_ready complete.
>
> Patch adds an error handling into rtsx_probe.
> Found by Linux Driver Verification project (linuxtesting.org).

Have you based your change on staging-next?

Seems not. You need to rebase and resend.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v5] staging: rts5208: add error handling into rtsx_probe
  2018-08-01 12:18                     ` Andy Shevchenko
@ 2018-08-01 14:08                       ` Anton Vasilyev
  2018-08-01 14:52                         ` Dan Carpenter
  2018-08-01 15:37                         ` Andy Shevchenko
  0 siblings, 2 replies; 16+ messages in thread
From: Anton Vasilyev @ 2018-08-01 14:08 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Dan Carpenter, Greg Kroah-Hartman, Sinan Kaya,
	Johannes Thumshirn, Gaurav Pathak, Hannes Reinecke, devel,
	Linux Kernel Mailing List, ldv-project

I found that staging-next already contains my patch v3, committed by 
Greg Kroah-Hartman.

Do I need to send a new patch with a label renaming based on Dan 
Carpenter comments?


-- 
Anton Vasilyev
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: vasilyev@ispras.ru

On 01.08.2018 15:18, Andy Shevchenko wrote:
> On Wed, Aug 1, 2018 at 2:55 PM, Anton Vasilyev <vasilyev@ispras.ru> wrote:
>> If rtsx_probe() fails to allocate dev->chip, then release_everything()
>> will crash on uninitialized dev->cmnd_ready complete.
>>
>> Patch adds an error handling into rtsx_probe.
>> Found by Linux Driver Verification project (linuxtesting.org).
> Have you based your change on staging-next?
>
> Seems not. You need to rebase and resend.
>


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v5] staging: rts5208: add error handling into rtsx_probe
  2018-08-01 14:08                       ` Anton Vasilyev
@ 2018-08-01 14:52                         ` Dan Carpenter
  2018-08-01 15:37                         ` Andy Shevchenko
  1 sibling, 0 replies; 16+ messages in thread
From: Dan Carpenter @ 2018-08-01 14:52 UTC (permalink / raw)
  To: Anton Vasilyev
  Cc: Andy Shevchenko, devel, ldv-project, Greg Kroah-Hartman,
	Johannes Thumshirn, Linux Kernel Mailing List, Sinan Kaya,
	Hannes Reinecke, Gaurav Pathak

On Wed, Aug 01, 2018 at 05:08:48PM +0300, Anton Vasilyev wrote:
> I found that staging-next already contains my patch v3, committed by Greg
> Kroah-Hartman.
> 
> Do I need to send a new patch with a label renaming based on Dan Carpenter
> comments?

I had to look to see what I had said earlier...  The naming isn't really
a problem but we should call scsi_host_put(host); if the
"dev->chip = kzalloc(sizeof(*dev->chip), GFP_KERNEL);" allocation fails.

regards,
dan capenter


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v5] staging: rts5208: add error handling into rtsx_probe
  2018-08-01 14:08                       ` Anton Vasilyev
  2018-08-01 14:52                         ` Dan Carpenter
@ 2018-08-01 15:37                         ` Andy Shevchenko
  1 sibling, 0 replies; 16+ messages in thread
From: Andy Shevchenko @ 2018-08-01 15:37 UTC (permalink / raw)
  To: Anton Vasilyev
  Cc: Dan Carpenter, Greg Kroah-Hartman, Sinan Kaya,
	Johannes Thumshirn, Gaurav Pathak, Hannes Reinecke, devel,
	Linux Kernel Mailing List, ldv-project

On Wed, Aug 1, 2018 at 5:08 PM, Anton Vasilyev <vasilyev@ispras.ru> wrote:
> I found that staging-next already contains my patch v3, committed by Greg
> Kroah-Hartman.
>
> Do I need to send a new patch

Yes. Based on staging-next.

> with a label renaming based on Dan Carpenter
> comments?

Dan is talking for himself :-)

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2018-08-01 15:37 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-09 16:38 [PATCH] staging: rts5208: add check on NULL before dereference Anton Vasilyev
2018-06-09 16:58 ` okaya
2018-06-09 19:34   ` Andy Shevchenko
2018-06-09 22:22     ` okaya
2018-06-12 13:06     ` Dan Carpenter
2018-06-13 16:55       ` [PATCH v2] " Anton Vasilyev
2018-06-13 17:00         ` Andy Shevchenko
2018-06-13 17:34           ` [PATCH v3] staging: rts5208: add error handling into rtsx_probe Anton Vasilyev
     [not found]           ` <20180613173128.32384-1-vasilyev@ispras.ru>
2018-06-19  7:42             ` your mail Dan Carpenter
2018-06-19 15:25               ` [PATCH v4] staging: rts5208: add error handling into rtsx_probe Anton Vasilyev
2018-06-19 17:13                 ` Andy Shevchenko
2018-08-01 11:55                   ` [PATCH v5] " Anton Vasilyev
2018-08-01 12:18                     ` Andy Shevchenko
2018-08-01 14:08                       ` Anton Vasilyev
2018-08-01 14:52                         ` Dan Carpenter
2018-08-01 15:37                         ` Andy Shevchenko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).