diff mbox series

[5/5] ACPI: scan: Fix race related to dropping dependencies

Message ID 3070024.5fSG56mABF@kreacher (mailing list archive)
State Superseded, archived
Headers show
Series ACPI: scan: Fixes and cleanups related to dependencies list handling | expand

Commit Message

Rafael J. Wysocki June 16, 2021, 2:25 p.m. UTC 
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

If acpi_add_single_object() runs concurrently with respect to
acpi_scan_clear_dep() which deletes a dependencies list entry where
the device being added is the consumer, the device's dep_unmet
counter may not be updated to reflect that change.

Namely, if the dependencies list entry is deleted right after
calling acpi_scan_dep_init() and before calling acpi_device_add(),
acpi_scan_clear_dep() will not find the device object corresponding
to the consumer device ACPI handle and it will not update its
dep_unmet counter to reflect the deletion of the list entry.
Consequently, the dep_unmet counter of the device will never
become zero going forward which may prevent it from being
completely enumerated.

To address this problem, modify acpi_add_single_object() to run
acpi_tie_acpi_dev(), to attach the ACPI device object created by it
to the corresponding ACPI namespace node, under acpi_dep_list_lock
along with acpi_scan_dep_init() whenever the latter is called.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/scan.c |   46 +++++++++++++++++++++++++++++++++-------------
 1 file changed, 33 insertions(+), 13 deletions(-)

Comments

Hans de Goede June 16, 2021, 2:55 p.m. UTC | #1 
Hi,

