[PATCH] btrfs: bugfix: handle FS_IOC32_{GETFLAGS,SETFLAGS,GETVERSION} in btrfs_ioctl

[PATCH] btrfs: bugfix: handle FS_IOC32_{GETFLAGS,SETFLAGS,GETVERSION} in btrfs_ioctl

[Luke Dashjr]

32-bit ioctl uses these rather than the regular FS_IOC_* versions. They can be 
handled in btrfs using the same code. Without this, 32-bit {ch,ls}attr fail.

Signed-off-by: Luke Dashjr <luke-jr+git@utopios.org>
---
 fs/btrfs/ioctl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 1c22c65..31af093 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -5225,10 +5225,13 @@ long btrfs_ioctl(struct file *file, unsigned int
 
 	switch (cmd) {
 	case FS_IOC_GETFLAGS:
+	case FS_IOC32_GETFLAGS:
 		return btrfs_ioctl_getflags(file, argp);
 	case FS_IOC_SETFLAGS:
+	case FS_IOC32_SETFLAGS:
 		return btrfs_ioctl_setflags(file, argp);
 	case FS_IOC_GETVERSION:
+	case FS_IOC32_GETVERSION:
 		return btrfs_ioctl_getversion(file, argp);
 	case FITRIM:
 		return btrfs_ioctl_fitrim(file, argp);
-- 
2.0.5

Re: [PATCH] btrfs: bugfix: handle FS_IOC32_{GETFLAGS,SETFLAGS,GETVERSION} in btrfs_ioctl

[Greg KH]

On Wed, May 13, 2015 at 05:15:26PM +0000, Luke Dashjr wrote:
> 32-bit ioctl uses these rather than the regular FS_IOC_* versions. They can be 
> handled in btrfs using the same code. Without this, 32-bit {ch,ls}attr fail.
> 
> Signed-off-by: Luke Dashjr <luke-jr+git@utopios.org>
> ---
>  fs/btrfs/ioctl.c | 3 +++
>  1 file changed, 3 insertions(+)

<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read Documentation/stable_kernel_rules.txt
for how to do this properly.

</formletter>

Re: [PATCH] btrfs: bugfix: handle FS_IOC32_{GETFLAGS,SETFLAGS,GETVERSION} in btrfs_ioctl

[David Sterba]

On Wed, May 13, 2015 at 05:15:26PM +0000, Luke Dashjr wrote:
> 32-bit ioctl uses these rather than the regular FS_IOC_* versions. They can be 
> handled in btrfs using the same code. Without this, 32-bit {ch,ls}attr fail.

Yes, but this has to be implemented in another way. See eg.
https://git.kernel.org/linus/e9750824114ff

Re: [PATCH] btrfs: bugfix: handle FS_IOC32_{GETFLAGS,SETFLAGS,GETVERSION} in btrfs_ioctl

[Luke Dashjr]

On Thursday, May 14, 2015 2:06:17 PM David Sterba wrote:
> On Wed, May 13, 2015 at 05:15:26PM +0000, Luke Dashjr wrote:
> > 32-bit ioctl uses these rather than the regular FS_IOC_* versions. They
> > can be handled in btrfs using the same code. Without this, 32-bit
> > {ch,ls}attr fail.
> 
> Yes, but this has to be implemented in another way. See eg.
> https://git.kernel.org/linus/e9750824114ff

I don't see what is different with that implementation. All f2fs_compat_ioctl 
does is change cmd to the plain-IOC equivalent and call f2fs_ioctl with the 
same arg (compat_ptr merely causes a cast to void* and back, which AFAIK is a 
noop on 64-bit?). Am I missing something? I could try to just imitate it, but 
I'd rather know what is significant/going on to ensure I don't waste your time 
with code I don't even properly understand myself.

Perhaps by coincidence, the patch does at least in practice work (although at 
least `btrfs send` appears to be broken still, and I'm at a loss for how to 
approach fixing that).

Luke

Re: [PATCH] btrfs: bugfix: handle FS_IOC32_{GETFLAGS,SETFLAGS,GETVERSION} in btrfs_ioctl

[David Sterba]

On Thu, May 14, 2015 at 04:27:54PM +0000, Luke Dashjr wrote:
> On Thursday, May 14, 2015 2:06:17 PM David Sterba wrote:
> > On Wed, May 13, 2015 at 05:15:26PM +0000, Luke Dashjr wrote:
> > > 32-bit ioctl uses these rather than the regular FS_IOC_* versions. They
> > > can be handled in btrfs using the same code. Without this, 32-bit
> > > {ch,ls}attr fail.
> > 
> > Yes, but this has to be implemented in another way. See eg.
> > https://git.kernel.org/linus/e9750824114ff
> 
> I don't see what is different with that implementation. All f2fs_compat_ioctl 
> does is change cmd to the plain-IOC equivalent and call f2fs_ioctl with the 
> same arg (compat_ptr merely causes a cast to void* and back, which AFAIK is a 
> noop on 64-bit?). Am I missing something?

No, that's the idea. Add new calback for compat_ioctl, put it under
#ifdef CONFIG_COMPAT and do the same number switch.

> I could try to just imitate it, but 
> I'd rather know what is significant/going on to ensure I don't waste your time 
> with code I don't even properly understand myself.
> 
> Perhaps by coincidence, the patch does at least in practice work (although at 
> least `btrfs send` appears to be broken still, and I'm at a loss for how to 
> approach fixing that).

The 'receive' 32bit/64bit was broken due to size difference in the ioctl
structure that led to different ioctl. This is transparently fixed, see
BTRFS_IOC_SET_RECEIVED_SUBVOL_32 at the top of ioctl.c.

In what way is SEND broken? There are only u64/s64 members in
btrfs_ioctl_send_args, I don't see how this could break on 32/64
userspace/kernel.

Re: [PATCH] btrfs: bugfix: handle FS_IOC32_{GETFLAGS,SETFLAGS,GETVERSION} in btrfs_ioctl

[Luke Dashjr]

On Friday, May 15, 2015 11:19:22 AM David Sterba wrote:
> On Thu, May 14, 2015 at 04:27:54PM +0000, Luke Dashjr wrote:
> > On Thursday, May 14, 2015 2:06:17 PM David Sterba wrote:
> > > On Wed, May 13, 2015 at 05:15:26PM +0000, Luke Dashjr wrote:
> > > > 32-bit ioctl uses these rather than the regular FS_IOC_* versions.
> > > > They can be handled in btrfs using the same code. Without this,
> > > > 32-bit {ch,ls}attr fail.
> > > 
> > > Yes, but this has to be implemented in another way. See eg.
> > > https://git.kernel.org/linus/e9750824114ff
> > 
> > I don't see what is different with that implementation. All
> > f2fs_compat_ioctl does is change cmd to the plain-IOC equivalent and
> > call f2fs_ioctl with the same arg (compat_ptr merely causes a cast to
> > void* and back, which AFAIK is a noop on 64-bit?). Am I missing
> > something?
> 
> No, that's the idea. Add new calback for compat_ioctl, put it under
> #ifdef CONFIG_COMPAT and do the same number switch.

The idea is to wrap it in a way that doesn't actually change any of the logic?
I'm not sure I understand still :(

> > I could try to just imitate it, but
> > I'd rather know what is significant/going on to ensure I don't waste your
> > time with code I don't even properly understand myself.
> > 
> > Perhaps by coincidence, the patch does at least in practice work
> > (although at least `btrfs send` appears to be broken still, and I'm at a
> > loss for how to approach fixing that).
> 
> The 'receive' 32bit/64bit was broken due to size difference in the ioctl
> structure that led to different ioctl. This is transparently fixed, see
> BTRFS_IOC_SET_RECEIVED_SUBVOL_32 at the top of ioctl.c.
> 
> In what way is SEND broken? There are only u64/s64 members in
> btrfs_ioctl_send_args, I don't see how this could break on 32/64
> userspace/kernel.

# btrfs send -p home/initial/ home/20150514_1431573370/
At subvol home/20150514_1431573370/
ERROR: send ioctl failed with -25: Inappropriate ioctl for device

But I am stuck on 3.14.41 due to Linux being unstable in newer versions[1], so 
maybe this is unrelated to 32-bit and already fixed in 4.0?

Luke

1. https://bugzilla.kernel.org/show_bug.cgi?id=87891

Re: [PATCH] btrfs: bugfix: handle FS_IOC32_{GETFLAGS,SETFLAGS,GETVERSION} in btrfs_ioctl

[Luke Dashjr]

On Friday, May 15, 2015 11:19:22 AM David Sterba wrote:
> On Thu, May 14, 2015 at 04:27:54PM +0000, Luke Dashjr wrote:
> > On Thursday, May 14, 2015 2:06:17 PM David Sterba wrote:
> > > On Wed, May 13, 2015 at 05:15:26PM +0000, Luke Dashjr wrote:
> > > > 32-bit ioctl uses these rather than the regular FS_IOC_* versions.
> > > > They can be handled in btrfs using the same code. Without this,
> > > > 32-bit {ch,ls}attr fail.
> > > 
> > > Yes, but this has to be implemented in another way. See eg.
> > > https://git.kernel.org/linus/e9750824114ff
> > 
> > I don't see what is different with that implementation. All
> > f2fs_compat_ioctl does is change cmd to the plain-IOC equivalent and
> > call f2fs_ioctl with the same arg (compat_ptr merely causes a cast to
> > void* and back, which AFAIK is a noop on 64-bit?). Am I missing
> > something?
> 
> No, that's the idea. Add new calback for compat_ioctl, put it under
> #ifdef CONFIG_COMPAT and do the same number switch.

Ok, someone else explained this to me. Please let me know if PATCHv2 (sent 
separately) does not address the needed changes.

> > I could try to just imitate it, but
> > I'd rather know what is significant/going on to ensure I don't waste your
> > time with code I don't even properly understand myself.
> > 
> > Perhaps by coincidence, the patch does at least in practice work
> > (although at least `btrfs send` appears to be broken still, and I'm at a
> > loss for how to approach fixing that).
> 
> The 'receive' 32bit/64bit was broken due to size difference in the ioctl
> structure that led to different ioctl. This is transparently fixed, see
> BTRFS_IOC_SET_RECEIVED_SUBVOL_32 at the top of ioctl.c.
> 
> In what way is SEND broken? There are only u64/s64 members in
> btrfs_ioctl_send_args, I don't see how this could break on 32/64
> userspace/kernel.

I've investigated this now, and it seems to be the pointer-type clone_sources 
member of struct btrfs_ioctl_send_args. I can't think of a perfect way to fix 
this, but it might not be *too* ugly to:
- replace the current clone_sources with a u64 that must always be (u64)-1;
  this causes older kernels to error cleanly if called with a new ioctl data
- use the top 1 or 2 bits of flags to indicate sizeof(void*) as it appears to
  userspace OR just use up reserved[0] for pointer size:
      io_send.ptr_size = sizeof(void*);
- replace one of the reserved fields with the new clone_sources

The way it was done for receive seems like it might not work for non-x86 
compat interfaces (eg, MIPS n32) - but I could be wrong.

Thoughts?

Luke

Re: [PATCH] btrfs: bugfix: handle FS_IOC32_{GETFLAGS,SETFLAGS,GETVERSION} in btrfs_ioctl

[Thomas Rohwer]

Hello,

> I've investigated this now, and it seems to be the pointer-type clone_sources
> member of struct btrfs_ioctl_send_args. I can't think of a perfect way to fix
> this, but it might not be *too* ugly to:
> - replace the current clone_sources with a u64 that must always be (u64)-1;
>    this causes older kernels to error cleanly if called with a new ioctl data
> - use the top 1 or 2 bits of flags to indicate sizeof(void*) as it appears to
>    userspace OR just use up reserved[0] for pointer size:
>        io_send.ptr_size = sizeof(void*);
> - replace one of the reserved fields with the new clone_sources
>
> The way it was done for receive seems like it might not work for non-x86
> compat interfaces (eg, MIPS n32) - but I could be wrong.
>
> Thoughts?

I also encountered that problem with send. I posted the following a while ago to
comp.file-systems.btrfs, but never got any replies. The patch works for me (I used
it on my system in some cases), but is not extensively tested.

Sincerely,

Thomas Rohwer





Hello,

I am using as kernel Linux 4.1.3 (64bit) and btrfs-prog version 4.0 (32 bit user space).
I wanted to use send/receive with btrfs for the first time today and I got the following error:

humbur:~# btrfs send /snap > /dev/null
At subvol /snap
ERROR: send ioctl failed with -25: Inappropriate ioctl for device
ERROR: failed to read stream from kernel. Bad file descriptor


Investigating the source, I noticed that probably the problem is the member clone_sources in the structure

struct btrfs_ioctl_send_args {
   __s64 send_fd;      /* in */
   __u64 clone_sources_count;  /* in */
   __u64 __user *clone_sources;  /* in */
   __u64 parent_root;    /* in */
   __u64 flags;      /* in */
   __u64 reserved[4];    /* in */
};

in include/uapi/linux/btrfs.h and the missing adaption code in fs/btrfs/ioctl.c. The member clone_sources is only 32 bit wide in case of 32 bit user space.
For the ioctl RECEIVED_SUBVOL somebody already added code for the in this case also necessary translation. I took this as a template and
wrote a patch (see below). The patch compiles and with the new kernel I seem to get valid data with send (I have to read it back yet,
but I get about the expected amount and structure).

This is a proof of concept patch; for example the compiler currently warns for
  (args64->clone_sources = (__u64*)args32->clone_sources;
and I would have to investigate how to properly convert the pointer. Further there are probably some issues with the formating of the source code.
I also have not tested the 32bit/32bit 64bit/64bit userspace/kernel combinations.
If there is interest, I can resubmit an improved patch. Please CC me in replies, since I have not subscribed to the list.


Sincerely,

Thomas Rohwer





Signed-off-by: Thomas Rohwer <trohwer85@gmail.com>

 From e4156c38105200fa83913b6a94f07a41631c5f75 Mon Sep 17 00:00:00 2001
From: Thomas <tr@humbur.fritz.box>
Date: Wed, 22 Jul 2015 22:03:15 +0200
Subject: [PATCH] add 32 bit adaption code for send ioctl

---
  fs/btrfs/ioctl.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
  fs/btrfs/send.c  | 11 +--------
  fs/btrfs/send.h  |  2 +-
  3 files changed, 75 insertions(+), 11 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 1c22c65..8b969c0 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -83,6 +83,19 @@ struct btrfs_ioctl_received_subvol_args_32 {

  #define BTRFS_IOC_SET_RECEIVED_SUBVOL_32 _IOWR(BTRFS_IOCTL_MAGIC, 37, \
                  struct btrfs_ioctl_received_subvol_args_32)
+
+struct btrfs_ioctl_send_args_32 {
+    __s64 send_fd;                  /* in */
+    __u64 clone_sources_count;      /* in */
+    __u32 clone_sources;            /* in */
+    __u64 parent_root;              /* in */
+    __u64 flags;                    /* in */
+    __u64 reserved[4];              /* in */
+} __attribute__ ((__packed__));
+
+#define BTRFS_IOC_SEND_32 _IOW(BTRFS_IOCTL_MAGIC, 38, \
+                struct btrfs_ioctl_send_args_32)
+
  #endif


@@ -4965,6 +4978,43 @@ out:
      kfree(args64);
      return ret;
  }
+
+static long btrfs_ioctl_send_32(struct file *file,
+                        void __user *arg)
+{
+    struct btrfs_ioctl_send_args_32 *args32 = NULL;
+    struct btrfs_ioctl_send_args *args64 = NULL;
+    int ret = 0;
+
+    args32 = memdup_user(arg, sizeof(*args32));
+    if (IS_ERR(args32)) {
+        ret = PTR_ERR(args32);
+        args32 = NULL;
+        goto out;
+    }
+
+    args64 = kmalloc(sizeof(*args64), GFP_NOFS);
+    if (!args64) {
+        ret = -ENOMEM;
+        goto out;
+    }
+
+    args64->send_fd = args32->send_fd;
+    args64->clone_sources_count = args32->clone_sources_count;
+    args64->clone_sources = (__u64*)args32->clone_sources;
+    args64->parent_root = args32->parent_root;
+    args64->flags = args32->flags;
+
+    ret = _btrfs_ioctl_send(file, args64);
+
+    // only in arguments, so no copy back to args32
+
+out:
+    kfree(args32);
+    kfree(args64);
+    return ret;
+}
+
  #endif

  static long btrfs_ioctl_set_received_subvol(struct file *file,
@@ -4994,6 +5044,27 @@ out:
      return ret;
  }

+static long btrfs_ioctl_send(struct file *file,
+                        void __user *arg)
+{
+    struct btrfs_ioctl_send_args *sa = NULL;
+    int ret = 0;
+
+    sa = memdup_user(arg, sizeof(*sa));
+    if (IS_ERR(sa)) {
+        ret = PTR_ERR(sa);
+        sa = NULL;
+        goto out;
+    }
+
+    ret = _btrfs_ioctl_send(file, sa);
+
+out:
+    kfree(sa);
+    return ret;
+}
+
+
  static int btrfs_ioctl_get_fslabel(struct file *file, void __user *arg)
  {
      struct btrfs_root *root = BTRFS_I(file_inode(file))->root;
@@ -5320,6 +5391,8 @@ long btrfs_ioctl(struct file *file, unsigned int
  #ifdef CONFIG_64BIT
      case BTRFS_IOC_SET_RECEIVED_SUBVOL_32:
          return btrfs_ioctl_set_received_subvol_32(file, argp);
+    case BTRFS_IOC_SEND_32:
+        return btrfs_ioctl_send_32(file, argp);
  #endif
      case BTRFS_IOC_SEND:
          return btrfs_ioctl_send(file, argp);
diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index a1216f9..6838078 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -5663,13 +5663,12 @@ static void btrfs_root_dec_send_in_progress(struct btrfs_root* root)
      spin_unlock(&root->root_item_lock);
  }

-long btrfs_ioctl_send(struct file *mnt_file, void __user *arg_)
+long _btrfs_ioctl_send(struct file *mnt_file, struct btrfs_ioctl_send_args *arg)
  {
      int ret = 0;
      struct btrfs_root *send_root;
      struct btrfs_root *clone_root;
      struct btrfs_fs_info *fs_info;
-    struct btrfs_ioctl_send_args *arg = NULL;
      struct btrfs_key key;
      struct send_ctx *sctx = NULL;
      u32 i;
@@ -5707,13 +5706,6 @@ long btrfs_ioctl_send(struct file *mnt_file, void __user *arg_)
          goto out;
      }

-    arg = memdup_user(arg_, sizeof(*arg));
-    if (IS_ERR(arg)) {
-        ret = PTR_ERR(arg);
-        arg = NULL;
-        goto out;
-    }
-
      if (!access_ok(VERIFY_READ, arg->clone_sources,
              sizeof(*arg->clone_sources) *
              arg->clone_sources_count)) {
@@ -5942,7 +5934,6 @@ out:
      if (sctx && !IS_ERR_OR_NULL(sctx->parent_root))
          btrfs_root_dec_send_in_progress(sctx->parent_root);

-    kfree(arg);
      vfree(clone_sources_tmp);

      if (sctx) {
diff --git a/fs/btrfs/send.h b/fs/btrfs/send.h
index 48d425a..fb43556 100644
--- a/fs/btrfs/send.h
+++ b/fs/btrfs/send.h
@@ -130,5 +130,5 @@ enum {
  #define BTRFS_SEND_A_MAX (__BTRFS_SEND_A_MAX - 1)

  #ifdef __KERNEL__
-long btrfs_ioctl_send(struct file *mnt_file, void __user *arg);
+long _btrfs_ioctl_send(struct file *mnt_file, struct btrfs_ioctl_send_args *arg);
  #endif
-- 
2.1.4

Re: [PATCH] btrfs: bugfix: handle FS_IOC32_{GETFLAGS,SETFLAGS,GETVERSION} in btrfs_ioctl

[David Sterba]

On Thu, Oct 29, 2015 at 01:05:13PM +0100, Thomas Rohwer wrote:
> Investigating the source, I noticed that probably the problem is the member clone_sources in the structure

That's, right. Yet another thing to keep in mind when designing ioctls.

> struct btrfs_ioctl_send_args {
>    __s64 send_fd;      /* in */
>    __u64 clone_sources_count;  /* in */
>    __u64 __user *clone_sources;  /* in */
>    __u64 parent_root;    /* in */
>    __u64 flags;      /* in */
>    __u64 reserved[4];    /* in */
> };
> 
> in include/uapi/linux/btrfs.h and the missing adaption code in
> fs/btrfs/ioctl.c. The member clone_sources is only 32 bit wide in case
> of 32 bit user space.  For the ioctl RECEIVED_SUBVOL somebody already
> added code for the in this case also necessary translation. I took
> this as a template and wrote a patch (see below). The patch compiles
> and with the new kernel I seem to get valid data with send (I have to
> read it back yet, but I get about the expected amount and structure).

I'm not yet sure if this is the right approach, but I'm still
considering it. My suggestion is to add the union and force the width.
In case of the receive subvol this was not possible because it was
around an external structure.

The basic difference if we use the same structure definition, let the
compiler pick the pointer type width, and do the conversion in kernel
transparently (ie. the RECEIVED_SUBVOL workaround), or if we patch the
structure so the pointer magic happens in the userspace.

> This is a proof of concept patch; for example the compiler currently warns for
>   (args64->clone_sources = (__u64*)args32->clone_sources;
> and I would have to investigate how to properly convert the pointer.

args64->clone_sources = (__u64*)(long)args32->clone_sources;

should work.

> Further there are probably some issues with the formating of the source code.

Yeah there are some, but at this point we're not sure what's the right
approach so don't worry.

Re: [PATCH] btrfs: bugfix: handle FS_IOC32_{GETFLAGS,SETFLAGS,GETVERSION} in btrfs_ioctl

[David Sterba]

On Thu, Oct 29, 2015 at 08:22:34AM +0000, Luke Dashjr wrote:
> > > I don't see what is different with that implementation. All
> > > f2fs_compat_ioctl does is change cmd to the plain-IOC equivalent and
> > > call f2fs_ioctl with the same arg (compat_ptr merely causes a cast to
> > > void* and back, which AFAIK is a noop on 64-bit?). Am I missing
> > > something?
> > 
> > No, that's the idea. Add new calback for compat_ioctl, put it under
> > #ifdef CONFIG_COMPAT and do the same number switch.
> 
> Ok, someone else explained this to me. Please let me know if PATCHv2 (sent 
> separately) does not address the needed changes.

Patch is ok, thanks.

> > > I could try to just imitate it, but
> > > I'd rather know what is significant/going on to ensure I don't waste your
> > > time with code I don't even properly understand myself.
> > > 
> > > Perhaps by coincidence, the patch does at least in practice work
> > > (although at least `btrfs send` appears to be broken still, and I'm at a
> > > loss for how to approach fixing that).
> > 
> > The 'receive' 32bit/64bit was broken due to size difference in the ioctl
> > structure that led to different ioctl. This is transparently fixed, see
> > BTRFS_IOC_SET_RECEIVED_SUBVOL_32 at the top of ioctl.c.
> > 
> > In what way is SEND broken? There are only u64/s64 members in
> > btrfs_ioctl_send_args, I don't see how this could break on 32/64
> > userspace/kernel.
> 
> I've investigated this now, and it seems to be the pointer-type clone_sources 
> member of struct btrfs_ioctl_send_args. I can't think of a perfect way to fix 
> this, but it might not be *too* ugly to:
> - replace the current clone_sources with a u64 that must always be (u64)-1;
>   this causes older kernels to error cleanly if called with a new ioctl data
> - use the top 1 or 2 bits of flags to indicate sizeof(void*) as it appears to
>   userspace OR just use up reserved[0] for pointer size:
>       io_send.ptr_size = sizeof(void*);
> - replace one of the reserved fields with the new clone_sources

All the change seem too intrusive or not so easy to use.

I suggest to add an anonymous union and add a u64 member that would
force the type width:

struct btrfs_ioctl_send_args {
        __s64 send_fd;                  /* in */
        __u64 clone_sources_count;      /* in */
	union {
		__u64 __user *clone_sources;    /* in */
		u64 __pointer_alignment;
	};
        __u64 parent_root;              /* in */
        __u64 flags;                    /* in */
        __u64 reserved[4];              /* in */
};

> The way it was done for receive seems like it might not work for non-x86 
> compat interfaces (eg, MIPS n32) - but I could be wrong.

Possible, but I don't see right now how it would not work on eg. mips32.
unless sizeof(long) is 8 bytes there and CONFIG_64BIT is not defined.

Re: [PATCH] btrfs: bugfix: handle FS_IOC32_{GETFLAGS,SETFLAGS,GETVERSION} in btrfs_ioctl

[Luke Dashjr]

On Thursday, October 29, 2015 2:39:32 PM David Sterba wrote:
> On Thu, Oct 29, 2015 at 08:22:34AM +0000, Luke Dashjr wrote:
> > > In what way is SEND broken? There are only u64/s64 members in
> > > btrfs_ioctl_send_args, I don't see how this could break on 32/64
> > > userspace/kernel.
> > 
> > I've investigated this now, and it seems to be the pointer-type
> > clone_sources member of struct btrfs_ioctl_send_args. I can't think of a
> > perfect way to fix this, but it might not be *too* ugly to:
> > - replace the current clone_sources with a u64 that must always be
> > (u64)-1;
> > 
> >   this causes older kernels to error cleanly if called with a new ioctl
> >   data
> > 
> > - use the top 1 or 2 bits of flags to indicate sizeof(void*) as it
> > appears to
> > 
> >   userspace OR just use up reserved[0] for pointer size:
> >       io_send.ptr_size = sizeof(void*);
> > 
> > - replace one of the reserved fields with the new clone_sources
> 
> All the change seem too intrusive or not so easy to use.
> 
> I suggest to add an anonymous union and add a u64 member that would
> force the type width:
> 
> struct btrfs_ioctl_send_args {
>         __s64 send_fd;                  /* in */
>         __u64 clone_sources_count;      /* in */
> 	union {
> 		__u64 __user *clone_sources;    /* in */
> 		u64 __pointer_alignment;
> 	};
>         __u64 parent_root;              /* in */
>         __u64 flags;                    /* in */
>         __u64 reserved[4];              /* in */
> };

What guarantees the union to position clone_sources in the LSB of 
__pointer_alignment (rather than the MSB side)?

> > The way it was done for receive seems like it might not work for non-x86
> > compat interfaces (eg, MIPS n32) - but I could be wrong.
> 
> Possible, but I don't see right now how it would not work on eg. mips32.
> unless sizeof(long) is 8 bytes there and CONFIG_64BIT is not defined.

n32 is a MIPS64 ABI, like the new x32 ABI for x86_64 machines, so I would 
expect sizeof(long) to be 8 bytes, and am uncertain of if this implies any 
particular alignment. (But I don't have any MIPS systems, so this isn't 
something I'm too concerned with myself.)

Luke

Re: [PATCH] btrfs: bugfix: handle FS_IOC32_{GETFLAGS,SETFLAGS,GETVERSION} in btrfs_ioctl

[Thomas Rohwer]

> I suggest to add an anonymous union and add a u64 member that would
> force the type width:
>
> struct btrfs_ioctl_send_args {
>          __s64 send_fd;                  /* in */
>          __u64 clone_sources_count;      /* in */
> 	union {
> 		__u64 __user *clone_sources;    /* in */
> 		u64 __pointer_alignment;
> 	};
>          __u64 parent_root;              /* in */
>          __u64 flags;                    /* in */
>          __u64 reserved[4];              /* in */
> };

I am no expert, but would this change alone modify the user space ABI of a 32-bit Linux kernel?
I.e. people in the (presumably currently working) btrfs-send situation (32-bit) user space/32-bit kernel
would have to upgrade user space tools and kernel at the same time. Otherwise, they will encounter a non-working setup.
I think, my suggested patch does not change any working ABI, and no change to the user
space tools are necessary.


Sincerely,

Thomas Rohwer

Re: [PATCH] btrfs: bugfix: handle FS_IOC32_{GETFLAGS,SETFLAGS,GETVERSION} in btrfs_ioctl

[Luke Dashjr]

On Thursday, October 29, 2015 7:36:35 PM Thomas Rohwer wrote:
> > I suggest to add an anonymous union and add a u64 member that would
> > force the type width:
> > 
> > struct btrfs_ioctl_send_args {
> > 
> >          __s64 send_fd;                  /* in */
> >          __u64 clone_sources_count;      /* in */
> > 	
> > 	union {
> > 	
> > 		__u64 __user *clone_sources;    /* in */
> > 		u64 __pointer_alignment;
> > 	
> > 	};
> > 	
> >          __u64 parent_root;              /* in */
> >          __u64 flags;                    /* in */
> >          __u64 reserved[4];              /* in */
> > 
> > };
> 
> I am no expert, but would this change alone modify the user space ABI of a
> 32-bit Linux kernel? I.e. people in the (presumably currently working)
> btrfs-send situation (32-bit) user space/32-bit kernel would have to
> upgrade user space tools and kernel at the same time. Otherwise, they will
> encounter a non-working setup.

Yes, it would, but this appears to already be the case for btrfs-progs in 
general.

> I think, my suggested patch does not change any working ABI, and no change
> to the user space tools are necessary.

Don't the user space tools need to call a different ioctl?

Luke