[PATCH] tmpfs: allow decoding a file handle of an unlinked file

[PATCH] tmpfs: allow decoding a file handle of an unlinked file

[Amir Goldstein]

tmpfs uses the helper d_find_alias() to find a dentry from a decoded
inode, but d_find_alias() skips unhashed dentries, so unlinked files
cannot be decoded from a file handle.

This can be reproduced using xfstests test program open_by_handle:
$ open_by handle -c /tmp/testdir
$ open_by_handle -dk /tmp/testdir
open_by_handle(/tmp/testdir/file000000) returned 116 incorrectly on an
unlinked open file!

To fix this, use a variant of d_find_alias() that returns any alias,
even an unhashed one.

Cc: Hugh Dickins <hughd@google.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 mm/shmem.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

Miklos,

Please see if that patch looks correct.

Bruce and Jeff indicated that the current tmpfs behavior is not desirable
for nfsd. It may be uncommon to export a tmpfs, but it is going to become
a lot more common when exporting an overlayfs with upper tmpfs.

Thanks,
Amir.

diff --git a/mm/shmem.c b/mm/shmem.c
index 07a1d22807be..f7c555ebf0f2 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -3404,6 +3404,26 @@ static int shmem_match(struct inode *ino, void *vfh)
 	return ino->i_ino == inum && fh[0] == ino->i_generation;
 }
 
