Re: [PATCH v3 0/3] btrfs: Add three new unprivileged ioctls to allow normal users to call "sub list/show" etc.

[Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>]

On 2018/04/11 3:23, David Sterba wrote:
> On Mon, Mar 19, 2018 at 04:27:09PM +0900, Misono, Tomohiro wrote:
>> changelog:
>>
>> v2-> v3
>>  - fix kbuild test bot warning
>> v1 -> v2
>>   - completely reimplement 1st/2nd ioctl to have user friendly api
>>   - various cleanup, remove unnecessary goto
>> ===
>>
>> This adds three new unprivileged ioctls:
>>
>> 1st patch: ioctl which returns subvolume information of ROOT_ITEM and ROOT_BACKREF
>> 2nd patch: ioctl which returns subvolume information of ROOT_REF (without subvolume name)
>> 3rd patch: user version of ino_lookup ioctl which also peforms permission check.
> 
> The overall approach to listing subvolumes looks good. We can enumerate
> them, get the relations and the details. Making some sense of that in
> the userspace is a whole different and maybe more difficult topic.
> 
> I hope we could use the opportunity to clean up the listing commandline
> interface and output at the same time, as there's going to be possibly
> some incompatibility introduced.
> 
> We need to start with current usecases and how they're implemented or
> mis-implemented (ie. leading to confusion). The discussions I've read so
> far cover a good part of that.

So, I'd like to continue working on progs part based on these ioctls
but there are some things I want to confirm.

I think current problems of "sub list" are:
  - printed path is ambiguous\v (the output may differ when specified different path in the fs)
  - -a or -o options do not work well
  - top level filed is meaningless

My idea of new list is (as in [1]):
  - (default, use new ioctls for normal user)\v
     list the subvolumes under the specified path, including subvolumes mounted
\v     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, use TREE_SEARCH ioctl)
\v     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.
  - (-o option)
     deprecated

and also cleanups of some fields.
For root, TREE_SEARCH ioctl still can be used so that new progs works with old kernels.

Do you have any comments about this?

Also, are you going to merge the omitted libbtrfsuitl patch which refactor "sub list" [2]?
This patch actually does two things:
 1. remove subvol print function from libbtrfs (abi break) and 
 2. refactor "sub list" by using libbtrfsutil.

Is the first part the reason this patch was omitted?

However, by this change subvolume information will be stored in an array instead of rb tree.
In order to implement proposed default behavior, this is a preferred way;
we can't use rb tree because there may exist the subvolume with the same id in different path.

In any case, I'd like to know which version of btrfs-progs I should use to implement the features.

Regards,
Tomohiro Misono

[1] https://www.mail-archive.com/linux-btrfs@vger.kernel.org/msg75119.html
[2] https://www.mail-archive.com/linux-btrfs@vger.kernel.org/msg73601.html

> 
> I'll review and add the kernel patches to misc-next to get some testing
> coverage. As this is a non-restricted ioctl full of pointers and bytes
> shuffled around, we will have to do an extra security-oriented review.
> 
>