[PATCH RFC 0/2] driver core: Add ability to delete device links of unregistered devices

[PATCH RFC 0/2] driver core: Add ability to delete device links of unregistered devices

[Adrian Hunter]

Hi

There is an issue with the SCSI UFS driver when the optional
BOOT well-known LUN fails to probe, which is not a fatal error.
The issue is that the device and its "managed" device link do not
then get deleted.  The device because the device link has a
reference to it.  The device link because it can only be deleted
by device_del(), but device_add() was never called, so device_del()
never will be either.

It is not clear if there is a way to get rid of the device link in
this case, so here is a patch to add a function that does just that.

There is also a patch to use the new function to fix the issue with
the SCSI UFS driver.


Adrian Hunter (2):
      driver core: Add ability to delete device links of unregistered devices
      scsi: ufshcd: Fix device links when BOOT WLUN fails to probe

 Documentation/driver-api/device_link.rst |  7 +++++--
 drivers/base/core.c                      | 22 +++++++++++++++++++---
 drivers/scsi/ufs/ufshcd.c                |  7 +++++++
 include/linux/device.h                   |  1 +
 4 files changed, 32 insertions(+), 5 deletions(-)


Regards
Adrian

[PATCH RFC 1/2] driver core: Add ability to delete device links of unregistered devices

[Adrian Hunter]

Managed device links are deleted by device_del(). However it is possible to
add a device link to a consumer before device_add(), and then discover an
error prevents the device from being used. In that case normally references
to the device would be dropped and the device would be deleted. However the
device link holds a reference to the device, so the device link and device
remain indefinitely.

Add a function to delete device links of an unregistered device, so that
drivers can get rid of unregistered devices and their device links.

To do that, the devlink_remove_symlinks() function must be amended to cope
with the absence of the consumer's sysfs presence, otherwise
sysfs_remove_link() will generate warnings.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 Documentation/driver-api/device_link.rst |  7 +++++--
 drivers/base/core.c                      | 22 +++++++++++++++++++---
 include/linux/device.h                   |  1 +
 3 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/Documentation/driver-api/device_link.rst b/Documentation/driver-api/device_link.rst
index ee913ae16371..12c0602822c8 100644
--- a/Documentation/driver-api/device_link.rst
+++ b/Documentation/driver-api/device_link.rst
@@ -75,7 +75,9 @@ driver is compiled as a module, the device link is added on module load and
 orderly deleted on unload.  The same restrictions that apply to device link
 addition (e.g. exclusion of a parallel suspend/resume transition) apply equally
 to deletion.  Device links managed by the driver core are deleted automatically
-by it.
+by it, except if the consumer device is never registered (e.g. because of an
+error), in which case the device links must be explicitly removed using
+:c:func:`device_links_scrap()`.
 
 Several flags may be specified on device link addition, two of which
 have already been mentioned above:  ``DL_FLAG_STATELESS`` to express that no
@@ -317,4 +319,5 @@ State machine
 API
 ===
 
-See device_link_add(), device_link_del() and device_link_remove().
+See device_link_add(), device_link_del(), device_link_remove() and
+device_links_scrap().
diff --git a/drivers/base/core.c b/drivers/base/core.c
index ea5b85354526..2ed32ab50b84 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -562,7 +562,8 @@ static void devlink_remove_symlinks(struct device *dev,
 	struct device *con = link->consumer;
 	char *buf;
 
-	sysfs_remove_link(&link->link_dev.kobj, "consumer");
+	if (device_is_registered(con))
+		sysfs_remove_link(&link->link_dev.kobj, "consumer");
 	sysfs_remove_link(&link->link_dev.kobj, "supplier");
 
 	len = max(strlen(dev_bus_name(sup)) + strlen(dev_name(sup)),
@@ -575,8 +576,10 @@ static void devlink_remove_symlinks(struct device *dev,
 		return;
 	}
 
-	snprintf(buf, len, "supplier:%s:%s", dev_bus_name(sup), dev_name(sup));
-	sysfs_remove_link(&con->kobj, buf);
+	if (device_is_registered(con)) {
+		snprintf(buf, len, "supplier:%s:%s", dev_bus_name(sup), dev_name(sup));
+		sysfs_remove_link(&con->kobj, buf);
+	}
 	snprintf(buf, len, "consumer:%s:%s", dev_bus_name(con), dev_name(con));
 	sysfs_remove_link(&sup->kobj, buf);
 	kfree(buf);
@@ -1538,6 +1541,19 @@ static void device_links_purge(struct device *dev)
 	device_links_write_unlock();
 }
 
+/**
+ * device_links_scrap - Device was never registered, delete any device links.
+ * @dev: Target device.
+ */
+void device_links_scrap(struct device *dev)
+{
+	if (WARN_ON(device_is_registered(dev)))
+		return;
+
+	device_links_purge(dev);
+}
+EXPORT_SYMBOL_GPL(device_links_scrap);
+
 #define FW_DEVLINK_FLAGS_PERMISSIVE	(DL_FLAG_INFERRED | \
 					 DL_FLAG_SYNC_STATE_ONLY)
 #define FW_DEVLINK_FLAGS_ON		(DL_FLAG_INFERRED | \
diff --git a/include/linux/device.h b/include/linux/device.h
index 2a22875238a6..90574f9aa8f2 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -967,6 +967,7 @@ struct device_link *device_link_add(struct device *consumer,
 				    struct device *supplier, u32 flags);
 void device_link_del(struct device_link *link);
 void device_link_remove(void *consumer, struct device *supplier);
