Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20220124150209.22202-1-bgeffon@google.com> <20220124151434.GB20331@agk-cloud1.hosts.prod.upshift.rdu2.redhat.com>
 <CADyq12ykDCswWZw05OdyYfP-zT6afuhXbckii1m1egQ2fSwB4w@mail.gmail.com> <20220125002025.GA21887@agk-cloud1.hosts.prod.upshift.rdu2.redhat.com>
In-Reply-To: <20220125002025.GA21887@agk-cloud1.hosts.prod.upshift.rdu2.redhat.com>
From:   Brian Geffon <bgeffon@google.com>
Date:   Tue, 25 Jan 2022 09:25:52 -0500
Message-ID: <CADyq12zS5ZP+0DBdiVWq0_kt4-n0ykdwH_Nw=kJj0V7oF=3OdQ@mail.gmail.com>
Subject: Re: [PATCH] dm: introduce a no open flag for deferred remove
To:     Brian Geffon <bgeffon@google.com>,
        Alasdair Kergon <agk@redhat.com>,
        Mike Snitzer <snitzer@redhat.com>, dm-devel@redhat.com,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Jan 24, 2022 at 7:21 PM Alasdair G Kergon <agk@redhat.com> wrote:
>
> On Mon, Jan 24, 2022 at 10:25:47AM -0500, Brian Geffon wrote:
> > Thank you for looking at this. There are a few reasons this might be
> > useful, the first is if you're trying to speed up a graceful teardown
> > of the device by informing userspace that this device is going to be
> > removed in the near future. Another might be on systems where it might
> > be worthwhile to not have users with CAP_DAC_OVERRIDE be able to open
> > the device. The logic on this second case is that, suppose you have a
> > dm-crypt block device which is backing swap, the data on this device
> > is ephemeral so a flow might be to setup swap followed by dmsetup
> > remove --deferred /dev/mapper/encrypted-swap. This will guarantee that
> > as soon as swap is torn down the encrypted block device is dropped,
> > additionally with this new flag you'll be guaranteed that there can be
> > no further opens on it.
>
> And is that the reason you propose this?
> - You want a special exclusive 'one time open' device that
>   self-destructs when closed?

Yes, this is the primary reason I'm exploring this. I tried to find an
implementation that might be useful in other situations which is why I
landed on this approach.

>
> > No, this is fully backwards compatible with the current deferred
> > remove behavior, it's not required. Additionally, since on the actual
> > remove userspace would receive an -ENXIO already once the remove
> > process has started it seems reasonable to return -ENXIO in the
> > deferred remove case when this flag is enabled.
>
> Well I feel it does break existing semantics which is why we wrote
> the code the way we did.  The state can be long-lived, the code
> that has it open might legitimately want to open it again in
> parallel etc. - in general this seems a bad idea.

Thanks for explaining and providing that context.

>
> But if the reason for this is basically "make it harder for
> anything else to access my encrypted swap" and to deliberately
> prevent access, then let's approach the requirement from that angle.
> Are there alternative implementations with interventions at different
> points?

Absolutely, we could perhaps create a new ioctl which allows the
caller to specify the maximum open count, and when the open count hits
that value it fails any new opens with -EBUSY. Perhaps this would be
enforced in dm_get_device? This type of behavior could theoretically
mimic the patch I mailed when used in conjunction with deferred
removal. Is this an approach you think might make more sense with the
existing design? Are there any implementation points you believe might
make more sense for such a feature?

>  Are there similar requirements for devices that don't need
> deferred remove?

I cannot immediately think of a situation where you'd do this without
deferred removal.

>  If this is indeed the best place to insert this type
> of restriction, then we should label it and document it accordingly so
> people don't mistakenly use it for the 'normal' case.  (We always keep
> libdevmapper and dmsetup in sync with kernel interface extensions, so
> we'd seek a tiny patch to do that too.)

Yes, absolutely. I'll happily send patches for userspace libraries,
applications, and documentation once we converge on an acceptable
approach.

Thanks again for spending your time discussing this,
Brian