acpi: fix NULL bug for HID/UID string

From: Lin Ming <[email protected]>

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 <[email protected]>

Cc Hugh Dickins <[email protected]>

Cc Len Brown <[email protected]>

Signed-off-by Lin Ming <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

---

 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

drivers/acpi/scan.c renamed from drivers/acpi/scan.c~acpi-fix-null-bug-for-hid-uid-string +4 -0

@@ -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

include/acpi/acpi_bus.h renamed from include/acpi/acpi_bus.h~acpi-fix-null-bug-for-hid-uid-string +2 -2

@@ -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 <[email protected]>

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 <[email protected]>

Signed-off-by Lin Ming <[email protected]>

---

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

drivers/acpi/scan.c +12 -9

@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 7 lines · Hugh Dickins

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

From: Hugh Dickins <[email protected]>

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.

Quoted 11 lines · Bjorn Helgaas

Quoted 7 lines · Hugh Dickins

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

From: Hugh Dickins <[email protected]>

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.

Quoted 11 lines

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

Quoted 7 lines · Hugh Dickins

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.

Quoted 2 lines

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));

Quoted 11 lines · Lin Ming

Quoted 9 lines · Hugh Dickins

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.

Quoted 3 lines

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?

Quoted 11 lines · Bjorn Helgaas

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 ***

Quoted 4 lines

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.

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?

Quoted 11 lines · Bjorn Helgaas

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 ***

Quoted 4 lines

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.

Quoted 18 lines · On Friday 14 August 2009 01:53:40 pm [email protected] wrote:

Quoted 15 lines · Bjorn Helgaas

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?

Quoted 2 lines

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.

Quoted 19 lines · On Friday 14 August 2009 22:10:53 Bjorn Helgaas wrote:

Quoted 18 lines · On Friday 14 August 2009 01:53:40 pm [email protected] wrote:

Quoted 15 lines · Bjorn Helgaas

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?

Quoted 5 lines

Quoted 2 lines

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).