Groups > linux.kernel > #1713144 > unrolled thread

[PATCH 0/1] devpts: use dynamic_dname() to generate proc name

Started by	Christian Brauner <christian.brauner@ubuntu.com>
First post	2017-08-16 19:20 +0200
Last post	2017-08-17 03:40 +0200
Articles	20 on this page of 54 — 5 participants

Back to article view | Back to linux.kernel

  [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Christian Brauner <christian.brauner@ubuntu.com> - 2017-08-16 19:20 +0200
    Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-16 20:30 +0200
      Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-16 20:50 +0200
        Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Christian Brauner <christian.brauner@canonical.com> - 2017-08-16 21:50 +0200
          Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-16 22:00 +0200
            Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-16 22:20 +0200
              Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-16 22:40 +0200
                Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-16 23:10 +0200
                  Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Christian Brauner <christian.brauner@canonical.com> - 2017-08-16 23:40 +0200
                    Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-16 23:50 +0200
                      Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-17 00:00 +0200
                        Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Christian Brauner <christian.brauner@canonical.com> - 2017-08-17 00:10 +0200
                      Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Christian Brauner <christian.brauner@canonical.com> - 2017-08-17 00:30 +0200
                        Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name ebiederm@xmission.com (Eric W. Biederman) - 2017-08-23 17:40 +0200
                          Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Christian Brauner <christian.brauner@canonical.com> - 2017-08-23 23:20 +0200
                  Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name ebiederm@xmission.com (Eric W. Biederman) - 2017-08-17 00:50 +0200
                    Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-17 01:00 +0200
                      Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name ebiederm@xmission.com (Eric W. Biederman) - 2017-08-17 02:00 +0200
                        Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-17 02:10 +0200
                          Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name ebiederm@xmission.com (Eric W. Biederman) - 2017-08-17 03:30 +0200
                          Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Stefan Lippers-Hollmann <s.l-h@gmx.de> - 2017-08-24 02:30 +0200
                            Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-24 02:50 +0200
                              Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-24 03:20 +0200
                              Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name ebiederm@xmission.com (Eric W. Biederman) - 2017-08-24 03:30 +0200
                                Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-24 03:40 +0200
                                  Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-24 03:50 +0200
                                    Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-24 04:10 +0200
                                      Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name ebiederm@xmission.com (Eric W. Biederman) - 2017-08-24 05:20 +0200
                                        Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-24 05:30 +0200
                                          Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name ebiederm@xmission.com (Eric W. Biederman) - 2017-08-24 18:00 +0200
                                        Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Stefan Lippers-Hollmann <s.l-h@gmx.de> - 2017-08-24 06:30 +0200
                                          Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name ebiederm@xmission.com (Eric W. Biederman) - 2017-08-24 18:00 +0200
                                            Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-24 20:00 +0200
                                              Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-24 20:10 +0200
                                                Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-24 20:20 +0200
                                                  Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-24 20:40 +0200
                                                    Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-24 20:40 +0200
                                                      Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Stefan Lippers-Hollmann <s.l-h@gmx.de> - 2017-08-24 22:30 +0200
                                                        Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-24 22:30 +0200
                                                  Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name ebiederm@xmission.com (Eric W. Biederman) - 2017-08-24 20:50 +0200
                                                    Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-24 21:00 +0200
                                                      Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name ebiederm@xmission.com (Eric W. Biederman) - 2017-08-24 21:30 +0200
                                                      [PATCH v3] pty: Repair TIOCGPTPEER ebiederm@xmission.com (Eric W. Biederman) - 2017-08-24 22:20 +0200
                                                        Re: [PATCH v3] pty: Repair TIOCGPTPEER Stefan Lippers-Hollmann <s.l-h@gmx.de> - 2017-08-24 23:10 +0200
                                                    Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-24 21:30 +0200
                                                      Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-24 21:30 +0200
                                                      Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name ebiederm@xmission.com (Eric W. Biederman) - 2017-08-24 22:50 +0200
                                                        Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-24 23:10 +0200
                                                          Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name ebiederm@xmission.com (Eric W. Biederman) - 2017-08-25 01:10 +0200
                                                            Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-25 01:30 +0200
                                                            Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Christian Brauner <christian.brauner@canonical.com> - 2017-08-25 01:40 +0200
                                                              Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-26 03:10 +0200
                                                    Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name ebiederm@xmission.com (Eric W. Biederman) - 2017-08-24 21:50 +0200
              Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name ebiederm@xmission.com (Eric W. Biederman) - 2017-08-17 03:40 +0200

Page 2 of 3 — ← Prev page 1 [2] 3 Next page →

#1718733

From	Stefan Lippers-Hollmann <s.l-h@gmx.de>
Date	2017-08-24 02:30 +0200
Message-ID	<uhOc1-7y2-3@gated-at.bofh.it>
In reply to	#1713434

[Multipart message — attachments visible in raw view] — view raw

Hi

On 2017-08-16, Linus Torvalds wrote:
> On Wed, Aug 16, 2017 at 4:51 PM, Eric W. Biederman
> <ebiederm@xmission.com> wrote:
[...]
> Maybe this attached patch is better anyway. It's smaller, because it
> keeps more closely to the old code, and just adds a mntput() in all
> the exit cases, and depends on the "path_get()" to have incremented
> the mnt refcount one extra time.
> 
> Can you find something in this one?
> 
> ENTIRELY UNTESTED!

This patch[1] as part of 4.13-rc6 (up to, at least, 
v4.13-rc6-45-g6470812e2226) introduces a regression for me when using
pbuilder 0.228.7[2] (a helper to build Debian packages in a chroot and
to create and update its chroots) when trying to umount /dev/ptmx (inside
the chroot) on Debian/ unstable (full log and pbuilder configuration 
file[3] attached).

[...]
Setting up build-essential (12.3) ...
Processing triggers for libc-bin (2.24-15) ...
I: unmounting dev/ptmx filesystem
W: Could not unmount dev/ptmx: umount: /var/cache/pbuilder/build/1340/dev/ptmx: target is busy
        (In some cases useful info about processes that
         use the device is found by lsof(8) or fuser(1).)
W: Retrying to unmount dev/ptmx in 5s
umount: /var/cache/pbuilder/build/1340/dev/ptmx: target is busy
        (In some cases useful info about processes that
         use the device is found by lsof(8) or fuser(1).)

  Could not unmount dev/ptmx, some programs might
  still be using files in /proc (klogd?).
  Please check and kill these processes manually
  so that I can unmount dev/ptmx.  Last umount error was:
umount: /var/cache/pbuilder/build/1340/dev/ptmx: target is busy
        (In some cases useful info about processes that
         use the device is found by lsof(8) or fuser(1).)
[...]

lsof isn't revealing (but this might point towards gvfs 1.30.4-1+b1 
involvement), fuser -k doesn't release the ressource.

