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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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);
> }
On Sat, Jun 9, 2018 at 7:58 PM, <[email protected]> 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
On 2018-06-09 15:34, Andy Shevchenko wrote:
> On Sat, Jun 9, 2018 at 7:58 PM, <[email protected]> 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()
On Sat, Jun 09, 2018 at 10:34:43PM +0300, Andy Shevchenko wrote:
> On Sat, Jun 9, 2018 at 7:58 PM, <[email protected]> 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
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 <[email protected]>
---
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
On Wed, Jun 13, 2018 at 7:55 PM, Anton Vasilyev <[email protected]> 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 <[email protected]>
> ---
> 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
Subject: [PATCH v3] staging: rts5208: add error handling into rtsx_probe
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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
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 <[email protected]>
---
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
On Tue, Jun 19, 2018 at 6:25 PM, Anton Vasilyev <[email protected]> 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 <[email protected]>
Couple of comments below though.
> Signed-off-by: Anton Vasilyev <[email protected]>
> ---
> 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
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 <[email protected]>
---
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
On Wed, Aug 1, 2018 at 2:55 PM, Anton Vasilyev <[email protected]> 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
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: [email protected]
On 01.08.2018 15:18, Andy Shevchenko wrote:
> On Wed, Aug 1, 2018 at 2:55 PM, Anton Vasilyev <[email protected]> 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.
>
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
On Wed, Aug 1, 2018 at 5:08 PM, Anton Vasilyev <[email protected]> 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