[CRIU] [PATCH 1/4] Support for dumping/restoring user namespaces

[Andrew Vagin]

On Mon, Aug 11, 2014 at 10:49:21PM -0700, Sophie Blee-Goldman wrote:
>     For example what I see now. My version:
>     * doesn't have comments, which I enumerated for this patch
>     * handles ghost uids
>     * fixes zdtm lib to test user namespaces
>     * prepares the root mount for pivot_root
>     * set PR_SET_DUMPABLE to have access to proc files
>     * extends ZDTM lib to execute tests in userns
> 
>     Sophie's version:
>     * dumps capability from parasite. I don't know which capabilities are
>       shown in /proc/PID/status. 
> 
>     * ... 
> 
> 
>     Sophie, could you continue  this list or comment my list.
> 
> 
> I think you've listed all the major points. The thing we seem to handle
> in a significantly different way is getting the ids inside the namespace.
> I think there's something to be said for dumping those in the parasite
> alongside the capabilities, which would avoid the need to store the
> maps as a global variable (Also, it's possible I overlooked something but
> I think you may be leaking the memory malloc'ed for these maps).

Yes, they leak now. It's easy to fix.

> 
> That said, the simplicity of just calling getresuid and getresgid from the
> parasite doesn't extend to getting the ghost files' ids, since it looks like
> a lot more would have to be moved to the parasite. So maybe it's best to
> use your method and just translate the ids. On that note, one thing I noticed
> in your code is that during a dump you translate the ids before calling
> may_dump(), which could be problematic if criu is ever to support dumping
> user namespaces by non-root users. A similar issue arises for may_restore().

Good catch! I will look.

> 
> 
> 
>     {u,g}id_map files can be written only once. If you use fprintf, you
>     don't know how many times write will be called.
>     kernel/user_namespace.c:
>     static ssize_t map_write(struct file *file, const char __user *buf,
>     ...
>             ret = -EPERM;
>             /* Only allow one successful write to the map */
>             if (map->nr_extents != 0)
>                     goto out;
>     ...
> 
> My mistake, I had thought the restriction was that the id_maps could only
> be opened for writing once.
> 
> 
>     > +int dump_user_ns(int ns_pid, int ns_id)
>     > +{
>     > +     int fd, ret, n_uid_entries, n_gid_entries;
>     > +     char map_fname[PATH_MAX];
>     > +
>     > +     LIST_HEAD(uid_list);
>     Why do we need this list? Can we write uid_maps in an image without
>     collection in the list?
> 
> 
> 
> 
> I felt that reading the map entries into a list first was cleaner than reading
> them directly into the array and resizing it while parsing the uid/gid maps.
> But
> either way of doing it seems reasonable, so I wouldn't say we *need* this list.
> 
>  
> 
>     > +     /* read uid map */
>     > +     sprintf(map_fname, "/proc/%d/uid_map", ns_pid);
> 
>     can we use fopen_proc here?
> 
> 
> I hadn't noticed the 'open_proc' set of functions, but yes that would be
> better.

I think we can take first patches from my series. They are splitted a
bit better and doesn't have these comments. So the open question is how
to translate uid-s.

Thanks,
Andrew.