Re: [PATCH] NFS: allow name_to_handle_at() to work for Amazon EFS.

[NeilBrown]

On Tue, Dec 19 2017, Jan Kara wrote:

> On Fri 08-12-17 13:17:31, NeilBrown wrote:
>> On Thu, Dec 07 2017, Amir Goldstein wrote:
>> 
>> > On Thu, Dec 7, 2017 at 5:20 AM, NeilBrown <neilb@xxxxxxxx> wrote:
>> >> On Wed, Dec 06 2017, Linus Torvalds wrote:
>> >>
>> >>> On Thu, Nov 30, 2017 at 12:56 PM, NeilBrown <neilb@xxxxxxxx> wrote:
>> >>>>
>> >>>> -/* limit the handle size to NFSv4 handle size now */
>> >>>> -#define MAX_HANDLE_SZ 128
>> >>>> +/* Must be larger than NFSv4 file handle, but small
>> >>>> + * enough for an on-stack allocation. overlayfs doesn't
>> >>>> + * want this too close to 255.
>> >>>> + */
>> >>>> +#define MAX_HANDLE_SZ 200
>> >>>
>> >>> This really smells for so many reasons.
>> >>>
>> >>> Also, that really is starting to be a fairly big stack allocation, and
>> >>> it seems to be used in exactly one place (show_mark_fhandle), which
>> >>> makes me go "why is that on the stack anyway?".
>> >>>
>> >>> Could we just allocate a buffer at open time or something?
>> >>>
>> >>>                Linus
>> >>
>> >> "open time" would be when /proc/X/fdinfo/Y was opened in
>> >> seq_fdinfo_open(), and allocating a file_handle there seems a bit odd.
>> >>
>> >> We can allocate in fs/notify/fdinfo.c:show_fdinfo() which is
>> >> the earliest 'notify' specific code to run.  There is no
>> >> opportunity to return an error but GFP_KERNEL allocations under 1 page
>> >> never fail..
>> >>
>> >> This patch allocates a single buffer for all inodes reported for a given
>> >> inotify fdinfo, and if the allocation files, the filehandle is silently
>> >> left blank.  More surgery would be needed to be able to return an error.
>> >>
>> >> Is that at all suitable?
>> >>
>> >> Thanks,
>> >> NeilBrown
>> >>
>> >> From: NeilBrown <neilb@xxxxxxxx>
>> >> Subject: fs/notify: don't put file handle buffer on stack.
>> >>
>> >> A file handle buffer is not tiny, and could need to be larger in future,
>> >> so it isn't safe to allocate one on the stack.  Instead, we need to
>> >> kmalloc().
>> >>
>> >> There is no way to return an error status from a ->show_fdinfo()
>> >> function, so if the kmalloc fails, we silently exclude the filehandle
>> >> from the output.  As it is at the end of line, this shouldn't
>> >> upset parsing too much.
>> >
>> > It shouldn't upset parsing because that would be the same out
>> > output as without CONFIG_EXPORTFS. AFAIK this information
>> > is used by CRUI.
>> >
>> >>
>> >> Signed-off-by: NeilBrown <neilb@xxxxxxxx>
>> >>
>> >> diff --git a/fs/notify/fdinfo.c b/fs/notify/fdinfo.c
>> >> index d478629c728b..20d863b9ae16 100644
>> >> --- a/fs/notify/fdinfo.c
>> >> +++ b/fs/notify/fdinfo.c
>> >> @@ -23,56 +23,58 @@
>> >>
>> >>  static void show_fdinfo(struct seq_file *m, struct file *f,
>> >>                         void (*show)(struct seq_file *m,
>> >> -                                    struct fsnotify_mark *mark))
>> >> +                                    struct fsnotify_mark *mark,
>> >> +                                    struct fid *fh))
>> >>  {
>> >>         struct fsnotify_group *group = f->private_data;
>> >>         struct fsnotify_mark *mark;
>> >> +       struct fid *fh = kmalloc(MAX_HANDLE_SZ, GFP_KERNEL);
>> >>
>> >>         mutex_lock(&group->mark_mutex);
>> >>         list_for_each_entry(mark, &group->marks_list, g_list) {
>> >> -               show(m, mark);
>> >> +               show(m, mark, fh);
>> >>                 if (seq_has_overflowed(m))
>> >>                         break;
>> >>         }
>> >>         mutex_unlock(&group->mark_mutex);
>> >> +       kfree(fh);
>> >>  }
>> >>
>> >>  #if defined(CONFIG_EXPORTFS)
>> >> -static void show_mark_fhandle(struct seq_file *m, struct inode *inode)
>> >> +static void show_mark_fhandle(struct seq_file *m, struct inode *inode,
>> >> +                             struct fid *fhbuf)
>> >>  {
>> >> -       struct {
>> >> -               struct file_handle handle;
>> >> -               u8 pad[MAX_HANDLE_SZ];
>> >> -       } f;
>> >>         int size, ret, i;
>> >> +       unsigned char *bytes;
>> >>
>> >> -       f.handle.handle_bytes = sizeof(f.pad);
>> >> -       size = f.handle.handle_bytes >> 2;
>> >> +       if (!fhbuf)
>> >> +               return;
>> >> +       size = MAX_HANDLE_SZ >> 2;
>> >>
>> >> -       ret = exportfs_encode_inode_fh(inode, (struct fid *)f.handle.f_handle, &size, 0);
>> >> +       ret = exportfs_encode_inode_fh(inode, fhbuf, &size, 0);
>> >>         if ((ret == FILEID_INVALID) || (ret < 0)) {
>> >>                 WARN_ONCE(1, "Can't encode file handler for inotify: %d\n", ret);
>> >
>> > This WARN_ONCE is out of order. It is perfectly valid for inotify/fanotify
>> > to watch over fs that doesn't support exportfs. Care to clean it up?
>> > Perhaps a pr_warn_ratelimited() for either !fhbuf or can't encode?
>> 
>> If I were going to clean it up, I would need to do more than remove the
>> WARN_ONCE(), which almost certainly never fires.
>> 
>> exportfs_encode_inode_fh() should only be called if sb->s_export_op is
>> not NULL.
>> When it is NULL, it means that the filesystem doesn't support file
>> handles.
>> do_sys_name_to_handle() tests this, as does nfsd.  But this inotify code
>> doesn't.
>> So it can report a "file handle" for a file for which file handles
>> aren't supported.  It will use the default export_encode_fh which
>> reports i_ino and i_generation, which may or may not be stable or
>> meaningful.
>> 
>> So yes, this code does need a bit of cleaning up....
>
> So something like the patch below?
>

I prefer to fix it in exportfs, as in
 https://patchwork.kernel.org/patch/10104253/

Thanks,
NeilBrown


> 								Honza
> -- 
> Jan Kara <jack@xxxxxxxx>
> SUSE Labs, CR
> From 66a6c05ae2fbe6cfcb24ca3088de39885a6fa5b8 Mon Sep 17 00:00:00 2001
> From: Jan Kara <jack@xxxxxxx>
> Date: Tue, 19 Dec 2017 13:38:54 +0100
> Subject: [PATCH] fsnotify: Do not show file handles for unsupported
>  filesystems
>
> Filesystems not setting their s_export_op do not support file handles.
> Do no try to encode them using exportfs_encode_inode_fh() since that may
> fail or return garbage.
>
> Reported-by: NeilBrown <neilb@xxxxxxxx>
> Signed-off-by: Jan Kara <jack@xxxxxxx>
> ---
>  fs/notify/fdinfo.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/fs/notify/fdinfo.c b/fs/notify/fdinfo.c
> index d478629c728b..041c2b0cc145 100644
> --- a/fs/notify/fdinfo.c
> +++ b/fs/notify/fdinfo.c
> @@ -46,6 +46,9 @@ static void show_mark_fhandle(struct seq_file *m, struct inode *inode)
>  	} f;
>  	int size, ret, i;
>  
> +	if (!inode->i_sb->s_export_op)
> +		return;
> +
>  	f.handle.handle_bytes = sizeof(f.pad);
>  	size = f.handle.handle_bytes >> 2;
>  
> -- 
> 2.12.3

Attachment: signature.asc
Description: PGP signature