LU-17497 obdclass: check upcall incorrect values

Identity upcall is set via lctl set_param mdt.*.identity_upcall=xxx,
and rsi upcall is set via lctl set_param sptlrpc.gss.rsi_upcall=xxx.
Possible values are a valid path to an executable, and also NONE to
disable identity upcall.
Add an upcall cache function that checks the user provided string, to
make sure we do not store an invalid value. And print a message to
stdout to explain the accepted values.

Add sanity-sec test_69 to exercise this.

Signed-off-by: Sebastien Buisson <sbuisson@ddn.com>
Change-Id: Iaf59e72aa1612f5579db175d8999dcf0053308ed
Reviewed-on: https://review.whamcloud.com/c/fs/lustre-release/+/53878
Tested-by: jenkins <devops@whamcloud.com>
Tested-by: Maloo <maloo@whamcloud.com>
Reviewed-by: Aurelien Degremont <adegremont@nvidia.com>
Reviewed-by: Andreas Dilger <adilger@whamcloud.com>
Reviewed-by: Oleg Drokin <green@whamcloud.com>

diff --git a/lustre/include/upcall_cache.h b/lustre/include/upcall_cache.h
index fa2c590..0ae9c1a 100644 (file)
--- a/lustre/include/upcall_cache.h
+++ b/lustre/include/upcall_cache.h
@@ -151,6 +151,8 @@ struct upcall_cache {
        struct upcall_cache_ops *uc_ops;
 };
 
+int upcall_cache_set_upcall(struct upcall_cache *cache, const char *buffer,
+                           size_t count, bool path_only);
 struct upcall_cache_entry *upcall_cache_get_entry(struct upcall_cache *cache,
                                                  __u64 key, void *args);
 void upcall_cache_get_entry_raw(struct upcall_cache_entry *entry);

diff --git a/lustre/mdt/mdt_lproc.c b/lustre/mdt/mdt_lproc.c
index 31d5f47..8d0fd4e 100644 (file)
--- a/lustre/mdt/mdt_lproc.c
+++ b/lustre/mdt/mdt_lproc.c
@@ -296,17 +296,16 @@ static ssize_t identity_upcall_store(struct kobject *kobj,
                                              obd_kset.kobj);
        struct mdt_device *mdt = mdt_dev(obd->obd_lu_dev);
        struct upcall_cache *hash = mdt->mdt_identity_cache;
+       int rc;
 
