bugfix GFP_KERNEL -> GFP_ATOMIC in spin_locked region

In a few files a function such as usb_submit_urb is taking GFP_KERNEL as an argument whereas this function call is inside a spin_lock_irqsave region of code. Documentation says that it must be GFP_ATOMIC instead.

Me and my colleagues have made a tool targeting program transformations in device drivers. We have designed a scripting language for specifying easily program transformations and a transformation engine for performing them. In the spirit of Linux development practice, the language is based on the patch syntax. We call our scripts "semantic patches" because as opposed to traditional patches, our semantic patches do not work at the line level but on a high level representation of the C program.

FYI here is our semantic patch detecting invalid use of GFP_KERNEL and fixing the problem:

@@
identifier fn;
@@

spin_lock_irqsave(...)
... when != spin_unlock_irqrestore(...)
fn(...,
- GFP_KERNEL
+ GFP_ATOMIC
,...
)

And now the real patch resulting from the automated transformation:

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

drivers/net/wan/lmc/lmc_main.c +0 -0

index ae132c1..750b3ef 100644

drivers/usb/serial/io_ti.c +1 -1

index 544098d..9ec38e3 100644
@@ -2351,7 +2351,7 @@ static int restart_read(struct edgeport_
 		urb->complete = edge_bulk_in_callback;
 		urb->context = edge_port;
 		urb->dev = edge_port->port->serial->dev;
-		status = usb_submit_urb(urb, GFP_KERNEL);
+		status = usb_submit_urb(urb, GFP_ATOMIC);
 	}
 	edge_port->ep_read_urb_state = EDGE_READ_URB_RUNNING;
 	edge_port->shadow_mcr |= MCR_RTS;

drivers/usb/serial/ti_usb_3410_5052.c +1 -1

index 4203e2b..10dc36f 100644
@@ -1559,7 +1559,7 @@ static int ti_restart_read(struct ti_por
 		urb->complete = ti_bulk_in_callback;
 		urb->context = tport;
 		urb->dev = tport->tp_port->serial->dev;
-		status = usb_submit_urb(urb, GFP_KERNEL);
+		status = usb_submit_urb(urb, GFP_ATOMIC);
 	}
 	tport->tp_read_urb_state = TI_READ_URB_RUNNING;
 

drivers/usb/serial/whiteheat.c +2 -2

