Master key removal semantics

Master key removal semantics

[Josef Bacik]

Hello,

While getting the fstests stuff nailed down to deal with btrfs I ran into
failures with generic/595, specifically the multi-threaded part.

In one thread we have a loop adding and removing the master key.

In the other thread we have us trying to echo a character into a flie in the
encrypted side, and if it succeeds we echo a character into a temporary file,
and then after the runtime has elapsed we compare these two files to make sure
they match.

The problem with this is that btrfs derives the per-extent key from the master
key at writeout time.  Everybody else has their content key derived at flie open
time, so they don't need the master key to be around once the file is opened, so
any writes that occur while that file is held open are allowed to happen.

Sweet Tea had some changes around soft unloading the master key to handle this
case.  Basically we allow the master key to stick around by anybody who may need
it who is currently open, and then any new users get denied.  Once all the
outstanding open files are closed the master key is unloaded.

This keeps the semantics of what happens for everybody else.

What is currently happening with my version of the patchset, which didn't bring
in those patches, is that you get an ENOKEY at writeout time if you remove the
key.  The fstest fails because even tho we let you write to the file sometimes,
it doesn't necessarily mean it'll make it to disk.

If we want to keep the semantics of "when userspace tells us to throw away the
master key, we absolutely throw the master key away" then I can just make
adjustments to the fstests test and call what I have good enough.

If we want to have the semantics of "when userspace tells us to throw away the
master key we'll make it unavailable for any new users, but existing open files
operate as normal" then I can pull in Sweet Tea's soft removal patches and call
it good enough.

There's a third option that is a bit of a middle ground with varying degrees of
raciness.  We could sync the file system before we do the removal just to narrow
the window where we could successfully write to a file but get an ENOKEY at
writeout time.  We could freeze the filesystem to make sure it's sync'ed and
allow any current writers to complete, this would be a stronger version of the
first option, again just narrows the window.  Neither of these cases help if the
file is being held open.  If we wanted to fully deal with the file being held
open case we could set a flag, sync, then remove the key.  Then we add a new
fscrypt_prep_write() hook that filesystems could optionally use, obviously just
btrfs for now, that we'd stick in the write path that would check for this flag
or if the master key had been removed so we can deny dirtying when the key is
removed.

At this point I don't have strong opinions, it's easier for me to just leave it
like it is and change fstests.  Anything else is a change in the semantics of
how the master key is handled, and that's not really a decision I feel
comfortable making for everybody.  Once we nail this detail down I can send the
updated version of all the patches and we can start talking about inclusion.
Thanks,

Josef

Re: Master key removal semantics

[Eric Biggers]

On Wed, Oct 04, 2023 at 12:44:12PM -0400, Josef Bacik wrote:
> Hello,
> 
> While getting the fstests stuff nailed down to deal with btrfs I ran into
> failures with generic/595, specifically the multi-threaded part.
> 
> In one thread we have a loop adding and removing the master key.
> 
> In the other thread we have us trying to echo a character into a flie in the
> encrypted side, and if it succeeds we echo a character into a temporary file,
> and then after the runtime has elapsed we compare these two files to make sure
> they match.
> 
> The problem with this is that btrfs derives the per-extent key from the master
> key at writeout time.  Everybody else has their content key derived at flie open
> time, so they don't need the master key to be around once the file is opened, so
> any writes that occur while that file is held open are allowed to happen.
> 
> Sweet Tea had some changes around soft unloading the master key to handle this
> case.  Basically we allow the master key to stick around by anybody who may need
> it who is currently open, and then any new users get denied.  Once all the
> outstanding open files are closed the master key is unloaded.
> 
> This keeps the semantics of what happens for everybody else.
> 
> What is currently happening with my version of the patchset, which didn't bring
> in those patches, is that you get an ENOKEY at writeout time if you remove the
> key.  The fstest fails because even tho we let you write to the file sometimes,
> it doesn't necessarily mean it'll make it to disk.
> 
> If we want to keep the semantics of "when userspace tells us to throw away the
> master key, we absolutely throw the master key away" then I can just make
> adjustments to the fstests test and call what I have good enough.
> 
> If we want to have the semantics of "when userspace tells us to throw away the
> master key we'll make it unavailable for any new users, but existing open files
> operate as normal" then I can pull in Sweet Tea's soft removal patches and call
> it good enough.
> 
> There's a third option that is a bit of a middle ground with varying degrees of
> raciness.  We could sync the file system before we do the removal just to narrow
> the window where we could successfully write to a file but get an ENOKEY at
> writeout time.  We could freeze the filesystem to make sure it's sync'ed and
> allow any current writers to complete, this would be a stronger version of the
> first option, again just narrows the window.  Neither of these cases help if the
> file is being held open.  If we wanted to fully deal with the file being held
> open case we could set a flag, sync, then remove the key.  Then we add a new
> fscrypt_prep_write() hook that filesystems could optionally use, obviously just
> btrfs for now, that we'd stick in the write path that would check for this flag
> or if the master key had been removed so we can deny dirtying when the key is
> removed.
> 
> At this point I don't have strong opinions, it's easier for me to just leave it
> like it is and change fstests.  Anything else is a change in the semantics of
> how the master key is handled, and that's not really a decision I feel
> comfortable making for everybody.  Once we nail this detail down I can send the
> updated version of all the patches and we can start talking about inclusion.
> Thanks,

There is already a sync just before the master key removal.  See
try_to_lock_evicted_files().  It's racy, of course, as you noticed.

The "soft removal" is what I recommended earlier.  See
https://lore.kernel.org/r/20230703181745.GA1194@sol.localdomain and
https://lore.kernel.org/r/20230704002854.GA860@sol.localdomain.  I think it's
probably still the way to go, but I was a bit confused by the way that Sweet Tea
had implemented it.  Maybe it can be simplified?

I've been pretty busy this week; I'll take a look at your latest patches soon.
I've gone ahead and tweaked your patch "fscrypt: rename fscrypt_info =>
fscrypt_inode_info" a bit and applied it to the fscrypt tree for 6.7 so that we
can get it out of the way; let me know if that's okay.  I've just sent out the
version that I applied.  BTW, I also applied my patchset "fscrypt: add support
for data_unit_size < fs_block_size" recently, so you'll need to rebase onto the
fscrypt tree anyway.  Sorry for the churn, but that feature is apparently
something that people need...

- Eric