[patch 9/9] acpi: fix NULL bug for HID/UID string

[patch 9/9] acpi: fix NULL bug for HID/UID string

[akpm]

From: Lin Ming <ming.m.lin@intel.com>

acpi_device->pnp.hardware_id and unique_id are now allocated pointer. 
If it's NULL, return "\0" for acpi_device_hid and acpi_device_uid. 
Also, free hardware_id and unique_id when acpi_device is going to be
freed.

Cc: Valdis Kletnieks <Valdis.Kletnieks@vt.edu>
Cc: Hugh Dickins <hugh.dickins@tiscali.co.uk>
Cc: Len Brown <lenb@kernel.org>
Signed-off-by: Lin Ming <ming.m.lin@intel.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 drivers/acpi/scan.c     |    4 ++++
 include/acpi/acpi_bus.h |    4 ++--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff -puN drivers/acpi/scan.c~acpi-fix-null-bug-for-hid-uid-string drivers/acpi/scan.c
--- a/drivers/acpi/scan.c~acpi-fix-null-bug-for-hid-uid-string
+++ a/drivers/acpi/scan.c
@@ -309,6 +309,8 @@ static void acpi_device_release(struct d
 	struct acpi_device *acpi_dev = to_acpi_device(dev);
 
 	kfree(acpi_dev->pnp.cid_list);
+	kfree(acpi_dev->pnp.hardware_id);
+	kfree(acpi_dev->pnp.unique_id);
 	kfree(acpi_dev);
 }
 
@@ -1359,6 +1361,8 @@ end:
 		*child = device;
 	else {
 		kfree(device->pnp.cid_list);
+		kfree(device->pnp.hardware_id);
+		kfree(device->pnp.unique_id);
 		kfree(device);
 	}
 
