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

[Thomas Rohwer <tr@ohwer.de>]

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