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@xxxxxxxxxx>
>>>
>>> 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@xxxxxxxxxx>
>>> ---
>>> 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@xxxxxxxxxx>
> 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@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