diff -puN include/acpi/acpi_bus.h~acpi-fix-null-bug-for-hid-uid-string include/acpi/acpi_bus.h
--- a/include/acpi/acpi_bus.h~acpi-fix-null-bug-for-hid-uid-string
+++ a/include/acpi/acpi_bus.h
@@ -186,8 +186,8 @@ struct acpi_device_pnp {
 
 #define acpi_device_bid(d)	((d)->pnp.bus_id)
 #define acpi_device_adr(d)	((d)->pnp.bus_address)
-#define acpi_device_hid(d)	((d)->pnp.hardware_id)
-#define acpi_device_uid(d)	((d)->pnp.unique_id)
+#define acpi_device_hid(d)	((d)->pnp.hardware_id ? (d)->pnp.hardware_id : "\0")
+#define acpi_device_uid(d)	((d)->pnp.unique_id ? (d)->pnp.unique_id : "\0")
 #define acpi_device_name(d)	((d)->pnp.device_name)
 #define acpi_device_class(d)	((d)->pnp.device_class)
 
_

Re: [patch 9/9] acpi: fix NULL bug for HID/UID string

[Hugh Dickins]

On Thu, 6 Aug 2009, akpm@linux-foundation.org wrote:
> From: Lin Ming <ming.m.lin@intel.com>
> 
> acpi_device->pnp.hardware_id and unique_id are now allocated pointer. 
> If it's NULL, return "\0" for acpi_device_hid and acpi_device_uid. 
> Also, free hardware_id and unique_id when acpi_device is going to be
> freed.
> 
> Cc: Valdis Kletnieks <Valdis.Kletnieks@vt.edu>
> Cc: Hugh Dickins <hugh.dickins@tiscali.co.uk>
> Cc: Len Brown <lenb@kernel.org>
> Signed-off-by: Lin Ming <ming.m.lin@intel.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  drivers/acpi/scan.c     |    4 ++++
>  include/acpi/acpi_bus.h |    4 ++--
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff -puN drivers/acpi/scan.c~acpi-fix-null-bug-for-hid-uid-string drivers/acpi/scan.c
> --- a/drivers/acpi/scan.c~acpi-fix-null-bug-for-hid-uid-string
> +++ a/drivers/acpi/scan.c
> @@ -309,6 +309,8 @@ static void acpi_device_release(struct d
>  	struct acpi_device *acpi_dev = to_acpi_device(dev);
>  
>  	kfree(acpi_dev->pnp.cid_list);
> +	kfree(acpi_dev->pnp.hardware_id);
> +	kfree(acpi_dev->pnp.unique_id);
>  	kfree(acpi_dev);
>  }
>  
> @@ -1359,6 +1361,8 @@ end:
>  		*child = device;
>  	else {
>  		kfree(device->pnp.cid_list);
> +		kfree(device->pnp.hardware_id);
> +		kfree(device->pnp.unique_id);
>  		kfree(device);
>  	}
>  
> diff -puN include/acpi/acpi_bus.h~acpi-fix-null-bug-for-hid-uid-string include/acpi/acpi_bus.h
> --- a/include/acpi/acpi_bus.h~acpi-fix-null-bug-for-hid-uid-string
> +++ a/include/acpi/acpi_bus.h
> @@ -186,8 +186,8 @@ struct acpi_device_pnp {
>  
>  #define acpi_device_bid(d)	((d)->pnp.bus_id)
>  #define acpi_device_adr(d)	((d)->pnp.bus_address)
> -#define acpi_device_hid(d)	((d)->pnp.hardware_id)
> -#define acpi_device_uid(d)	((d)->pnp.unique_id)
> +#define acpi_device_hid(d)	((d)->pnp.hardware_id ? (d)->pnp.hardware_id : "\0")
> +#define acpi_device_uid(d)	((d)->pnp.unique_id ? (d)->pnp.unique_id : "\0")
>  #define acpi_device_name(d)	((d)->pnp.device_name)
>  #define acpi_device_class(d)	((d)->pnp.device_class)
>  
> _

It's up to Len to choose, but I thought we preferred...

From: Hugh Dickins <hugh.dickins@tiscali.co.uk>

acpi_device->pnp.hardware_id and unique_id are now allocated pointers,
replacing the previous arrays.  acpi_device_install_notify_handler()
oopsed on the NULL hid when probing the video device, and perhaps other
uses are vulnerable too.  So initialize those pointers to empty strings
when there is no hid or uid.  Also, free hardware_id and unique_id when
when acpi_device is going to be freed.

Signed-off-by: Hugh Dickins <hugh.dickins@tiscali.co.uk>
Signed-off-by: Lin Ming <ming.m.lin@intel.com>
---

 drivers/acpi/scan.c |   20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

--- mmotm/drivers/acpi/scan.c	2009-07-17 12:53:20.000000000 +0100
+++ linux/drivers/acpi/scan.c	2009-07-27 18:01:32.000000000 +0100
@@ -309,6 +309,10 @@ static void acpi_device_release(struct d
 	struct acpi_device *acpi_dev = to_acpi_device(dev);
 
 	kfree(acpi_dev->pnp.cid_list);
+	if (acpi_dev->flags.hardware_id)
+		kfree(acpi_dev->pnp.hardware_id);
+	if (acpi_dev->flags.unique_id)
+		kfree(acpi_dev->pnp.unique_id);
 	kfree(acpi_dev);
 }
 
@@ -1144,8 +1148,9 @@ static void acpi_device_set_id(struct ac
 			strcpy(device->pnp.hardware_id, hid);
 			device->flags.hardware_id = 1;
 		}
-	} else
-		device->pnp.hardware_id = NULL;
+	}
+	if (!device->flags.hardware_id)
+		device->pnp.hardware_id = "";
 
 	if (uid) {
 		device->pnp.unique_id = ACPI_ALLOCATE_ZEROED(strlen (uid) + 1);
@@ -1153,8 +1158,9 @@ static void acpi_device_set_id(struct ac
 			strcpy(device->pnp.unique_id, uid);
 			device->flags.unique_id = 1;
 		}
-	} else
-		device->pnp.unique_id = NULL;
+	}
+	if (!device->flags.unique_id)
+		device->pnp.unique_id = "";
 
 	if (cid_list || cid_add) {
 		struct acpica_device_id_list *list;
@@ -1369,10 +1375,8 @@ acpi_add_single_object(struct acpi_devic
 end:
 	if (!result)
 		*child = device;
-	else {
-		kfree(device->pnp.cid_list);
-		kfree(device);
-	}
+	else
+		acpi_device_release(&device->dev);
 
 	return result;
 }

Re: [patch 9/9] acpi: fix NULL bug for HID/UID string

[Bjorn Helgaas]

On Thursday 06 August 2009 05:18:12 pm Hugh Dickins wrote:
> It's up to Len to choose, but I thought we preferred...
> 
> From: Hugh Dickins <hugh.dickins@tiscali.co.uk>
> 
> acpi_device->pnp.hardware_id and unique_id are now allocated pointers,
> replacing the previous arrays.  acpi_device_install_notify_handler()
> oopsed on the NULL hid when probing the video device, 

I'm sorry I wasn't aware of the acpi_device_install_notify_handler()
issue until just now, since I added the code that oopses (46ec8598fd).

I recently posted patches that removed the dependency on the HID
in this path:

  http://marc.info/?l=linux-acpi&m=124907623701270&w=2

I thought Len said they had been applied to acpi-test, but I don't
see them there.

But I don't object to Hugh's patch, since it might be useful for other
paths.

Bjorn


> and perhaps other 
> uses are vulnerable too.  So initialize those pointers to empty strings
> when there is no hid or uid.  Also, free hardware_id and unique_id when
> when acpi_device is going to be freed.
> 
> Signed-off-by: Hugh Dickins <hugh.dickins@tiscali.co.uk>
> Signed-off-by: Lin Ming <ming.m.lin@intel.com>
> ---
> 
>  drivers/acpi/scan.c |   20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> --- mmotm/drivers/acpi/scan.c	2009-07-17 12:53:20.000000000 +0100
> +++ linux/drivers/acpi/scan.c	2009-07-27 18:01:32.000000000 +0100
> @@ -309,6 +309,10 @@ static void acpi_device_release(struct d
>  	struct acpi_device *acpi_dev = to_acpi_device(dev);
>  
>  	kfree(acpi_dev->pnp.cid_list);
> +	if (acpi_dev->flags.hardware_id)
> +		kfree(acpi_dev->pnp.hardware_id);
> +	if (acpi_dev->flags.unique_id)
> +		kfree(acpi_dev->pnp.unique_id);
>  	kfree(acpi_dev);
>  }
>  
> @@ -1144,8 +1148,9 @@ static void acpi_device_set_id(struct ac
>  			strcpy(device->pnp.hardware_id, hid);
>  			device->flags.hardware_id = 1;
>  		}
> -	} else
> -		device->pnp.hardware_id = NULL;
> +	}
> +	if (!device->flags.hardware_id)
> +		device->pnp.hardware_id = "";
>  
>  	if (uid) {
>  		device->pnp.unique_id = ACPI_ALLOCATE_ZEROED(strlen (uid) + 1);
> @@ -1153,8 +1158,9 @@ static void acpi_device_set_id(struct ac
>  			strcpy(device->pnp.unique_id, uid);
>  			device->flags.unique_id = 1;
>  		}
> -	} else
> -		device->pnp.unique_id = NULL;
> +	}
> +	if (!device->flags.unique_id)
> +		device->pnp.unique_id = "";
>  
>  	if (cid_list || cid_add) {
>  		struct acpica_device_id_list *list;
> @@ -1369,10 +1375,8 @@ acpi_add_single_object(struct acpi_devic
>  end:
>  	if (!result)
>  		*child = device;
> -	else {
> -		kfree(device->pnp.cid_list);
> -		kfree(device);
> -	}
> +	else
> +		acpi_device_release(&device->dev);
>  
>  	return result;
>  }
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [patch 9/9] acpi: fix NULL bug for HID/UID string

[Hugh Dickins]

On Thu, 13 Aug 2009, Bjorn Helgaas wrote:
> On Thursday 06 August 2009 05:18:12 pm Hugh Dickins wrote:
> > It's up to Len to choose, but I thought we preferred...
> > 
> > From: Hugh Dickins <hugh.dickins@tiscali.co.uk>
> > 
> > acpi_device->pnp.hardware_id and unique_id are now allocated pointers,
> > replacing the previous arrays.  acpi_device_install_notify_handler()
> > oopsed on the NULL hid when probing the video device, 
> 
> I'm sorry I wasn't aware of the acpi_device_install_notify_handler()
> issue until just now, since I added the code that oopses (46ec8598fd).

Sorry for not Cc'ing you: but you can hardly be blamed for
not foreseeing future changes which might affect your code.

> 
> I recently posted patches that removed the dependency on the HID
> in this path:
> 
>   http://marc.info/?l=linux-acpi&m=124907623701270&w=2
> 
> I thought Len said they had been applied to acpi-test, but I don't
> see them there.
> 
> But I don't object to Hugh's patch, since it might be useful for other
> paths.

My original patch, when I hit the bug, was just to
acpi_device_install_notify_handler(): I have never seen a
problem anywhere else, and your patch would deal with that.

But Lin Ming believes there might be issues elsewhere (I have no idea),
posting a more general patch; then my patch here was a retort to that.

What I'm saying is: so far as I know, your patch is more appropriate
than mine; but probably Lin Ming knows better, and mine needed too.

Hugh

Re: [patch 9/9] acpi: fix NULL bug for HID/UID string

[Lin Ming]

On Fri, 2009-08-14 at 00:46 +0800, Hugh Dickins wrote:
> On Thu, 13 Aug 2009, Bjorn Helgaas wrote:
> > On Thursday 06 August 2009 05:18:12 pm Hugh Dickins wrote:
> > > It's up to Len to choose, but I thought we preferred...
> > > 
> > > From: Hugh Dickins <hugh.dickins@tiscali.co.uk>
> > > 
> > > acpi_device->pnp.hardware_id and unique_id are now allocated pointers,
> > > replacing the previous arrays.  acpi_device_install_notify_handler()
> > > oopsed on the NULL hid when probing the video device, 
> > 
> > I'm sorry I wasn't aware of the acpi_device_install_notify_handler()
> > issue until just now, since I added the code that oopses (46ec8598fd).
> 
> Sorry for not Cc'ing you: but you can hardly be blamed for
> not foreseeing future changes which might affect your code.
> 
> > 
> > I recently posted patches that removed the dependency on the HID
> > in this path:
> > 
> >   http://marc.info/?l=linux-acpi&m=124907623701270&w=2
> > 
> > I thought Len said they had been applied to acpi-test, but I don't
> > see them there.
> > 
> > But I don't object to Hugh's patch, since it might be useful for other
> > paths.
> 
> My original patch, when I hit the bug, was just to
> acpi_device_install_notify_handler(): I have never seen a
> problem anywhere else, and your patch would deal with that.
> 
> But Lin Ming believes there might be issues elsewhere (I have no idea),
> posting a more general patch; then my patch here was a retort to that.
> 
> What I'm saying is: so far as I know, your patch is more appropriate
> than mine; but probably Lin Ming knows better, and mine needed too.

As below, there are many calls of acpi_device_hid without any check of
NULL hid.
So yes, we need a general patch, as yours.

0 drivers/acpi/button.c                 acpi_button_add                     308 hid = acpi_device_hid(device);
1 drivers/acpi/processor_core.c         acpi_processor_get_info             599 if (!strcmp(acpi_device_hid(device), ACPI_PROCESSOR_OBJECT_HID)) {
2 drivers/acpi/scan.c                   acpi_device_install_notify_handler  378 hid = acpi_device_hid(device);
3 drivers/acpi/scan.c                   acpi_device_remove_notify_handler   402 if (!strcmp(acpi_device_hid(device), ACPI_BUTTON_HID_POWERF))
4 drivers/acpi/scan.c                   acpi_device_remove_notify_handler   405 else if (!strcmp(acpi_device_hid(device), ACPI_BUTTON_HID_SLEEPF))
5 drivers/acpi/video.c                  acpi_video_bus_add                 2248 "%s/video/input0", acpi_device_hid(video->device));
6 drivers/gpu/drm/i915/intel_lvds.c     check_lid_device                    822 if (!strncmp(acpi_device_hid(acpi_dev), "PNP0C0D", 7))
7 drivers/platform/x86/fujitsu-laptop.c acpi_fujitsu_add                    681 "%s/video/input0", acpi_device_hid(device));
8 drivers/platform/x86/fujitsu-laptop.c acpi_fujitsu_hotkey_add             852 "%s/video/input0", acpi_device_hid(device));
9 drivers/pnp/pnpacpi/core.c            pnpacpi_add_device                  162 if (ACPI_FAILURE(status) || !ispnpidacpi(acpi_device_hid(device)) ||
a drivers/pnp/pnpacpi/core.c            pnpacpi_add_device                  166 dev = pnp_alloc_dev(&pnpacpi_protocol, num, acpi_device_hid(device));

Lin Ming

> 
> Hugh

Re: [patch 9/9] acpi: fix NULL bug for HID/UID string

[Bjorn Helgaas]

On Thursday 13 August 2009 07:28:56 pm Lin Ming wrote:
> On Fri, 2009-08-14 at 00:46 +0800, Hugh Dickins wrote:
> > My original patch, when I hit the bug, was just to
> > acpi_device_install_notify_handler(): I have never seen a
> > problem anywhere else, and your patch would deal with that.
> > 
> > But Lin Ming believes there might be issues elsewhere (I have no idea),
> > posting a more general patch; then my patch here was a retort to that.
> > 
> > What I'm saying is: so far as I know, your patch is more appropriate
> > than mine; but probably Lin Ming knows better, and mine needed too.
> 
> As below, there are many calls of acpi_device_hid without any check of
> NULL hid.
> So yes, we need a general patch, as yours.

I'm a little concerned because there's a deeper issue here, and I
want to make sure we have a clean solution and not a temporary band-aid.

In general, ACPI drivers bind to an acpi_device based on the HID.
There are a few exceptions where drivers walk the namespace themselves
and look for things other than the HID, but IMHO, this is a problem
because those drivers won't work for devices added or removed after
the driver loads.

I think we'd be better off if we made a rule that "every acpi_device
has a non-empty HID."  That way, we could get drivers out of the
business of walking the namespace, which would fix some hotplug issues.
We already synthesize a number of Linux-specific HIDs (LNXPOWER,
LNXCPU, LNXVIDEO, etc), and I think the Linux/ACPI core should
probably do this whenever we build an acpi_device for an object
with no _HID method.

I don't quite understand how this oops happens, though.  It seems that
we crashed in this path:

  acpi_device_probe
    acpi_bus_driver_init
      driver->ops.add  (calls acpi_video_bus_add)
    acpi_device_install_notify_handler
      hid = acpi_device_hid(device)
      strcmp(hid, ACPI_BUTTON_HID_POWERF))
        *** OOPS, hid == NULL ***