On 6/16/21 4:25 PM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> If acpi_add_single_object() runs concurrently with respect to
> acpi_scan_clear_dep() which deletes a dependencies list entry where
> the device being added is the consumer, the device's dep_unmet
> counter may not be updated to reflect that change.
> 
> Namely, if the dependencies list entry is deleted right after
> calling acpi_scan_dep_init() and before calling acpi_device_add(),
> acpi_scan_clear_dep() will not find the device object corresponding
> to the consumer device ACPI handle and it will not update its
> dep_unmet counter to reflect the deletion of the list entry.
> Consequently, the dep_unmet counter of the device will never
> become zero going forward which may prevent it from being
> completely enumerated.
> 
> To address this problem, modify acpi_add_single_object() to run
> acpi_tie_acpi_dev(), to attach the ACPI device object created by it
> to the corresponding ACPI namespace node, under acpi_dep_list_lock
> along with acpi_scan_dep_init() whenever the latter is called.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/acpi/scan.c |   46 +++++++++++++++++++++++++++++++++-------------
>  1 file changed, 33 insertions(+), 13 deletions(-)
> 
> Index: linux-pm/drivers/acpi/scan.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/scan.c
> +++ linux-pm/drivers/acpi/scan.c
> @@ -657,16 +657,12 @@ static int acpi_tie_acpi_dev(struct acpi
>  	return 0;
>  }
>  
> -int acpi_device_add(struct acpi_device *device,
> -		    void (*release)(struct device *))
> +int __acpi_device_add(struct acpi_device *device,
> +		      void (*release)(struct device *))
>  {
>  	struct acpi_device_bus_id *acpi_device_bus_id;
>  	int result;
>  
> -	result = acpi_tie_acpi_dev(device);
> -	if (result)
> -		return result;
> -
>  	/*
>  	 * Linkage
>  	 * -------
> @@ -755,6 +751,17 @@ err_unlock:
>  	return result;
>  }
>  
> +int acpi_device_add(struct acpi_device *adev, void (*release)(struct device *))
> +{
> +	int ret;
> +
> +	ret = acpi_tie_acpi_dev(adev);
> +	if (ret)
> +		return ret;
> +
> +	return __acpi_device_add(adev, release);
> +}
> +
>  /* --------------------------------------------------------------------------
>                                   Device Enumeration
>     -------------------------------------------------------------------------- */
> @@ -1681,14 +1688,10 @@ static void acpi_scan_dep_init(struct ac
>  {
>  	struct acpi_dep_data *dep;
>  
> -	mutex_lock(&acpi_dep_list_lock);
> -
>  	list_for_each_entry(dep, &acpi_dep_list, node) {
>  		if (dep->consumer == adev->handle)
>  			adev->dep_unmet++;
>  	}
> -
> -	mutex_unlock(&acpi_dep_list_lock);
>  }
>  
>  void acpi_device_add_finalize(struct acpi_device *device)
> @@ -1707,6 +1710,7 @@ static int acpi_add_single_object(struct
>  				  acpi_handle handle, int type, bool dep_init)
>  {
>  	struct acpi_device *device;
> +	bool release_dep_lock = false;
>  	int result;
>  
>  	device = kzalloc(sizeof(struct acpi_device), GFP_KERNEL);
> @@ -1720,16 +1724,32 @@ static int acpi_add_single_object(struct
>  	 * this must be done before the get power-/wakeup_dev-flags calls.
>  	 */
>  	if (type == ACPI_BUS_TYPE_DEVICE || type == ACPI_BUS_TYPE_PROCESSOR) {
> -		if (dep_init)
> +		if (dep_init) {
> +			mutex_lock(&acpi_dep_list_lock);
> +			/*
> +			 * Hold the lock until the acpi_tie_acpi_dev() call
> +			 * below to prevent concurrent acpi_scan_clear_dep()
> +			 * from deleting a dependency list entry without
> +			 * updating dep_unmet for the device.
> +			 */
> +			release_dep_lock = true;
>  			acpi_scan_dep_init(device);
> -
> +		}
>  		acpi_scan_init_status(device);
>  	}
>  
>  	acpi_bus_get_power_flags(device);
>  	acpi_bus_get_wakeup_device_flags(device);
>  
> -	result = acpi_device_add(device, acpi_device_release);
> +	result = acpi_tie_acpi_dev(device);
> +
> +	if (release_dep_lock)
> +		mutex_unlock(&acpi_dep_list_lock);
> +
> +	if (result)

AFAICT you are missing a "acpi_device_release(&device->dev);"
call in the error-exit path here, causing a mem-leak.

Otherwise this looks good, with the above fixed this is:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


> +		return result;
> +
> +	result = __acpi_device_add(device, acpi_device_release);
>  	if (result) {
>  		acpi_device_release(&device->dev);
>  		return result;
> 
> 
>
Rafael J. Wysocki June 16, 2021, 3:19 p.m. UTC | #2 
On Wed, Jun 16, 2021 at 4:55 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 6/16/21 4:25 PM, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > If acpi_add_single_object() runs concurrently with respect to
> > acpi_scan_clear_dep() which deletes a dependencies list entry where
> > the device being added is the consumer, the device's dep_unmet
> > counter may not be updated to reflect that change.
> >
> > Namely, if the dependencies list entry is deleted right after
> > calling acpi_scan_dep_init() and before calling acpi_device_add(),
> > acpi_scan_clear_dep() will not find the device object corresponding
> > to the consumer device ACPI handle and it will not update its
> > dep_unmet counter to reflect the deletion of the list entry.
> > Consequently, the dep_unmet counter of the device will never
> > become zero going forward which may prevent it from being
> > completely enumerated.
> >
> > To address this problem, modify acpi_add_single_object() to run
> > acpi_tie_acpi_dev(), to attach the ACPI device object created by it
> > to the corresponding ACPI namespace node, under acpi_dep_list_lock
> > along with acpi_scan_dep_init() whenever the latter is called.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/acpi/scan.c |   46 +++++++++++++++++++++++++++++++++-------------
> >  1 file changed, 33 insertions(+), 13 deletions(-)
> >
> > Index: linux-pm/drivers/acpi/scan.c
> > ===================================================================
> > --- linux-pm.orig/drivers/acpi/scan.c
> > +++ linux-pm/drivers/acpi/scan.c
> > @@ -657,16 +657,12 @@ static int acpi_tie_acpi_dev(struct acpi
> >       return 0;
> >  }
> >
> > -int acpi_device_add(struct acpi_device *device,
> > -                 void (*release)(struct device *))
> > +int __acpi_device_add(struct acpi_device *device,
> > +                   void (*release)(struct device *))
> >  {
> >       struct acpi_device_bus_id *acpi_device_bus_id;
> >       int result;
> >
> > -     result = acpi_tie_acpi_dev(device);
> > -     if (result)
> > -             return result;
> > -
> >       /*
> >        * Linkage
> >        * -------
> > @@ -755,6 +751,17 @@ err_unlock:
> >       return result;
> >  }
> >
> > +int acpi_device_add(struct acpi_device *adev, void (*release)(struct device *))
> > +{
> > +     int ret;
> > +
> > +     ret = acpi_tie_acpi_dev(adev);
> > +     if (ret)
> > +             return ret;
> > +
> > +     return __acpi_device_add(adev, release);
> > +}
> > +
> >  /* --------------------------------------------------------------------------
> >                                   Device Enumeration
> >     -------------------------------------------------------------------------- */
> > @@ -1681,14 +1688,10 @@ static void acpi_scan_dep_init(struct ac
> >  {
> >       struct acpi_dep_data *dep;
> >
> > -     mutex_lock(&acpi_dep_list_lock);
> > -
> >       list_for_each_entry(dep, &acpi_dep_list, node) {
> >               if (dep->consumer == adev->handle)
> >                       adev->dep_unmet++;
> >       }
> > -
> > -     mutex_unlock(&acpi_dep_list_lock);
> >  }
> >
> >  void acpi_device_add_finalize(struct acpi_device *device)
> > @@ -1707,6 +1710,7 @@ static int acpi_add_single_object(struct
> >                                 acpi_handle handle, int type, bool dep_init)
> >  {
> >       struct acpi_device *device;
> > +     bool release_dep_lock = false;
> >       int result;
> >
> >       device = kzalloc(sizeof(struct acpi_device), GFP_KERNEL);
> > @@ -1720,16 +1724,32 @@ static int acpi_add_single_object(struct
> >        * this must be done before the get power-/wakeup_dev-flags calls.
> >        */
> >       if (type == ACPI_BUS_TYPE_DEVICE || type == ACPI_BUS_TYPE_PROCESSOR) {
> > -             if (dep_init)
> > +             if (dep_init) {
> > +                     mutex_lock(&acpi_dep_list_lock);
> > +                     /*
> > +                      * Hold the lock until the acpi_tie_acpi_dev() call
> > +                      * below to prevent concurrent acpi_scan_clear_dep()
> > +                      * from deleting a dependency list entry without
> > +                      * updating dep_unmet for the device.
> > +                      */
> > +                     release_dep_lock = true;
> >                       acpi_scan_dep_init(device);
> > -
> > +             }
> >               acpi_scan_init_status(device);
> >       }
> >
> >       acpi_bus_get_power_flags(device);
> >       acpi_bus_get_wakeup_device_flags(device);
> >
> > -     result = acpi_device_add(device, acpi_device_release);
> > +     result = acpi_tie_acpi_dev(device);
> > +
> > +     if (release_dep_lock)
> > +             mutex_unlock(&acpi_dep_list_lock);
> > +
> > +     if (result)
>
> AFAICT you are missing a "acpi_device_release(&device->dev);"
> call in the error-exit path here, causing a mem-leak.

Indeed.

I'll send a v2 of this patch alone to fix this issue.

> Otherwise this looks good, with the above fixed this is:
>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Thanks!
kernel test robot June 17, 2021, 12:25 a.m. UTC | #3 
Hi "Rafael,

I love your patch! Perhaps something to improve:

[auto build test WARNING on pm/linux-next]
[also build test WARNING on next-20210616]
[cannot apply to linux/master linus/master v5.13-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Rafael-J-Wysocki/ACPI-scan-Fixes-and-cleanups-related-to-dependencies-list-handling/20210617-013528
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
config: x86_64-randconfig-b001-20210615 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 64720f57bea6a6bf033feef4a5751ab9c0c3b401)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/6369a007980e42b3ba7bbfb9833146f1867c790b
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Rafael-J-Wysocki/ACPI-scan-Fixes-and-cleanups-related-to-dependencies-list-handling/20210617-013528
        git checkout 6369a007980e42b3ba7bbfb9833146f1867c790b
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/acpi/scan.c:660:5: warning: no previous prototype for function '__acpi_device_add' [-Wmissing-prototypes]
   int __acpi_device_add(struct acpi_device *device,
       ^
   drivers/acpi/scan.c:660:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int __acpi_device_add(struct acpi_device *device,
   ^
   static 
   1 warning generated.


vim +/__acpi_device_add +660 drivers/acpi/scan.c

   659	
 > 660	int __acpi_device_add(struct acpi_device *device,
   661			      void (*release)(struct device *))
   662	{
   663		struct acpi_device_bus_id *acpi_device_bus_id;
   664		int result;
   665	
   666		/*
   667		 * Linkage
   668		 * -------
   669		 * Link this device to its parent and siblings.
   670		 */
   671		INIT_LIST_HEAD(&device->children);
   672		INIT_LIST_HEAD(&device->node);
   673		INIT_LIST_HEAD(&device->wakeup_list);
   674		INIT_LIST_HEAD(&device->physical_node_list);
   675		INIT_LIST_HEAD(&device->del_list);
   676		mutex_init(&device->physical_node_lock);
   677	
   678		mutex_lock(&acpi_device_lock);
   679	
   680		acpi_device_bus_id = acpi_device_bus_id_match(acpi_device_hid(device));
   681		if (acpi_device_bus_id) {
   682			result = acpi_device_set_name(device, acpi_device_bus_id);
   683			if (result)
   684				goto err_unlock;
   685		} else {
   686			acpi_device_bus_id = kzalloc(sizeof(*acpi_device_bus_id),
   687						     GFP_KERNEL);
   688			if (!acpi_device_bus_id) {
   689				result = -ENOMEM;
   690				goto err_unlock;
   691			}
   692			acpi_device_bus_id->bus_id =
   693				kstrdup_const(acpi_device_hid(device), GFP_KERNEL);
   694			if (!acpi_device_bus_id->bus_id) {
   695				kfree(acpi_device_bus_id);
   696				result = -ENOMEM;
   697				goto err_unlock;
   698			}
   699	
   700			ida_init(&acpi_device_bus_id->instance_ida);
   701	
   702			result = acpi_device_set_name(device, acpi_device_bus_id);
   703			if (result) {
   704				kfree_const(acpi_device_bus_id->bus_id);
   705				kfree(acpi_device_bus_id);
   706				goto err_unlock;
   707			}
   708	
   709			list_add_tail(&acpi_device_bus_id->node, &acpi_bus_id_list);
   710		}
   711	
   712		if (device->parent)
   713			list_add_tail(&device->node, &device->parent->children);
   714	
   715		if (device->wakeup.flags.valid)
   716			list_add_tail(&device->wakeup_list, &acpi_wakeup_device_list);
   717	
   718		mutex_unlock(&acpi_device_lock);
   719	
   720		if (device->parent)
   721			device->dev.parent = &device->parent->dev;
   722	
   723		device->dev.bus = &acpi_bus_type;
   724		device->dev.release = release;
   725		result = device_add(&device->dev);
   726		if (result) {
   727			dev_err(&device->dev, "Error registering device\n");
   728			goto err;
   729		}
   730	
   731		result = acpi_device_setup_files(device);
   732		if (result)
   733			pr_err("Error creating sysfs interface for device %s\n",
   734			       dev_name(&device->dev));
   735	
   736		return 0;
   737	
   738	err:
   739		mutex_lock(&acpi_device_lock);
   740	
   741		if (device->parent)
   742			list_del(&device->node);
   743	
   744		list_del(&device->wakeup_list);
   745	
   746	err_unlock:
   747		mutex_unlock(&acpi_device_lock);
   748	
   749		acpi_detach_data(device->handle, acpi_scan_drop_device);
   750	
   751		return result;
   752	}
   753	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
kernel test robot June 17, 2021, 12:41 a.m. UTC | #4 
Hi "Rafael,

I love your patch! Perhaps something to improve:

[auto build test WARNING on pm/linux-next]
[also build test WARNING on next-20210616]
[cannot apply to linux/master linus/master v5.13-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Rafael-J-Wysocki/ACPI-scan-Fixes-and-cleanups-related-to-dependencies-list-handling/20210617-013528
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
config: i386-randconfig-s001-20210615 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.3-341-g8af24329-dirty
        # https://github.com/0day-ci/linux/commit/6369a007980e42b3ba7bbfb9833146f1867c790b
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Rafael-J-Wysocki/ACPI-scan-Fixes-and-cleanups-related-to-dependencies-list-handling/20210617-013528
        git checkout 6369a007980e42b3ba7bbfb9833146f1867c790b
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)
>> drivers/acpi/scan.c:660:5: sparse: sparse: symbol '__acpi_device_add' was not declared. Should it be static?

Please review and possibly fold the followup patch.

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff mbox series

Patch

Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -657,16 +657,12 @@  static int acpi_tie_acpi_dev(struct acpi
 	return 0;
 }
 
-int acpi_device_add(struct acpi_device *device,
-		    void (*release)(struct device *))
+int __acpi_device_add(struct acpi_device *device,
+		      void (*release)(struct device *))
 {
 	struct acpi_device_bus_id *acpi_device_bus_id;
 	int result;
 
-	result = acpi_tie_acpi_dev(device);
-	if (result)
-		return result;
-
 	/*
 	 * Linkage
 	 * -------
@@ -755,6 +751,17 @@  err_unlock:
 	return result;
 }
 
+int acpi_device_add(struct acpi_device *adev, void (*release)(struct device *))
+{
+	int ret;
+
+	ret = acpi_tie_acpi_dev(adev);
+	if (ret)
+		return ret;
+
+	return __acpi_device_add(adev, release);
+}
+
 /* --------------------------------------------------------------------------
                                  Device Enumeration
    -------------------------------------------------------------------------- */
@@ -1681,14 +1688,10 @@  static void acpi_scan_dep_init(struct ac
 {
 	struct acpi_dep_data *dep;
 
-	mutex_lock(&acpi_dep_list_lock);
-
 	list_for_each_entry(dep, &acpi_dep_list, node) {
 		if (dep->consumer == adev->handle)
 			adev->dep_unmet++;
 	}
-
-	mutex_unlock(&acpi_dep_list_lock);
 }
 
 void acpi_device_add_finalize(struct acpi_device *device)
@@ -1707,6 +1710,7 @@  static int acpi_add_single_object(struct
 				  acpi_handle handle, int type, bool dep_init)
 {
 	struct acpi_device *device;
+	bool release_dep_lock = false;
 	int result;
 
 	device = kzalloc(sizeof(struct acpi_device), GFP_KERNEL);
@@ -1720,16 +1724,32 @@  static int acpi_add_single_object(struct
 	 * this must be done before the get power-/wakeup_dev-flags calls.
 	 */
 	if (type == ACPI_BUS_TYPE_DEVICE || type == ACPI_BUS_TYPE_PROCESSOR) {
-		if (dep_init)
+		if (dep_init) {
+			mutex_lock(&acpi_dep_list_lock);
+			/*
+			 * Hold the lock until the acpi_tie_acpi_dev() call
+			 * below to prevent concurrent acpi_scan_clear_dep()
+			 * from deleting a dependency list entry without
+			 * updating dep_unmet for the device.
+			 */
+			release_dep_lock = true;
 			acpi_scan_dep_init(device);
-
+		}
 		acpi_scan_init_status(device);
 	}
 
 	acpi_bus_get_power_flags(device);
 	acpi_bus_get_wakeup_device_flags(device);
 
-	result = acpi_device_add(device, acpi_device_release);
+	result = acpi_tie_acpi_dev(device);
+
+	if (release_dep_lock)
+		mutex_unlock(&acpi_dep_list_lock);
+
+	if (result)
+		return result;
+
+	result = __acpi_device_add(device, acpi_device_release);
 	if (result) {
 		acpi_device_release(&device->dev);
 		return result;