| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <0157609c-f99a-8a51-4d2e-fe55bcd971c9@themaw.net>
Date: Tue, 26 Jul 2022 15:10:23 +0800
From: Ian Kent <raven@...maw.net>
To: Al Viro <viro@...iv.linux.org.uk>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
David Howells <dhowells@...hat.com>,
Miklos Szeredi <miklos@...redi.hu>,
linux-fsdevel <linux-fsdevel@...r.kernel.org>,
Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/3] vfs: track count of child mounts
On 26/7/22 13:11, Ian Kent wrote:
> On 20/7/22 10:17, Ian Kent wrote:
>>
>> On 20/7/22 09:50, Al Viro wrote:
>>> On Mon, Jul 11, 2022 at 11:37:40AM +0800, Ian Kent wrote:
>>>> While the total reference count of a mount is mostly all that's needed
>>>> the reference count corresponding to the mounts only is occassionally
>>>> also needed (for example, autofs checking if a tree of mounts can be
>>>> expired).
>>>>
>>>> To make this reference count avaialble with minimal changes add a
>>>> counter to track the number of child mounts under a given mount. This
>>>> count can then be used to calculate the mounts only reference count.
>>> No. This is a wrong approach - instead of keeping track of number of
>>> children, we should just stop having them contribute to refcount of
>>> the parent. Here's what I've got in my local tree; life gets simpler
>>> that way.
>>
>> Right, I'll grab this and run some tests.
>
> Just a heads up, I've been able to reliably hang autofs with the
>
> below patch using my submount test (which is actually pretty good
>
> at exposing problems).
>
>
> No idea what it is yet but I'll look around and keep trying to work
>
> it out, ;)
Mmm ... so it's just slower ... that was unexpected ...
>
>
> Ian
>
>>
>>
>> Ian
>>
>>>
>>> commit e99f1f9cc864103f326a5352e6ce1e377613437f
>>> Author: Al Viro <viro@...iv.linux.org.uk>
>>> Date: Sat Jul 9 14:45:39 2022 -0400
>>>
>>> namespace: don't keep ->mnt_parent pinned
>>> makes refcounting more consistent
>>> Signed-off-by: Al Viro <viro@...iv.linux.org.uk>
>>>
>>> diff --git a/fs/namespace.c b/fs/namespace.c
>>> index 68789f896f08..53c29110a0cd 100644
>>> --- a/fs/namespace.c
>>> +++ b/fs/namespace.c
>>> @@ -906,7 +906,6 @@ void mnt_set_mountpoint(struct mount *mnt,
>>> struct mount *child_mnt)
>>> {
>>> mp->m_count++;
>>> - mnt_add_count(mnt, 1); /* essentially, that's mntget */
>>> child_mnt->mnt_mountpoint = mp->m_dentry;
>>> child_mnt->mnt_parent = mnt;
>>> child_mnt->mnt_mp = mp;
>>> @@ -1429,22 +1428,18 @@ void mnt_cursor_del(struct mnt_namespace
>>> *ns, struct mount *cursor)
>>> int may_umount_tree(struct vfsmount *m)
>>> {
>>> struct mount *mnt = real_mount(m);
>>> - int actual_refs = 0;
>>> - int minimum_refs = 0;
>>> - struct mount *p;
>>> BUG_ON(!m);
>>> /* write lock needed for mnt_get_count */
>>> lock_mount_hash();
>>> - for (p = mnt; p; p = next_mnt(p, mnt)) {
>>> - actual_refs += mnt_get_count(p);
>>> - minimum_refs += 2;
>>> + for (struct mount *p = mnt; p; p = next_mnt(p, mnt)) {
>>> + int allowed = p == mnt ? 2 : 1;
>>> + if (mnt_get_count(p) > allowed) {
>>> + unlock_mount_hash();
>>> + return 0;
>>> + }
>>> }
>>> unlock_mount_hash();
>>> -
>>> - if (actual_refs > minimum_refs)
>>> - return 0;
>>> -
>>> return 1;
>>> }
>>> @@ -1586,7 +1581,6 @@ static void umount_tree(struct mount *mnt,
>>> enum umount_tree_flags how)
>>> disconnect = disconnect_mount(p, how);
>>> if (mnt_has_parent(p)) {
>>> - mnt_add_count(p->mnt_parent, -1);
>>> if (!disconnect) {
>>> /* Don't forget about p */
>>> list_add_tail(&p->mnt_child,
>>> &p->mnt_parent->mnt_mounts);
>>> @@ -2892,12 +2886,8 @@ static int do_move_mount(struct path
>>> *old_path, struct path *new_path)
>>> put_mountpoint(old_mp);
>>> out:
>>> unlock_mount(mp);
>>> - if (!err) {
>>> - if (attached)
>>> - mntput_no_expire(parent);
>>> - else
>>> - free_mnt_ns(ns);
>>> - }
>>> + if (!err && !attached)
>>> + free_mnt_ns(ns);
>>> return err;
>>> }
>>> @@ -3869,7 +3859,7 @@ SYSCALL_DEFINE2(pivot_root, const char
>>> __user *, new_root,
>>> const char __user *, put_old)
>>> {
>>> struct path new, old, root;
>>> - struct mount *new_mnt, *root_mnt, *old_mnt, *root_parent,
>>> *ex_parent;
>>> + struct mount *new_mnt, *root_mnt, *old_mnt, *root_parent;
>>> struct mountpoint *old_mp, *root_mp;
>>> int error;
>>> @@ -3900,10 +3890,9 @@ SYSCALL_DEFINE2(pivot_root, const char
>>> __user *, new_root,
>>> new_mnt = real_mount(new.mnt);
>>> root_mnt = real_mount(root.mnt);
>>> old_mnt = real_mount(old.mnt);
>>> - ex_parent = new_mnt->mnt_parent;
>>> root_parent = root_mnt->mnt_parent;
>>> if (IS_MNT_SHARED(old_mnt) ||
>>> - IS_MNT_SHARED(ex_parent) ||
>>> + IS_MNT_SHARED(new_mnt->mnt_parent) ||
>>> IS_MNT_SHARED(root_parent))
>>> goto out4;
>>> if (!check_mnt(root_mnt) || !check_mnt(new_mnt))
>>> @@ -3942,7 +3931,6 @@ SYSCALL_DEFINE2(pivot_root, const char __user
>>> *, new_root,
>>> attach_mnt(root_mnt, old_mnt, old_mp);
>>> /* mount new_root on / */
>>> attach_mnt(new_mnt, root_parent, root_mp);
>>> - mnt_add_count(root_parent, -1);
>>> touch_mnt_namespace(current->nsproxy->mnt_ns);
>>> /* A moved mount should not expire automatically */
>>> list_del_init(&new_mnt->mnt_expire);
>>> @@ -3952,8 +3940,6 @@ SYSCALL_DEFINE2(pivot_root, const char __user
>>> *, new_root,
>>> error = 0;
>>> out4:
>>> unlock_mount(old_mp);
>>> - if (!error)
>>> - mntput_no_expire(ex_parent);
>>> out3:
>>> path_put(&root);
>>> out2:
>>> diff --git a/fs/pnode.c b/fs/pnode.c
>>> index 1106137c747a..e2c8a4b18857 100644
>>> --- a/fs/pnode.c
>>> +++ b/fs/pnode.c
>>> @@ -368,7 +368,7 @@ static inline int do_refcount_check(struct mount
>>> *mnt, int count)
>>> */
>>> int propagate_mount_busy(struct mount *mnt, int refcnt)
>>> {
>>> - struct mount *m, *child, *topper;
>>> + struct mount *m, *child;
>>> struct mount *parent = mnt->mnt_parent;
>>> if (mnt == parent)
>>> @@ -384,7 +384,6 @@ int propagate_mount_busy(struct mount *mnt, int
>>> refcnt)
>>> for (m = propagation_next(parent, parent); m;
>>> m = propagation_next(m, parent)) {
>>> - int count = 1;
>>> child = __lookup_mnt(&m->mnt, mnt->mnt_mountpoint);
>>> if (!child)
>>> continue;
>>> @@ -392,13 +391,10 @@ int propagate_mount_busy(struct mount *mnt,
>>> int refcnt)
>>> /* Is there exactly one mount on the child that covers
>>> * it completely whose reference should be ignored?
>>> */
>>> - topper = find_topper(child);
>>> - if (topper)
>>> - count += 1;
>>> - else if (!list_empty(&child->mnt_mounts))
>>> + if (!find_topper(child) && !list_empty(&child->mnt_mounts))
>>> continue;
>>> - if (do_refcount_check(child, count))
>>> + if (do_refcount_check(child, 1))
>>> return 1;
>>> }
>>> return 0;
Powered by blists - more mailing lists