Messages in this thread |  | | From | Linus Torvalds <> | Date | Wed, 16 Aug 2017 11:26:37 -0700 | Subject | Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name |
| |
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
|  |