All of lore.kernel.org
 help / color / mirror / Atom feed
From: Saravana Kannan <saravanak@google.com>
To: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"Martin K . Petersen" <martin.petersen@oracle.com>,
	"James E . J . Bottomley" <jejb@linux.ibm.com>,
	"open list:TARGET SUBSYSTEM" <linux-scsi@vger.kernel.org>,
	Avri Altman <avri.altman@wdc.com>, Bean Huo <huobean@gmail.com>,
	Can Guo <cang@codeaurora.org>,
	Asutosh Das <asutoshd@codeaurora.org>,
	Bart Van Assche <bvanassche@acm.org>,
	Linux PM <linux-pm@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH RFC 2/2] scsi: ufshcd: Fix device links when BOOT WLUN fails to probe
Date: Thu, 8 Jul 2021 09:57:01 -0700	[thread overview]
Message-ID: <CAGETcx9hYu-n+tGOnEspWOckvgVQG+QcZPoR-DwesPh1qfrnXA@mail.gmail.com> (raw)
In-Reply-To: <CAJZ5v0jnWWLyCFub=-ETr7d_ck=roVexTj8M0NRLi-svfjJy3Q@mail.gmail.com>

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

  reply	other threads:[~2021-07-08 16:57 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-07 17:29 [PATCH RFC 0/2] driver core: Add ability to delete device links of unregistered devices Adrian Hunter
2021-07-07 17:29 ` [PATCH RFC 1/2] " Adrian Hunter
2021-07-07 17:38   ` Greg Kroah-Hartman
2021-07-07 17:29 ` [PATCH RFC 2/2] scsi: ufshcd: Fix device links when BOOT WLUN fails to probe Adrian Hunter
2021-07-07 17:39   ` Greg Kroah-Hartman
2021-07-07 17:49     ` Adrian Hunter
2021-07-08 12:31       ` Rafael J. Wysocki
2021-07-08 14:17         ` Adrian Hunter
2021-07-08 15:03           ` Rafael J. Wysocki
2021-07-08 15:12             ` Rafael J. Wysocki
2021-07-08 16:02               ` Adrian Hunter
2021-07-08 16:45           ` Saravana Kannan
2021-07-08 16:48             ` Rafael J. Wysocki
2021-07-08 16:57               ` Saravana Kannan [this message]
2021-07-08 17:39                 ` Rafael J. Wysocki
2021-07-07 17:40 ` [PATCH RFC 0/2] driver core: Add ability to delete device links of unregistered devices Greg Kroah-Hartman
2021-07-07 17:41   ` Saravana Kannan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAGETcx9hYu-n+tGOnEspWOckvgVQG+QcZPoR-DwesPh1qfrnXA@mail.gmail.com \
    --to=saravanak@google.com \
    --cc=adrian.hunter@intel.com \
    --cc=asutoshd@codeaurora.org \
    --cc=avri.altman@wdc.com \
    --cc=bvanassche@acm.org \
    --cc=cang@codeaurora.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=huobean@gmail.com \
    --cc=jejb@linux.ibm.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=linux-scsi@vger.kernel.org \
    --cc=martin.petersen@oracle.com \
    --cc=rafael@kernel.org \
    --subject='Re: [PATCH RFC 2/2] scsi: ufshcd: Fix device links when BOOT WLUN fails to probe' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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.