mbox series

[RESEND,0/8] btrfs-progs: sub: Relax the privileges of "subvolume list/show"

Message ID cover.1543294426.git.misono.tomohiro@jp.fujitsu.com (mailing list archive)
Headers show
Series btrfs-progs: sub: Relax the privileges of "subvolume list/show" | expand

Message

Misono Tomohiro Nov. 27, 2018, 5:24 a.m. UTC
Hello,

This is basically the resend of 
  "[PATCH v2 00/20] btrfs-progs: Rework of "subvolume list/show" and relax the
	root privileges of them" [1]
which I submitted in June. The aim of this series is to allow non-privileged user
to use basic subvolume functionality (create/list/snapshot/delete; this allows "list")

They were once in devel branch with some whitespace/comment modification by david.
I rebased them to current devel branch.

github: https://github.com/t-msn/btrfs-progs/tree/rework-sub-list

Basic logic/code is the same as before. Some differences are:
 - Use latest libbtrfsutil from Omar [2] (thus drop first part of patches).
   As a result, "sub list" cannot accept an ordinary directry to be
   specified (which is allowed in previous version)
 - Drop patches which add new options to "sub list"
 - Use 'nobody' as non-privileged test user just like libbtrfsutil test
 - Update comments

Importantly, in order to make output consistent for both root and non-privileged
user, this changes the behavior of "subvolume list": 
 - (default) Only list in subvolume under the specified path.
   Path needs to be a subvolume.
 - (-a) filter is dropped. i.e. its output is the same as the
        default behavior of "sub list" in progs <= 4.19

Therefore, existent scripts may need to update to add -a option
(I believe nobody uses current -a option).
If anyone thinks this is not good, please let me know.

Behavior summary from cover letter in [1]
====
* Behavior summary of new "sub list"
  - default (no option)
    - lists subvolumes below the specified path (inc. path itself)
    - If new ioctls exists non-privileged user can call it
        (subvolumes to which the user cannot access will be skipped)

  - -a
    - updated to remove filter. i.e. the output is the same as current progs
      without option (require root privileges)

 [Example]
  $ mkfs.btrfs -f $DEV
  $ mkfs.btrfs -f $DEV2
  $ mount $DEV $MNT

  $ btrfs subvolume create $MNT/AAA
  $ btrfs subvolume create $MNT/BBB
  $ btrfs subvolume create $MNT/CCC
  $ btrfs subvolume create $MNT/DDD
  $ mkdir $MNT/AAA/bbb
  $ mkdir $MNT/AAA/ccc
  $ mkdir $MNT/AAA/other

  $ umount $MNT
  $ mount -o subvol=AAA $DEV $MNT
  $ mount -o subvol=BBB $DEV $MNT/bbb
  $ mount -o subvol=CCC $DEV $MNT/ccc
  $ mount -o $DEV2 $MNT/other

  $ btrfs subvolume list $MNT # print subvolumes below the path
  ID 256 gen 10 top level 5 path .

  $ btrfs subvolume list -a $MNT
  # print all subvolumes in the fs. the same output as progs<=4.19 without option
  ID 256 gen 10 top level 5 path AAA
  ID 258 gen 7 top level 5 path BBB
  ID 259 gen 8 top level 5 path CCC
  ID 260 gen 9 top level 5 path DDD

* Behavior summary of new "sub show"
  - No change for root's output
  - If new ioctls exists, non-privileged user can call it
    - In that case, path to be shown is absolute path
      (for root, it is relative to top-level subvolume)
      Also, snapshots to be shown are to which the user can access from current
			mount point.
      (for root, all snapshots in the fs)
===

[1] https://lore.kernel.org/linux-btrfs/cover.1529310485.git.misono.tomohiro@jp.fujitsu.com/
[2] https://lore.kernel.org/linux-btrfs/cover.1542181521.git.osandov@fb.com/

Thanks,
Misono

Misono Tomohiro (8):
  btrfs-progs: sub list: Use libbtrfsuitl for subvolume list
  btrfs-progs: sub list: factor out main part of btrfs_list_subvols
  btrfs-progs: sub list: Change the default behavior of "subvolume list"
    and allow non-privileged user to call it
  btrfs-progs: sub list: Update -a option and remove meaningless filter
  btrfs-progs: utils: Fallback to open without O_NOATIME flag in
    find_mount_root():
  btrfs-progs: sub show: Allow non-privileged user to call "subvolume
    show"
  btrfs-progs: test: Add helper function to check if test user exists
  btrfs-porgs: test: Add cli-test/009 to check subvolume list for both
    root and normal user

 Documentation/btrfs-subvolume.asciidoc     |   25 +-
 cmds-subvolume.c                           | 1149 +++++++++++++++++++-
 tests/cli-tests/009-subvolume-list/test.sh |  130 +++
 tests/common                               |   10 +
 utils.c                                    |    3 +
 5 files changed, 1266 insertions(+), 51 deletions(-)
 create mode 100755 tests/cli-tests/009-subvolume-list/test.sh

