ufs: setting "hba" private pointer too late -- oops in ufshcd_devfreq_get_dev_status()

ufs: setting "hba" private pointer too late -- oops in ufshcd_devfreq_get_dev_status()

[Alexey Dobriyan]

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.

Re: ufs: setting "hba" private pointer too late -- oops in ufshcd_devfreq_get_dev_status()

[Bart Van Assche]

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.

Re: ufs: setting "hba" private pointer too late -- oops in ufshcd_devfreq_get_dev_status()

[Alexey Dobriyan]

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.

Re: ufs: setting "hba" private pointer too late -- oops in ufshcd_devfreq_get_dev_status()

[Bart Van Assche]

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) {