Al Viro <viro@xxxxxxxxxxxxxxxxxx> writes: > On Thu, Jan 12, 2017 at 05:18:12AM +1300, Eric W. Biederman wrote: >> >> When I look at what propagate_mount_busy is trying to do and I look >> at the code closely I discover there is a great disconnect between the >> two. In the ordinary non-propagation case propagate_mount_busy has >> been verifying that there are no submounts and that there are no >> extraneous references on the mount. >> >> For mounts that the unmount would propagate to propagate_mount_busy has >> been verifying that there are no extraneous references only if there >> are no submounts. Which is nonsense. > > ... because? > >> Thefore rework the logic in propgate_mount_busy so that for each >> mount it examines it considers that mount busy if that mount has >> children or if there are extraneous references to that mount. >> >> While this check was incorrect we could leak mounts instead of simply >> failing umount. > > What do you mean, leak? We ended up not unmounting them, and they > stayed around until umount of whatever they'd been shadowed by/slipped under > had exposed them and they got explicitly unmounted. Leak in the sense of userspace expecting everything to be cleaned up and it was not. My concerns exist in the presence of a slave mount with something mounted on it. Nothing exotic needs to exist. > This is not a leak in a sense of "data structure is unreachable and > will never be freed", unlike the one your previous version had introduced. > > Your change might very well be a nicer behaviour - or a DoS in making. > But it really deserves more detailed rationale than that and yes, it > is a user-visible change. With rather insane userland setups in that > area (*cough* systemd *cough* docker), it's _not_ obviously correct. I wrote this patch primarily because I looked at what the code was doing and saw semantics that make no obvious sense to me given my experience with how unmount ordinarily works, I needed to point that out so we can have this conversation independently. Having just looked and doubled checked I can say this is how umount is documented to behave in Documentation/filesystems/shared-subtrees. My experience with umount in other contexts is either umount succeeds, umount fails because something is making the mount busy, or a lazy umount is requested and users of the mount are ignored. The way umount propagation works adds another case into my understanding of umount behavior. Namely that the umount will be propagated to some places but not to other places depending on the presence of submounts. For me at least that violates the principle of least surprise. I do not understand if we are going to give up in some cases if the mount is busy but not in other cases why we even bother looking at propgated mounts. Given this behavior has existed for a decade and we have some very creative pieces of userspace code I completely agree that my case for making this change is insufficiently strong. To actually make this change would require extensive testing to verify I don't introduce any regressions in userspace applications. This patch was my way of pointing out the very strange (to my eyes) behaviour of umount propagation ignoring some cases of busy mounts, and asking if that was what you were concerned about in propagate_mount_busy. As my case is insufficient for this change. And my concerns about what you were concerned about with propagate_mount_busy have been addressed I am going to drop this patch. 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
