This patch fixes up the following :
CC drivers/message/i2o/i2o_config.o
include/asm/uaccess.h: In function `i2o_cfg_getiops':
drivers/message/i2o/i2o_config.c:190: warning: ignoring return value of `__copy_to_user', declared with attribute warn_unused_result
include/asm/uaccess.h: In function `i2o_cfg_swul':
drivers/message/i2o/i2o_config.c:477: warning: ignoring return value of `__copy_to_user', declared with attribute warn_unused_result
Signed-off-by: Jesper Juhl <[email protected]>
diff -up linux-2.6.9-rc3-bk5-orig/drivers/message/i2o/i2o_config.c linux-2.6.9-rc3-bk5/drivers/message/i2o/i2o_config.c
--- linux-2.6.9-rc3-bk5-orig/drivers/message/i2o/i2o_config.c 2004-09-30 05:05:40.000000000 +0200
+++ linux-2.6.9-rc3-bk5/drivers/message/i2o/i2o_config.c 2004-10-05 23:32:43.000000000 +0200
@@ -187,7 +187,8 @@ static int i2o_cfg_getiops(unsigned long
list_for_each_entry(c, &i2o_controllers, list)
tmp[c->unit] = 1;
- __copy_to_user(user_iop_table, tmp, MAX_I2O_CONTROLLERS);
+ if (__copy_to_user(user_iop_table, tmp, MAX_I2O_CONTROLLERS))
+ return -EFAULT;
return 0;
};
@@ -474,7 +475,9 @@ static int i2o_cfg_swul(unsigned long ar
return status;
}
- __copy_to_user(kxfer.buf, buffer.virt, fragsize);
+ if (__copy_to_user(kxfer.buf, buffer.virt, fragsize))
+ return -EFAULT;
+
i2o_dma_free(&c->pdev->dev, &buffer);
return 0;
Jesper Juhl <[email protected]> wrote:
>
> - __copy_to_user(kxfer.buf, buffer.virt, fragsize);
> + if (__copy_to_user(kxfer.buf, buffer.virt, fragsize))
> + return -EFAULT;
> +
> i2o_dma_free(&c->pdev->dev, &buffer);
>
Obvious leak.
On Tue, 5 Oct 2004, Andrew Morton wrote:
> Jesper Juhl <[email protected]> wrote:
> >
> > - __copy_to_user(kxfer.buf, buffer.virt, fragsize);
> > + if (__copy_to_user(kxfer.buf, buffer.virt, fragsize))
> > + return -EFAULT;
> > +
> > i2o_dma_free(&c->pdev->dev, &buffer);
> >
>
> Obvious leak.
>
Whoops, so sorry about that, I must be sleeping. Thank you for your
vigilance :)
Here's a better patch.
Signed-off-by: Jesper Juhl <[email protected]>
diff -up linux-2.6.9-rc3-bk5-orig/drivers/message/i2o/i2o_config.c linux-2.6.9-rc3-bk5/drivers/message/i2o/i2o_config.c
--- linux-2.6.9-rc3-bk5-orig/drivers/message/i2o/i2o_config.c 2004-09-30 05:05:40.000000000 +0200
+++ linux-2.6.9-rc3-bk5/drivers/message/i2o/i2o_config.c 2004-10-06 00:29:30.000000000 +0200
@@ -187,7 +187,8 @@ static int i2o_cfg_getiops(unsigned long
list_for_each_entry(c, &i2o_controllers, list)
tmp[c->unit] = 1;
- __copy_to_user(user_iop_table, tmp, MAX_I2O_CONTROLLERS);
+ if (__copy_to_user(user_iop_table, tmp, MAX_I2O_CONTROLLERS))
+ return -EFAULT;
return 0;
};
@@ -474,7 +475,11 @@ static int i2o_cfg_swul(unsigned long ar
return status;
}
- __copy_to_user(kxfer.buf, buffer.virt, fragsize);
+ if (__copy_to_user(kxfer.buf, buffer.virt, fragsize)) {
+ i2o_dma_free(&c->pdev->dev, &buffer);
+ return -EFAULT;
+ }
+
i2o_dma_free(&c->pdev->dev, &buffer);
return 0;
Could probably be done a little nicer for the entire function with a
"retval" variable and some goto's here and there, but I opted for the
simple solution.
Jesper Juhl <[email protected]> wrote:
>
> - __copy_to_user(kxfer.buf, buffer.virt, fragsize);
> + if (__copy_to_user(kxfer.buf, buffer.virt, fragsize)) {
> + i2o_dma_free(&c->pdev->dev, &buffer);
> + return -EFAULT;
> + }
> +
> i2o_dma_free(&c->pdev->dev, &buffer);
Please try to avoid more than a single return statement per function.
Also, please try to avoid duplicating code such as the above - it's a
maintainance problem is nothing else.
Like this:
--- 25/drivers/message/i2o/i2o_config.c~add-missing-checks-of-__copy_to_user-return-value-in Tue Oct 5 15:41:04 2004
+++ 25-akpm/drivers/message/i2o/i2o_config.c Tue Oct 5 15:42:17 2004
@@ -187,7 +187,8 @@ static int i2o_cfg_getiops(unsigned long
list_for_each_entry(c, &i2o_controllers, list)
tmp[c->unit] = 1;
- __copy_to_user(user_iop_table, tmp, MAX_I2O_CONTROLLERS);
+ if (__copy_to_user(user_iop_table, tmp, MAX_I2O_CONTROLLERS))
+ return -EFAULT;
return 0;
};
@@ -416,6 +417,7 @@ static int i2o_cfg_swul(unsigned long ar
u32 m;
unsigned int status = 0, swlen = 0, fragsize = 8192;
struct i2o_controller *c;
+ int ret;
if (copy_from_user(&kxfer, pxfer, sizeof(struct i2o_sw_xfer)))
return -EFAULT;
@@ -474,10 +476,11 @@ static int i2o_cfg_swul(unsigned long ar
return status;
}
- __copy_to_user(kxfer.buf, buffer.virt, fragsize);
+ ret = 0;
+ if (__copy_to_user(kxfer.buf, buffer.virt, fragsize))
+ ret = -EFAULT;
i2o_dma_free(&c->pdev->dev, &buffer);
-
- return 0;
+ return ret;
};
static int i2o_cfg_swdel(unsigned long arg)
_
On Tue, 5 Oct 2004, Andrew Morton wrote:
> Jesper Juhl <[email protected]> wrote:
> >
> > - __copy_to_user(kxfer.buf, buffer.virt, fragsize);
> > + if (__copy_to_user(kxfer.buf, buffer.virt, fragsize)) {
> > + i2o_dma_free(&c->pdev->dev, &buffer);
> > + return -EFAULT;
> > + }
> > +
> > i2o_dma_free(&c->pdev->dev, &buffer);
>
> Please try to avoid more than a single return statement per function.
>
Will do. But this function already had a ton of return statements so I
assumed one more wouldn't make any real difference - I'll be sure to give
that more thought in the future.
> Also, please try to avoid duplicating code such as the above - it's a
> maintainance problem is nothing else.
>
> Like this:
>
> - __copy_to_user(kxfer.buf, buffer.virt, fragsize);
> + ret = 0;
> + if (__copy_to_user(kxfer.buf, buffer.virt, fragsize))
> + ret = -EFAULT;
> i2o_dma_free(&c->pdev->dev, &buffer);
> -
> - return 0;
> + return ret;
> };
Yes, that's what I had in mind when I noted a prettier version could be
made with a retval variable, the goto's I hinted at where intended to get
rid of the many return statements and create a single exit point for the
function. Your point about maintainability is noted.
How about just doing it all as in this patch below ?
I considered adding
return_nomem:
ret = -ENOMEM;
goto return_ret;
etc at the bottom as well, for the other return statements to use, but
that would be a lot of labels and gotos at the end, seems cleaner not to -
comments?
Signed-off-by: Jesper Juhl <[email protected]>
diff -up linux-2.6.9-rc3-bk5-orig/drivers/message/i2o/i2o_config.c linux-2.6.9-rc3-bk5/drivers/message/i2o/i2o_config.c
--- linux-2.6.9-rc3-bk5-orig/drivers/message/i2o/i2o_config.c 2004-09-30 05:05:40.000000000 +0200
+++ linux-2.6.9-rc3-bk5/drivers/message/i2o/i2o_config.c 2004-10-06 01:05:04.000000000 +0200
@@ -187,7 +187,8 @@ static int i2o_cfg_getiops(unsigned long
list_for_each_entry(c, &i2o_controllers, list)
tmp[c->unit] = 1;
- __copy_to_user(user_iop_table, tmp, MAX_I2O_CONTROLLERS);
+ if (__copy_to_user(user_iop_table, tmp, MAX_I2O_CONTROLLERS))
+ return -EFAULT;
return 0;
};
@@ -416,24 +417,25 @@ static int i2o_cfg_swul(unsigned long ar
u32 m;
unsigned int status = 0, swlen = 0, fragsize = 8192;
struct i2o_controller *c;
+ int ret = 0;
if (copy_from_user(&kxfer, pxfer, sizeof(struct i2o_sw_xfer)))
- return -EFAULT;
+ goto return_fault;
if (get_user(swlen, kxfer.swlen) < 0)
- return -EFAULT;
+ goto return_fault;
if (get_user(maxfrag, kxfer.maxfrag) < 0)
- return -EFAULT;
+ goto return_fault;
if (get_user(curfrag, kxfer.curfrag) < 0)
- return -EFAULT;
+ goto return_fault;
if (curfrag == maxfrag)
fragsize = swlen - (maxfrag - 1) * 8192;
if (!kxfer.buf || !access_ok(VERIFY_WRITE, kxfer.buf, fragsize))
- return -EFAULT;
+ goto return_fault;
c = i2o_find_iop(kxfer.iop);
if (!c)
@@ -474,10 +476,16 @@ static int i2o_cfg_swul(unsigned long ar
return status;
}
- __copy_to_user(kxfer.buf, buffer.virt, fragsize);
+ if (__copy_to_user(kxfer.buf, buffer.virt, fragsize))
+ ret = -EFAULT;
+
i2o_dma_free(&c->pdev->dev, &buffer);
- return 0;
+return_ret:
+ return ret;
+return_fault:
+ ret = -EFAULT;
+ goto return_ret;
};
static int i2o_cfg_swdel(unsigned long arg)
On Tue, Oct 05, 2004 at 11:43:54PM +0200, Jesper Juhl wrote:
>
> This patch fixes up the following :
>
> CC drivers/message/i2o/i2o_config.o
> include/asm/uaccess.h: In function `i2o_cfg_getiops':
> drivers/message/i2o/i2o_config.c:190: warning: ignoring return value of `__copy_to_user', declared with attribute warn_unused_result
> include/asm/uaccess.h: In function `i2o_cfg_swul':
> drivers/message/i2o/i2o_config.c:477: warning: ignoring return value of `__copy_to_user', declared with attribute warn_unused_result
>
> Signed-off-by: Jesper Juhl <[email protected]>
>
> diff -up linux-2.6.9-rc3-bk5-orig/drivers/message/i2o/i2o_config.c linux-2.6.9-rc3-bk5/drivers/message/i2o/i2o_config.c
> --- linux-2.6.9-rc3-bk5-orig/drivers/message/i2o/i2o_config.c 2004-09-30 05:05:40.000000000 +0200
> +++ linux-2.6.9-rc3-bk5/drivers/message/i2o/i2o_config.c 2004-10-05 23:32:43.000000000 +0200
> @@ -187,7 +187,8 @@ static int i2o_cfg_getiops(unsigned long
> list_for_each_entry(c, &i2o_controllers, list)
> tmp[c->unit] = 1;
>
> - __copy_to_user(user_iop_table, tmp, MAX_I2O_CONTROLLERS);
> + if (__copy_to_user(user_iop_table, tmp, MAX_I2O_CONTROLLERS))
> + return -EFAULT;
should be copy_to_user (with return value checked) and the
access_ok above should be removed.
> return 0;
> };
> @@ -474,7 +475,9 @@ static int i2o_cfg_swul(unsigned long ar
> return status;
> }
>
> - __copy_to_user(kxfer.buf, buffer.virt, fragsize);
> + if (__copy_to_user(kxfer.buf, buffer.virt, fragsize))
> + return -EFAULT;
> +
> i2o_dma_free(&c->pdev->dev, &buffer);
you're adding a leak here,and again please use copy_to_user and remove
the access_ok abov
Updated patch at the end (fourth edition).
On Wed, 6 Oct 2004, Christoph Hellwig wrote:
> On Tue, Oct 05, 2004 at 11:43:54PM +0200, Jesper Juhl wrote:
> >
> > diff -up linux-2.6.9-rc3-bk5-orig/drivers/message/i2o/i2o_config.c linux-2.6.9-rc3-bk5/drivers/message/i2o/i2o_config.c
> > --- linux-2.6.9-rc3-bk5-orig/drivers/message/i2o/i2o_config.c 2004-09-30 05:05:40.000000000 +0200
> > +++ linux-2.6.9-rc3-bk5/drivers/message/i2o/i2o_config.c 2004-10-05 23:32:43.000000000 +0200
> > @@ -187,7 +187,8 @@ static int i2o_cfg_getiops(unsigned long
> > list_for_each_entry(c, &i2o_controllers, list)
> > tmp[c->unit] = 1;
> >
> > - __copy_to_user(user_iop_table, tmp, MAX_I2O_CONTROLLERS);
> > + if (__copy_to_user(user_iop_table, tmp, MAX_I2O_CONTROLLERS))
> > + return -EFAULT;
>
> should be copy_to_user (with return value checked) and the
> access_ok above should be removed.
>
Makes sense to me. No need to explicitly check when copy_to_user() will do
so for us.
> > return 0;
> > };
> > @@ -474,7 +475,9 @@ static int i2o_cfg_swul(unsigned long ar
> > return status;
> > }
> >
> > - __copy_to_user(kxfer.buf, buffer.virt, fragsize);
> > + if (__copy_to_user(kxfer.buf, buffer.virt, fragsize))
> > + return -EFAULT;
> > +
> > i2o_dma_free(&c->pdev->dev, &buffer);
>
> you're adding a leak here,and again please use copy_to_user and remove
> the access_ok abov
Yeah, I goofed, Andrew already spotted that one and I send him an updated
patch (it's in this thread). Unfortunately my second patch also left
something to be desired, so I send off a third (didn't get any comment on
that one yet). After reading your comments I guess a fourth attempt at
this is required - how about something like the following ?
It does the following:
- gets rid of access_ok
- replaces __copy_to_user with copy_to_user that calls access_ok itself
- makes sure copy_to_user return value is checked and acted upon
- gets a bit closer to the goal of having only one exit point pr function
- doesn't leak resources and doesn't duplicate code
Signed-off-by: Jesper Juhl <[email protected]>
diff -up linux-2.6.9-rc3-bk5-orig/drivers/message/i2o/i2o_config.c linux-2.6.9-rc3-bk5/drivers/message/i2o/i2o_config.c
--- linux-2.6.9-rc3-bk5-orig/drivers/message/i2o/i2o_config.c 2004-09-30 05:05:40.000000000 +0200
+++ linux-2.6.9-rc3-bk5/drivers/message/i2o/i2o_config.c 2004-10-06 21:58:01.000000000 +0200
@@ -178,18 +178,17 @@ static int i2o_cfg_getiops(unsigned long
struct i2o_controller *c;
u8 __user *user_iop_table = (void __user *)arg;
u8 tmp[MAX_I2O_CONTROLLERS];
+ int ret = 0;
memset(tmp, 0, MAX_I2O_CONTROLLERS);
- if (!access_ok(VERIFY_WRITE, user_iop_table, MAX_I2O_CONTROLLERS))
- return -EFAULT;
-
list_for_each_entry(c, &i2o_controllers, list)
tmp[c->unit] = 1;
- __copy_to_user(user_iop_table, tmp, MAX_I2O_CONTROLLERS);
+ if (copy_to_user(user_iop_table, tmp, MAX_I2O_CONTROLLERS))
+ ret = -EFAULT;
- return 0;
+ return ret;
};
static int i2o_cfg_gethrt(unsigned long arg)
@@ -416,24 +415,25 @@ static int i2o_cfg_swul(unsigned long ar
u32 m;
unsigned int status = 0, swlen = 0, fragsize = 8192;
struct i2o_controller *c;
+ int ret = 0;
if (copy_from_user(&kxfer, pxfer, sizeof(struct i2o_sw_xfer)))
- return -EFAULT;
+ goto return_fault;
if (get_user(swlen, kxfer.swlen) < 0)
- return -EFAULT;
+ goto return_fault;
if (get_user(maxfrag, kxfer.maxfrag) < 0)
- return -EFAULT;
+ goto return_fault;
if (get_user(curfrag, kxfer.curfrag) < 0)
- return -EFAULT;
+ goto return_fault;
if (curfrag == maxfrag)
fragsize = swlen - (maxfrag - 1) * 8192;
- if (!kxfer.buf || !access_ok(VERIFY_WRITE, kxfer.buf, fragsize))
- return -EFAULT;
+ if (!kxfer.buf)
+ goto return_fault;
c = i2o_find_iop(kxfer.iop);
if (!c)
@@ -474,10 +474,16 @@ static int i2o_cfg_swul(unsigned long ar
return status;
}
- __copy_to_user(kxfer.buf, buffer.virt, fragsize);
+ if (copy_to_user(kxfer.buf, buffer.virt, fragsize))
+ ret = -EFAULT;
+
i2o_dma_free(&c->pdev->dev, &buffer);
- return 0;
+return_ret:
+ return ret;
+return_fault:
+ ret = -EFAULT;
+ goto return_ret;
};
static int i2o_cfg_swdel(unsigned long arg)
On Thu, 7 Oct 2004, Markus Lidel wrote:
> Hello,
> thanks for fixing i2o_config.
You're welcome.
> In the meantime i've also made a patch, which
> tries to reduce the many return's too.
> Note: i've not touched the passthru stuff yet.
> Please let me know, which you think of it.
> --- a/drivers/message/i2o/i2o_config.c 2004-09-24 11:05:12.044972000 +0200
> +++ b/drivers/message/i2o/i2o_config.c 2004-10-07 22:49:59.786543596 +0200
> @@ -178,18 +178,17 @@
> struct i2o_controller *c;
> u8 __user *user_iop_table = (void __user *)arg;
> u8 tmp[MAX_I2O_CONTROLLERS];
> + int rc = 0;
Purely personal preference, I think "ret" is a more intuitive name for
that variable.
+ if (copy_from_user(&kcmd, cmd, sizeof(struct i2o_cmd_hrtlct))) {
+ rc = -EFAULT;
+ goto exit;
+ }
[...]
+exit:
+ return rc;
};
I prefer the
if (foo)
goto return_fault;
[...]
return_ret:
return ret;
return_fault:
ret = -EFAULT;
goto return_ret;
variant.
Sure, your way results in just a single mov and a single jmp when
the branch is taken, but, my way has only a single jmp at each branch,
then a mov and an additional jmp at the end, so for a single branch your
way wins with
1 mov + 1 jmp vs my 2 jmp's and 1 mov.
but, when you have a lot those branches my way generates less code. At 5
branches you have 5 mov + 5 jmp while I'll have 1 mov + 6 jmp.
Also, my way takes up less space in the source, so more code fits on
screen.
I doubt it matters much since this doesn't look like a very hot code path,
but I prefer the version that generates the smaller code and takes up less
space in the source.
Apart from those nitpicks it looks good to me :)
--
Jesper Juhl
Hello,
thanks for fixing i2o_config. In the meantime i've also made a patch,
which tries to reduce the many return's too.
Note: i've not touched the passthru stuff yet.
Please let me know, which you think of it.
Best regards,
Markus Lidel
------------------------------------------
Markus Lidel (Senior IT Consultant)
Shadow Connect GmbH
Carl-Reisch-Weg 12
D-86381 Krumbach
Germany
Phone: +49 82 82/99 51-0
Fax: +49 82 82/99 51-11
E-Mail: [email protected]
URL: http://www.shadowconnect.com