But the acpi_video_bus driver claims devices using ACPI_VIDEO_HID
("LNXVIDEO"), and acpi_device_set_id() already does synthesize
that HID, so acpi_device_hid() should have been valid.  So why
did we oops?

Bjorn

> 0 drivers/acpi/button.c                 acpi_button_add                     308 hid = acpi_device_hid(device);
> 1 drivers/acpi/processor_core.c         acpi_processor_get_info             599 if (!strcmp(acpi_device_hid(device), ACPI_PROCESSOR_OBJECT_HID)) {
> 2 drivers/acpi/scan.c                   acpi_device_install_notify_handler  378 hid = acpi_device_hid(device);
> 3 drivers/acpi/scan.c                   acpi_device_remove_notify_handler   402 if (!strcmp(acpi_device_hid(device), ACPI_BUTTON_HID_POWERF))
> 4 drivers/acpi/scan.c                   acpi_device_remove_notify_handler   405 else if (!strcmp(acpi_device_hid(device), ACPI_BUTTON_HID_SLEEPF))
> 5 drivers/acpi/video.c                  acpi_video_bus_add                 2248 "%s/video/input0", acpi_device_hid(video->device));
> 6 drivers/gpu/drm/i915/intel_lvds.c     check_lid_device                    822 if (!strncmp(acpi_device_hid(acpi_dev), "PNP0C0D", 7))
> 7 drivers/platform/x86/fujitsu-laptop.c acpi_fujitsu_add                    681 "%s/video/input0", acpi_device_hid(device));
> 8 drivers/platform/x86/fujitsu-laptop.c acpi_fujitsu_hotkey_add             852 "%s/video/input0", acpi_device_hid(device));
> 9 drivers/pnp/pnpacpi/core.c            pnpacpi_add_device                  162 if (ACPI_FAILURE(status) || !ispnpidacpi(acpi_device_hid(device)) ||
> a drivers/pnp/pnpacpi/core.c            pnpacpi_add_device                  166 dev = pnp_alloc_dev(&pnpacpi_protocol, num, acpi_device_hid(device));

Re: [patch 9/9] acpi: fix NULL bug for HID/UID string

[Hugh Dickins]

On Fri, 14 Aug 2009, Bjorn Helgaas wrote:
> On Thursday 13 August 2009 07:28:56 pm Lin Ming wrote:
> > On Fri, 2009-08-14 at 00:46 +0800, Hugh Dickins wrote:
> > > My original patch, when I hit the bug, was just to
> > > acpi_device_install_notify_handler(): I have never seen a
> > > problem anywhere else, and your patch would deal with that.
> > > 
> > > But Lin Ming believes there might be issues elsewhere (I have no idea),
> > > posting a more general patch; then my patch here was a retort to that.
> > > 
> > > What I'm saying is: so far as I know, your patch is more appropriate
> > > than mine; but probably Lin Ming knows better, and mine needed too.
> > 
> > As below, there are many calls of acpi_device_hid without any check of
> > NULL hid.
> > So yes, we need a general patch, as yours.
> 
> I'm a little concerned because there's a deeper issue here, and I
> want to make sure we have a clean solution and not a temporary band-aid.

Yes, without appreciating any of the details, I believe you're right
to have that concern.

And, maybe irrelevant, but I'd noticed a line in acpi_device_register:
    if(!strcmp(acpi_device_bus_id->bus_id,
	device->flags.hardware_id? device->pnp.hardware_id : "device"))
which made me wonder if the band-aid should say "device" rather than "".

> 
> In general, ACPI drivers bind to an acpi_device based on the HID.
> There are a few exceptions where drivers walk the namespace themselves
> and look for things other than the HID, but IMHO, this is a problem
> because those drivers won't work for devices added or removed after
> the driver loads.
> 
> I think we'd be better off if we made a rule that "every acpi_device
> has a non-empty HID."  That way, we could get drivers out of the
> business of walking the namespace, which would fix some hotplug issues.
> We already synthesize a number of Linux-specific HIDs (LNXPOWER,
> LNXCPU, LNXVIDEO, etc), and I think the Linux/ACPI core should
> probably do this whenever we build an acpi_device for an object
> with no _HID method.

I don't know my way around here, I cannot comment.

> 
> I don't quite understand how this oops happens, though.  It seems that
> we crashed in this path:
> 
>   acpi_device_probe
>     acpi_bus_driver_init
>       driver->ops.add  (calls acpi_video_bus_add)
>     acpi_device_install_notify_handler
>       hid = acpi_device_hid(device)
>       strcmp(hid, ACPI_BUTTON_HID_POWERF))
>         *** OOPS, hid == NULL ***
> 
> But the acpi_video_bus driver claims devices using ACPI_VIDEO_HID
> ("LNXVIDEO"), and acpi_device_set_id() already does synthesize
> that HID, so acpi_device_hid() should have been valid.  So why
> did we oops?