Comments

Martin Steigerwald Nov. 27, 2018, 9:48 a.m. UTC | #1
Misono Tomohiro - 27.11.18, 06:24:
> Importantly, in order to make output consistent for both root and
> non-privileged user, this changes the behavior of "subvolume list":
>  - (default) Only list in subvolume under the specified path.
>    Path needs to be a subvolume.

Does that work recursively?

I wound find it quite unexpected if I did btrfs subvol list in or on the 
root directory of a BTRFS filesystem would not display any subvolumes on 
that filesystem no matter where they are.

Thanks,
Tomohiro Misono (Fujitsu) Nov. 28, 2018, 1:26 a.m. UTC | #2
Hi,

> -----Original Message-----
> From: Martin Steigerwald [mailto:martin@lichtvoll.de]
> Sent: Tuesday, November 27, 2018 6:48 PM
> To: Misono, Tomohiro <misono.tomohiro@fujitsu.com>
> Cc: linux-btrfs@vger.kernel.org
> Subject: Re: [PATCH RESEND 0/8] btrfs-progs: sub: Relax the privileges
> of "subvolume list/show"
> 
> Misono Tomohiro - 27.11.18, 06:24:
> > Importantly, in order to make output consistent for both root and
> > non-privileged user, this changes the behavior of "subvolume list":
> >  - (default) Only list in subvolume under the specified path.
> >    Path needs to be a subvolume.
> 
> Does that work recursively?

Not in this version.

Previous version has -f option which recursively search and list subbvolumes
(only if they have the same btrfs fsid):
https://lore.kernel.org/linux-btrfs/84d06767762b4285ddefec0392ee16e2d7e06f62.1529310485.git.misono.tomohiro@jp.fujitsu.com/

However, current "sub list" command already has many options and we cannot
add new one randomly, I drop the patches which add new options in this version.
(In other word, previous version can be divided in two parts: 
  1. Relax the privileges of sub list/show
  2. Add new option to sub list
And this version contains only 1).

> 
> I wound find it quite unexpected if I did btrfs subvol list in or on the
> root directory of a BTRFS filesystem would not display any subvolumes
> on
> that filesystem no matter where they are.

Yes, I think output of -f option is more readable in such case.
If agreement has been made about how "sub list" should really work,
I could update/send patches again.

Thanks,
Misono
Omar Sandoval Dec. 7, 2018, 1:02 a.m. UTC | #3
On Tue, Nov 27, 2018 at 02:24:41PM +0900, Misono Tomohiro wrote:
> Hello,
> 
> This is basically the resend of 
>   "[PATCH v2 00/20] btrfs-progs: Rework of "subvolume list/show" and relax the
> 	root privileges of them" [1]
> which I submitted in June. The aim of this series is to allow non-privileged user
> to use basic subvolume functionality (create/list/snapshot/delete; this allows "list")
> 
> They were once in devel branch with some whitespace/comment modification by david.
> I rebased them to current devel branch.
> 
> github: https://github.com/t-msn/btrfs-progs/tree/rework-sub-list
> 
> Basic logic/code is the same as before. Some differences are:
>  - Use latest libbtrfsutil from Omar [2] (thus drop first part of patches).
>    As a result, "sub list" cannot accept an ordinary directry to be
>    specified (which is allowed in previous version)
>  - Drop patches which add new options to "sub list"
>  - Use 'nobody' as non-privileged test user just like libbtrfsutil test
>  - Update comments
> 
> Importantly, in order to make output consistent for both root and non-privileged
> user, this changes the behavior of "subvolume list": 
>  - (default) Only list in subvolume under the specified path.
>    Path needs to be a subvolume.
>  - (-a) filter is dropped. i.e. its output is the same as the
>         default behavior of "sub list" in progs <= 4.19
> 
> Therefore, existent scripts may need to update to add -a option
> (I believe nobody uses current -a option).
> If anyone thinks this is not good, please let me know.

I think there are a few options in the case that the path isn't a
subvolume:

1. List all subvolumes in the filesystem with randomly mangled paths,
   which is what we currently do.
2. Error out, which is what this version of the series does.
3. List all subvolumes under the containing subvolume, which is what the
   previous version does.
4. List all subvolumes under the containing subvolume that are
   underneath the given path.

Option 1 won't work well for unprivileged users. Option 2 (this series)
is definitely going to break people's workflows/scripts. Option 3 is
unintuitive. In my opinion, option 4 is the nicest, but it may also
break scripts that expect all subvolumes to be printed.

