From: ebiederm@xmission.com (Eric W. Biederman)
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Stefan Lippers-Hollmann <s.l-h@gmx.de>,
        Christian Brauner <christian.brauner@canonical.com>,
        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>,
        Thorsten Leemhuis <regressions@leemhuis.info>
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>
        <20170816194805.hnof3aqiqykwki7p@gmail.com>
        <CA+55aFzmcydKsuwXpocd0hYMrACEXt+UUuaqa7j3L5LG7SjajQ@mail.gmail.com>
        <CA+55aFxMnEQVq8zRG1JEsW+6x23qkBV=KKXksgVHmBegPuepMg@mail.gmail.com>
        <CA+55aFz+qGxM9MidcvQkP4er-qn0_oEqCac=ob64f+twhptSgQ@mail.gmail.com>
        <CA+55aFxXzwoDdN72m2oaV4XuxO0DGoT+Bp84i07VFpLQ3Ftcpw@mail.gmail.com>
        <87pobvruzt.fsf@xmission.com>
        <CA+55aFyJsxsxeW-8RX83AM2zs+_i8h85i99X+YkuFEqpqjtp4w@mail.gmail.com>
        <87ziazqdfr.fsf@xmission.com>
        <CA+55aFwssaZpapQdc2iVVmMt0KxTHYdeo71Xbp4Ex-pYP_MpMw@mail.gmail.com>
        <20170824022436.44adb497@mir>
        <CA+55aFzHa0vL2Yui05r6aSQM0-oHuUAjqi1kxheG_tFQRkUGYw@mail.gmail.com>
Date: Wed, 23 Aug 2017 20:25:37 -0500
In-Reply-To: <CA+55aFzHa0vL2Yui05r6aSQM0-oHuUAjqi1kxheG_tFQRkUGYw@mail.gmail.com>
        (Linus Torvalds's message of "Wed, 23 Aug 2017 17:42:05 -0700")
Message-ID: <87378hhi3y.fsf@xmission.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1898
Lines: 50

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