The backtrace I jotted down, from Intel 915 graphics, looked like this
(acpi_device_install_notify_handler is inlined into acpi_device_probe):

strcmp
acpi_device_probe
really_probe
driver_probe_device
__driver_attach
bus_for_each_dev
driver_attach
bus_add_driver
driver_register
acpi_bus_register
acpi_video_register
intel_opregion_init
i915_driver_load
drm_get_dev
drm_init
i915_init
do_one_initcall
do_basic_setup
kernel_init
kernel_thread_helper

Valdis reported a similar but shorter hand-copied backtrace from
nVidia Corporation G72M [Quadro NVS 110M/GeForce Go 7300] (rev a1):

strcmp+0x4/0x1f
acpi_device+probe+0xac/0x13e
driver_probe_device+0xc9/0x14e
__driver_attach+0x58/0x7c
? __driver_attach+0x58/0x7c
? __driver_attach+0x58/0x7c
bus_for_each_dev+0x54/0x89
driver_attach+0x19/0x1b
bus_add_driver+0xv4/0x1fe
driver_register+0xb7/0x128
? acpi_video_init+0x0/0x17
acpi_bus_register_driver+0x3e/0x42
acpi_video_register+0x42/0x6e
acpi_video_init+0x15/0x17
do_one_initcall+0x56/0x130

