All of lore.kernel.org
 help / color / mirror / Atom feed
From: Allen Pais <apais@linux.microsoft.com>
To: Jens Wiklander <jens.wiklander@linaro.org>
Cc: Tyler Hicks <tyhicks@linux.microsoft.com>,
	zajec5@gmail.com, Allen Pais <allen.lkml@gmail.com>,
	bcm-kernel-feedback-list@broadcom.com,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	OP-TEE TrustedFirmware <op-tee@lists.trustedfirmware.org>
Subject: Re: [PATCH] optee: Disable shm cache when booting the crash kernel
Date: Fri, 7 May 2021 15:02:58 +0530	[thread overview]
Message-ID: <14C574FD-AF31-4D10-9685-E59FDD248C2C@linux.microsoft.com> (raw)
In-Reply-To: <CAHUa44FHo2_EUzFzHnakkm3o7H-Nn+k4hgqT2WNFezZO6D8mxA@mail.gmail.com>

>> 
>> 
>>> On 07-May-2021, at 9:28 AM, Tyler Hicks <tyhicks@linux.microsoft.com> wrote:
>>> 
>>> The .shutdown hook is not called after a kernel crash when a kdump
>>> kernel is pre-loaded. A kexec into the kdump kernel takes place as
>>> quickly as possible without allowing drivers to clean up.
>>> 
>>> That means that the OP-TEE shared memory cache, which was initialized by
>>> the kernel that crashed, is still in place when the kdump kernel is
>>> booted. As the kdump kernel is shutdown, the .shutdown hook is called,
>>> which calls optee_disable_shm_cache(), and OP-TEE's
>>> OPTEE_SMC_DISABLE_SHM_CACHE API returns virtual addresses that are not
>>> mapped for the kdump kernel since the cache was set up by the previous
>>> kernel. Trying to dereference the tee_shm pointer or otherwise translate
>>> the address results in a fault that cannot be handled:
>>> 
>>> Unable to handle kernel paging request at virtual address ffff4317b9c09744
>>> Mem abort info:
>>>  ESR = 0x96000004
>>>  EC = 0x25: DABT (current EL), IL = 32 bits
>>>  SET = 0, FnV = 0
>>>  EA = 0, S1PTW = 0
>>> Data abort info:
>>>  ISV = 0, ISS = 0x00000004
>>>  CM = 0, WnR = 0
>>> swapper pgtable: 4k pages, 48-bit VAs, pgdp=0000000970b1e000
>>> [ffff4317b9c09744] pgd=0000000000000000, p4d=0000000000000000
>>> Internal error: Oops: 96000004 [#1] SMP
>>> Modules linked in: bnxt_en pcie_iproc_platform pcie_iproc diagbe(O)
>>> CPU: 4 PID: 1 Comm: systemd-shutdow Tainted: G           O      5.10.19.8 #1
>>> Hardware name: Redacted (DT)
>>> pstate: 60400005 (nZCv daif +PAN -UAO -TCO BTYPE=--)
>>> pc : tee_shm_free (/usr/src/kernel/drivers/tee/tee_shm.c:363)
>>> lr : optee_disable_shm_cache (/usr/src/kernel/drivers/tee/optee/call.c:441)
>>> sp : ffff80001005bb70
>>> x29: ffff80001005bb70 x28: ffff608e74648e00
>>> x27: ffff80001005bb98 x26: dead000000000100
>>> x25: ffff80001005bbb8 x24: aaaaaaaaaaaaaaaa
>>> x23: ffff608e74cf8818 x22: ffff608e738be600
>>> x21: ffff80001005bbc8 x20: ffff608e738be638
>>> x19: ffff4317b9c09700 x18: ffffffffffffffff
>>> x17: 0000000000000041 x16: ffffba61b5171764
>>> x15: 0000000000000004 x14: 0000000000000fff
>>> x13: ffffba61b5c9dfc8 x12: 0000000000000003
>>> x11: 0000000000000000 x10: 0000000000000000
>>> x9 : ffffba61b5413824 x8 : 00000000ffff4317
>>> x7 : 0000000000000000 x6 : 0000000000000000
>>> x5 : 0000000000000000 x4 : 0000000000000000
>>> x3 : 0000000000000000 x2 : ffff4317b9c09700
>>> x1 : 00000000ffff4317 x0 : ffff4317b9c09700
>>> Call trace:
>>> tee_shm_free (/usr/src/kernel/drivers/tee/tee_shm.c:363)
>>> optee_disable_shm_cache (/usr/src/kernel/drivers/tee/optee/call.c:441)
>>> optee_shutdown (/usr/src/kernel/drivers/tee/optee/core.c:636)
>>> platform_drv_shutdown (/usr/src/kernel/drivers/base/platform.c:800)
>>> device_shutdown (/usr/src/kernel/include/linux/device.h:758 /usr/src/kernel/drivers/base/core.c:4078)
>>> kernel_restart (/usr/src/kernel/kernel/reboot.c:221 /usr/src/kernel/kernel/reboot.c:248)
>>> __arm64_sys_reboot (/usr/src/kernel/kernel/reboot.c:349 /usr/src/kernel/kernel/reboot.c:312 /usr/src/kernel/kernel/reboot.c:312)
>>> do_el0_svc (/usr/src/kernel/arch/arm64/kernel/syscall.c:56 /usr/src/kernel/arch/arm64/kernel/syscall.c:158 /usr/src/kernel/arch/arm64/kernel/syscall.c:197)
>>> el0_svc (/usr/src/kernel/arch/arm64/kernel/entry-common.c:368)
>>> el0_sync_handler (/usr/src/kernel/arch/arm64/kernel/entry-common.c:428)
>>> el0_sync (/usr/src/kernel/arch/arm64/kernel/entry.S:671)
>>> Code: aa0003f3 b5000060 12800003 14000002 (b9404663)
>>> 
>>> When booting the kdump kernel, drain the shared memory cache while being
>>> careful to not translate the addresses returned from
>>> OPTEE_SMC_DISABLE_SHM_CACHE. Once the invalid cache objects are drained
>>> and the cache is disabled, proceed with re-enabling the cache so that we
>>> aren't dealing with invalid addresses while shutting down the kdump
>>> kernel.
>>> 
>>> Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
>>> ---
>>> 
>>> This patch fixes a crash introduced by "optee: fix tee out of memory
>>> failure seen during kexec reboot"[1]. However, I don't think that the
>>> original two patch series[2] plus this patch is the full solution to
>>> properly handling OP-TEE shared memory across kexec.
>>> 
>>> While testing this fix, I did about 10 kexec reboots and then triggered
>>> a kernel crash by writing 'c' to /proc/sysrq-trigger. The kdump kernel
>>> became unresponsive during boot while steadily streaming the following
>>> errors to the serial console:
>>> 
>>> arm-smmu 64000000.mmu: Blocked unknown Stream ID 0x2000; boot with "arm-smmu.disable_bypass=0" to allow, but this may have security implications
>>> arm-smmu 64000000.mmu:     GFSR 0x00000002, GFSYNR0 0x00000002, GFSYNR1 0x00002000, GFSYNR2 0x00000000
>>> 
>>> I suspect that this is related to the problems of OP-TEE shared memory
>>> handling across kexec. My current hunch is that while we've disabled the
>>> shared memory cache with this patch, we haven't unregistered all of the
>>> addresses that the previous kernel (which crashed) had registered with
>>> OP-TEE and that perhaps OP-TEE OS is still trying to make use those
>>> addresses?
>>> 
>>> I'm still pretty early in investigating that assumption and
>>> I'm learning about OP-TEE as I go but I wanted to get this initial
>>> fix-of-the-fix out so that it was clear that the v2 of the series[2] is
>>> not complete.
>>> 
>>> [1] https://lore.kernel.org/lkml/20210225090610.242623-2-allen.lkml@gmail.com/
>>> [2] https://lore.kernel.org/lkml/20210225090610.242623-1-allen.lkml@gmail.com/#t
>>> 
>>> drivers/tee/optee/call.c          | 11 ++++++++++-
>>> drivers/tee/optee/core.c          | 13 +++++++++++--
>>> drivers/tee/optee/optee_private.h |  2 +-
>>> 3 files changed, 22 insertions(+), 4 deletions(-)
>>> 
>>> diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c
>>> index 6132cc8d014c..799e84bec63d 100644
>>> --- a/drivers/tee/optee/call.c
>>> +++ b/drivers/tee/optee/call.c
>>> @@ -417,8 +417,10 @@ void optee_enable_shm_cache(struct optee *optee)
>>> * optee_disable_shm_cache() - Disables caching of some shared memory allocation
>>> *                          in OP-TEE
>>> * @optee:    main service struct
>>> + * @is_mapped:       true if the cached shared memory addresses were mapped by this
>>> + *           kernel, are safe to dereference, and should be freed
>>> */
>>> -void optee_disable_shm_cache(struct optee *optee)
>>> +void optee_disable_shm_cache(struct optee *optee, bool is_mapped)
>>> {
>>>      struct optee_call_waiter w;
>>> 
>>> @@ -437,6 +439,13 @@ void optee_disable_shm_cache(struct optee *optee)
>>>              if (res.result.status == OPTEE_SMC_RETURN_OK) {
>>>                      struct tee_shm *shm;
>>> 
>> 
>> Thanks Tyler.
>> From what I understand from my email exchange with Jens, I don’t
>> Think we want to touch optee_disable_shm_cache(), I could be wrong too,
>> @Jens, comments?
> 
> Changing optee_disable_shm_cache() is fine. Bear in mind that there
> are other times where we can't recover from a kernel crash. For
> instance if a thread is executing in OP-TEE in secure world.

I agree. My bad, I meant, “we don’t want to touch optee_disable_shm_cache()”.
And precisely for the reason you have mentioned above.

Thanks.

WARNING: multiple messages have this Message-ID (diff)
From: Allen Pais <apais@linux.microsoft.com>
To: Jens Wiklander <jens.wiklander@linaro.org>
Cc: Tyler Hicks <tyhicks@linux.microsoft.com>,
	zajec5@gmail.com, Allen Pais <allen.lkml@gmail.com>,
	bcm-kernel-feedback-list@broadcom.com,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	OP-TEE TrustedFirmware <op-tee@lists.trustedfirmware.org>
Subject: Re: [PATCH] optee: Disable shm cache when booting the crash kernel
Date: Fri, 7 May 2021 15:02:58 +0530	[thread overview]
Message-ID: <14C574FD-AF31-4D10-9685-E59FDD248C2C@linux.microsoft.com> (raw)
In-Reply-To: <CAHUa44FHo2_EUzFzHnakkm3o7H-Nn+k4hgqT2WNFezZO6D8mxA@mail.gmail.com>

>> 
>> 
>>> On 07-May-2021, at 9:28 AM, Tyler Hicks <tyhicks@linux.microsoft.com> wrote:
>>> 
>>> The .shutdown hook is not called after a kernel crash when a kdump
>>> kernel is pre-loaded. A kexec into the kdump kernel takes place as
>>> quickly as possible without allowing drivers to clean up.
>>> 
>>> That means that the OP-TEE shared memory cache, which was initialized by
>>> the kernel that crashed, is still in place when the kdump kernel is
>>> booted. As the kdump kernel is shutdown, the .shutdown hook is called,
>>> which calls optee_disable_shm_cache(), and OP-TEE's
>>> OPTEE_SMC_DISABLE_SHM_CACHE API returns virtual addresses that are not
>>> mapped for the kdump kernel since the cache was set up by the previous
>>> kernel. Trying to dereference the tee_shm pointer or otherwise translate
>>> the address results in a fault that cannot be handled:
>>> 
>>> Unable to handle kernel paging request at virtual address ffff4317b9c09744
>>> Mem abort info:
>>>  ESR = 0x96000004
>>>  EC = 0x25: DABT (current EL), IL = 32 bits
>>>  SET = 0, FnV = 0
>>>  EA = 0, S1PTW = 0
>>> Data abort info:
>>>  ISV = 0, ISS = 0x00000004
>>>  CM = 0, WnR = 0
>>> swapper pgtable: 4k pages, 48-bit VAs, pgdp=0000000970b1e000
>>> [ffff4317b9c09744] pgd=0000000000000000, p4d=0000000000000000
>>> Internal error: Oops: 96000004 [#1] SMP
>>> Modules linked in: bnxt_en pcie_iproc_platform pcie_iproc diagbe(O)
>>> CPU: 4 PID: 1 Comm: systemd-shutdow Tainted: G           O      5.10.19.8 #1
>>> Hardware name: Redacted (DT)
>>> pstate: 60400005 (nZCv daif +PAN -UAO -TCO BTYPE=--)
>>> pc : tee_shm_free (/usr/src/kernel/drivers/tee/tee_shm.c:363)
>>> lr : optee_disable_shm_cache (/usr/src/kernel/drivers/tee/optee/call.c:441)
>>> sp : ffff80001005bb70
>>> x29: ffff80001005bb70 x28: ffff608e74648e00
>>> x27: ffff80001005bb98 x26: dead000000000100
>>> x25: ffff80001005bbb8 x24: aaaaaaaaaaaaaaaa
>>> x23: ffff608e74cf8818 x22: ffff608e738be600
>>> x21: ffff80001005bbc8 x20: ffff608e738be638
>>> x19: ffff4317b9c09700 x18: ffffffffffffffff
>>> x17: 0000000000000041 x16: ffffba61b5171764
>>> x15: 0000000000000004 x14: 0000000000000fff
>>> x13: ffffba61b5c9dfc8 x12: 0000000000000003
>>> x11: 0000000000000000 x10: 0000000000000000
>>> x9 : ffffba61b5413824 x8 : 00000000ffff4317
>>> x7 : 0000000000000000 x6 : 0000000000000000
>>> x5 : 0000000000000000 x4 : 0000000000000000
>>> x3 : 0000000000000000 x2 : ffff4317b9c09700
>>> x1 : 00000000ffff4317 x0 : ffff4317b9c09700
>>> Call trace:
>>> tee_shm_free (/usr/src/kernel/drivers/tee/tee_shm.c:363)
>>> optee_disable_shm_cache (/usr/src/kernel/drivers/tee/optee/call.c:441)
>>> optee_shutdown (/usr/src/kernel/drivers/tee/optee/core.c:636)
>>> platform_drv_shutdown (/usr/src/kernel/drivers/base/platform.c:800)
>>> device_shutdown (/usr/src/kernel/include/linux/device.h:758 /usr/src/kernel/drivers/base/core.c:4078)
>>> kernel_restart (/usr/src/kernel/kernel/reboot.c:221 /usr/src/kernel/kernel/reboot.c:248)
>>> __arm64_sys_reboot (/usr/src/kernel/kernel/reboot.c:349 /usr/src/kernel/kernel/reboot.c:312 /usr/src/kernel/kernel/reboot.c:312)
>>> do_el0_svc (/usr/src/kernel/arch/arm64/kernel/syscall.c:56 /usr/src/kernel/arch/arm64/kernel/syscall.c:158 /usr/src/kernel/arch/arm64/kernel/syscall.c:197)
>>> el0_svc (/usr/src/kernel/arch/arm64/kernel/entry-common.c:368)
>>> el0_sync_handler (/usr/src/kernel/arch/arm64/kernel/entry-common.c:428)
>>> el0_sync (/usr/src/kernel/arch/arm64/kernel/entry.S:671)
>>> Code: aa0003f3 b5000060 12800003 14000002 (b9404663)
>>> 
>>> When booting the kdump kernel, drain the shared memory cache while being
>>> careful to not translate the addresses returned from
>>> OPTEE_SMC_DISABLE_SHM_CACHE. Once the invalid cache objects are drained
>>> and the cache is disabled, proceed with re-enabling the cache so that we
>>> aren't dealing with invalid addresses while shutting down the kdump
>>> kernel.
>>> 
>>> Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
>>> ---
>>> 
>>> This patch fixes a crash introduced by "optee: fix tee out of memory
>>> failure seen during kexec reboot"[1]. However, I don't think that the
>>> original two patch series[2] plus this patch is the full solution to
>>> properly handling OP-TEE shared memory across kexec.
>>> 
>>> While testing this fix, I did about 10 kexec reboots and then triggered
>>> a kernel crash by writing 'c' to /proc/sysrq-trigger. The kdump kernel
>>> became unresponsive during boot while steadily streaming the following
>>> errors to the serial console:
>>> 
>>> arm-smmu 64000000.mmu: Blocked unknown Stream ID 0x2000; boot with "arm-smmu.disable_bypass=0" to allow, but this may have security implications
>>> arm-smmu 64000000.mmu:     GFSR 0x00000002, GFSYNR0 0x00000002, GFSYNR1 0x00002000, GFSYNR2 0x00000000
>>> 
>>> I suspect that this is related to the problems of OP-TEE shared memory
>>> handling across kexec. My current hunch is that while we've disabled the
>>> shared memory cache with this patch, we haven't unregistered all of the
>>> addresses that the previous kernel (which crashed) had registered with
>>> OP-TEE and that perhaps OP-TEE OS is still trying to make use those
>>> addresses?
>>> 
>>> I'm still pretty early in investigating that assumption and
>>> I'm learning about OP-TEE as I go but I wanted to get this initial
>>> fix-of-the-fix out so that it was clear that the v2 of the series[2] is
>>> not complete.
>>> 
>>> [1] https://lore.kernel.org/lkml/20210225090610.242623-2-allen.lkml@gmail.com/
>>> [2] https://lore.kernel.org/lkml/20210225090610.242623-1-allen.lkml@gmail.com/#t
>>> 
>>> drivers/tee/optee/call.c          | 11 ++++++++++-
>>> drivers/tee/optee/core.c          | 13 +++++++++++--
>>> drivers/tee/optee/optee_private.h |  2 +-
>>> 3 files changed, 22 insertions(+), 4 deletions(-)
>>> 
>>> diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c
>>> index 6132cc8d014c..799e84bec63d 100644
>>> --- a/drivers/tee/optee/call.c
>>> +++ b/drivers/tee/optee/call.c
>>> @@ -417,8 +417,10 @@ void optee_enable_shm_cache(struct optee *optee)
>>> * optee_disable_shm_cache() - Disables caching of some shared memory allocation
>>> *                          in OP-TEE
>>> * @optee:    main service struct
>>> + * @is_mapped:       true if the cached shared memory addresses were mapped by this
>>> + *           kernel, are safe to dereference, and should be freed
>>> */
>>> -void optee_disable_shm_cache(struct optee *optee)
>>> +void optee_disable_shm_cache(struct optee *optee, bool is_mapped)
>>> {
>>>      struct optee_call_waiter w;
>>> 
>>> @@ -437,6 +439,13 @@ void optee_disable_shm_cache(struct optee *optee)
>>>              if (res.result.status == OPTEE_SMC_RETURN_OK) {
>>>                      struct tee_shm *shm;
>>> 
>> 
>> Thanks Tyler.
>> From what I understand from my email exchange with Jens, I don’t
>> Think we want to touch optee_disable_shm_cache(), I could be wrong too,
>> @Jens, comments?
> 
> Changing optee_disable_shm_cache() is fine. Bear in mind that there
> are other times where we can't recover from a kernel crash. For
> instance if a thread is executing in OP-TEE in secure world.

I agree. My bad, I meant, “we don’t want to touch optee_disable_shm_cache()”.
And precisely for the reason you have mentioned above.

Thanks.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2021-05-07  9:33 UTC|newest]

Thread overview: 56+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-25  9:06 [PATCH v2 0/2] optee: fix OOM seen due to tee_shm_free() Allen Pais
2021-02-25  9:06 ` Allen Pais
2021-02-25  9:06 ` [PATCH v2 1/2] optee: fix tee out of memory failure seen during kexec reboot Allen Pais
2021-02-25  9:06   ` Allen Pais
2021-03-01 14:35   ` Jens Wiklander
2021-03-01 14:35     ` Jens Wiklander
2021-03-02  5:51     ` Allen Pais
2021-03-02  5:51       ` Allen Pais
2021-03-16 13:21     ` Allen Pais
2021-03-16 13:21       ` Allen Pais
2021-03-19  7:00       ` Jens Wiklander
2021-03-19  7:00         ` Jens Wiklander
2021-03-22  7:59         ` Allen Pais
2021-03-22  7:59           ` Allen Pais
2021-05-05 13:45         ` Allen Pais
2021-05-05 13:45           ` Allen Pais
2021-05-06  7:02           ` Jens Wiklander
2021-05-06  7:02             ` Jens Wiklander
2021-05-06  7:10             ` Allen Pais
2021-05-06  7:10               ` Allen Pais
2021-05-06  7:19               ` Jens Wiklander
2021-05-06  7:19                 ` Jens Wiklander
2021-05-06  7:29                 ` Allen Pais
2021-05-06  7:29                   ` Allen Pais
2021-05-06  8:15                   ` Jens Wiklander
2021-05-06  8:15                     ` Jens Wiklander
2021-05-06  8:35                     ` Allen Pais
2021-05-06  8:35                       ` Allen Pais
2021-05-07  7:03                     ` Allen Pais
2021-05-07  7:03                       ` Allen Pais
2021-03-18 20:51   ` Tyler Hicks
2021-03-18 20:51     ` Tyler Hicks
2021-02-25  9:06 ` [PATCH v2 2/2] firmware: tee_bnxt: implement shutdown method to handle kexec reboots Allen Pais
2021-02-25  9:06   ` Allen Pais
2021-03-18 20:55   ` Tyler Hicks
2021-03-18 20:55     ` Tyler Hicks
2021-05-07  3:58 ` [PATCH] optee: Disable shm cache when booting the crash kernel Tyler Hicks
2021-05-07  3:58   ` Tyler Hicks
2021-05-07  7:00   ` Allen Pais
2021-05-07  7:00     ` Allen Pais
2021-05-07  9:23     ` Jens Wiklander
2021-05-07  9:23       ` Jens Wiklander
2021-05-07  9:32       ` Allen Pais [this message]
2021-05-07  9:32         ` Allen Pais
2021-05-07 13:17       ` Tyler Hicks
2021-05-07 13:17         ` Tyler Hicks
2021-05-10  7:31         ` Jens Wiklander
2021-05-10  7:31           ` Jens Wiklander
2021-05-12  0:23           ` Tyler Hicks
2021-05-12  0:23             ` Tyler Hicks
2021-05-12  5:50             ` Jens Wiklander
2021-05-12  5:50               ` Jens Wiklander
2021-05-17 20:24               ` Tyler Hicks
2021-05-17 20:24                 ` Tyler Hicks
2021-05-17 20:31   ` Tyler Hicks
2021-05-17 20:31     ` Tyler Hicks

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=14C574FD-AF31-4D10-9685-E59FDD248C2C@linux.microsoft.com \
    --to=apais@linux.microsoft.com \
    --cc=allen.lkml@gmail.com \
    --cc=bcm-kernel-feedback-list@broadcom.com \
    --cc=jens.wiklander@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=op-tee@lists.trustedfirmware.org \
    --cc=tyhicks@linux.microsoft.com \
    --cc=zajec5@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.