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

* [PATCH 0/1] devpts: use dynamic_dname() to generate proc name
@ 2017-08-16 17:12 Christian Brauner
  2017-08-16 17:12 ` [PATCH 1/1] " Christian Brauner
  2017-08-16 18:26 ` [PATCH 0/1] " Linus Torvalds
  0 siblings, 2 replies; 55+ messages in thread
From: Christian Brauner @ 2017-08-16 17:12 UTC (permalink / raw)
  To: linux-kernel, serge, torvalds, viro; +Cc: Christian Brauner

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

^ permalink raw reply	[flat|nested] 55+ messages in thread