Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1586576

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

Show all headers | View raw

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

Back to linux.kernel | Previous | Next — Previous in thread | Find similar | Unroll thread

Thread

<Query> Looking more details and reasons for using orig_add_limit. Sodagudi Prasad <psodagud@codeaurora.org> - 2017-02-15 07:00 +0100
  Re: <Query> Looking more details and reasons for using  orig_add_limit. Will Deacon <will.deacon@arm.com> - 2017-02-15 12:40 +0100
  Re: <Query> Looking more details and reasons for using orig_add_limit. James Morse <james.morse@arm.com> - 2017-02-15 13:20 +0100
    Re: <Query> Looking more details and reasons for using  orig_add_limit. Sodagudi Prasad <psodagud@codeaurora.org> - 2017-02-15 22:20 +0100
      Re: <Query> Looking more details and reasons for using orig_add_limit. James Morse <james.morse@arm.com> - 2017-02-16 11:40 +0100
        Re: <Query> Looking more details and reasons for using  orig_add_limit. Sodagudi Prasad <psodagud@codeaurora.org> - 2017-02-21 15:30 +0100
          Re: <Query> Looking more details and reasons for using  orig_add_limit. Mauro Carvalho Chehab <mchehab@s-opensource.com> - 2017-02-22 21:00 +0100
          Re: <Query> Looking more details and reasons for using orig_add_limit. Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2017-02-22 21:00 +0100
            Re: <Query> Looking more details and reasons for using  orig_add_limit. Mauro Carvalho Chehab <mchehab@s-opensource.com> - 2017-02-22 21:40 +0100
              Re: <Query> Looking more details and reasons for using orig_add_limit. Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2017-02-23 01:30 +0100

csiph-web