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

[Al Viro <viro@ZenIV.linux.org.uk>]

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?

> > * 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"...