diff mbox

[1/4] fs/notify: fdinfo can report unsupported file handles.

Message ID 151297224512.7818.18333908109878259066.stgit@noble
State New, archived
Headers show

Commit Message

NeilBrown Dec. 11, 2017, 6:04 a.m. UTC
If a filesystem does not set sb->s_export_op, then it
does not support filehandles and export_fs_encode_fh()
and exportfs_encode_inode_fh() should not be called.
They will use export_encode_fh() is which is a default
that uses inode number generation number, but in general
they may not be stable.

So change exportfs_encode_inode_fh() to return FILEID_INVALID
if called on an unsupported Filesystem.  Currently only
notify/fdinfo can do that.

Also remove the WARNing from fdinfo when exportfs_encode_inode_fh()
returns FILEID_INVALID, as that is no an erroneous condition.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 fs/exportfs/expfs.c |    4 +++-
 fs/notify/fdinfo.c  |    4 +---
 2 files changed, 4 insertions(+), 4 deletions(-)

Comments

Al Viro Dec. 11, 2017, 6:29 a.m. UTC | #1
On Mon, Dec 11, 2017 at 05:04:05PM +1100, NeilBrown wrote:
> @@ -385,7 +385,9 @@ int exportfs_encode_inode_fh(struct inode *inode, struct fid *fid,
>  {
>  	const struct export_operations *nop = inode->i_sb->s_export_op;
>  
> -	if (nop && nop->encode_fh)
> +	if (nop)
> +		return FILEID_INVALID;
> +	if (nop->encode_fh)
>  		return nop->encode_fh(inode, fid->raw, max_len, parent);

This might as well have been

	if (nop)
		return FILEID_INVALID;
	BUG();

Have you ever tested that?
Amir Goldstein Dec. 11, 2017, 6:41 a.m. UTC | #2
On Mon, Dec 11, 2017 at 8:04 AM, NeilBrown <neilb@suse.com> wrote:
> If a filesystem does not set sb->s_export_op, then it
> does not support filehandles and export_fs_encode_fh()
> and exportfs_encode_inode_fh() should not be called.
> They will use export_encode_fh() is which is a default
> that uses inode number generation number, but in general
> they may not be stable.
>
> So change exportfs_encode_inode_fh() to return FILEID_INVALID
> if called on an unsupported Filesystem.  Currently only
> notify/fdinfo can do that.
>

I wish you would leave this check to the caller, maybe add a helper
exportfs_can_decode_fh() for callers to use.

Although there are no current uses for it in-tree, there is value in
being able to encode a unique file handle even when it cannot be
decoded back to an open file.

I am using this property in my fanotify super block watch patches,
where the object identifier on the event is an encoded file handle
of the object, which delegates tracking filesystem objects to
userspace and prevents fanotify from keeping elevated refcounts
on inodes and dentries.

There are quite a few userspace tools out there that are checking
that st_ino hasn't changed on a file between non atomic operations.
Those tools (or others) could benefit from a unique file handle if
we ever decide to provide a relaxed version of name_to_handle_at().

Cheers,
Amir.
Amir Goldstein Dec. 11, 2017, 7:05 a.m. UTC | #3
On Mon, Dec 11, 2017 at 8:41 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Mon, Dec 11, 2017 at 8:04 AM, NeilBrown <neilb@suse.com> wrote:
>> If a filesystem does not set sb->s_export_op, then it
>> does not support filehandles and export_fs_encode_fh()
>> and exportfs_encode_inode_fh() should not be called.
>> They will use export_encode_fh() is which is a default
>> that uses inode number generation number, but in general
>> they may not be stable.
>>
>> So change exportfs_encode_inode_fh() to return FILEID_INVALID
>> if called on an unsupported Filesystem.  Currently only
>> notify/fdinfo can do that.
>>
>
> I wish you would leave this check to the caller, maybe add a helper
> exportfs_can_decode_fh() for callers to use.
>
> Although there are no current uses for it in-tree, there is value in
> being able to encode a unique file handle even when it cannot be
> decoded back to an open file.
>
> I am using this property in my fanotify super block watch patches,
> where the object identifier on the event is an encoded file handle
> of the object, which delegates tracking filesystem objects to
> userspace and prevents fanotify from keeping elevated refcounts
> on inodes and dentries.
>
> There are quite a few userspace tools out there that are checking
> that st_ino hasn't changed on a file between non atomic operations.
> Those tools (or others) could benefit from a unique file handle if
> we ever decide to provide a relaxed version of name_to_handle_at().
>

And this change need a clause about not breaking userspace.

Pavel,

Will this break any version of criu in the wild?

Amir.
Pavel Emelyanov Dec. 11, 2017, 1:46 p.m. UTC | #4
On 12/11/2017 10:05 AM, Amir Goldstein wrote:
> On Mon, Dec 11, 2017 at 8:41 AM, Amir Goldstein <amir73il@gmail.com> wrote:
>> On Mon, Dec 11, 2017 at 8:04 AM, NeilBrown <neilb@suse.com> wrote:
>>> If a filesystem does not set sb->s_export_op, then it
>>> does not support filehandles and export_fs_encode_fh()
>>> and exportfs_encode_inode_fh() should not be called.
>>> They will use export_encode_fh() is which is a default
>>> that uses inode number generation number, but in general
>>> they may not be stable.
>>>
>>> So change exportfs_encode_inode_fh() to return FILEID_INVALID
>>> if called on an unsupported Filesystem.  Currently only
>>> notify/fdinfo can do that.
>>>
>>
>> I wish you would leave this check to the caller, maybe add a helper
>> exportfs_can_decode_fh() for callers to use.
>>
>> Although there are no current uses for it in-tree, there is value in
>> being able to encode a unique file handle even when it cannot be
>> decoded back to an open file.
>>
>> I am using this property in my fanotify super block watch patches,
>> where the object identifier on the event is an encoded file handle
>> of the object, which delegates tracking filesystem objects to
>> userspace and prevents fanotify from keeping elevated refcounts
>> on inodes and dentries.
>>
>> There are quite a few userspace tools out there that are checking
>> that st_ino hasn't changed on a file between non atomic operations.
>> Those tools (or others) could benefit from a unique file handle if
>> we ever decide to provide a relaxed version of name_to_handle_at().
>>
> 
> And this change need a clause about not breaking userspace.
> 
> Pavel,
> 
> Will this break any version of criu in the wild?

If there's no fliehandle in the output, it will make dump fail, but we're
already prepared for the fact, that there's no handle at hands. In the
worst case criu will exit with error.

I also agree that it should only happen when current is OOM killed, and in
case of CRIU this means killing criu process itself.

-- Pavel
Amir Goldstein Dec. 11, 2017, 2:08 p.m. UTC | #5
On Mon, Dec 11, 2017 at 3:46 PM, Pavel Emelyanov <xemul@virtuozzo.com> wrote:
> On 12/11/2017 10:05 AM, Amir Goldstein wrote:
>> On Mon, Dec 11, 2017 at 8:41 AM, Amir Goldstein <amir73il@gmail.com> wrote:
>>> On Mon, Dec 11, 2017 at 8:04 AM, NeilBrown <neilb@suse.com> wrote:
>>>> If a filesystem does not set sb->s_export_op, then it
>>>> does not support filehandles and export_fs_encode_fh()
>>>> and exportfs_encode_inode_fh() should not be called.
>>>> They will use export_encode_fh() is which is a default
>>>> that uses inode number generation number, but in general
>>>> they may not be stable.
>>>>
>>>> So change exportfs_encode_inode_fh() to return FILEID_INVALID
>>>> if called on an unsupported Filesystem.  Currently only
>>>> notify/fdinfo can do that.
>>>>
>>>
>>> I wish you would leave this check to the caller, maybe add a helper
>>> exportfs_can_decode_fh() for callers to use.
>>>
>>> Although there are no current uses for it in-tree, there is value in
>>> being able to encode a unique file handle even when it cannot be
>>> decoded back to an open file.
>>>
>>> I am using this property in my fanotify super block watch patches,
>>> where the object identifier on the event is an encoded file handle
>>> of the object, which delegates tracking filesystem objects to
>>> userspace and prevents fanotify from keeping elevated refcounts
>>> on inodes and dentries.
>>>
>>> There are quite a few userspace tools out there that are checking
>>> that st_ino hasn't changed on a file between non atomic operations.
>>> Those tools (or others) could benefit from a unique file handle if
>>> we ever decide to provide a relaxed version of name_to_handle_at().
>>>
>>
>> And this change need a clause about not breaking userspace.
>>
>> Pavel,
>>
>> Will this break any version of criu in the wild?
>
> If there's no fliehandle in the output, it will make dump fail, but we're
> already prepared for the fact, that there's no handle at hands. In the
> worst case criu will exit with error.
>
> I also agree that it should only happen when current is OOM killed, and in
> case of CRIU this means killing criu process itself.
>

But this patch [1/4] changes behavior so you cannot dump fsnotify
state if watched file system does not support *decoding* file handles.
This means that criu anyway won't be able to restore the fsnotify state.
Is it OK that criu dump state will fail in that case?

Amir.
Pavel Emelyanov Dec. 11, 2017, 3:21 p.m. UTC | #6
On 12/11/2017 05:08 PM, Amir Goldstein wrote:
> On Mon, Dec 11, 2017 at 3:46 PM, Pavel Emelyanov <xemul@virtuozzo.com> wrote:
>> On 12/11/2017 10:05 AM, Amir Goldstein wrote:
>>> On Mon, Dec 11, 2017 at 8:41 AM, Amir Goldstein <amir73il@gmail.com> wrote:
>>>> On Mon, Dec 11, 2017 at 8:04 AM, NeilBrown <neilb@suse.com> wrote:
>>>>> If a filesystem does not set sb->s_export_op, then it
>>>>> does not support filehandles and export_fs_encode_fh()
>>>>> and exportfs_encode_inode_fh() should not be called.
>>>>> They will use export_encode_fh() is which is a default
>>>>> that uses inode number generation number, but in general
>>>>> they may not be stable.
>>>>>
>>>>> So change exportfs_encode_inode_fh() to return FILEID_INVALID
>>>>> if called on an unsupported Filesystem.  Currently only
>>>>> notify/fdinfo can do that.
>>>>>
>>>>
>>>> I wish you would leave this check to the caller, maybe add a helper
>>>> exportfs_can_decode_fh() for callers to use.
>>>>
>>>> Although there are no current uses for it in-tree, there is value in
>>>> being able to encode a unique file handle even when it cannot be
>>>> decoded back to an open file.
>>>>
>>>> I am using this property in my fanotify super block watch patches,
>>>> where the object identifier on the event is an encoded file handle
>>>> of the object, which delegates tracking filesystem objects to
>>>> userspace and prevents fanotify from keeping elevated refcounts
>>>> on inodes and dentries.
>>>>
>>>> There are quite a few userspace tools out there that are checking
>>>> that st_ino hasn't changed on a file between non atomic operations.
>>>> Those tools (or others) could benefit from a unique file handle if
>>>> we ever decide to provide a relaxed version of name_to_handle_at().
>>>>
>>>
>>> And this change need a clause about not breaking userspace.
>>>
>>> Pavel,
>>>
>>> Will this break any version of criu in the wild?
>>
>> If there's no fliehandle in the output, it will make dump fail, but we're
>> already prepared for the fact, that there's no handle at hands. In the
>> worst case criu will exit with error.
>>
>> I also agree that it should only happen when current is OOM killed, and in
>> case of CRIU this means killing criu process itself.
>>
> 
> But this patch [1/4] changes behavior so you cannot dump fsnotify
> state if watched file system does not support *decoding* file handles.

That's OK :) After we get a filehandle we check it's accessible, so for
FSs that couldn't decode one, we'd fail the dump anyway.

> This means that criu anyway won't be able to restore the fsnotify state.
> Is it OK that criu dump state will fail in that case?
> 
> Amir.
> .
>
NeilBrown Dec. 11, 2017, 9:52 p.m. UTC | #7
On Mon, Dec 11 2017, Amir Goldstein wrote:

> On Mon, Dec 11, 2017 at 8:04 AM, NeilBrown <neilb@suse.com> wrote:
>> If a filesystem does not set sb->s_export_op, then it
>> does not support filehandles and export_fs_encode_fh()
>> and exportfs_encode_inode_fh() should not be called.
>> They will use export_encode_fh() is which is a default
>> that uses inode number generation number, but in general
>> they may not be stable.
>>
>> So change exportfs_encode_inode_fh() to return FILEID_INVALID
>> if called on an unsupported Filesystem.  Currently only
>> notify/fdinfo can do that.
>>
>
> I wish you would leave this check to the caller, maybe add a helper
> exportfs_can_decode_fh() for callers to use.
>
> Although there are no current uses for it in-tree, there is value in
> being able to encode a unique file handle even when it cannot be
> decoded back to an open file.
>
> I am using this property in my fanotify super block watch patches,
> where the object identifier on the event is an encoded file handle
> of the object, which delegates tracking filesystem objects to
> userspace and prevents fanotify from keeping elevated refcounts
> on inodes and dentries.
>
> There are quite a few userspace tools out there that are checking
> that st_ino hasn't changed on a file between non atomic operations.
> Those tools (or others) could benefit from a unique file handle if
> we ever decide to provide a relaxed version of name_to_handle_at().

If the filesystem doesn't define ->s_export_op, then you really cannot
trust anything beyond the inode number (and maybe not even that), and
the inode number is already easily available.
What actual value do you think you get from this pretend-file-handle
on filesystems that don't support file handles?

If there is a demonstrated need for some sort of identifier that is
stronger than an inode number, but not strong enough for
open_by_handle_at(), then you should explain that need and propose a
well defined interface.  You shouldn't use a back-door and hope no-one
notices.

Thanks,
NeilBrown
Amir Goldstein Dec. 12, 2017, 6:39 a.m. UTC | #8
On Mon, Dec 11, 2017 at 11:52 PM, NeilBrown <neilb@suse.com> wrote:
> On Mon, Dec 11 2017, Amir Goldstein wrote:
>
>> On Mon, Dec 11, 2017 at 8:04 AM, NeilBrown <neilb@suse.com> wrote:
>>> If a filesystem does not set sb->s_export_op, then it
>>> does not support filehandles and export_fs_encode_fh()
>>> and exportfs_encode_inode_fh() should not be called.
>>> They will use export_encode_fh() is which is a default
>>> that uses inode number generation number, but in general
>>> they may not be stable.
>>>
>>> So change exportfs_encode_inode_fh() to return FILEID_INVALID
>>> if called on an unsupported Filesystem.  Currently only
>>> notify/fdinfo can do that.
>>>
>>
>> I wish you would leave this check to the caller, maybe add a helper
>> exportfs_can_decode_fh() for callers to use.
>>
>> Although there are no current uses for it in-tree, there is value in
>> being able to encode a unique file handle even when it cannot be
>> decoded back to an open file.
>>
>> I am using this property in my fanotify super block watch patches,
>> where the object identifier on the event is an encoded file handle
>> of the object, which delegates tracking filesystem objects to
>> userspace and prevents fanotify from keeping elevated refcounts
>> on inodes and dentries.
>>
>> There are quite a few userspace tools out there that are checking
>> that st_ino hasn't changed on a file between non atomic operations.
>> Those tools (or others) could benefit from a unique file handle if
>> we ever decide to provide a relaxed version of name_to_handle_at().
>
> If the filesystem doesn't define ->s_export_op, then you really cannot
> trust anything beyond the inode number (and maybe not even that), and
> the inode number is already easily available.
> What actual value do you think you get from this pretend-file-handle
> on filesystems that don't support file handles?
>

Sorry, I misread your patch. In my mind I thought you wanted to
eliminate the default export_encode_fh if there was no fh_to_dentry
operation like do_sys_name_to_handle() does. Just in my head...

FWIW, according to Pavel, if fdinfo would not export file handle
in case !fh_to_dentry op would probably be desirable, because
criu has no need for file handles that cannot be decoded.

Amir.
NeilBrown Dec. 13, 2017, 2:20 a.m. UTC | #9
On Tue, Dec 12 2017, Amir Goldstein wrote:

> On Mon, Dec 11, 2017 at 11:52 PM, NeilBrown <neilb@suse.com> wrote:
>> On Mon, Dec 11 2017, Amir Goldstein wrote:
>>
>>> On Mon, Dec 11, 2017 at 8:04 AM, NeilBrown <neilb@suse.com> wrote:
>>>> If a filesystem does not set sb->s_export_op, then it
>>>> does not support filehandles and export_fs_encode_fh()
>>>> and exportfs_encode_inode_fh() should not be called.
>>>> They will use export_encode_fh() is which is a default
>>>> that uses inode number generation number, but in general
>>>> they may not be stable.
>>>>
>>>> So change exportfs_encode_inode_fh() to return FILEID_INVALID
>>>> if called on an unsupported Filesystem.  Currently only
>>>> notify/fdinfo can do that.
>>>>
>>>
>>> I wish you would leave this check to the caller, maybe add a helper
>>> exportfs_can_decode_fh() for callers to use.
>>>
>>> Although there are no current uses for it in-tree, there is value in
>>> being able to encode a unique file handle even when it cannot be
>>> decoded back to an open file.
>>>
>>> I am using this property in my fanotify super block watch patches,
>>> where the object identifier on the event is an encoded file handle
>>> of the object, which delegates tracking filesystem objects to
>>> userspace and prevents fanotify from keeping elevated refcounts
>>> on inodes and dentries.
>>>
>>> There are quite a few userspace tools out there that are checking
>>> that st_ino hasn't changed on a file between non atomic operations.
>>> Those tools (or others) could benefit from a unique file handle if
>>> we ever decide to provide a relaxed version of name_to_handle_at().
>>
>> If the filesystem doesn't define ->s_export_op, then you really cannot
>> trust anything beyond the inode number (and maybe not even that), and
>> the inode number is already easily available.
>> What actual value do you think you get from this pretend-file-handle
>> on filesystems that don't support file handles?
>>
>
> Sorry, I misread your patch. In my mind I thought you wanted to
> eliminate the default export_encode_fh if there was no fh_to_dentry
> operation like do_sys_name_to_handle() does. Just in my head...

I see... I would have said that fh_to_dentry was mandatory if
s_export_op was set, and Documentation/filesystems/nfs/Exporting agrees
with me.  But I do see that sys_name_to_handle() and nfsd explicitly
test for it as well as for s_export_op.

It appears that cifs sets s_export_op, but doesn't provide fh_to_dentry,
and it is unique in this.
But the CIFS_NFSD_EXPORT config option is marked as BROKEN, so
that probably doesn't matter.

So for all current filesystems, filehandles are only reported if they
are usable for dentry lookup...

>
> FWIW, according to Pavel, if fdinfo would not export file handle
> in case !fh_to_dentry op would probably be desirable, because
> criu has no need for file handles that cannot be decoded.

Yes, it was good to have that confirmed - thanks for getting that
checked.

NeilBrown
diff mbox

Patch

diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
index 329a5d103846..f5b27dd843a1 100644
--- a/fs/exportfs/expfs.c
+++ b/fs/exportfs/expfs.c
@@ -385,7 +385,9 @@  int exportfs_encode_inode_fh(struct inode *inode, struct fid *fid,
 {
 	const struct export_operations *nop = inode->i_sb->s_export_op;
 
-	if (nop && nop->encode_fh)
+	if (nop)
+		return FILEID_INVALID;
+	if (nop->encode_fh)
 		return nop->encode_fh(inode, fid->raw, max_len, parent);
 
 	return export_encode_fh(inode, fid, max_len, parent);
diff --git a/fs/notify/fdinfo.c b/fs/notify/fdinfo.c
index d478629c728b..d1135ed61229 100644
--- a/fs/notify/fdinfo.c
+++ b/fs/notify/fdinfo.c
@@ -50,10 +50,8 @@  static void show_mark_fhandle(struct seq_file *m, struct inode *inode)
 	size = f.handle.handle_bytes >> 2;
 
 	ret = exportfs_encode_inode_fh(inode, (struct fid *)f.handle.f_handle, &size, 0);
-	if ((ret == FILEID_INVALID) || (ret < 0)) {
-		WARN_ONCE(1, "Can't encode file handler for inotify: %d\n", ret);
+	if ((ret == FILEID_INVALID) || (ret < 0))
 		return;
-	}
 
 	f.handle.handle_type = ret;
 	f.handle.handle_bytes = size * sizeof(u32);