linux-kernel - Re: [NFS] [PATCH 001 of 8] knfsd: Add nfs-export support to tmpfs

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 for Android: free password hash cracker in your pocket

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

Message-ID: <17692.49605.248998.607609@cse.unsw.edu.au>
Date:	Fri, 29 Sep 2006 16:48:37 +1000
From:	Neil Brown <neilb@...e.de>
To:	Andrew Morton <akpm@...l.org>
Cc:	"David M. Grimes" <dgrimes@...isite.com>,
	Atal Shargorodsky <atal@...efidence.com>,
	Gilad Ben-Yossef <gilad@...efidence.com>,
	Hugh Dickins <hugh@...itas.com>, nfs@...ts.sourceforge.net,
	linux-kernel@...r.kernel.org
Subject: Re: [NFS] [PATCH 001 of 8] knfsd: Add nfs-export support to tmpfs

On Thursday September 28, akpm@...l.org wrote:
> On Fri, 29 Sep 2006 13:08:39 +1000
> NeilBrown <neilb@...e.de> wrote:
> 
> > +static int shmem_encode_fh(struct dentry *dentry, __u32 *fh, int *len, int connectable)
> > +{
> > +	struct inode *inode = dentry->d_inode;
> > +
> > +	if (*len < 2)
> > +		return 255;
> > +
> > +	if (hlist_unhashed(&inode->i_hash)) {
> > +		/* Unfortunately insert_inode_hash is not idempotent,
> > +		 * so as we hash inodes here rather than at creation
> > +		 * time, we need a lock to ensure we only try
> > +		 * to do it once
> > +		 */
> > +		static DEFINE_SPINLOCK(lock);
> > +		spin_lock(&lock);
> > +		if (hlist_unhashed(&inode->i_hash))
> > +			insert_inode_hash(inode);
> > +		spin_unlock(&lock);
> > +	}
> 
> This looks fishy.
> 
> How do we get two callers in here at the same time for the same inode?

Probably not very easily.
But imagine a file has two hard links in different directories.
And two clients issue LOOKUP requests, one for each link.
They could conceivably be processed at exactly the same time and so
shmem_encode_fh could be running on two different CPU's at the same
time for the same inode.

> 
> Why don't other filesystems have the same problem?
> 

Because most filesystems that hash their inodes do so at the point
where the 'struct inode' is initialised, and that has suitable locking
(I_NEW).  Here in shmem, we are hashing the inode later, the first
time we need an NFS file handle for it.  We no longer have I_NEW to
ensure only one thread tries to add it to the hash table.

The comment tries to explain this, but obviously isn't completely
successful.

NeilBrown
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/