+/* Find any alias of inode, even an unhashed one */
+static struct dentry *shmem_find_alias(struct inode *inode)
+{
+	struct dentry *alias;
+
+	spin_lock(&inode->i_lock);
+	hlist_for_each_entry(alias, &inode->i_dentry, d_u.d_alias) {
+		dget(alias);
+		if (alias->d_inode == inode) {
+			spin_unlock(&inode->i_lock);
+			return alias;
+		}
+		dput(alias);
+	}
+	spin_unlock(&inode->i_lock);
+
+	return NULL;
+}
+
+
 static struct dentry *shmem_fh_to_dentry(struct super_block *sb,
 		struct fid *fid, int fh_len, int fh_type)
 {
@@ -3420,7 +3440,7 @@ static struct dentry *shmem_fh_to_dentry(struct super_block *sb,
 	inode = ilookup5(sb, (unsigned long)(inum + fid->raw[0]),
 			shmem_match, fid->raw);
 	if (inode) {
-		dentry = d_find_alias(inode);
+		dentry = shmem_find_alias(inode);
 		iput(inode);
 	}
 
-- 
2.7.4

Re: [PATCH] tmpfs: allow decoding a file handle of an unlinked file

[James Bottomley]

On Tue, 2017-11-07 at 22:50 +0200, Amir Goldstein wrote:
> tmpfs uses the helper d_find_alias() to find a dentry from a decoded
> inode, but d_find_alias() skips unhashed dentries, so unlinked files
> cannot be decoded from a file handle.
> 
> This can be reproduced using xfstests test program open_by_handle:
> $ open_by handle -c /tmp/testdir
> $ open_by_handle -dk /tmp/testdir
> open_by_handle(/tmp/testdir/file000000) returned 116 incorrectly on
> an
> unlinked open file!
> 
> To fix this, use a variant of d_find_alias() that returns any alias,
> even an unhashed one.
> 
> Cc: Hugh Dickins <hughd@google.com>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  mm/shmem.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> Miklos,
> 
> Please see if that patch looks correct.
> 
> Bruce and Jeff indicated that the current tmpfs behavior is not
> desirable
> for nfsd. It may be uncommon to export a tmpfs, but it is going to
> become
> a lot more common when exporting an overlayfs with upper tmpfs.
> 
> Thanks,
> Amir.
> 
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 07a1d22807be..f7c555ebf0f2 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -3404,6 +3404,26 @@ static int shmem_match(struct inode *ino, void
> *vfh)
>  	return ino->i_ino == inum && fh[0] == ino->i_generation;
>  }
>  
> +/* Find any alias of inode, even an unhashed one */
> +static struct dentry *shmem_find_alias(struct inode *inode)
> +{
> +	struct dentry *alias;
> +
> +	spin_lock(&inode->i_lock);
> +	hlist_for_each_entry(alias, &inode->i_dentry, d_u.d_alias) {
> +		dget(alias);
> +		if (alias->d_inode == inode) {
> +			spin_unlock(&inode->i_lock);
> +			return alias;
> +		}
> +		dput(alias);
> +	}
> +	spin_unlock(&inode->i_lock);
> +
> +	return NULL;
> +}

This doesn't look right in the case of a multiply linked inode for
which you've removing some of the link names because it will return the
first alias it finds, which may be unhashed.  Isn't what you want for
it to return the first hashed alias if one exists, or the first
unhashed one if none do, so this code

> @@ -3420,7 +3440,7 @@ static struct dentry *shmem_fh_to_dentry(struct
> super_block *sb,
>         inode = ilookup5(sb, (unsigned long)(inum + fid->raw[0]),
>                         shmem_match, fid->raw);
>         if (inode) {
> -               dentry = d_find_alias(inode);
> +               dentry = shmem_find_alias(inode);
>                 iput(inode);
>         } 


Should actually be

if (inode) {
	dentry = d_find_alias(inode);
	if (!dentry)
		dentry = shmem_find_alias(inode);
	iput(inode)
}

?

James

Re: [PATCH] tmpfs: allow decoding a file handle of an unlinked file

[Amir Goldstein]

On Tue, Nov 7, 2017 at 10:58 PM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> On Tue, 2017-11-07 at 22:50 +0200, Amir Goldstein wrote:
>> tmpfs uses the helper d_find_alias() to find a dentry from a decoded
>> inode, but d_find_alias() skips unhashed dentries, so unlinked files
>> cannot be decoded from a file handle.
>>
>> This can be reproduced using xfstests test program open_by_handle:
>> $ open_by handle -c /tmp/testdir
>> $ open_by_handle -dk /tmp/testdir
>> open_by_handle(/tmp/testdir/file000000) returned 116 incorrectly on
>> an
>> unlinked open file!
>>
>> To fix this, use a variant of d_find_alias() that returns any alias,
>> even an unhashed one.
>>
>> Cc: Hugh Dickins <hughd@google.com>
>> Cc: Al Viro <viro@zeniv.linux.org.uk>
>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>> ---
>>  mm/shmem.c | 22 +++++++++++++++++++++-
>>  1 file changed, 21 insertions(+), 1 deletion(-)
>>
>> Miklos,
>>
>> Please see if that patch looks correct.
>>
>> Bruce and Jeff indicated that the current tmpfs behavior is not
>> desirable
>> for nfsd. It may be uncommon to export a tmpfs, but it is going to
>> become
>> a lot more common when exporting an overlayfs with upper tmpfs.
>>
>> Thanks,
>> Amir.
>>
>> diff --git a/mm/shmem.c b/mm/shmem.c
>> index 07a1d22807be..f7c555ebf0f2 100644
>> --- a/mm/shmem.c
>> +++ b/mm/shmem.c
>> @@ -3404,6 +3404,26 @@ static int shmem_match(struct inode *ino, void
>> *vfh)
>>       return ino->i_ino == inum && fh[0] == ino->i_generation;
>>  }
>>
>> +/* Find any alias of inode, even an unhashed one */
>> +static struct dentry *shmem_find_alias(struct inode *inode)
>> +{
>> +     struct dentry *alias;
>> +
>> +     spin_lock(&inode->i_lock);
>> +     hlist_for_each_entry(alias, &inode->i_dentry, d_u.d_alias) {
>> +             dget(alias);
>> +             if (alias->d_inode == inode) {
>> +                     spin_unlock(&inode->i_lock);
>> +                     return alias;
>> +             }
>> +             dput(alias);
>> +     }
>> +     spin_unlock(&inode->i_lock);
>> +
>> +     return NULL;
>> +}
>
> This doesn't look right in the case of a multiply linked inode for
> which you've removing some of the link names because it will return the
> first alias it finds, which may be unhashed.  Isn't what you want for
> it to return the first hashed alias if one exists, or the first
> unhashed one if none do, so this code
>
>> @@ -3420,7 +3440,7 @@ static struct dentry *shmem_fh_to_dentry(struct
>> super_block *sb,
>>         inode = ilookup5(sb, (unsigned long)(inum + fid->raw[0]),
>>                         shmem_match, fid->raw);
>>         if (inode) {
>> -               dentry = d_find_alias(inode);
>> +               dentry = shmem_find_alias(inode);
>>                 iput(inode);
>>         }
>
>
> Should actually be
>
> if (inode) {
>         dentry = d_find_alias(inode);
>         if (!dentry)
>                 dentry = shmem_find_alias(inode);
>         iput(inode)
> }
>
> ?
>


Perhaps, but I don't think that matters to nfsd. ??
I think nfsd is going to use that alias only for accessing the inode anyway.
tmpfs doesn't implement get_parent() for reconnect of non-dir, and for
directory, there is only one alias and d_find_alias() returns it even if it
is unhashed.

Amir.