On Sun, Oct 15, 2023 at 2:12 AM Eric Biggers <ebigg...@kernel.org> wrote: > > From: Eric Biggers <ebigg...@google.com> > > Master keys can be in one of three states: present, incompletely > removed, and absent (as per FSCRYPT_KEY_STATUS_* used in the UAPI). > Currently, the way that "present" is distinguished from "incompletely > removed" internally is by whether ->mk_secret exists or not. > > With extent-based encryption, it will be necessary to allow per-extent > keys to be derived while the master key is incompletely removed, so that > I/O on open files will reliably continue working after removal of the > key has been initiated. (We could allow I/O to sometimes fail in that > case, but that seems problematic for reasons such as writes getting > silently thrown away and diverging from the existing fscrypt semantics.) > Therefore, when the filesystem is using extent-based encryption, > ->mk_secret can't be wiped when the key becomes incompletely removed. > > As a prerequisite for doing that, this patch makes the "present" state > be tracked using a new field, ->mk_present. No behavior is changed yet. > > The basic idea here is borrowed from Josef Bacik's patch > "fscrypt: use a flag to indicate that the master key is being evicted" > (https://lore.kernel.org/r/e86c16dddc049ff065f877d793ad773e4c6bfad9.1696970227.git.jo...@toxicpanda.com). > I reimplemented it using a "present" bool instead of an "evicted" flag, > fixed a couple bugs, and tried to update everything to be consistent. > > Note: I considered adding a ->mk_status field instead, holding one of > FSCRYPT_KEY_STATUS_*. At first that seemed nice, but it ended up being > more complex (despite simplifying FS_IOC_GET_ENCRYPTION_KEY_STATUS), > since it would have introduced redundancy and had weird locking rules. > > Signed-off-by: Eric Biggers <ebigg...@google.com> > --- > Documentation/filesystems/fscrypt.rst | 4 +- > fs/crypto/fscrypt_private.h | 66 +++++++++++++---------- > fs/crypto/hooks.c | 2 +- > fs/crypto/keyring.c | 78 ++++++++++++++++----------- > fs/crypto/keysetup.c | 13 ++--- > 5 files changed, 95 insertions(+), 68 deletions(-) > > diff --git a/Documentation/filesystems/fscrypt.rst > b/Documentation/filesystems/fscrypt.rst > index 28700fb41a00b..1b84f818e574e 100644 > --- a/Documentation/filesystems/fscrypt.rst > +++ b/Documentation/filesystems/fscrypt.rst > @@ -1127,22 +1127,22 @@ The caller must zero all input fields, then fill in > ``key_spec``: > ``key_spec.type`` to FSCRYPT_KEY_SPEC_TYPE_DESCRIPTOR and fill > in ``key_spec.u.descriptor``. > > - To get the status of a key for v2 encryption policies, set > ``key_spec.type`` to FSCRYPT_KEY_SPEC_TYPE_IDENTIFIER and fill > in ``key_spec.u.identifier``. > > On success, 0 is returned and the kernel fills in the output fields: > > - ``status`` indicates whether the key is absent, present, or > - incompletely removed. Incompletely removed means that the master > - secret has been removed, but some files are still in use; i.e., > + incompletely removed. Incompletely removed means that removal has > + been initiated, but some files are still in use; i.e., > `FS_IOC_REMOVE_ENCRYPTION_KEY`_ returned 0 but set the informational > status flag FSCRYPT_KEY_REMOVAL_STATUS_FLAG_FILES_BUSY. > > - ``status_flags`` can contain the following flags: > > - ``FSCRYPT_KEY_STATUS_FLAG_ADDED_BY_SELF`` indicates that the key > has added by the current user. This is only set for keys > identified by ``identifier`` rather than by ``descriptor``. > > - ``user_count`` specifies the number of users who have added the key. > diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h > index 2fb4ba435d27d..1892356cf924a 100644 > --- a/fs/crypto/fscrypt_private.h > +++ b/fs/crypto/fscrypt_private.h > @@ -468,65 +468,78 @@ struct fscrypt_master_key_secret { > > /* For v1 policy keys: the raw key. Wiped for v2 policy keys. */ > u8 raw[FSCRYPT_MAX_KEY_SIZE]; > > } __randomize_layout; > > /* > * fscrypt_master_key - an in-use master key > * > * This represents a master encryption key which has been added to the > - * filesystem and can be used to "unlock" the encrypted files which were > - * encrypted with it. > + * filesystem. There are three high-level states that a key can be in: > + * > + * FSCRYPT_KEY_STATUS_PRESENT > + * Key is fully usable; it can be used to unlock inodes that are > encrypted > + * with it (this includes being able to create new inodes). ->mk_present > + * indicates whether the key is in this state. ->mk_secret exists, the > key > + * is in the keyring, and ->mk_active_refs > 0 due to ->mk_present. > + * > + * FSCRYPT_KEY_STATUS_INCOMPLETELY_REMOVED > + * Removal of this key has been initiated, but some inodes that were > + * unlocked with it are still in-use. Like ABSENT, ->mk_secret is wiped, > + * and the key can no longer be used to unlock inodes. Unlike ABSENT, > the > + * key is still in the keyring; ->mk_decrypted_inodes is nonempty; and > + * ->mk_active_refs > 0, being equal to the size of > ->mk_decrypted_inodes. > + * > + * This state transitions to ABSENT if ->mk_decrypted_inodes becomes > empty, > + * or to PRESENT if FS_IOC_ADD_ENCRYPTION_KEY is called again for this > key. > + * > + * FSCRYPT_KEY_STATUS_ABSENT > + * Key is fully removed. The key is no longer in the keyring, > + * ->mk_decrypted_inodes is empty, ->mk_active_refs == 0, ->mk_secret is > + * wiped, and the key can no longer be used to unlock inodes. > */ > struct fscrypt_master_key { > > /* > * Link in ->s_master_keys->key_hashtable. > * Only valid if ->mk_active_refs > 0. > */ > struct hlist_node mk_node; > > - /* Semaphore that protects ->mk_secret and ->mk_users */ > + /* Semaphore that protects ->mk_secret, ->mk_users, and ->mk_present > */ > struct rw_semaphore mk_sem; > > /* > * Active and structural reference counts. An active ref guarantees > * that the struct continues to exist, continues to be in the keyring > * ->s_master_keys, and that any embedded subkeys (e.g. > * ->mk_direct_keys) that have been prepared continue to exist. > * A structural ref only guarantees that the struct continues to > exist. > * > - * There is one active ref associated with ->mk_secret being present, > - * and one active ref for each inode in ->mk_decrypted_inodes. > + * There is one active ref associated with ->mk_present being true, > and > + * one active ref for each inode in ->mk_decrypted_inodes. > * > * There is one structural ref associated with the active refcount > being > * nonzero. Finding a key in the keyring also takes a structural ref, > * which is then held temporarily while the key is operated on. > */ > refcount_t mk_active_refs; > refcount_t mk_struct_refs; > > struct rcu_head mk_rcu_head; > > /* > - * The secret key material. After FS_IOC_REMOVE_ENCRYPTION_KEY is > - * executed, this is wiped and no new inodes can be unlocked with this > - * key; however, there may still be inodes in ->mk_decrypted_inodes > - * which could not be evicted. As long as some inodes still remain, > - * FS_IOC_REMOVE_ENCRYPTION_KEY can be retried, or > - * FS_IOC_ADD_ENCRYPTION_KEY can add the secret again. > + * The secret key material. Wiped as soon as it is no longer needed; > + * for details, see the fscrypt_master_key struct comment. > * > - * While ->mk_secret is present, one ref in ->mk_active_refs is held. > - * > - * Locking: protected by ->mk_sem. The manipulation of > ->mk_active_refs > - * associated with this field is protected by ->mk_sem as > well. > + * Locking: protected by ->mk_sem. > */ > struct fscrypt_master_key_secret mk_secret; > > /* > * For v1 policy keys: an arbitrary key descriptor which was assigned > by > * userspace (->descriptor). > * > * For v2 policy keys: a cryptographic hash of this key > (->identifier). > */ > struct fscrypt_key_specifier mk_spec; > @@ -535,21 +548,21 @@ struct fscrypt_master_key { > * Keyring which contains a key of type 'key_type_fscrypt_user' for > each > * user who has added this key. Normally each key will be added by > just > * one user, but it's possible that multiple users share a key, and in > * that case we need to keep track of those users so that one user > can't > * remove the key before the others want it removed too. > * > * This is NULL for v1 policy keys; those can only be added by root. > * > * Locking: protected by ->mk_sem. (We don't just rely on the > keyrings > * subsystem semaphore ->mk_users->sem, as we need support for atomic > - * search+insert along with proper synchronization with ->mk_secret.) > + * search+insert along with proper synchronization with other fields.) > */ > struct key *mk_users; > > /* > * List of inodes that were unlocked using this key. This allows the > * inodes to be evicted efficiently if the key is removed. > */ > struct list_head mk_decrypted_inodes; > spinlock_t mk_decrypted_inodes_lock; > > @@ -558,34 +571,31 @@ struct fscrypt_master_key { > * that use them. Allocated and derived on-demand. > */ > struct fscrypt_prepared_key mk_direct_keys[FSCRYPT_MODE_MAX + 1]; > struct fscrypt_prepared_key mk_iv_ino_lblk_64_keys[FSCRYPT_MODE_MAX + > 1]; > struct fscrypt_prepared_key mk_iv_ino_lblk_32_keys[FSCRYPT_MODE_MAX + > 1]; > > /* Hash key for inode numbers. Initialized only when needed. */ > siphash_key_t mk_ino_hash_key; > bool mk_ino_hash_key_initialized; > > -} __randomize_layout; > - > -static inline bool > -is_master_key_secret_present(const struct fscrypt_master_key_secret *secret) > -{ > /* > - * The READ_ONCE() is only necessary for fscrypt_drop_inode(). > - * fscrypt_drop_inode() runs in atomic context, so it can't take the > key > - * semaphore and thus 'secret' can change concurrently which would be > a > - * data race. But fscrypt_drop_inode() only need to know whether the > - * secret *was* present at the time of check, so READ_ONCE() suffices. > + * Whether this key is in the "present" state, i.e. fully usable. For > + * details, see the fscrypt_master_key struct comment. > + * > + * Locking: protected by ->mk_sem, but can be read locklessly using > + * READ_ONCE(). Writers must use WRITE_ONCE() when concurrent readers > + * are possible. > */ > - return READ_ONCE(secret->size) != 0; > -} > + bool mk_present; > + > +} __randomize_layout; > > static inline const char *master_key_spec_type( > const struct fscrypt_key_specifier *spec) > { > switch (spec->type) { > case FSCRYPT_KEY_SPEC_TYPE_DESCRIPTOR: > return "descriptor"; > case FSCRYPT_KEY_SPEC_TYPE_IDENTIFIER: > return "identifier"; > } > diff --git a/fs/crypto/hooks.c b/fs/crypto/hooks.c > index 85d2975b69b78..52504dd478d31 100644 > --- a/fs/crypto/hooks.c > +++ b/fs/crypto/hooks.c > @@ -180,21 +180,21 @@ int fscrypt_prepare_setflags(struct inode *inode, > */ > if (IS_ENCRYPTED(inode) && (flags & ~oldflags & FS_CASEFOLD_FL)) { > err = fscrypt_require_key(inode); > if (err) > return err; > ci = inode->i_crypt_info; > if (ci->ci_policy.version != FSCRYPT_POLICY_V2) > return -EINVAL; > mk = ci->ci_master_key; > down_read(&mk->mk_sem); > - if (is_master_key_secret_present(&mk->mk_secret)) > + if (mk->mk_present) > err = fscrypt_derive_dirhash_key(ci, mk); > else > err = -ENOKEY; > up_read(&mk->mk_sem); > return err; > } > return 0; > } > > /** > diff --git a/fs/crypto/keyring.c b/fs/crypto/keyring.c > index a51fa6a33de10..f34a9b0b9e922 100644 > --- a/fs/crypto/keyring.c > +++ b/fs/crypto/keyring.c > @@ -92,42 +92,54 @@ void fscrypt_put_master_key_activeref(struct super_block > *sb, > * destroying any subkeys embedded in it. > */ > > if (WARN_ON_ONCE(!sb->s_master_keys)) > return; > spin_lock(&sb->s_master_keys->lock); > hlist_del_rcu(&mk->mk_node); > spin_unlock(&sb->s_master_keys->lock); > > /* > - * ->mk_active_refs == 0 implies that ->mk_secret is not present and > - * that ->mk_decrypted_inodes is empty. > + * ->mk_active_refs == 0 implies that ->mk_present is false and > + * ->mk_decrypted_inodes is empty. > */ > - WARN_ON_ONCE(is_master_key_secret_present(&mk->mk_secret)); > + WARN_ON_ONCE(mk->mk_present); > WARN_ON_ONCE(!list_empty(&mk->mk_decrypted_inodes)); > > for (i = 0; i <= FSCRYPT_MODE_MAX; i++) { > fscrypt_destroy_prepared_key( > sb, &mk->mk_direct_keys[i]); > fscrypt_destroy_prepared_key( > sb, &mk->mk_iv_ino_lblk_64_keys[i]); > fscrypt_destroy_prepared_key( > sb, &mk->mk_iv_ino_lblk_32_keys[i]); > } > memzero_explicit(&mk->mk_ino_hash_key, > sizeof(mk->mk_ino_hash_key)); > mk->mk_ino_hash_key_initialized = false; > > /* Drop the structural ref associated with the active refs. */ > fscrypt_put_master_key(mk); > } > > +/* > + * This transitions the key state from present to incompletely removed, and > then > + * potentially to absent (depending on whether inodes remain). > + */ > +static void fscrypt_initiate_key_removal(struct super_block *sb, > + struct fscrypt_master_key *mk) > +{ > + WRITE_ONCE(mk->mk_present, false); > + wipe_master_key_secret(&mk->mk_secret); > + fscrypt_put_master_key_activeref(sb, mk); > +} > + > static inline bool valid_key_spec(const struct fscrypt_key_specifier *spec) > { > if (spec->__reserved) > return false; > return master_key_spec_len(spec) != 0; > } > > static int fscrypt_user_key_instantiate(struct key *key, > struct key_preparsed_payload *prep) > { > @@ -227,28 +239,27 @@ void fscrypt_destroy_keyring(struct super_block *sb) > struct hlist_head *bucket = &keyring->key_hashtable[i]; > struct fscrypt_master_key *mk; > struct hlist_node *tmp; > > hlist_for_each_entry_safe(mk, tmp, bucket, mk_node) { > /* > * Since all potentially-encrypted inodes were already > * evicted, every key remaining in the keyring should > * have an empty inode list, and should only still be > in > * the keyring due to the single active ref associated > - * with ->mk_secret. There should be no structural > refs > - * beyond the one associated with the active ref. > + * with ->mk_present. There should be no structural > + * refs beyond the one associated with the active ref. > */ > WARN_ON_ONCE(refcount_read(&mk->mk_active_refs) != 1); > WARN_ON_ONCE(refcount_read(&mk->mk_struct_refs) != 1); > - > WARN_ON_ONCE(!is_master_key_secret_present(&mk->mk_secret)); > - wipe_master_key_secret(&mk->mk_secret); > - fscrypt_put_master_key_activeref(sb, mk); > + WARN_ON_ONCE(!mk->mk_present); > + fscrypt_initiate_key_removal(sb, mk); > } > } > kfree_sensitive(keyring); > sb->s_master_keys = NULL; > } > > static struct hlist_head * > fscrypt_mk_hash_bucket(struct fscrypt_keyring *keyring, > const struct fscrypt_key_specifier *mk_spec) > { > @@ -432,21 +443,22 @@ static int add_new_master_key(struct super_block *sb, > if (mk_spec->type == FSCRYPT_KEY_SPEC_TYPE_IDENTIFIER) { > err = allocate_master_key_users_keyring(mk); > if (err) > goto out_put; > err = add_master_key_user(mk); > if (err) > goto out_put; > } > > move_master_key_secret(&mk->mk_secret, secret); > - refcount_set(&mk->mk_active_refs, 1); /* ->mk_secret is present */ > + mk->mk_present = true; > + refcount_set(&mk->mk_active_refs, 1); /* ->mk_present is true */ > > spin_lock(&keyring->lock); > hlist_add_head_rcu(&mk->mk_node, > fscrypt_mk_hash_bucket(keyring, mk_spec)); > spin_unlock(&keyring->lock); > return 0; > > out_put: > fscrypt_put_master_key(mk); > return err; > @@ -471,25 +483,32 @@ static int add_existing_master_key(struct > fscrypt_master_key *mk, > if (IS_ERR(mk_user)) > return PTR_ERR(mk_user); > key_put(mk_user); > return 0; > } > err = add_master_key_user(mk); > if (err) > return err; > } > > - /* Re-add the secret if needed. */ > - if (!is_master_key_secret_present(&mk->mk_secret)) { > - if (!refcount_inc_not_zero(&mk->mk_active_refs)) > + /* If the key is incompletely removed, make it present again. */ > + if (!mk->mk_present) { > + if (!refcount_inc_not_zero(&mk->mk_active_refs)) { > + /* > + * Raced with the last active ref being dropped, so > the > + * key has become, or is about to become, "absent". > + * Therefore, we need to allocate a new key struct. > + */ > return KEY_DEAD; > + } > move_master_key_secret(&mk->mk_secret, secret); > + WRITE_ONCE(mk->mk_present, true); > } > > return 0; > } > > static int do_add_master_key(struct super_block *sb, > struct fscrypt_master_key_secret *secret, > const struct fscrypt_key_specifier *mk_spec) > { > static DEFINE_MUTEX(fscrypt_add_key_mutex); > @@ -499,22 +518,22 @@ static int do_add_master_key(struct super_block *sb, > mutex_lock(&fscrypt_add_key_mutex); /* serialize find + link */ > > mk = fscrypt_find_master_key(sb, mk_spec); > if (!mk) { > /* Didn't find the key in ->s_master_keys. Add it. */ > err = allocate_filesystem_keyring(sb); > if (!err) > err = add_new_master_key(sb, secret, mk_spec); > } else { > /* > - * Found the key in ->s_master_keys. Re-add the secret if > - * needed, and add the user to ->mk_users if needed. > + * Found the key in ->s_master_keys. Add the user to > ->mk_users > + * if needed, and make the key "present" again if possible. > */ > down_write(&mk->mk_sem); > err = add_existing_master_key(mk, secret); > up_write(&mk->mk_sem); > if (err == KEY_DEAD) { > /* > * We found a key struct, but it's already been fully > * removed. Ignore the old struct and add a new one. > * fscrypt_add_key_mutex means we don't need to worry > * about concurrent adds. > @@ -982,23 +1001,22 @@ static int try_to_lock_encrypted_files(struct > super_block *sb, > * claim to the key, then removes the key itself if no other users have > claims. > * FS_IOC_REMOVE_ENCRYPTION_KEY_ALL_USERS (all_users=true) always removes the > * key itself. > * > * To "remove the key itself", first we wipe the actual master key secret, so > * that no more inodes can be unlocked with it. Then we try to evict all > cached > * inodes that had been unlocked with the key. > * > * If all inodes were evicted, then we unlink the fscrypt_master_key from the > * keyring. Otherwise it remains in the keyring in the "incompletely > removed" > - * state (without the actual secret key) where it tracks the list of > remaining > - * inodes. Userspace can execute the ioctl again later to retry eviction, or > - * alternatively can re-add the secret key again. > + * state where it tracks the list of remaining inodes. Userspace can execute > + * the ioctl again later to retry eviction, or alternatively can re-add the > key. > * > * For more details, see the "Removing keys" section of > * Documentation/filesystems/fscrypt.rst. > */ > static int do_remove_key(struct file *filp, void __user *_uarg, bool > all_users) > { > struct super_block *sb = file_inode(filp)->i_sb; > struct fscrypt_remove_key_arg __user *uarg = _uarg; > struct fscrypt_remove_key_arg arg; > struct fscrypt_master_key *mk; > @@ -1046,44 +1064,43 @@ static int do_remove_key(struct file *filp, void > __user *_uarg, bool all_users) > * can't remove the key itself. > */ > status_flags |= > FSCRYPT_KEY_REMOVAL_STATUS_FLAG_OTHER_USERS; > err = 0; > up_write(&mk->mk_sem); > goto out_put_key; > } > } > > - /* No user claims remaining. Go ahead and wipe the secret. */ > + /* No user claims remaining. Initiate removal of the key. */ > err = -ENOKEY; > - if (is_master_key_secret_present(&mk->mk_secret)) { > - wipe_master_key_secret(&mk->mk_secret); > - fscrypt_put_master_key_activeref(sb, mk); > + if (mk->mk_present) { > + fscrypt_initiate_key_removal(sb, mk); > err = 0; > } > inodes_remain = refcount_read(&mk->mk_active_refs) > 0; > up_write(&mk->mk_sem); > > if (inodes_remain) { > /* Some inodes still reference this key; try to evict them. */ > err = try_to_lock_encrypted_files(sb, mk); > if (err == -EBUSY) { > status_flags |= > FSCRYPT_KEY_REMOVAL_STATUS_FLAG_FILES_BUSY; > err = 0; > } > } > /* > * We return 0 if we successfully did something: removed a claim to > the > - * key, wiped the secret, or tried locking the files again. Users > need > - * to check the informational status flags if they care whether the > key > - * has been fully removed including all files locked. > + * key, initiated removal of the key, or tried locking the files > again. > + * Users need to check the informational status flags if they care > + * whether the key has been fully removed including all files locked. > */ > out_put_key: > fscrypt_put_master_key(mk); > if (err == 0) > err = put_user(status_flags, &uarg->removal_status_flags); > return err; > } > > int fscrypt_ioctl_remove_key(struct file *filp, void __user *uarg) > { > @@ -1096,26 +1113,25 @@ int fscrypt_ioctl_remove_key_all_users(struct file > *filp, void __user *uarg) > if (!capable(CAP_SYS_ADMIN)) > return -EACCES; > return do_remove_key(filp, uarg, true); > } > EXPORT_SYMBOL_GPL(fscrypt_ioctl_remove_key_all_users); > > /* > * Retrieve the status of an fscrypt master encryption key. > * > * We set ->status to indicate whether the key is absent, present, or > - * incompletely removed. "Incompletely removed" means that the master key > - * secret has been removed, but some files which had been unlocked with it > are > - * still in use. This field allows applications to easily determine the > state > - * of an encrypted directory without using a hack such as trying to open a > - * regular file in it (which can confuse the "incompletely removed" state > with > - * absent or present). > + * incompletely removed. (For an explanation of what these statuses mean and > + * how they are represented internally, see struct fscrypt_master_key.) This > + * field allows applications to easily determine the status of an encrypted > + * directory without using a hack such as trying to open a regular file in it > + * (which can confuse the "incompletely removed" status with absent or > present). > * > * In addition, for v2 policy keys we allow applications to determine, via > * ->status_flags and ->user_count, whether the key has been added by the > * current user, by other users, or by both. Most applications should not > need > * this, since ordinarily only one user should know a given key. However, > if a > * secret key is shared by multiple users, applications may wish to add an > * already-present key to prevent other users from removing it. This ioctl > can > * be used to check whether that really is the case before the work is done > to > * add the key --- which might e.g. require prompting the user for a > passphrase. > * > @@ -1143,21 +1159,21 @@ int fscrypt_ioctl_get_key_status(struct file *filp, > void __user *uarg) > memset(arg.__out_reserved, 0, sizeof(arg.__out_reserved)); > > mk = fscrypt_find_master_key(sb, &arg.key_spec); > if (!mk) { > arg.status = FSCRYPT_KEY_STATUS_ABSENT; > err = 0; > goto out; > } > down_read(&mk->mk_sem); > > - if (!is_master_key_secret_present(&mk->mk_secret)) { > + if (!mk->mk_present) { > arg.status = refcount_read(&mk->mk_active_refs) > 0 ? > FSCRYPT_KEY_STATUS_INCOMPLETELY_REMOVED : > FSCRYPT_KEY_STATUS_ABSENT /* raced with full removal > */; > err = 0; > goto out_release_key; > } > > arg.status = FSCRYPT_KEY_STATUS_PRESENT; > if (mk->mk_users) { > struct key *mk_user; > diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c > index 094d1b7a1ae61..d71f7c799e79e 100644 > --- a/fs/crypto/keysetup.c > +++ b/fs/crypto/keysetup.c > @@ -479,22 +479,22 @@ static int setup_file_encryption_key(struct > fscrypt_inode_info *ci, > /* > * As a legacy fallback for v1 policies, search for the key in > * the current task's subscribed keyrings too. Don't move > this > * to before the search of ->s_master_keys, since users > * shouldn't be able to override filesystem-level keys. > */ > return fscrypt_setup_v1_file_key_via_subscribed_keyrings(ci); > } > down_read(&mk->mk_sem); > > - /* Has the secret been removed (via FS_IOC_REMOVE_ENCRYPTION_KEY)? */ > - if (!is_master_key_secret_present(&mk->mk_secret)) { > + if (!mk->mk_present) { > + /* FS_IOC_REMOVE_ENCRYPTION_KEY has been executed on this key > */ > err = -ENOKEY; > goto out_release_key; > } > > if (!fscrypt_valid_master_key_size(mk, ci)) { > err = -ENOKEY; > goto out_release_key; > } > > switch (ci->ci_policy.version) { > @@ -532,22 +532,22 @@ static void put_crypt_info(struct fscrypt_inode_info > *ci) > fscrypt_put_direct_key(ci->ci_direct_key); > else if (ci->ci_owns_key) > fscrypt_destroy_prepared_key(ci->ci_inode->i_sb, > &ci->ci_enc_key); > > mk = ci->ci_master_key; > if (mk) { > /* > * Remove this inode from the list of inodes that were > unlocked > * with the master key. In addition, if we're removing the > last > - * inode from a master key struct that already had its secret > - * removed, then complete the full removal of the struct. > + * inode from an incompletely removed key, then complete the > + * full removal of the key. > */ > spin_lock(&mk->mk_decrypted_inodes_lock); > list_del(&ci->ci_master_key_link); > spin_unlock(&mk->mk_decrypted_inodes_lock); > fscrypt_put_master_key_activeref(ci->ci_inode->i_sb, mk); > } > memzero_explicit(ci, sizeof(*ci)); > kmem_cache_free(fscrypt_inode_info_cachep, ci); > } > > @@ -794,20 +794,21 @@ int fscrypt_drop_inode(struct inode *inode) > /* > * With proper, non-racy use of FS_IOC_REMOVE_ENCRYPTION_KEY, all > inodes > * protected by the key were cleaned by sync_filesystem(). But if > * userspace is still using the files, inodes can be dirtied between > * then and now. We mustn't lose any writes, so skip dirty inodes > here. > */ > if (inode->i_state & I_DIRTY_ALL) > return 0; > > /* > - * Note: since we aren't holding the key semaphore, the result here > can > + * We can't take ->mk_sem here, since this runs in atomic context. > + * Therefore, ->mk_present can change concurrently, and our result may > * immediately become outdated. But there's no correctness problem > with > * unnecessarily evicting. Nor is there a correctness problem with > not > * evicting while iput() is racing with the key being removed, since > * then the thread removing the key will either evict the inode itself > * or will correctly detect that it wasn't evicted due to the race. > */ > - return !is_master_key_secret_present(&ci->ci_master_key->mk_secret); > + return !READ_ONCE(ci->ci_master_key->mk_present); > } > EXPORT_SYMBOL_GPL(fscrypt_drop_inode); > > base-commit: 3e7807d5a7d770c59837026e9967fe99ad043174 > -- > 2.42.0 >
This patch looks reasonable to me. The updated comment details help a lot. :) Reviewed-by: Neal Gompa <n...@gompa.dev> -- 真実はいつも一つ！/ Always, there's only one truth!
