From 003713139c8a7830a90a40dfead72f8299c17e31 Mon Sep 17 00:00:00 2001 From: shishir gowda Date: Mon, 18 Mar 2013 14:44:20 +0530 Subject: 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 Reviewed-on: http://review.gluster.org/4682 Tested-by: Gluster Build System Reviewed-by: Jeff Darcy --- tests/bugs/bug-884597.t | 42 ++++++++++++++++++++++++ xlators/cluster/dht/src/dht-linkfile.c | 7 ++++ xlators/cluster/dht/src/dht-rename.c | 60 ++++++++++++++++++++++------------ 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, ©_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); } -- cgit