23222f8f8dce6: acpi, nfit: Add function to look up nvdimm device and provide SMBIOS handle

23222f8f8dce6: acpi, nfit: Add function to look up nvdimm device and provide SMBIOS handle

[Jane Chu]

Hi, Tony,

I happen to be looking at the above patch, and noticed below change in
drivers/acpi/nfit/core.c

+       mutex_lock(&acpi_desc_lock);
+       list_for_each_entry(acpi_desc, &acpi_descs, list) {
+               mutex_lock(&acpi_desc->init_mutex);
+               list_for_each_entry(nfit_mem, &acpi_desc->dimms, list) {
+                       memdev = __to_nfit_memdev(nfit_mem);
+                       if (memdev->device_handle == device_handle) {
+                               mutex_unlock(&acpi_desc->init_mutex);
+                               mutex_unlock(&acpi_desc_lock);
+                               *flags = memdev->flags;   <-----
+                               return memdev->physical_id;
+                       }
+               }
+               mutex_unlock(&acpi_desc->init_mutex);
+       }
+       mutex_unlock(&acpi_desc_lock);

Is there a reason to retrieve the memdev->flags value after releasing
the locks?

Thanks!
-jane

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

RE: 23222f8f8dce6: acpi, nfit: Add function to look up nvdimm device and provide SMBIOS handle

[Luck, Tony]

Jane,

It does look a bit fishy.  But if there is an issue with “memdev” changing once we release the locks,
then it will only help a little to move the “*flags = memdev->flags” back before we release the locks.
Note that we access memdev one more time with the “return memdev->physical_id”

But I’m not at all sure what the possible races are here. I hope someone from the linx-nvdimm list
can comment. I think I just cut & pasted the bulk of those nested loops from elsewhere. As far as I
know we don’t currently have any support for hotplug NFIT devices, so I suspect there is currently
no race here.

-Tony

From: Jane Chu [mailto:jane.chu@oracle.com]
Sent: Friday, January 11, 2019 1:43 PM
To: Luck, Tony <tony.luck@intel.com>
Cc: linux-nvdimm <linux-nvdimm@lists.01.org>
Subject: 23222f8f8dce6: acpi, nfit: Add function to look up nvdimm device and provide SMBIOS handle


Hi, Tony,



I happen to be looking at the above patch, and noticed below change in

drivers/acpi/nfit/core.c



+       mutex_lock(&acpi_desc_lock);

+       list_for_each_entry(acpi_desc, &acpi_descs, list) {

+               mutex_lock(&acpi_desc->init_mutex);

+               list_for_each_entry(nfit_mem, &acpi_desc->dimms, list) {

+                       memdev = __to_nfit_memdev(nfit_mem);

+                       if (memdev->device_handle == device_handle) {

+                               mutex_unlock(&acpi_desc->init_mutex);

+                               mutex_unlock(&acpi_desc_lock);

+                               *flags = memdev->flags;   <-----

+                               return memdev->physical_id;

+                       }

+               }

+               mutex_unlock(&acpi_desc->init_mutex);

+       }

+       mutex_unlock(&acpi_desc_lock);



Is there a reason to retrieve the memdev->flags value after releasing

the locks?



Thanks!

-jane


_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: 23222f8f8dce6: acpi, nfit: Add function to look up nvdimm device and provide SMBIOS handle

[Jane Chu]

On 1/11/2019 1:54 PM, Luck, Tony wrote:

> Jane,
>
> It does look a bit fishy.  But if there is an issue with “memdev” 
> changing once we release the locks,
>
> then it will only help a little to move the “*flags = memdev->flags” 
> back before we release the locks.
>
> Note that we access memdev one more time with the “return 
> memdev->physical_id”
>
Indeed, that line too.

> But I’m not at all sure what the possible races are here. I hope 
> someone from the linx-nvdimm list
>
> can comment. I think I just cut & pasted the bulk of those nested 
> loops from elsewhere. As far as I
>
> know we don’t currently have any support for hotplug NFIT devices, so 
> I suspect there is currently
>
> no race here.
>
I don't understand the bulk of the code, just trying to follow what you said
here.  So if we *ever* decide to support hotplug NFIT device, what would remind
us to close the race here?   Is there any harm to reference memdev while we
still have the locks?

