LU-5435 libcfs: copy out ioctl inline buffer

- libcfs_ioctl_popdata should copy out inline buffers.
- code cleanup for libcfs ioctl handler
- error number fix for obd_ioctl_getdata
- add new function libcfs_ioctl_unpack for upcoming patches

Signed-off-by: Liang Zhen <liang.zhen@intel.com>
Change-Id: I3969e4e96b1dc0c2f6c3e86039b97af5482edc74
Reviewed-on: http://review.whamcloud.com/11313
Tested-by: Jenkins
Reviewed-by: Bobi Jam <bobijam@gmail.com>
Reviewed-by: Johann Lombardi <johann.lombardi@intel.com>
Tested-by: Maloo <hpdd-maloo@intel.com>
Reviewed-by: Oleg Drokin <oleg.drokin@intel.com>

diff --git a/libcfs/include/libcfs/libcfs_ioctl.h b/libcfs/include/libcfs/libcfs_ioctl.h
index b185a96..eab645c 100644 (file)
--- a/libcfs/include/libcfs/libcfs_ioctl.h
+++ b/libcfs/include/libcfs/libcfs_ioctl.h
@@ -49,6 +49,9 @@ struct libcfs_ioctl_hdr {
        __u32 ioc_version;
 };
 
+/** max size to copy from userspace */
+#define LIBCFS_IOC_DATA_MAX    (128 * 1024)
+
 struct libcfs_ioctl_data {
        struct libcfs_ioctl_hdr ioc_hdr;
 
@@ -75,7 +78,6 @@ struct libcfs_ioctl_data {
 
 #define ioc_priority ioc_u32[0]
 
-
 struct libcfs_debug_ioctl_data
 {
        struct libcfs_ioctl_hdr hdr;
@@ -125,7 +127,6 @@ struct libcfs_ioctl_handler {
 #define IOC_LIBCFS_CLEAR_DEBUG             _IOWR('e', 31, IOCTL_LIBCFS_TYPE)
 #define IOC_LIBCFS_MARK_DEBUG              _IOWR('e', 32, IOCTL_LIBCFS_TYPE)
 #define IOC_LIBCFS_MEMHOG                  _IOWR('e', 36, IOCTL_LIBCFS_TYPE)
-#define IOC_LIBCFS_PING_TEST               _IOWR('e', 37, IOCTL_LIBCFS_TYPE)
 /* lnet ioctls */
 #define IOC_LIBCFS_GET_NI                 _IOWR('e', 50, IOCTL_LIBCFS_TYPE)
 #define IOC_LIBCFS_FAIL_NID               _IOWR('e', 51, IOCTL_LIBCFS_TYPE)
@@ -251,11 +252,22 @@ static inline bool libcfs_ioctl_is_invalid(struct libcfs_ioctl_data *data)
 
 extern int libcfs_register_ioctl(struct libcfs_ioctl_handler *hand);
 extern int libcfs_deregister_ioctl(struct libcfs_ioctl_handler *hand);
-extern int libcfs_ioctl_getdata(struct libcfs_ioctl_hdr *buf, __u32 buf_len,
-                               const void __user *arg);
-extern int libcfs_ioctl_getdata_len(const struct libcfs_ioctl_hdr __user *arg,
-                                   __u32 *buf_len);
-extern int libcfs_ioctl_popdata(void __user *arg, void *buf, int size);
+extern int libcfs_ioctl_getdata(struct libcfs_ioctl_hdr **hdr_pp,
+                               struct libcfs_ioctl_hdr __user *uparam);
+
+static inline int libcfs_ioctl_popdata(struct libcfs_ioctl_hdr *hdr,
+                                      struct libcfs_ioctl_hdr __user *uparam)
+{
+        if (copy_to_user(uparam, hdr, hdr->ioc_len))
+               return -EFAULT;
+        return 0;
+}
+
+static inline void libcfs_ioctl_freedata(struct libcfs_ioctl_hdr *hdr)
+{
+       LIBCFS_FREE(hdr, hdr->ioc_len);
+}
+
 #endif
 
 extern int libcfs_ioctl_data_adjust(struct libcfs_ioctl_data *data);

diff --git a/libcfs/include/libcfs/util/libcfsutil_ioctl.h b/libcfs/include/libcfs/util/libcfsutil_ioctl.h
index fb08c42..09ea0f7 100644 (file)
--- a/libcfs/include/libcfs/util/libcfsutil_ioctl.h
+++ b/libcfs/include/libcfs/util/libcfsutil_ioctl.h
@@ -40,6 +40,7 @@
 /* FIXME - rename these to libcfs_ */
 
 int libcfs_ioctl_pack(struct libcfs_ioctl_data *data, char **pbuf, int max);
+void libcfs_ioctl_unpack(struct libcfs_ioctl_data *data, char *pbuf);
 typedef int (ioc_handler_t)(int dev_id, unsigned int opc, void *buf);
 void set_ioc_handler(ioc_handler_t *handler);
 int register_ioc_dev(int dev_id, const char * dev_name, int major, int minor);

diff --git a/libcfs/libcfs/linux/linux-module.c b/libcfs/libcfs/linux/linux-module.c
index 6307129..668e3c5 100644 (file)
--- a/libcfs/libcfs/linux/linux-module.c
+++ b/libcfs/libcfs/linux/linux-module.c
@@ -42,8 +42,10 @@
 
 int libcfs_ioctl_data_adjust(struct libcfs_ioctl_data *data)
 {
+       ENTRY;
+
        if (libcfs_ioctl_is_invalid(data)) {
-               CERROR("LNET: ioctl not correctly formatted\n");
+               CERROR("libcfs ioctl: parameter not correctly formatted\n");
                RETURN(-EINVAL);
        }
 
@@ -52,48 +54,50 @@ int libcfs_ioctl_data_adjust(struct libcfs_ioctl_data *data)
 
        if (data->ioc_inllen2 != 0)
                data->ioc_inlbuf2 = &data->ioc_bulk[0] +
-                       cfs_size_round(data->ioc_inllen1);
+                                   cfs_size_round(data->ioc_inllen1);
 
        RETURN(0);
 }
 
-int libcfs_ioctl_getdata_len(const struct libcfs_ioctl_hdr __user *arg,
-                            __u32 *len)
+int libcfs_ioctl_getdata(struct libcfs_ioctl_hdr **hdr_pp,
+                        struct libcfs_ioctl_hdr __user *uhdr)
 {
-       struct libcfs_ioctl_hdr hdr;
+       struct libcfs_ioctl_hdr   hdr;
+       int err = 0;
        ENTRY;
 
-       if (copy_from_user(&hdr, arg, sizeof(hdr)))
+       if (copy_from_user(&hdr, uhdr, sizeof(hdr)))
                RETURN(-EFAULT);
 
        if (hdr.ioc_version != LIBCFS_IOCTL_VERSION &&
            hdr.ioc_version != LIBCFS_IOCTL_VERSION2) {
-               CERROR("LNET: version mismatch expected %#x, got %#x\n",
+               CERROR("libcfs ioctl: version mismatch expected %#x, got %#x\n",
                       LIBCFS_IOCTL_VERSION, hdr.ioc_version);
                RETURN(-EINVAL);
        }
 
-       *len = hdr.ioc_len;
+       if (hdr.ioc_len < sizeof(struct libcfs_ioctl_data)) {
+               CERROR("libcfs ioctl: user buffer too small for ioctl\n");
+               RETURN(-EINVAL);
+       }
 
-       RETURN(0);
-}
+       if (hdr.ioc_len > LIBCFS_IOC_DATA_MAX) {
+               CERROR("libcfs ioctl: user buffer is too large %d/%d\n",
+                      hdr.ioc_len, LIBCFS_IOC_DATA_MAX);
+               RETURN(-EINVAL);
+       }
 
-int libcfs_ioctl_getdata(struct libcfs_ioctl_hdr *buf, __u32 buf_len,
-                        const void __user *arg)
-{
-       ENTRY;
+       LIBCFS_ALLOC(*hdr_pp, hdr.ioc_len);
+       if (*hdr_pp == NULL)
+               RETURN(-ENOMEM);
 
-       if (copy_from_user(buf, arg, buf_len))
-               RETURN(-EINVAL);
+       if (copy_from_user(*hdr_pp, uhdr, hdr.ioc_len))
+               GOTO(failed, err = -EFAULT);
 
        RETURN(0);
-}
-
-int libcfs_ioctl_popdata(void __user *arg, void *data, int size)
-{
-       if (copy_to_user(arg, data, size))
-               return -EFAULT;
-       return 0;
+failed:
+       libcfs_ioctl_freedata(*hdr_pp);
+       RETURN(err);
 }
 
 extern struct cfs_psdev_ops          libcfs_psdev_ops;

diff --git a/libcfs/libcfs/module.c b/libcfs/libcfs/module.c
index ee678ec..b35bade 100644 (file)
--- a/libcfs/libcfs/module.c
+++ b/libcfs/libcfs/module.c
@@ -35,8 +35,6 @@
  */
 
 #define DEBUG_SUBSYSTEM S_LNET
-#define LNET_MAX_IOCTL_BUF_LEN (sizeof(struct lnet_ioctl_net_config) + \
-                                sizeof(struct lnet_ioctl_config_data))
 
 #include <libcfs/libcfs.h>
 #include <libcfs/libcfs_crypto.h>
@@ -222,71 +220,65 @@ int libcfs_deregister_ioctl(struct libcfs_ioctl_handler *hand)
 }
 EXPORT_SYMBOL(libcfs_deregister_ioctl);
 
-static int libcfs_ioctl_handle(struct cfs_psdev_file *pfile, unsigned long cmd,
-                              void __user *arg, struct libcfs_ioctl_hdr *hdr)
+static int libcfs_ioctl(struct cfs_psdev_file *pfile,
+                       unsigned long cmd, void __user *uparam)
 {
        struct libcfs_ioctl_data *data = NULL;
-       int err;
+       struct libcfs_ioctl_hdr  *hdr;
+       int                       err;
        ENTRY;
 
-       /* The libcfs_ioctl_data_adjust() function performs adjustment
-        * operations on the libcfs_ioctl_data structure to make
-        * it usable by the code.  This doesn't need to be called
-        * for new data structures added. */
+       /* 'cmd' and permissions get checked in our arch-specific caller */
+       err = libcfs_ioctl_getdata(&hdr, uparam);
+       if (err != 0) {
+               CDEBUG_LIMIT(D_ERROR,
+                            "libcfs ioctl: data header error %d\n", err);
+               RETURN(err);
+       }
+
        if (hdr->ioc_version == LIBCFS_IOCTL_VERSION) {
+               /* The libcfs_ioctl_data_adjust() function performs adjustment
+                * operations on the libcfs_ioctl_data structure to make
+                * it usable by the code.  This doesn't need to be called
+                * for new data structures added. */
                data = container_of(hdr, struct libcfs_ioctl_data, ioc_hdr);
                err = libcfs_ioctl_data_adjust(data);
-               if (err != 0) {
-                       RETURN(err);
-               }
+               if (err != 0)
+                       GOTO(out, err);
        }
 
+       CDEBUG(D_IOCTL, "libcfs ioctl cmd %lu\n", cmd);
        switch (cmd) {
        case IOC_LIBCFS_CLEAR_DEBUG:
                libcfs_debug_clear_buffer();
-               RETURN(0);
+               break;
        /*
         * case IOC_LIBCFS_PANIC:
         * Handled in arch/cfs_module.c
         */
        case IOC_LIBCFS_MARK_DEBUG:
-               if (data->ioc_inlbuf1 == NULL ||
+               if (data == NULL ||
+                   data->ioc_inlbuf1 == NULL ||
                    data->ioc_inlbuf1[data->ioc_inllen1 - 1] != '\0')
-                       RETURN(-EINVAL);
+                       GOTO(out, err = -EINVAL);
+
                libcfs_debug_mark_buffer(data->ioc_inlbuf1);
-               RETURN(0);
+               break;
+
        case IOC_LIBCFS_MEMHOG:
-               if (pfile->private_data == NULL) {
-                       err = -EINVAL;
-               } else {
+               if (data == NULL)
+                       GOTO(out, err = -EINVAL);
+
+               if (pfile->private_data == NULL)
+                       GOTO(out, err = -EINVAL);
+
+               kportal_memhog_free(pfile->private_data);
+               err = kportal_memhog_alloc(pfile->private_data,
+                                          data->ioc_count, data->ioc_flags);
+               if (err != 0)
                        kportal_memhog_free(pfile->private_data);
-                       /* XXX The ioc_flags is not GFP flags now, need to
-                        * be fixed */
-                       err = kportal_memhog_alloc(pfile->private_data,
-                                                  data->ioc_count,
-                                                  data->ioc_flags);
-                       if (err != 0)
-                               kportal_memhog_free(pfile->private_data);
-               }
                break;
 
-       case IOC_LIBCFS_PING_TEST: {
-               extern void (kping_client)(struct libcfs_ioctl_data *);
-               void (*ping)(struct libcfs_ioctl_data *);
-
-               CDEBUG(D_IOCTL, "doing %d pings to nid %s (%s)\n",
-                      data->ioc_count, libcfs_nid2str(data->ioc_nid),
-                      libcfs_nid2str(data->ioc_nid));
-               ping = symbol_get(kping_client);
-               if (!ping) {
-                       CERROR("symbol_get failed\n");
-               } else {
-                       ping(data);
-                       symbol_put(kping_client);
-               }
-               RETURN(0);
-       }
-
        default: {
                struct libcfs_ioctl_handler *hand;
 
@@ -294,60 +286,21 @@ static int libcfs_ioctl_handle(struct cfs_psdev_file *pfile, unsigned long cmd,
                down_read(&ioctl_list_sem);
                list_for_each_entry(hand, &ioctl_list, item) {
                        err = hand->handle_ioctl(cmd, hdr);
-                       if (err != -EINVAL) {
-                               if (err == 0)
-                                       err = libcfs_ioctl_popdata(arg,
-                                                       hdr, hdr->ioc_len);
-                               break;
-                       }
+                       if (err == -EINVAL)
+                               continue;
+
+                       if (err == 0)
+                               err = libcfs_ioctl_popdata(hdr, uparam);
+                       break;
                }
                up_read(&ioctl_list_sem);
-               break;
-       }
+               break; }
        }
-
-       RETURN(err);
-}
-
-static int libcfs_ioctl(struct cfs_psdev_file *pfile,
-                       unsigned long cmd, void __user *arg)
-{
-       struct libcfs_ioctl_hdr *hdr;
-       int err = 0;
-       __u32 buf_len;
-       ENTRY;
-
-       err = libcfs_ioctl_getdata_len(arg, &buf_len);
-       if (err != 0)
-               RETURN(err);
-
-       /*
-        * do a check here to restrict the size of the memory
-        * to allocate to guard against DoS attacks.
-        */
-       if (buf_len > LNET_MAX_IOCTL_BUF_LEN) {
-               CERROR("LNET: user buffer exceeds kernel buffer\n");
-               RETURN(-EINVAL);
-       }
-
-       LIBCFS_ALLOC_GFP(hdr, buf_len, GFP_IOFS);
-       if (hdr == NULL)
-               RETURN(-ENOMEM);
-
-       /* 'cmd' and permissions get checked in our arch-specific caller */
-       if (libcfs_ioctl_getdata(hdr, buf_len, arg)) {
-               CERROR("LNET ioctl: data error\n");
-               GOTO(out, err = -EINVAL);
-       }
-
-       err = libcfs_ioctl_handle(pfile, cmd, arg, hdr);
-
 out:
-       LIBCFS_FREE(hdr, buf_len);
+       libcfs_ioctl_freedata(hdr);
        RETURN(err);
 }
 
-
 struct cfs_psdev_ops libcfs_psdev_ops = {
         libcfs_psdev_open,
         libcfs_psdev_release,

diff --git a/libcfs/libcfs/util/l_ioctl.c b/libcfs/libcfs/util/l_ioctl.c
index e27c404..fe35073 100644 (file)
--- a/libcfs/libcfs/util/l_ioctl.c
+++ b/libcfs/libcfs/util/l_ioctl.c
@@ -340,3 +340,21 @@ int libcfs_ioctl_pack(struct libcfs_ioctl_data *data, char **pbuf,
        return 0;
 }
 
+void
+libcfs_ioctl_unpack(struct libcfs_ioctl_data *data, char *pbuf)
+{
+       struct libcfs_ioctl_data *overlay = (struct libcfs_ioctl_data *)pbuf;
+       char *ptr;
+
+       /* Preserve the caller's buffer pointers */
+       overlay->ioc_inlbuf1 = data->ioc_inlbuf1;
+       overlay->ioc_inlbuf2 = data->ioc_inlbuf2;
+
+       memcpy(data, pbuf, sizeof(*data));
+       ptr = &overlay->ioc_bulk[0];
+
+       if (data->ioc_inlbuf1 != NULL)
+               LOGU(data->ioc_inlbuf1, data->ioc_inllen1, ptr);
+       if (data->ioc_inlbuf2 != NULL)
+               LOGU(data->ioc_inlbuf2, data->ioc_inllen2, ptr);
+}

diff --git a/lnet/lnet/api-ni.c b/lnet/lnet/api-ni.c
index 7fc2158..bb08436 100644 (file)
--- a/lnet/lnet/api-ni.c
+++ b/lnet/lnet/api-ni.c
@@ -2075,6 +2075,8 @@ LNetCtl(unsigned int cmd, void *arg)
        lnet_ni_t                *ni;
        int                       rc;
 
+       CLASSERT(LIBCFS_IOC_DATA_MAX >= sizeof(struct lnet_ioctl_net_config) +
+                                       sizeof(struct lnet_ioctl_config_data));
        LASSERT(the_lnet.ln_init);
 
        switch (cmd) {

diff --git a/lustre/obdclass/linux/linux-module.c b/lustre/obdclass/linux/linux-module.c
index b8b86de..8b114d2 100644 (file)
--- a/lustre/obdclass/linux/linux-module.c
+++ b/lustre/obdclass/linux/linux-module.c
@@ -80,15 +80,13 @@ int proc_version;
 /* buffer MUST be at least the size of obd_ioctl_hdr */
 int obd_ioctl_getdata(char **buf, int *len, void __user *arg)
 {
-        struct obd_ioctl_hdr hdr;
-        struct obd_ioctl_data *data;
-        int err;
-        int offset = 0;
-        ENTRY;
+       struct obd_ioctl_hdr hdr;
+       struct obd_ioctl_data *data;
+       int offset = 0;
+       ENTRY;
 
-       err = copy_from_user(&hdr, arg, sizeof(hdr));
-        if ( err )
-                RETURN(err);
+       if (copy_from_user(&hdr, arg, sizeof(hdr)))
+               RETURN(-EFAULT);
 
         if (hdr.ioc_version != OBD_IOCTL_VERSION) {
                 CERROR("Version mismatch kernel (%x) vs application (%x)\n",
@@ -120,11 +118,10 @@ int obd_ioctl_getdata(char **buf, int *len, void __user *arg)
         *len = hdr.ioc_len;
         data = (struct obd_ioctl_data *)*buf;
 
-       err = copy_from_user(*buf, arg, hdr.ioc_len);
-        if ( err ) {
-                OBD_FREE_LARGE(*buf, hdr.ioc_len);
-                RETURN(err);
-        }
+       if (copy_from_user(*buf, arg, hdr.ioc_len)) {
+               OBD_FREE_LARGE(*buf, hdr.ioc_len);
+               RETURN(-EFAULT);
+       }
 
         if (obd_ioctl_is_invalid(data)) {
                 CERROR("ioctl not correctly formatted\n");
@@ -147,23 +144,20 @@ int obd_ioctl_getdata(char **buf, int *len, void __user *arg)
                 offset += cfs_size_round(data->ioc_inllen3);
         }
 
-        if (data->ioc_inllen4) {
-                data->ioc_inlbuf4 = &data->ioc_bulk[0] + offset;
-        }
+       if (data->ioc_inllen4)
+               data->ioc_inlbuf4 = &data->ioc_bulk[0] + offset;
 
-        EXIT;
-        return 0;
+       RETURN(0);
 }
 EXPORT_SYMBOL(obd_ioctl_getdata);
 
 int obd_ioctl_popdata(void __user *arg, void *data, int len)
 {
        int err;
+       ENTRY;
 
-       err = copy_to_user(arg, data, len);
-       if (err)
-               err = -EFAULT;
-       return err;
+       err = copy_to_user(arg, data, len) ? -EFAULT : 0;
+       RETURN(err);
 }
 EXPORT_SYMBOL(obd_ioctl_popdata);