Re: [KJ] Re: [PATCH] bugfix GFP_KERNEL -> GFP_ATOMIC in spin_locked region

[Yoann Padioleau <padator@wanadoo.fr>]

Andrew Morton <akpm@linux-foundation.org> writes:

>> 
>>  net/wan/lmc/lmc_main.c        |    2 +-
>>  scsi/megaraid/megaraid_mm.c   |    2 +-
>>  usb/serial/io_ti.c            |    2 +-
>>  usb/serial/ti_usb_3410_5052.c |    2 +-
>>  usb/serial/whiteheat.c        |    6 +++---
>>  5 files changed, 7 insertions(+), 7 deletions(-)
>
> This patch covers three areas of maintainer responsibility, so poor old me
> has to split it up and redo the changelogs.  Oh well.

Sorry. 

For modifications that crosscut the kernel it is not very practical to
look each time for the maintainer. We plan to send a patch that
replaces some calls to kmalloc/memset with kzalloc; do we have to split
it for each maintainer ?

>> diff --git a/drivers/net/wan/lmc/lmc_main.c b/drivers/net/wan/lmc/lmc_main.c
>> index ae132c1..750b3ef 100644
>> --- a/drivers/net/wan/lmc/lmc_main.c
>> +++ b/drivers/net/wan/lmc/lmc_main.c
>> @@ -483,7 +483,7 @@ #endif /* end ifdef _DBG_EVENTLOG */
>>                              break;
>>                      }
>>  
>> -                    data = kmalloc(xc.len, GFP_KERNEL);
>> +                    data = kmalloc(xc.len, GFP_ATOMIC);
>>                      if(data == 0x0){
>>                              printk(KERN_WARNING "%s: Failed to allocate memory for copy\n", dev->name);
>>                              ret = -ENOMEM;
>
> A few lines later we do:
>
> 	if(copy_from_user(data, xc.data, xc.len))
>
> which also is illegal under spinlock.

I have launched my tool to detect such situations and I get this 
(in a diff-like format).

--- /home/pad/kernels/git/linux-2.6/arch/cris/arch-v10/drivers/gpio.c	2007-03-27 15:12:38.000000000 +0200
+++ /tmp/output.c	2007-06-05 11:38:47.000000000 +0200
@@ -776,7 +776,7 @@
 		/* bits set in *arg is set to input,
 		 * *arg updated with current input pins.
 		 */
-		if (copy_from_user(&val, (unsigned long*)arg, sizeof(val)))
 		{
 			ret = -EFAULT;
 			break;
@@ -789,7 +789,7 @@
 		/* bits set in *arg is set to output,
 		 * *arg updated with current output pins.
 		 */
-		if (copy_from_user(&val, (unsigned long*)arg, sizeof(val)))
 		{
 			ret = -EFAULT;
 			break;


--- /home/pad/kernels/git/linux-2.6/arch/mips/au1000/common/power.c	2007-03-27 15:12:41.000000000 +0200
+++ /tmp/output.c	2007-06-05 11:38:57.000000000 +0200
@@ -330,7 +330,7 @@
 			spin_unlock_irqrestore(&pm_lock, flags);
 			return -EFAULT;
 		}
-		if (copy_from_user(buf, buffer, *len)) {
 			spin_unlock_irqrestore(&pm_lock, flags);
 			return -EFAULT;
 		}

and some cases in lmc_main.c


>
>
> Frankly, I think that a better use of this tool would be to just report on
> the errors, let humans fix them up.

Ok. Do you have a preference on the format ?  a <file>:<line> format  ?

Is there a place that gathered all those implicit programming rules 
(that copy_from_user must not be called inside a spinlock, etc) so that
I can translate them in a script for our tool.