linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Adam Borowski <kilobyte@angband.pl>
To: Mark Fasheh <mfasheh@suse.de>
Cc: linux-fsdevel@vger.kernel.org, linux-btrfs@vger.kernel.org,
	linux-xfs@vger.kernel.org
Subject: Re: [PATCH 1/2] vfs: allow dedupe of user owned read-only files
Date: Sat, 12 May 2018 04:49:20 +0200	[thread overview]
Message-ID: <20180512024920.i7duhoi3lnkha4yl@angband.pl> (raw)
In-Reply-To: <20180511192651.21324-2-mfasheh@suse.de>

On Fri, May 11, 2018 at 12:26:50PM -0700, Mark Fasheh wrote:
> The permission check in vfs_dedupe_file_range() is too coarse - We
> only allow dedupe of the destination file if the user is root, or
> they have the file open for write.
> 
> This effectively limits a non-root user from deduping their own
> read-only files. As file data during a dedupe does not change,
> this is unexpected behavior and this has caused a number of issue
> reports. For an example, see:
> 
> https://github.com/markfasheh/duperemove/issues/129
> 
> So change the check so we allow dedupe on the target if:
> 
> - the root or admin is asking for it
> - the owner of the file is asking for the dedupe
> - the process has write access

I submitted a similar patch in May 2016, yet it has never been applied
despite multiple pings, with no NAK.  My version allowed dedupe if:
- the root or admin is asking for it
- the file has w permission (on the inode -- ie, could have been opened rw)

There was a request to include in xfstests a test case for the ETXTBSY race
this patch fixes, but there's no reasonable way to make such a test case:
the race condition is not a bug, it's write-xor-exec working as designed.

Another idea discussed was about possibly just allowing everyone who can
open the file to deduplicate it, as the file contents are not modified in
any way.  Zygo Blaxell expressed a concern that it could be used by an
unprivileged user who can trigger a crash to abuse writeout bugs.

I like this new version better than mine: "root or owner or w" is more
Unixy than "could have been opened w".

> Signed-off-by: Mark Fasheh <mfasheh@suse.de>
> ---
>  fs/read_write.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/read_write.c b/fs/read_write.c
> index c4eabbfc90df..77986a2e2a3b 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -2036,7 +2036,8 @@ 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 || (dst_file->f_mode & FMODE_WRITE) ||
> +			     uid_eq(current_fsuid(), dst->i_uid))) {
I had:
  +		} 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;
> -- 

Meow!
-- 
⢀⣴⠾⠻⢶⣦⠀ 
⣾⠁⢰⠒⠀⣿⡁ 
⢿⡄⠘⠷⠚⠋⠀ Certified airhead; got the CT scan to prove that!
⠈⠳⣄⠀⠀⠀⠀ 

  parent reply	other threads:[~2018-05-12  2:49 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-11 19:26 [PATCH 0/2] vfs: better dedupe permission check Mark Fasheh
2018-05-11 19:26 ` [PATCH 1/2] vfs: allow dedupe of user owned read-only files Mark Fasheh
2018-05-11 23:58   ` Darrick J. Wong
2018-05-12  2:49   ` Adam Borowski [this message]
2018-05-13 18:16     ` Mark Fasheh
2018-05-13 20:50       ` Adam Borowski
2018-05-17 23:01         ` Mark Fasheh
2018-05-11 19:26 ` [PATCH 2/2] vfs: dedupe should return EPERM if permission is not granted Mark Fasheh
2018-05-12  0:06   ` Darrick J. Wong
2018-05-12  4:15     ` Amir Goldstein
2018-05-12  4:37     ` Duncan
2018-05-13 14:30     ` Adam Borowski
2018-05-13 18:21     ` Mark Fasheh
2018-05-13 18:26       ` Darrick J. Wong
2018-05-17  5:15         ` Zygo Blaxell
2018-05-17 23:03           ` Mark Fasheh
2018-05-14 14:58   ` David Sterba
2018-09-10 23:21 [PATCH v6 0/2] vfs: fix dedupe permission check Mark Fasheh
2018-09-10 23:21 ` [PATCH 1/2] vfs: allow dedupe of user owned read-only files Mark Fasheh

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180512024920.i7duhoi3lnkha4yl@angband.pl \
    --to=kilobyte@angband.pl \
    --cc=linux-btrfs@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-xfs@vger.kernel.org \
    --cc=mfasheh@suse.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).