There's also an option 5, which is to keep the behavior the same for
root (like what my previous patch [1] did) and implement option 4 for
unprivileged users.

I think 4 and 5 are the two main choices: do we want to preserve
backwards compatibility as carefully as possible (at the cost of
consistency), or do we want to risk it and improve the interface?

1: https://github.com/osandov/btrfs-progs/commit/fb61c21aeb998b12c1d02532639083d7f40c41e0
Tomohiro Misono (Fujitsu) Dec. 11, 2018, 9:06 a.m. UTC | #4
> -----Original Message-----
> From: Omar Sandoval [mailto:osandov@osandov.com]
> Sent: Friday, December 7, 2018 10:02 AM
> To: Misono, Tomohiro/味曽野 智礼 <misono.tomohiro@fujitsu.com>
> Cc: linux-btrfs@vger.kernel.org
> Subject: Re: [PATCH RESEND 0/8] btrfs-progs: sub: Relax the privileges of
> "subvolume list/show"
> 
> On Tue, Nov 27, 2018 at 02:24:41PM +0900, Misono Tomohiro wrote:
> > Hello,
> >
> > This is basically the resend of
> >   "[PATCH v2 00/20] btrfs-progs: Rework of "subvolume list/show" and relax
> the
> > 	root privileges of them" [1]
> > which I submitted in June. The aim of this series is to allow non-privileged
> user
> > to use basic subvolume functionality (create/list/snapshot/delete; this
> allows "list")
> >
> > They were once in devel branch with some whitespace/comment modification by
> david.
> > I rebased them to current devel branch.
> >
> > github: https://github.com/t-msn/btrfs-progs/tree/rework-sub-list
> >
> > Basic logic/code is the same as before. Some differences are:
> >  - Use latest libbtrfsutil from Omar [2] (thus drop first part of patches).
> >    As a result, "sub list" cannot accept an ordinary directry to be
> >    specified (which is allowed in previous version)
> >  - Drop patches which add new options to "sub list"
> >  - Use 'nobody' as non-privileged test user just like libbtrfsutil test
> >  - Update comments
> >
> > Importantly, in order to make output consistent for both root and non-privileged
> > user, this changes the behavior of "subvolume list":
> >  - (default) Only list in subvolume under the specified path.
> >    Path needs to be a subvolume.
> >  - (-a) filter is dropped. i.e. its output is the same as the
> >         default behavior of "sub list" in progs <= 4.19
> >
> > Therefore, existent scripts may need to update to add -a option
> > (I believe nobody uses current -a option).
> > If anyone thinks this is not good, please let me know.
> 
> I think there are a few options in the case that the path isn't a
> subvolume:
> 
> 1. List all subvolumes in the filesystem with randomly mangled paths,
>    which is what we currently do.
> 2. Error out, which is what this version of the series does.
> 3. List all subvolumes under the containing subvolume, which is what the
>    previous version does.
> 4. List all subvolumes under the containing subvolume that are
>    underneath the given path.
> 
> Option 1 won't work well for unprivileged users. Option 2 (this series)
> is definitely going to break people's workflows/scripts. Option 3 is
> unintuitive. In my opinion, option 4 is the nicest, but it may also
> break scripts that expect all subvolumes to be printed.

I think my previous version is 4, or am I misunderstanding?
In previous version, output could be:
 $ mkfs.btrfs -f $DEV
 $ mount $DEV /mnt

 $ btrfs subvolume create /mnt/sub1
 $ btrfs subvolume create /mnt/sub1/sub2
 $ mkdir /mnt/sub1/dir
 $ btrfs subvolume create /mnt/sub1/dir/sub3

 $ btrfs subvolume list /mnt
  ID 256 gen 8 top level 5 path sub1
  ID 258 gen 7 top level 256 path sub1/sub2
  ID 259 gen 8 top level 256 path sub1/dir/sub3
 
 $ btrfs subvolume list /mnt/sub1
  ID 256 gen 8 top level 5 path .
  ID 258 gen 7 top level 256 path sub2
  ID 259 gen 8 top level 256 path dir/sub3

 $ btrfs subvolume list /mnt/sub1/dir
  ID 259 gen 8 top level 256 path sub3

https://lore.kernel.org/linux-btrfs/6af6dac8829d3ba405e3c53baffd828c9c428ef6.1529310485.git.misono.tomohiro@jp.fujitsu.com/

> 
> There's also an option 5, which is to keep the behavior the same for
> root (like what my previous patch [1] did) and implement option 4 for
> unprivileged users.
> 
> I think 4 and 5 are the two main choices: do we want to preserve
> backwards compatibility as carefully as possible (at the cost of
> consistency), or do we want to risk it and improve the interface?
> 
> 1:
> https://github.com/osandov/btrfs-progs/commit/fb61c21aeb998b12c1d0253263908
> 3d7f40c41e0