On Thu, Dec 07, 2017 at 07:21:46AM -0500, Austin S. Hemmelgarn wrote:
> On 2017-12-07 06:55, Duncan wrote:
> >Misono, Tomohiro posted on Thu, 07 Dec 2017 16:15:47 +0900 as excerpted:
> >
> >>On 2017/12/07 11:56, Duncan wrote:
> >>>Austin S. Hemmelgarn posted on Wed, 06 Dec 2017 07:39:56 -0500 as
> >>>excerpted:
> >>>
> >>>>Somewhat OT, but the only operation that's remotely 'instant' is
> >>>>creating an empty subvolume.  Snapshot creation has to walk the tree
> >>>>in the subvolume being snapshotted, which can take a long time (and as
> >>>>a result of it's implementation, also means BTRFS snapshots are _not_
> >>>>atomic). Subvolume deletion has to do a bunch of cleanup work in the
> >>>>background (though it may be fairly quick if it was a snapshot and the
> >>>>source subvolume hasn't changed much).
> >>>
> >>>Indeed, while btrfs in general has taken a strategy of making
> >>>/creating/ snapshots and subvolumes fast, snapshot deletion in
> >>>particular can take some time[1].
> >>>
> >>>And in that regard a question just occurred to me regarding this whole
> >>>very tough problem of a user being able to create but not delete
> >>>subvolumes and snapshots:
> >>>
> >>>Given that at least snapshot deletion (not so sure about non-snapshot
> >>>subvolume deletion, tho I strongly suspect it would depend on the
> >>>number of cross-subvolume reflinks) is already a task that can take
> >>>some time, why /not/ just bite the bullet and make the behavior much
> >>>more like the directory deletion, given that subvolumes already behave
> >>>much like directories.  Yes, for non-root, that /does/ mean tracing the
> >>>entire subtree and checking permissions, and yes, that's going to take
> >>>time and lower performance somewhat, but subvolume and in particular
> >>>snapshot deletion is already an operation that takes time, so this
> >>>wouldn't be unduly changing the situation, and it would eliminate the
> >>>entire class of security issues that come with either asymmetrically
> >>>restricting deletion (but not creation) to root on the one hand,
> >>
> >>>or possible data loss due to allowing a user to delete a subvolume they
> >>>couldn't delete were it an ordinary directory due to not owning stuff
> >>>further down the tree.
> >>
> >>But, this is also the very reason I'm for "sub del" instead of unlink().
> >>Since snapshot creation won't check the permissions of the containing
> >>files/dirs, it can copy a directory which cannot be deleted by the user.
> >>Therefore if we won't allow "sub del" for the user, he couldn't remove
> >>the snapshot.
> >
> >Maybe snapshot creation /should/ check all that, in ordered to allow
> >permissions to allow deletion.
> >
> >Tho that would unfortunately increase the creation time, and btrfs is
> >currently optimized for fast creation time.
> >
> >Hmm... What about creating a "temporary" snapshot if not root, then
> >walking the tree to check perms and deleting it without ever showing it
> >to userspace if the perms wouldn't let the user delete it.  That would
> >retain fast creation logic, tho it wouldn't show up until the perms walk
> >was completed.
> >
> I would argue that it makes more sense to keep snapshot creation as
> is, keep the subvolume deletion command as is (with some proper
> permissions checks of course), and just make unlink() work for
> subvolumes like it does for directories.

   Definitely this.

   Principle of least surprise.

   Hugo.

-- 
Hugo Mills             | ... one ping(1) to rule them all, and in the
hugo@... carfax.org.uk | darkness bind(2) them.
http://carfax.org.uk/  |
PGP: E2AB1DE4          |                                                Illiad