On Wed, Mar 24, 2021 at 03:33:53PM +0200, Amir Goldstein wrote: > On Wed, Mar 24, 2021 at 1:24 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > > > On Wed, Mar 24, 2021 at 10:41 AM Christian Brauner > > <christian.brauner@xxxxxxxxxx> wrote: > > > > > > On Wed, Mar 24, 2021 at 10:03:43AM +0200, Amir Goldstein wrote: > > > > On Mon, Mar 22, 2021 at 4:22 PM Christian Brauner > > > > <christian.brauner@xxxxxxxxxx> wrote: > > > > > > > > > > On Mon, Mar 22, 2021 at 02:50:02PM +0100, Christian Brauner wrote: > > > > > > On Mon, Mar 22, 2021 at 02:45:16PM +0100, Christian Brauner wrote: > > > > > > > From: Christian Brauner <christian.brauner@xxxxxxxxxx> > > > > > > > > > > > > > > Hey everyone, > > > > > > > > > > > > > > This series is available from: > > > > > > > https://git.kernel.org/brauner/xfstests-dev/h/idmapped_mounts > > > > > > > https://gitlab.com/brauner/xfstests/-/tree/idmapped_mounts > > > > > > > https://github.com/brauner/xfstests/tree/idmapped_mounts > > > > > > > > > > > > I sent this series from my kernel.org mail address and patch 2/6 hasn't > > > > > > made it through this time too. So it seems that vger is rejecting it due > > > > > > to its size is my guess. I'll go poing the #korg folks to ask what's > > > > > > going on and whether that can be handled. > > > > > > > > > > Ok, Konstantin confirmed that patch 2/6 got dropped because of it's > > > > > size. Nothing to do about this now but just as an fyi. > > > > > > > > > > > > > Since 2/6 got dropped, I'll write a small nit here which is also > > > > > > You could still pull it from above. I don't think resending would retain > > > the patch afaict until vger has been ported by Konstantin. > > > > > > > relevant to the rest of the series: > > > > > > > > --- a/tests/generic/group > > > > +++ b/tests/generic/group > > > > @@ -634,3 +634,4 @@ > > > > 629 auto quick rw copy_range > > > > 630 auto quick rw dedupe clone > > > > 631 auto quick mount > > > > +632 auto atime attr cap io_uring mount perms quick rw unlink > > > > > > > > This is a mouthful of test tags, but that doesn't hurt. > > > > I would personally not bother with obscure tags like 'unlink' but whatever. > > > > > > > > Two things I would request are: > > > > 1. Keep 'auto quick' before all other tags. There is no strong rule about > > > > this format, but that's the common practice and it makes sense IMO > > > > because -g auto and -g quick are the far more commonly used groups of > > > > tests, so it's convenient to be able to 'eye grep' those tests in > > > > the group file. > > > > 2. Please tags all the idmapped tests with a tag 'idmapped' (or whatever) > > > > This would be used for running tests with -g idmapped for quick sanity > > > > when modifiying idmapped mounts related code > > > > > > Ok, I'll wait for Eryu to respond (Since I assume he'll be the one in > > > charge of applying it?) > > > > He will. > > > > Meanwhile, found a bug in Makefile: > > > > --- a/src/idmapped-mounts/Makefile > > +++ b/src/idmapped-mounts/Makefile > > @@ -25,11 +25,11 @@ depend: .dep > > > > include $(BUILDRULES) > > > > -idmapped-mounts: > > +idmapped-mounts: $(CFILES_IDMAPPED_MOUNTS) > > @echo " [CC] $@" > > $(Q)$(LTLINK) $(CFILES_IDMAPPED_MOUNTS) -o $@ $(CFLAGS) > > $(LDFLAGS) $(LDLIBS) > > > > -mount-idmapped: > > +mount-idmapped: $(CFILES_MOUNT_IDMAPPED) > > @echo " [CC] $@" > > $(Q)$(LTLINK) $(CFILES_MOUNT_IDMAPPED) -o $@ $(CFLAGS) > > $(LDFLAGS) $(LDLIBS) > > --- > > > > And in parsing of /proc/<pid>/ns/user: > > > > --- a/src/idmapped-mounts/mount-idmapped.c > > +++ b/src/idmapped-mounts/mount-idmapped.c > > @@ -175,7 +175,7 @@ static int write_id_mapping(idmap_type_t map_type, > > pid_t pid, const char *buf, s > > int fd = -EBADF, setgroups_fd = -EBADF; > > int fret = -1; > > int ret; > > - char path[STRLITERALLEN("/proc") + INTTYPE_TO_STRLEN(pid_t) + > > + char path[STRLITERALLEN("/proc/") + INTTYPE_TO_STRLEN(pid_t) + > > STRLITERALLEN("/setgroups") + 1]; > > > > if (geteuid() != 0 && map_type == ID_TYPE_GID) { > > @@ -273,7 +273,7 @@ static int get_userns_fd_from_idmap(struct list *idmap) > > { > > int ret; > > pid_t pid; > > - char path_ns[STRLITERALLEN("/proc") + INTTYPE_TO_STRLEN(pid_t) + > > + char path_ns[STRLITERALLEN("/proc/") + INTTYPE_TO_STRLEN(pid_t) + > > STRLITERALLEN("/ns/user") + 1]; > > > > pid = do_clone(get_userns_fd_cb, NULL, CLONE_NEWUSER | CLONE_NEWNS); > > @@ -364,7 +364,7 @@ int main(int argc, char *argv[]) > > while ((ret = getopt_long_only(argc, argv, "", longopts, > > &index)) != -1) { > > switch (ret) { > > case 'a': > > - if (strnequal(optarg, "/proc", > > STRLITERALLEN("/proc/"))) { > > + if (strnequal(optarg, "/proc/", > > STRLITERALLEN("/proc/"))) { > > fd_userns = open(optarg, O_RDONLY | O_CLOEXEC); > > --- > > > > And also: > > @@ -402,12 +402,15 @@ int main(int argc, char *argv[]) > exit(EXIT_FAILURE); > } > > - if (!list_empty(&active_map)) { > + if (!list_empty(&active_map) || fd_userns > 0) { > struct mount_attr attr = { > .attr_set = MOUNT_ATTR_IDMAP, > }; > > - attr.userns_fd = get_userns_fd_from_idmap(&active_map); > + if (fd_userns > 0) > + attr.userns_fd = fd_userns; > + else > + attr.userns_fd = get_userns_fd_from_idmap(&active_map); > if (attr.userns_fd < 0) > exit_log("%m - Failed to create user namespace\n"); > --- > > It's a bug in the test helper program, not a bug in the test per-se because > the test does not use the > --map-mount=/proc/<pid>/ns/user option. Thanks! Will fix and also re-order the patches. Christian
