Re: [PATCHES] tty ioctls cleanups, compat and not only

From: Al Viro <viro@ZenIV.linux.org.uk>
To: Arnd Bergmann <arnd@arndb.de>
Cc: David Miller <davem@davemloft.net>,
	gregkh <gregkh@linuxfoundation.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCHES] tty ioctls cleanups, compat and not only
Date: Fri, 14 Sep 2018 16:10:11 +0100	[thread overview]
Message-ID: <20180914151011.GZ19965@ZenIV.linux.org.uk> (raw)
In-Reply-To: <CAK8P3a0wN0uMZdj-mxXwbN9AnZjHzMU6_b01H5y+Vpdt+UoeAw@mail.gmail.com>

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.