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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 17 Jan 2018 03:19:10 +0000
From:   Al Viro <viro@...IV.linux.org.uk>
To:     Hans de Goede <hdegoede@...hat.com>
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

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