vfs: allow FILE_EXTENT_SAME (dedupe_file_range) on a file opened ro
diff mbox

Message ID 1468793618-10496-1-git-send-email-kilobyte@angband.pl
State New
Headers show

Commit Message

Adam Borowski July 17, 2016, 10:13 p.m. UTC
Instead of checking the mode of the file descriptor, let's check whether it
could have been opened rw.  This allows fixing intermittent exec failures
when deduping a live system: anyone trying to exec a file currently being
deduped gets ETXTBSY.

Issuing this ioctl on a ro file was already allowed for root/cap.

Tested on btrfs and not-yet-merged xfs, as only them implement this ioctl.

Signed-off-by: Adam Borowski <kilobyte@angband.pl>
---
 fs/read_write.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Mark Fasheh July 18, 2016, 7:51 p.m. UTC | #1
On Mon, Jul 18, 2016 at 12:13:38AM +0200, Adam Borowski wrote:
> Instead of checking the mode of the file descriptor, let's check whether it
> could have been opened rw.  This allows fixing intermittent exec failures
> when deduping a live system: anyone trying to exec a file currently being
> deduped gets ETXTBSY.
> 
> Issuing this ioctl on a ro file was already allowed for root/cap.
> 
> Tested on btrfs and not-yet-merged xfs, as only them implement this ioctl.
> 
> Signed-off-by: Adam Borowski <kilobyte@angband.pl>

Reviewed-by: Mark Fasheh <mfasheh@suse.de>
	--Mark

--
Mark Fasheh
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Darrick J. Wong July 19, 2016, 2:41 a.m. UTC | #2
On Mon, Jul 18, 2016 at 12:13:38AM +0200, Adam Borowski wrote:
> Instead of checking the mode of the file descriptor, let's check whether it
> could have been opened rw.  This allows fixing intermittent exec failures
> when deduping a live system: anyone trying to exec a file currently being
> deduped gets ETXTBSY.
> 
> Issuing this ioctl on a ro file was already allowed for root/cap.
> 
> Tested on btrfs and not-yet-merged xfs, as only them implement this ioctl.
> 
> Signed-off-by: Adam Borowski <kilobyte@angband.pl>

Could you please send an xfstest to test this aspect of the dedupe ioctl?

--D

> ---
>  fs/read_write.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/read_write.c b/fs/read_write.c
> index 933b53a..df59dc6 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -1723,7 +1723,7 @@ int vfs_dedupe_file_range(struct file *file, struct file_dedupe_range *same)
>  
>  		if (info->reserved) {
>  			info->status = -EINVAL;
> -		} else if (!(is_admin || (dst_file->f_mode & FMODE_WRITE))) {
> +		} else if (!(is_admin || !inode_permission(dst, MAY_WRITE))) {
>  			info->status = -EINVAL;
>  		} else if (file->f_path.mnt != dst_file->f_path.mnt) {
>  			info->status = -EXDEV;
> -- 
> 2.8.1
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Adam Borowski Oct. 5, 2016, 2:32 p.m. UTC | #3
On Mon, Jul 18, 2016 at 07:41:30PM -0700, Darrick J. Wong wrote:
> On Mon, Jul 18, 2016 at 12:13:38AM +0200, Adam Borowski wrote:
> > Instead of checking the mode of the file descriptor, let's check whether it
> > could have been opened rw.  This allows fixing intermittent exec failures
> > when deduping a live system: anyone trying to exec a file currently being
> > deduped gets ETXTBSY.
> > 
> > Issuing this ioctl on a ro file was already allowed for root/cap.
> > 
> > Tested on btrfs and not-yet-merged xfs, as only them implement this ioctl.
> > 
> > Signed-off-by: Adam Borowski <kilobyte@angband.pl>
> 
> Could you please send an xfstest to test this aspect of the dedupe ioctl?

(Sorry for a late answer, at the time I kept thinking about possible other
tests.)

It looks like I don't see an appropriate test here.  The reason is, while
the intermittent failures can be reliably reproduced, they're not caused by
this ioctl but its prerequisite -- ie, needing the file open rw, and that
causing EXTXBSY on exec is expected and correct.

So what else could be tested?  Not general operation of FIDEDUPERANGE -- it
already has adequate tests.

As for this very change, ie, being able to dedupe with O_RDONLY:
1. it's in vfs rather than individual fs drivers, thus is unlikely to
   regress or fail to account for bugs in a new driver
2. the code to open the file lives in xfs_io in xfsprogs rather than
   fstests, thus changing it would need careful coordination between
   the two
On the other hand, once this commit gets merged, it'd be trivial to change
xfsprogs to do:
   open(..., (kernel version >= 4.9)?O_RDONLY:O_RDWR);
which would make all existing tests also guard against regressions in the
positive permissions case.


Meow!

Patch
diff mbox

diff --git a/fs/read_write.c b/fs/read_write.c
index 933b53a..df59dc6 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1723,7 +1723,7 @@  int vfs_dedupe_file_range(struct file *file, struct file_dedupe_range *same)
 
 		if (info->reserved) {
 			info->status = -EINVAL;
-		} else if (!(is_admin || (dst_file->f_mode & FMODE_WRITE))) {
+		} else if (!(is_admin || !inode_permission(dst, MAY_WRITE))) {
 			info->status = -EINVAL;
 		} else if (file->f_path.mnt != dst_file->f_path.mnt) {
 			info->status = -EXDEV;