Re: [PATCH] virtiofs: Enable SB_NOSEC flag to improve small write performance

[Miklos Szeredi <miklos@szeredi.hu>]

On Mon, Jul 20, 2020 at 5:41 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Fri, Jul 17, 2020 at 10:53:07AM +0200, Miklos Szeredi wrote:

> I see in VFS that chown() always kills suid/sgid. While truncate() and
> write(), will suid/sgid only if caller does not have CAP_FSETID.
>
> How does this work with FUSE_HANDLE_KILLPRIV. IIUC, file server does not
> know if caller has CAP_FSETID or not. That means file server will be
> forced to kill suid/sgid on every write and truncate. And that will fail
> some of the tests.
>
> For WRITE requests now we do have the notion of setting
> FUSE_WRITE_KILL_PRIV flag to tell server explicitly to kill suid/sgid.
> Probably we could use that in cached write path as well to figure out
> whether to kill suid/sgid or not. But truncate() will still continue
> to be an issue.

Yes, not doing the same for truncate seems to be an oversight.
Unfortunate, since we'll need another INIT flag to enable selective
clearing of suid/sgid on truncate.

>
> >
> > Even writeback_cache could be handled by this addition, since we call
> > fuse_update_attributes() before generic_file_write_iter() :
> >
> > --- a/fs/fuse/dir.c
> > +++ b/fs/fuse/dir.c
> > @@ -985,6 +985,7 @@ static int fuse_update_get_attr(struct inode
> > *inode, struct file *file,
> >
> >         if (sync) {
> >                 forget_all_cached_acls(inode);
> > +               inode->i_flags &= ~S_NOSEC;
>
> Ok, So I was clearing S_NOSEC only if server reports that file has
> suid/sgid bit set. This change will clear S_NOSEC whenever we fetch
> attrs from host and will force getxattr() when we call file_remove_privs()
> and will increase overhead for non cache writeback mode. We probably
> could keep both. For cache writeback mode, clear it undonditionally
> otherwise not.

We clear S_NOSEC because the attribute timeout has expired.  This
means we need to refresh all metadata, including cached xattr (which
is what S_NOSEC effectively is).

> What I don't understand is though that how this change will clear
> suid/sgid on host in cache=writeback mode. I see fuse_setattr()
> will not set ATTR_MODE and clear S_ISUID and S_ISGID if
> fc->handle_killpriv is set. So when server receives setattr request
> (if it does), then how will it know it is supposed to kill suid/sgid
> bit. (its not chown, truncate and its not write).

Depends.  If the attribute timeout is infinity, then that means the
cache is always up to date.  In that case we only need to clear
suid/sgid if set in i_mode.  Similarly, the security.capability will
only be cleared if it was set in the first place (which would clear
S_NOSEC).

If the timeout is finite, then that means we need to check if the
metadata changed after a timeout.  That's the purpose of the
fuse_update_attributes() call before generic_file_write_iter().

Does that make it clear?

Thanks,
Miklos