Hi ZhangHui,
On Mon, Mar 17, 2025 at 07:01:01PM +0800, ZhangHui wrote:
> From: zhanghui <zhanghui31@xxxxxxxxxx>
>
> On Android devices, we found that there is a probability that
> the ufs has been shut down but the thread is still deleting the
> key, which will cause the bus error.
>
> We checked the Android reboot process and found that it is indeed
> possible that some threads have not been killed before the device
> shutdown, because the Android reboot process will not wait until
> all threads are killed before continuing to execute.
>
> The call stack is as follows:
>
> __blk_crypto_evict_key+0x148/0x1c4
> blk_crypto_evict_key+0x38/0x9c
> dm_keyslot_evict_callback+0x18/0x2c
> default_key_iterate_devices+0x40/0x50
> dm_keyslot_evict+0xac/0xfc
> __blk_crypto_evict_key+0xf4/0x1c4
> blk_crypto_evict_key+0x38/0x9c
> fscrypt_destroy_inline_crypt_key+0xb8/0x10c
> fscrypt_destroy_prepared_key+0x30/0x48
> fscrypt_put_master_key_activeref+0xc4/0x308
> fscrypt_destroy_keyring+0xb0/0xfc
> generic_shutdown_super+0x60/0x12c
> kill_block_super+0x1c/0x48
> kill_f2fs_super+0xc4/0x1a8
> deactivate_locked_super+0x98/0x144
> deactivate_super+0x78/0x8c
> cleanup_mnt+0x110/0x148
> __cleanup_mnt+0x14/0x20
> task_work_run+0xc4/0xec
> get_signal+0x6c/0x8d8
> do_notify_resume+0x128/0x828
> el0_svc+0x6c/0x70
> el0t_64_sync_handler+0x68/0xbc
> el0t_64_sync+0x1a8/0x1ac
>
> Let's fixed this issue by adding a lock in program_key flow.
>
> Signed-off-by: zhanghui <zhanghui31@xxxxxxxxxx>
> ---
> drivers/ufs/core/ufshcd-crypto.c | 20 +++++++++++++++-----
> 1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/ufs/core/ufshcd-crypto.c b/drivers/ufs/core/ufshcd-crypto.c
> index 694ff7578fc1..f3260a072c0c 100644
> --- a/drivers/ufs/core/ufshcd-crypto.c
> +++ b/drivers/ufs/core/ufshcd-crypto.c
> @@ -5,6 +5,7 @@
>
> #include <ufs/ufshcd.h>
> #include "ufshcd-crypto.h"
> +#include "ufshcd-priv.h"
>
> /* Blk-crypto modes supported by UFS crypto */
> static const struct ufs_crypto_alg_entry {
> @@ -17,12 +18,18 @@ static const struct ufs_crypto_alg_entry {
> },
> };
>
> -static void ufshcd_program_key(struct ufs_hba *hba,
> +static int ufshcd_program_key(struct ufs_hba *hba,
> const union ufs_crypto_cfg_entry *cfg, int slot)
> {
> int i;
> u32 slot_offset = hba->crypto_cfg_register + slot * sizeof(*cfg);
>
> + down(&hba->host_sem);
> + if (!ufshcd_is_user_access_allowed(hba)) {
> + up(&hba->host_sem);
> + return -EBUSY;
> + }
> +
It seems broken that the filesystem doesn't get unmounted until after the UFS is
shut down. It would be helpful to get a clearer picture of exactly why things
are happening in that order.
But disregarding that, it's indeed logical for blk_crypto_evict_key() to return
an error if it cannot fulfill the request.
But I'm wondering if this needs to be solved in the UFS driver itself or whether
the blk-crypto framework should handle this (so that it also gets fixed for
other drivers that may have the same problem). In block/blk-crypto-profile.c,
pm_runtime_get_sync() is already called before ->keyslot_evict. So
->keyslot_evict is supposed to be called only when the device is resumed.
The blk-crypto code (in blk_crypto_hw_enter()) doesn't check the return value of
pm_runtime_get_sync(), though. That seems like a bug. Is it possible this
issue would be fixed if it checked the return value?
Or does the UFS driver still need to check ufshcd_is_user_access_allowed() too?
If that's the case, I'm also wondering whether it's okay to nest host_sem inside
pm_runtime_get_sync(). Elsewhere in the UFS driver they are called in the
opposite order.
- Eric
