Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1713144 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [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 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 1 of 3  [1] 2 3  Next page →

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

Hi,

Recently the kernel has implemented the TIOCGPTPEER ioctl() call which allows
users to retrieve an fd for the slave side of a pty solely based on the
corresponding fd for the master side. The ioctl()-based fd retrieval however
causes the "/proc/<pid>/fd/<pty-slave-fd>" symlink to point to the wrong dentry.
Currently, it will always point to "/". The following simple program can be used
to illustrate the problem when run on a system that implements the TIOCGPTPEER
ioctl() call.

#define _GNU_SOURCE
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <sys/ioctl.h>
#include <sys/types.h>
#include <sys/stat.h>

int main()
{
	int master;
	int ret = -1, slave = -1;
	char buf[4096], path[4096];

	master = open("/dev/ptmx", O_RDWR | O_NOCTTY);
	if (master < 0)
		return -1;

	ret = grantpt(master);
	if (ret < 0)
		return -1;

	ret = unlockpt(master);
	if (ret < 0)
		goto on_error;

	slave = ioctl(master, TIOCGPTPEER, O_RDWR | O_NOCTTY);
	if (slave < 0)
		goto on_error;

	ret = snprintf(path, 4096, "/proc/self/fd/%d", slave);
	if (ret < 0 || ret >= 4096)
		goto on_error;

	ret = readlink(path, buf, sizeof(buf));
	if (ret < 0)
		goto on_error;
	printf("I point to \"%s\"\n", buf);

on_error:
	close(master);
	if (slave >= 0)
		close(slave);
	return ret;
}

With the symlink pointing to the wrong path any interesting path-based operation
on the slave side will fail. Also this will cause ttyname{_r}() to falsely
report that this fd does not return to a tty. So I really think this needs to be
fixed. The fix however doesn't seem super obvious to me. It seems the most
straightforward way for now is to behave like the implementation of pipes and
sockets that implement a dynamic_dname() method to correctly set the content of
the "/proc/<pid>/fd/<n>" symlink without requiring special-casing in the proc
implementation itself. I prefer this approach. The downside of this however is
that in case the devpts is not mounted at its standard "/dev/pts" location but
e.g. at "/mnt" the content of the corresponding "/proc/<pid>/fd/<n>" symlink
will still be "/dev/pts/<idx>" although it should likely be "/mnt/<idx". I've
gone over this back and forth in my head but I think that this is not a deal
breaker. All libcs currently hard-code "/dev/pts/<idx>" into their
implementation of ptsname{_r}() and so wouldn't be affected by this change at
all. Furthermore, mounting devpts somewhere other than "/dev/pts" (e.g. "/mnt")
doesn't seem to work and from what I gather from LKML is not really expected nor
supported to work.
All ioctl() I've seens so far that return fds seems to implement a
dynamic_dname() method and I didn't see any other way how to do this here. One
thing that came to mind was to somehow get at the "absolute path" for the slave
side dentry such that you retrieve the mountpoint for the devpts fs and then
combine this with the pty slave index to generate the proc name. However, the
concept of an absolute path does not really make sense for dentries and in the
face of bind-mounts it becomes questionable what devpts instance should win.
Furthermore, the dynamic_dname() method will only allow you to access the dentry
itself and not a struct path which would contain the vfsmount information. In
any case, here is my patch, when applied the fd returned by ioctl(fd,
TIOCGPTPEER) will have the correct content ("/dev/pts/<idx>"):

Christian Brauner (1):
  devpts: use dynamic_dname() to generate proc name

 fs/devpts/inode.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

-- 
2.13.3

[toc] | [next] | [standalone]

#1713217

On Wed, Aug 16, 2017 at 10:12 AM, Christian Brauner
<christian.brauner@ubuntu.com> wrote:
>
> Recently the kernel has implemented the TIOCGPTPEER ioctl() call which allows
> users to retrieve an fd for the slave side of a pty solely based on the
> corresponding fd for the master side. The ioctl()-based fd retrieval however
> causes the "/proc/<pid>/fd/<pty-slave-fd>" symlink to point to the wrong dentry.
> Currently, it will always point to "/". The following simple program can be used
> to illustrate the problem when run on a system that implements the TIOCGPTPEER
> ioctl() call.

I think your patch is wrong - we need to actually use the *right*
path, rather than hardcode "/dev/pts/%d" in there.

Hardcoding "/dev/pts/%d" is something that user space can already do.
The kernel can and should do better.

That "dynamic_dname()" helper is for things that don't really have a
real pathname at all, so things like pipes and other file descriptors
that were opened without an associated entry in a filesystem (sockets,
other "special" inodes with no filesystem backing).

For things like pts slaves, we actually *do* have a real pathname -
and we should expose it. If the devpts filesystem is mounted somewhere
else than /dev/pts, it should give that correct pathname.

I also think your test program is a bit buggy, and silly:

>         ret = snprintf(path, 4096, "/proc/self/fd/%d", slave);
>         if (ret < 0 || ret >= 4096)
>                 goto on_error;
>
>         ret = readlink(path, buf, sizeof(buf));
>         if (ret < 0)
>                 goto on_error;
>         printf("I point to \"%s\"\n", buf);

This just smells wrong to me. And not just because you don't
NUL-terminate the readlink() return value (or just use "%.*s" with
ret, buf) .

We should just give people a better way to get the pathname than that
readlink on /proc/self/fd/<fd> thing. It's kind of ridiculous that we
don't. We already have a "getcwd()" system call that only does it for
cwd.

So I think we could/should just add a system call for 'fdname()' or similar.

                Linus

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

#1713229

On Wed, Aug 16, 2017 at 11:26 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Hardcoding "/dev/pts/%d" is something that user space can already do.
> The kernel can and should do better.

Put another way: there's no point in applying the patch as-is, since
existing glibc ptsname() does the same thing better and faster
entirely in user space.

Also, we already do special things to get a path for this, but it
clearly isn't working. See the

        /* We need to cache a fake path for TIOCGPTPEER. */

comment in ptmx_open(). Why doesn't the file d_path get filled in
correctly there, I wonder.

Because The regular

        readlink("/proc/self/fd/0", ...)

that 'tty' does works correctly.  I think we've done something
incorrect in pty_open_peer(), which means that the fd path hasn't been
fully filled in.

Fixing that *should* fix the readlink() automatically, since it
clearly works for the 'tty' binary.

I'm wondering why it's not working as-is. "vfs_open()" does that

        file->f_path = *path;

thing. Why aren't we getting the right path? The ptmx_open() code
looks ok to me.

Al, do you see what the issue is, and why we don't get a proper path
on that readlink?

            Linus

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

#1713272

On Wed, Aug 16, 2017 at 11:48:48AM -0700, Linus Torvalds wrote:
> On Wed, Aug 16, 2017 at 11:26 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > Hardcoding "/dev/pts/%d" is something that user space can already do.
> > The kernel can and should do better.
> 
> Put another way: there's no point in applying the patch as-is, since
> existing glibc ptsname() does the same thing better and faster
> entirely in user space.

Right. I actually took that to be an argument for this patch not against it. :)
Another point I was trying to make in my initial mail is that ttyname{_r}() will
currently look at "/proc/self/fd/<nr> to detect the name of the associated pts
device and it will error out if the content it reads is not a "/dev/pts/<n>"
path. Afaict musl and glibc both currently rely on this. This would be broken
with the current TIOCGPTPEER change.

