mnt: Fix a memory stomp in umount

Why is this piece of code using its own made up and buggy list handling in the first place? We have list functions for these things, exactly so that people shouldn't write buggy stuff by hand.

Quoted 3 lines · Linus Torvalds

Why is this piece of code using its own made up and buggy list handling in the first place? We have list functions for these things, exactly so that people shouldn't write buggy stuff by hand.

Oh. Ok, I see what's going on. We have "list_splice()", but we don't have the equivalent "hlist_splice()". So it's doing that by hand, and did it badly.

Al, this is your bug. I guess I can take the "manual hlist_splice" fix from Eric, but I'm not really happy with it. There's a few other places in that same commit where the list splice operation has been open-coded.

Mind taking a look?

It looks like we can pretty easily use mnt_list instead of mnt_hash, see below (note: the code is only compile tested).

Quoted 10 lines · Linus Torvalds

Quoted 3 lines · Linus Torvalds

Why is this piece of code using its own made up and buggy list handling in the first place? We have list functions for these things, exactly so that people shouldn't write buggy stuff by hand.

Oh. Ok, I see what's going on. We have "list_splice()", but we don't have the equivalent "hlist_splice()". So it's doing that by hand, and did it badly.

Quoted 6 lines

Al, this is your bug. I guess I can take the "manual hlist_splice" fix from Eric, but I'm not really happy with it. There's a few other places in that same commit where the list splice operation has been open-coded.

Mind taking a look?

It looks like we can pretty easily use mnt_list instead of mnt_hash, see below (note: the code is only compile tested).

While converting this to ordinary list helpers I found something strange.

In __propagate_umount we currently add the child to be unmounted in a different location in the list then we did before the conversion of mnt_hash to a hlist for rcu's accesses benefit.

Now maybe propagate_next handles this (I still need to read and understand that code) if not it looks like I may have found another bug, as it looks like today we can add a node to our list without propogating the unmount from the node.

Quoted 29 lines · Eric W. Biederman

Quoted 10 lines · Linus Torvalds

Quoted 3 lines · Linus Torvalds

Why is this piece of code using its own made up and buggy list handling in the first place? We have list functions for these things, exactly so that people shouldn't write buggy stuff by hand.

Oh. Ok, I see what's going on. We have "list_splice()", but we don't have the equivalent "hlist_splice()". So it's doing that by hand, and did it badly.

Quoted 6 lines

Al, this is your bug. I guess I can take the "manual hlist_splice" fix from Eric, but I'm not really happy with it. There's a few other places in that same commit where the list splice operation has been open-coded.

Mind taking a look?

It looks like we can pretty easily use mnt_list instead of mnt_hash, see below (note: the code is only compile tested).

While converting this to ordinary list helpers I found something strange.

In __propagate_umount we currently add the child to be unmounted in a different location in the list then we did before the conversion of mnt_hash to a hlist for rcu's accesses benefit.

Quoted 4 lines

Now maybe propagate_next handles this (I still need to read and understand that code) if not it looks like I may have found another bug, as it looks like today we can add a node to our list without propogating the unmount from the node.

Er... Why would we want to reprocess it? The loop goes through all nodes getting events from ours; everything that gets events from _them_ included at the same time they are.

We might have wanted to because before your change to an hlist that is what the code did.

Having read through the code of propagate_next it does look like it iterates through the entire propagation hierarchy so there shouldn't be a need to visit mounts that we have placed on the propagation list.

Any thoughts on using mnt_list instead of mnt_hash to allow the use of list_splice and list_move?

Quoted 9 lines · Eric W. Biederman

We might have wanted to because before your change to an hlist that is what the code did.

Having read through the code of propagate_next it does look like it iterates through the entire propagation hierarchy so there shouldn't be a need to visit mounts that we have placed on the propagation list.

Any thoughts on using mnt_list instead of mnt_hash to allow the use of list_splice and list_move?

Frankly, I don't see much benefit in that. What's wrong with actually adding
__hlist_splice(struct hlist_node *head,
struct hlist_node *tail,
struct hlist_node **where)

{
	next = *where;
	*where = head;
	head->pprev = where;
	if (next) {
		tail->next = next;
		next->pprev = tail;
	}
}

and use it as

	__hlist_splice(tmp_list.first, &last->mnt_hash, &unmounted.first);

