* ufs: setting "hba" private pointer too late -- oops in ufshcd_devfreq_get_dev_status()
@ 2021-11-10 14:32 Alexey Dobriyan
2021-11-10 18:18 ` Bart Van Assche
0 siblings, 1 reply; 4+ messages in thread
From: Alexey Dobriyan @ 2021-11-10 14:32 UTC (permalink / raw)
To: alim.akhtar, avri.altman; +Cc: linux-scsi
I've stumbled into a race while working on an earlier kernel,
but it looks like mainline is affected as well.
err = ufshcd_init(hba, mmio_base, irq);
async_schedule(ufshcd_async_scan, hba);
ufshcd_add_lus(hba);
if (ufshcd_is_clkscaling_supported(hba)) {
[enable devfreq]
platform_set_drvdata(pdev, hba);
Device's private pointer is set too late, as devfreq hook get HBA
pointer from private data and uses it:
static int ufshcd_devfreq_get_dev_status(struct device *dev, struct devfreq_dev_status *stat)
{
struct ufs_hba *hba = dev_get_drvdata(dev);
if (!ufshcd_is_clkscaling_supported(hba))
return -EINVAL;
Unable to handle kernel NULL pointer dereference at virtual address ...0f10
pc : ufshcd_devfreq_get_dev_status
lr : devfreq_simple_ondemand_func
update_devfreq
devfreq_monitor
I reproduced it by turning async LU scan into sync, so it is easier to
trigger.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: ufs: setting "hba" private pointer too late -- oops in ufshcd_devfreq_get_dev_status()
2021-11-10 14:32 ufs: setting "hba" private pointer too late -- oops in ufshcd_devfreq_get_dev_status() Alexey Dobriyan
@ 2021-11-10 18:18 ` Bart Van Assche
2021-11-11 20:28 ` Alexey Dobriyan
0 siblings, 1 reply; 4+ messages in thread
From: Bart Van Assche @ 2021-11-10 18:18 UTC (permalink / raw)
To: Alexey Dobriyan, alim.akhtar, avri.altman; +Cc: linux-scsi
On 11/10/21 6:32 AM, Alexey Dobriyan wrote:
> I've stumbled into a race while working on an earlier kernel,
> but it looks like mainline is affected as well.
>
> err = ufshcd_init(hba, mmio_base, irq);
> async_schedule(ufshcd_async_scan, hba);
> ufshcd_add_lus(hba);
> if (ufshcd_is_clkscaling_supported(hba)) {
> [enable devfreq]
>
> platform_set_drvdata(pdev, hba);
>
> Device's private pointer is set too late, as devfreq hook get HBA
> pointer from private data and uses it:
>
> static int ufshcd_devfreq_get_dev_status(struct device *dev, struct devfreq_dev_status *stat)
> {
> struct ufs_hba *hba = dev_get_drvdata(dev);
> if (!ufshcd_is_clkscaling_supported(hba))
> return -EINVAL;
>
> Unable to handle kernel NULL pointer dereference at virtual address ...0f10
> pc : ufshcd_devfreq_get_dev_status
> lr : devfreq_simple_ondemand_func
> update_devfreq
> devfreq_monitor
>
>
> I reproduced it by turning async LU scan into sync, so it is easier to
> trigger.
Hi Alexey,
Thanks for having reported this. Do you perhaps plan to post a patch to
fix this?
Thanks,
Bart.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: ufs: setting "hba" private pointer too late -- oops in ufshcd_devfreq_get_dev_status()
2021-11-10 18:18 ` Bart Van Assche
@ 2021-11-11 20:28 ` Alexey Dobriyan
2021-11-17 0:59 ` Bart Van Assche
0 siblings, 1 reply; 4+ messages in thread
From: Alexey Dobriyan @ 2021-11-11 20:28 UTC (permalink / raw)
To: Bart Van Assche; +Cc: alim.akhtar, avri.altman, linux-scsi
On Wed, Nov 10, 2021 at 10:18:15AM -0800, Bart Van Assche wrote:
> On 11/10/21 6:32 AM, Alexey Dobriyan wrote:
> > I've stumbled into a race while working on an earlier kernel,
> > but it looks like mainline is affected as well.
> >
> > err = ufshcd_init(hba, mmio_base, irq);
> > async_schedule(ufshcd_async_scan, hba);
> > ufshcd_add_lus(hba);
> > if (ufshcd_is_clkscaling_supported(hba)) {
> > [enable devfreq]
> >
> > platform_set_drvdata(pdev, hba);
> >
> > Device's private pointer is set too late, as devfreq hook get HBA
> > pointer from private data and uses it:
> >
> > static int ufshcd_devfreq_get_dev_status(struct device *dev, struct devfreq_dev_status *stat)
> > {
> > struct ufs_hba *hba = dev_get_drvdata(dev);
> > if (!ufshcd_is_clkscaling_supported(hba))
> > return -EINVAL;
> >
> > Unable to handle kernel NULL pointer dereference at virtual address ...0f10
> > pc : ufshcd_devfreq_get_dev_status
> > lr : devfreq_simple_ondemand_func
> > update_devfreq
> > devfreq_monitor
> >
> >
> > I reproduced it by turning async LU scan into sync, so it is easier to
> > trigger.
>
> Hi Alexey,
>
> Thanks for having reported this. Do you perhaps plan to post a patch to fix
> this?
Not really, my workaround is
if (!hba) {
return -EINVAL;
}
But it is likely incorrect.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: ufs: setting "hba" private pointer too late -- oops in ufshcd_devfreq_get_dev_status()
2021-11-11 20:28 ` Alexey Dobriyan
@ 2021-11-17 0:59 ` Bart Van Assche
0 siblings, 0 replies; 4+ messages in thread
From: Bart Van Assche @ 2021-11-17 0:59 UTC (permalink / raw)
To: Alexey Dobriyan; +Cc: alim.akhtar, avri.altman, linux-scsi
On 11/11/21 12:28, Alexey Dobriyan wrote:
> Not really, my workaround is
>
> if (!hba) {
> return -EINVAL;
> }
>
> But it is likely incorrect.
How about the untested patch below?
Thanks,
Bart.
---
drivers/scsi/ufs/tc-dwc-g210-pci.c | 1 -
drivers/scsi/ufs/ufshcd-pci.c | 2 --
drivers/scsi/ufs/ufshcd-pltfrm.c | 2 --
drivers/scsi/ufs/ufshcd.c | 7 +++++++
4 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/scsi/ufs/tc-dwc-g210-pci.c b/drivers/scsi/ufs/tc-dwc-g210-pci.c
index 679289e1a78e..7b08e2e07cc5 100644
--- a/drivers/scsi/ufs/tc-dwc-g210-pci.c
+++ b/drivers/scsi/ufs/tc-dwc-g210-pci.c
@@ -110,7 +110,6 @@ tc_dwc_g210_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
return err;
}
- pci_set_drvdata(pdev, hba);
pm_runtime_put_noidle(&pdev->dev);
pm_runtime_allow(&pdev->dev);
diff --git a/drivers/scsi/ufs/ufshcd-pci.c b/drivers/scsi/ufs/ufshcd-pci.c
index 51424557810d..a673eedb2f05 100644
--- a/drivers/scsi/ufs/ufshcd-pci.c
+++ b/drivers/scsi/ufs/ufshcd-pci.c
@@ -522,8 +522,6 @@ ufshcd_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
return err;
}
- pci_set_drvdata(pdev, hba);
-
hba->vops = (struct ufs_hba_variant_ops *)id->driver_data;
err = ufshcd_init(hba, mmio_base, pdev->irq);
diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c
index eaeae83b999f..8b16bbbcb806 100644
--- a/drivers/scsi/ufs/ufshcd-pltfrm.c
+++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
@@ -361,8 +361,6 @@ int ufshcd_pltfrm_init(struct platform_device *pdev,
goto dealloc_host;
}
- platform_set_drvdata(pdev, hba);
-
pm_runtime_set_active(&pdev->dev);
pm_runtime_enable(&pdev->dev);
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index a7a8d3fbb89d..87b8bd837342 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -9608,6 +9608,13 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
*/
mb();
+ /*
+ * dev_set_drvdata() must be called before
+ * async_schedule(ufshcd_async_scan, hba) since the callbacks registered
+ * with the devfreq framework use dev_get_drvdata().
+ */
+ dev_set_drvdata(dev, hba);
+
/* IRQ registration */
err = devm_request_irq(dev, irq, ufshcd_intr, IRQF_SHARED, UFSHCD, hba);
if (err) {
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-11-17 0:59 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-10 14:32 ufs: setting "hba" private pointer too late -- oops in ufshcd_devfreq_get_dev_status() Alexey Dobriyan
2021-11-10 18:18 ` Bart Van Assche
2021-11-11 20:28 ` Alexey Dobriyan
2021-11-17 0:59 ` Bart Van Assche
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.