[patch] acpi: silence kmemcheck false positive

[patch] acpi: silence kmemcheck false positive

[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 <casteyde.christian@free.fr>
Signed-off-by: Dan Carpenter <error27@gmail.com>
Tested-by: Christian Casteyde <casteyde.christian@free.fr>

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 37132dc..4ef7c97 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -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;
 

Re: [patch] acpi: silence kmemcheck false positive

[Dan Carpenter]

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

regards,
dan carpenter

On Thu, Apr 22, 2010 at 09:43:46PM +0200, Dan Carpenter wrote:
> 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 <casteyde.christian@free.fr>
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> Tested-by: Christian Casteyde <casteyde.christian@free.fr>
> 
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index 37132dc..4ef7c97 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -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;
>  

Re: [patch] acpi: silence kmemcheck false positive

[Bjorn Helgaas]

On Thursday 22 April 2010 02:02:20 pm Dan Carpenter wrote:
> On Thu, Apr 22, 2010 at 09:43:46PM +0200, Dan Carpenter wrote:
> > 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 <casteyde.christian@free.fr>
> > Signed-off-by: Dan Carpenter <error27@gmail.com>
> > Tested-by: Christian Casteyde <casteyde.christian@free.fr>
> > 
> > diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> > index 37132dc..4ef7c97 100644
> > --- a/drivers/acpi/bus.c
> > +++ b/drivers/acpi/bus.c
> > @@ -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);

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

Re: [patch] acpi: silence kmemcheck false positive

[Dan Carpenter]

On Thu, Apr 22, 2010 at 04:32:20PM -0600, Bjorn Helgaas wrote:
> > > -	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

Re: [patch] acpi: silence kmemcheck false positive

[Dan Carpenter]

On Thu, Apr 22, 2010 at 04:32:20PM -0600, Bjorn Helgaas wrote:
> > > -	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:

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

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

> 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

[patch v2] acpi: silence kmemcheck false positive

[Dan Carpenter]

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 <casteyde.christian@free.fr>
Signed-off-by: Dan Carpenter <error27@gmail.com>
---
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()

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 37132dc..743576b 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -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;
 

Re: [patch v2] acpi: silence kmemcheck false positive

[Len Brown]

applied

thanks,
Len Brown, Intel Open Source Technology Center