+void device_links_scrap(struct device *dev);
 void device_links_supplier_sync_state_pause(void);
 void device_links_supplier_sync_state_resume(void);
 
-- 
2.17.1

[PATCH RFC 2/2] scsi: ufshcd: Fix device links when BOOT WLUN fails to probe

[Adrian Hunter]

If a LUN fails to probe (e.g. absent BOOT WLUN), the device will not have
been registered but can still have a device link holding a reference to the
device. The unwanted device link will prevent runtime suspend indefinitely,
and cause some warnings if the supplier is ever deleted (e.g. by unbinding
the UFS host controller). Fix by explicitly deleting the device link when
SCSI destroys the SCSI device.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/scsi/ufs/ufshcd.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 708b3b62fc4d..483aa74fe2c8 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -5029,6 +5029,13 @@ static void ufshcd_slave_destroy(struct scsi_device *sdev)
 		spin_lock_irqsave(hba->host->host_lock, flags);
 		hba->sdev_ufs_device = NULL;
 		spin_unlock_irqrestore(hba->host->host_lock, flags);
+	} else {
+		/*
+		 * If a LUN fails to probe (e.g. absent BOOT WLUN), the device
+		 * will not have been registered but can still have a device
+		 * link holding a reference to the device.
+		 */
+		device_links_scrap(&sdev->sdev_gendev);
 	}
 }
 
-- 
2.17.1

Re: [PATCH RFC 1/2] driver core: Add ability to delete device links of unregistered devices

[Greg Kroah-Hartman]

On Wed, Jul 07, 2021 at 08:29:47PM +0300, Adrian Hunter wrote:
> +void device_links_scrap(struct device *dev)
> +{
> +	if (WARN_ON(device_is_registered(dev)))

You just rebooted a box if this was hit, never add new WARN_ON() please.

greg k-h

Re: [PATCH RFC 2/2] scsi: ufshcd: Fix device links when BOOT WLUN fails to probe

[Greg Kroah-Hartman]

On Wed, Jul 07, 2021 at 08:29:48PM +0300, Adrian Hunter wrote:
> If a LUN fails to probe (e.g. absent BOOT WLUN), the device will not have
> been registered but can still have a device link holding a reference to the
> device. The unwanted device link will prevent runtime suspend indefinitely,
> and cause some warnings if the supplier is ever deleted (e.g. by unbinding
> the UFS host controller). Fix by explicitly deleting the device link when
> SCSI destroys the SCSI device.
> 
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>  drivers/scsi/ufs/ufshcd.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 708b3b62fc4d..483aa74fe2c8 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -5029,6 +5029,13 @@ static void ufshcd_slave_destroy(struct scsi_device *sdev)
>  		spin_lock_irqsave(hba->host->host_lock, flags);
>  		hba->sdev_ufs_device = NULL;
>  		spin_unlock_irqrestore(hba->host->host_lock, flags);
> +	} else {
> +		/*
> +		 * If a LUN fails to probe (e.g. absent BOOT WLUN), the device
> +		 * will not have been registered but can still have a device
> +		 * link holding a reference to the device.
> +		 */
> +		device_links_scrap(&sdev->sdev_gendev);

What created that link?  And why did it do that before probe happened
successfully?

thanks,

greg k-h

Re: [PATCH RFC 0/2] driver core: Add ability to delete device links of unregistered devices

[Greg Kroah-Hartman]

On Wed, Jul 07, 2021 at 08:29:46PM +0300, Adrian Hunter wrote:
> Hi
> 
> There is an issue with the SCSI UFS driver when the optional
> BOOT well-known LUN fails to probe, which is not a fatal error.
> The issue is that the device and its "managed" device link do not
> then get deleted.  The device because the device link has a
> reference to it.  The device link because it can only be deleted
> by device_del(), but device_add() was never called, so device_del()
> never will be either.

How was a link created for something that never had device_add() called
on it?  Who is doing that?

thanks,

greg k-h

Re: [PATCH RFC 0/2] driver core: Add ability to delete device links of unregistered devices

[Saravana Kannan]

On Wed, Jul 7, 2021 at 10:40 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, Jul 07, 2021 at 08:29:46PM +0300, Adrian Hunter wrote:
> > Hi
> >
> > There is an issue with the SCSI UFS driver when the optional
> > BOOT well-known LUN fails to probe, which is not a fatal error.
> > The issue is that the device and its "managed" device link do not
> > then get deleted.  The device because the device link has a
> > reference to it.  The device link because it can only be deleted
> > by device_del(), but device_add() was never called, so device_del()
> > never will be either.
>
> How was a link created for something that never had device_add() called
> on it?  Who is doing that?
>

Greg responded as I was typing :)

Nak for the series. I don't like mixing and matching the control of
managed device links like this.

Can I get more details please. How is the device link added in the first place?

-Saravana

Re: [PATCH RFC 2/2] scsi: ufshcd: Fix device links when BOOT WLUN fails to probe

[Adrian Hunter]