Hugh

Re: [patch 9/9] acpi: fix NULL bug for HID/UID string

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 753 bytes --]

On Fri, 14 Aug 2009 10:44:50 MDT, Bjorn Helgaas said:

> I don't quite understand how this oops happens, though.  It seems that
> we crashed in this path:
> 
>   acpi_device_probe
>     acpi_bus_driver_init
>       driver->ops.add  (calls acpi_video_bus_add)
>     acpi_device_install_notify_handler
>       hid = acpi_device_hid(device)
>       strcmp(hid, ACPI_BUTTON_HID_POWERF))
>         *** OOPS, hid == NULL ***
> 
> But the acpi_video_bus driver claims devices using ACPI_VIDEO_HID
> ("LNXVIDEO"), and acpi_device_set_id() already does synthesize
> that HID, so acpi_device_hid() should have been valid.  So why
> did we oops?

When I tripped over it, acpi_device_set_id() *wasn't* synthesizing an HID,
it was explicitly setting a NULL pointer.

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: [patch 9/9] acpi: fix NULL bug for HID/UID string

[Bjorn Helgaas]

On Friday 14 August 2009 12:18:47 pm Hugh Dickins wrote:
> On Fri, 14 Aug 2009, Bjorn Helgaas wrote:
> > I don't quite understand how this oops happens, though.  It seems that
> > we crashed in this path:
> > 
> >   acpi_device_probe
> >     acpi_bus_driver_init
> >       driver->ops.add  (calls acpi_video_bus_add)
> >     acpi_device_install_notify_handler
> >       hid = acpi_device_hid(device)
> >       strcmp(hid, ACPI_BUTTON_HID_POWERF))
> >         *** OOPS, hid == NULL ***
> > 
> > But the acpi_video_bus driver claims devices using ACPI_VIDEO_HID
> > ("LNXVIDEO"), and acpi_device_set_id() already does synthesize
> > that HID, so acpi_device_hid() should have been valid.  So why
> > did we oops?
> 
> The backtrace I jotted down, from Intel 915 graphics, looked like this
> (acpi_device_install_notify_handler is inlined into acpi_device_probe):
> 
> strcmp
> acpi_device_probe
> really_probe
> driver_probe_device
> __driver_attach
> bus_for_each_dev
> driver_attach
> bus_add_driver
> driver_register
> acpi_bus_register
> acpi_video_register
> intel_opregion_init
> i915_driver_load
> drm_get_dev
> drm_init
> i915_init
> do_one_initcall
> do_basic_setup
> kernel_init
> kernel_thread_helper