(and I'd start with if (unlikely(!mnt)) return; /* idiot caller */)

Another primitive would be something like hlist_transplant(struct hlist_head **from, struct hlist_head **to)

{
	*to = *from;
	if (!hlist_empty(*to)) {
		to->first->pprev = to;
		INIT_HLIST_HEAD(from);
	}
}

with namespace_unlock() starting with

	hlist_transplant(&head, &unmounted);
	if (likely(hlist_empty(&head))) {
                up_write(&namespace_sem);
                return;
	}
        /* undo decrements we'd done in umount_tree() */
	...

Linus, would that work for you, or would you prefer something more fancy? --
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 9 lines · Eric W. Biederman

We might have wanted to because before your change to an hlist that is what the code did.

Having read through the code of propagate_next it does look like it iterates through the entire propagation hierarchy so there shouldn't be a need to visit mounts that we have placed on the propagation list.

Any thoughts on using mnt_list instead of mnt_hash to allow the use of list_splice and list_move?

Frankly, I don't see much benefit in that. What's wrong with actually adding
__hlist_splice(struct hlist_node *head,
struct hlist_node *tail,
struct hlist_node **where)

{
	next = *where;
	*where = head;
	head->pprev = where;
	if (next) {
		tail->next = next;
		next->pprev = tail;
	}
}

and use it as

	__hlist_splice(tmp_list.first, &last->mnt_hash, &unmounted.first);

(and I'd start with if (unlikely(!mnt)) return; /* idiot caller */)

Another primitive would be something like hlist_transplant(struct hlist_head **from, struct hlist_head **to)

{
	*to = *from;
	if (!hlist_empty(*to)) {
		to->first->pprev = to;
		INIT_HLIST_HEAD(from);
	}
}

with namespace_unlock() starting with

	hlist_transplant(&head, &unmounted);
	if (likely(hlist_empty(&head))) {
                up_write(&namespace_sem);
                return;
	}
        /* undo decrements we'd done in umount_tree() */
	...

Linus, would that work for you, or would you prefer something more fancy?

- It requires keeping track of the tail of the list which is half of the error prone complexity.

- I have a fix for a bad interaction between MNT_LOCKED and MNT_DETACH where I need to leave mounts on the mount hash, until the final mntput. For that I need not to reuse the the mnt_hash lists.

Quoted 2 lines · Eric W. Biederman

- It requires keeping track of the tail of the list which is half of the error prone complexity.

Er... In umount_tree() it comes pretty much for free, actually.

Quoted 3 lines

- I have a fix for a bad interaction between MNT_LOCKED and MNT_DETACH where I need to leave mounts on the mount hash, until the final mntput. For that I need not to reuse the the mnt_hash lists.

Details, please.

While reviewing the code of umount_tree I realized that when we append to a preexisting unmounted list we do not change pprev of the former first item in the list.

Which means later in namespace_unlock hlist_del_init(&mnt->mnt_hash) on the former first item of the list will stomp unmounted.first leaving it set to some random mount point which we are likely to free soon.

This isn't likely to hit, but if it does I don't know how anyone could track it down.

Quoted 5 lines · Eric W. Biederman

While reviewing the code of umount_tree I realized that when we append to a preexisting unmounted list we do not change pprev of the former first item in the list.

Quoted 6 lines

Which means later in namespace_unlock hlist_del_init(&mnt->mnt_hash) on the former first item of the list will stomp unmounted.first leaving it set to some random mount point which we are likely to free soon.

This isn't likely to hit, but if it does I don't know how anyone could track it down.

All you need for that kind of loop to work is correct -next on all elements.

Quoted 6 lines · Al Viro

Quoted 5 lines · Eric W. Biederman

While reviewing the code of umount_tree I realized that when we append to a preexisting unmounted list we do not change pprev of the former first item in the list.

Quoted 8 lines

Quoted 6 lines

Which means later in namespace_unlock hlist_del_init(&mnt->mnt_hash) on the former first item of the list will stomp unmounted.first leaving it set to some random mount point which we are likely to free soon.

This isn't likely to hit, but if it does I don't know how anyone could track it down.

All you need for that kind of loop to work is correct -next on all elements.

Mind you, I'm not saying that this isn't worth sanitizing. Just that analysis in your commit message is incorrect - we actually do _not_ stomp on memory there. It's brittle and it's only saved by having only very few things done to that hlist, but the actual memory corruption does not happen. Again, I'm no defending that code; it's certaily better off making a properly-spliced hlist rather than relying on precise sequence of operations in namespace_unlock().