-       if (count >= UC_CACHE_UPCALL_MAXPATH) {
-               CERROR("%s: identity upcall too long\n", mdt_obd_name(mdt));
-               return -EINVAL;
+       rc = upcall_cache_set_upcall(hash, buffer, count, false);
+       if (rc) {
+               CERROR("%s: incorrect identity upcall %.*s. Valid values for mdt.%s.identity_upcall are NONE, or an executable pathname: rc = %d\n",
+                      mdt_obd_name(mdt), (int)count, buffer,
+                      mdt_obd_name(mdt), rc);
+               return rc;
        }
 
-       /* Remove any extraneous bits from the upcall (e.g. linefeeds) */
-       down_write(&hash->uc_upcall_rwsem);
-       sscanf(buffer, "%s", hash->uc_upcall);
-       up_write(&hash->uc_upcall_rwsem);
-
        if (strcmp(hash->uc_name, mdt_obd_name(mdt)) != 0)
                CWARN("%s: write to upcall name %s\n",
                      mdt_obd_name(mdt), hash->uc_upcall);
@@ -317,7 +316,7 @@ static ssize_t identity_upcall_store(struct kobject *kobj,
 
        CDEBUG(D_CONFIG, "%s: identity upcall set to %s\n", mdt_obd_name(mdt),
               hash->uc_upcall);
-       RETURN(count);
+       return count;
 }
 LUSTRE_RW_ATTR(identity_upcall);
 

diff --git a/lustre/obdclass/upcall_cache.c b/lustre/obdclass/upcall_cache.c
index 05be32d..4bc61b7 100644 (file)
--- a/lustre/obdclass/upcall_cache.c
+++ b/lustre/obdclass/upcall_cache.c
@@ -151,6 +151,47 @@ static int check_unlink_entry(struct upcall_cache *cache,
        return 1;
 }
 
+int upcall_cache_set_upcall(struct upcall_cache *cache, const char *buffer,
+                           size_t count, bool path_only)
+{
+       char *upcall;
+
+       if (count >= UC_CACHE_UPCALL_MAXPATH)
+               return -E2BIG;
+
+       OBD_ALLOC(upcall, count + 1);
+       if (upcall == NULL)
+               return -ENOMEM;
+
+       /* Remove any extraneous bits from the upcall (e.g. linefeeds) */
+       if (sscanf(buffer, "%s", upcall) != 1)
+               goto invalid;
+
+       if (upcall[0] == '/')
+               goto valid;
+
+       if (path_only)
+               goto invalid;
+
+       if (strcasecmp(upcall, "NONE") == 0) {
+               snprintf(upcall, count + 1, "NONE");
+               goto valid;
+       }
+
+invalid:
+       OBD_FREE(upcall, count + 1);
+       return -EINVAL;
+
+valid:
+       down_write(&cache->uc_upcall_rwsem);
+       strcpy(cache->uc_upcall, upcall);
+       up_write(&cache->uc_upcall_rwsem);
+
+       OBD_FREE(upcall, count + 1);
+       return 0;
+}
+EXPORT_SYMBOL(upcall_cache_set_upcall);
+
 static inline int refresh_entry(struct upcall_cache *cache,
                         struct upcall_cache_entry *entry)
 {

diff --git a/lustre/ptlrpc/gss/lproc_gss.c b/lustre/ptlrpc/gss/lproc_gss.c
index 593b5d1..4e6b9a1 100644 (file)
--- a/lustre/ptlrpc/gss/lproc_gss.c
+++ b/lustre/ptlrpc/gss/lproc_gss.c
@@ -198,11 +198,6 @@ static ssize_t rsi_upcall_seq_write(struct file *file,
        char *kbuf = NULL;
        int rc;
 
-       if (count >= UC_CACHE_UPCALL_MAXPATH) {
-               CERROR("%s: rsi upcall too long\n", rsicache->uc_name);
-               return -EINVAL;
-       }
-
        OBD_ALLOC(kbuf, count + 1);
        if (kbuf == NULL)
                return -ENOMEM;
@@ -212,22 +207,20 @@ static ssize_t rsi_upcall_seq_write(struct file *file,
 
        kbuf[count] = '\0';
 
-       /* Remove any extraneous bits from the upcall (e.g. linefeeds) */
-       down_write(&rsicache->uc_upcall_rwsem);
-       rc = sscanf(kbuf, "%s", rsicache->uc_upcall);
-       up_write(&rsicache->uc_upcall_rwsem);
-
-       if (rc != 1) {
-               CERROR("%s: invalid rsi upcall provided\n", rsicache->uc_name);
-               GOTO(out, rc = -EINVAL);
+       rc = upcall_cache_set_upcall(rsicache, kbuf, count, true);
+       if (rc) {
+               CERROR("%s: incorrect rsi upcall %.*s. Valid value for sptlrpc.gss.rsi_upcall is an executable pathname: rc = %d\n",
+                      rsicache->uc_name, (int)count, buffer, rc);
+               GOTO(out, rc);
        }
 
        CDEBUG(D_CONFIG, "%s: rsi upcall set to %s\n", rsicache->uc_name,
               rsicache->uc_upcall);
+       rc = count;
 
 out:
        OBD_FREE(kbuf, count + 1);
-       return count;
+       return rc;
 }
 LPROC_SEQ_FOPS(rsi_upcall);
 

diff --git a/lustre/tests/sanity-sec.sh b/lustre/tests/sanity-sec.sh
index 789c73e..652e669 100755 (executable)
--- a/lustre/tests/sanity-sec.sh
+++ b/lustre/tests/sanity-sec.sh
@@ -6229,6 +6229,42 @@ test_68() {
 }
 run_test 68 "all config logs are processed"
 
+test_69() {
+       local mdt="$(mdtname_from_index 0 $MOUNT)"
+       local param
+       local orig
+
+       param="mdt.$mdt.identity_upcall"
+       orig="$(do_facet mds1 "$LCTL get_param -n $param")"
+       stack_trap "do_facet mds1 $LCTL set_param $param=$orig" EXIT
+
+       # identity_upcall accepts a path to an executable,
+       # or NONE (case insensitive)
+       do_facet mds1 $LCTL set_param $param=/path/to/prog ||
+               error "set_param $param=/path/to/prog failed"
+       do_facet mds1 $LCTL set_param $param=prog &&
+               error "set_param $param=prog should failed"
+       do_facet mds1 $LCTL set_param $param=NONE ||
+               error "set_param $param=NONE failed"
+       do_facet mds1 $LCTL set_param $param=none ||
+               error "set_param $param=none failed"
+
+       if $GSS; then
+               param="sptlrpc.gss.rsi_upcall"
+               orig="$(do_facet mds1 "$LCTL get_param -n $param")"
+               stack_trap "do_facet mds1 $LCTL set_param $param=$orig" EXIT
+
+               # rsi_upcall only accepts a path to an executable
+               do_facet mds1 $LCTL set_param $param=prog &&
+                       error "set_param $param=prog should failed"
+               do_facet mds1 $LCTL set_param $param=NONE &&
+                       error "set_param $param=NONE should fail"
+               do_facet mds1 $LCTL set_param $param=/path/to/prog ||
+                       error "set_param $param=/path/to/prog failed"
+       fi
+}
+run_test 69 "check upcall incorrect values"
+
 log "cleanup: ======================================================"
 
 sec_unsetup() {