> 
> Also, we already do special things to get a path for this, but it
> clearly isn't working. See the
> 
>         /* We need to cache a fake path for TIOCGPTPEER. */
> 
> comment in ptmx_open(). Why doesn't the file d_path get filled in
> correctly there, I wonder.
> 
> Because The regular
> 
>         readlink("/proc/self/fd/0", ...)
> 
> that 'tty' does works correctly.  I think we've done something
> incorrect in pty_open_peer(), which means that the fd path hasn't been
> fully filled in.
> 
> Fixing that *should* fix the readlink() automatically, since it
> clearly works for the 'tty' binary.
> 
> I'm wondering why it's not working as-is. "vfs_open()" does that
> 
>         file->f_path = *path;
> 
> thing. Why aren't we getting the right path? The ptmx_open() code
> looks ok to me.

I thought - and sorry if I'm completely wrong here - that the proc name came
from the open(const char *pathname, ...) call. Currently the only way to
retrieve a slave side fd for a given pty pair is by calling open("/dev/pts/<n>",
O_RDWR | O_NOCTTY). This would take care of placing the correct path in the proc
symlink through do_filp_open() and friends. However, for ioctl() calls that open
dentries to return an fd this is not possible and - or so I thought - the proc
name is/has to be generated via dynamic_dname(). But again, I might be totally
off here.

