Use mount_setattr() in bind_mount()#756
Conversation
xxyzz
force-pushed
the
mount_setattr
branch
2 times, most recently
from
69fdb9e to
f65b3ec
Compare
Were they related somehow? |
No, these actions are just too outdated so I updated them. |
|
What was wrong with The code changes have some AI smell to me. |
|
I feel |
It needs a review from.someone who is actually responsible for the codebase. |
Please move unrelated changes to a separate commit, or even to a separate PR, to have a more focused review, and in general avoid unnecessary space changes. Also the commit message of the main change should mention the rationale from #754 and also have lines like: Ideally with your real name, but I don't think this is a strict requirement 😄 |
| const char *src); | ||
|
|
||
| #ifndef __NR_mount_setattr | ||
| #define __NR_mount_setattr 442 |
There was a problem hiding this comment.
Please note that syscall numbers in linux are platform specific, so this is not general enough. E.g. on alpha __NR_mount_setattr is 552, see https://github.com/torvalds/linux/blob/6f3ed7fec72fc8979b2a8c7219c0a9fcfc8d07b5/arch/alpha/kernel/syscalls/syscall.tbl#L484
What about having a simpler approach like done for pivot_root? So that we require that __NR_mount_setattr is defined but we support the cases where the system function mount_setattr is not.
There was a problem hiding this comment.
Code updated. I think I've read that function before but somehow forgot it...
There was a problem hiding this comment.
Yes, I like the approach @ao2 suggested here.
This also means that when compiling on ancient kernels, we would have the option to define __NR_mount_setattr on only the architectures that we expect might genuinely be running ancient kernels (in practice usually x86), like this:
#ifndef __NR_mount_setattr
# if defined(__x86_64__)
# define __NR_mount_setattr ...
# elif defined(__i386__)
# define __NR_mount_setattr ...
# endif
#endifbubblewrap is used by Steam Runtime version 1, which is very old, so we should check whether its libc/kernel headers define __NR_mount_setattr. (But that's an easy change to add later; and Steam Runtime v1 only supports x86, so it doesn't have to try to cope with other architectures.)
smcv
left a comment
smcv
left a comment
There was a problem hiding this comment.
Thanks for looking at this!
| const char *src); | ||
|
|
||
| #ifndef __NR_mount_setattr | ||
| #define __NR_mount_setattr 442 |
There was a problem hiding this comment.
Yes, I like the approach @ao2 suggested here.
This also means that when compiling on ancient kernels, we would have the option to define __NR_mount_setattr on only the architectures that we expect might genuinely be running ancient kernels (in practice usually x86), like this:
#ifndef __NR_mount_setattr
# if defined(__x86_64__)
# define __NR_mount_setattr ...
# elif defined(__i386__)
# define __NR_mount_setattr ...
# endif
#endifbubblewrap is used by Steam Runtime version 1, which is very old, so we should check whether its libc/kernel headers define __NR_mount_setattr. (But that's an easy change to add later; and Steam Runtime v1 only supports x86, so it doesn't have to try to cope with other architectures.)
This was a policy question in the past, and the repo owners concluded that contributions under a pseudonym can be accepted. But...
Is this a real address that is really yours, or a fake/placeholder email address that doesn't really exist? I think we do need the Signed-off-by to come from an identifiable person, even if they're only identifiable by a pseudonym. Github provides a pseudo-email-address that identifies a Github account which is probably sufficient, although I'd have to check that policy with the repo owners (cc @alexlarsson @cgwalters). (If this is really your email address then that's fine, it just looks like it might be a placeholder.) |
Yes please. Perhaps something like:
... and it can probably have Are you using bubblewrap for something? Does this change solve a problem that you were encountering? |
|
This is my real email. I use bwrap indirectly via bubblejail. I don't have any problem with it, I guess the commit probably should fix those linked issues. |
Unlike the old mount(2) API, this lets us apply the same mount flags recursively to a whole tree of mount points in a single operation, avoiding time-of-check/time-of-use race conditions if the mount table changes (containers#650) and improving performance when there are many mount points (containers#384). Fixes: containers#754 Signed-off-by: xxyzz <gitpull@protonmail.com>
5 participants
Fix #754, all tests passed. I also update some GitHub Actions.