Message-ID: <9477e79f-266d-48ba-909f-49443fea543c@redhat.com>
Date:   Wed, 17 Jan 2018 11:45:14 +0100
From:   Hans de Goede <hdegoede@...hat.com>
To:     Al Viro <viro@...IV.linux.org.uk>
Cc:     Arnd Bergmann <arnd@...db.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Michael Thayer <michael.thayer@...cle.com>,
        Larry Finger <Larry.Finger@...inger.net>,
        Christoph Hellwig <hch@...radead.org>,
        linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH v2] fs: Add VirtualBox guest shared folder (vboxsf)
 support

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

Powered by blists - more mailing lists