| Message ID | 87po7zv62h.fsf@notabene.neil.brown.name (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Dec 01, 2017 at 07:56:38AM +1100, NeilBrown wrote: > > Amazon EFS provides an NFSv4.1 filesystem which appears to use > (close to) full length (128 byte) file handles. I wonder why? Anyway, it seems unfortunate that systemd should need to worry about filehandle sizes at all, given that (as far as I can tell) all it's really calling name_to_handle_at() for is the mount_id, so it can recognize mountpoints. Userland NFS servers were a major motivation for the filehandle code. And filehandles larger than 128 bytes aren't much use to them. Hopefully they'll be able to identify the problem and fail early on. Oh well. ACK to both patches from me. --b. > This causes the handle reported by name_to_handle_at() to exceed > MAX_HANDLE_SZ, resulting in > EOVERFLOW if 128 bytes were requested, or > EINVAL if the size reported by the previous call was requested. > > To fix this, increase MAX_HANDLE_SIZE a little, and add a BUILD_BUG > so that this sort of inconsistent error reporting won't happen again. > > Link: https://github.com/systemd/systemd/issues/7082#issuecomment-347380436 > Signed-off-by: NeilBrown <neilb@suse.com> > --- > > Sorry for the scatter-gun To: list. This is really more of a VFS patch > than an NFS patch and sending patches in either direction has been a bit > hit-and-miss lately, so I'm hoping Andrew will take it unless someone > else objects or beats him to it... > > Thanks, > NeilBrown > > fs/nfs/export.c | 2 ++ > include/linux/exportfs.h | 7 +++++-- > 2 files changed, 7 insertions(+), 2 deletions(-) > > diff --git a/fs/nfs/export.c b/fs/nfs/export.c > index 83fd09fc8f77..23b2fc3ab2bb 100644 > --- a/fs/nfs/export.c > +++ b/fs/nfs/export.c > @@ -39,6 +39,8 @@ nfs_encode_fh(struct inode *inode, __u32 *p, int *max_len, struct inode *parent) > size_t fh_size = offsetof(struct nfs_fh, data) + server_fh->size; > int len = EMBED_FH_OFF + XDR_QUADLEN(fh_size); > > + BUILD_BUG_ON_MSG(EMBED_FH_OFF + NFS_MAXFHSIZE > MAX_HANDLE_SZ, > + "MAX_HANDLE_SZ is too small"); > dprintk("%s: max fh len %d inode %p parent %p", > __func__, *max_len, inode, parent); > > diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h > index 0d3037419bc7..e7ab1b071c5e 100644 > --- a/include/linux/exportfs.h > +++ b/include/linux/exportfs.h > @@ -11,8 +11,11 @@ struct iomap; > struct super_block; > struct vfsmount; > > -/* 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 > > /* > * The fileid_type identifies how the file within the filesystem is encoded. > -- > 2.14.0.rc0.dirty >
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
diff --git a/fs/nfs/export.c b/fs/nfs/export.c index 83fd09fc8f77..23b2fc3ab2bb 100644 --- a/fs/nfs/export.c +++ b/fs/nfs/export.c @@ -39,6 +39,8 @@ nfs_encode_fh(struct inode *inode, __u32 *p, int *max_len, struct inode *parent) size_t fh_size = offsetof(struct nfs_fh, data) + server_fh->size; int len = EMBED_FH_OFF + XDR_QUADLEN(fh_size); + BUILD_BUG_ON_MSG(EMBED_FH_OFF + NFS_MAXFHSIZE > MAX_HANDLE_SZ, + "MAX_HANDLE_SZ is too small"); dprintk("%s: max fh len %d inode %p parent %p", __func__, *max_len, inode, parent); diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h index 0d3037419bc7..e7ab1b071c5e 100644 --- a/include/linux/exportfs.h +++ b/include/linux/exportfs.h @@ -11,8 +11,11 @@ struct iomap; struct super_block; struct vfsmount; -/* 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 /* * The fileid_type identifies how the file within the filesystem is encoded.
Amazon EFS provides an NFSv4.1 filesystem which appears to use (close to) full length (128 byte) file handles. This causes the handle reported by name_to_handle_at() to exceed MAX_HANDLE_SZ, resulting in EOVERFLOW if 128 bytes were requested, or EINVAL if the size reported by the previous call was requested. To fix this, increase MAX_HANDLE_SIZE a little, and add a BUILD_BUG so that this sort of inconsistent error reporting won't happen again. Link: https://github.com/systemd/systemd/issues/7082#issuecomment-347380436 Signed-off-by: NeilBrown <neilb@suse.com> --- Sorry for the scatter-gun To: list. This is really more of a VFS patch than an NFS patch and sending patches in either direction has been a bit hit-and-miss lately, so I'm hoping Andrew will take it unless someone else objects or beats him to it... Thanks, NeilBrown fs/nfs/export.c | 2 ++ include/linux/exportfs.h | 7 +++++-- 2 files changed, 7 insertions(+), 2 deletions(-)