On 2020/3/25 上午9:52, Marcos Paulo de Souza wrote:
> From: Marcos Paulo de Souza <mpdesouza@suse.com>
> 
> [PROBLEM]
> When doing incremental send with a file with capabilities, there is a
> situation where the capability can be lost in the receiving side. The
> sequence of actions bellow show the problem:
> 
> $ mount /dev/sda fs1
> $ mount /dev/sdb fs2
> 
> $ touch fs1/foo.bar
> $ setcap cap_sys_nice+ep fs1/foo.bar
> $ btrfs subvol snap -r fs1 fs1/snap_init
> $ btrfs send fs1/snap_init | btrfs receive fs2
> 
> $ chgrp adm fs1/foo.bar
> $ setcap cap_sys_nice+ep fs1/foo.bar
> 
> $ btrfs subvol snap -r fs1 fs1/snap_complete
> $ btrfs subvol snap -r fs1 fs1/snap_incremental
> 
> $ btrfs send fs1/snap_complete | btrfs receive fs2
> $ btrfs send -p fs1/snap_init fs1/snap_incremental | btrfs receive fs2
> 
> At this point fs/snap_increment/foo.bar lost the capability, since a
> chgrp was emitted by "btrfs send". The current code only checks for the
> items that changed, and as the XATTR kept the value only the chgrp change
> is emitted.
> 
> [FIX]
> In order to fix this issue, check if the uid/gid of the inode change,
> and if yes, emit all XATTR again, including the capability.
> 
> Fixes: https://github.com/kdave/btrfs-progs/issues/202
> 
> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> ---
>  I'm posting this patch as a RFC because I had some questions
>  * Is this the correct place to fix?
>  * Also, emitting all XATTR of the inode seems overkill...
>  * Should it be fixed in userspace?

+1 for fixing it in userspace.

To me, although not an expert in send, the send stream looks more like a
diff between two subvolumes, other than instructions to rebuild a subvolume.

So in above case, since the XATTR is not changed, not sending the XATTR
is OK.
Thus it's the receiver side to workaround the missing XATTR.

And it shouldn't be that complex for user space to backup the XATTR, and
restore it after gid/uid change.

Although the main chanllege is how to distinguish plain uid/gid change
(with XATTR dropped) and uid/gid change with XATTR restored.

Thanks,
Qu

> 
>  fs/btrfs/send.c | 29 +++++++++++++++++++++++++----
>  1 file changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
> index c5f41bd86765..5cffe5da91cf 100644
> --- a/fs/btrfs/send.c
> +++ b/fs/btrfs/send.c
> @@ -6187,6 +6187,14 @@ static int changed_inode(struct send_ctx *sctx,
>  		sctx->cur_inode_mode = btrfs_inode_mode(
>  				sctx->right_path->nodes[0], right_ii);
>  	} else if (result == BTRFS_COMPARE_TREE_CHANGED) {
> +		u64 left_uid = btrfs_inode_uid(sctx->left_path->nodes[0],
> +					left_ii);
> +		u64 left_gid = btrfs_inode_gid(sctx->left_path->nodes[0],
> +					left_ii);
> +		u64 right_uid = btrfs_inode_uid(sctx->right_path->nodes[0],
> +					right_ii);
> +		u64 right_gid = btrfs_inode_gid(sctx->right_path->nodes[0],
> +					right_ii);
>  		/*
>  		 * We need to do some special handling in case the inode was
>  		 * reported as changed with a changed generation number. This
> @@ -6236,15 +6244,12 @@ static int changed_inode(struct send_ctx *sctx,
>  			sctx->send_progress = sctx->cur_ino + 1;
>  
>  			/*
> -			 * Now process all extents and xattrs of the inode as if
> +			 * Now process all extents of the inode as if
>  			 * they were all new.
>  			 */
>  			ret = process_all_extents(sctx);
>  			if (ret < 0)
>  				goto out;
> -			ret = process_all_new_xattrs(sctx);
> -			if (ret < 0)
> -				goto out;
>  		} else {
>  			sctx->cur_inode_gen = left_gen;
>  			sctx->cur_inode_new = 0;
> @@ -6255,6 +6260,22 @@ static int changed_inode(struct send_ctx *sctx,
>  			sctx->cur_inode_mode = btrfs_inode_mode(
>  					sctx->left_path->nodes[0], left_ii);
>  		}
> +
> +		/*
> +		 * Process all XATTR of the inode if the generation or owner
> +		 * changed.
> +		 *
> +		 * If the inode changed it's uid/gid, but kept a
> +		 * security.capability xattr, only the uid/gid will be emitted,
> +		 * causing the related xattr to deleted. For this reason always
> +		 * emit the XATTR when an inode has changed.
> +		 */
> +		if (sctx->cur_inode_new_gen || left_uid != right_uid ||
> +		    left_gid != right_gid) {
> +			ret = process_all_new_xattrs(sctx);
> +			if (ret < 0)
> +				goto out;
> +		}
>  	}
>  
>  out:
>