On 7/07/21 8:39 pm, Greg Kroah-Hartman wrote:
> On Wed, Jul 07, 2021 at 08:29:48PM +0300, Adrian Hunter wrote:
>> If a LUN fails to probe (e.g. absent BOOT WLUN), the device will not have
>> been registered but can still have a device link holding a reference to the
>> device. The unwanted device link will prevent runtime suspend indefinitely,
>> and cause some warnings if the supplier is ever deleted (e.g. by unbinding
>> the UFS host controller). Fix by explicitly deleting the device link when
>> SCSI destroys the SCSI device.
>>
>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>> ---
>>  drivers/scsi/ufs/ufshcd.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 708b3b62fc4d..483aa74fe2c8 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -5029,6 +5029,13 @@ static void ufshcd_slave_destroy(struct scsi_device *sdev)
>>  		spin_lock_irqsave(hba->host->host_lock, flags);
>>  		hba->sdev_ufs_device = NULL;
>>  		spin_unlock_irqrestore(hba->host->host_lock, flags);
>> +	} else {
>> +		/*
>> +		 * If a LUN fails to probe (e.g. absent BOOT WLUN), the device
>> +		 * will not have been registered but can still have a device
>> +		 * link holding a reference to the device.
>> +		 */
>> +		device_links_scrap(&sdev->sdev_gendev);
> 
> What created that link?  And why did it do that before probe happened
> successfully?

The same driver created the link.

The documentation seems to say it is allowed to, if it is the consumer.
From Documentation/driver-api/device_link.rst

  Usage
  =====

  The earliest point in time when device links can be added is after
  :c:func:`device_add()` has been called for the supplier and
  :c:func:`device_initialize()` has been called for the consumer.

Re: [PATCH RFC 2/2] scsi: ufshcd: Fix device links when BOOT WLUN fails to probe

[Rafael J. Wysocki]

On Wed, Jul 7, 2021 at 7:49 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 7/07/21 8:39 pm, Greg Kroah-Hartman wrote:
> > On Wed, Jul 07, 2021 at 08:29:48PM +0300, Adrian Hunter wrote:
> >> If a LUN fails to probe (e.g. absent BOOT WLUN), the device will not have
> >> been registered but can still have a device link holding a reference to the
> >> device. The unwanted device link will prevent runtime suspend indefinitely,
> >> and cause some warnings if the supplier is ever deleted (e.g. by unbinding
> >> the UFS host controller). Fix by explicitly deleting the device link when
> >> SCSI destroys the SCSI device.
> >>
> >> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> >> ---
> >>  drivers/scsi/ufs/ufshcd.c | 7 +++++++
> >>  1 file changed, 7 insertions(+)
> >>
> >> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> >> index 708b3b62fc4d..483aa74fe2c8 100644
> >> --- a/drivers/scsi/ufs/ufshcd.c
> >> +++ b/drivers/scsi/ufs/ufshcd.c
> >> @@ -5029,6 +5029,13 @@ static void ufshcd_slave_destroy(struct scsi_device *sdev)
> >>              spin_lock_irqsave(hba->host->host_lock, flags);
> >>              hba->sdev_ufs_device = NULL;
> >>              spin_unlock_irqrestore(hba->host->host_lock, flags);
> >> +    } else {
> >> +            /*
> >> +             * If a LUN fails to probe (e.g. absent BOOT WLUN), the device
> >> +             * will not have been registered but can still have a device
> >> +             * link holding a reference to the device.
> >> +             */
> >> +            device_links_scrap(&sdev->sdev_gendev);
> >
> > What created that link?  And why did it do that before probe happened
> > successfully?
>
> The same driver created the link.
>
> The documentation seems to say it is allowed to, if it is the consumer.
> From Documentation/driver-api/device_link.rst
>
>   Usage
>   =====
>
>   The earliest point in time when device links can be added is after
>   :c:func:`device_add()` has been called for the supplier and
>   :c:func:`device_initialize()` has been called for the consumer.

Yes, this is allowed, but if you've added device links to a device
object that is not going to be registered after all, you are
responsible for doing the cleanup.

Why can't you call device_link_del() directly on those links?

Or device_link_remove() if you don't want to deal with link pointers?

Re: [PATCH RFC 2/2] scsi: ufshcd: Fix device links when BOOT WLUN fails to probe

[Adrian Hunter]

On 8/07/21 3:31 pm, Rafael J. Wysocki wrote:
> On Wed, Jul 7, 2021 at 7:49 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
>>
>> On 7/07/21 8:39 pm, Greg Kroah-Hartman wrote:
>>> On Wed, Jul 07, 2021 at 08:29:48PM +0300, Adrian Hunter wrote:
>>>> If a LUN fails to probe (e.g. absent BOOT WLUN), the device will not have
>>>> been registered but can still have a device link holding a reference to the
>>>> device. The unwanted device link will prevent runtime suspend indefinitely,
>>>> and cause some warnings if the supplier is ever deleted (e.g. by unbinding
>>>> the UFS host controller). Fix by explicitly deleting the device link when
>>>> SCSI destroys the SCSI device.
>>>>
>>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>>>> ---
>>>>  drivers/scsi/ufs/ufshcd.c | 7 +++++++
>>>>  1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>>>> index 708b3b62fc4d..483aa74fe2c8 100644
>>>> --- a/drivers/scsi/ufs/ufshcd.c
>>>> +++ b/drivers/scsi/ufs/ufshcd.c
>>>> @@ -5029,6 +5029,13 @@ static void ufshcd_slave_destroy(struct scsi_device *sdev)
>>>>              spin_lock_irqsave(hba->host->host_lock, flags);
>>>>              hba->sdev_ufs_device = NULL;
>>>>              spin_unlock_irqrestore(hba->host->host_lock, flags);
>>>> +    } else {
>>>> +            /*
>>>> +             * If a LUN fails to probe (e.g. absent BOOT WLUN), the device
>>>> +             * will not have been registered but can still have a device
>>>> +             * link holding a reference to the device.
>>>> +             */
>>>> +            device_links_scrap(&sdev->sdev_gendev);
>>>
>>> What created that link?  And why did it do that before probe happened
>>> successfully?
>>
>> The same driver created the link.
>>
>> The documentation seems to say it is allowed to, if it is the consumer.
>> From Documentation/driver-api/device_link.rst
>>
>>   Usage
>>   =====
>>
>>   The earliest point in time when device links can be added is after
>>   :c:func:`device_add()` has been called for the supplier and
>>   :c:func:`device_initialize()` has been called for the consumer.
> 
> Yes, this is allowed, but if you've added device links to a device
> object that is not going to be registered after all, you are
> responsible for doing the cleanup.
> 
> Why can't you call device_link_del() directly on those links?
> 
> Or device_link_remove() if you don't want to deal with link pointers?
> 

Those only work for DL_FLAG_STATELESS device links, but we use only
DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE flags.

Re: [PATCH RFC 2/2] scsi: ufshcd: Fix device links when BOOT WLUN fails to probe

[Rafael J. Wysocki]

On Thu, Jul 8, 2021 at 4:17 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 8/07/21 3:31 pm, Rafael J. Wysocki wrote:
> > On Wed, Jul 7, 2021 at 7:49 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
> >>
> >> On 7/07/21 8:39 pm, Greg Kroah-Hartman wrote:
> >>> On Wed, Jul 07, 2021 at 08:29:48PM +0300, Adrian Hunter wrote:
> >>>> If a LUN fails to probe (e.g. absent BOOT WLUN), the device will not have
> >>>> been registered but can still have a device link holding a reference to the
> >>>> device. The unwanted device link will prevent runtime suspend indefinitely,
> >>>> and cause some warnings if the supplier is ever deleted (e.g. by unbinding
> >>>> the UFS host controller). Fix by explicitly deleting the device link when
> >>>> SCSI destroys the SCSI device.
> >>>>
> >>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> >>>> ---
> >>>>  drivers/scsi/ufs/ufshcd.c | 7 +++++++
> >>>>  1 file changed, 7 insertions(+)
> >>>>
> >>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> >>>> index 708b3b62fc4d..483aa74fe2c8 100644
> >>>> --- a/drivers/scsi/ufs/ufshcd.c
> >>>> +++ b/drivers/scsi/ufs/ufshcd.c
> >>>> @@ -5029,6 +5029,13 @@ static void ufshcd_slave_destroy(struct scsi_device *sdev)
> >>>>              spin_lock_irqsave(hba->host->host_lock, flags);
> >>>>              hba->sdev_ufs_device = NULL;
> >>>>              spin_unlock_irqrestore(hba->host->host_lock, flags);
> >>>> +    } else {
> >>>> +            /*
> >>>> +             * If a LUN fails to probe (e.g. absent BOOT WLUN), the device
> >>>> +             * will not have been registered but can still have a device
> >>>> +             * link holding a reference to the device.
> >>>> +             */
> >>>> +            device_links_scrap(&sdev->sdev_gendev);
> >>>
> >>> What created that link?  And why did it do that before probe happened
> >>> successfully?
> >>
> >> The same driver created the link.
> >>
> >> The documentation seems to say it is allowed to, if it is the consumer.
> >> From Documentation/driver-api/device_link.rst
> >>
> >>   Usage
> >>   =====
> >>
> >>   The earliest point in time when device links can be added is after
> >>   :c:func:`device_add()` has been called for the supplier and
> >>   :c:func:`device_initialize()` has been called for the consumer.
> >
> > Yes, this is allowed, but if you've added device links to a device
> > object that is not going to be registered after all, you are
> > responsible for doing the cleanup.
> >
> > Why can't you call device_link_del() directly on those links?
> >
> > Or device_link_remove() if you don't want to deal with link pointers?
> >
>
> Those only work for DL_FLAG_STATELESS device links, but we use only
> DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE flags.

So I'd probably modify device_link_remove() to check if the consumer
device has been registered and run __device_link_del() directly
instead of device_link_put_kref() if it hasn't.

Or add an argument to it to force the removal.

Re: [PATCH RFC 2/2] scsi: ufshcd: Fix device links when BOOT WLUN fails to probe

[Rafael J. Wysocki]

On Thu, Jul 8, 2021 at 5:03 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Jul 8, 2021 at 4:17 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
> >
> > On 8/07/21 3:31 pm, Rafael J. Wysocki wrote:
> > > On Wed, Jul 7, 2021 at 7:49 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
> > >>
> > >> On 7/07/21 8:39 pm, Greg Kroah-Hartman wrote:
> > >>> On Wed, Jul 07, 2021 at 08:29:48PM +0300, Adrian Hunter wrote:
> > >>>> If a LUN fails to probe (e.g. absent BOOT WLUN), the device will not have
> > >>>> been registered but can still have a device link holding a reference to the
> > >>>> device. The unwanted device link will prevent runtime suspend indefinitely,
> > >>>> and cause some warnings if the supplier is ever deleted (e.g. by unbinding
> > >>>> the UFS host controller). Fix by explicitly deleting the device link when
> > >>>> SCSI destroys the SCSI device.
> > >>>>
> > >>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> > >>>> ---
> > >>>>  drivers/scsi/ufs/ufshcd.c | 7 +++++++
> > >>>>  1 file changed, 7 insertions(+)
> > >>>>
> > >>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > >>>> index 708b3b62fc4d..483aa74fe2c8 100644
> > >>>> --- a/drivers/scsi/ufs/ufshcd.c
> > >>>> +++ b/drivers/scsi/ufs/ufshcd.c
> > >>>> @@ -5029,6 +5029,13 @@ static void ufshcd_slave_destroy(struct scsi_device *sdev)
> > >>>>              spin_lock_irqsave(hba->host->host_lock, flags);
> > >>>>              hba->sdev_ufs_device = NULL;
> > >>>>              spin_unlock_irqrestore(hba->host->host_lock, flags);
> > >>>> +    } else {
> > >>>> +            /*
> > >>>> +             * If a LUN fails to probe (e.g. absent BOOT WLUN), the device
> > >>>> +             * will not have been registered but can still have a device
> > >>>> +             * link holding a reference to the device.
> > >>>> +             */
> > >>>> +            device_links_scrap(&sdev->sdev_gendev);
> > >>>
> > >>> What created that link?  And why did it do that before probe happened
> > >>> successfully?
> > >>
> > >> The same driver created the link.
> > >>
> > >> The documentation seems to say it is allowed to, if it is the consumer.
> > >> From Documentation/driver-api/device_link.rst
> > >>
> > >>   Usage
> > >>   =====
> > >>
> > >>   The earliest point in time when device links can be added is after
> > >>   :c:func:`device_add()` has been called for the supplier and
> > >>   :c:func:`device_initialize()` has been called for the consumer.
> > >
> > > Yes, this is allowed, but if you've added device links to a device
> > > object that is not going to be registered after all, you are
> > > responsible for doing the cleanup.
> > >
> > > Why can't you call device_link_del() directly on those links?
> > >
> > > Or device_link_remove() if you don't want to deal with link pointers?
> > >
> >
> > Those only work for DL_FLAG_STATELESS device links, but we use only
> > DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE flags.
>
> So I'd probably modify device_link_remove() to check if the consumer
> device has been registered and run __device_link_del() directly
> instead of device_link_put_kref() if it hasn't.
>
> Or add an argument to it to force the removal.

Or even modify device_link_put_kref() like this:

 static void device_link_put_kref(struct device_link *link)
 {
          if (link->flags & DL_FLAG_STATELESS)
                  kref_put(&link->kref, __device_link_del);
+        else if (!device_is_registered(link->consumer))
+                __device_link_del(link);
          else
                 WARN(1, "Unable to drop a managed device link reference\n");
 }

Re: [PATCH RFC 2/2] scsi: ufshcd: Fix device links when BOOT WLUN fails to probe

[Adrian Hunter]

On 8/07/21 6:12 pm, Rafael J. Wysocki wrote:
> On Thu, Jul 8, 2021 at 5:03 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>>
>> On Thu, Jul 8, 2021 at 4:17 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>
>>> On 8/07/21 3:31 pm, Rafael J. Wysocki wrote:
>>>> On Wed, Jul 7, 2021 at 7:49 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>>
>>>>> On 7/07/21 8:39 pm, Greg Kroah-Hartman wrote:
>>>>>> On Wed, Jul 07, 2021 at 08:29:48PM +0300, Adrian Hunter wrote:
>>>>>>> If a LUN fails to probe (e.g. absent BOOT WLUN), the device will not have
>>>>>>> been registered but can still have a device link holding a reference to the
>>>>>>> device. The unwanted device link will prevent runtime suspend indefinitely,
>>>>>>> and cause some warnings if the supplier is ever deleted (e.g. by unbinding
>>>>>>> the UFS host controller). Fix by explicitly deleting the device link when
>>>>>>> SCSI destroys the SCSI device.
>>>>>>>
>>>>>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>>>>>>> ---
>>>>>>>  drivers/scsi/ufs/ufshcd.c | 7 +++++++
>>>>>>>  1 file changed, 7 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>>>>>>> index 708b3b62fc4d..483aa74fe2c8 100644
>>>>>>> --- a/drivers/scsi/ufs/ufshcd.c
>>>>>>> +++ b/drivers/scsi/ufs/ufshcd.c
>>>>>>> @@ -5029,6 +5029,13 @@ static void ufshcd_slave_destroy(struct scsi_device *sdev)
>>>>>>>              spin_lock_irqsave(hba->host->host_lock, flags);
>>>>>>>              hba->sdev_ufs_device = NULL;
>>>>>>>              spin_unlock_irqrestore(hba->host->host_lock, flags);
>>>>>>> +    } else {
>>>>>>> +            /*
>>>>>>> +             * If a LUN fails to probe (e.g. absent BOOT WLUN), the device
>>>>>>> +             * will not have been registered but can still have a device
>>>>>>> +             * link holding a reference to the device.
>>>>>>> +             */
>>>>>>> +            device_links_scrap(&sdev->sdev_gendev);
>>>>>>
>>>>>> What created that link?  And why did it do that before probe happened
>>>>>> successfully?
>>>>>
>>>>> The same driver created the link.
>>>>>
>>>>> The documentation seems to say it is allowed to, if it is the consumer.
>>>>> From Documentation/driver-api/device_link.rst
>>>>>
>>>>>   Usage
>>>>>   =====
>>>>>
>>>>>   The earliest point in time when device links can be added is after
>>>>>   :c:func:`device_add()` has been called for the supplier and
>>>>>   :c:func:`device_initialize()` has been called for the consumer.
>>>>
>>>> Yes, this is allowed, but if you've added device links to a device
>>>> object that is not going to be registered after all, you are
>>>> responsible for doing the cleanup.
>>>>
>>>> Why can't you call device_link_del() directly on those links?
>>>>
>>>> Or device_link_remove() if you don't want to deal with link pointers?
>>>>
>>>
>>> Those only work for DL_FLAG_STATELESS device links, but we use only
>>> DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE flags.
>>
>> So I'd probably modify device_link_remove() to check if the consumer
>> device has been registered and run __device_link_del() directly
>> instead of device_link_put_kref() if it hasn't.
>>
>> Or add an argument to it to force the removal.
> 
> Or even modify device_link_put_kref() like this:
> 
>  static void device_link_put_kref(struct device_link *link)
>  {
>           if (link->flags & DL_FLAG_STATELESS)
>                   kref_put(&link->kref, __device_link_del);
> +        else if (!device_is_registered(link->consumer))
> +                __device_link_del(link);
>           else
>                  WARN(1, "Unable to drop a managed device link reference\n");
>  }
> 

Thanks! :-) I will do that.