Christian

> 
> Al, do you see what the issue is, and why we don't get a proper path
> on that readlink?
> 
>             Linus

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

#1713279

On Wed, Aug 16, 2017 at 12:48 PM, Christian Brauner
<christian.brauner@canonical.com> wrote:
>
> I thought - and sorry if I'm completely wrong here - that the proc name came
> from the open(const char *pathname, ...) call.

No. It comes from the path associated with the file descriptor, and is
expanded from the dentry tree.

Which is why you get a full pathname even when you only opened
something using a relative pathname.

So the fact that we _don't_ get the right pathname for the pts entry
here means that something got screwed up in setting filp->f_path to
the right thing. We have all the code in place that _tries_ to do it,
but it clearly has a bug somewhere.

               Linus

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

#1713289

On Wed, Aug 16, 2017 at 12:56 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> So the fact that we _don't_ get the right pathname for the pts entry
> here means that something got screwed up in setting filp->f_path to
> the right thing. We have all the code in place that _tries_ to do it,
> but it clearly has a bug somewhere.

Ok, I think I see what the bug is, although I don't have a fix for it yet.

We generate the path largely correctly: the path has a nice dentry
that contains the right pts number, and has the right parent pointer
that points to the root of the pts mount.

And we also fill in the path 'mnt' field. Everything should be fine.

