On Fri, Sep 14, 2018 at 10:21:53AM +0200, Arnd Bergmann wrote:
> This does sound very appealing, but there is a small downside:
> The difference between ".compat_ioctl = NULL" and
> ".compat_ioctl=native_ioctl" is now very subtle, and I wouldn't
> necessarily expect casual readers to understand that.
???
One is "all non-generics take pointers to wordsize-neutral objects",
another "all non-generics take integers".
That solution certainly needs to be documented more than just in commit
message, though; IMO the method descriptions next to declaration are the
best place for that. Will update...
> If we go with my file_operations patch for generic_compat_ioctl_ptrarg
> and add generic_compat_ioctl_intarg, we can do the same thing here
> with ldisc_compat_ioctl_ptrarg/ldisc_compat_ioctl_intarg to make it
> a little more consistent with fops and self-documenting.
No, we can't - ldisc ->ioctl() (or ->compat_ioctl()) doesn't get ldisc
in arguments. Besides, indirect calls are costly these days...
> + if (retval == -ENOIOCTLCMD && _IOC_TYPE(cmd) == 'T') {
> + retval = tty_ioctl(file, cmd, (unsigned
> long)compat_ptr(arg));
> + WARN_ON_ONCE(retval != -ENOIOCTLCMD && retval != -ENOTTY);
> + }
>
> Seeing that you list every single 'T' command in tty_compat_ioctl()
> that we handle in the native case, that WARN_ON_ONCE should not
> trigger for any input, but it would catch (and warn about) any of those
> that might get added in the future to the native code path without the
> compat entry.
Anything adding new ioctls needs careful review anyway. And blind bets upon
that stuff taking compat pointer are, AFAICS, completely unfounded.
