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

From: Arnd Bergmann <arnd@arndb.de>
To: Al Viro <viro@zeniv.linux.org.uk>
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 17:33:51 +0200	[thread overview]
Message-ID: <CAK8P3a2vbLueZ=h6RY74wucH9u2St1z3z62CHUqz9uBs-at7Ew@mail.gmail.com> (raw)
In-Reply-To: <20180914151011.GZ19965@ZenIV.linux.org.uk>

On Fri, Sep 14, 2018 at 5:10 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> 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".

Yes, I understand that distinction, I'm just trying to look at it from
teh perspective of the majority of developers that roughtly understand
what compat mode is but have never deal with arch/s390 and
compat_ptr() conversion but simply try to get stuff working.

> 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...

That helps of course.

> > 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...

Ah right. I suppose the argument list could be changed, or
we could call tty_ldisc_ref()/tty_ldisc_deref() again (not sure
if that's safe when we already hold the reference).

> > +        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.

I was basing it only on statistics: only two commands out of the whole set
take a translation, and the worst case that would happen if we add a new
command without a compat handler is that user space doesn't get what it
wants but we do get the WARN_ON. In comparison, without that check, user
space never gets what it wants for that command (always -ENOTTY) but
no indication that this is a kernel bug.

      Arnd