From patchwork Tue Dec 19 12:42:34 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jan Kara X-Patchwork-Id: 10123197 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 68914603B5 for ; Tue, 19 Dec 2017 12:42:44 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 5815F292F9 for ; Tue, 19 Dec 2017 12:42:44 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 4CDE0292FF; Tue, 19 Dec 2017 12:42:44 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=unavailable version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id CAAC6292F9 for ; Tue, 19 Dec 2017 12:42:43 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S967214AbdLSMmj (ORCPT ); Tue, 19 Dec 2017 07:42:39 -0500 Received: from mx2.suse.de ([195.135.220.15]:35528 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S967196AbdLSMmh (ORCPT ); Tue, 19 Dec 2017 07:42:37 -0500 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id AADE3AE77; Tue, 19 Dec 2017 12:42:35 +0000 (UTC) Received: by quack2.suse.cz (Postfix, from userid 1000) id 4497E1E0CE1; Tue, 19 Dec 2017 13:42:34 +0100 (CET) Date: Tue, 19 Dec 2017 13:42:34 +0100 From: Jan Kara To: NeilBrown Cc: Amir Goldstein , Linus Torvalds , Trond Myklebust , Anna Schumaker , Al Viro , Andrew Morton , lkml , "linux-nfs@vger.kernel.org" , linux-fsdevel , Lennart Poettering , Pavel Emelyanov , Jan Kara Subject: Re: [PATCH] NFS: allow name_to_handle_at() to work for Amazon EFS. Message-ID: <20171219124234.GH2277@quack2.suse.cz> References: <87po7zv62h.fsf@notabene.neil.brown.name> <87r2s7ql5m.fsf@notabene.neil.brown.name> <878teeq7yc.fsf@notabene.neil.brown.name> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <878teeq7yc.fsf@notabene.neil.brown.name> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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 wrote: > >> On Wed, Dec 06 2017, Linus Torvalds wrote: > >> > >>> On Thu, Nov 30, 2017 at 12:56 PM, NeilBrown 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 > >> 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 > >> > >> 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 From 66a6c05ae2fbe6cfcb24ca3088de39885a6fa5b8 Mon Sep 17 00:00:00 2001 From: Jan Kara 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 Signed-off-by: Jan Kara --- 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