Re: [RFC PATCH v3 0/7] btrfs-progs: Allow normal user to call "subvolume list/show"
On 03/30/2018 06:46 AM, Misono Tomohiro wrote:
> On 2018/03/30 2:35, Goffredo Baroncelli wrote:
>> Hi Misono,
>
> Hello,
[...]
>> My conclusion, is that your work is not consistent with the current
>> implementation; so I am suggesting to create a new subcommand ("btrfs sub
>> tree" ?) where you can use, without constraint, your api.
>
> I agree that the current output of "sub list" is confusing first of all and
> it is not easy to keep consistent behavior between user and root.
>
> So, I think "sub list" (or new command) should be:
> - (default) list the subvolumes under the specified path, including
> subvolumes mounted below the specified path.
> Any user can do this (with appropriate permission checks).
> The path to be printed is the relative from the specified path.
> - (-a option) list all the subvolmumes in the filesystem. Only root can do
> this.
> The path to be printed is the absolute path from the toplevel
> subvolume.
>
> This would need some changes in progs code, but I think we can use the same
> new ioctls I proposed[1] for the default behavior.
>
> I'd like to update "sub list" command eventually rather than adding new
> command, even though this breaks the compatibility.
> I want to know what other developer/user think.
>
> [1] https://www.spinics.net/lists/linux-btrfs/msg76003.html
>
>>
>> Another small differences that I found is in the subcommand "btrfs sub
>> show". This is not capable to show the snapshots of a subvolume; I think
>> that, in "user mode", it should be reported that there is a "missing
>> capabilities" problem than to show an empty list
>
> Yes, this is because that only the snapshots *under the specified path* are
> searched.
> This can be easily changed to search under the mount point, but this also
> results in
> slight change in print format of the path for root. I tried to keep as much
> consistency in this version.
When I perform a snapshot, I think the new snapshot more as a sister/brother
than a child, so I put it at the same level instead of below the source
subvolume. Moreover the path printed at the first line seems to be relative at
the root of filesystem.
> Thanks,
> Tomohiro Misono
>
>>
>> Below an example of what I am say:
>>
>> ghigo@venice:/tmp$ btrfs sub cre a
>> ghigo@venice:/tmp$ btrfs sub snap a c
>>
>> ghigo@venice:/tmp$ # patched btrfs
>> ghigo@venice:/tmp$ /home/ghigo/btrfs/btrfs-progs/btrfs sub show a
>> tmp/a
>> Name: a
>> UUID: 0c19a7a4-a7f5-1849-9262-88ab3da504d0
>> Parent UUID:-
>> Received UUID: -
>> Creation time: 2018-03-28 22:48:09 +0200
>> Subvolume ID: 598
>> Generation: 696908
>> Gen at creation:696907
>> Parent ID: 257
>> Top level ID: 257
>> Flags: -
>> Snapshot(s):
>>
>>
>> ghigo@venice:/tmp$ # stock btrfs
>> ghigo@venice:/tmp$ sudo /home/ghigo/btrfs/btrfs-progs/btrfs sub show a
>> ^^
>> tmp/a
>> Name: a
>> UUID: 0c19a7a4-a7f5-1849-9262-88ab3da504d0
>> Parent UUID:-
>> Received UUID: -
>> Creation time: 2018-03-28 22:48:09 +0200
>> Subvolume ID: 598
>> Generation: 696908
>> Gen at creation:696907
>> Parent ID: 257
>> Top level ID: 257
>> Flags: -
>> Snapshot(s):
>> debian/tmp/c
>> ^
>>
>>
>> BR
>> G.Baroncelli
>>
>>
>> -
>> Test performed:
>>
>> ghigo@venice:/tmp$ sudo btrfs sub crea a
>> ghigo@venice:/tmp$ sudo btrfs sub crea a/a1
>> ghigo@venice:/tmp$ sudo btrfs sub crea a/a1/a2
>> ghigo@venice:/tmp$ sudo btrfs sub crea b
>> ghigo@venice:/tmp$ sudo btrfs sub crea b/b1
>> ghigo@venice:/tmp$ sudo chmod og-rwx b/.
>>
>>
>> # stock btrfs progs
>> ghigo@venice:/tmp$ sudo btrfs sub l .
>> ID 257 gen 696886 top level 5 path debian
>> ID 289 gen 587461 top level 257 path var/lib/machines
>> ID 299 gen 693561 top level 5 path boot
>> ID 582 gen 683965 top level 5 path i386
>> ID 592 gen 696884 top level 257 path tmp/a
>> ID 593 gen 696885 top level 592 path tmp/a/a1
>> ID 594 gen 696885 top level 593 path tmp/a/a1/a2
>> ID 595 gen 696887 top level 257 path tmp/b
>> ID 596 gen 696887 top level 595 path tmp/b/b1
>>
>> # patched btrfs progs
>> ghigo@venice:/tmp$ /home/ghigo/btrfs/btrfs-progs/btrfs sub lis .
>> ID 592 gen 696884 top level 257 path a
>> ID 593 gen 696885 top level 592 path a/a1
>> ID 594 gen 696885 top level 593 path a/a1/a2
>> ID 595 gen 0 top level 257 path b
>>
>>
>> ghigo@venice:/tmp$ /home/ghigo/btrfs/btrfs-progs/btrfs sub show a
>> tmp/a
>> Name: a
>>
Re: [RFC PATCH v3 0/7] btrfs-progs: Allow normal user to call "subvolume list/show"
On 03/30/2018 06:46 AM, Misono Tomohiro wrote: > [Un]fortunately, the stock "btrfs sub list" has the capability to show all > the subvolumes, even the ones not mounted, so this can be entirely replaced > by your API. s/can be entirely/can't be entirely/ -- gpg @keyserver.linux.it: Goffredo Baroncelli Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH v3 0/7] btrfs-progs: Allow normal user to call "subvolume list/show"
On 2018/03/30 2:35, Goffredo Baroncelli wrote:
> Hi Misono,
Hello,
>
> I tested you patch, and it seems to work. I verified that the output is
> correct and the permission check is performed. However the output of "btrfs
> sub list" in the "root" mode and the "user" mode are a bit different.
>
> As reported before, I find your output more consistent, and understandable
> than the original one, which is quite confusing about the sub-volume path
> relationship.
>
> The problem which I am talking is probably due to the fact that I mount a
> subvolume as root filesystem, and the root-subvolume (the one with ID=5) in a
> subdirectory (/var/btrfs for what matters).
I didn't think this situation and thanks for pointing out.
Currently this RFC does not care about mounted subvolumes under the specified
path.
However, it should be able to search those subvolumes too and list up all the
subvolumes
including mounted ones (and below it), by changing progs code.
>
> I think that the "stock" "btrfs sub list", trying to scan the metadata of
> btrfs and merging these information with this filesystem layout (composed by
> different subvolumes mounted in different places) doesn't do a good job.
>
> The constraints that your API has (the fact that the subvolume has to be
> accessed by the filesystem), create an output more friendly. Unfortunately
> this output became again confusing when the command is started by root.
>
> [Un]fortunately, the stock "btrfs sub list" has the capability to show all
> the subvolumes, even the ones not mounted, so this can be entirely replaced
> by your API.
>
> My conclusion, is that your work is not consistent with the current
> implementation; so I am suggesting to create a new subcommand ("btrfs sub
> tree" ?) where you can use, without constraint, your api.
I agree that the current output of "sub list" is confusing first of all and
it is not easy to keep consistent behavior between user and root.
So, I think "sub list" (or new command) should be:
- (default) list the subvolumes under the specified path, including
subvolumes mounted below the specified path.
Any user can do this (with appropriate permission checks).
The path to be printed is the relative from the specified path.
- (-a option) list all the subvolmumes in the filesystem. Only root can do
this.
The path to be printed is the absolute path from the toplevel
subvolume.
This would need some changes in progs code, but I think we can use the same new
ioctls I proposed[1] for the default behavior.
I'd like to update "sub list" command eventually rather than adding new
command, even though this breaks the compatibility.
I want to know what other developer/user think.
[1] https://www.spinics.net/lists/linux-btrfs/msg76003.html
>
> Another small differences that I found is in the subcommand "btrfs sub show".
> This is not capable to show the snapshots of a subvolume; I think that, in
> "user mode", it should be reported that there is a "missing capabilities"
> problem than to show an empty list
Yes, this is because that only the snapshots *under the specified path* are
searched.
This can be easily changed to search under the mount point, but this also
results in
slight change in print format of the path for root. I tried to keep as much
consistency in this version.
Thanks,
Tomohiro Misono
>
> Below an example of what I am say:
>
> ghigo@venice:/tmp$ btrfs sub cre a
> ghigo@venice:/tmp$ btrfs sub snap a c
>
> ghigo@venice:/tmp$ # patched btrfs
> ghigo@venice:/tmp$ /home/ghigo/btrfs/btrfs-progs/btrfs sub show a
> tmp/a
> Name: a
> UUID: 0c19a7a4-a7f5-1849-9262-88ab3da504d0
> Parent UUID:-
> Received UUID: -
> Creation time: 2018-03-28 22:48:09 +0200
> Subvolume ID: 598
> Generation: 696908
> Gen at creation:696907
> Parent ID: 257
> Top level ID: 257
> Flags: -
> Snapshot(s):
>
>
> ghigo@venice:/tmp$ # stock btrfs
> ghigo@venice:/tmp$ sudo /home/ghigo/btrfs/btrfs-progs/btrfs sub show a
> ^^
> tmp/a
> Name: a
> UUID: 0c19a7a4-a7f5-1849-9262-88ab3da504d0
> Parent UUID:-
> Received UUID: -
> Creation time: 2018-03-28 22:48:09 +0200
> Subvolume ID: 598
> Generation: 696908
> Gen at creation:696907
> Parent ID: 257
> Top level ID: 257
> Flags: -
> Snapshot(s):
> debian/tmp/c
> ^
>
>
> BR
> G.Baroncelli
>
>
> -
> Test performed:
>
> ghigo@venice:/tmp$ sudo
Re: [RFC PATCH v3 0/7] btrfs-progs: Allow normal user to call "subvolume list/show"
Hi Misono,
I tested you patch, and it seems to work. I verified that the output is correct
and the permission check is performed. However the output of "btrfs sub list"
in the "root" mode and the "user" mode are a bit different.
As reported before, I find your output more consistent, and understandable than
the original one, which is quite confusing about the sub-volume path
relationship.
The problem which I am talking is probably due to the fact that I mount a
subvolume as root filesystem, and the root-subvolume (the one with ID=5) in a
subdirectory (/var/btrfs for what matters).
I think that the "stock" "btrfs sub list", trying to scan the metadata of btrfs
and merging these information with this filesystem layout (composed by
different subvolumes mounted in different places) doesn't do a good job.
The constraints that your API has (the fact that the subvolume has to be
accessed by the filesystem), create an output more friendly. Unfortunately this
output became again confusing when the command is started by root.
[Un]fortunately, the stock "btrfs sub list" has the capability to show all the
subvolumes, even the ones not mounted, so this can be entirely replaced by your
API.
My conclusion, is that your work is not consistent with the current
implementation; so I am suggesting to create a new subcommand ("btrfs sub tree"
?) where you can use, without constraint, your api.
Another small differences that I found is in the subcommand "btrfs sub show".
This is not capable to show the snapshots of a subvolume; I think that, in
"user mode", it should be reported that there is a "missing capabilities"
problem than to show an empty list
Below an example of what I am say:
ghigo@venice:/tmp$ btrfs sub cre a
ghigo@venice:/tmp$ btrfs sub snap a c
ghigo@venice:/tmp$ # patched btrfs
ghigo@venice:/tmp$ /home/ghigo/btrfs/btrfs-progs/btrfs sub show a
tmp/a
Name: a
UUID: 0c19a7a4-a7f5-1849-9262-88ab3da504d0
Parent UUID:-
Received UUID: -
Creation time: 2018-03-28 22:48:09 +0200
Subvolume ID: 598
Generation: 696908
Gen at creation:696907
Parent ID: 257
Top level ID: 257
Flags: -
Snapshot(s):
ghigo@venice:/tmp$ # stock btrfs
ghigo@venice:/tmp$ sudo /home/ghigo/btrfs/btrfs-progs/btrfs sub show a
^^
tmp/a
Name: a
UUID: 0c19a7a4-a7f5-1849-9262-88ab3da504d0
Parent UUID:-
Received UUID: -
Creation time: 2018-03-28 22:48:09 +0200
Subvolume ID: 598
Generation: 696908
Gen at creation:696907
Parent ID: 257
Top level ID: 257
Flags: -
Snapshot(s):
debian/tmp/c
^
BR
G.Baroncelli
-
Test performed:
ghigo@venice:/tmp$ sudo btrfs sub crea a
ghigo@venice:/tmp$ sudo btrfs sub crea a/a1
ghigo@venice:/tmp$ sudo btrfs sub crea a/a1/a2
ghigo@venice:/tmp$ sudo btrfs sub crea b
ghigo@venice:/tmp$ sudo btrfs sub crea b/b1
ghigo@venice:/tmp$ sudo chmod og-rwx b/.
# stock btrfs progs
ghigo@venice:/tmp$ sudo btrfs sub l .
ID 257 gen 696886 top level 5 path debian
ID 289 gen 587461 top level 257 path var/lib/machines
ID 299 gen 693561 top level 5 path boot
ID 582 gen 683965 top level 5 path i386
ID 592 gen 696884 top level 257 path tmp/a
ID 593 gen 696885 top level 592 path tmp/a/a1
ID 594 gen 696885 top level 593 path tmp/a/a1/a2
ID 595 gen 696887 top level 257 path tmp/b
ID 596 gen 696887 top level 595 path tmp/b/b1
# patched btrfs progs
ghigo@venice:/tmp$ /home/ghigo/btrfs/btrfs-progs/btrfs sub lis .
ID 592 gen 696884 top level 257 path a
ID 593 gen 696885 top level 592 path a/a1
ID 594 gen 696885 top level 593 path a/a1/a2
ID 595 gen 0 top level 257 path b
ghigo@venice:/tmp$ /home/ghigo/btrfs/btrfs-progs/btrfs sub show a
tmp/a
Name: a
UUID: 17520c11-ec8b-5b49-a07e-37ba58113261
Parent UUID:-
Received UUID: -
Creation time: 2018-03-28 22:19:48 +0200
Subvolume ID: 592
Generation: 696884
Gen at creation:696883
Parent ID: 257
Top level ID: 257
Flags: -
Snapshot(s):
On 03/19/2018 08:30 AM, Misono, Tomohiro wrote:
> changelog:
>
> v2 -> v3
> - use get_euid() to check the caller's privilege (and remove 3rd patch)
> - improve error handling
> v1 -> v2
> - add independent error handling patch (1st patch)
> - reimplement according to ioctl change
>
Re: [RFC PATCH v3 0/7] btrfs-progs: Allow normal user to call "subvolume list/show"
On Mon, Mar 19, 2018 at 04:30:17PM +0900, Misono, Tomohiro wrote: > This is a part of RFC I sent last December[1] whose aim is to improve normal > users' usability. > The remaining works of RFC are: > - Allow "sub delete" for empty subvolume I don't mean to scope creep on you, but I have a couple of wishes related to this topic: - allow "rmdir" to remove an empty subvolume, i.e. when a subvolume is detected in rmdir, try switching to subvol delete before returning an error. This lets admin tools that are not btrfs-aware do 'rm -fr' on a user directory when it contains a subvolume. Legacy admin tools (or legacy tools in general) can't remove a subvol, and there is no solution for environments where we can't just fire users who create them. - mount option to restrict "sub create" and "sub snapshot" to root only. If we get "rmdir" working then this is significantly less important. > - Allow "qgroup show" to check quota limit > > [1] https://www.mail-archive.com/linux-btrfs@vger.kernel.org/msg70991.html signature.asc Description: PGP signature
[RFC PATCH v3 0/7] btrfs-progs: Allow normal user to call "subvolume list/show"
changelog: v2 -> v3 - use get_euid() to check the caller's privilege (and remove 3rd patch) - improve error handling v1 -> v2 - add independent error handling patch (1st patch) - reimplement according to ioctl change - various cleanup === This RFC implements user version of "subvolume list/show" using three new ioctls. The ioctl patch to the kernel can be found in the ML titled "[PATCH v3 0/3] btrfs: Add three new unprivileged ioctls to allow normal users to call "sub list/show" etc. 1th patch is independent and improvements of error handling 2nd-4th are some prepartion works. 5th patch is the main part. 6th-7th adds the test for "subvolume list" The main behavior differences between root and normal users are: - "sub list" list the subvolumes which exist under the specified path (including the path itself). The specified path itself is not needed to be a subvolume. Also If the subvolume cannot be opend but the parent directory can be, the information other than name or id would be zeroed out. - snapshot filed of "subvolume show" just lists the snapshots under the specified subvolume. This is a part of RFC I sent last December[1] whose aim is to improve normal users' usability. The remaining works of RFC are: - Allow "sub delete" for empty subvolume - Allow "qgroup show" to check quota limit [1] https://www.mail-archive.com/linux-btrfs@vger.kernel.org/msg70991.html Tomohiro Misono (7): btrfs-progs: sub list: Call rb_free_nodes() in error path btrfs-progs: ioctl: Add 3 definitions of new unprivileged ioctl btrfs-progs: sub list: Pass specified path down to btrfs_list_subvols() btrfs-progs: fallback to open without O_NOATIME flag in find_mount_root() btrfs-progs: sub list: Allow normal user to call "subvolume list/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 btrfs-list.c | 376 +++-- btrfs-list.h | 7 +- cmds-subvolume.c | 14 +- ioctl.h| 86 +++ tests/cli-tests/009-subvolume-list/test.sh | 136 +++ tests/common | 10 + utils.c| 13 +- 7 files changed, 609 insertions(+), 33 deletions(-) create mode 100755 tests/cli-tests/009-subvolume-list/test.sh -- 2.14.3 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
