acpi: silence kmemcheck false positive

PATCH Superseded Dan Carpenter 3 participants 7 messages Apr 22, 2010 – Apr 27, 2010 ACPI

lore

Share & tools

lore

Dan Carpenter

author

2010-04-22 19:44 UTC #0 lore raw

PATCH 1/1 acpi: silence kmemcheck false positive

This addresses: https://bugzilla.kernel.org/show_bug.cgi?id=14998

We copy some strings into "event" and kmemcheck complains that the bytes after the NULL terminators are uninitialized. That's true but it's harmless. The "event" struct is used in acpi_system_read_event() and we don't read past the terminator.

This patch just silences the warning.

Reported-by Christian Casteyde <[email protected]>

Signed-off-by Dan Carpenter <[email protected]>

Tested-by Christian Casteyde <[email protected]>

drivers/acpi/bus.c +1 -1

index 37132dc..4ef7c97 100644
@@ -527,7 +527,7 @@ int acpi_bus_generate_proc_event4(const char *device_class, const char *bus_id,
 	if (!event_is_open)
 		return 0;
 
-	event = kmalloc(sizeof(struct acpi_bus_event), GFP_ATOMIC);
+	event = kmalloc(sizeof(struct acpi_bus_event), GFP_ATOMIC | __GFP_NOTRACK_FALSE_POSITIVE);
 	if (!event)
 		return -ENOMEM;

Dan Carpenter

author

2010-04-22 20:02 UTC #1 lore raw

Oops. I forgot to add Christian to the CC list. Added.

regards,
dan carpenter

Quoted 26 lines · Dan Carpenter

This addresses: https://bugzilla.kernel.org/show_bug.cgi?id=14998
We copy some strings into "event" and kmemcheck complains that the bytes after the NULL terminators are uninitialized. That's true but it's harmless. The "event" struct is used in acpi_system_read_event() and we don't read past the terminator.
This patch just silences the warning.
Reported-by Christian Casteyde <[email protected]>
Signed-off-by Dan Carpenter <[email protected]>
Tested-by Christian Casteyde <[email protected]>
drivers/acpi/bus.c +1 -1
index 37132dc..4ef7c97 100644
@@ -527,7 +527,7 @@ int acpi_bus_generate_proc_event4(const char *device_class, const char *bus_id,
 	if (!event_is_open)
 		return 0;
 
-	event = kmalloc(sizeof(struct acpi_bus_event), GFP_ATOMIC);
+	event = kmalloc(sizeof(struct acpi_bus_event), GFP_ATOMIC | __GFP_NOTRACK_FALSE_POSITIVE);
 	if (!event)
 		return -ENOMEM;
 

Bjorn Helgaas

2010-04-22 22:32 UTC #2 lore raw

Quoted 22 lines · Dan Carpenter

Quoted 21 lines · Dan Carpenter
This addresses: https://bugzilla.kernel.org/show_bug.cgi?id=14998
We copy some strings into "event" and kmemcheck complains that the bytes after the NULL terminators are uninitialized. That's true but it's harmless. The "event" struct is used in acpi_system_read_event() and we don't read past the terminator.
This patch just silences the warning.
Reported-by Christian Casteyde <[email protected]>
Signed-off-by Dan Carpenter <[email protected]>
Tested-by Christian Casteyde <[email protected]>
drivers/acpi/bus.c +0 -0
index 37132dc..4ef7c97 100644
@@ -527,7 +527,7 @@ int acpi_bus_generate_proc_event4(const char *device_class, const char *bus_id,
 	if (!event_is_open)
 		return 0;
 

Quoted 2 lines

drivers/acpi/bus.c int acpi_bus_generate_proc_event4(const char *device_class, const char *bus_id, · line 530
Quoted 2 lines
drivers/acpi/bus.c +1 -1
-	event = kmalloc(sizeof(struct acpi_bus_event), GFP_ATOMIC);
+	event = kmalloc(sizeof(struct acpi_bus_event), GFP_ATOMIC | __GFP_NOTRACK_FALSE_POSITIVE);

Just in terms of reading the code, this solution is fairly ugly. I think __GFP_NOTRACK should be sort of the last resort, after we've ruled out all the more conventional strategies.

Has anybody tried any of the alternatives Vegard suggested here:
https://bugzilla.kernel.org/show_bug.cgi?id=14998#c35 ?

Bjorn

Dan Carpenter

author

2010-04-23 19:17 UTC #3 lore raw

Quoted 9 lines · Bjorn Helgaas

Quoted 2 lines
drivers/acpi/bus.c int acpi_bus_generate_proc_event4(const char *device_class, const char *bus_id, · line 530
Quoted 2 lines
drivers/acpi/bus.c +1 -1
-	event = kmalloc(sizeof(struct acpi_bus_event), GFP_ATOMIC);
+	event = kmalloc(sizeof(struct acpi_bus_event), GFP_ATOMIC | __GFP_NOTRACK_FALSE_POSITIVE);
Just in terms of reading the code, this solution is fairly ugly. I think __GFP_NOTRACK should be sort of the last resort, after we've ruled out all the more conventional strategies.
Has anybody tried any of the alternatives Vegard suggested here:
https://bugzilla.kernel.org/show_bug.cgi?id=14998#c35 ?

Another potential solution would be to change the kmalloc() to a kzalloc().

This isn't really my code, I was just randomly poking through bugzilla. Tell me which approach you prefer and I'll do it.

regards,
dan carpenter

Dan Carpenter

author

2010-04-26 22:19 UTC #4 lore raw

In reply to Bjorn Helgaas

Quoted 10 lines · Bjorn Helgaas

Quoted 2 lines
drivers/acpi/bus.c int acpi_bus_generate_proc_event4(const char *device_class, const char *bus_id, · line 530
Quoted 2 lines
drivers/acpi/bus.c +1 -1
-	event = kmalloc(sizeof(struct acpi_bus_event), GFP_ATOMIC);
+	event = kmalloc(sizeof(struct acpi_bus_event), GFP_ATOMIC | __GFP_NOTRACK_FALSE_POSITIVE);
Just in terms of reading the code, this solution is fairly ugly. I think __GFP_NOTRACK should be sort of the last resort, after we've ruled out all the more conventional strategies.
Has anybody tried any of the alternatives Vegard suggested here:
https://bugzilla.kernel.org/show_bug.cgi?id=14998#c35 ?

First of all I really want to thank Vegard for taking the time to look at this. There are three options in the bugzilla entry:

Quoted 2 lines

1. Copy the struct "by hand" and use strncpy() for the strings to avoid the uninitialized areas.

The downside is that copying by hand is a maintainance hassle.

Quoted 2 lines

2. Allocate the event with kmalloc() instead of on the stack and free it after the call to acpi_bus_receive_event().

The problem with this is that if a future function uses stack data instead of allocated data we're back at square one. Perhaps if we put a huge warning sign it would be OK. There isn't a sparse anotation for this, but it would be nice to have. One other place where we could use it would be passing stack space as a DMA transfer buffer.

Quoted 2 lines

3. Allocate the event with kmalloc() and pass ownership of the structure to acpi_bus_receive_event() to avoid the copying altogether.

I don't think the kmalloc() is meant here because it says we avoid the copying. This is a simple api change but in the end I didn't think it was as nice as the current api. I did write a patch to do it though so if anyone prefers that I can send it.

In the end, I decided it was easiest to just to change the kmalloc() to a kzalloc().

regards,
dan carpenter

Dan Carpenter

author

2010-04-26 22:23 UTC #5 lore raw

PATCH v2 1/1 acpi: silence kmemcheck false positive

In reply to Bjorn Helgaas

This addresses: https://bugzilla.kernel.org/show_bug.cgi?id=14998

We copy some strings into "event" but we leave the space after the NULL terminators uninitialized. Later in acpi_bus_receive_event() we copy the whole struct to another buffer with memcpy(). If the new buffer is stored on the stack, kmemcheck prints a warning about the unitialized space after the NULL terminators.

It's true that the space is uninitialized, but it's harmless. The buffer is only used in acpi_system_read_event() and we don't read past the NULL terminators.

This patch changes the kmalloc() to kzalloc() so that we initialize the memory and silence the kmemcheck warning.

Reported-by Christian Casteyde <[email protected]>

Signed-off-by Dan Carpenter <[email protected]>

---
v2: In the first version I used __GFP_NOTRACK_FALSE_POSITIVE to silence the warning, but that was ugly so this version initializes the memory with kzalloc()

drivers/acpi/bus.c +1 -1

index 37132dc..743576b 100644
@@ -527,7 +527,7 @@ int acpi_bus_generate_proc_event4(const char *device_class, const char *bus_id,
 	if (!event_is_open)
 		return 0;
 
-	event = kmalloc(sizeof(struct acpi_bus_event), GFP_ATOMIC);
+	event = kzalloc(sizeof(struct acpi_bus_event), GFP_ATOMIC);
 	if (!event)
 		return -ENOMEM;

Len Brown

reviewer Applied

2010-04-27 07:16 UTC #6 lore raw

applied

thanks,
Len Brown, Intel Open Source Technology Center