[PATCH v2 2/2] hostfs: store permissions in extended attributes

[Johannes Berg]

On Fri, 2023-04-14 at 19:19 +0200, Marko Petrović wrote:
> > > +++ b/fs/hostfs/hostfs.h
> > > @@ -37,6 +37,7 @@
> > >   * is on, and remove the appropriate bits from attr->ia_mode (attr is a
> > >   * "struct iattr *"). -BlaisorBlade
> > >   */
> > > +extern int use_xattr;
> > 
> > 
> > That seems like an odd place to put it - the comment above is surely for
> > the hostfs_timespec?
> Actually it appears to be talking about the missing defines of
> ATTR_KILL_SUID and ATTR_KILL_SGID, unrelated to the struct hostfs_timespec.
> As for why I put it there, I thought it would be nice not to mix
> function declarations with variable declarations.

Oh, OK, sorry. Maybe just add a couple of blank line around it then so
it doesn't look so grouped? :)
> > 
> > Not relevant just _yet_, but FYI, I have a patch somewhere to build the
> > _user.c part always into the kernel image, and then this needs to be
> > over there and exported for the module, I think.
> > 
> > (But I need to see what's the state of my patch)
> Ok. In that case, I can add EXPORT_SYMBOL() for these variables.

Right. FWIW, the patch series is here:
https://patchwork.ozlabs.org/project/linux-um/list/?series=341340

I hope Richard will apply these soon.

> > > @@ -578,7 +587,9 @@ static int hostfs_create(struct mnt_idmap *idmap, struct inode *dir,
> > >  	if (name == NULL)
> > >  		goto out_put;
> > >  
> > > -	fd = file_create(name, mode & 0777);
> > > +	currentuid = from_kuid(current->cred->user_ns, current->cred->euid);
> > > +	currentgid = from_kgid(current->cred->user_ns, current->cred->egid);
> > > +	fd = file_create(name, mode & 0777, currentuid, currentgid);
> > >  	if (fd < 0)
> > >  		error = fd;
> > >  	else
> > 
> > That probably somehow interacts with the idmapping, +Glenn & Christian.
> > 
> > Not saying it's wrong, but I don't understand it.

> If I understood documentation correctly, filesystems should store userspace
> ids and kernel performs translation of userspace id to kernel id when
> file is read from the disk.

Makes sense.

> On the other hand, the variables euid and egid in struct cred are of
> type kuid_t and kgid_t so this here performs reverse translation
> (from kernel id to userspace id) based on the process' current user
> namespace. Then the resulting userspace ids are sent to file_create()
> to be stored on the disk.

OK :)

> If I have made some misunderstanding and this code has any error,
> I apologise and please inform me so that I can fix it.

Oh, no, I really just don't know! Just noticed that there was other work
in this area.

> > 
> > > +
> > > +	if (use_xattr) {
> > > +		if (owner != -1) {
> > > +			status = setxattr(pathname, "user.umluid", &owner,
> > > +							sizeof(unsigned int), 0);
> > 
> > Indentation _seems_ to be off a bit here, but could be my mail client.
> I just checked, it is 82 columns wide. Sorry about that, will fix it.

Ah, that's not what I meant, I meant the indentation should be only to
just after the ( on the next line.

> Regarding nested UMLs using hostfs, a simple append of some char
> (or string?) to the attribute name when it starts with user.umlperms
> should solve the issue. Then first UML would access user.umlperms, UML
> inside it would access user.umlperms1, UML inside that UML would access
> user.umlperms11, etc. as seen by the host.

Not sure that seems like a good idea, then wouldn't you read the
permissions depending on the nesting level you're running on or
something? How would that even work?

Anyway it all doesn't work since hostfs itself doesn't seem to support
xattrs now, so you can't store xattrs in it, so only then would we have
to think about it.

And I think at that point we'd probably want to do some mangling like

	user.xyz
	security.xyz

attributes inside hostfs getting stored into

	user.uml.user.xyz
	user.uml.security.xyz

on the host filesystem, or such. And then it wouldn't conflict, and if
you nested, you'd get "user.uml.user.umlcred" (or what you chose now),
you'd just limit the nesting here on the depth of the xattr name ;)

Nothing we need to think about too much right now, I guess.

> > 
> > > +	struct stat64 buf = { 0 };
> > > +
> > > +	strcpy(parent, path);
> > > +	i = i - 3;
> > 
> > why -3?
> i is initially strlen(path) + 1, i.e. the length of the whole string
> including the null byte. Then i-1 is the index of the null byte and i-2
> is the index of the last character, but last charcter cannot be the
> slash that separates file from it's parent directory, so search starts
> from i-3.

Would be good to add a comment with that I think :)


> > > +	while (parent[i] != '/')
> > > +		i--;
> > > +	parent[i] = '\0';
> > > +	fix_path(parent);
> > > +
> > > +	stat64(parent, &buf);
> > 
> > Is it even worth doing the stat on the host? if the S_ISGID is set
> > there, it'll be done anyway by the host no? IOW, don't you only need to
> > do the handling for 'internal' xattr permissions?
> > 
> Yes, it will be done by the host but it will then be overridden by the
> permissions in xattr so UML would present wrong GID to UML processes.
> To fix that, UML does stat on the file to determine whether S_ISGID is
> set there so that xattr permissions reflect that.

Ah right, makes sense.

johannes