Likely the same issue as Valdis saw, but I don't see the actual
problem yet.  If it's convenient, the patch below might shed a
little light.

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 781435d..e8de598 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -376,6 +376,12 @@ static int acpi_device_install_notify_handler(struct acpi_device *device)
 	char *hid;
 
 	hid = acpi_device_hid(device);
+	if (!hid || strlen(hid) == 0) {
+		printk(KERN_INFO PREFIX
+		       "install notify handler for %s, no HID!\n",
+		       dev_name(&device->dev));
+		hid = "";
+	}
 	if (!strcmp(hid, ACPI_BUTTON_HID_POWERF))
 		status =
 		    acpi_install_fixed_event_handler(ACPI_EVENT_POWER_BUTTON,
@@ -420,6 +426,8 @@ static int acpi_device_probe(struct device * dev)
 
 	ret = acpi_bus_driver_init(acpi_dev, acpi_drv);
 	if (!ret) {
+		printk(KERN_INFO PREFIX "binding driver [%s] to device [%s]\n",
+			acpi_drv->name, acpi_dev->pnp.bus_id);
 		if (acpi_dev->bus_ops.acpi_op_start)
 			acpi_start_single_object(acpi_dev);
 
@@ -1182,6 +1190,7 @@ acpi_add_single_object(struct acpi_device **child,
 {
 	int result = 0;
 	struct acpi_device *device = NULL;
+	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
 
 
 	if (!child)
@@ -1315,9 +1324,14 @@ acpi_add_single_object(struct acpi_device **child,
 	}
 
 end:
-	if (!result)
+	if (!result) {
+		acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer);
+		printk(KERN_INFO PREFIX "adding %s [%s] HID %s\n",
+		       dev_name(&device->dev), (char *) buffer.pointer,
+		       acpi_device_hid(device) ? acpi_device_hid(device) :
+						 "(null)");
 		*child = device;
-	else {
+	} else {
 		kfree(device->pnp.cid_list);
 		kfree(device);
 	}

Re: [patch 9/9] acpi: fix NULL bug for HID/UID string

[Bjorn Helgaas]

On Friday 14 August 2009 01:53:40 pm Valdis.Kletnieks@vt.edu wrote:
> On Fri, 14 Aug 2009 10:44:50 MDT, Bjorn Helgaas said:
> 
> > I don't quite understand how this oops happens, though.  It seems that
> > we crashed in this path:
> > 
> >   acpi_device_probe
> >     acpi_bus_driver_init
> >       driver->ops.add  (calls acpi_video_bus_add)
> >     acpi_device_install_notify_handler
> >       hid = acpi_device_hid(device)
> >       strcmp(hid, ACPI_BUTTON_HID_POWERF))
> >         *** OOPS, hid == NULL ***
> > 
> > But the acpi_video_bus driver claims devices using ACPI_VIDEO_HID
> > ("LNXVIDEO"), and acpi_device_set_id() already does synthesize
> > that HID, so acpi_device_hid() should have been valid.  So why
> > did we oops?
> 
> When I tripped over it, acpi_device_set_id() *wasn't* synthesizing an HID,
> it was explicitly setting a NULL pointer.

What I don't understand is why the acpi_video_bus driver could get
bound to the device if the device didn't have a HID.

Bjorn

Re: [patch 9/9] acpi: fix NULL bug for HID/UID string

[Hugh Dickins]

On Fri, 14 Aug 2009, Bjorn Helgaas wrote:
> 
> Likely the same issue as Valdis saw, but I don't see the actual
> problem yet.  If it's convenient, the patch below might shed a
> little light.

Here's an extract from dmesg with that patch applied on top:

Linux agpgart interface v0.103
agpgart-intel 0000:00:00.0: Intel 965GM Chipset
agpgart-intel 0000:00:00.0: detected 7676K stolen memory
agpgart-intel 0000:00:00.0: AGP aperture is 256M @ 0xd0000000
[drm] Initialized drm 1.1.0 20060810
pci 0000:00:02.0: PCI INT A -> GSI 16 (level, low) -> IRQ 16
pci 0000:00:02.0: setting latency timer to 64
pci 0000:00:02.0: irq 28 for MSI/MSI-X
acpi device:05: registered as cooling_device2
input: Video Bus as /devices/LNXSYSTM:00/device:00/PNP0A08:00/device:02/input/input3
ACPI: Video Device [GFX0] (multi-head: yes  rom: no  post: no)
ACPI: binding driver [video] to device [GFX0]
ACPI: install notify handler for device:02, no HID!
[drm] Initialized i915 1.6.0 20080730 for 0000:00:02.0 on minor 0

Hugh

Re: [patch 9/9] acpi: fix NULL bug for HID/UID string

[Bartlomiej Zolnierkiewicz]

On Friday 14 August 2009 22:10:53 Bjorn Helgaas wrote:
> On Friday 14 August 2009 01:53:40 pm Valdis.Kletnieks@vt.edu wrote:
> > On Fri, 14 Aug 2009 10:44:50 MDT, Bjorn Helgaas said:
> > 
> > > I don't quite understand how this oops happens, though.  It seems that
> > > we crashed in this path:
> > > 
> > >   acpi_device_probe
> > >     acpi_bus_driver_init
> > >       driver->ops.add  (calls acpi_video_bus_add)
> > >     acpi_device_install_notify_handler
> > >       hid = acpi_device_hid(device)
> > >       strcmp(hid, ACPI_BUTTON_HID_POWERF))
> > >         *** OOPS, hid == NULL ***
> > > 
> > > But the acpi_video_bus driver claims devices using ACPI_VIDEO_HID
> > > ("LNXVIDEO"), and acpi_device_set_id() already does synthesize
> > > that HID, so acpi_device_hid() should have been valid.  So why
> > > did we oops?
> > 
> > When I tripped over it, acpi_device_set_id() *wasn't* synthesizing an HID,
> > it was explicitly setting a NULL pointer.
> 
> What I don't understand is why the acpi_video_bus driver could get
> bound to the device if the device didn't have a HID.

This was also puzzling me while I hit the original issue so I did some more
research at that time and it turned out that binding is not controlled by
HID but by ->ops.add method of ACPI driver.

In the case of ACPI video driver it goes like that (for 2.6.31-rc6):

acpi_device_probe()
  acpi_bus_driver_init()
    driver->ops.add() [ acpi_video_bus_add() ]
      acpi_video_bus_check()

and in acpi_video_bus_check() we have:

...
	/* Since there is no HID, CID and so on for VGA driver, we have
	 * to check well known required nodes.
	 */

	/* Does this device support video switching? */
	if (video->cap._DOS) {
		video->flags.multihead = 1;
		status = 0;
	}

	/* Does this device support retrieving a video ROM? */
	if (video->cap._ROM) {
		video->flags.rom = 1;
		status = 0;
	}

	/* Does this device support configuring which video device to POST? */
	if (video->cap._GPD && video->cap._SPD && video->cap._VPO) {
		video->flags.post = 1;
		status = 0;
	}

	return status;
...

IOW HID is not required at all for ACPI device drivers at the moment which
becomes a problem after "ACPICA: Major update for acpi_get_object_info
external interface" commit since acpi_device_hid() will no longer return
a pointer to an empty static buffer but a NULL instead..

I think that ideally we should always have a valid HID for ACPI devices
(synthesized in case of ACPI video and similar devices) in the long-term
but could we get Hugh's patch applied for now, please?

While your patch also looks valid and most likely fixes ACPI video oops
Hugh's patch is still needed for fully fixing the problem at the moment
(i.e. ACPI button driver seems to need it).

This problem has been in ACPI tree (and thus in -next) for way too long
already (three weeks with the known fix) and the current situation is not
moving things forward (we have just people rediscovering the issue the
hard way)..

Thanks,
Bart

Re: [patch 9/9] acpi: fix NULL bug for HID/UID string

[Bjorn Helgaas]

On Saturday 15 August 2009 08:14:22 am Bartlomiej Zolnierkiewicz wrote:
> On Friday 14 August 2009 22:10:53 Bjorn Helgaas wrote:
> > On Friday 14 August 2009 01:53:40 pm Valdis.Kletnieks@vt.edu wrote:
> > > On Fri, 14 Aug 2009 10:44:50 MDT, Bjorn Helgaas said:
> > > 
> > > > I don't quite understand how this oops happens, though.  It seems that
> > > > we crashed in this path:
> > > > 
> > > >   acpi_device_probe
> > > >     acpi_bus_driver_init
> > > >       driver->ops.add  (calls acpi_video_bus_add)
> > > >     acpi_device_install_notify_handler
> > > >       hid = acpi_device_hid(device)
> > > >       strcmp(hid, ACPI_BUTTON_HID_POWERF))
> > > >         *** OOPS, hid == NULL ***
> > > > 
> > > > But the acpi_video_bus driver claims devices using ACPI_VIDEO_HID
> > > > ("LNXVIDEO"), and acpi_device_set_id() already does synthesize
> > > > that HID, so acpi_device_hid() should have been valid.  So why
> > > > did we oops?
> > > 
> > > When I tripped over it, acpi_device_set_id() *wasn't* synthesizing an HID,
> > > it was explicitly setting a NULL pointer.
> > 
> > What I don't understand is why the acpi_video_bus driver could get
> > bound to the device if the device didn't have a HID.
> 
> This was also puzzling me while I hit the original issue so I did some more
> research at that time and it turned out that binding is not controlled by
> HID but by ->ops.add method of ACPI driver.
> 
> In the case of ACPI video driver it goes like that (for 2.6.31-rc6):
> 
> acpi_device_probe()
>   acpi_bus_driver_init()
>     driver->ops.add() [ acpi_video_bus_add() ]
>       acpi_video_bus_check()
> 
> and in acpi_video_bus_check() we have:
> 
> ...
> 	/* Since there is no HID, CID and so on for VGA driver, we have
> 	 * to check well known required nodes.
> 	 */
> 
> 	/* Does this device support video switching? */
> 	if (video->cap._DOS) {
> 		video->flags.multihead = 1;
> 		status = 0;
> 	}
> 
> 	/* Does this device support retrieving a video ROM? */
> 	if (video->cap._ROM) {
> 		video->flags.rom = 1;
> 		status = 0;
> 	}
> 
> 	/* Does this device support configuring which video device to POST? */
> 	if (video->cap._GPD && video->cap._SPD && video->cap._VPO) {
> 		video->flags.post = 1;
> 		status = 0;
> 	}
> 
> 	return status;
> ...
> 
> IOW HID is not required at all for ACPI device drivers at the moment which
> becomes a problem after "ACPICA: Major update for acpi_get_object_info
> external interface" commit since acpi_device_hid() will no longer return
> a pointer to an empty static buffer but a NULL instead..
> 
> I think that ideally we should always have a valid HID for ACPI devices
> (synthesized in case of ACPI video and similar devices) in the long-term
> but could we get Hugh's patch applied for now, please?
> 
> While your patch also looks valid and most likely fixes ACPI video oops
> Hugh's patch is still needed for fully fixing the problem at the moment
> (i.e. ACPI button driver seems to need it).

Oh, I think I see what happened.  acpi_device_set_id() synthesizes
ACPI_VIDEO_HID, but puts it on the CID list rather than making it
the HID, so we can end up with a device with a CID but no HID.

As far as I can see, there's no value in maintaining the HID
separately from the CID list.  Keeping a single list, with the
HID (if present) being the first element, would simplify the
code in a few places and avoid issues like this.

I think it's a good idea to put in Hugh's patch for now, and I'll
work on a patch to unify HID/CID and guarantee that every acpi_device
has a valid ID.

Bjorn

Re: [patch 9/9] acpi: fix NULL bug for HID/UID string

[Len Brown]

http://patchwork.kernel.org/patch/39735/

applied to acpica branch in acpi-test tree

thanks,
Len Brown, Intel Open Source Technology Center