diff mbox series

[2/5] fs,nfs: lift compat nfs4 mount data handling into the nfs code

Message ID 20200917082236.2518236-3-hch@lst.de
State New
Headers show
Series [1/5] nfs: simplify nfs4_parse_monolithic | expand

Commit Message

Christoph Hellwig Sept. 17, 2020, 8:22 a.m. UTC
There is no reason the generic fs code should bother with NFS specific
binary mount data - lift the conversion into nfs4_parse_monolithic
instead.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/compat.c         | 75 ---------------------------------------------
 fs/nfs/fs_context.c | 62 +++++++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+), 75 deletions(-)

Comments

Al Viro Sept. 17, 2020, 5:16 p.m. UTC | #1
On Thu, Sep 17, 2020 at 10:22:33AM +0200, Christoph Hellwig wrote:
> There is no reason the generic fs code should bother with NFS specific
> binary mount data - lift the conversion into nfs4_parse_monolithic
> instead.

Considering the size of struct compat_nfs4_mount_data_v1...  Do we really
need to bother with that "copy in place, so we go through the fields
backwards" logics?  Just make that

> +static void nfs4_compat_mount_data_conv(struct nfs4_mount_data *data)
> +{
	struct compat_nfs4_mount_data_v1 compat;
	compat = *(struct compat_nfs4_mount_data_v1 *)data;
and copy the damnt thing without worrying about the field order...
Christoph Hellwig Sept. 17, 2020, 5:18 p.m. UTC | #2
On Thu, Sep 17, 2020 at 06:16:04PM +0100, Al Viro wrote:
> On Thu, Sep 17, 2020 at 10:22:33AM +0200, Christoph Hellwig wrote:
> > There is no reason the generic fs code should bother with NFS specific
> > binary mount data - lift the conversion into nfs4_parse_monolithic
> > instead.
> 
> Considering the size of struct compat_nfs4_mount_data_v1...  Do we really
> need to bother with that "copy in place, so we go through the fields
> backwards" logics?  Just make that
> 
> > +static void nfs4_compat_mount_data_conv(struct nfs4_mount_data *data)
> > +{
> 	struct compat_nfs4_mount_data_v1 compat;
> 	compat = *(struct compat_nfs4_mount_data_v1 *)data;
> and copy the damnt thing without worrying about the field order...

Maybe.  But then again why bother?  I just sticked to the existing
code as much as possible.
Christoph Hellwig Sept. 21, 2020, 6:48 a.m. UTC | #3
On Thu, Sep 17, 2020 at 07:18:26PM +0200, Christoph Hellwig wrote:
> On Thu, Sep 17, 2020 at 06:16:04PM +0100, Al Viro wrote:
> > On Thu, Sep 17, 2020 at 10:22:33AM +0200, Christoph Hellwig wrote:
> > > There is no reason the generic fs code should bother with NFS specific
> > > binary mount data - lift the conversion into nfs4_parse_monolithic
> > > instead.
> > 
> > Considering the size of struct compat_nfs4_mount_data_v1...  Do we really
> > need to bother with that "copy in place, so we go through the fields
> > backwards" logics?  Just make that
> > 
> > > +static void nfs4_compat_mount_data_conv(struct nfs4_mount_data *data)
> > > +{
> > 	struct compat_nfs4_mount_data_v1 compat;
> > 	compat = *(struct compat_nfs4_mount_data_v1 *)data;
> > and copy the damnt thing without worrying about the field order...
> 
> Maybe.  But then again why bother?  I just sticked to the existing
> code as much as possible.

Trond, Anna: what is your preference?
Anna Schumaker Sept. 21, 2020, 4:05 p.m. UTC | #4
This is for the binary mount stuff? That was already legacy code when
I first started, and mount uses text options now. My preference is for
keeping it as close to the original code as possible.

I'm curious if you've been able to test this? I'm not sure if there is
a way to force binary mount data through mount.nfs

Anna

On Mon, Sep 21, 2020 at 2:49 AM Christoph Hellwig <hch@lst.de> wrote:
>
> On Thu, Sep 17, 2020 at 07:18:26PM +0200, Christoph Hellwig wrote:
> > On Thu, Sep 17, 2020 at 06:16:04PM +0100, Al Viro wrote:
> > > On Thu, Sep 17, 2020 at 10:22:33AM +0200, Christoph Hellwig wrote:
> > > > There is no reason the generic fs code should bother with NFS specific
> > > > binary mount data - lift the conversion into nfs4_parse_monolithic
> > > > instead.
> > >
> > > Considering the size of struct compat_nfs4_mount_data_v1...  Do we really
> > > need to bother with that "copy in place, so we go through the fields
> > > backwards" logics?  Just make that
> > >
> > > > +static void nfs4_compat_mount_data_conv(struct nfs4_mount_data *data)
> > > > +{
> > >     struct compat_nfs4_mount_data_v1 compat;
> > >     compat = *(struct compat_nfs4_mount_data_v1 *)data;
> > > and copy the damnt thing without worrying about the field order...
> >
> > Maybe.  But then again why bother?  I just sticked to the existing
> > code as much as possible.
>
> Trond, Anna: what is your preference?
Christoph Hellwig Sept. 21, 2020, 6:11 p.m. UTC | #5
On Mon, Sep 21, 2020 at 12:05:52PM -0400, Anna Schumaker wrote:
> This is for the binary mount stuff? That was already legacy code when
> I first started, and mount uses text options now. My preference is for
> keeping it as close to the original code as possible.

Ok.  Al, are you fine with the series as-is then?

> 
> I'm curious if you've been able to test this? I'm not sure if there is
> a way to force binary mount data through mount.nfs

The test is pretty trivial - even the most recent nfsutils supports the
binary mount data.  You just need to patch out the detection for a
modern kernel.
Al Viro Sept. 23, 2020, 3:45 a.m. UTC | #6
On Mon, Sep 21, 2020 at 08:11:23PM +0200, Christoph Hellwig wrote:
> On Mon, Sep 21, 2020 at 12:05:52PM -0400, Anna Schumaker wrote:
> > This is for the binary mount stuff? That was already legacy code when
> > I first started, and mount uses text options now. My preference is for
> > keeping it as close to the original code as possible.
> 
> Ok.  Al, are you fine with the series as-is then?

I can live with that.  I'm not fond of in_compat_syscall() proliferation,
but in this case it's reasonably sane...

OK, applied.
diff mbox series

Patch

diff --git a/fs/compat.c b/fs/compat.c
index 436d228cf71c09..9b00523d7fa571 100644
--- a/fs/compat.c
+++ b/fs/compat.c
@@ -19,73 +19,6 @@ 
 #include <linux/uaccess.h>
 #include "internal.h"
 
-struct compat_nfs_string {
-	compat_uint_t len;
-	compat_uptr_t data;
-};
-
-static inline void compat_nfs_string(struct nfs_string *dst,
-				     struct compat_nfs_string *src)
-{
-	dst->data = compat_ptr(src->data);
-	dst->len = src->len;
-}
-
-struct compat_nfs4_mount_data_v1 {
-	compat_int_t version;
-	compat_int_t flags;
-	compat_int_t rsize;
-	compat_int_t wsize;
-	compat_int_t timeo;
-	compat_int_t retrans;
-	compat_int_t acregmin;
-	compat_int_t acregmax;
-	compat_int_t acdirmin;
-	compat_int_t acdirmax;
-	struct compat_nfs_string client_addr;
-	struct compat_nfs_string mnt_path;
-	struct compat_nfs_string hostname;
-	compat_uint_t host_addrlen;
-	compat_uptr_t host_addr;
-	compat_int_t proto;
-	compat_int_t auth_flavourlen;
-	compat_uptr_t auth_flavours;
-};
-
-static int do_nfs4_super_data_conv(void *raw_data)
-{
-	int version = *(compat_uint_t *) raw_data;
-
-	if (version == 1) {
-		struct compat_nfs4_mount_data_v1 *raw = raw_data;
-		struct nfs4_mount_data *real = raw_data;
-
-		/* copy the fields backwards */
-		real->auth_flavours = compat_ptr(raw->auth_flavours);
-		real->auth_flavourlen = raw->auth_flavourlen;
-		real->proto = raw->proto;
-		real->host_addr = compat_ptr(raw->host_addr);
-		real->host_addrlen = raw->host_addrlen;
-		compat_nfs_string(&real->hostname, &raw->hostname);
-		compat_nfs_string(&real->mnt_path, &raw->mnt_path);
-		compat_nfs_string(&real->client_addr, &raw->client_addr);
-		real->acdirmax = raw->acdirmax;
-		real->acdirmin = raw->acdirmin;
-		real->acregmax = raw->acregmax;
-		real->acregmin = raw->acregmin;
-		real->retrans = raw->retrans;
-		real->timeo = raw->timeo;
-		real->wsize = raw->wsize;
-		real->rsize = raw->rsize;
-		real->flags = raw->flags;
-		real->version = raw->version;
-	}
-
-	return 0;
-}
-
-#define NFS4_NAME	"nfs4"
-
 COMPAT_SYSCALL_DEFINE5(mount, const char __user *, dev_name,
 		       const char __user *, dir_name,
 		       const char __user *, type, compat_ulong_t, flags,
@@ -111,14 +44,6 @@  COMPAT_SYSCALL_DEFINE5(mount, const char __user *, dev_name,
 	if (IS_ERR(options))
 		goto out2;
 
-	if (kernel_type && options) {
-		if (!strcmp(kernel_type, NFS4_NAME)) {
-			retval = -EINVAL;
-			if (do_nfs4_super_data_conv(options))
-				goto out3;
-		}
-	}
-
 	retval = do_mount(kernel_dev, dir_name, kernel_type, flags, options);
 
  out3:
diff --git a/fs/nfs/fs_context.c b/fs/nfs/fs_context.c
index cbf6a4ba5e5806..222afba70bc08e 100644
--- a/fs/nfs/fs_context.c
+++ b/fs/nfs/fs_context.c
@@ -1039,6 +1039,65 @@  static int nfs23_parse_monolithic(struct fs_context *fc,
 }
 
 #if IS_ENABLED(CONFIG_NFS_V4)
+struct compat_nfs_string {
+	compat_uint_t len;
+	compat_uptr_t data;
+};
+
+static inline void compat_nfs_string(struct nfs_string *dst,
+				     struct compat_nfs_string *src)
+{
+	dst->data = compat_ptr(src->data);
+	dst->len = src->len;
+}
+
+struct compat_nfs4_mount_data_v1 {
+	compat_int_t version;
+	compat_int_t flags;
+	compat_int_t rsize;
+	compat_int_t wsize;
+	compat_int_t timeo;
+	compat_int_t retrans;
+	compat_int_t acregmin;
+	compat_int_t acregmax;
+	compat_int_t acdirmin;
+	compat_int_t acdirmax;
+	struct compat_nfs_string client_addr;
+	struct compat_nfs_string mnt_path;
+	struct compat_nfs_string hostname;
+	compat_uint_t host_addrlen;
+	compat_uptr_t host_addr;
+	compat_int_t proto;
+	compat_int_t auth_flavourlen;
+	compat_uptr_t auth_flavours;
+};
+
+static void nfs4_compat_mount_data_conv(struct nfs4_mount_data *data)
+{
+	struct compat_nfs4_mount_data_v1 *compat =
+			(struct compat_nfs4_mount_data_v1 *)data;
+
+	/* copy the fields backwards */
+	data->auth_flavours = compat_ptr(compat->auth_flavours);
+	data->auth_flavourlen = compat->auth_flavourlen;
+	data->proto = compat->proto;
+	data->host_addr = compat_ptr(compat->host_addr);
+	data->host_addrlen = compat->host_addrlen;
+	compat_nfs_string(&data->hostname, &compat->hostname);
+	compat_nfs_string(&data->mnt_path, &compat->mnt_path);
+	compat_nfs_string(&data->client_addr, &compat->client_addr);
+	data->acdirmax = compat->acdirmax;
+	data->acdirmin = compat->acdirmin;
+	data->acregmax = compat->acregmax;
+	data->acregmin = compat->acregmin;
+	data->retrans = compat->retrans;
+	data->timeo = compat->timeo;
+	data->wsize = compat->wsize;
+	data->rsize = compat->rsize;
+	data->flags = compat->flags;
+	data->version = compat->version;
+}
+
 /*
  * Validate NFSv4 mount options
  */
@@ -1061,6 +1120,9 @@  static int nfs4_parse_monolithic(struct fs_context *fc,
 	if (data->version != 1)
 		return generic_parse_monolithic(fc, data);
 
+	if (in_compat_syscall())
+		nfs4_compat_mount_data_conv(data);
+
 	if (data->host_addrlen > sizeof(ctx->nfs_server.address))
 		goto out_no_address;
 	if (data->host_addrlen == 0)