cluster/dht: Linkfiles creation with correct uid/gid

If renames are done with different uid/gid (non-owners), then we would
end up with incorrect uid/gid.

The fix is to create linkfiles, and heal the uid/gid as root:root. This
preserves our notion of creation as root:root and heal the uid/gid as
root:root in all paths. Additionally, we need to consider uid/gid from
only src_cached subvol, and not from linkfiles.

rename is also done as root:root if done on linkfile, as setattr of ownership
on linkfile is done after the rename

Change-Id: Icb5d431dc42da9c02dfae81980e3fe769a47a274
BUG: 884597
Signed-off-by: shishir gowda <sgowda@redhat.com>
Reviewed-on: http://review.gluster.org/4682
Tested-by: Gluster Build System <jenkins@build.gluster.com>
Reviewed-by: Jeff Darcy <jdarcy@redhat.com>

3 files changed, 89 insertions, 20 deletions

diff --git a/tests/bugs/bug-884597.t b/tests/bugs/bug-884597.t
index 0d3495a65..db82fbd6d 100755
--- a/tests/bugs/bug-884597.t
+++ b/tests/bugs/bug-884597.t
@@ -107,3 +107,45 @@ BACKEND_UID=`stat --printf=%u $B0/${V0}$cached/link$i`;
 BACKEND_GID=`stat --printf=%g $B0/${V0}$cached/link$i`;
 
 EXPECT "0" uid_gid_compare $NEW_UID $NEW_GID $BACKEND_UID $BACKEND_GID
+
+## UID/GID creation as different user
+i=1
+NEW_UID=36
+NEW_GID=36
+
+TEST touch $M0/user_file1
+TEST chown $NEW_UID:$NEW_GID $M0/user_file1;
+
+## Give permission on volume, so that different users can perform rename
+
+TEST chmod 0777 $M0
+
+## Add a user known as ABC and perform renames
+TEST `useradd -M ABC 2>/dev/null`
+
+TEST cd $M0
+## rename as different user till file gets a linkfile
+
+while [ $i -ne 0 ]
+do
+        su -c "mv $M0/user_file$i $M0/user_file$(( $i+1 ))" ABC
+        let i++
+        file_has_linkfile user_file$i
+        has_link=$?
+        if [ $has_link -eq 2 ]
+        then
+                break;
+        fi
+done
+
+## del user ABC
+TEST userdel ABC
+
+get_hashed_brick user_file$i
+cached=$?
+
+# check if uid/gid on linkfile is created with correct uid/gid
+BACKEND_UID=`stat --printf=%u $B0/${V0}$cached/user_file$i`;
+BACKEND_GID=`stat --printf=%g $B0/${V0}$cached/user_file$i`;
+
+EXPECT "0" uid_gid_compare $NEW_UID $NEW_GID $BACKEND_UID $BACKEND_GID
diff --git a/xlators/cluster/dht/src/dht-linkfile.c b/xlators/cluster/dht/src/dht-linkfile.c
index b4a074b65..6360c73e3 100644
--- a/xlators/cluster/dht/src/dht-linkfile.c
+++ b/xlators/cluster/dht/src/dht-linkfile.c
@@ -34,6 +34,8 @@ dht_linkfile_create_cbk (call_frame_t *frame, void *cookie, xlator_t *this,
         if (!op_ret)
                 local->linked = _gf_true;
 
+        FRAME_SU_UNDO (frame, dht_local_t);
+
         local->linkfile.linkfile_cbk (frame, cookie, this, op_ret, op_errno,
                                       inode, stbuf, preparent, postparent,
                                       xdata);
@@ -88,6 +90,9 @@ dht_linkfile_create (call_frame_t *frame, fop_mknod_cbk_t linkfile_cbk,
         }
 
         local->link_subvol = fromvol;
+        /* Always create as root:root. dht_linkfile_attr_heal fixes the
+         * ownsership */
+        FRAME_SU_DO (frame, dht_local_t);
         STACK_WIND (frame, dht_linkfile_create_cbk,
                     fromvol, fromvol->fops->mknod, loc,
                     S_IFREG | DHT_LINKFILE_MODE, 0, 0, dict);
@@ -253,6 +258,8 @@ dht_linkfile_attr_heal (call_frame_t *frame, xlator_t *this)
 
         copy->local = copy_local;
 
+        FRAME_SU_DO (copy, dht_local_t);
+
         STACK_WIND (copy, dht_linkfile_setattr_cbk, subvol,
                     subvol->fops->setattr, &copy_local->loc,
                     &stbuf, (GF_SET_ATTR_UID | GF_SET_ATTR_GID), NULL);
diff --git a/xlators/cluster/dht/src/dht-rename.c b/xlators/cluster/dht/src/dht-rename.c
index fd0208f71..08965976b 100644
--- a/xlators/cluster/dht/src/dht-rename.c
+++ b/xlators/cluster/dht/src/dht-rename.c
@@ -307,6 +307,25 @@ err:
         return 0;
 }
 
