[resend,1/2] btrfs: allow defrag on a file opened ro that has rw permissions
diff mbox

Message ID 20180717220900.3588-1-kilobyte@angband.pl
State New
Headers show

Commit Message

Adam Borowski July 17, 2018, 10:08 p.m. UTC
Requiring a rw descriptor conflicts both ways with exec, returning ETXTBSY
whenever you try to defrag a program that's currently being run, or
causing intermittent exec failures on a live system being defragged.

As defrag doesn't change the file's contents in any way, there's no reason
to consider it a rw operation.  Thus, let's check only whether the file
could have been opened rw.  Such access control is still needed as
currently defrag can use extra disk space, and might trigger bugs.

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

Comments

David Sterba July 23, 2018, 3:26 p.m. UTC | #1
On Wed, Jul 18, 2018 at 12:08:59AM +0200, Adam Borowski wrote:
> Requiring a rw descriptor conflicts both ways with exec, returning ETXTBSY
> whenever you try to defrag a program that's currently being run, or
> causing intermittent exec failures on a live system being defragged.
> 
> As defrag doesn't change the file's contents in any way, there's no reason
> to consider it a rw operation.  Thus, let's check only whether the file
> could have been opened rw.  Such access control is still needed as
> currently defrag can use extra disk space, and might trigger bugs.
> 
> Signed-off-by: Adam Borowski <kilobyte@angband.pl>
> ---
>  fs/btrfs/ioctl.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 43ecbe620dea..01c150b6ab62 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -2941,7 +2941,8 @@ static int btrfs_ioctl_defrag(struct file *file, void __user *argp)
>  		ret = btrfs_defrag_root(root);
>  		break;
>  	case S_IFREG:
> -		if (!(file->f_mode & FMODE_WRITE)) {
> +		if (!capable(CAP_SYS_ADMIN) &&
> +		    inode_permission(inode, MAY_WRITE)) {

The dedupe ioctl does the admin check or the FMODE_WRITE, which means
admin can dedupe anything but user has to have the file write open.
Doing the same for defrag should be ok for same reasons it is for
dedupe.

I'm not sure about using plain inode_permissions here, though it looks
correct and I'm not able to see inode vs file descriptor issues that
could cause trouble here. There are uid/gid, rws, acl, immutable,
capabilities, namespace aware checks, security hooks.

So, I'll add the patch to 4.19 queue. It's small and isolated change so
a revert would be easy in case we find something bad. The 2nd patch
should be IMHO part of this change as it's logical to return the error
code in the patch that modifies the user visible behaviour.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Adam Borowski July 24, 2018, 8:35 p.m. UTC | #2
On Mon, Jul 23, 2018 at 05:26:24PM +0200, David Sterba wrote:
> On Wed, Jul 18, 2018 at 12:08:59AM +0200, Adam Borowski wrote:
(Combined with as-folded)

| | btrfs: allow defrag on a file opened read-only that has rw permissions
| |
> > Requiring a rw descriptor conflicts both ways with exec, returning ETXTBSY
> > whenever you try to defrag a program that's currently being run, or
> > causing intermittent exec failures on a live system being defragged.
> > 
> > As defrag doesn't change the file's contents in any way, there's no reason
> > to consider it a rw operation.  Thus, let's check only whether the file
> > could have been opened rw.  Such access control is still needed as
> > currently defrag can use extra disk space, and might trigger bugs.
<-----
| | We give EINVAL when the request is invalid; here it's ok but merely the
| | user has insufficient privileges.  Thus, this return value reflects the
| | error better -- as discussed in the identical case for dedupe.
| |
| | According to codesearch.debian.net, no userspace program distinguishes
| | these values beyond strerror().
| |
| | Signed-off-by: Adam Borowski <kilobyte@angband.pl>
| | Reviewed-by: David Sterba <dsterba@suse.com>
| | [ fold the EPERM patch from Adam ]
| | Signed-off-by: David Sterba <dsterba@suse.com>

[...]
> So, I'll add the patch to 4.19 queue. It's small and isolated change so
> a revert would be easy in case we find something bad. The 2nd patch
> should be IMHO part of this change as it's logical to return the error
> code in the patch that modifies the user visible behaviour.

A nitpick: the new commit message has a dangling pointer "this" to the title
of the commit that was squashed.  It was:

| btrfs: defrag: return EPERM not EINVAL when only permissions fail

It'd be nice if it could be inserted in some form in the place I marked with
an arrow.

But then, commit messages are not vital.  The actual functionality patch has
been applied correctly.  And thanks for adding the comment.


Meow!
David Sterba Aug. 1, 2018, 1:03 p.m. UTC | #3
On Tue, Jul 24, 2018 at 10:35:28PM +0200, Adam Borowski wrote:
> On Mon, Jul 23, 2018 at 05:26:24PM +0200, David Sterba wrote:
> > On Wed, Jul 18, 2018 at 12:08:59AM +0200, Adam Borowski wrote:
> (Combined with as-folded)
> 
> | | btrfs: allow defrag on a file opened read-only that has rw permissions
> | |
> > > Requiring a rw descriptor conflicts both ways with exec, returning ETXTBSY
> > > whenever you try to defrag a program that's currently being run, or
> > > causing intermittent exec failures on a live system being defragged.
> > > 
> > > As defrag doesn't change the file's contents in any way, there's no reason
> > > to consider it a rw operation.  Thus, let's check only whether the file
> > > could have been opened rw.  Such access control is still needed as
> > > currently defrag can use extra disk space, and might trigger bugs.
> <-----
> | | We give EINVAL when the request is invalid; here it's ok but merely the
> | | user has insufficient privileges.  Thus, this return value reflects the
> | | error better -- as discussed in the identical case for dedupe.
> | |
> | | According to codesearch.debian.net, no userspace program distinguishes
> | | these values beyond strerror().
> | |
> | | Signed-off-by: Adam Borowski <kilobyte@angband.pl>
> | | Reviewed-by: David Sterba <dsterba@suse.com>
> | | [ fold the EPERM patch from Adam ]
> | | Signed-off-by: David Sterba <dsterba@suse.com>
> 
> [...]
> > So, I'll add the patch to 4.19 queue. It's small and isolated change so
> > a revert would be easy in case we find something bad. The 2nd patch
> > should be IMHO part of this change as it's logical to return the error
> > code in the patch that modifies the user visible behaviour.
> 
> A nitpick: the new commit message has a dangling pointer "this" to the title
> of the commit that was squashed.  It was:
> 
> | btrfs: defrag: return EPERM not EINVAL when only permissions fail
> 
> It'd be nice if it could be inserted in some form in the place I marked with
> an arrow.

Right. I've replaced 'this' with 'EPERM', thanks for catching it.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 43ecbe620dea..01c150b6ab62 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2941,7 +2941,8 @@  static int btrfs_ioctl_defrag(struct file *file, void __user *argp)
 		ret = btrfs_defrag_root(root);
 		break;
 	case S_IFREG:
-		if (!(file->f_mode & FMODE_WRITE)) {
+		if (!capable(CAP_SYS_ADMIN) &&
+		    inode_permission(inode, MAY_WRITE)) {
 			ret = -EINVAL;
 			goto out;
 		}