set_termiox() and the TCGETX handler bail out with -EINVAL immediately
if ->termiox is NULL, but there are no code paths that can set
->termiox to a non-NULL pointer; and no such code paths seem to have
existed since the termiox mechanism was introduced back in
commit 1d65b4a088de ("tty: Add termiox") in v2.6.28.
Similarly, no driver actually implements .set_termiox; and it looks like
no driver ever has.
Delete this dead code; but leave the definition of struct termiox in the
UAPI headers intact.
Signed-off-by: Jann Horn <[email protected]>
---
drivers/tty/tty_ioctl.c | 61 ++------------------------------------
include/linux/tty.h | 1 -
include/linux/tty_driver.h | 9 ------
3 files changed, 2 insertions(+), 69 deletions(-)
diff --git a/drivers/tty/tty_ioctl.c b/drivers/tty/tty_ioctl.c
index e18f318586ab..4de1c6ddb8ff 100644
--- a/drivers/tty/tty_ioctl.c
+++ b/drivers/tty/tty_ioctl.c
@@ -443,51 +443,6 @@ static int get_termio(struct tty_struct *tty, struct termio __user *termio)
return 0;
}
-
-#ifdef TCGETX
-
-/**
- * set_termiox - set termiox fields if possible
- * @tty: terminal
- * @arg: termiox structure from user
- * @opt: option flags for ioctl type
- *
- * Implement the device calling points for the SYS5 termiox ioctl
- * interface in Linux
- */
-
-static int set_termiox(struct tty_struct *tty, void __user *arg, int opt)
-{
- struct termiox tnew;
- struct tty_ldisc *ld;
-
- if (tty->termiox == NULL)
- return -EINVAL;
- if (copy_from_user(&tnew, arg, sizeof(struct termiox)))
- return -EFAULT;
-
- ld = tty_ldisc_ref(tty);
- if (ld != NULL) {
- if ((opt & TERMIOS_FLUSH) && ld->ops->flush_buffer)
- ld->ops->flush_buffer(tty);
- tty_ldisc_deref(ld);
- }
- if (opt & TERMIOS_WAIT) {
- tty_wait_until_sent(tty, 0);
- if (signal_pending(current))
- return -ERESTARTSYS;
- }
-
- down_write(&tty->termios_rwsem);
- if (tty->ops->set_termiox)
- tty->ops->set_termiox(tty, &tnew);
- up_write(&tty->termios_rwsem);
- return 0;
-}
-
-#endif
-
-
#ifdef TIOCGETP
/*
* These are deprecated, but there is limited support..
@@ -815,23 +770,11 @@ int tty_mode_ioctl(struct tty_struct *tty, struct file *file,
return ret;
#endif
#ifdef TCGETX
- case TCGETX: {
- struct termiox ktermx;
- if (real_tty->termiox == NULL)
- return -EINVAL;
- down_read(&real_tty->termios_rwsem);
- memcpy(&ktermx, real_tty->termiox, sizeof(struct termiox));
- up_read(&real_tty->termios_rwsem);
- if (copy_to_user(p, &ktermx, sizeof(struct termiox)))
- ret = -EFAULT;
- return ret;
- }
+ case TCGETX:
case TCSETX:
- return set_termiox(real_tty, p, 0);
case TCSETXW:
- return set_termiox(real_tty, p, TERMIOS_WAIT);
case TCSETXF:
- return set_termiox(real_tty, p, TERMIOS_FLUSH);
+ return -EINVAL;
#endif
case TIOCGSOFTCAR:
copy_termios(real_tty, &kterm);
diff --git a/include/linux/tty.h b/include/linux/tty.h
index a99e9b8e4e31..52f5544bcd85 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -303,7 +303,6 @@ struct tty_struct {
spinlock_t flow_lock;
/* Termios values are protected by the termios rwsem */
struct ktermios termios, termios_locked;
- struct termiox *termiox; /* May be NULL for unsupported */
char name[64];
struct pid *pgrp; /* Protected by ctrl lock */
struct pid *session;
diff --git a/include/linux/tty_driver.h b/include/linux/tty_driver.h
index 358446247ccd..61c3372d3f32 100644
--- a/include/linux/tty_driver.h
+++ b/include/linux/tty_driver.h
@@ -224,14 +224,6 @@
* line). See tty_do_resize() if you need to wrap the standard method
* in your own logic - the usual case.
*
- * void (*set_termiox)(struct tty_struct *tty, struct termiox *new);
- *
- * Called when the device receives a termiox based ioctl. Passes down
- * the requested data from user space. This method will not be invoked
- * unless the tty also has a valid tty->termiox pointer.
- *
- * Optional: Called under the termios lock
- *
* int (*get_icount)(struct tty_struct *tty, struct serial_icounter *icount);
*
* Called when the device receives a TIOCGICOUNT ioctl. Passed a kernel
@@ -285,7 +277,6 @@ struct tty_operations {
int (*tiocmset)(struct tty_struct *tty,
unsigned int set, unsigned int clear);
int (*resize)(struct tty_struct *tty, struct winsize *ws);
- int (*set_termiox)(struct tty_struct *tty, struct termiox *tnew);
int (*get_icount)(struct tty_struct *tty,
struct serial_icounter_struct *icount);
int (*get_serial)(struct tty_struct *tty, struct serial_struct *p);
base-commit: 3bb61aa61828499a7d0f5e560051625fd02ae7e4
--
2.29.2.576.ga3fc446d84-goog
On Thu, Dec 03, 2020 at 03:03:31AM +0100, Jann Horn wrote:
> set_termiox() and the TCGETX handler bail out with -EINVAL immediately
> if ->termiox is NULL, but there are no code paths that can set
> ->termiox to a non-NULL pointer; and no such code paths seem to have
> existed since the termiox mechanism was introduced back in
> commit 1d65b4a088de ("tty: Add termiox") in v2.6.28.
> Similarly, no driver actually implements .set_termiox; and it looks like
> no driver ever has.
>
> Delete this dead code; but leave the definition of struct termiox in the
> UAPI headers intact.
Crazy that no one uses it. Nice cleanup, thanks for doing this.
greg k-h
On 03. 12. 20, 3:03, Jann Horn wrote:
> set_termiox() and the TCGETX handler bail out with -EINVAL immediately
> if ->termiox is NULL, but there are no code paths that can set
> ->termiox to a non-NULL pointer; and no such code paths seem to have
> existed since the termiox mechanism was introduced back in
> commit 1d65b4a088de ("tty: Add termiox") in v2.6.28.
> Similarly, no driver actually implements .set_termiox; and it looks like
> no driver ever has.
Nice!
> Delete this dead code; but leave the definition of struct termiox in the
> UAPI headers intact.
I am thinking -- can/should we mark the structure as deprecated so that
userspace stops using it eventually?
--
js
On Fri, Dec 04, 2020 at 08:22:41AM +0100, Jiri Slaby wrote:
> On 03. 12. 20, 3:03, Jann Horn wrote:
> > set_termiox() and the TCGETX handler bail out with -EINVAL immediately
> > if ->termiox is NULL, but there are no code paths that can set
> > ->termiox to a non-NULL pointer; and no such code paths seem to have
> > existed since the termiox mechanism was introduced back in
> > commit 1d65b4a088de ("tty: Add termiox") in v2.6.28.
> > Similarly, no driver actually implements .set_termiox; and it looks like
> > no driver ever has.
>
> Nice!
>
> > Delete this dead code; but leave the definition of struct termiox in the
> > UAPI headers intact.
>
> I am thinking -- can/should we mark the structure as deprecated so that
> userspace stops using it eventually?
If it doesn't do anything, how can userspace even use it today? :)
On 04. 12. 20, 9:17, Greg Kroah-Hartman wrote:
> On Fri, Dec 04, 2020 at 08:22:41AM +0100, Jiri Slaby wrote:
>> On 03. 12. 20, 3:03, Jann Horn wrote:
>>> set_termiox() and the TCGETX handler bail out with -EINVAL immediately
>>> if ->termiox is NULL, but there are no code paths that can set
>>> ->termiox to a non-NULL pointer; and no such code paths seem to have
>>> existed since the termiox mechanism was introduced back in
>>> commit 1d65b4a088de ("tty: Add termiox") in v2.6.28.
>>> Similarly, no driver actually implements .set_termiox; and it looks like
>>> no driver ever has.
>>
>> Nice!
>>
>>> Delete this dead code; but leave the definition of struct termiox in the
>>> UAPI headers intact.
>>
>> I am thinking -- can/should we mark the structure as deprecated so that
>> userspace stops using it eventually?
>
> If it doesn't do anything, how can userspace even use it today? :)
Well, right. I am in favor to remove it, BUT: what if someone tries that
ioctl and bails out if EINVAL is returned. I mean: if they define a
local var of that struct type and pass it to the ioctl, we would break
the build by removing the struct completely. Even if the code didn't do
anything useful, it still could be built. So is this very potential
breakage OK?
thanks,
--
js
On Fri, Dec 04, 2020 at 09:20:39AM +0100, Jiri Slaby wrote:
> On 04. 12. 20, 9:17, Greg Kroah-Hartman wrote:
> > On Fri, Dec 04, 2020 at 08:22:41AM +0100, Jiri Slaby wrote:
> > > On 03. 12. 20, 3:03, Jann Horn wrote:
> > > > set_termiox() and the TCGETX handler bail out with -EINVAL immediately
> > > > if ->termiox is NULL, but there are no code paths that can set
> > > > ->termiox to a non-NULL pointer; and no such code paths seem to have
> > > > existed since the termiox mechanism was introduced back in
> > > > commit 1d65b4a088de ("tty: Add termiox") in v2.6.28.
> > > > Similarly, no driver actually implements .set_termiox; and it looks like
> > > > no driver ever has.
> > >
> > > Nice!
> > >
> > > > Delete this dead code; but leave the definition of struct termiox in the
> > > > UAPI headers intact.
> > >
> > > I am thinking -- can/should we mark the structure as deprecated so that
> > > userspace stops using it eventually?
> >
> > If it doesn't do anything, how can userspace even use it today? :)
>
> Well, right. I am in favor to remove it, BUT: what if someone tries that
> ioctl and bails out if EINVAL is returned. I mean: if they define a local
> var of that struct type and pass it to the ioctl, we would break the build
> by removing the struct completely. Even if the code didn't do anything
> useful, it still could be built. So is this very potential breakage OK?
I'm sorry, but I don't understand. This is a kernel-internal-only
structure, right? If someone today tries to call these ioctls, they
will get a -EINVAL error as no serial driver in the tree supports them.
If we remove the structure (i.e. what this patch does), and someone
makes an ioctl call, they will still get the same -EINVAL error they did
before.
So nothing has changed as far as userspace can tell.
Now if they have an out-of-tree serial driver that does implement this
call, then yes, they will have problems, but that's not our problem,
that is theirs for not ever submitting their code. We don't support
in-kernel apis with no in-kernel users.
Or am I still confused?
thanks,
greg k-h
On 04. 12. 20, 9:36, Greg Kroah-Hartman wrote:
> On Fri, Dec 04, 2020 at 09:20:39AM +0100, Jiri Slaby wrote:
>> On 04. 12. 20, 9:17, Greg Kroah-Hartman wrote:
>>> On Fri, Dec 04, 2020 at 08:22:41AM +0100, Jiri Slaby wrote:
>>>> On 03. 12. 20, 3:03, Jann Horn wrote:
>>>>> set_termiox() and the TCGETX handler bail out with -EINVAL immediately
>>>>> if ->termiox is NULL, but there are no code paths that can set
>>>>> ->termiox to a non-NULL pointer; and no such code paths seem to have
>>>>> existed since the termiox mechanism was introduced back in
>>>>> commit 1d65b4a088de ("tty: Add termiox") in v2.6.28.
>>>>> Similarly, no driver actually implements .set_termiox; and it looks like
>>>>> no driver ever has.
>>>>
>>>> Nice!
>>>>
>>>>> Delete this dead code; but leave the definition of struct termiox in the
>>>>> UAPI headers intact.
Note this ^^^^^. He is talking about _not_ touching the definition in
the UAPI header. Does the rest below makes more sense now?
>>>> I am thinking -- can/should we mark the structure as deprecated so that
>>>> userspace stops using it eventually?
>>>
>>> If it doesn't do anything, how can userspace even use it today? :)
>>
>> Well, right. I am in favor to remove it, BUT: what if someone tries that
>> ioctl and bails out if EINVAL is returned. I mean: if they define a local
>> var of that struct type and pass it to the ioctl, we would break the build
>> by removing the struct completely. Even if the code didn't do anything
>> useful, it still could be built. So is this very potential breakage OK?
>
> I'm sorry, but I don't understand. This is a kernel-internal-only
> structure, right? If someone today tries to call these ioctls, they
> will get a -EINVAL error as no serial driver in the tree supports them.
>
> If we remove the structure (i.e. what this patch does), and someone
> makes an ioctl call, they will still get the same -EINVAL error they did
> before.
>
> So nothing has changed as far as userspace can tell.
>
> Now if they have an out-of-tree serial driver that does implement this
> call, then yes, they will have problems, but that's not our problem,
> that is theirs for not ever submitting their code. We don't support
> in-kernel apis with no in-kernel users.
>
> Or am I still confused?
>
> thanks,
>
> greg k-h
>
--
js
On Fri, Dec 04, 2020 at 09:51:07AM +0100, Jiri Slaby wrote:
> On 04. 12. 20, 9:36, Greg Kroah-Hartman wrote:
> > On Fri, Dec 04, 2020 at 09:20:39AM +0100, Jiri Slaby wrote:
> > > On 04. 12. 20, 9:17, Greg Kroah-Hartman wrote:
> > > > On Fri, Dec 04, 2020 at 08:22:41AM +0100, Jiri Slaby wrote:
> > > > > On 03. 12. 20, 3:03, Jann Horn wrote:
> > > > > > set_termiox() and the TCGETX handler bail out with -EINVAL immediately
> > > > > > if ->termiox is NULL, but there are no code paths that can set
> > > > > > ->termiox to a non-NULL pointer; and no such code paths seem to have
> > > > > > existed since the termiox mechanism was introduced back in
> > > > > > commit 1d65b4a088de ("tty: Add termiox") in v2.6.28.
> > > > > > Similarly, no driver actually implements .set_termiox; and it looks like
> > > > > > no driver ever has.
> > > > >
> > > > > Nice!
> > > > >
> > > > > > Delete this dead code; but leave the definition of struct termiox in the
> > > > > > UAPI headers intact.
>
> Note this ^^^^^. He is talking about _not_ touching the definition in the
> UAPI header. Does the rest below makes more sense now?
No, I'm still confused :)
We can't touch the UAPI definitions, but the fact that this api never
did anything still is ok as after this patch it continues to not do
anything.
I'm confused as to what you are proposing...
greg k-h
On Fri, Dec 04, 2020 at 10:10:16AM +0100, Greg Kroah-Hartman wrote:
> On Fri, Dec 04, 2020 at 09:51:07AM +0100, Jiri Slaby wrote:
> > > > > On Fri, Dec 04, 2020 at 08:22:41AM +0100, Jiri Slaby wrote:
> > > > > > On 03. 12. 20, 3:03, Jann Horn wrote:
> > > > > > > Delete this dead code; but leave the definition of struct termiox in the
> > > > > > > UAPI headers intact.
[was snipped]
> > > > > > I am thinking -- can/should we mark the structure as deprecated so that
> > > > > > userspace stops using it eventually?
> > Note this ^^^^^. He is talking about _not_ touching the definition in the
> > UAPI header. Does the rest below makes more sense now?
>
> No, I'm still confused :)
>
> We can't touch the UAPI definitions, but the fact that this api never
> did anything still is ok as after this patch it continues to not do
> anything.
>
> I'm confused as to what you are proposing...
The UAPI definition can't be removed, but it would be nice to issue a
compiler _warning_ if it's ever used.
Like eg. __attribute__ ((deprecated))
Meow!
--
⢀⣴⠾⠻⢶⣦⠀ .--[ Makefile ]
⣾⠁⢠⠒⠀⣿⡁ # beware of races
⢿⡄⠘⠷⠚⠋⠀ all: pillage burn
⠈⠳⣄⠀⠀⠀⠀ `----
On Mon, Dec 07, 2020 at 11:19:04AM +0100, Adam Borowski wrote:
> On Fri, Dec 04, 2020 at 10:10:16AM +0100, Greg Kroah-Hartman wrote:
> > On Fri, Dec 04, 2020 at 09:51:07AM +0100, Jiri Slaby wrote:
> > > > > > On Fri, Dec 04, 2020 at 08:22:41AM +0100, Jiri Slaby wrote:
> > > > > > > On 03. 12. 20, 3:03, Jann Horn wrote:
> > > > > > > > Delete this dead code; but leave the definition of struct termiox in the
> > > > > > > > UAPI headers intact.
> [was snipped]
> > > > > > > I am thinking -- can/should we mark the structure as deprecated so that
> > > > > > > userspace stops using it eventually?
>
> > > Note this ^^^^^. He is talking about _not_ touching the definition in the
> > > UAPI header. Does the rest below makes more sense now?
> >
> > No, I'm still confused :)
> >
> > We can't touch the UAPI definitions, but the fact that this api never
> > did anything still is ok as after this patch it continues to not do
> > anything.
> >
> > I'm confused as to what you are proposing...
>
> The UAPI definition can't be removed, but it would be nice to issue a
> compiler _warning_ if it's ever used.
>
> Like eg. __attribute__ ((deprecated))
Don't add build warnings for no good reasons, that's not nice. As the
feature just doesn't work, anyone who tries to use it will very quickly
realize that :)
thanks,
greg k-h
On Fri, Dec 04, 2020 at 09:20:39AM +0100, Jiri Slaby wrote:
> > > > Delete this dead code; but leave the definition of struct termiox in the
> > > > UAPI headers intact.
> > >
> > > I am thinking -- can/should we mark the structure as deprecated so that
> > > userspace stops using it eventually?
> >
> > If it doesn't do anything, how can userspace even use it today? :)
>
> Well, right. I am in favor to remove it, BUT: what if someone tries that
> ioctl and bails out if EINVAL is returned. I mean: if they define a local
> var of that struct type and pass it to the ioctl, we would break the build
> by removing the struct completely. Even if the code didn't do anything
> useful, it still could be built. So is this very potential breakage OK?
Um, we do guarantee a stable ABI. We have never guaranteed that all old
crappy code will continue to compile, although we avoid gratious
breakage. And assuming there ever was code using termiox (which I'm not
sure about to start with) it will surely have some form of feature
check, and I think we are better off with that feature check not
detecting the presence as that would be completely pointless.
Or in short: by keeping the uapi definition we do userspace software a
disfavor.
On 08. 12. 20, 12:13, Christoph Hellwig wrote:
> On Fri, Dec 04, 2020 at 09:20:39AM +0100, Jiri Slaby wrote:
>>>>> Delete this dead code; but leave the definition of struct termiox in the
>>>>> UAPI headers intact.
>>>>
>>>> I am thinking -- can/should we mark the structure as deprecated so that
>>>> userspace stops using it eventually?
>>>
>>> If it doesn't do anything, how can userspace even use it today? :)
>>
>> Well, right. I am in favor to remove it, BUT: what if someone tries that
>> ioctl and bails out if EINVAL is returned. I mean: if they define a local
>> var of that struct type and pass it to the ioctl, we would break the build
>> by removing the struct completely. Even if the code didn't do anything
>> useful, it still could be built. So is this very potential breakage OK?
>
> Um, we do guarantee a stable ABI. We have never guaranteed that all old
> crappy code will continue to compile, although we avoid gratious
> breakage. And assuming there ever was code using termiox (which I'm not
> sure about to start with) it will surely have some form of feature
> check, and I think we are better off with that feature check not
> detecting the presence as that would be completely pointless.
>
> Or in short: by keeping the uapi definition we do userspace software a
> disfavor.
OK, even better. I will remove it once I get to it (if noone beats me to
it, of course).
thanks,
--
js
suse labs