Thanks!
-jane

> -Tony
>
> *From:*Jane Chu [mailto:jane.chu@oracle.com]
> *Sent:* Friday, January 11, 2019 1:43 PM
> *To:* Luck, Tony <tony.luck@intel.com>
> *Cc:* linux-nvdimm <linux-nvdimm@lists.01.org>
> *Subject:* 23222f8f8dce6: acpi, nfit: Add function to look up nvdimm 
> device and provide SMBIOS handle
>
> Hi, Tony,
> I happen to be looking at the above patch, and noticed below change in
> drivers/acpi/nfit/core.c
> +       mutex_lock(&acpi_desc_lock);
> +       list_for_each_entry(acpi_desc, &acpi_descs, list) {
> +               mutex_lock(&acpi_desc->init_mutex);
> +               list_for_each_entry(nfit_mem, &acpi_desc->dimms, list) {
> +                       memdev = __to_nfit_memdev(nfit_mem);
> +                       if (memdev->device_handle == device_handle) {
> +                               mutex_unlock(&acpi_desc->init_mutex);
> +                               mutex_unlock(&acpi_desc_lock);
> +                               *flags = memdev->flags;   <-----
> +                               return memdev->physical_id;
> +                       }
> +               }
> +               mutex_unlock(&acpi_desc->init_mutex);
> +       }
> +       mutex_unlock(&acpi_desc_lock);
> Is there a reason to retrieve the memdev->flags value after releasing
> the locks?
> Thanks!
> -jane
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: 23222f8f8dce6: acpi, nfit: Add function to look up nvdimm device and provide SMBIOS handle

[Dan Williams]

On Fri, Jan 11, 2019 at 2:03 PM Jane Chu <jane.chu@oracle.com> wrote:
>
> On 1/11/2019 1:54 PM, Luck, Tony wrote:
>
> > Jane,
> >
> > It does look a bit fishy.  But if there is an issue with “memdev”
> > changing once we release the locks,
> >
> > then it will only help a little to move the “*flags = memdev->flags”
> > back before we release the locks.
> >
> > Note that we access memdev one more time with the “return
> > memdev->physical_id”
> >
> Indeed, that line too.
>
> > But I’m not at all sure what the possible races are here. I hope
> > someone from the linx-nvdimm list
> >
> > can comment. I think I just cut & pasted the bulk of those nested
> > loops from elsewhere. As far as I
> >
> > know we don’t currently have any support for hotplug NFIT devices, so
> > I suspect there is currently
> >
> > no race here.
> >
> I don't understand the bulk of the code, just trying to follow what you said
> here.  So if we *ever* decide to support hotplug NFIT device, what would remind
> us to close the race here?   Is there any harm to reference memdev while we
> still have the locks?

No harm in fixing it up, I'd take that patch if you wrote it up.

There is potentially a race today if the kernel was calling this
routine, but some other thread was doing:

   echo "ACPI0012:00" > /sys/bus/acpi/drivers/nfit/unbind
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: 23222f8f8dce6: acpi, nfit: Add function to look up nvdimm device and provide SMBIOS handle

[Luck, Tony]

On Fri, Jan 11, 2019 at 02:07:00PM -0800, Dan Williams wrote:
> No harm in fixing it up, I'd take that patch if you wrote it up.

Something like this?


diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 011d3db19c80..22945bf803c8 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -721,6 +721,7 @@ int nfit_get_smbios_id(u32 device_handle, u16 *flags)
 	struct acpi_nfit_memory_map *memdev;
 	struct acpi_nfit_desc *acpi_desc;
 	struct nfit_mem *nfit_mem;
