Re: [PATCH] restore: don't trash file capabilities

[Brian Foster <bfoster@redhat.com>]

On 02/04/2014 08:48 PM, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> xfsrestore fails to restore file capabilities correctly because it
> sets the owner on the file after it has restored the capability
> attributes. This results in the kernel stripping the capabilities
> when changing the owner of the file and hence the restored file is
> not complete.
> 
> Fix this by changing the owner of the file when it is created rather
> than after it has been fully restored. This ensures we don't kill
> the caps as they are restored after the owner it appropriately set.
> This fixes the xfs/296 failure.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  restore/content.c | 116 ++++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 77 insertions(+), 39 deletions(-)
> 
> diff --git a/restore/content.c b/restore/content.c
> index 54d933c..b655ed1 100644
> --- a/restore/content.c
> +++ b/restore/content.c
...
> @@ -7442,6 +7509,12 @@ restore_reg( drive_t *drivep,
>  		}
>  	}
>  
> +	if (strctxp->sc_ownerset == BOOL_FALSE && persp->a.ownerpr) {
> +		rval = set_file_owner(path, fdp, strctxp);
> +		if (rval)
> +			return BOOL_TRUE;
> +	}
> +

If this fails (rval == -1), we have unlinked the file and closed the fd
in set_file_owner(). Why return BOOL_TRUE? I've not gone through and
learned what dump/restore is doing, but this path looks like it's going
to continue on with the restore in that scenario. Even if that is what
we want to happen, it seems inconsistent with failure handling in
set_file_owner() and the fact that we skip the rest of this function.

>  	if ( persp->a.dstdirisxfspr ) {
>  
>  		/* set the extended inode flags, except those which must
> @@ -7623,45 +7696,10 @@ restore_complete_reg(stream_context_t *strcxtp)
>  
>  	/* set the owner and group (if enabled)
>  	 */
> -	if ( persp->a.ownerpr ) {
> -		rval = fchown( fd,
> -			       ( uid_t )bstatp->bs_uid,
> -			       ( gid_t )bstatp->bs_gid );
> -		if ( rval ) {
> -			mode_t mode = (mode_t)bstatp->bs_mode;
> -
> -			mlog( MLOG_VERBOSE | MLOG_WARNING,
> -			      _("chown (uid=%u, gid=%u) %s failed: %s\n"),
> -			      bstatp->bs_uid,
> -			      bstatp->bs_gid,
> -			      path,
> -			      strerror( errno ));
> -
> -			if ( mode & S_ISUID ) {
> -				mlog( MLOG_VERBOSE | MLOG_WARNING,
> -				      _("stripping setuid bit on %s "
> -				      "since chown failed\n"),
> -				      path );
> -				mode &= ~S_ISUID;
> -			}
> -			if ( (mode & (S_ISGID|S_IXGRP)) == (S_ISGID|S_IXGRP) ) {
> -				mlog( MLOG_VERBOSE | MLOG_WARNING,
> -				      _("stripping setgid bit on %s "
> -				      "since chown failed\n"),
> -				      path );
> -				mode &= ~S_ISGID;
> -			}
> -			if ( mode != (mode_t)bstatp->bs_mode ) {
> -				rval = fchmod( fd, mode );
> -				if ( rval ) {
> -					mlog( MLOG_VERBOSE | MLOG_ERROR,
> -					      _("unable to strip setuid/setgid "
> -					      "on %s, unlinking file.\n"),
> -					      path );
> -					unlink( path );
> -				}
> -			}
> -		}
> +	if (strcxtp->sc_ownerset == BOOL_FALSE && persp->a.ownerpr) {
> +		rval = set_file_owner(path, &fd, strcxtp);
> +		if (rval)
> +			return BOOL_TRUE;
>  	}

Ok, so we handle both set_file_owner() situations because the ordering
here is really only important for regular files. IIUC, this particular
block is still required for that non-ifreg case. Though, in the failure
case, it looks like we're leaking the fd by returning now.

Brian

>  
>  	/* set the permissions/mode
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs