LU-14989 sec: access to enc file's xattrs

Encryption context is stored in 'security.c' xattr. This is put in the
xattr cache via ll_xattr_cache_insert() to avoid sending a getxattr
request to the server. But this operation declares the xattr cache for
the inode as 'valid', with two consequences. It prevents any further
filling with other xattrs, and trying to read an xattr value will
directly return -ENODATA, without any attempt to fetch the xattr from
the server.
This is solved by adding a new ll_file_flags 'LLIF_XATTR_CACHE_FILLED'
that tells if the xattr cache for the inode has been filled. This bit
is set only by ll_xattr_cache_refill(), and 'valid' now just means the
xattr cache for the inode has been initialized.

Lustre-change: https://review.whamcloud.com/44855
Lustre-commit: 1faf54e8bf19c28a4de7b069309d607cb3f10f91

Fixes: 40d91eafe2 ("LU-12275 sec: atomicity of encryption context getting/setting")
Signed-off-by: Sebastien Buisson <sbuisson@ddn.com>
Change-Id: I6c2b6870df29f26f048dedeb7212d1c801ca69e1
Reviewed-on: https://review.whamcloud.com/45084
Tested-by: jenkins <devops@whamcloud.com>
Tested-by: Maloo <maloo@whamcloud.com>
Reviewed-by: Andreas Dilger <adilger@whamcloud.com>

diff --git a/lustre/llite/llite_internal.h b/lustre/llite/llite_internal.h
index 25aa200..bb1dc03 100644 (file)
--- a/lustre/llite/llite_internal.h
+++ b/lustre/llite/llite_internal.h
@@ -426,6 +426,8 @@ enum ll_file_flags {
        LLIF_UPDATE_ATIME       = 4,
        /* setting encryption context in progress */
        LLIF_SET_ENC_CTX        = 6,
+       /* Xattr cache is filled */
+       LLIF_XATTR_CACHE_FILLED = 7,
 
 };
 

