[PATCH] restore: don't trash file capabilities

[PATCH] restore: don't trash file capabilities

[Dave Chinner]

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
@@ -362,6 +362,15 @@ struct stream_context {
 	char       sc_path[2 * MAXPATHLEN];
 	intgen_t   sc_fd;
 	intgen_t   sc_hsmflags;
+
+	/*
+	 * we have to set the owner before we set extended attributes otherwise
+	 * capabilities will not be restored correctly as setting the owner with
+	 * fchmod will strip the capability attribute from the file. Hence we
+	 * need to do this before restoring xattrs and record it so we don't do
+	 * it again on completion of file restoration.
+	 */
+	bool_t	   sc_ownerset;
 };
 
 typedef struct stream_context stream_context_t;
@@ -3429,6 +3438,8 @@ applynondirdump( drive_t *drivep,
 	memset(&strctxp->sc_bstat, 0, sizeof(bstat_t));
 	strctxp->sc_path[0] = '\0';
 	strctxp->sc_fd = -1;
+	strctxp->sc_ownerset = BOOL_FALSE;
+
 
 	for ( ; ; ) {
 		drive_ops_t *dop = drivep->d_opsp;
@@ -3455,6 +3466,7 @@ applynondirdump( drive_t *drivep,
 			memcpy(&strctxp->sc_bstat, bstatp, sizeof(bstat_t));
 			strctxp->sc_path[0] = '\0';
 			strctxp->sc_fd = -1;
+			strctxp->sc_ownerset = BOOL_FALSE;
 
 			rv = restore_file( drivep, fhdrp, ehcs, ahcs, path1, path2 );
 
@@ -7351,6 +7363,61 @@ restore_file_cb( void *cp, bool_t linkpr, char *path1, char *path2 )
 	}
 }
 
+/*
+ * Set the file owner and strip suid/sgid if necessary. On failure, it will
+ * close the file descriptor, unlink the file and return -1. On success,
+ * it will mark the stream contexts as having set the owner and return 0.
+ */
+static int
+set_file_owner(
+	char		 *path,
+	intgen_t	 *fdp,
+	stream_context_t *strcxtp)
+{
+	bstat_t		*bstatp = &strcxtp->sc_bstat;
+	mode_t		mode = (mode_t)bstatp->bs_mode;
+	int		rval;
+
+	rval = fchown(*fdp, (uid_t)bstatp->bs_uid, (gid_t)bstatp->bs_gid );
+	if (!rval)
+		goto done;
+
+	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)
+		goto done;
+
+	rval = fchmod(*fdp, mode);
+	if (rval) {
+		mlog(MLOG_VERBOSE | MLOG_ERROR,
+		     _("unable to strip setuid/setgid on %s, unlinking file.\n"),
+		     path);
+		unlink(path);
+		close(*fdp);
+		*fdp = -1;
+		return -1;
+	}
+done:
+	strcxtp->sc_ownerset = BOOL_TRUE;
+	return 0;
+}
+
 /* called to begin a regular file. if no path given, or if just toc,
  * don't actually write, just read. also get into that situation if
  * cannot prepare destination. fd == -1 signifies no write. *statp
@@ -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 ( 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;
 	}
 
 	/* set the permissions/mode
-- 
1.8.4.rc3

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

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

[Brian Foster]

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

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

[Dave Chinner]

On Wed, Feb 05, 2014 at 09:12:15AM -0500, Brian Foster wrote:
> 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 any of the rest of the operations in this function fail, then it
keeps trying to do more operations and then finally it returns
BOOL_TRUE even after errors have occurred. Because the file has been
unlinked and the fd has been closed, we know all further operations
are going to fail, so we may as well just return immediately.

And if we follow the caller path back, we get back to


applynondirdump
  for (;;) {
    restore_file
      treecb_link
        restore_file_cb
          restore_reg
   if (not ok)
     break;
  }

So, returning BOOL_FALSE in the context above results in
tree_cb_link() returning RV_NOT_OK, which results in the stream
processing loop in applynondirdump() being aborted. i.e. returning
an error restoring a file causes the entire dump stream to be
aborted at that point.

IOWs, the dump image cannot be restored past that point, hence
returning an error of any kind is a bad idea.  Mostly complete
restores missing just a file (that were reported on during the
restore) is much, much better than having restore stop half way
through and refusing to go any further.

The code is convoluted, obtuse and difficult to read. But we're not
going to rewrite it from scratch any time soon so we just have to
live with it's craziness.

> > -			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.

set_file_owner() closes the fd on error, immediately after unlinking
the file. Hence there is no fd leak in the error path.

And, again, we return BOOL_TRUE because that's what all the other
paths return even on error. And, in fact, none of the callers even
look at the error returned, so it's pretty much irrelevant in this
function.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

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

[Brian Foster]

On 02/05/2014 04:54 PM, Dave Chinner wrote:
> On Wed, Feb 05, 2014 at 09:12:15AM -0500, Brian Foster wrote:
>> 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 any of the rest of the operations in this function fail, then it
> keeps trying to do more operations and then finally it returns
> BOOL_TRUE even after errors have occurred. Because the file has been
> unlinked and the fd has been closed, we know all further operations
> are going to fail, so we may as well just return immediately.
> 
> And if we follow the caller path back, we get back to
> 
> 
> applynondirdump
>   for (;;) {
>     restore_file
>       treecb_link
>         restore_file_cb
>           restore_reg
>    if (not ok)
>      break;
>   }
> 
> So, returning BOOL_FALSE in the context above results in
> tree_cb_link() returning RV_NOT_OK, which results in the stream
> processing loop in applynondirdump() being aborted. i.e. returning
> an error restoring a file causes the entire dump stream to be
> aborted at that point.
> 
> IOWs, the dump image cannot be restored past that point, hence
> returning an error of any kind is a bad idea.  Mostly complete
> restores missing just a file (that were reported on during the
> restore) is much, much better than having restore stop half way
> through and refusing to go any further.
> 

Ok.. and we nuke the output file anyways if we can't set the owner, so
that makes sense. It kind of stinks we continue with the whole dance for
this file after this. On the other hand, perhaps there is value in still
reading through the source content. Anyways, it looks like it should
handle this scenario (aside from probably spewing the additional error
messages) but...

> The code is convoluted, obtuse and difficult to read. But we're not
> going to rewrite it from scratch any time soon so we just have to
> live with it's craziness.
> 

... that about sums it up. ;)

>>> -			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.
> 
> set_file_owner() closes the fd on error, immediately after unlinking
> the file. Hence there is no fd leak in the error path.
> 

Oops, right.

Reviewed-by: Brian Foster <bfoster@redhat.com>

> And, again, we return BOOL_TRUE because that's what all the other
> paths return even on error. And, in fact, none of the callers even
> look at the error returned, so it's pretty much irrelevant in this
> function.
> 
> Cheers,
> 
> Dave.
> 

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