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(-)
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;
diff --git a/drivers/scsi/megaraid/megaraid_mm.c b/drivers/scsi/megaraid/megaraid_mm.c
index e075a52..edee220 100644
--- a/drivers/scsi/megaraid/megaraid_mm.c
+++ b/drivers/scsi/megaraid/megaraid_mm.c
@@ -547,7 +547,7 @@ mraid_mm_attach_buf(mraid_mmadp_t *adp,
kioc->pool_index = right_pool;
kioc->free_buf = 1;
- kioc->buf_vaddr = pci_pool_alloc(pool->handle, GFP_KERNEL,
+ kioc->buf_vaddr = pci_pool_alloc(pool->handle, GFP_ATOMIC,
&kioc->buf_paddr);
spin_unlock_irqrestore(&pool->lock, flags);
diff --git a/drivers/usb/serial/io_ti.c b/drivers/usb/serial/io_ti.c
index 544098d..9ec38e3 100644
--- a/drivers/usb/serial/io_ti.c
+++ b/drivers/usb/serial/io_ti.c
@@ -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;
diff --git a/drivers/usb/serial/ti_usb_3410_5052.c b/drivers/usb/serial/ti_usb_3410_5052.c
index 4203e2b..10dc36f 100644
--- a/drivers/usb/serial/ti_usb_3410_5052.c
+++ b/drivers/usb/serial/ti_usb_3410_5052.c
@@ -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;
diff --git a/drivers/usb/serial/whiteheat.c b/drivers/usb/serial/whiteheat.c
index 27c5f8f..1b01207 100644
--- a/drivers/usb/serial/whiteheat.c
+++ b/drivers/usb/serial/whiteheat.c
@@ -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;
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) {
On Mon, 04 Jun 2007 18:25:28 +0200 Yoann Padioleau <[email protected]> wrote:
>
> 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
> ,...
> )
I think I read that paper.
> 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(-)
This patch covers three areas of maintainer responsibility, so poor old me
has to split it up and redo the changelogs. Oh well.
>
> 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.
Frankly, I think that a better use of this tool would be to just report on
the errors, let humans fix them up.
Nobody maintains this ATM code afaik.
> index e075a52..edee220 100644
> --- a/drivers/scsi/megaraid/megaraid_mm.c
> +++ b/drivers/scsi/megaraid/megaraid_mm.c
> @@ -547,7 +547,7 @@ mraid_mm_attach_buf(mraid_mmadp_t *adp,
>
> kioc->pool_index = right_pool;
> kioc->free_buf = 1;
> - kioc->buf_vaddr = pci_pool_alloc(pool->handle, GFP_KERNEL,
> + kioc->buf_vaddr = pci_pool_alloc(pool->handle, GFP_ATOMIC,
> &kioc->buf_paddr);
> spin_unlock_irqrestore(&pool->lock, flags);
Again, a better fix would probably be to move the pci_pool_alloc() to
before the spin_lock_irqsave(), so we can continue to use the stronger
GFP_KERNEL.
But the locking in there looks basically nonsensical or wrong anyway. It
appears that local variable `right_pool' cannot be validly used unless
we're holding pool->lock for the whole duration.
Somebody does maintain the megaraid driver, but I'm not sure who, and
the MAINTAINERS file isn't very useful. So I'll spray it around a bit.
We definitely have bugs in there.
> diff --git a/drivers/usb/serial/io_ti.c b/drivers/usb/serial/io_ti.c
> index 544098d..9ec38e3 100644
> --- a/drivers/usb/serial/io_ti.c
> +++ b/drivers/usb/serial/io_ti.c
> @@ -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;
> diff --git a/drivers/usb/serial/ti_usb_3410_5052.c b/drivers/usb/serial/ti_usb_3410_5052.c
> index 4203e2b..10dc36f 100644
> --- a/drivers/usb/serial/ti_usb_3410_5052.c
> +++ b/drivers/usb/serial/ti_usb_3410_5052.c
> @@ -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;
>
> diff --git a/drivers/usb/serial/whiteheat.c b/drivers/usb/serial/whiteheat.c
> index 27c5f8f..1b01207 100644
> --- a/drivers/usb/serial/whiteheat.c
> +++ b/drivers/usb/serial/whiteheat.c
> @@ -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;
> 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.
On Mon, 4 Jun 2007 21:00:18 -0700 Andrew Morton <[email protected]> wrote:
> > diff --git a/drivers/usb/serial/io_ti.c b/drivers/usb/serial/io_ti.c
> > index 544098d..9ec38e3 100644
> > --- a/drivers/usb/serial/io_ti.c
> > +++ b/drivers/usb/serial/io_ti.c
> > @@ -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;
> > diff --git a/drivers/usb/serial/ti_usb_3410_5052.c b/drivers/usb/serial/ti_usb_3410_5052.c
> > index 4203e2b..10dc36f 100644
> > --- a/drivers/usb/serial/ti_usb_3410_5052.c
> > +++ b/drivers/usb/serial/ti_usb_3410_5052.c
> > @@ -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;
> >
> > diff --git a/drivers/usb/serial/whiteheat.c b/drivers/usb/serial/whiteheat.c
> > index 27c5f8f..1b01207 100644
> > --- a/drivers/usb/serial/whiteheat.c
> > +++ b/drivers/usb/serial/whiteheat.c
> > @@ -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;
> > 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.
Am Dienstag, 5. Juni 2007 06:08 schrieb Andrew Morton:
> Everything in USB appears to already be fixed, apart from the io_ti.c bug.
Yes, that's a bug. I've queued a patch.
Regards
Oliver
Andrew Morton <[email protected]> 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.
Am Dienstag, 5. Juni 2007 13:05 schrieb Yoann Padioleau:
> 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
On Tue, 05 Jun 2007 13:05:18 +0200 Yoann Padioleau <[email protected]> wrote:
> Andrew Morton <[email protected]> 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.
That's OK - I can take care of that
> We plan to send a patch that
> replaces some calls to kmalloc/memset with kzalloc; do we have to split
> it for each maintainer ?
Per subsystem: yes, please. That maps pretty well onto per-subdirectory so
I'd suggest that each patch only affect the files in a single directory.
> >> 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
OK.
>
> >
> >
> > 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 ?
file-n-line is good. If it can quote the relevant test then that's better
- line numbers change often.
Please always work against the very latest Linus git tree,
> 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.
Not that I can think of, sorry.
Oliver Neukum <[email protected]> writes:
> Am Dienstag, 5. Juni 2007 13:05 schrieb Yoann Padioleau:
>> 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.
> 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 ?
>
> Regards
> Oliver
>
> _______________________________________________
> Kernel-janitors mailing list
> [email protected]
> https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors
On Tue, 05 Jun 2007 18:31:42 +0200 Yoann Padioleau wrote:
> Oliver Neukum <[email protected]> writes:
>
> > Am Dienstag, 5. Juni 2007 13:05 schrieb Yoann Padioleau:
> >> 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.
>
> > 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 ?
>
There are a few people who have registered for access to the
Coverity database and occasionally go thru it looking for bugs
and then posting fixes.
sparse can check for unbalanced locking, but it needs annotations
for those AFAIK.
---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***