Kernel v4.13-rc5 and before (at least 4.11.x and 4.12.x) are not
affected, but this problem is reliably reproducible on three different 
x86_64 systems running Debian/ unstable when the host is running a 
kernel >=4.6.13-rc6. Unfortunately I haven't really found an easier/
smaller way to reproduce this issue yet, but creating a new build
chroot[4] always triggers this problem, updating an existing build
chroot[5] (which also mounts and umounts /dev/ptmx) triggers most of 
the time, but not reliably - building a package (e.g. the kernel, also 
mounting and umounting /dev/ptmx) also triggered this issue at least 
once (but I didn't try this more often).

My git bisection log is this:

$ git bisect log
git bisect start
# good: [ef954844c7ace62f773f4f23e28d2d915adc419f] Linux 4.13-rc5
git bisect good ef954844c7ace62f773f4f23e28d2d915adc419f
# bad: [14ccee78fc82f5512908f4424f541549a5705b89] Linux 4.13-rc6
git bisect bad 14ccee78fc82f5512908f4424f541549a5705b89
# bad: [cb247857f3dae0bdb843362c35027a0066b963a4] Merge tag 'sound-4.13-rc6' of git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound
git bisect bad cb247857f3dae0bdb843362c35027a0066b963a4
# good: [88a5c690b66110ad255380d8f629c629cf6ca559] bpf: fix bpf_trace_printk on 32 bit archs
git bisect good 88a5c690b66110ad255380d8f629c629cf6ca559
# bad: [3bc6c906eacec34f0d8dcfd3c7e4513edf152297] Merge branch 'parisc-4.13-5' of git://git.kernel.org/pub/scm/linux/kernel/git/deller/parisc-linux
git bisect bad 3bc6c906eacec34f0d8dcfd3c7e4513edf152297
# good: [40c6d1b9e2fc4251ca19fa69398f6fa34e813e27] Merge tag 'linux-kselftest-4.13-rc6-fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest
git bisect good 40c6d1b9e2fc4251ca19fa69398f6fa34e813e27
# good: [ac9a40905a610fb02086a37b11ff4bf046825a88] Merge tag 'scsi-fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi
git bisect good ac9a40905a610fb02086a37b11ff4bf046825a88
# bad: [99f781b1bfc199ec8eb86d4e015920faf79d5d57] Merge branch 'for_linus' of git://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs
git bisect bad 99f781b1bfc199ec8eb86d4e015920faf79d5d57
# good: [41e327b586762833e48b3703d53312ac32f05f24] quota: correct space limit check
git bisect good 41e327b586762833e48b3703d53312ac32f05f24

Reverting just c8c03f1858331e85d397bacccd34ef409aae993c from
v4.13-rc6-65-g2acf097f16ab reliably fixes the problem for me.

Regards
	Stefan Lippers-Hollmann

[1]	commit c8c03f1858331e85d397bacccd34ef409aae993c (HEAD)
	Author: Linus Torvalds <torvalds@linux-foundation.org>
	Date:   Wed Aug 16 17:08:07 2017 -0700
	Subject: pty: fix the cached path of the pty slave file 
	              descriptor in the master
[2]	https://packages.qa.debian.org/p/pbuilder.html
	https://pbuilder.alioth.debian.org/
	https://anonscm.debian.org/git/pbuilder/pbuilder.git
	mounting/ umounting /dev/ptmx happens in
	https://anonscm.debian.org/git/pbuilder/pbuilder.git/tree/pbuilder-modules
[3]	the configuration file as attached relies on BUILDUSERNAME="pbuilder"
	and BUILDUSERID="$(getent passwd $BUILDUSERNAME | cut -d\: -f3)",
	commenting those out should be possible, my full BUILDUSER* setup
	is:
	# addgroup --system pbuilder
	# adduser --system --disabled-password --ingroup pbuilder --gecos "pbuilder buildd user" --home /var/run/pbuilder pbuilder
	# chown pbuilder:pbuilder /var/cache/pbuilder/deps /var/cache/pbuilder/build-result
	# chmod 2775 /var/cache/pbuilder/deps /var/cache/pbuilder/build-result
[4]	sudo /usr/sbin/pbuilder create --configfile pbuilderrc.debian.sid.amd64
[5]	sudo /usr/sbin/pbuilder update --configfile pbuilderrc.debian.sid.amd64

[toc] | [prev] | [next] | [standalone]

#1718736

From	Linus Torvalds <torvalds@linux-foundation.org>
Date	2017-08-24 02:50 +0200
Message-ID	<uhOvn-7F7-3@gated-at.bofh.it>
In reply to	#1718733

On Wed, Aug 23, 2017 at 5:24 PM, Stefan Lippers-Hollmann <s.l-h@gmx.de> wrote:
>
> This patch[1] as part of 4.13-rc6 (up to, at least,
> v4.13-rc6-45-g6470812e2226) introduces a regression for me when using
> pbuilder 0.228.7[2] (a helper to build Debian packages in a chroot and
> to create and update its chroots) when trying to umount /dev/ptmx (inside
> the chroot) on Debian/ unstable (full log and pbuilder configuration
> file[3] attached).
>
> [...]
> Setting up build-essential (12.3) ...
> Processing triggers for libc-bin (2.24-15) ...
> I: unmounting dev/ptmx filesystem
> W: Could not unmount dev/ptmx: umount: /var/cache/pbuilder/build/1340/dev/ptmx: target is busy

Yes, that patch definitely keeps a reference to the pts filesystem
around while a pty is open.

We always used to do that, but we did it differently - we would keep
the 's_active' count elevated so that the superblock never went away,
even after it was unmounted.

Now it does an actual mntget(), and that makes umount _notice_ that
the filesystem is still busy.

How annoying.

Because in a very real sehse the filesystem really is busy, but we
used to hide it (perhaps on purpose - it's possible that people hit
this problem before).

Let me try to think about alteratives. Clearly this is a regression
and I need to fix it, I just need to figure out _how_.

                   Linus

[toc] | [prev] | [next] | [standalone]

#1718748

From	Linus Torvalds <torvalds@linux-foundation.org>
Date	2017-08-24 03:20 +0200
Message-ID	<uhOYp-84n-5@gated-at.bofh.it>
In reply to	#1718736

On Wed, Aug 23, 2017 at 5:42 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Let me try to think about alteratives. Clearly this is a regression
> and I need to fix it, I just need to figure out _how_.

Ok, sadly, I think it's unfixable with the current model.

We literally used to keep the wrong 'struct path' around, and sadly,
fixing the struct path to point to the right vfsmount fundamentally
means that we'd be keeping the mount count elevated for that pts
mount.

And that fundamentally means that umount() will return -EBUSY. There's
no way around it.

So I think I will have to just revert that fix.

Damn.

Now, I think there's a way forward: get rid of the 'struct path'
(which is bogus anyway), and only remember the pts denty.

Then, at TIOCGPTPEER time (which is why we currently have that 'struct
path' anyway), look up the right 'vfsmount' by looking up the 'pts'
path again.

That's a rather bigger patch than the one I'll have to revert, I'm afraid ;(

                 Linus

[toc] | [prev] | [next] | [standalone]

#1718753

From	ebiederm@xmission.com (Eric W. Biederman)
Date	2017-08-24 03:30 +0200
Message-ID	<uhP85-87D-11@gated-at.bofh.it>
In reply to	#1718736

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Wed, Aug 23, 2017 at 5:24 PM, Stefan Lippers-Hollmann <s.l-h@gmx.de> wrote:
>>
>> This patch[1] as part of 4.13-rc6 (up to, at least,
>> v4.13-rc6-45-g6470812e2226) introduces a regression for me when using
>> pbuilder 0.228.7[2] (a helper to build Debian packages in a chroot and
>> to create and update its chroots) when trying to umount /dev/ptmx (inside
>> the chroot) on Debian/ unstable (full log and pbuilder configuration
>> file[3] attached).
>>
>> [...]
>> Setting up build-essential (12.3) ...
>> Processing triggers for libc-bin (2.24-15) ...
>> I: unmounting dev/ptmx filesystem
>> W: Could not unmount dev/ptmx: umount: /var/cache/pbuilder/build/1340/dev/ptmx: target is busy
>
> Yes, that patch definitely keeps a reference to the pts filesystem
> around while a pty is open.
>
> We always used to do that, but we did it differently - we would keep
> the 's_active' count elevated so that the superblock never went away,
> even after it was unmounted.
>
> Now it does an actual mntget(), and that makes umount _notice_ that
> the filesystem is still busy.
>
> How annoying.
>
> Because in a very real sehse the filesystem really is busy, but we
> used to hide it (perhaps on purpose - it's possible that people hit
> this problem before).
>
> Let me try to think about alteratives. Clearly this is a regression
> and I need to fix it, I just need to figure out _how_.

The new behavior is that when we open ptmx we cache a path the slave
pty.

If instead of caching that path we call devpts_acquire to compute the
mount point of the dentry we should be able to skip caching mountpoint
in ptmx_open.

That should trivially remove the regression.

We will have to fail if someone crazy unmounted the devpts filesystem
before we ask for the peer file descriptor.  But otherwise the behavior
should be exactly the same.

Eric

[toc] | [prev] | [next] | [standalone]

#1718755

From	Linus Torvalds <torvalds@linux-foundation.org>
Date	2017-08-24 03:40 +0200
Message-ID	<uhPhL-8aO-9@gated-at.bofh.it>
In reply to	#1718753

On Wed, Aug 23, 2017 at 6:25 PM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
>
> The new behavior is that when we open ptmx we cache a path the slave
> pty.

Yes. It's not strictly "new", though - we've done that for a while,
and if you used /dev/pts/ptmx you'd even have had the *right* path
for a while ;)

And this exact issue that Stefan is reporting.

But nobody ever used /dev/pts/ptmx, so nobody got the right path, and
nobody kept an extra reference to the pts mount.

> If instead of caching that path we call devpts_acquire to compute the
> mount point of the dentry we should be able to skip caching mountpoint
> in ptmx_open.

Yes, that's my plan - get rid of the 'struct path' entirely, make
'driver_data' point to just the dentry, and then at TIOCGPTPEER time
just re-create the path by looking up the vfsmount again (by doingf
that "pts" lookup again)

It should all be _fairly_ straightforward, but it's definitely a
rather bigger change than that "just fix the path" patch was.

Anyway, it's already reverted in my tree, I'll push it out after I've
verified that there isn't some silly build issue (there won't be, but
I've been burned by "this is obviously correct" too many times, so now
I always build before pushing anything out unless I'm on my laptop of
something when it's just too inconvenient).

                      Linus

[toc] | [prev] | [next] | [standalone]

#1718768

From	Linus Torvalds <torvalds@linux-foundation.org>
Date	2017-08-24 03:50 +0200
Message-ID	<uhPrs-8ei-47@gated-at.bofh.it>
In reply to	#1718755

On Wed, Aug 23, 2017 at 6:32 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> It should all be _fairly_ straightforward, but it's definitely a
> rather bigger change than that "just fix the path" patch was.

Argh. And it's *not* fairly straightforward, because the
tty_operations "ioctl()" function pointer only gets 'struct tty *'.

So in the TIOCGPTPEER path, we don't actually have access to the file
pointer of the fd we're doing the ioctl on.

And that's where the 'struct path' to the 'ptmx' node is - which we
need to then look up the 'pts' directory.

How very annoying. I think that's why we did it all at ptmx_open()
time, because then we had all the information.

                Linus

[toc] | [prev] | [next] | [standalone]

#1718775

From	Linus Torvalds <torvalds@linux-foundation.org>
Date	2017-08-24 04:10 +0200
Message-ID	<uhPKO-aL-9@gated-at.bofh.it>
In reply to	#1718768

On Wed, Aug 23, 2017 at 6:49 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Argh. And it's *not* fairly straightforward, because the
> tty_operations "ioctl()" function pointer only gets 'struct tty *'.
>
> So in the TIOCGPTPEER path, we don't actually have access to the file
> pointer of the fd we're doing the ioctl on.
>
> And that's where the 'struct path' to the 'ptmx' node is - which we
> need to then look up the 'pts' directory.
>
> How very annoying. I think that's why we did it all at ptmx_open()
> time, because then we had all the information.

Anyway, the revert is pushed out. So we're back to the old behavior
that gives the wrong pathname in /proc.

And I think I can handle the lack of a 'struct file *' to the ioctl
operations by just special-casing TIOCGPTPEER directly in tty_ioctl()
itself.

That's where we handle "generic" tty ioctls, and doing pty stuff there
is kind of wrong, but pty's are special.

But I think I'll leave it for tomorrow. So Eric, if you feel like
looking at this, I'd appreciate it.

                Linus

[toc] | [prev] | [next] | [standalone]

#1718795

From	ebiederm@xmission.com (Eric W. Biederman)
Date	2017-08-24 05:20 +0200
Message-ID	<uhQQx-US-1@gated-at.bofh.it>
In reply to	#1718775

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Wed, Aug 23, 2017 at 6:49 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> Argh. And it's *not* fairly straightforward, because the
>> tty_operations "ioctl()" function pointer only gets 'struct tty *'.
>>
>> So in the TIOCGPTPEER path, we don't actually have access to the file
>> pointer of the fd we're doing the ioctl on.
>>
>> And that's where the 'struct path' to the 'ptmx' node is - which we
>> need to then look up the 'pts' directory.
>>
>> How very annoying. I think that's why we did it all at ptmx_open()
>> time, because then we had all the information.
>
> Anyway, the revert is pushed out. So we're back to the old behavior
> that gives the wrong pathname in /proc.
>
> And I think I can handle the lack of a 'struct file *' to the ioctl
> operations by just special-casing TIOCGPTPEER directly in tty_ioctl()
> itself.
>
> That's where we handle "generic" tty ioctls, and doing pty stuff there
> is kind of wrong, but pty's are special.
>
> But I think I'll leave it for tomorrow. So Eric, if you feel like
> looking at this, I'd appreciate it.

This is so far untested (except for compiling) but I think this will
work.

I factor out devpts_ptmx_path out of devpts_acquire so the code
doesn't have to do unnecessary and confusing work, and add the
new function devpts_mnt.

I revert the code to keep anything except a dentry in
tty->link->driver_data.

And reduce the peer opening to a single function ptm_open_peer.

It takes lines of code but the result is very straightforward code.

Eric


 drivers/tty/pty.c         | 63 ++++++++++++++++++++---------------------------
 drivers/tty/tty_io.c      |  3 +++
 fs/devpts/inode.c         | 60 +++++++++++++++++++++++++++++++++-----------
 include/linux/devpts_fs.h | 10 ++++++++
 4 files changed, 85 insertions(+), 51 deletions(-)
diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index 284749fb0f6b..269e6ea65a33 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -69,13 +69,8 @@ static void pty_close(struct tty_struct *tty, struct file *filp)
 #ifdef CONFIG_UNIX98_PTYS
 		if (tty->driver == ptm_driver) {
 			mutex_lock(&devpts_mutex);
-			if (tty->link->driver_data) {
-				struct path *path = tty->link->driver_data;
-
-				devpts_pty_kill(path->dentry);
-				path_put(path);
-				kfree(path);
-			}
+			if (tty->link->driver_data)
+				devpts_pty_kill(tty->link->driver_data);
 			mutex_unlock(&devpts_mutex);
 		}
 #endif
@@ -607,25 +602,25 @@ static inline void legacy_pty_init(void) { }
 static struct cdev ptmx_cdev;
 
 /**
- *	pty_open_peer - open the peer of a pty
- *	@tty: the peer of the pty being opened
+ *	ptm_open_peer - open the peer of a pty
+ *	@master: the open struct file of the ptmx device node
+ *	@tty: the master of the pty being opened
+ *	@flags: the flags for open
  *
- *	Open the cached dentry in tty->link, providing a safe way for userspace
- *	to get the slave end of a pty (where they have the master fd and cannot
- *	access or trust the mount namespace /dev/pts was mounted inside).
+ *	Provide a race free way for userspace to open the slave end of a pty
+ *	(where they have the master fd and cannot access or trust the mount
+ *	namespace /dev/pts was mounted inside).
  */
-static struct file *pty_open_peer(struct tty_struct *tty, int flags)
-{
-	if (tty->driver->subtype != PTY_TYPE_MASTER)
-		return ERR_PTR(-EIO);
-	return dentry_open(tty->link->driver_data, flags, current_cred());
-}
-
-static int pty_get_peer(struct tty_struct *tty, int flags)
+int ptm_open_peer(struct file *master, struct tty_struct *tty, int flags)
 {
 	int fd = -1;
 	struct file *filp = NULL;
 	int retval = -EINVAL;
+	struct path path;
+
+	if ((tty->driver->type != TTY_DRIVER_TYPE_PTY) ||
+	    (tty->driver->subtype != PTY_TYPE_MASTER))
+		return -EIO;
 
 	fd = get_unused_fd_flags(0);
 	if (fd < 0) {
@@ -633,7 +628,16 @@ static int pty_get_peer(struct tty_struct *tty, int flags)
 		goto err;
 	}
 
-	filp = pty_open_peer(tty, flags);
+	/* Compute the slave's path */
+	path.mnt = devpts_mnt(filp);
+	if (IS_ERR(path.mnt)) {
+		retval = PTR_ERR(path.mnt);
+		goto err_put;
+	}
+	path.dentry = tty->link->driver_data;
+
+	filp = dentry_open(&path, flags, current_cred());
+	mntput(path.mnt);
 	if (IS_ERR(filp)) {
 		retval = PTR_ERR(filp);
 		goto err_put;
@@ -662,8 +666,6 @@ static int pty_unix98_ioctl(struct tty_struct *tty,
 		return pty_get_pktmode(tty, (int __user *)arg);
 	case TIOCGPTN: /* Get PT Number */
 		return put_user(tty->index, (unsigned int __user *)arg);
-	case TIOCGPTPEER: /* Open the other end */
-		return pty_get_peer(tty, (int) arg);
 	case TIOCSIG:    /* Send signal to other side of pty */
 		return pty_signal(tty, (int) arg);
 	}
@@ -791,7 +793,6 @@ static int ptmx_open(struct inode *inode, struct file *filp)
 {
 	struct pts_fs_info *fsi;
 	struct tty_struct *tty;
-	struct path *pts_path;
 	struct dentry *dentry;
 	int retval;
 	int index;
@@ -845,26 +846,16 @@ static int ptmx_open(struct inode *inode, struct file *filp)
 		retval = PTR_ERR(dentry);
 		goto err_release;
 	}
-	/* We need to cache a fake path for TIOCGPTPEER. */
-	pts_path = kmalloc(sizeof(struct path), GFP_KERNEL);
-	if (!pts_path)
-		goto err_release;
-	pts_path->mnt = filp->f_path.mnt;
-	pts_path->dentry = dentry;
-	path_get(pts_path);
-	tty->link->driver_data = pts_path;
+	tty->link->driver_data = dentry;
 
 	retval = ptm_driver->ops->open(tty, filp);
 	if (retval)
-		goto err_path_put;
+		goto err_release;
 
 	tty_debug_hangup(tty, "opening (count=%d)\n", tty->count);
 
 	tty_unlock(tty);
 	return 0;
-err_path_put:
-	path_put(pts_path);
-	kfree(pts_path);
 err_release:
 	tty_unlock(tty);
 	// This will also put-ref the fsi
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 974b13d24401..ba3371449a5c 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2518,6 +2518,9 @@ long tty_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	case TIOCSSERIAL:
 		tty_warn_deprecated_flags(p);
 		break;
+	case TIOCGPTPEER:
+		retval = ptm_open_peer(file, tty, (int)arg);
+		break;
 	default:
 		retval = tty_jobctrl_ioctl(tty, real_tty, file, cmd, arg);
 		if (retval != -ENOIOCTLCMD)
diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index 108df2e3602c..6e8816cf7d54 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -133,37 +133,67 @@ static inline struct pts_fs_info *DEVPTS_SB(struct super_block *sb)
 	return sb->s_fs_info;
 }
 
-struct pts_fs_info *devpts_acquire(struct file *filp)
+static int devpts_ptmx_path(struct path *path)
 {
-	struct pts_fs_info *result;
-	struct path path;
 	struct super_block *sb;
-	int err;
-
-	path = filp->f_path;
-	path_get(&path);
+	int err = 0;
 
 	/* Has the devpts filesystem already been found? */
-	sb = path.mnt->mnt_sb;
+	sb = path->mnt->mnt_sb;
 	if (sb->s_magic != DEVPTS_SUPER_MAGIC) {
 		/* Is a devpts filesystem at "pts" in the same directory? */
-		err = path_pts(&path);
-		if (err) {
-			result = ERR_PTR(err);
+		err = path_pts(path);
+		if (err)
 			goto out;
-		}
 
 		/* Is the path the root of a devpts filesystem? */
-		result = ERR_PTR(-ENODEV);
-		sb = path.mnt->mnt_sb;
+		err = -ENODEV;
+		sb = path->mnt->mnt_sb;
 		if ((sb->s_magic != DEVPTS_SUPER_MAGIC) ||
-		    (path.mnt->mnt_root != sb->s_root))
+		    (path->mnt->mnt_root != sb->s_root))
 			goto out;
 	}
 
+out:
+	return err;
+}
+
+struct vfsmount *devpts_mnt(struct file *filp)
+{
+	struct path path;
+	int err;
+
+	path = filp->f_path;
+	path_get(&path);
+
+	err = devpts_ptmx_path(&path);
+	if (err) {
+		path_put(&path);
+		path.mnt = ERR_PTR(err);
+	}
+	return path.mnt;
+}
+
+struct pts_fs_info *devpts_acquire(struct file *filp)
+{
+	struct pts_fs_info *result;
+	struct path path;
+	struct super_block *sb;
+	int err;
+
+	path = filp->f_path;
+	path_get(&path);
+
+	err = devpts_ptmx_path(&path);
+	if (err) {
+		result = ERR_PTR(err);
+		goto out;
+	}
+
 	/*
 	 * pty code needs to hold extra references in case of last /dev/tty close
 	 */
+	sb = path.mnt->mnt_sb;
 	atomic_inc(&sb->s_active);
 	result = DEVPTS_SB(sb);
 
diff --git a/include/linux/devpts_fs.h b/include/linux/devpts_fs.h
index 277ab9af9ac2..e27c548acfb0 100644
--- a/include/linux/devpts_fs.h
+++ b/include/linux/devpts_fs.h
@@ -19,6 +19,7 @@
 
 struct pts_fs_info;
 
+struct vfsmount *devpts_mnt(struct file *);
 struct pts_fs_info *devpts_acquire(struct file *);
 void devpts_release(struct pts_fs_info *);
 
@@ -32,6 +33,15 @@ void *devpts_get_priv(struct dentry *);
 /* unlink */
 void devpts_pty_kill(struct dentry *);
 
+/* in pty.c */
+int ptm_open_peer(struct file *master, struct tty_struct *tty, int flags);
+
+#else
+static inline int
+ptm_open_peer(struct file *master, struct tty_struct *tty, int flags)
+{
+	return -EIO;
+}
 #endif

[toc] | [prev] | [next] | [standalone]

#1718796

From	Linus Torvalds <torvalds@linux-foundation.org>
Date	2017-08-24 05:30 +0200
Message-ID	<uhR0d-Y3-1@gated-at.bofh.it>
In reply to	#1718795

On Wed, Aug 23, 2017 at 8:11 PM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
> -static int pty_get_peer(struct tty_struct *tty, int flags)
> +int ptm_open_peer(struct file *master, struct tty_struct *tty, int flags)
>  {
>         int fd = -1;
>         struct file *filp = NULL;
>         int retval = -EINVAL;
> +       struct path path;
> +
> +       if ((tty->driver->type != TTY_DRIVER_TYPE_PTY) ||
> +           (tty->driver->subtype != PTY_TYPE_MASTER))
> +               return -EIO;

No. Afaik, that could be a legact PTY, which wouldn't be ok.

I think you need to do

        if (tty->driver != ptm_driver)
                return -EIO;

which should check both that it's the unix98 pty, and that it's the master.

Maybe I'm missing something.

That check used to be implicit, in that only the unix98 pty's could
reach that pty_unix98_ioctl() function, so then testing just that it
was a master was sufficient.

> -       /* We need to cache a fake path for TIOCGPTPEER. */
> -       pts_path = kmalloc(sizeof(struct path), GFP_KERNEL);
> -       if (!pts_path)
> -               goto err_release;
> -       pts_path->mnt = filp->f_path.mnt;
> -       pts_path->dentry = dentry;
> -       path_get(pts_path);
> -       tty->link->driver_data = pts_path;
> +       tty->link->driver_data = dentry;

We used to do "path_get()". Shouldn't we now use "dget()"?

But maybe the slave dentry is guaranteed to be around and we don't
need to do that. So your approach may be fine. You did remove all the
path_put() calls too, so I guess it all matches up.

So this looks like it could be fine, but I'd like to make sure.

> +struct vfsmount *devpts_mnt(struct file *filp)
> +{
> +       struct path path;
> +       int err;
> +
> +       path = filp->f_path;
> +       path_get(&path);
> +
> +       err = devpts_ptmx_path(&path);
> +       if (err) {
> +               path_put(&path);
> +               path.mnt = ERR_PTR(err);
> +       }
> +       return path.mnt;
> +}

That can't be right. You're leaking the dentry that you're not returning, no?

But yes, apart from those comments, this looks like what I envisioned.

Needs testing, and needs more looking at those reference counts, but
otherwise looks good.

And while the patch is a bit bigger, I do like getting rid of that
'struct path' thing, and keeping just the dentry.

                      Linus

[toc] | [prev] | [next] | [standalone]

#1719352

From	ebiederm@xmission.com (Eric W. Biederman)
Date	2017-08-24 18:00 +0200
Message-ID	<ui2I1-8tG-7@gated-at.bofh.it>
In reply to	#1718796

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Wed, Aug 23, 2017 at 8:11 PM, Eric W. Biederman
> <ebiederm@xmission.com> wrote:
>> -static int pty_get_peer(struct tty_struct *tty, int flags)
>> +int ptm_open_peer(struct file *master, struct tty_struct *tty, int flags)
>>  {
>>         int fd = -1;
>>         struct file *filp = NULL;
>>         int retval = -EINVAL;
>> +       struct path path;
>> +
>> +       if ((tty->driver->type != TTY_DRIVER_TYPE_PTY) ||
>> +           (tty->driver->subtype != PTY_TYPE_MASTER))
>> +               return -EIO;
>
> No. Afaik, that could be a legact PTY, which wouldn't be ok.
>
> I think you need to do
>
>         if (tty->driver != ptm_driver)
>                 return -EIO;
>
> which should check both that it's the unix98 pty, and that it's the master.
>
> Maybe I'm missing something.
>
> That check used to be implicit, in that only the unix98 pty's could
> reach that pty_unix98_ioctl() function, so then testing just that it
> was a master was sufficient.

No.  That seems correct.  Change made.  If nothing else it is cheaper
and clearer so even if the other version wasn't wrong it is a good idea.

>> -       /* We need to cache a fake path for TIOCGPTPEER. */
>> -       pts_path = kmalloc(sizeof(struct path), GFP_KERNEL);
>> -       if (!pts_path)
>> -               goto err_release;
>> -       pts_path->mnt = filp->f_path.mnt;
>> -       pts_path->dentry = dentry;
>> -       path_get(pts_path);
>> -       tty->link->driver_data = pts_path;
>> +       tty->link->driver_data = dentry;
>
> We used to do "path_get()". Shouldn't we now use "dget()"?
>
> But maybe the slave dentry is guaranteed to be around and we don't
> need to do that. So your approach may be fine. You did remove all the
> path_put() calls too, so I guess it all matches up.
>
> So this looks like it could be fine, but I'd like to make sure.

That change is a revert to the old v4.12 code.  So it is definitely
not regression inducing.

Further devpts_pty_new allocates a dentry keeps it in the devpts
filesystem.  The dentry is good until devpts_pty_kill where the
dentry is unlinked and killed.

I figure not differences from v4.12 if the logic hasn't changed
seems a good good way to cut down on the search for bugs/regressions.

>> +struct vfsmount *devpts_mnt(struct file *filp)
>> +{
>> +       struct path path;
>> +       int err;
>> +
>> +       path = filp->f_path;
>> +       path_get(&path);
>> +
>> +       err = devpts_ptmx_path(&path);
>> +       if (err) {
>> +               path_put(&path);
>> +               path.mnt = ERR_PTR(err);
>> +       }
>> +       return path.mnt;
>> +}
>
> That can't be right. You're leaking the dentry that you're not returning, no?

Correct. That is buggy.  Will fix before I resend.

> But yes, apart from those comments, this looks like what I envisioned.
>
> Needs testing, and needs more looking at those reference counts, but
> otherwise looks good.
>
> And while the patch is a bit bigger, I do like getting rid of that
> 'struct path' thing, and keeping just the dentry.

Eric

[toc] | [prev] | [next] | [standalone]

#1718810

From	Stefan Lippers-Hollmann <s.l-h@gmx.de>
Date	2017-08-24 06:30 +0200
Message-ID	<uhRWi-1yC-7@gated-at.bofh.it>
In reply to	#1718795

[Multipart message — attachments visible in raw view] — view raw

Hi

On 2017-08-23, Eric W. Biederman wrote:
> Linus Torvalds <torvalds@linux-foundation.org> writes:
> > On Wed, Aug 23, 2017 at 6:49 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote:  
[...]
> This is so far untested (except for compiling) but I think this will
> work.
> 
> I factor out devpts_ptmx_path out of devpts_acquire so the code
> doesn't have to do unnecessary and confusing work, and add the
> new function devpts_mnt.
> 
> I revert the code to keep anything except a dentry in
> tty->link->driver_data.
> 
> And reduce the peer opening to a single function ptm_open_peer.
> 
> It takes lines of code but the result is very straightforward code.

I've given this a quick test, while it seems to fix the initial problem
with umounting /dev/ptmx, it does introduce a new one - trying to open 
an xterm (KDE5's konsole to be exact) doesn't open a shell (the shell 
window remains totally empty) and trying to ssh into the system fails 
with "PTY allocation request failed on channel 0", logging in via a 
real tty and creating a new pbuilder chroot from there succeeds.

Regards
	Stefan Lippers-Hollmann

[toc] | [prev] | [next] | [standalone]

#1719358

From	ebiederm@xmission.com (Eric W. Biederman)
Date	2017-08-24 18:00 +0200
Message-ID	<ui2I2-8tG-31@gated-at.bofh.it>
In reply to	#1718810

Stefan Lippers-Hollmann <s.l-h@gmx.de> writes:

> Hi
>
> On 2017-08-23, Eric W. Biederman wrote:
>> Linus Torvalds <torvalds@linux-foundation.org> writes:
>> > On Wed, Aug 23, 2017 at 6:49 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote:  
> [...]
>> This is so far untested (except for compiling) but I think this will
>> work.
>> 
>> I factor out devpts_ptmx_path out of devpts_acquire so the code
>> doesn't have to do unnecessary and confusing work, and add the
>> new function devpts_mnt.
>> 
>> I revert the code to keep anything except a dentry in
>> tty->link->driver_data.
>> 
>> And reduce the peer opening to a single function ptm_open_peer.
>> 
>> It takes lines of code but the result is very straightforward code.
>
> I've given this a quick test, while it seems to fix the initial problem
> with umounting /dev/ptmx, it does introduce a new one - trying to open 
> an xterm (KDE5's konsole to be exact) doesn't open a shell (the shell 
> window remains totally empty) and trying to ssh into the system fails 
> with "PTY allocation request failed on channel 0", logging in via a 
> real tty and creating a new pbuilder chroot from there succeeds.

Weird.  There is at least one leak inducing bug in there.  So perhaps
that is the cause.   *Scratches my head*  Are you also testing the new
ioctl?

I will resend shortly with a version that has no differences in the old
code from v4.12 (other than the refactoring in fs/devpts/inode.c).
Which should make it very hard to have a regression.

Eric

[toc] | [prev] | [next] | [standalone]

#1719482

From	Linus Torvalds <torvalds@linux-foundation.org>
Date	2017-08-24 20:00 +0200
Message-ID	<ui4Aa-1eO-17@gated-at.bofh.it>
In reply to	#1719358

On Thu, Aug 24, 2017 at 8:54 AM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
>
> Weird.  There is at least one leak inducing bug in there.  So perhaps
> that is the cause.   *Scratches my head*  Are you also testing the new
> ioctl?

I can verify, and it's not the leak. I tried your patch with that leak
fix (and the other fixes I pointed out), and I see similar issues that
Stefan noted.

With gnome-terminal, the terminal window opens, and then it says

   Failed to open PTY: No such device

and the terminal obviously doesn't work.

And I see the error: your devpts_ptmx_path() code always returns
-ENODEV because you set the error unconditionally before an error
check, and then you don't clear it if the error didn't happen.

I'll test the fix.

                    Linus

[toc] | [prev] | [next] | [standalone]

#1719485

From	Linus Torvalds <torvalds@linux-foundation.org>
Date	2017-08-24 20:10 +0200
Message-ID	<ui4JP-1xk-9@gated-at.bofh.it>
In reply to	#1719482

On Thu, Aug 24, 2017 at 10:52 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> I'll test the fix.

Yes, that was it, and things work with that fixed.

But that still fails the TIOCGPTPEER ioctl with an NULL pointer
dereference in devpts_mnt,  I probably messed up when I fixed the
dentry refcount leak.

               Linus

[toc] | [prev] | [next] | [standalone]

#1719492

From	Linus Torvalds <torvalds@linux-foundation.org>
Date	2017-08-24 20:20 +0200
Message-ID	<ui4Tw-1Au-5@gated-at.bofh.it>
In reply to	#1719485

[Multipart message — attachments visible in raw view] — view raw

On Thu, Aug 24, 2017 at 11:06 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> But that still fails the TIOCGPTPEER ioctl with an NULL pointer
> dereference in devpts_mnt,  I probably messed up when I fixed the
> dentry refcount leak.

No, that was another bug in the original patch.

ptm_open_peer() passed in 'filp' to devpts_mnt(), but it should
obviously pass in the 'master' one.

filp is NULL at that point.

The attached patch should work. It's Eric's original patch with
various cleanups and fixes.

                    Linus

[toc] | [prev] | [next] | [standalone]

#1719499

From	Linus Torvalds <torvalds@linux-foundation.org>
Date	2017-08-24 20:40 +0200
Message-ID	<ui5cR-1GO-3@gated-at.bofh.it>
In reply to	#1719492

On Thu, Aug 24, 2017 at 11:13 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> The attached patch should work. It's Eric's original patch with
> various cleanups and fixes.

No, one more bug in there: Eric did

+ case TIOCGPTPEER:
+        retval = ptm_open_peer(file, tty, (int)arg);
+        break;

to call that ptm_open_peer(), but that's bogus. The "break;" will just
cause it to continue with the tty ioctl handling, and result in
-ENOTTY in the end.

So it actually *did* open the slave, but then threw the returned fd value away.

It should just do a "return ptm_open_peer(file, tty, (int)arg);" instead.

                       Linus

[toc] | [prev] | [next] | [standalone]

#1719500

From	Linus Torvalds <torvalds@linux-foundation.org>
Date	2017-08-24 20:40 +0200
Message-ID	<ui5cS-1GO-7@gated-at.bofh.it>
In reply to	#1719499

[Multipart message — attachments visible in raw view] — view raw

On Thu, Aug 24, 2017 at 11:31 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> It should just do a "return ptm_open_peer(file, tty, (int)arg);" instead.

Here's the actual tested patch. It "WorksForMe(tm)", including the
TIOCGPTPEER ioctl, and also verified that it gets the pathname right
in /proc, which was the original problem.

But I did *not* check that pbuilder is still happy. Stefan?

                  Linus

[toc] | [prev] | [next] | [standalone]

#1719541

From	Stefan Lippers-Hollmann <s.l-h@gmx.de>
Date	2017-08-24 22:30 +0200
Message-ID	<ui6Vj-2MH-1@gated-at.bofh.it>
In reply to	#1719500

Hi

[ sorry for the re-send, this accidentally only reached you, rather than
 the mailing list and the other recipients as well ]

On 2017-08-24, Linus Torvalds wrote:
> On Thu, Aug 24, 2017 at 11:31 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > It should just do a "return ptm_open_peer(file, tty, (int)arg);" instead.  
> 
> Here's the actual tested patch. It "WorksForMe(tm)", including the
> TIOCGPTPEER ioctl, and also verified that it gets the pathname right
> in /proc, which was the original problem.
> 
> But I did *not* check that pbuilder is still happy. Stefan?

This patch seems to work, ssh, xterm (konsole5), real tty and pbuilder
(creating- and updating the build chroots, just as well as building 
several fairly involved packages) are fine with this patch on top of 
v4.13-rc6-66-g143c97cc6529 (tested on x86_64).

Thanks a lot
	Stefan Lippers-Hollmann

[toc] | [prev] | [next] | [standalone]

#1719542

From	Linus Torvalds <torvalds@linux-foundation.org>
Date	2017-08-24 22:30 +0200
Message-ID	<ui6Vj-2MH-13@gated-at.bofh.it>
In reply to	#1719541

On Thu, Aug 24, 2017 at 1:24 PM, Stefan Lippers-Hollmann <s.l-h@gmx.de> wrote:
>
> This patch seems to work, ssh, xterm (konsole5), real tty and pbuilder
> (creating- and updating the build chroots, just as well as building
> several fairly involved packages) are fine with this patch on top of
> v4.13-rc6-66-g143c97cc6529 (tested on x86_64).

Ok. I just committed Eric's final patch, which is pretty much exactly
the same as what I sent out except with an added verification that the
mount has not changed.

So I marked you as having "Reported-and-tested" it, even if the
version you tested was not 100% identical.

Will push out after the usual final build test,

            Linus

[toc] | [prev] | [next] | [standalone]

#1719506

From	ebiederm@xmission.com (Eric W. Biederman)
Date	2017-08-24 20:50 +0200
Message-ID	<ui5mx-1JS-9@gated-at.bofh.it>
In reply to	#1719492

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Thu, Aug 24, 2017 at 11:06 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> But that still fails the TIOCGPTPEER ioctl with an NULL pointer
>> dereference in devpts_mnt,  I probably messed up when I fixed the
>> dentry refcount leak.
>
> No, that was another bug in the original patch.
>
> ptm_open_peer() passed in 'filp' to devpts_mnt(), but it should
> obviously pass in the 'master' one.
>
> filp is NULL at that point.
>
> The attached patch should work. It's Eric's original patch with
> various cleanups and fixes.

It still retains two of my bugs.  I forgot to return from ioctl in
tty_io.c without which the return code is lost.  I failed to verify
that the devpts filesystem we find via path lookup is the same one
that was found when /dev/ptmx was opened.

Here is my tested version of the patch.

It survives light testing here.

Eric


From: "Eric W. Biederman" <ebiederm@xmission.com>
Date: Thu, 24 Aug 2017 10:55:19 -0500
Subject: [PATCH] pty: Repair TIOCGPTPEER

The implementation of TIOCGPTPEER has two issues.

When /dev/ptmx (as opposed to /dev/pts/ptmx) is opened the wrong
vfsmount is passed to dentry_open.  Which results in the kernel displaying
the wrong pathname for the peer.

The second is simply by caching the vfsmount and dentry of the peer it leaves
them open, in a way they were not previously Which because of the inreased
reference counts can cause unnecessary behaviour differences resulting in
regressions.

To fix these move the ioctl into tty_io.c at a generic level allowing
the ioctl to have access to the struct file on which the ioctl is
being called.  This allows the path of the slave to be derived when
opening the slave through TIOCGPTPEER instead of requiring the path to
the slave be cached.  Thus removing the need for caching the path.

A new function devpts_ptmx_path is factored out of devpts_acquire and
used to implement a function devpts_mntget.   The new function devpts_mntget
takes a filp to perform the lookup on and fsi so that it can confirm
that the superblock that is found by devpts_ptmx_path is the proper superblock.

Fixes: 54ebbfb16034 ("tty: add TIOCGPTPEER ioctl")
Reported-by: Christian Brauner <christian.brauner@canonical.com>
Reported-by: Stefan Lippers-Hollmann <s.l-h@gmx.de>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 drivers/tty/pty.c         | 62 +++++++++++++++++++--------------------------
 drivers/tty/tty_io.c      |  3 +++
 fs/devpts/inode.c         | 64 ++++++++++++++++++++++++++++++++++++-----------
 include/linux/devpts_fs.h | 10 ++++++++
 4 files changed, 89 insertions(+), 50 deletions(-)

diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index 284749fb0f6b..3856bd228fa9 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -69,13 +69,8 @@ static void pty_close(struct tty_struct *tty, struct file *filp)
 #ifdef CONFIG_UNIX98_PTYS
 		if (tty->driver == ptm_driver) {
 			mutex_lock(&devpts_mutex);
-			if (tty->link->driver_data) {
-				struct path *path = tty->link->driver_data;
-
-				devpts_pty_kill(path->dentry);
-				path_put(path);
-				kfree(path);
-			}
+			if (tty->link->driver_data)
+				devpts_pty_kill(tty->link->driver_data);
 			mutex_unlock(&devpts_mutex);
 		}
 #endif
@@ -607,25 +602,24 @@ static inline void legacy_pty_init(void) { }
 static struct cdev ptmx_cdev;
 
 /**
- *	pty_open_peer - open the peer of a pty
- *	@tty: the peer of the pty being opened
+ *	ptm_open_peer - open the peer of a pty
+ *	@master: the open struct file of the ptmx device node
+ *	@tty: the master of the pty being opened
+ *	@flags: the flags for open
  *
- *	Open the cached dentry in tty->link, providing a safe way for userspace
- *	to get the slave end of a pty (where they have the master fd and cannot
- *	access or trust the mount namespace /dev/pts was mounted inside).
+ *	Provide a race free way for userspace to open the slave end of a pty
+ *	(where they have the master fd and cannot access or trust the mount
+ *	namespace /dev/pts was mounted inside).
  */
-static struct file *pty_open_peer(struct tty_struct *tty, int flags)
-{
-	if (tty->driver->subtype != PTY_TYPE_MASTER)
-		return ERR_PTR(-EIO);
-	return dentry_open(tty->link->driver_data, flags, current_cred());
-}
-
-static int pty_get_peer(struct tty_struct *tty, int flags)
+int ptm_open_peer(struct file *master, struct tty_struct *tty, int flags)
 {
 	int fd = -1;
 	struct file *filp = NULL;
 	int retval = -EINVAL;
+	struct path path;
+
+	if (tty->driver != ptm_driver)
+		return -EIO;
 
 	fd = get_unused_fd_flags(0);
 	if (fd < 0) {
@@ -633,7 +627,16 @@ static int pty_get_peer(struct tty_struct *tty, int flags)
 		goto err;
 	}
 
-	filp = pty_open_peer(tty, flags);
+	/* Compute the slave's path */
+	path.mnt = devpts_mntget(master, tty->driver_data);
+	if (IS_ERR(path.mnt)) {
+		retval = PTR_ERR(path.mnt);
+		goto err_put;
+	}
+	path.dentry = tty->link->driver_data;
+
+	filp = dentry_open(&path, flags, current_cred());
+	mntput(path.mnt);
 	if (IS_ERR(filp)) {
 		retval = PTR_ERR(filp);
 		goto err_put;
@@ -662,8 +665,6 @@ static int pty_unix98_ioctl(struct tty_struct *tty,
 		return pty_get_pktmode(tty, (int __user *)arg);
 	case TIOCGPTN: /* Get PT Number */
 		return put_user(tty->index, (unsigned int __user *)arg);
-	case TIOCGPTPEER: /* Open the other end */
-		return pty_get_peer(tty, (int) arg);
 	case TIOCSIG:    /* Send signal to other side of pty */
 		return pty_signal(tty, (int) arg);
 	}
@@ -791,7 +792,6 @@ static int ptmx_open(struct inode *inode, struct file *filp)
 {
 	struct pts_fs_info *fsi;
 	struct tty_struct *tty;
-	struct path *pts_path;
 	struct dentry *dentry;
 	int retval;
 	int index;
@@ -845,26 +845,16 @@ static int ptmx_open(struct inode *inode, struct file *filp)
 		retval = PTR_ERR(dentry);
 		goto err_release;
 	}
-	/* We need to cache a fake path for TIOCGPTPEER. */
-	pts_path = kmalloc(sizeof(struct path), GFP_KERNEL);
-	if (!pts_path)
-		goto err_release;
-	pts_path->mnt = filp->f_path.mnt;
-	pts_path->dentry = dentry;
-	path_get(pts_path);
-	tty->link->driver_data = pts_path;
+	tty->link->driver_data = dentry;
 
 	retval = ptm_driver->ops->open(tty, filp);
 	if (retval)
-		goto err_path_put;
+		goto err_release;
 
 	tty_debug_hangup(tty, "opening (count=%d)\n", tty->count);
 
 	tty_unlock(tty);
 	return 0;
-err_path_put:
-	path_put(pts_path);
-	kfree(pts_path);
 err_release:
 	tty_unlock(tty);
 	// This will also put-ref the fsi
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 974b13d24401..10c4038c0e8d 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2518,6 +2518,9 @@ long tty_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	case TIOCSSERIAL:
 		tty_warn_deprecated_flags(p);
 		break;
+	case TIOCGPTPEER:
+		/* Special because the struct file is needed */
+		return ptm_open_peer(file, tty, (int)arg);
 	default:
 		retval = tty_jobctrl_ioctl(tty, real_tty, file, cmd, arg);
 		if (retval != -ENOIOCTLCMD)
diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index 108df2e3602c..809da242a452 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -133,37 +133,73 @@ static inline struct pts_fs_info *DEVPTS_SB(struct super_block *sb)
 	return sb->s_fs_info;
 }
 
-struct pts_fs_info *devpts_acquire(struct file *filp)
+static int devpts_ptmx_path(struct path *path)
 {
-	struct pts_fs_info *result;
-	struct path path;
 	struct super_block *sb;
 	int err;
 
-	path = filp->f_path;
-	path_get(&path);
-
 	/* Has the devpts filesystem already been found? */
-	sb = path.mnt->mnt_sb;
+	sb = path->mnt->mnt_sb;
 	if (sb->s_magic != DEVPTS_SUPER_MAGIC) {
 		/* Is a devpts filesystem at "pts" in the same directory? */
-		err = path_pts(&path);
-		if (err) {
-			result = ERR_PTR(err);
+		err = path_pts(path);
+		if (err)
 			goto out;
-		}
 
 		/* Is the path the root of a devpts filesystem? */
-		result = ERR_PTR(-ENODEV);
-		sb = path.mnt->mnt_sb;
+		err = -ENODEV;
+		sb = path->mnt->mnt_sb;
 		if ((sb->s_magic != DEVPTS_SUPER_MAGIC) ||
-		    (path.mnt->mnt_root != sb->s_root))
+		    (path->mnt->mnt_root != sb->s_root))
 			goto out;
 	}
 
+	err = 0;
+out:
+	return err;
+}
+
+struct vfsmount *devpts_mntget(struct file *filp, struct pts_fs_info *fsi)
+{
+	struct path path;
+	int err;
+
+	path = filp->f_path;
+	path_get(&path);
+
+	err = devpts_ptmx_path(&path);
+	dput(path.dentry);
+	if (err) {
+		mntput(path.mnt);
+		path.mnt = ERR_PTR(err);
+	}
+	if (DEVPTS_SB(path.mnt->mnt_sb) != fsi) {
+		mntput(path.mnt);
+		path.mnt = ERR_PTR(-ENODEV);
+	}
+	return path.mnt;
+}
+
+struct pts_fs_info *devpts_acquire(struct file *filp)
+{
+	struct pts_fs_info *result;
+	struct path path;
+	struct super_block *sb;
+	int err;
+
+	path = filp->f_path;
+	path_get(&path);
+
+	err = devpts_ptmx_path(&path);
+	if (err) {
+		result = ERR_PTR(err);
+		goto out;
+	}
+
 	/*
 	 * pty code needs to hold extra references in case of last /dev/tty close
 	 */
+	sb = path.mnt->mnt_sb;
 	atomic_inc(&sb->s_active);
 	result = DEVPTS_SB(sb);
 
diff --git a/include/linux/devpts_fs.h b/include/linux/devpts_fs.h
index 277ab9af9ac2..100cb4343763 100644
--- a/include/linux/devpts_fs.h
+++ b/include/linux/devpts_fs.h
@@ -19,6 +19,7 @@
 
 struct pts_fs_info;
 
+struct vfsmount *devpts_mntget(struct file *, struct pts_fs_info *);
 struct pts_fs_info *devpts_acquire(struct file *);
 void devpts_release(struct pts_fs_info *);
 
@@ -32,6 +33,15 @@ void *devpts_get_priv(struct dentry *);
 /* unlink */
 void devpts_pty_kill(struct dentry *);
 
+/* in pty.c */
+int ptm_open_peer(struct file *master, struct tty_struct *tty, int flags);
+
+#else
+static inline int
+ptm_open_peer(struct file *master, struct tty_struct *tty, int flags)
+{
+	return -EIO;
+}
 #endif
 
 
-- 
2.14.1

[toc] | [prev] | [next] | [standalone]

Page 2 of 3 — ← Prev page 1 [2] 3 Next page →

csiph-web

[PATCH 0/1] devpts: use dynamic_dname() to generate proc name

Contents

#1718733

#1718736

#1718748

#1718753

#1718755

#1718768

#1718775

#1718795

#1718796

#1719352

#1718810

#1719358

#1719482

#1719485

#1719492

#1719499

#1719500

#1719541

#1719542

#1719506