Al Viro <viro@xxxxxxxxxxxxxxxxxx> writes:
> On Thu, Jan 12, 2017 at 05:19:34AM +1300, Eric W. Biederman wrote:
>
>> + if (child->mnt.mnt_root == smp->m_dentry) {
>> + struct mount *q;
>> + q = __lookup_mnt(&child->mnt_parent->mnt,
>> + child->mnt_mountpoint);
>> + if (q)
>> + mnt_change_mountpoint(child, smp, q);
>> + }
>
> This is wrong; condition will be true for *all* mounts seen by that loop.
> Feel free to add else WARN_ON(1); to the line above and try to trigger
> it. You are misinterpreting what propagate_mnt() and commit_tree() are
> doing - the loop in commit_tree() goes through the submounts and sets ->mnt_ns
> on those. The of the fields is already set up by that point. For roots
> of those copies we need to set ->mnt_hash/->mnt_child as well, but for
> all submounts it's already been done by copy_tree(). Again, commit_tree()
> is called once per secondary copy of source tree, not once per created
> mount.
*doh*
>> +static struct mount *find_topper(struct mount *mnt)
>> +{
>> + /* If there is exactly one mount covering mnt completely return it. */
>> + struct mount *child;
>> +
>> + if (!list_is_singular(&mnt->mnt_mounts))
>> + return NULL;
>> +
>> + child = list_first_entry(&mnt->mnt_mounts, struct mount, mnt_child);
>> + if (child->mnt_parent != mnt ||
>
> The first part can't happen. Turn that into WARN_ON(child->mnt_parent != mnt)
> if you wish, but that never occurs unless the data structures are
> corrupted.
Agreed.
>> + child->mnt_mountpoint != mnt->mnt.mnt_root)
>> + return NULL;
>> @@ -431,6 +459,15 @@ static void __propagate_umount(struct mount *mnt)
>> if (!child || !IS_MNT_MARKED(child))
>> continue;
>> CLEAR_MNT_MARK(child);
>> +
>> + /* If there is exactly one mount covering all of child
>> + * replace child with that mount.
>> + */
>> + topper = find_topper(child);
>> + if (topper)
>> + mnt_change_mountpoint(child->mnt_parent, child->mnt_mp,
>> + topper);
>> +
>> if (list_empty(&child->mnt_mounts)) {
>> list_del_init(&child->mnt_child);
>> child->mnt.mnt_flags |= MNT_UMOUNT;
>
> Umm... With fallthrough from "is completely overmounted" case? And
> I'm not sure I understand what that list_empty() is doing there after
> your previous semantics change - how _can_ we reach that point with
> non-empty ->mnt_mounts now?
With the semantic change to propagate_mnt_busy, to reach the list_empty
with a non-empty mnt_mounts requires the a umount(MNT_DETACH) as that
skips the propagate_mnt_busy call.
Without the semantic change it is even easier to get there.
A respin of this patch without the semantic change in a moment.
Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
