* RE: [patch 1/2] Staging: intel_sst: return -EFAULT if
2010-10-15 3:44 [patch 1/2] Staging: intel_sst: return -EFAULT if copy_to_user() Dan Carpenter
@ 2010-10-15 4:35 ` Koul, Vinod
2010-10-15 5:29 ` Dan Carpenter
` (4 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Koul, Vinod @ 2010-10-15 4:35 UTC (permalink / raw)
To: kernel-janitors
> copy_to_user() returns the number of bytes remaining to be copied but we
> want to return an error code. And that error code is -EFAULT not -EIO.
>
> Signed-off-by: Dan Carpenter <error27@gmail.com>
>
> diff --git a/drivers/staging/intel_sst/intel_sst_app_interface.c
> b/drivers/staging/intel_sst/intel_sst_app_interface.c
> index baf0ddc..5ead4d1 100644
> --- a/drivers/staging/intel_sst/intel_sst_app_interface.c
> +++ b/drivers/staging/intel_sst/intel_sst_app_interface.c
> @@ -865,6 +865,8 @@ long intel_sst_ioctl(struct file *file_ptr, unsigned int cmd,
> unsigned long arg)
> str_info->src = SST_DRV;
> retval = copy_to_user(&str_param->stream_id,
> &retval, sizeof(__u32));
> + if (retval)
> + retval = -EFAULT;
> } else {
> if (retval = -SST_ERR_INVALID_PARAMS)
> retval = -EINVAL;
> @@ -938,7 +940,7 @@ long intel_sst_ioctl(struct file *file_ptr, unsigned int cmd,
> unsigned long arg)
> retval = copy_to_user((struct snd_sst_vol *)arg,
> &get_vol, sizeof(get_vol));
> if (retval) {
> - retval = -EIO;
> + retval = -EFAULT;
> break;
> }
> /*sst_print_get_vol_info(str_id, &get_vol);*/
Since retval is not used and value assigned is supposed to be -EFAULT,
how about this?
if(copy_to_user())
return -EIO;
~Vinod
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [patch 1/2] Staging: intel_sst: return -EFAULT if
2010-10-15 3:44 [patch 1/2] Staging: intel_sst: return -EFAULT if copy_to_user() Dan Carpenter
2010-10-15 4:35 ` [patch 1/2] Staging: intel_sst: return -EFAULT if Koul, Vinod
@ 2010-10-15 5:29 ` Dan Carpenter
2010-10-15 5:51 ` Koul, Vinod
` (3 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Dan Carpenter @ 2010-10-15 5:29 UTC (permalink / raw)
To: kernel-janitors
On Fri, Oct 15, 2010 at 09:53:57AM +0530, Koul, Vinod wrote:
> > @@ -938,7 +940,7 @@ long intel_sst_ioctl(struct file *file_ptr, unsigned int cmd,
> > unsigned long arg)
> > retval = copy_to_user((struct snd_sst_vol *)arg,
> > &get_vol, sizeof(get_vol));
> > if (retval) {
> > - retval = -EIO;
> > + retval = -EFAULT;
> > break;
> > }
> > /*sst_print_get_vol_info(str_id, &get_vol);*/
> Since retval is not used and value assigned is supposed to be -EFAULT,
> how about this?
> if(copy_to_user())
> return -EIO;
>
No. No. retval is used. We can't return directly because we need to
call unlock_kernel().
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread* RE: [patch 1/2] Staging: intel_sst: return -EFAULT if
2010-10-15 3:44 [patch 1/2] Staging: intel_sst: return -EFAULT if copy_to_user() Dan Carpenter
2010-10-15 4:35 ` [patch 1/2] Staging: intel_sst: return -EFAULT if Koul, Vinod
2010-10-15 5:29 ` Dan Carpenter
@ 2010-10-15 5:51 ` Koul, Vinod
2010-10-15 12:58 ` Alan Cox
` (2 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Koul, Vinod @ 2010-10-15 5:51 UTC (permalink / raw)
To: kernel-janitors
> > > @@ -938,7 +940,7 @@ long intel_sst_ioctl(struct file *file_ptr, unsigned int
> cmd,
> > > unsigned long arg)
> > > retval = copy_to_user((struct snd_sst_vol *)arg,
> > > &get_vol, sizeof(get_vol));
> > > if (retval) {
> > > - retval = -EIO;
> > > + retval = -EFAULT;
> > > break;
> > > }
> > > /*sst_print_get_vol_info(str_id, &get_vol);*/
> > Since retval is not used and value assigned is supposed to be -EFAULT,
> > how about this?
> > if(copy_to_user())
> > return -EIO;
> >
>
> No. No. retval is used. We can't return directly because we need to
> call unlock_kernel().
Kernel locking is already gone :) [1].
The point here was we don't need to store return value from this function
Anyway you could have done it this way
if(copy_to_user()) {
retval = -EIO;
break;
}
~Vinod
[1]: http://mailman.alsa-project.org/pipermail/alsa-devel/2010-October/032690.html
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [patch 1/2] Staging: intel_sst: return -EFAULT if
2010-10-15 3:44 [patch 1/2] Staging: intel_sst: return -EFAULT if copy_to_user() Dan Carpenter
` (2 preceding siblings ...)
2010-10-15 5:51 ` Koul, Vinod
@ 2010-10-15 12:58 ` Alan Cox
2010-10-15 12:59 ` Alan Cox
2010-10-15 14:24 ` Dan Carpenter
5 siblings, 0 replies; 7+ messages in thread
From: Alan Cox @ 2010-10-15 12:58 UTC (permalink / raw)
To: kernel-janitors
On Fri, 15 Oct 2010 05:44:07 +0200
Dan Carpenter <error27@gmail.com> wrote:
> copy_to_user() returns the number of bytes remaining to be copied but
> we want to return an error code. And that error code is -EFAULT not
> -EIO.
>
> Signed-off-by: Dan Carpenter <error27@gmail.com>
Acked-by: Alan Cox <alan@linux.intel.com>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [patch 1/2] Staging: intel_sst: return -EFAULT if
2010-10-15 3:44 [patch 1/2] Staging: intel_sst: return -EFAULT if copy_to_user() Dan Carpenter
` (3 preceding siblings ...)
2010-10-15 12:58 ` Alan Cox
@ 2010-10-15 12:59 ` Alan Cox
2010-10-15 14:24 ` Dan Carpenter
5 siblings, 0 replies; 7+ messages in thread
From: Alan Cox @ 2010-10-15 12:59 UTC (permalink / raw)
To: kernel-janitors
On Fri, 15 Oct 2010 07:29:01 +0200
Dan Carpenter <error27@gmail.com> wrote:
> On Fri, Oct 15, 2010 at 09:53:57AM +0530, Koul, Vinod wrote:
> > > @@ -938,7 +940,7 @@ long intel_sst_ioctl(struct file *file_ptr,
> > > unsigned int cmd, unsigned long arg)
> > > retval = copy_to_user((struct snd_sst_vol *)arg,
> > > &get_vol,
> > > sizeof(get_vol)); if (retval) {
> > > - retval = -EIO;
> > > + retval = -EFAULT;
> > > break;
> > > }
> > > /*sst_print_get_vol_info(str_id, &get_vol);*/
> > Since retval is not used and value assigned is supposed to be
> > -EFAULT, how about this?
> > if(copy_to_user())
> > return -EIO;
> >
>
> No. No. retval is used. We can't return directly because we need to
> call unlock_kernel().
Dan - your tree is a couple of patches stale at this point if it has
the lock_kernel in it - the lock_kernel has gone in the latest
submission.
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [patch 1/2] Staging: intel_sst: return -EFAULT if
2010-10-15 3:44 [patch 1/2] Staging: intel_sst: return -EFAULT if copy_to_user() Dan Carpenter
` (4 preceding siblings ...)
2010-10-15 12:59 ` Alan Cox
@ 2010-10-15 14:24 ` Dan Carpenter
5 siblings, 0 replies; 7+ messages in thread
From: Dan Carpenter @ 2010-10-15 14:24 UTC (permalink / raw)
To: kernel-janitors
On Fri, Oct 15, 2010 at 01:59:48PM +0100, Alan Cox wrote:
> On Fri, 15 Oct 2010 07:29:01 +0200
> Dan Carpenter <error27@gmail.com> wrote:
>
> > On Fri, Oct 15, 2010 at 09:53:57AM +0530, Koul, Vinod wrote:
> > > > @@ -938,7 +940,7 @@ long intel_sst_ioctl(struct file *file_ptr,
> > > > unsigned int cmd, unsigned long arg)
> > > > retval = copy_to_user((struct snd_sst_vol *)arg,
> > > > &get_vol,
> > > > sizeof(get_vol)); if (retval) {
> > > > - retval = -EIO;
> > > > + retval = -EFAULT;
> > > > break;
> > > > }
> > > > /*sst_print_get_vol_info(str_id, &get_vol);*/
> > > Since retval is not used and value assigned is supposed to be
> > > -EFAULT, how about this?
> > > if(copy_to_user())
> > > return -EIO;
> > >
> >
> > No. No. retval is used. We can't return directly because we need to
> > call unlock_kernel().
>
> Dan - your tree is a couple of patches stale at this point if it has
> the lock_kernel in it - the lock_kernel has gone in the latest
> submission.
Yes. That went in last night. Vinod already told me.
I'll submit another patch which changes the file to the
if (copy_to_user())
retval = -EFAULT;
format. It will apply on top of the patches I sent already.
There are some other bugs I noticed when I wrote that so it's worth it
to clean it up.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread