Re: [PATCH v2] fs: Add VirtualBox guest shared folder (vboxsf) support

From: Hans de Goede
Date: Wed Jan 17 2018 - 05:45:26 EST


Hi,

On 17-01-18 04:19, Al Viro wrote:
On Tue, Jan 16, 2018 at 11:15:15AM +0100, Hans de Goede wrote:
* your ->rename() can race with ->get_link(). Look at the place where
the former reassigns ->path and frees the old value and think what
happens if the latter is called just prior to that kfree().

* the same goes for sf_inode_revalidate() vs. rename().

* just what happens to ->path of inode when e.g. its grandparent
directory is renamed?

Can I summarize all 3 above with: Caching the path is a bad idea and
instead the code should always look up the name from e.g. file->f_path.dentry ?

Caching the path is very likely to be a bad idea. OTOH, caching conversions
of individual components, which appear to be completely independent of
any inodes, might be useful, especially if you attach the results to dentries
instead of inodes. That way you only do nls shite on lookups. Concatenation
of pieces into the pathname is probably best done later and in each case
we really want to decide what to do with racing renames of ancestors *DURING*
vboxsf_...() primitives. We can be clever and careful while building the
pathname, but what's to prevent it going stale just as we'd formed the damn
string and started to do whatever it was we'd formed it for and what happens
in case of such races?

vboxsf really works like a networkfs wrt this, for any calls to the hypervisor
taking a path (rather then a handle) if we race and we loose then the hypervisor
will simply return to use which a file-does-not-exist (at the now no longer
valid path) error.

There are 2 types of races here:
1) Racing with accesses to the shared-folder outside of the guest, there
is really nothing we can do here and in this case just reporting the error to
the higher layers is the right thing to do IMHO

2) Racing with other accesses inside the guest, so we do something which
requires a path and some of the parents may e.g. be renamed from inside the
guest while we do this. I'm tempted to just also report the error returned
by the host here. Since 1. is unavoidable anyways treating them both the
same seems easiest.

Ideally 2 should never happen at all and all calls which fall under 2
would work with a handle, but at least re-reading a dir, which we do when
a file inside it gets renamed, requires closing and opening the handle.

The call to get entries of the dir has an index argument, but the
implementation in the hypervisor has:

Assert(*pIndex == 0);

And uses hypervisor private data attached to the handle to actually
track where we are in the listing :|

* AFAICS, you consider all negative dentries invalid. Why do you even
hash them, then?

I'm afraid I'm not entirely following you. Note I've no experience with fs
code prior to this. Also I'm not the original author of this code, this
code started as part of the out-of-tree kernel modules used by the
VirtualBox guest-additions. I've been working on cleaning these modules up
and then mainlinging them (there are 3 of them, this is the last).

Note I believe that this code is based on the fs/hostfs code.

Can you reword your question keeping my lack of experience wrt fs code
in mind ?

Your ->d_revalidate() flat-out returns 0 on negative dentries. Which
means "consider them invalid when found by dcache lookup". So what's
the point of hashing them at all? Note that the right answer might
very well be "build a pathname anyway, stat the sucker and consider
ENOENT as it's still valid"...

Ah ok, so reading up a bit on this I see now that a "negative"
dentry is a dentry for an unlinked filename, so without an inode linked
to it (dentry->inode is NULL).

As you suggest building the path and check if it has not been recreated
underneath us with a stat seems like the best solution here.

Regards,

Hans