index 27c5f8f..1b01207 100644
@@ -1116,7 +1116,7 @@ static int firm_send_command (struct usb
 	memcpy (&transfer_buffer[1], data, datasize);
 	command_port->write_urb->transfer_buffer_length = datasize + 1;
 	command_port->write_urb->dev = port->serial->dev;
-	retval = usb_submit_urb (command_port->write_urb, GFP_KERNEL);
+	retval = usb_submit_urb (command_port->write_urb, GFP_ATOMIC);
 	if (retval) {
 		dbg("%s - submit urb failed", __FUNCTION__);
 		goto exit;

@@ -1316,7 +1316,7 @@ static int start_command_port(struct usb
 		usb_clear_halt(serial->dev, command_port->read_urb->pipe);
 
 		command_port->read_urb->dev = serial->dev;
-		retval = usb_submit_urb(command_port->read_urb, GFP_KERNEL);
+		retval = usb_submit_urb(command_port->read_urb, GFP_ATOMIC);
 		if (retval) {
 			err("%s - failed submitting read urb, error %d", __FUNCTION__, retval);
 			goto exit;

Quoted 51 lines

drivers/usb/serial/io_ti.c +1 -1

index 544098d..9ec38e3 100644
@@ -2351,7 +2351,7 @@ static int restart_read(struct edgeport_
 		urb->complete = edge_bulk_in_callback;
 		urb->context = edge_port;
 		urb->dev = edge_port->port->serial->dev;
-		status = usb_submit_urb(urb, GFP_KERNEL);
+		status = usb_submit_urb(urb, GFP_ATOMIC);
 	}
 	edge_port->ep_read_urb_state = EDGE_READ_URB_RUNNING;
 	edge_port->shadow_mcr |= MCR_RTS;

drivers/usb/serial/ti_usb_3410_5052.c +1 -1

index 4203e2b..10dc36f 100644
@@ -1559,7 +1559,7 @@ static int ti_restart_read(struct ti_por
 		urb->complete = ti_bulk_in_callback;
 		urb->context = tport;
 		urb->dev = tport->tp_port->serial->dev;
-		status = usb_submit_urb(urb, GFP_KERNEL);
+		status = usb_submit_urb(urb, GFP_ATOMIC);
 	}
 	tport->tp_read_urb_state = TI_READ_URB_RUNNING;
 

drivers/usb/serial/whiteheat.c +2 -2

index 27c5f8f..1b01207 100644
@@ -1116,7 +1116,7 @@ static int firm_send_command (struct usb
 	memcpy (&transfer_buffer[1], data, datasize);
 	command_port->write_urb->transfer_buffer_length = datasize + 1;
 	command_port->write_urb->dev = port->serial->dev;
-	retval = usb_submit_urb (command_port->write_urb, GFP_KERNEL);
+	retval = usb_submit_urb (command_port->write_urb, GFP_ATOMIC);
 	if (retval) {
 		dbg("%s - submit urb failed", __FUNCTION__);
 		goto exit;

@@ -1316,7 +1316,7 @@ static int start_command_port(struct usb
 		usb_clear_halt(serial->dev, command_port->read_urb->pipe);
 
 		command_port->read_urb->dev = serial->dev;
-		retval = usb_submit_urb(command_port->read_urb, GFP_KERNEL);
+		retval = usb_submit_urb(command_port->read_urb, GFP_ATOMIC);
 		if (retval) {
 			err("%s - failed submitting read urb, error %d", __FUNCTION__, retval);
 			goto exit;

@@ -1363,7 +1363,7 @@ static int start_port_read(struct usb_se
 		wrap = list_entry(tmp, struct whiteheat_urb_wrap, list);
 		urb = wrap->urb;

drivers/usb/serial/whiteheat.c static int start_port_read(struct usb_se · line 1369

Quoted 6 lines

drivers/usb/serial/whiteheat.c +1 -1

 		urb->dev = port->serial->dev;
-		retval = usb_submit_urb(urb, GFP_KERNEL);
+		retval = usb_submit_urb(urb, GFP_ATOMIC);
 		if (retval) {
 			list_add(tmp, &info->rx_urbs_free);
 			list_for_each_safe(tmp, tmp2, &info->rx_urbs_submitted) {

This part might make sense so I'll queue it for the USB guys to look at.

Everything in USB appears to already be fixed, apart from the io_ti.c bug.

Quoted 7 lines

 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.

drivers/net/wan/lmc/lmc_main.c #endif /* end ifdef _DBG_EVENTLOG */ · line 489

Quoted 13 lines

drivers/net/wan/lmc/lmc_main.c +1 -1

index ae132c1..750b3ef 100644
@@ -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.

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.

Am Dienstag, 5. Juni 2007 13:05 schrieb Yoann Padioleau:

Quoted 5 lines

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.

How much C does your tool understand?

You might basically
test for code paths that go to "might_sleep()"

Regards
Oliver

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors

Quoted 8 lines · Oliver Neukum

Am Dienstag, 5. Juni 2007 13:05 schrieb Yoann Padioleau:

Quoted 5 lines

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.

How much C does your tool understand?

The tool understands almost all the C language but the analysis we do for the moment are intra-procedural so when we look for
spin_lock();
...
copy_from_user();

it can detect cases and code paths only when the two function calls are in the same function. It could be extended but it would require to do a full analysis of the kernel source. Maybe if some functions of the library have an attribute in their prototype in the .h such as

__might_sleep copy_from_user();

it could help.

Quoted 2 lines

You might basically
test for code paths that go to "might_sleep()"

Ok, thanks. If you know other implicit programming rules, I would be glad to know them, or if you know places where thus rules are written.

BTW at one point I think the Linux community were using advanced static analysis tools such as the one made by Dawson Engler (now Coverity). The communitty have stopped using such tools ? Isn't the role of sparse to detect bugs such as the dangerous copy_from_user() inside spinlocked region ?

Quoted 8 lines · Andrew Morton

Quoted 7 lines

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

Quoted 2 lines

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 ?

Quoted 16 lines

drivers/net/wan/lmc/lmc_main.c #endif /* end ifdef _DBG_EVENTLOG */ · line 489

Quoted 13 lines

drivers/net/wan/lmc/lmc_main.c +1 -1

index ae132c1..750b3ef 100644
@@ -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:

Quoted 3 lines

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

tmp/output.c renamed from home/pad/kernels/git/linux-2.6/arch/cris/arch-v10/drivers/gpio.c +0 -2

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

tmp/output.c renamed from home/pad/kernels/git/linux-2.6/arch/mips/au1000/common/power.c +0 -1

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

Quoted 5 lines

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.