Re: [PATCH v10 0/6] fstests: add idmapped mounts tests

[Christian Brauner <christian.brauner@ubuntu.com>]

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@gmail.com> wrote:
> >
> > On Wed, Mar 24, 2021 at 10:41 AM Christian Brauner
> > <christian.brauner@ubuntu.com> 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@ubuntu.com> 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@ubuntu.com>
> > > > > > >
> > > > > > > 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