LU-14428 libcfs: discard cfs_trace_copyin_string()

Instead of cfs_trace_copyin_string(), use memdup_user_nul().
This combines the allocation with the copyin, and nul-terminates.

The resulting code is a lot simpler.

Lustre-change: https://review.whamcloud.com/41490
Lustre-commit: 67af976c806994cec27414d24b43f6519d72c240

LU-14788 lnet: check memdup_user_nul using IS_ERR

Crash in __proc_lnet_portal_rotor. memdup_user_nul returns an ERR_PTR
on error, not a NULL pointer. IS_ERR and PTR_ERR functions have to be
used to check and return the correct error code. The fix has been
applied in other locations having the wrong check.

Lustre-change: https://review.whamcloud.com/44091
Lustre-commit: 449d046e55a42cc4d1c4ab0217551cded1864bc4

Signed-off-by: Alex Zhuravlev <bzzz@whamcloud.com>
Change-Id: I089c5da96b59ec62d177aea2f3d170bf751c6fec
Reviewed-on: https://review.whamcloud.com/c/ex/lustre-release/+/48835
Tested-by: jenkins <devops@whamcloud.com>
Tested-by: Maloo <maloo@whamcloud.com>
Reviewed-by: Andreas Dilger <adilger@whamcloud.com>

diff --git a/libcfs/libcfs/module.c b/libcfs/libcfs/module.c
index 1e5f6f1..0fbfec7 100644 (file)
--- a/libcfs/libcfs/module.c
+++ b/libcfs/libcfs/module.c
@@ -287,17 +287,16 @@ static int __proc_dobitmasks(void *data, int write,
                             loff_t pos, void __user *buffer, int nob)
 {
        const int     tmpstrlen = 512;
-       char         *tmpstr;
+       char         *tmpstr = NULL;
        int           rc;
        unsigned int *mask = data;
        int           is_subsys = (mask == &libcfs_subsystem_debug) ? 1 : 0;
        int           is_printk = (mask == &libcfs_printk) ? 1 : 0;
 
-       rc = cfs_trace_allocate_string_buffer(&tmpstr, tmpstrlen);
-       if (rc < 0)
-               return rc;
-
        if (!write) {
+               rc = cfs_trace_allocate_string_buffer(&tmpstr, tmpstrlen);
+               if (rc < 0)
+                       return rc;
                libcfs_debug_mask2str(tmpstr, tmpstrlen, *mask, is_subsys);
                rc = strlen(tmpstr);
 
@@ -308,13 +307,11 @@ static int __proc_dobitmasks(void *data, int write,
                                                      tmpstr + pos, "\n");
                }
        } else {
-               rc = cfs_trace_copyin_string(tmpstr, tmpstrlen, buffer, nob);
-               if (rc < 0) {
-                       kfree(tmpstr);
-                       return rc;
-               }
+               tmpstr = memdup_user_nul(buffer, nob);
+               if (IS_ERR(tmpstr))
+                       return PTR_ERR(tmpstr);
 
-               rc = libcfs_debug_str2mask(mask, tmpstr, is_subsys);
+               rc = libcfs_debug_str2mask(mask, strim(tmpstr), is_subsys);
                /* Always print LBUG/LASSERT to console, so keep this mask */
                if (is_printk)
                        *mask |= D_EMERG;

diff --git a/libcfs/libcfs/tracefile.c b/libcfs/libcfs/tracefile.c
index 925ea1d..e7aad28 100644 (file)
--- a/libcfs/libcfs/tracefile.c
+++ b/libcfs/libcfs/tracefile.c
@@ -827,33 +827,6 @@ void cfs_trace_flush_pages(void)
        }
 }
 
