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

[Marko Petrović]

On Tue Apr 18, 2023 at 10:26 AM CEST, Johannes Berg wrote:
Hello,
Thank you for your review!
> 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? :)
Ok. Will add them. :)

> > 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?
Yes, that was the idea, kind of. It seemed like a good idea for each UML
on each nesting level to have it's own permission attributes and not
mess with host's permissions.
>
> 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.
If I understood correctly, that also accomplishes more or less the same
thing, it just prepends the "user.uml." string to the original attribute
instead of appending some char so there would be user.umlcred for the
first UML, user.uml.user.umlcred for the second UML,
user.uml.user.uml.user.umlcred for the third UML, etc.

Apart from longer names, it seems fine. In any case, as you said, we will
think this through when hostfs gets extended attributes support.

> > > 
> > > > +	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 :)
>
Ok. Will add it.

Best Regards,
Marko Petrović