+int
+dht_rename_done (call_frame_t *frame, xlator_t *this)
+{
+        dht_local_t     *local = NULL;
+
+        local = frame->local;
+
+        if (local->linked == _gf_true) {
+                local->linked = _gf_false;
+                dht_linkfile_attr_heal (frame, this);
+        }
+        DHT_STRIP_PHASE1_FLAGS (&local->stbuf);
+        DHT_STACK_UNWIND (rename, frame, local->op_ret, local->op_errno,
+                          &local->stbuf, &local->preoldparent,
+                          &local->postoldparent, &local->preparent,
+                          &local->postparent, NULL);
+
+        return 0;
+}
 
 int
 dht_rename_unlink_cbk (call_frame_t *frame, void *cookie, xlator_t *this,
@@ -340,11 +359,7 @@ dht_rename_unlink_cbk (call_frame_t *frame, void *cookie, xlator_t *this,
         WIPE (&local->postparent);
 
         if (is_last_call (this_call_cnt)) {
-                DHT_STRIP_PHASE1_FLAGS (&local->stbuf);
-                DHT_STACK_UNWIND (rename, frame, local->op_ret, local->op_errno,
-                                  &local->stbuf, &local->preoldparent,
-                                  &local->postoldparent, &local->preparent,
-                                  &local->postparent, NULL);
+                dht_rename_done (frame, this);
         }
 
 out:
@@ -476,6 +491,8 @@ dht_rename_cbk (call_frame_t *frame, void *cookie, xlator_t *this,
         dst_hashed = local->dst_hashed;
         dst_cached = local->dst_cached;
 
+        if (local->linked == _gf_true)
+                FRAME_SU_UNDO (frame, dht_local_t);
         if (op_ret == -1) {
                 gf_log (this->name, GF_LOG_WARNING,
                         "%s: rename on %s failed (%s)", local->loc.path,
@@ -510,17 +527,21 @@ dht_rename_cbk (call_frame_t *frame, void *cookie, xlator_t *this,
         }
 
 err:
-        dht_iatt_merge (this, &local->stbuf, stbuf, prev->this);
-        dht_iatt_merge (this, &local->preoldparent, preoldparent, prev->this);
-        dht_iatt_merge (this, &local->postoldparent, postoldparent, prev->this);
-        dht_iatt_merge (this, &local->preparent, prenewparent, prev->this);
-        dht_iatt_merge (this, &local->postparent, postnewparent, prev->this);
-
-        if (local->linked == _gf_true) {
-                local->linked = _gf_false;
-                dht_linkfile_attr_heal (frame, this);
+        /* Merge attrs only from src_cached. In case there of src_cached !=
+         * dst_hashed, this ignores linkfile attrs. */
+        if (prev->this == src_cached) {
+                dht_iatt_merge (this, &local->stbuf, stbuf, prev->this);
+                dht_iatt_merge (this, &local->preoldparent, preoldparent,
+                                prev->this);
+                dht_iatt_merge (this, &local->postoldparent, postoldparent,
+                                prev->this);
+                dht_iatt_merge (this, &local->preparent, prenewparent,
+                                prev->this);
+                dht_iatt_merge (this, &local->postparent, postnewparent,
+                                prev->this);
         }
 
+
         /* NOTE: rename_subvol is the same subvolume from which dht_rename_cbk
          *       is called. since rename has already happened on rename_subvol,
          *       unlink should not be sent for oldpath (either linkfile or cached-file)
@@ -583,11 +604,7 @@ unwind:
         WIPE (&local->preparent);
         WIPE (&local->postparent);
 
-        DHT_STRIP_PHASE1_FLAGS (&local->stbuf);
-        DHT_STACK_UNWIND (rename, frame, local->op_ret, local->op_errno,
-                          &local->stbuf, &local->preoldparent,
-                          &local->postoldparent, &local->preparent,
-                          &local->postparent, NULL);
+        dht_rename_done (frame, this);
 
         return 0;
 
@@ -625,6 +642,8 @@ dht_do_rename (call_frame_t *frame)
                 "renaming %s => %s (%s)",
                 local->loc.path, local->loc2.path, rename_subvol->name);
 
+        if (local->linked == _gf_true)
+                FRAME_SU_DO (frame, dht_local_t);
         STACK_WIND (frame, dht_rename_cbk,
                     rename_subvol, rename_subvol->fops->rename,
                     &local->loc, &local->loc2, NULL);
@@ -655,7 +674,8 @@ dht_rename_links_cbk (call_frame_t *frame, void *cookie, xlator_t *this,
                 local->op_ret   = -1;
                 if (op_errno != ENOENT)
                         local->op_errno = op_errno;
-        } else {
+        } else if (local->src_cached == prev->this) {
+                /* merge of attr returned only from linkfile creation */
                 dht_iatt_merge (this, &local->stbuf, stbuf, prev->this);
         }