-int cfs_trace_copyin_string(char *knl_buffer, int knl_buffer_nob,
-                           const char __user *usr_buffer, int usr_buffer_nob)
-{
-        int    nob;
-
-        if (usr_buffer_nob > knl_buffer_nob)
-                return -EOVERFLOW;
-
-       if (copy_from_user(knl_buffer, usr_buffer, usr_buffer_nob))
-                return -EFAULT;
-
-        nob = strnlen(knl_buffer, usr_buffer_nob);
-       while (--nob >= 0)                      /* strip trailing whitespace */
-                if (!isspace(knl_buffer[nob]))
-                        break;
-
-        if (nob < 0)                            /* empty string */
-                return -EINVAL;
-
-        if (nob == knl_buffer_nob)              /* no space to terminate */
-                return -EOVERFLOW;
-
-        knl_buffer[nob + 1] = 0;                /* terminate */
-        return 0;
-}
-EXPORT_SYMBOL(cfs_trace_copyin_string);
-
 int cfs_trace_copyout_string(char __user *usr_buffer, int usr_buffer_nob,
                              const char *knl_buffer, char *append)
 {
@@ -893,26 +866,22 @@ int cfs_trace_allocate_string_buffer(char **str, int nob)
 
 int cfs_trace_dump_debug_buffer_usrstr(void __user *usr_str, int usr_str_nob)
 {
-        char         *str;
-        int           rc;
+       char *str;
+       char *path;
+       int rc;
 
-        rc = cfs_trace_allocate_string_buffer(&str, usr_str_nob + 1);
-        if (rc != 0)
-                return rc;
+       str = memdup_user_nul(usr_str, usr_str_nob);
+       if (IS_ERR(str))
+               return PTR_ERR(str);
 
-        rc = cfs_trace_copyin_string(str, usr_str_nob + 1,
-                                     usr_str, usr_str_nob);
-        if (rc != 0)
-                goto out;
-
-        if (str[0] != '/') {
-                rc = -EINVAL;
-                goto out;
-        }
-        rc = cfs_tracefile_dump_all_pages(str);
-out:
+       path = strim(str);
+       if (path[0] != '/')
+               rc = -EINVAL;
+       else
+               rc = cfs_tracefile_dump_all_pages(path);
        kfree(str);
-        return rc;
+
+       return rc;
 }
 
 int cfs_trace_daemon_command(char *str)
@@ -956,20 +925,17 @@ int cfs_trace_daemon_command(char *str)
 
 int cfs_trace_daemon_command_usrstr(void __user *usr_str, int usr_str_nob)
 {
-        char *str;
-        int   rc;
-
-        rc = cfs_trace_allocate_string_buffer(&str, usr_str_nob + 1);
-        if (rc != 0)
-                return rc;
+       char *str;
+       int   rc;
 
-        rc = cfs_trace_copyin_string(str, usr_str_nob + 1,
-                                 usr_str, usr_str_nob);
-        if (rc == 0)
-                rc = cfs_trace_daemon_command(str);
+       str = memdup_user_nul(usr_str, usr_str_nob);
+       if (IS_ERR(str))
+               return PTR_ERR(str);
 
+       rc = cfs_trace_daemon_command(strim(str));
        kfree(str);
-        return rc;
+
+       return rc;
 }
 
 int cfs_trace_set_debug_mb(int mb)

diff --git a/libcfs/libcfs/tracefile.h b/libcfs/libcfs/tracefile.h
index 8413413..601442d 100644 (file)
--- a/libcfs/libcfs/tracefile.h
+++ b/libcfs/libcfs/tracefile.h
@@ -54,8 +54,6 @@ void cfs_tracefile_exit(void);
 
 
 
-int cfs_trace_copyin_string(char *knl_buffer, int knl_buffer_nob,
-                           const char __user *usr_buffer, int usr_buffer_nob);
 int cfs_trace_copyout_string(char __user *usr_buffer, int usr_buffer_nob,
                              const char *knl_str, char *append);
 int cfs_trace_allocate_string_buffer(char **str, int nob);

diff --git a/lnet/lnet/router_proc.c b/lnet/lnet/router_proc.c
index 6ea567d..fa36882 100644 (file)
--- a/lnet/lnet/router_proc.c
+++ b/lnet/lnet/router_proc.c
@@ -796,11 +796,11 @@ static int __proc_lnet_portal_rotor(void *data, int write,
        int             rc;
        int             i;
 
-       LIBCFS_ALLOC(buf, buf_len);
-       if (buf == NULL)
-               return -ENOMEM;
-
        if (!write) {
+               LIBCFS_ALLOC(buf, buf_len);
+               if (buf == NULL)
+                       return -ENOMEM;
+
                lnet_res_lock(0);
 
                for (i = 0; portal_rotors[i].pr_value >= 0; i++) {
@@ -821,14 +821,16 @@ static int __proc_lnet_portal_rotor(void *data, int write,
                        rc = 0;
                } else {
                        rc = cfs_trace_copyout_string(buffer, nob,
-                                       buf + pos, "\n");
+                                                     buf + pos, "\n");
                }
-               goto out;
+               LIBCFS_FREE(buf, buf_len);
+
+               return rc;
        }
 
-       rc = cfs_trace_copyin_string(buf, buf_len, buffer, nob);
-       if (rc < 0)
-               goto out;
+       buf = memdup_user_nul(buffer, nob);
+       if (IS_ERR(buf))
+               return PTR_ERR(buf);
 
        tmp = strim(buf);
 
@@ -843,8 +845,8 @@ static int __proc_lnet_portal_rotor(void *data, int write,
                }
        }
        lnet_res_unlock(0);
-out:
-       LIBCFS_FREE(buf, buf_len);
+       kfree(buf);
+
        return rc;
 }