Message-ID: <YLjEOMaxk8/8GOex@x1-carbon>
Date:   Thu, 3 Jun 2021 11:59:53 +0000
From:   Niklas Cassel <Niklas.Cassel@....com>
To:     Damien Le Moal <Damien.LeMoal@....com>
CC:     "dsterba@...e.cz" <dsterba@...e.cz>, Jens Axboe <axboe@...nel.dk>,
        "linux-block@...r.kernel.org" <linux-block@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] blk-zoned: allow BLKREPORTZONE without CAP_SYS_ADMIN

On Thu, Jun 03, 2021 at 11:20:33AM +0000, Damien Le Moal wrote:
> On 2021/06/03 19:07, David Sterba wrote:
> > On Thu, Jun 03, 2021 at 10:00:08AM +0000, Damien Le Moal wrote:
> >> On 2021/06/03 18:54, David Sterba wrote:
> >>> On Mon, May 31, 2021 at 01:54:53PM +0000, Niklas Cassel wrote:
> >>>> From: Niklas Cassel <niklas.cassel@....com>
> >>>>
> >>>> Performing a BLKREPORTZONE operation should be allowed under the same
> >>>> permissions as read(). (read() does not require CAP_SYS_ADMIN).
> >>>>
> >>>> Remove the CAP_SYS_ADMIN requirement, and instead check that the fd was
> >>>> successfully opened with FMODE_READ. This way BLKREPORTZONE will match
> >>>> the access control requirement of read().
> >>>
> >>> Does this mean that a process that does not have read nor write access
> >>> to the device itself (blocks) is capable of reading the zone
> >>> information? Eg. some monitoring tool.
> >>
> >> With this change, to do a report zones, the process will only need to have read
> >> access to the device. And if it has read access, it also means that it can read
> >> the zones content.
> > 
> > Ok, so this is a bit restricting. The zone information is like block
> > device metadata, comparing it to a file that has permissionx 0600 I can
> > see the all the stat info (name, tiemstamps) but can't read the data.
> > 
> > But as the ioctl work, it needs a file descriptor and there's probably
> > no way to separate the permissions to read blocks and just the metadata.
> > For a monitoring/reporting tool this would be useful. Eg. for btrfs it
> > could be part of filesystem status overview regarding full or near-full
> > zones and emitting an early warning or poking some service to start the
> > reclaim.
> 
> You lost me... the change is less restrictive than before because the process
> does not need SYS_CAP_ADMIN anymore. The block device file open is untouched, no
> change. So whatever process could open it before, will still be able to do so as
> is. More processes will be able to do report zones with the change. That is all
> really that changes, so I do not see what potentially breaks, nor how this may
> prevent writing some monitoring tool. Whoever can open the block device file has
> FMODE_READ rights, no ? Am I missing something here ?

What David is saying is that for e.g. stat(), you can get metadata when you
don't even have read permission for a device, since stat() takes a pathname.

An ioctl requires you to first do an open(), which will will check permissions,
so implementing the same is not really possible for an ioctl like BLKREPORTZONE.

However, I think the current ioctl is fine.
The amount of data that is transferred from a zoned block device for the zone
report is more than the data that is transferred when someone does a stat(),
so in one way getting the zone report is more like a read.

Doing what David suggests would, as far as I can tell, require another solution
than the existing ioctl method, which this patch changes.

We can think about his suggestion, but it would need to be addressed is a
separate patch series. (If his suggestion is something that we want to pursue.)


Kind regards,
Niklas

Powered by blists - more mailing lists