lkml.org 
[lkml]   [2017]   [Dec]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH] NFS: allow name_to_handle_at() to work for Amazon EFS.
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@suse.com> wrote:
> >> On Wed, Dec 06 2017, Linus Torvalds wrote:
> >>
> >>> On Thu, Nov 30, 2017 at 12:56 PM, NeilBrown <neilb@suse.com> 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@suse.com>
> >> 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@suse.com>
> >>
> >> 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?

Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
From 66a6c05ae2fbe6cfcb24ca3088de39885a6fa5b8 Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@suse.cz>
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@suse.com>
Signed-off-by: Jan Kara <jack@suse.cz>
---
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
\
 
 \ /
  Last update: 2017-12-19 13:43    [W:0.059 / U:4.888 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site