+	u16 physical_id;
 
 	mutex_lock(&acpi_desc_lock);
 	list_for_each_entry(acpi_desc, &acpi_descs, list) {
@@ -728,10 +729,11 @@ int nfit_get_smbios_id(u32 device_handle, u16 *flags)
 		list_for_each_entry(nfit_mem, &acpi_desc->dimms, list) {
 			memdev = __to_nfit_memdev(nfit_mem);
 			if (memdev->device_handle == device_handle) {
+				*flags = memdev->flags;
+				physical_id = memdev->physical_id;
 				mutex_unlock(&acpi_desc->init_mutex);
 				mutex_unlock(&acpi_desc_lock);
-				*flags = memdev->flags;
-				return memdev->physical_id;
+				return physical_id;
 			}
 		}
 		mutex_unlock(&acpi_desc->init_mutex);
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: 23222f8f8dce6: acpi, nfit: Add function to look up nvdimm device and provide SMBIOS handle

[Dan Williams]

On Fri, Jan 11, 2019 at 2:12 PM Luck, Tony <tony.luck@intel.com> wrote:
>
> On Fri, Jan 11, 2019 at 02:07:00PM -0800, Dan Williams wrote:
> > No harm in fixing it up, I'd take that patch if you wrote it up.
>
> Something like this?
>
>
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index 011d3db19c80..22945bf803c8 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -721,6 +721,7 @@ int nfit_get_smbios_id(u32 device_handle, u16 *flags)
>         struct acpi_nfit_memory_map *memdev;
>         struct acpi_nfit_desc *acpi_desc;
>         struct nfit_mem *nfit_mem;
> +       u16 physical_id;
>
>         mutex_lock(&acpi_desc_lock);
>         list_for_each_entry(acpi_desc, &acpi_descs, list) {
> @@ -728,10 +729,11 @@ int nfit_get_smbios_id(u32 device_handle, u16 *flags)
>                 list_for_each_entry(nfit_mem, &acpi_desc->dimms, list) {
>                         memdev = __to_nfit_memdev(nfit_mem);
>                         if (memdev->device_handle == device_handle) {
> +                               *flags = memdev->flags;
> +                               physical_id = memdev->physical_id;
>                                 mutex_unlock(&acpi_desc->init_mutex);
>                                 mutex_unlock(&acpi_desc_lock);
> -                               *flags = memdev->flags;
> -                               return memdev->physical_id;
> +                               return physical_id;
>                         }
>                 }
>                 mutex_unlock(&acpi_desc->init_mutex);

Yup, looks good.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH] acpi/nfit: Fix race accessing memdev in nfit_get_smbios_id()

[Luck, Tony]

Possible race accessing memdev structures after dropping the
mutex. Dan Williams says this could race against another thread
that is doing:

 # echo "ACPI0012:00" > /sys/bus/acpi/drivers/nfit/unbind

Reported-by: Jane Chu <jane.chu@oracle.com>
Signed-off-by: Tony Luck <tony.luck@intel.com>
---

This time with a commit message and credit to Jane for finding it.

 drivers/acpi/nfit/core.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 011d3db19c80..22945bf803c8 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -721,6 +721,7 @@ int nfit_get_smbios_id(u32 device_handle, u16 *flags)
 	struct acpi_nfit_memory_map *memdev;
 	struct acpi_nfit_desc *acpi_desc;
 	struct nfit_mem *nfit_mem;
+	u16 physical_id;
 
 	mutex_lock(&acpi_desc_lock);
 	list_for_each_entry(acpi_desc, &acpi_descs, list) {
@@ -728,10 +729,11 @@ int nfit_get_smbios_id(u32 device_handle, u16 *flags)
 		list_for_each_entry(nfit_mem, &acpi_desc->dimms, list) {
 			memdev = __to_nfit_memdev(nfit_mem);
 			if (memdev->device_handle == device_handle) {
+				*flags = memdev->flags;
+				physical_id = memdev->physical_id;
 				mutex_unlock(&acpi_desc->init_mutex);
 				mutex_unlock(&acpi_desc_lock);
-				*flags = memdev->flags;
-				return memdev->physical_id;
+				return physical_id;
 			}
 		}
 		mutex_unlock(&acpi_desc->init_mutex);
-- 
2.19.1

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] acpi/nfit: Fix race accessing memdev in nfit_get_smbios_id()

[Dan Williams]

On Fri, Jan 11, 2019 at 2:46 PM Luck, Tony <tony.luck@intel.com> wrote:
>
>
> Possible race accessing memdev structures after dropping the
> mutex. Dan Williams says this could race against another thread
> that is doing:
>
>  # echo "ACPI0012:00" > /sys/bus/acpi/drivers/nfit/unbind
>
> Reported-by: Jane Chu <jane.chu@oracle.com>
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
>
> This time with a commit message and credit to Jane for finding it.

Thanks, applied.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm