* esp_scsi incorrect unsigned test [not found] ` <Pine.LNX.4.64.0804182101200.14832@ask.diku.dk> @ 2008-04-19 14:05 ` Matthew Wilcox 2008-04-19 14:16 ` James Bottomley 2008-04-19 14:17 ` u14-3f " Matthew Wilcox 1 sibling, 1 reply; 4+ messages in thread From: Matthew Wilcox @ 2008-04-19 14:05 UTC (permalink / raw) To: Julia Lawall, David Miller, linux-scsi Cc: Roel Kluin, kernel-janitors, kernelnewbies-bounce On Fri, Apr 18, 2008 at 09:08:55PM +0200, Julia Lawall wrote: > I found 63 occurrences of this problem with the following semantic match > (http://www.emn.fr/x-info/coccinelle/): > > @@ unsigned int i; @@ > > * i < 0 > > I looked through all of the results by hand, and they all seem to be > problems. In many cases, it seems like the variable should not be > unsigned as it is used to hold the return value of a function that might > return a negative error code, but I haven't looked into this in detail. > > In the output below, the lines that begin with a single start contain a > test of whether an unsigned variable or structure field is less than 0. > The output is actually generated with diff, but I converted the -s to *s > to avoid confusion. > diff -u -p a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c > *** a/drivers/scsi/esp_scsi.c 2008-03-12 14:13:14.000000000 +0100 > @@ -380,7 +380,7 @@ static void esp_advance_dma(struct esp * > > p->cur_residue -= len; > p->tot_residue -= len; > * if (p->cur_residue < 0 || p->tot_residue < 0) { > printk(KERN_ERR PFX "esp%d: Data transfer overflow.\n", > esp->host->unique_id); > printk(KERN_ERR PFX "esp%d: cur_residue[%d] tot_residue[%d] " This is clearly buggy. A residue is, though, inherently unsigned, so I don't think we should change the type of the variable. Rather, we should test it before subtraction. Dave, what do you think? ---- Fix ESP data transfer overflow checks The current code attempts to detect data transfer overflow by checking whether an unsigned variable is negative. Instead, we should compare the two variables before subtracting them. Reported-by: Julia Lawall <julia@diku.dk> Signed-off-by: Matthew Wilcox <willy@linux.intel.com> diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c index bfdee59..b5e8a94 100644 --- a/drivers/scsi/esp_scsi.c +++ b/drivers/scsi/esp_scsi.c @@ -378,9 +378,7 @@ static void esp_advance_dma(struct esp *esp, struct esp_cmd_entry *ent, return; } - p->cur_residue -= len; - p->tot_residue -= len; - if (p->cur_residue < 0 || p->tot_residue < 0) { + if (unlikely(p->cur_residue < len || p->tot_residue < len)) { printk(KERN_ERR PFX "esp%d: Data transfer overflow.\n", esp->host->unique_id); printk(KERN_ERR PFX "esp%d: cur_residue[%d] tot_residue[%d] " @@ -389,7 +387,11 @@ static void esp_advance_dma(struct esp *esp, struct esp_cmd_entry *ent, p->cur_residue, p->tot_residue, len); p->cur_residue = 0; p->tot_residue = 0; + } else { + p->cur_residue -= len; + p->tot_residue -= len; } + if (!p->cur_residue && p->tot_residue) { p->cur_sg++; p->cur_residue = sg_dma_len(p->cur_sg); -- Intel are signing my paycheques ... these opinions are still mine "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step." ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: esp_scsi incorrect unsigned test 2008-04-19 14:05 ` esp_scsi incorrect unsigned test Matthew Wilcox @ 2008-04-19 14:16 ` James Bottomley 2008-04-20 0:59 ` David Miller 0 siblings, 1 reply; 4+ messages in thread From: James Bottomley @ 2008-04-19 14:16 UTC (permalink / raw) To: Matthew Wilcox Cc: Julia Lawall, David Miller, linux-scsi, Roel Kluin, kernel-janitors, kernelnewbies-bounce On Sat, 2008-04-19 at 08:05 -0600, Matthew Wilcox wrote: > On Fri, Apr 18, 2008 at 09:08:55PM +0200, Julia Lawall wrote: > > I found 63 occurrences of this problem with the following semantic match > > (http://www.emn.fr/x-info/coccinelle/): > > > > @@ unsigned int i; @@ > > > > * i < 0 > > > > I looked through all of the results by hand, and they all seem to be > > problems. In many cases, it seems like the variable should not be > > unsigned as it is used to hold the return value of a function that might > > return a negative error code, but I haven't looked into this in detail. > > > > In the output below, the lines that begin with a single start contain a > > test of whether an unsigned variable or structure field is less than 0. > > The output is actually generated with diff, but I converted the -s to *s > > to avoid confusion. > > > diff -u -p a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c > > *** a/drivers/scsi/esp_scsi.c 2008-03-12 14:13:14.000000000 +0100 > > @@ -380,7 +380,7 @@ static void esp_advance_dma(struct esp * > > > > p->cur_residue -= len; > > p->tot_residue -= len; > > * if (p->cur_residue < 0 || p->tot_residue < 0) { > > printk(KERN_ERR PFX "esp%d: Data transfer overflow.\n", > > esp->host->unique_id); > > printk(KERN_ERR PFX "esp%d: cur_residue[%d] tot_residue[%d] " > > This is clearly buggy. A residue is, though, inherently unsigned, so I > don't think we should change the type of the variable. Rather, we > should test it before subtraction. Dave, what do you think? > > ---- > > Fix ESP data transfer overflow checks > > The current code attempts to detect data transfer overflow by checking > whether an unsigned variable is negative. Instead, we should > compare the two variables before subtracting them. > > Reported-by: Julia Lawall <julia@diku.dk> > Signed-off-by: Matthew Wilcox <willy@linux.intel.com> > > diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c > index bfdee59..b5e8a94 100644 > --- a/drivers/scsi/esp_scsi.c > +++ b/drivers/scsi/esp_scsi.c > @@ -378,9 +378,7 @@ static void esp_advance_dma(struct esp *esp, struct esp_cmd_entry *ent, > return; > } > > - p->cur_residue -= len; > - p->tot_residue -= len; > - if (p->cur_residue < 0 || p->tot_residue < 0) { > + if (unlikely(p->cur_residue < len || p->tot_residue < len)) { > printk(KERN_ERR PFX "esp%d: Data transfer overflow.\n", > esp->host->unique_id); > printk(KERN_ERR PFX "esp%d: cur_residue[%d] tot_residue[%d] " To be pedantic, this should be %u not %d for unsigned. > @@ -389,7 +387,11 @@ static void esp_advance_dma(struct esp *esp, struct esp_cmd_entry *ent, > p->cur_residue, p->tot_residue, len); > p->cur_residue = 0; > p->tot_residue = 0; > + } else { > + p->cur_residue -= len; > + p->tot_residue -= len; > } > + > if (!p->cur_residue && p->tot_residue) { > p->cur_sg++; > p->cur_residue = sg_dma_len(p->cur_sg); It look fine to me ... although an alternative (and possibly simpler) fix would just be to remove the unsigned from the two residue definitions in the struct. James ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: esp_scsi incorrect unsigned test 2008-04-19 14:16 ` James Bottomley @ 2008-04-20 0:59 ` David Miller 0 siblings, 0 replies; 4+ messages in thread From: David Miller @ 2008-04-20 0:59 UTC (permalink / raw) To: James.Bottomley Cc: matthew, julia, linux-scsi, 12o3l, kernel-janitors, kernelnewbies-bounce From: James Bottomley <James.Bottomley@HansenPartnership.com> Date: Sat, 19 Apr 2008 09:16:38 -0500 > It look fine to me ... although an alternative (and possibly simpler) > fix would just be to remove the unsigned from the two residue > definitions in the struct. I think that's the better approach too. James, please apply. esp_scsi: Make cur_residue and tot_residue signed. Many of the overflow checks test whether the value has gone negative, and we want to retain such checks. Reported by Julia Lawall. Signed-off-by: David S. Miller <davem@davemloft.net> diff --git a/drivers/scsi/esp_scsi.h b/drivers/scsi/esp_scsi.h index d5576d5..9367a88 100644 --- a/drivers/scsi/esp_scsi.h +++ b/drivers/scsi/esp_scsi.h @@ -240,9 +240,9 @@ struct esp_cmd_priv { int num_sg; } u; - unsigned int cur_residue; + int cur_residue; struct scatterlist *cur_sg; - unsigned int tot_residue; + int tot_residue; }; #define ESP_CMD_PRIV(CMD) ((struct esp_cmd_priv *)(&(CMD)->SCp)) ^ permalink raw reply related [flat|nested] 4+ messages in thread
* u14-3f incorrect unsigned test [not found] ` <Pine.LNX.4.64.0804182101200.14832@ask.diku.dk> 2008-04-19 14:05 ` esp_scsi incorrect unsigned test Matthew Wilcox @ 2008-04-19 14:17 ` Matthew Wilcox 1 sibling, 0 replies; 4+ messages in thread From: Matthew Wilcox @ 2008-04-19 14:17 UTC (permalink / raw) To: Julia Lawall, linux-scsi Cc: Roel Kluin, kernel-janitors, kernelnewbies-bounce On Fri, Apr 18, 2008 at 09:08:55PM +0200, Julia Lawall wrote: > I found 63 occurrences of this problem with the following semantic match > (http://www.emn.fr/x-info/coccinelle/): > > @@ unsigned int i; @@ > > * i < 0 > > I looked through all of the results by hand, and they all seem to be > problems. In many cases, it seems like the variable should not be > unsigned as it is used to hold the return value of a function that might > return a negative error code, but I haven't looked into this in detail. > > In the output below, the lines that begin with a single start contain a > test of whether an unsigned variable or structure field is less than 0. > The output is actually generated with diff, but I converted the -s to *s > to avoid confusion. > diff -u -p a/drivers/scsi/u14-34f.c b/drivers/scsi/u14-34f.c > *** a/drivers/scsi/u14-34f.c 2008-03-12 14:13:14.000000000 +0100 > @@ -1126,7 +1126,7 @@ static void map_dma(unsigned int i, unsi > > if (scsi_bufflen(SCpnt)) { > count = scsi_dma_map(SCpnt); > * BUG_ON(count < 0); > > scsi_for_each_sg(SCpnt, sg, count, k) { > cpp->sglist[k].address = H2DEV(sg_dma_address(sg)); ---- u14-34f: Correct unsigned comparison against 0 scsi_dma_map() can return an error. The current situation of BUG_ON for an out of memory condition is far from ideal, but it's better than the current situation where running out of memory will lead to the driver trying to walk through 4 billion sg entries. Discovered-by: Julia Lawall <julia@diku.dk> Signed-off-by: Matthew Wilcox <willy@linux.intel.com> diff --git a/drivers/scsi/u14-34f.c b/drivers/scsi/u14-34f.c index 58d7eee..11df071 100644 --- a/drivers/scsi/u14-34f.c +++ b/drivers/scsi/u14-34f.c @@ -1110,7 +1110,7 @@ static int u14_34f_detect(struct scsi_host_template *tpnt) { static void map_dma(unsigned int i, unsigned int j) { unsigned int data_len = 0; - unsigned int k, count, pci_dir; + unsigned int k, pci_dir; struct scatterlist *sg; struct mscp *cpp; struct scsi_cmnd *SCpnt; @@ -1125,7 +1125,7 @@ static void map_dma(unsigned int i, unsigned int j) { cpp->sense_len = SCSI_SENSE_BUFFERSIZE; if (scsi_bufflen(SCpnt)) { - count = scsi_dma_map(SCpnt); + int count = scsi_dma_map(SCpnt); BUG_ON(count < 0); scsi_for_each_sg(SCpnt, sg, count, k) { -- Intel are signing my paycheques ... these opinions are still mine "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step." ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2008-04-20 0:59 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <4808C90A.5040600@tiscali.nl>
[not found] ` <Pine.LNX.4.64.0804182101200.14832@ask.diku.dk>
2008-04-19 14:05 ` esp_scsi incorrect unsigned test Matthew Wilcox
2008-04-19 14:16 ` James Bottomley
2008-04-20 0:59 ` David Miller
2008-04-19 14:17 ` u14-3f " Matthew Wilcox
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox