On 01/29/2018 10:56 AM, Sakari Ailus wrote: > Hi Hans, > > On Fri, Jan 26, 2018 at 01:43:26PM +0100, Hans Verkuil wrote: >> From: Hans Verkuil <hans.verkuil@xxxxxxxxx> >> >> Some ioctls need to copy back the result even if the ioctl returned >> an error. However, don't do this for the error codes -ENOTTY, -EFAULT >> and -ENOIOCTLCMD. It makes no sense in those cases. >> >> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx> > > Acked-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx> > > Shouldn't such a change be made to video_usercopy() as well? Doesn't need > to be in this set though. Good point. I'll add that for v2. This is not actually a bug as such, but it's just weird to copy back results if the ioctl wasn't implemented at all. I realize that I need to drop the -EFAULT check: if you call VIDIOC_G_EXT_CTRLS with an incorrect userspace buffer for the payload, then the control framework will set error_idx to the index of the control with the wrong buffer. So you do need to copy back the data in case of -EFAULT. I can also drop -ENOIOCTLCMD since video_usercopy() converts that to -ENOTTY. Regards, Hans > >> --- >> drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c >> index 790473b45a21..2aa9b43daf60 100644 >> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c >> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c >> @@ -966,6 +966,9 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar >> set_fs(old_fs); >> } >> >> + if (err == -ENOTTY || err == -EFAULT || err == -ENOIOCTLCMD) >> + return err; >> + >> /* Special case: even after an error we need to put the >> results back for these ioctls since the error_idx will >> contain information on which control failed. */ >
