Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1752561AbdHPTsN (ORCPT <rfc822;w@1wt.eu>);
        Wed, 16 Aug 2017 15:48:13 -0400
Received: from mail-io0-f171.google.com ([209.85.223.171]:37489 "EHLO
        mail-io0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1751943AbdHPTsM (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Wed, 16 Aug 2017 15:48:12 -0400
Date: Wed, 16 Aug 2017 21:48:07 +0200
From: Christian Brauner <christian.brauner@canonical.com>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Christian Brauner <christian.brauner@ubuntu.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        "Serge E. Hallyn" <serge@hallyn.com>,
        Al Viro <viro@zeniv.linux.org.uk>
Subject: Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name
Message-ID: <20170816194805.hnof3aqiqykwki7p@gmail.com>
References: <20170816171211.4021-1-christian.brauner@ubuntu.com>
 <CA+55aFzqedKjjq+FDF3AEgyQfqQRHyR-6PTR_BTvyyQLqnPwPw@mail.gmail.com>
 <CA+55aFyy1RU2emO6giU9gNVb_doYP+K_y61ij2cumgs1_VMMmA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <CA+55aFyy1RU2emO6giU9gNVb_doYP+K_y61ij2cumgs1_VMMmA@mail.gmail.com>
User-Agent: NeoMutt/20170609 (1.8.3)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 2366
Lines: 61

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