Re: fs, tty: WARNING in devpts_get_priv

[Dmitry Vyukov]

Hello,

The following program triggers WARNING in devpts_get_priv:

// autogenerated by syzkaller (http://github.com/google/syzkaller)
#include <unistd.h>
#include <fcntl.h>
#include <sys/stat.h>
#include <sys/types.h>

int main()
{
mknodat(AT_FDCWD, "./bus", S_IFCHR, makedev(136, 49));
openat(AT_FDCWD, "./bus", O_RDONLY);
return 0;
}


WARNING: CPU: 0 PID: 9491 at fs/devpts/inode.c:588 devpts_get_priv+0xb9/0xe0
CPU: 0 PID: 9491 Comm: syz-executor Not tainted 4.8.0-rc3-next-20160825+ #8
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
ffffffff886b6fe0 ffff880033917768 ffffffff82db38d9 ffffffff021d0000
fffffbfff10d6dfc ffffffff8708f000 ffff880033917840 ffffffff8715e100
dffffc0000000000 0000000000000009 ffff880033917830 ffffffff816cf888
Call Trace:
[< inline >] __dump_stack lib/dump_stack.c:15
[<ffffffff82db38d9>] dump_stack+0x12e/0x185 lib/dump_stack.c:51
[<ffffffff816cf888>] panic+0x1e4/0x3fb kernel/panic.c:179
[<ffffffff813863b4>] __warn+0x1c4/0x1e0 kernel/panic.c:542
[<ffffffff8138659c>] warn_slowpath_null+0x2c/0x40 kernel/panic.c:585
[<ffffffff81a2aff9>] devpts_get_priv+0xb9/0xe0 fs/devpts/inode.c:588
[<ffffffff83241c4f>] pts_unix98_lookup+0x3f/0x70 drivers/tty/pty.c:648
[< inline >] tty_driver_lookup_tty drivers/tty/tty_io.c:1371
[< inline >] tty_open_by_driver drivers/tty/tty_io.c:2038
[<ffffffff83228171>] tty_open+0x871/0x10a0 drivers/tty/tty_io.c:2108
[<ffffffff8187020e>] chrdev_open+0x22e/0x5c0 fs/char_dev.c:392
[<ffffffff8185ad25>] do_dentry_open+0x6f5/0xd30 fs/open.c:747
[<ffffffff8185e445>] vfs_open+0x105/0x220 fs/open.c:860
[< inline >] do_last fs/namei.c:3374
[<ffffffff81895909>] path_openat+0x12f9/0x2ab0 fs/namei.c:3497
[<ffffffff8189a7ac>] do_filp_open+0x18c/0x250 fs/namei.c:3532
[<ffffffff8185ed4c>] do_sys_open+0x1fc/0x440 fs/open.c:1047
[< inline >] SYSC_openat fs/open.c:1074
[<ffffffff8185f000>] SyS_openat+0x30/0x40 fs/open.c:1068
[<ffffffff86e10700>] entry_SYSCALL_64_fastpath+0x23/0xc1


On 0f98f121e1670eaa2a2fbb675e07d6ba7f0e146f of linux-next.

[Eric W. Biederman]

Dmitry Vyukov <dvy...@google.com> writes:

> Hello,
>
> The following program triggers WARNING in devpts_get_priv:

Unless I am missing part of the history this problem exists on 4.6 and
up.

Yes it looks like we need to stop supporting ptys that are not
on /dev/pts.

The fix is possibly as simple as:

diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index 51e0d32883ba..704c5cd548b6 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -852,8 +852,10 @@ static void __init unix98_pty_init(void)

if (tty_register_driver(ptm_driver))
panic("Couldn't register Unix98 ptm driver");
+#if 0
if (tty_register_driver(pts_driver))
panic("Couldn't register Unix98 pts driver");
+#endif

/* Now create the /dev/ptmx special device */
tty_default_fops(&ptmx_fops);

Or possibly we don't want to do:
tty_set_operations(pts_driver, &pty_unix98_ops);
But instead do:
tty_set_operations(pts_driver, &hung_up_tty_fops);

And in fs/devpts/inode force the use of pty_unix98_ops on the
inodes.

Opinions?

Eric

[Linus Torvalds]

On Sat, Sep 3, 2016 at 9:40 AM, Eric W. Biederman <ebie...@xmission.com> wrote:
>
> Yes it looks like we need to stop supporting ptys that are not
> on /dev/pts.

We never have.

The warning was added to see if that case actually ever triggers, but
it didn't use to work before that either: we *used* to just return
NULL without warning at all (and I actually expected d_fsdata to be
NULL normally, but that's not necessarily true for all filesystems).

So replacing the warning with a "return NULL" will get the old behavior.

And I don't think your patch actually works anyway (did you test it? I
haven't, but I *think* it is broken), because the way the devpts pts
nodes work is that they do

init_special_inode(inode, S_IFCHR|opts->mode,
MKDEV(UNIX98_PTY_SLAVE_MAJOR, index));

and then they rely on the vfs layer filling things in and then the tty
layer to do the lookup. So even the real devpts pty entries do rely on
that "tty_register_driver(pts_driver)", I think.

Which is all silly, of course: we now have the inode and the dentry,
so we *could* just have made the inode open op go directly to the pts
open, rather than indirectly through both the vfs character devices
*and* the tty layer open code.

But that would be a much bigger change. Probably not worth it.

So I think the correct thing to do for now is to just replace the
warning with a return NULL. Like we used to. So something like

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index d116453b0276..79a5941c2474 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -585,7 +585,8 @@ struct dentry *devpts_pty_new(struct pts_fs_info
*fsi, int index, void *priv)
*/
void *devpts_get_priv(struct dentry *dentry)
{
- WARN_ON_ONCE(dentry->d_sb->s_magic != DEVPTS_SUPER_MAGIC);
+ if (dentry->d_sb->s_magic != DEVPTS_SUPER_MAGIC)
+ return NULL;
return dentry->d_fsdata;
}

and mark it for stable.

Comments?

Linus

[Linus Torvalds]

On Sat, Sep 3, 2016 at 10:58 AM, Linus Torvalds
<torv...@linux-foundation.org> wrote:
>
> So I think the correct thing to do for now is to just replace the
> warning with a return NULL. Like we used to. So something like

Ok, I did that and tested it just to be sure, so it's in my tree now.

Linus