Re: [PATCH RFC 2/2] scsi: ufshcd: Fix device links when BOOT WLUN fails to probe

[Saravana Kannan]

On Thu, Jul 8, 2021 at 7:17 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 8/07/21 3:31 pm, Rafael J. Wysocki wrote:
> > On Wed, Jul 7, 2021 at 7:49 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
> >>
> >> On 7/07/21 8:39 pm, Greg Kroah-Hartman wrote:
> >>> On Wed, Jul 07, 2021 at 08:29:48PM +0300, Adrian Hunter wrote:
> >>>> If a LUN fails to probe (e.g. absent BOOT WLUN), the device will not have
> >>>> been registered but can still have a device link holding a reference to the
> >>>> device. The unwanted device link will prevent runtime suspend indefinitely,
> >>>> and cause some warnings if the supplier is ever deleted (e.g. by unbinding
> >>>> the UFS host controller). Fix by explicitly deleting the device link when
> >>>> SCSI destroys the SCSI device.
> >>>>
> >>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> >>>> ---
> >>>>  drivers/scsi/ufs/ufshcd.c | 7 +++++++
> >>>>  1 file changed, 7 insertions(+)
> >>>>
> >>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> >>>> index 708b3b62fc4d..483aa74fe2c8 100644
> >>>> --- a/drivers/scsi/ufs/ufshcd.c
> >>>> +++ b/drivers/scsi/ufs/ufshcd.c
> >>>> @@ -5029,6 +5029,13 @@ static void ufshcd_slave_destroy(struct scsi_device *sdev)
> >>>>              spin_lock_irqsave(hba->host->host_lock, flags);
> >>>>              hba->sdev_ufs_device = NULL;
> >>>>              spin_unlock_irqrestore(hba->host->host_lock, flags);
> >>>> +    } else {
> >>>> +            /*
> >>>> +             * If a LUN fails to probe (e.g. absent BOOT WLUN), the device
> >>>> +             * will not have been registered but can still have a device
> >>>> +             * link holding a reference to the device.
> >>>> +             */
> >>>> +            device_links_scrap(&sdev->sdev_gendev);
> >>>
> >>> What created that link?  And why did it do that before probe happened
> >>> successfully?
> >>
> >> The same driver created the link.
> >>
> >> The documentation seems to say it is allowed to, if it is the consumer.
> >> From Documentation/driver-api/device_link.rst
> >>
> >>   Usage
> >>   =====
> >>
> >>   The earliest point in time when device links can be added is after
> >>   :c:func:`device_add()` has been called for the supplier and
> >>   :c:func:`device_initialize()` has been called for the consumer.
> >
> > Yes, this is allowed, but if you've added device links to a device
> > object that is not going to be registered after all, you are
> > responsible for doing the cleanup.
> >
> > Why can't you call device_link_del() directly on those links?
> >
> > Or device_link_remove() if you don't want to deal with link pointers?
> >
>
> Those only work for DL_FLAG_STATELESS device links, but we use only
> DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE flags.

Is there a reason you can't use DL_FLAG_STATELESS? It doesn't preclude
you from using RPM_ACTIVE as far as I can tell.

-Saravana


-Saravana

Re: [PATCH RFC 2/2] scsi: ufshcd: Fix device links when BOOT WLUN fails to probe

[Rafael J. Wysocki]

On Thu, Jul 8, 2021 at 6:46 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Thu, Jul 8, 2021 at 7:17 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
> >
> > On 8/07/21 3:31 pm, Rafael J. Wysocki wrote:
> > > On Wed, Jul 7, 2021 at 7:49 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
> > >>
> > >> On 7/07/21 8:39 pm, Greg Kroah-Hartman wrote:
> > >>> On Wed, Jul 07, 2021 at 08:29:48PM +0300, Adrian Hunter wrote:
> > >>>> If a LUN fails to probe (e.g. absent BOOT WLUN), the device will not have
> > >>>> been registered but can still have a device link holding a reference to the
> > >>>> device. The unwanted device link will prevent runtime suspend indefinitely,
> > >>>> and cause some warnings if the supplier is ever deleted (e.g. by unbinding
> > >>>> the UFS host controller). Fix by explicitly deleting the device link when
> > >>>> SCSI destroys the SCSI device.
> > >>>>
> > >>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> > >>>> ---
> > >>>>  drivers/scsi/ufs/ufshcd.c | 7 +++++++
> > >>>>  1 file changed, 7 insertions(+)
> > >>>>
> > >>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > >>>> index 708b3b62fc4d..483aa74fe2c8 100644
> > >>>> --- a/drivers/scsi/ufs/ufshcd.c
> > >>>> +++ b/drivers/scsi/ufs/ufshcd.c
> > >>>> @@ -5029,6 +5029,13 @@ static void ufshcd_slave_destroy(struct scsi_device *sdev)
> > >>>>              spin_lock_irqsave(hba->host->host_lock, flags);
> > >>>>              hba->sdev_ufs_device = NULL;
> > >>>>              spin_unlock_irqrestore(hba->host->host_lock, flags);
> > >>>> +    } else {
> > >>>> +            /*
> > >>>> +             * If a LUN fails to probe (e.g. absent BOOT WLUN), the device
> > >>>> +             * will not have been registered but can still have a device
> > >>>> +             * link holding a reference to the device.
> > >>>> +             */
> > >>>> +            device_links_scrap(&sdev->sdev_gendev);
> > >>>
> > >>> What created that link?  And why did it do that before probe happened
> > >>> successfully?
> > >>
> > >> The same driver created the link.
> > >>
> > >> The documentation seems to say it is allowed to, if it is the consumer.
> > >> From Documentation/driver-api/device_link.rst
> > >>
> > >>   Usage
> > >>   =====
> > >>
> > >>   The earliest point in time when device links can be added is after
> > >>   :c:func:`device_add()` has been called for the supplier and
> > >>   :c:func:`device_initialize()` has been called for the consumer.
> > >
> > > Yes, this is allowed, but if you've added device links to a device
> > > object that is not going to be registered after all, you are
> > > responsible for doing the cleanup.
> > >
> > > Why can't you call device_link_del() directly on those links?
> > >
> > > Or device_link_remove() if you don't want to deal with link pointers?
> > >
> >
> > Those only work for DL_FLAG_STATELESS device links, but we use only
> > DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE flags.
>
> Is there a reason you can't use DL_FLAG_STATELESS? It doesn't preclude
> you from using RPM_ACTIVE as far as I can tell.

Perhaps he wants the links to be managed if they are used after all.

Anyway, this is a valid use case that is not covered right now.

Re: [PATCH RFC 2/2] scsi: ufshcd: Fix device links when BOOT WLUN fails to probe

[Saravana Kannan]

On Thu, Jul 8, 2021 at 9:49 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Jul 8, 2021 at 6:46 PM Saravana Kannan <saravanak@google.com> wrote:
> >
> > On Thu, Jul 8, 2021 at 7:17 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
> > >
> > > On 8/07/21 3:31 pm, Rafael J. Wysocki wrote:
> > > > On Wed, Jul 7, 2021 at 7:49 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
> > > >>
> > > >> On 7/07/21 8:39 pm, Greg Kroah-Hartman wrote:
> > > >>> On Wed, Jul 07, 2021 at 08:29:48PM +0300, Adrian Hunter wrote:
> > > >>>> If a LUN fails to probe (e.g. absent BOOT WLUN), the device will not have
> > > >>>> been registered but can still have a device link holding a reference to the
> > > >>>> device. The unwanted device link will prevent runtime suspend indefinitely,
> > > >>>> and cause some warnings if the supplier is ever deleted (e.g. by unbinding
> > > >>>> the UFS host controller). Fix by explicitly deleting the device link when
> > > >>>> SCSI destroys the SCSI device.
> > > >>>>
> > > >>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> > > >>>> ---
> > > >>>>  drivers/scsi/ufs/ufshcd.c | 7 +++++++
> > > >>>>  1 file changed, 7 insertions(+)
> > > >>>>
> > > >>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > > >>>> index 708b3b62fc4d..483aa74fe2c8 100644
> > > >>>> --- a/drivers/scsi/ufs/ufshcd.c
> > > >>>> +++ b/drivers/scsi/ufs/ufshcd.c
> > > >>>> @@ -5029,6 +5029,13 @@ static void ufshcd_slave_destroy(struct scsi_device *sdev)
> > > >>>>              spin_lock_irqsave(hba->host->host_lock, flags);
> > > >>>>              hba->sdev_ufs_device = NULL;
> > > >>>>              spin_unlock_irqrestore(hba->host->host_lock, flags);
> > > >>>> +    } else {
> > > >>>> +            /*
> > > >>>> +             * If a LUN fails to probe (e.g. absent BOOT WLUN), the device
> > > >>>> +             * will not have been registered but can still have a device
> > > >>>> +             * link holding a reference to the device.
> > > >>>> +             */
> > > >>>> +            device_links_scrap(&sdev->sdev_gendev);
> > > >>>
> > > >>> What created that link?  And why did it do that before probe happened
> > > >>> successfully?
> > > >>
> > > >> The same driver created the link.
> > > >>
> > > >> The documentation seems to say it is allowed to, if it is the consumer.
> > > >> From Documentation/driver-api/device_link.rst
> > > >>
> > > >>   Usage
> > > >>   =====
> > > >>
> > > >>   The earliest point in time when device links can be added is after
> > > >>   :c:func:`device_add()` has been called for the supplier and
> > > >>   :c:func:`device_initialize()` has been called for the consumer.
> > > >
> > > > Yes, this is allowed, but if you've added device links to a device
> > > > object that is not going to be registered after all, you are
> > > > responsible for doing the cleanup.
> > > >
> > > > Why can't you call device_link_del() directly on those links?
> > > >
> > > > Or device_link_remove() if you don't want to deal with link pointers?
> > > >
> > >
> > > Those only work for DL_FLAG_STATELESS device links, but we use only
> > > DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE flags.
> >
> > Is there a reason you can't use DL_FLAG_STATELESS? It doesn't preclude
> > you from using RPM_ACTIVE as far as I can tell.
>
> Perhaps he wants the links to be managed if they are used after all.
>
> Anyway, this is a valid use case that is not covered right now.

Maybe. But the suggested patch is certainly risky.

There is no requirement the consumer is registered before the links
are added though. So, randomly deleting a managed link when
device_link_put_kref() is called on a stateless refcount (they are
still the same link still) isn't right. The entity that created the
managed device link might still want it there. Also, if two entities
create a managed link and one of them calls device_link_put_kref()
before the device is registered, we have a UAF problem because managed
links aren't refcounted (more than once).

-Saravana

Re: [PATCH RFC 2/2] scsi: ufshcd: Fix device links when BOOT WLUN fails to probe

[Rafael J. Wysocki]

On Thu, Jul 8, 2021 at 6:57 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Thu, Jul 8, 2021 at 9:49 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Thu, Jul 8, 2021 at 6:46 PM Saravana Kannan <saravanak@google.com> wrote:
> > >
> > > On Thu, Jul 8, 2021 at 7:17 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
> > > >
> > > > On 8/07/21 3:31 pm, Rafael J. Wysocki wrote:
> > > > > On Wed, Jul 7, 2021 at 7:49 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
> > > > >>
> > > > >> On 7/07/21 8:39 pm, Greg Kroah-Hartman wrote:
> > > > >>> On Wed, Jul 07, 2021 at 08:29:48PM +0300, Adrian Hunter wrote:
> > > > >>>> If a LUN fails to probe (e.g. absent BOOT WLUN), the device will not have
> > > > >>>> been registered but can still have a device link holding a reference to the
> > > > >>>> device. The unwanted device link will prevent runtime suspend indefinitely,
> > > > >>>> and cause some warnings if the supplier is ever deleted (e.g. by unbinding
> > > > >>>> the UFS host controller). Fix by explicitly deleting the device link when
> > > > >>>> SCSI destroys the SCSI device.
> > > > >>>>
> > > > >>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> > > > >>>> ---
> > > > >>>>  drivers/scsi/ufs/ufshcd.c | 7 +++++++
> > > > >>>>  1 file changed, 7 insertions(+)
> > > > >>>>
> > > > >>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > > > >>>> index 708b3b62fc4d..483aa74fe2c8 100644
> > > > >>>> --- a/drivers/scsi/ufs/ufshcd.c
> > > > >>>> +++ b/drivers/scsi/ufs/ufshcd.c
> > > > >>>> @@ -5029,6 +5029,13 @@ static void ufshcd_slave_destroy(struct scsi_device *sdev)
> > > > >>>>              spin_lock_irqsave(hba->host->host_lock, flags);
> > > > >>>>              hba->sdev_ufs_device = NULL;
> > > > >>>>              spin_unlock_irqrestore(hba->host->host_lock, flags);
> > > > >>>> +    } else {
> > > > >>>> +            /*
> > > > >>>> +             * If a LUN fails to probe (e.g. absent BOOT WLUN), the device
> > > > >>>> +             * will not have been registered but can still have a device
> > > > >>>> +             * link holding a reference to the device.
> > > > >>>> +             */
> > > > >>>> +            device_links_scrap(&sdev->sdev_gendev);
> > > > >>>
> > > > >>> What created that link?  And why did it do that before probe happened
> > > > >>> successfully?
> > > > >>
> > > > >> The same driver created the link.
> > > > >>
> > > > >> The documentation seems to say it is allowed to, if it is the consumer.
> > > > >> From Documentation/driver-api/device_link.rst
> > > > >>
> > > > >>   Usage
> > > > >>   =====
> > > > >>
> > > > >>   The earliest point in time when device links can be added is after
> > > > >>   :c:func:`device_add()` has been called for the supplier and
> > > > >>   :c:func:`device_initialize()` has been called for the consumer.
> > > > >
> > > > > Yes, this is allowed, but if you've added device links to a device
> > > > > object that is not going to be registered after all, you are
> > > > > responsible for doing the cleanup.
> > > > >
> > > > > Why can't you call device_link_del() directly on those links?
> > > > >
> > > > > Or device_link_remove() if you don't want to deal with link pointers?
> > > > >
> > > >
> > > > Those only work for DL_FLAG_STATELESS device links, but we use only
> > > > DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE flags.
> > >
> > > Is there a reason you can't use DL_FLAG_STATELESS? It doesn't preclude
> > > you from using RPM_ACTIVE as far as I can tell.
> >
> > Perhaps he wants the links to be managed if they are used after all.
> >
> > Anyway, this is a valid use case that is not covered right now.
>
> Maybe. But the suggested patch is certainly risky.
>
> There is no requirement the consumer is registered before the links
> are added though. So, randomly deleting a managed link when
> device_link_put_kref() is called on a stateless refcount (they are
> still the same link still) isn't right.

Device pointers are needed in order to create a device link and it is
quite unlikely that a pointer to an unregistered device will be shared
between two different pieces of code.

> The entity that created the
> managed device link might still want it there.

So the stateless kref is going to be put first.

> Also, if two entities
> create a managed link and one of them calls device_link_put_kref()
> before the device is registered, we have a UAF problem because managed
> links aren't refcounted (more than once).

IMO until a device object is registered, its creator should be allowed
to do the cleanup in the case when it gets released without
registration, including the removal of any device links to it that
have been added so far.

Messing up with a device object created by someone else that may still
go away without registration is a risky business regardless.