Re: <Query> Looking more details and reasons for using orig_add_limit.

[Laurent Pinchart]

Hi Mauro,

On Wednesday 22 Feb 2017 17:25:41 Mauro Carvalho Chehab wrote:
> Em Wed, 22 Feb 2017 21:53:08 +0200 Laurent Pinchart escreveu:
> > On Tuesday 21 Feb 2017 06:20:58 Sodagudi Prasad wrote:
> >> Hi mchehab/linux-media,
> >> 
> >> It is not clear why KERNEL_DS was set explicitly here. In this path
> >> video_usercopy() gets  called  and it
> >> copies the âstruct v4l2_bufferâ struct to user space stack memory.
> >> 
> >> Can you please share reasons for setting to KERNEL_DS here?
> > 
> > It's a bit of historical hack. To implement compat ioctl handling, we copy
> > the ioctl 32-bit argument from userspace, turn it into a native 64-bit
> > ioctl argument, and call the native ioctl code. That code expects the
> > argument to be stored in userspace memory and uses get_user() and
> > put_user() to access it. As the 64-bit argument now lives in kernel
> > memory, my understanding is that we fake things up with KERNEL_DS.
> 
> Precisely. Actually, if I remember well, this was needed to pass pointer
> arguments from 32 bits userspace to 64 bits kernelspace. There are a lot of
> V4L2 ioctls that pass structures with pointers on it. Setting DS cause
> those pointers to do the right thing, but yeah, it is hackish.

We should restructure the core ioctl code to decouple copy from/to user and 
ioctl execution (this might just be a matter of exporting a currently static 
function), and change the compat code to perform the copy/from to user 
directly when converting between 32-bit and 64-bit structures (dropping all 
the alloc in userspace hacks) and call the ioctl execution handler. That will 
fix the problem. Any volunteer ? :-)

> This used to work fine on x86_64 (when such code was written e. g. Kernel
> 2.6.1x). I never tested myself on ARM64, but I guess it used to work, as we
> received some patches fixing support for some ioctl compat code due to
> x86_64/arm64 differences in the past.
> 
> On what Kernel version it started to cause troubles? 4.9? If so, then
> maybe the breakage is a side effect of VM stack changes.
> 
> > The ioctl code should be refactored to get rid of this hack.
> 
> Agreed.

-- 
Regards,

Laurent Pinchart