diff --git a/lustre/llite/statahead.c b/lustre/llite/statahead.c
index 5bfa4f1..c8f4eb3 100644 (file)
--- a/lustre/llite/statahead.c
+++ b/lustre/llite/statahead.c
@@ -678,6 +678,30 @@ static void sa_instantiate(struct ll_statahead_info *sai,
        if (rc)
                GOTO(out, rc);
 
+       /* If encryption context was returned by MDT, put it in
+        * inode now to save an extra getxattr.
+        */
+       if (body->mbo_valid & OBD_MD_ENCCTX) {
+               void *encctx = req_capsule_server_get(&req->rq_pill,
+                                                     &RMF_FILE_ENCCTX);
+               __u32 encctxlen = req_capsule_get_size(&req->rq_pill,
+                                                      &RMF_FILE_ENCCTX,
+                                                      RCL_SERVER);
+
+               if (encctxlen) {
+                       CDEBUG(D_SEC,
+                              "server returned encryption ctx for "DFID"\n",
+                              PFID(ll_inode2fid(child)));
+                       rc = ll_xattr_cache_insert(child,
+                                              LL_XATTR_NAME_ENCRYPTION_CONTEXT,
+                                              encctx, encctxlen);
+                       if (rc)
+                               CWARN("%s: cannot set enc ctx for "DFID": rc = %d\n",
+                                     ll_i2sbi(child)->ll_fsname,
+                                     PFID(ll_inode2fid(child)), rc);
+               }
+       }
+
        CDEBUG(D_READA, "%s: setting %.*s"DFID" l_data to inode %p\n",
               ll_i2sbi(dir)->ll_fsname, entry->se_qstr.len,
               entry->se_qstr.name, PFID(ll_inode2fid(child)), child);

diff --git a/lustre/llite/xattr_cache.c b/lustre/llite/xattr_cache.c
index 3489f0b..fe95002 100644 (file)
--- a/lustre/llite/xattr_cache.c
+++ b/lustre/llite/xattr_cache.c
@@ -139,6 +139,12 @@ static int ll_xattr_cache_add(struct list_head *cache,
        ENTRY;
 
        if (ll_xattr_cache_find(cache, xattr_name, &xattr) == 0) {
+               if (!strcmp(xattr_name, LL_XATTR_NAME_ENCRYPTION_CONTEXT))
+                       /* it means enc ctx was already in cache,
+                        * ignore error as it cannot be modified
+                        */
+                       RETURN(0);
+
                CDEBUG(D_CACHE, "duplicate xattr: [%s]\n", xattr_name);
                RETURN(-EPROTO);
        }
@@ -250,7 +256,7 @@ static int ll_xattr_cache_list(struct list_head *cache,
 }
 
 /**
- * Check if the xattr cache is initialized (filled).
+ * Check if the xattr cache is initialized.
  *
  * \retval 0 @cache is not initialized
  * \retval 1 @cache is initialized
@@ -261,6 +267,17 @@ static int ll_xattr_cache_valid(struct ll_inode_info *lli)
 }
 
 /**
+ * Check if the xattr cache is filled.
+ *
+ * \retval 0 @cache is not filled
+ * \retval 1 @cache is filled
+ */
+static int ll_xattr_cache_filled(struct ll_inode_info *lli)
+{
+       return ll_file_test_flag(lli, LLIF_XATTR_CACHE_FILLED);
+}
+
+/**
  * This finalizes the xattr cache.
  *
  * Free all xattr memory. @lli is the inode info pointer.
@@ -277,6 +294,7 @@ static int ll_xattr_cache_destroy_locked(struct ll_inode_info *lli)
        while (ll_xattr_cache_del(&lli->lli_xattrs, NULL) == 0)
                /* empty loop */ ;
 
+       ll_file_clear_flag(lli, LLIF_XATTR_CACHE_FILLED);
        ll_file_clear_flag(lli, LLIF_XATTR_CACHE);
 
        RETURN(0);
@@ -302,7 +320,8 @@ int ll_xattr_cache_destroy(struct inode *inode)
  * Find or request an LDLM lock with xattr data.
  * Since LDLM does not provide API for atomic match_or_enqueue,
  * the function handles it with a separate enq lock.
- * If successful, the function exits with the list lock held.
+ * If successful, the function exits with a write lock held
+ * on lli_xattrs_list_rwsem.
  *
  * \retval 0       no error occured
  * \retval -ENOMEM not enough memory
@@ -324,7 +343,7 @@ static int ll_xattr_find_get_lock(struct inode *inode,
        mutex_lock(&lli->lli_xattrs_enq_lock);
        /* inode may have been shrunk and recreated, so data is gone, match lock
         * only when data exists. */
-       if (ll_xattr_cache_valid(lli)) {
+       if (ll_xattr_cache_filled(lli)) {
                /* Try matching first. */
                mode = ll_take_md_lock(inode, MDS_INODELOCK_XATTR, &lockh, 0,
                                        LCK_PR);
@@ -367,7 +386,9 @@ out:
 /**
  * Refill the xattr cache.
  *
- * Fetch and cache the whole of xattrs for @inode, acquiring a read lock.
+ * Fetch and cache the whole of xattrs for @inode, thanks to the write lock
+ * on lli_xattrs_list_rwsem obtained from ll_xattr_find_get_lock().
+ * If successful, this write lock is kept.
  *
  * \retval 0       no error occured
  * \retval -EPROTO network protocol error
@@ -391,7 +412,7 @@ static int ll_xattr_cache_refill(struct inode *inode)
                GOTO(err_req, rc);
 
        /* Do we have the data at this point? */
-       if (ll_xattr_cache_valid(lli)) {
+       if (ll_xattr_cache_filled(lli)) {
                ll_stats_ops_tally(sbi, LPROC_LL_GETXATTR_HITS, 1);
                ll_intent_drop_lock(&oit);
                GOTO(err_req, rc = 0);
@@ -427,7 +448,8 @@ static int ll_xattr_cache_refill(struct inode *inode)
 
        CDEBUG(D_CACHE, "caching: xdata=%p xtail=%p\n", xdata, xtail);
 
-       ll_xattr_cache_init(lli);
+       if (!ll_xattr_cache_valid(lli))
+               ll_xattr_cache_init(lli);
 
        for (i = 0; i < body->mbo_max_mdsize; i++) {
                CDEBUG(D_CACHE, "caching [%s]=%.*s\n", xdata, *xsizes, xval);
@@ -464,6 +486,8 @@ static int ll_xattr_cache_refill(struct inode *inode)
 
        if (xdata != xtail || xval != xvtail)
                CERROR("a hole in xattr data\n");
+       else
+               ll_file_set_flag(lli, LLIF_XATTR_CACHE_FILLED);
 
        ll_set_lock_data(sbi->ll_md_exp, inode, &oit, NULL);
        ll_intent_drop_lock(&oit);
@@ -513,16 +537,27 @@ int ll_xattr_cache_get(struct inode *inode,
        LASSERT(!!(valid & OBD_MD_FLXATTR) ^ !!(valid & OBD_MD_FLXATTRLS));
 
        down_read(&lli->lli_xattrs_list_rwsem);
-       if (!ll_xattr_cache_valid(lli)) {
+       /* For performance reasons, we do not want to refill complete xattr
+        * cache if we are just interested in encryption context.
+        */
+       if ((valid & OBD_MD_FLXATTRLS ||
+            strcmp(name, LL_XATTR_NAME_ENCRYPTION_CONTEXT) != 0) &&
+           !ll_xattr_cache_filled(lli)) {
                up_read(&lli->lli_xattrs_list_rwsem);
                rc = ll_xattr_cache_refill(inode);
                if (rc)
                        RETURN(rc);
+               /* Turn the write lock obtained in ll_xattr_cache_refill()
+                * into a read lock.
+                */
                downgrade_write(&lli->lli_xattrs_list_rwsem);
        } else {
                ll_stats_ops_tally(ll_i2sbi(inode), LPROC_LL_GETXATTR_HITS, 1);
        }
 
+       if (!ll_xattr_cache_valid(lli))
+               GOTO(out, rc = -ENODATA);
+
        if (valid & OBD_MD_FLXATTR) {
                struct ll_xattr_entry *xattr;
 
@@ -570,18 +605,10 @@ int ll_xattr_cache_insert(struct inode *inode,
 
        ENTRY;
 
-       down_read(&lli->lli_xattrs_list_rwsem);
+       down_write(&lli->lli_xattrs_list_rwsem);
        if (!ll_xattr_cache_valid(lli))
                ll_xattr_cache_init(lli);
-       rc = ll_xattr_cache_add(&lli->lli_xattrs, name, buffer,
-                               size);
-       up_read(&lli->lli_xattrs_list_rwsem);
-
-       if (rc == -EPROTO &&
-           strcmp(name, LL_XATTR_NAME_ENCRYPTION_CONTEXT) == 0)
-               /* it means enc ctx was already in cache,
-                * ignore error as it cannot be modified
-                */
-               rc = 0;
+       rc = ll_xattr_cache_add(&lli->lli_xattrs, name, buffer, size);
+       up_write(&lli->lli_xattrs_list_rwsem);
        RETURN(rc);
 }

diff --git a/lustre/tests/sanity-sec.sh b/lustre/tests/sanity-sec.sh
index 6f3c54d..85592cc 100755 (executable)
--- a/lustre/tests/sanity-sec.sh
+++ b/lustre/tests/sanity-sec.sh
@@ -2623,6 +2623,8 @@ setup_for_enc_tests() {
 }
 
 cleanup_for_enc_tests() {
+       rm -rf $DIR/$tdir
+
        # remount client normally
        if is_mounted $MOUNT; then
                umount_client $MOUNT || error "umount $MOUNT failed"
@@ -3510,7 +3512,6 @@ run_test 48b "encrypted file: concurrent truncate"
 
 trace_cmd() {
        local cmd="$@"
-       local xattr_name="security.c"
 
        cancel_lru_locks
        $LCTL set_param debug=+info
@@ -3520,7 +3521,11 @@ trace_cmd() {
        eval $cmd
        [ $? -eq 0 ] || error "$cmd failed"
 
-       $LCTL dk | grep -E "get xattr '${xattr_name}'|get xattrs"
+       if [ -z "$MATCHING_STRING" ]; then
+               $LCTL dk | grep -E "get xattr 'security.c'|get xattrs"
+       else
+               $LCTL dk | grep -E "$MATCHING_STRING"
+       fi
        [ $? -ne 0 ] || error "get xattr event was triggered"
 }
 
@@ -3543,7 +3548,8 @@ test_49() {
        trace_cmd stat $dirname/f1
        trace_cmd cat $dirname/f1
        dd if=/dev/zero of=$dirname/f1 bs=1M count=10 conv=fsync
-       trace_cmd $TRUNCATE $dirname/f1 10240
+       MATCHING_STRING="get xattr 'security.c'" \
+               trace_cmd $TRUNCATE $dirname/f1 10240
        trace_cmd $LFS setstripe -E -1 -S 4M $dirname/f2
        trace_cmd $LFS migrate -E -1 -S 256K $dirname/f2
 
@@ -3563,7 +3569,8 @@ test_49() {
                trace_cmd stat $dirname/f1
                trace_cmd cat $dirname/f1
                dd if=/dev/zero of=$dirname/f1 bs=1M count=10 conv=fsync
-               trace_cmd $TRUNCATE $dirname/f1 10240
+               MATCHING_STRING="get xattr 'security.c'" \
+                       trace_cmd $TRUNCATE $dirname/f1 10240
                trace_cmd $LFS setstripe -E -1 -S 4M $dirname/f2
                trace_cmd $LFS migrate -E -1 -S 256K $dirname/f2
        else
@@ -4232,6 +4239,46 @@ test_57() {
 }
 run_test 57 "security.c xattr protection"
 
+test_58() {
+       local testdir=$DIR/$tdir/mytestdir
+       local testfile=$DIR/$tdir/$tfile
+
+       [[ $(facet_fstype ost1) == zfs ]] && skip "skip ZFS backend"
+
+       $LCTL get_param mdc.*.import | grep -q client_encryption ||
+               skip "client encryption not supported"
+
+       mount.lustre --help |& grep -q "test_dummy_encryption:" ||
+               skip "need dummy encryption support"
+
+       stack_trap cleanup_for_enc_tests EXIT
+       setup_for_enc_tests
+
+       touch $DIR/$tdir/$tfile
+       mkdir $DIR/$tdir/subdir
+
+       cancel_lru_locks
+       sync ; sync
+       echo 3 > /proc/sys/vm/drop_caches
+
+       ll_decode_linkea $DIR/$tdir/$tfile || error "cannot read $tfile linkea"
+       ll_decode_linkea $DIR/$tdir/subdir || error "cannot read subdir linkea"
+
+       for ((i = 0; i < 1000; i = $((i+1)))); do
+               mkdir -p $DIR/$tdir/d${i}
+               touch $DIR/$tdir/f${i}
+               createmany -m $DIR/$tdir/d${i}/f 5 > /dev/null
+       done
+
+       cancel_lru_locks
+       sync ; sync
+       echo 3 > /proc/sys/vm/drop_caches
+
+       sleep 10
+       ls -ailR $MOUNT > /dev/null || error "fail to ls"
+}
+run_test 58 "access to enc file's xattrs"
+
 log "cleanup: ======================================================"
 
 sec_unsetup() {

diff --git a/lustre/utils/ll_decode_linkea.c b/lustre/utils/ll_decode_linkea.c
index c1bc6e8..9327428 100644 (file)
--- a/lustre/utils/ll_decode_linkea.c
+++ b/lustre/utils/ll_decode_linkea.c
@@ -36,12 +36,26 @@
 #include <errno.h>
 #include <string.h>
 #include <limits.h>
+#include <ctype.h>
 #include <sys/types.h>
 #include <sys/xattr.h>
 #include <linux/lustre/lustre_fid.h>
 
 #define BUFFER_SIZE 65536
 
+static void print_name(const char *cp, int len)
+{
+       unsigned char ch;
+
+       while (len--) {
+               ch = *cp++;
+               if (!isprint(ch) || ch == '\\')
+                       printf("\\x%02x", ch);
+               else
+                       putchar(ch);
+       }
+}
+
 int decode_linkea(const char *fname)
 {
        char buf[BUFFER_SIZE];
@@ -106,8 +120,9 @@ int decode_linkea(const char *fname)
                memcpy(&pfid, &lee->lee_parent_fid, sizeof(pfid));
                fid_be_to_cpu(&pfid, &pfid);
 
-               printf("    %d: pfid "DFID", name '%s'\n", i, PFID(&pfid),
-                      lee->lee_name);
+               printf("    %d: pfid "DFID", name '", i, PFID(&pfid));
+               print_name(lee->lee_name, reclen - (int)sizeof(*lee));
+               printf("'\n");
                lee = (struct link_ea_entry *)((char *)lee + reclen);
        }