Re: Master key removal semantics

[Eric Biggers <ebiggers@kernel.org>]

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