Use new mount API for bind mount

[xxyzz]

Use open_tree() and move_mount() if they are available otherwise fallback to mount(), GH issue #755

[smcv]

This is great, thanks for working on this!

Am I correct to think that there's a behaviour change here: previously if the source or the destination was a symlink, we would dereference the symlink and act on the target (because mount(2) didn't give us any alternative), but now we'll act on the symlink itself?

(Or does the code we already had, for working around mount(2) not having an O_NOFOLLOW equivalent, make that irrelevant? It's quite subtle.)

I wonder whether it would be better for new_bind_mount() to default to being exactly equivalent to the old code based on mount(2) (that would mean MOVE_MOUNT_F_SYMLINKS and MOVE_MOUNT_T_SYMLINKS, I think?), and let the caller opt-in to mounting symlinks as symlinks? (But if so, we would have to prevent the fallback to mount(2) in the don't-follow-symlinks case, because it cannot implement those semantics, and document the opt-in option as not working on kernels < 5.2.)

[smcv]

At the moment the only caller of new_bind_mount is assuming that the failing path that is relevant is always the destination, but open_tree() acts on the source.

I think this function will probably need to look more like

static bind_mount_result
new_bind_mount (int src_fd,
                const char *src_path,
                int dest_fd,
                const char *dest_path,
                bind_option_t options,
                char **failing_path)

with a copy of either src_path or dest_path, as appropriate, reported as the failing_path on error?

(I'm not sure which one is more useful for move_mount() - probably whichever one is involved in more error situations.)

[xxyzz]

The new code use src_path if errno is EINVAL otherwise use dest_path when move_mount fails. I also added failing_path to bind_mount_fallback() but failing_path seems not used when mount() fails.

[smcv]

This would probably benefit from having some debug() calls to keep track of which implementation we're using, similar to the ones I added for mount_setattr() in #796. At the moment debug() is #if 0 (not compiled) by default, needs to be enabled manually, and if enabled, is always used; but I've proposed #796 which makes it a lot more convenient.

[smcv]

#788 is an alternative to this, but with a lot more code (LLM-generated), which makes me cautious about it. There might be something in there that's useful, though.

[xxyzz]

test-sanbox.py has tests for both src and dest symlink, and these tests passed. I think this is because open_tree dereferences the src symlink and dest sysmlink is not allowed at here. Therefore I think MOVE_MOUNT_*_SYMLINKS flags are not needed.

[smcv]

We should probably get a bubblewrap 0.13.0 release out with the mount_setattr() improvements before making more structural changes to how we mount stuff; I'll try to get that out today or early next week.

[smcv]

#788 is an alternative to this, but with a lot more code (LLM-generated), which makes me cautious about it. There might be something in there that's useful, though.

One thing I do notice in #788 is this:

  /* Syscall presence is insufficient: older kernels reject cloning detached
   * mounts and attaching a mount to a detached target

If that is, in fact, true (I haven't verified either way), then this will need to be cleverer about distinguishing between three scenarios and not just two, where your current code is handling the first and last but not the middle one:

• old kernel, syscall missing (or seccomp emulating an old kernel by forcibly failing the syscall)
• intermediate kernel, syscall is present but doesn't work
• new kernel, syscall is present and works

@domenkozar, do you have a reference for what the problem was, and what kernel version fixed it?

[rusty-snake]

Linux 6.15

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=130e696aa68b0e0c13f790898529b2cc1a5f8f8e

[smcv]

OK, if the fix is that recent, we should definitely not rush into using this syscall family.

[xxyzz]

That kernel commit is about "mount detached mounts on detached mounts", it doesn't affect this pr.