Except when we actually hit that root dentry of the pts mount, the
code in prepend_path() hits this condition:

                if (dentry == vfsmnt->mnt_root || IS_ROOT(dentry)) {
                        struct mount *parent = ACCESS_ONCE(mnt->mnt_parent);
                        /* Escaped? */
                        if (dentry != vfsmnt->mnt_root) {

and we break out, and reset the path to '/' because we think it
somehow escaped out of the user namespace.

So it looks like we filled in the path with the *wrong* mount information.

And THAT in turn is because we fill the path with the mount
information for the "/dev/ptmx" field - which is *not* in the
/dev/pts/ mount - that's the mount for '/dev'.

So we have a dentry and a mnt, but they simply aren't paired up correctly.

And you can see this with your test program: if you open /dev/pts/ptmx
for the master, it actually works correctly (but you need to make sure
the permissions for that ptmx node allow that).

Anyway, I know what's wrong, next step is to figure out what the fix is.

                   Linus

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

#1713310

On Wed, Aug 16, 2017 at 1:19 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Anyway, I know what's wrong, next step is to figure out what the fix is.

Grr, We actually look up the right mnt in devpts_acquire(), but we
don't save it.  We only save the superblock pointer, because that's
traditionally what we used.

I suspect the easiest fix is to just add a "mnt" argument to
devpts_acquire(),  It shouldn't be too painful. Let me try.

              Linus

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

#1713326

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

On Wed, Aug 16, 2017 at 1:30 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> I suspect the easiest fix is to just add a "mnt" argument to
> devpts_acquire(),  It shouldn't be too painful. Let me try.

Ok, here's a *very* lightly tested patch. It might have new bugs, but
it makes your test program DTRT.

Al, mind going over this and making sure I didn't miss anything?

And Christian, if you can beat on this, that would be good.

                        Linus

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

#1713346

On Wed, Aug 16, 2017 at 11:03 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Wed, Aug 16, 2017 at 1:30 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> I suspect the easiest fix is to just add a "mnt" argument to
>> devpts_acquire(),  It shouldn't be too painful. Let me try.
>
> Ok, here's a *very* lightly tested patch. It might have new bugs, but
> it makes your test program DTRT.

Cool. Very happy this could be fixed so quickly!

>
> Al, mind going over this and making sure I didn't miss anything?
>
> And Christian, if you can beat on this, that would be good.

Yes, I can pound on this nicely with liblxc. We have patch
( https://github.com/lxc/lxc/pull/1728 ) up for review that
allocates pty fds from private devpts mounts in different namespaces
and sends those fds around between different namespaces. This
was one of the original motivations for implementing TIOCGPTPEER.
I had marked it as blocked until this bug was fixed which now seems
to be the case.

Thanks Linus!
Christian

>
>                         Linus

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

#1713348

On Wed, Aug 16, 2017 at 2:37 PM, Christian Brauner
<christian.brauner@canonical.com> wrote:
>> And Christian, if you can beat on this, that would be good.
>
> Yes, I can pound on this nicely with liblxc. We have patch
> ( https://github.com/lxc/lxc/pull/1728 ) up for review that
> allocates pty fds from private devpts mounts in different namespaces
> and sends those fds around between different namespaces.

Good. Testing that this works with different pts filesystems in
different places is exactly the kind of thing I'd like to see. I only
tested with my single pts filesystem that is mounted at /dev/pts, and
making sure it works when there are multiple mounts and in different
places is exactly the kind of testing this should get.

For example, if some namespace has it's pty's in _its_ /dev/pts/
hierarchy, and you then pass such a pty to somebody else that either
doesn't have that pts mount at all, or has it visible somewhere
entirely different, the result should now be something else than that
"/dev/pts/n" path.

But it would be good to just test this in general too, and make sure I
didn't screw up some reference count or something. The patch *looks*
obviously correct, but ...

               Linus

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

#1713361

On Wed, Aug 16, 2017 at 2:45 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> But it would be good to just test this in general too, and make sure I
> didn't screw up some reference count or something. The patch *looks*
> obviously correct, but ...

Side note: I suspect it should be marked for stable, but it's going to
be basically impossible to back-port this any further than 4.7 or
something where we made each pts mount its own proper filesystem. The
code before that was a mess, and this is never going to work right in
old kernels.

Of course, TIOCGPTPEER itself is much more recent than that and was
done in this merge window, so hopefully you haven't backported it and
expect this all to work in some older kernel?

               Linus

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

#1713369

On Wed, Aug 16, 2017 at 11:55 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Wed, Aug 16, 2017 at 2:45 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> But it would be good to just test this in general too, and make sure I
>> didn't screw up some reference count or something. The patch *looks*
>> obviously correct, but ...
>
> Side note: I suspect it should be marked for stable, but it's going to
> be basically impossible to back-port this any further than 4.7 or
> something where we made each pts mount its own proper filesystem. The
> code before that was a mess, and this is never going to work right in
> old kernels.

Yeah, it would be nice if this could make it into stable at least and of course
into 4.13 but I take it that's a given anyway.

>
> Of course, TIOCGPTPEER itself is much more recent than that and was
> done in this merge window, so hopefully you haven't backported it and
> expect this all to work in some older kernel?

No, I haven't backported anything and we never intended to.
Serge, Stéphane, and I were very careful to not blindly rely on anything
that recent and - imho - security sensitive in our api. I see this more as a
"tech-preview" until this has seen some decent userspace testing. Even
if the kernel side seems fine now a lot of the userspace pty api is currently
using path-based operations instead of operations on the pty fds themselves.
This is tricky and error-prone when sending around pty fds between
different namespaces as one can see from the ttyname{_r}() example above.

Christian

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

#1713378

On Wed, Aug 16, 2017 at 11:45 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Wed, Aug 16, 2017 at 2:37 PM, Christian Brauner
> <christian.brauner@canonical.com> wrote:
>>> And Christian, if you can beat on this, that would be good.
>>
>> Yes, I can pound on this nicely with liblxc. We have patch
>> ( https://github.com/lxc/lxc/pull/1728 ) up for review that
>> allocates pty fds from private devpts mounts in different namespaces
>> and sends those fds around between different namespaces.
>
> Good. Testing that this works with different pts filesystems in
> different places is exactly the kind of thing I'd like to see. I only
> tested with my single pts filesystem that is mounted at /dev/pts, and
> making sure it works when there are multiple mounts and in different
> places is exactly the kind of testing this should get.

I'm compiling a kernel now and depending on how good the in-flight
wifi is I try to test this right away and answer here if that helps. If the
in-flight wifi sucks it might take me until tomorrow.

>
> For example, if some namespace has it's pty's in _its_ /dev/pts/
> hierarchy, and you then pass such a pty to somebody else that either
> doesn't have that pts mount at all, or has it visible somewhere
> entirely different, the result should now be something else than that
> "/dev/pts/n" path.
>
> But it would be good to just test this in general too, and make sure I
> didn't screw up some reference count or something. The patch *looks*
> obviously correct, but ...
>
>                Linus

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

#1718457

Christian Brauner <christian.brauner@canonical.com> writes:

> On Wed, Aug 16, 2017 at 11:45 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>> On Wed, Aug 16, 2017 at 2:37 PM, Christian Brauner
>> <christian.brauner@canonical.com> wrote:
>>>> And Christian, if you can beat on this, that would be good.
>>>
>>> Yes, I can pound on this nicely with liblxc. We have patch
>>> ( https://github.com/lxc/lxc/pull/1728 ) up for review that
>>> allocates pty fds from private devpts mounts in different namespaces
>>> and sends those fds around between different namespaces.
>>
>> Good. Testing that this works with different pts filesystems in
>> different places is exactly the kind of thing I'd like to see. I only
>> tested with my single pts filesystem that is mounted at /dev/pts, and
>> making sure it works when there are multiple mounts and in different
>> places is exactly the kind of testing this should get.
>
> I'm compiling a kernel now and depending on how good the in-flight
> wifi is I try to test this right away and answer here if that helps. If the
> in-flight wifi sucks it might take me until tomorrow.

Linus has merged the fix but have you been able to test and verify all
is well from your side?

Eric

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

#1718654

On Wed, Aug 23, 2017 at 10:31:53AM -0500, Eric W. Biederman wrote:
> Christian Brauner <christian.brauner@canonical.com> writes:
> 
> > On Wed, Aug 16, 2017 at 11:45 PM, Linus Torvalds
> > <torvalds@linux-foundation.org> wrote:
> >> On Wed, Aug 16, 2017 at 2:37 PM, Christian Brauner
> >> <christian.brauner@canonical.com> wrote:
> >>>> And Christian, if you can beat on this, that would be good.
> >>>
> >>> Yes, I can pound on this nicely with liblxc. We have patch
> >>> ( https://github.com/lxc/lxc/pull/1728 ) up for review that
> >>> allocates pty fds from private devpts mounts in different namespaces
> >>> and sends those fds around between different namespaces.
> >>
> >> Good. Testing that this works with different pts filesystems in
> >> different places is exactly the kind of thing I'd like to see. I only
> >> tested with my single pts filesystem that is mounted at /dev/pts, and
> >> making sure it works when there are multiple mounts and in different
> >> places is exactly the kind of testing this should get.
> >
> > I'm compiling a kernel now and depending on how good the in-flight
> > wifi is I try to test this right away and answer here if that helps. If the
> > in-flight wifi sucks it might take me until tomorrow.
> 
> Linus has merged the fix but have you been able to test and verify all
> is well from your side?

Hi Eric,

Sorry for the late reply! So I've tested the patch and it does what I expect it
to do, i.e. it places the correct path as the content of /proc/<pid>/fd/<n>.
However, if I'm correct not in all cases. But I need to confirm this first and I
didn't want to start pointless discussions before having something reliable.
Thanks!

The reason for the late reply is that I was investigating some other "weirdness"
related to this patch which also - I believe - touches on the bind-mount
escaping you mentioned in your previous mail. It relates to an idea I had in
mind for a while now but never thought through sufficiently. I hope to find some
time on the weekend to think about this more clearly. I had hoped to bundle this
up with my testing but didn't get around to it.

Christian

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

#1713392

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

> On Wed, Aug 16, 2017 at 1:30 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> I suspect the easiest fix is to just add a "mnt" argument to
>> devpts_acquire(),  It shouldn't be too painful. Let me try.
>
> Ok, here's a *very* lightly tested patch. It might have new bugs, but
> it makes your test program DTRT.
>
> Al, mind going over this and making sure I didn't miss anything?
>
> And Christian, if you can beat on this, that would be good.

Linus reading through this it looks like your error handling is wrong.

> diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
> index 284749fb0f6b..432f514e3f42 100644
> --- a/drivers/tty/pty.c
> +++ b/drivers/tty/pty.c
> @@ -793,6 +793,7 @@ static int ptmx_open(struct inode *inode, struct file *filp)
>  	struct tty_struct *tty;
>  	struct path *pts_path;
>  	struct dentry *dentry;
> +	struct vfsmount *mnt;
>  	int retval;
>  	int index;
>  
> @@ -805,7 +806,7 @@ static int ptmx_open(struct inode *inode, struct file *filp)
>  	if (retval)
>  		return retval;
>  
> -	fsi = devpts_acquire(filp);
> +	fsi = devpts_acquire(filp, &mnt);
>  	if (IS_ERR(fsi)) {
>  		retval = PTR_ERR(fsi);
>  		goto out_free_file;
> @@ -849,9 +850,14 @@ static int ptmx_open(struct inode *inode, struct file *filp)
>  	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);
> +
> +	/*
> +	 * The mnt already got a ref from devpts_acquire(),
> +	 * so we only dget() on the dentry.
> +	 */
> +	pts_path->mnt = mnt;
> +	pts_path->dentry = dget(dentry);
> +
>  	tty->link->driver_data = pts_path;
>  
>  	retval = ptm_driver->ops->open(tty, filp);
        ^^^^^^^

If this open fails the code jumps to err_put_path which falls
through into out_put_fsi.  But it also does path_put(pts_path).
Which will result in a double mntput of mnt.

So I think err_path_put needs to be updated to just put the dentry,
and let the later mntput put the mount.

> @@ -874,6 +880,7 @@ static int ptmx_open(struct inode *inode, struct file *filp)
>  	devpts_kill_index(fsi, index);
>  out_put_fsi:
>  	devpts_release(fsi);
> +	mntput(mnt);
>  out_free_file:
>  	tty_free_file(filp);
>  	return retval;

Eric

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

#1713401

On Wed, Aug 16, 2017 at 3:46 PM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
>>       tty->link->driver_data = pts_path;
>>
>>       retval = ptm_driver->ops->open(tty, filp);
>         ^^^^^^^
>
> If this open fails the code jumps to err_put_path which falls
> through into out_put_fsi.

No it doesn't.

err_path_put falls through to err_release, but that then does a
"return retval;". It doesn't get to out_put_fsi.

Now, I _do_ want people to check the release path, but I don't think
this was it.  Or am I blind and missing something?

                 Linus

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

#1713419

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

> On Wed, Aug 16, 2017 at 3:46 PM, Eric W. Biederman
> <ebiederm@xmission.com> wrote:
>>>       tty->link->driver_data = pts_path;
>>>
>>>       retval = ptm_driver->ops->open(tty, filp);
>>         ^^^^^^^
>>
>> If this open fails the code jumps to err_put_path which falls
>> through into out_put_fsi.
>
> No it doesn't.
>
> err_path_put falls through to err_release, but that then does a
> "return retval;". It doesn't get to out_put_fsi.

*Blink* You are right I missed that.

In which case I am concerned about failures that make it to err_release.
Unless I am missing something (again) failures that jump to err_release
won't call mntput and will result in a mnt leak.

> Now, I _do_ want people to check the release path, but I don't think
> this was it.  Or am I blind and missing something?

Eric

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

#1713434

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

On Wed, Aug 16, 2017 at 4:51 PM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
>
> *Blink* You are right I missed that.
>
> In which case I am concerned about failures that make it to err_release.
> Unless I am missing something (again) failures that jump to err_release
> won't call mntput and will result in a mnt leak.

Yes, I think you're right.

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!

               Linus

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

#1713472

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

> On Wed, Aug 16, 2017 at 4:51 PM, Eric W. Biederman
> <ebiederm@xmission.com> wrote:
>>
>> *Blink* You are right I missed that.
>>
>> In which case I am concerned about failures that make it to err_release.
>> Unless I am missing something (again) failures that jump to err_release
>> won't call mntput and will result in a mnt leak.
>
> Yes, I think you're right.
>
> 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?

My eyeballs don't see any problems with the patch below.

Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>


Eric


> ENTIRELY UNTESTED!
>
>                Linus
>
>  drivers/tty/pty.c         | 7 +++++--
>  fs/devpts/inode.c         | 4 +++-
>  include/linux/devpts_fs.h | 2 +-
>  3 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
> index 284749fb0f6b..1fc80ea87c13 100644
> --- a/drivers/tty/pty.c
> +++ b/drivers/tty/pty.c
> @@ -793,6 +793,7 @@ static int ptmx_open(struct inode *inode, struct file *filp)
>  	struct tty_struct *tty;
>  	struct path *pts_path;
>  	struct dentry *dentry;
> +	struct vfsmount *mnt;
>  	int retval;
>  	int index;
>  
> @@ -805,7 +806,7 @@ static int ptmx_open(struct inode *inode, struct file *filp)
>  	if (retval)
>  		return retval;
>  
> -	fsi = devpts_acquire(filp);
> +	fsi = devpts_acquire(filp, &mnt);
>  	if (IS_ERR(fsi)) {
>  		retval = PTR_ERR(fsi);
>  		goto out_free_file;
> @@ -849,7 +850,7 @@ static int ptmx_open(struct inode *inode, struct file *filp)
>  	pts_path = kmalloc(sizeof(struct path), GFP_KERNEL);
>  	if (!pts_path)
>  		goto err_release;
> -	pts_path->mnt = filp->f_path.mnt;
> +	pts_path->mnt = mnt;
>  	pts_path->dentry = dentry;
>  	path_get(pts_path);
>  	tty->link->driver_data = pts_path;
> @@ -866,6 +867,7 @@ static int ptmx_open(struct inode *inode, struct file *filp)
>  	path_put(pts_path);
>  	kfree(pts_path);
>  err_release:
> +	mntput(mnt);
>  	tty_unlock(tty);
>  	// This will also put-ref the fsi
>  	tty_release(inode, filp);
> @@ -874,6 +876,7 @@ static int ptmx_open(struct inode *inode, struct file *filp)
>  	devpts_kill_index(fsi, index);
>  out_put_fsi:
>  	devpts_release(fsi);
> +	mntput(mnt);
>  out_free_file:
>  	tty_free_file(filp);
>  	return retval;
> diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
> index 108df2e3602c..44dfbca9306f 100644
> --- a/fs/devpts/inode.c
> +++ b/fs/devpts/inode.c
> @@ -133,7 +133,7 @@ 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)
> +struct pts_fs_info *devpts_acquire(struct file *filp, struct vfsmount **ptsmnt)
>  {
>  	struct pts_fs_info *result;
>  	struct path path;
> @@ -142,6 +142,7 @@ struct pts_fs_info *devpts_acquire(struct file *filp)
>  
>  	path = filp->f_path;
>  	path_get(&path);
> +	*ptsmnt = NULL;
>  
>  	/* Has the devpts filesystem already been found? */
>  	sb = path.mnt->mnt_sb;
> @@ -165,6 +166,7 @@ struct pts_fs_info *devpts_acquire(struct file *filp)
>  	 * pty code needs to hold extra references in case of last /dev/tty close
>  	 */
>  	atomic_inc(&sb->s_active);
> +	*ptsmnt = mntget(path.mnt);
>  	result = DEVPTS_SB(sb);
>  
>  out:
> diff --git a/include/linux/devpts_fs.h b/include/linux/devpts_fs.h
> index 277ab9af9ac2..7883e901f65c 100644
> --- a/include/linux/devpts_fs.h
> +++ b/include/linux/devpts_fs.h
> @@ -19,7 +19,7 @@
>  
>  struct pts_fs_info;
>  
> -struct pts_fs_info *devpts_acquire(struct file *);
> +struct pts_fs_info *devpts_acquire(struct file *, struct vfsmount **ptsmnt);
>  void devpts_release(struct pts_fs_info *);
>  
>  int devpts_new_index(struct pts_fs_info *);

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

Page 1 of 3  [1] 2 3  Next page →

Back to top | Article view | linux.kernel

csiph-web