Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ARM: Cortex-M: IRQ lock/unlock() API non-functional but accessible from user mode #21735

Closed
ioannisg opened this issue Jan 7, 2020 · 3 comments · Fixed by #21752
Closed

ARM: Cortex-M: IRQ lock/unlock() API non-functional but accessible from user mode #21735

ioannisg opened this issue Jan 7, 2020 · 3 comments · Fixed by #21752
Assignees
Labels
area: ARM ARM (32-bit) Architecture area: Userspace Userspace bug The issue is a bug, or the PR is fixing a bug priority: low Low impact/importance bug

Comments

@ioannisg
Copy link
Member

ioannisg commented Jan 7, 2020

Describe the bug

IRQ API is a kernel API, so, normally it shall not be accessible from user mode. This is the case for most of the API functions; an attempt to use them leads to a CPU fault.

This is not the case, however, for irq_lock(), irq_unlock() for ARM Cortex-M Mainline architecture (both ARMv7-M and ARMv8-M): these functions do not cause a fault if called from user mode, although, they (correctly) do not lock or unlock interrupts. Since we do not explicitly document that the IRQ API shall only be used from supervisor mode, this might end up very confusing for user threads calling irq_lock/unlock API.

To Reproduce
Steps to reproduce the behavior:
Simply call irq_lock() from user mode on ARM Cortex-M Mainline. The call does not fail and does not lock the IRQs.

Expected behavior
It should be clear for user mode threads that irq lock /unlock cannot be used.

I would see two options here

  • either add an assert on irq_lock, unlock when called from user mode
  • or simply document explicitly that the API shall not be used from user mode, otherwise we might have undefined behavior.
@ioannisg ioannisg added bug The issue is a bug, or the PR is fixing a bug priority: medium Medium impact/importance bug area: ARM ARM (32-bit) Architecture area: Userspace Userspace labels Jan 7, 2020
@ioannisg
Copy link
Member Author

ioannisg commented Jan 7, 2020

CC @andrewboie

@ioannisg ioannisg changed the title ARM: Cortex-M: IRQ lock/unlock() API accessible but non-functional from user mode ARM: Cortex-M: IRQ lock/unlock() API non-functional but accessible from user mode Jan 7, 2020
@ioannisg ioannisg removed the priority: medium Medium impact/importance bug label Jan 7, 2020
@jhedberg jhedberg added the priority: low Low impact/importance bug label Jan 7, 2020
@jhedberg
Copy link
Member

jhedberg commented Jan 7, 2020

Discussed in the release meeting: The documentation update needs to be done either way, however an assert is probably nice too since otherwise this risks hard-to-catch bugs with critical regions seemingly working but actually not.

@pderwin
Copy link
Contributor

pderwin commented Jan 7, 2020

I'll echo the 'hard-to-catch bugs' commentary. I have been actively pursuing a problem on Cortex-R in user mode that turned out to be this exact issue. Our user-mode code believed it was locking a critical section, but in reality was not.

Now investigating how to re-factor the code to rely on semaphore instead of interrupt disable.

andrewboie pushed a commit to andrewboie/zephyr that referenced this issue Jan 7, 2020
On ARM irq_lock() simply fails silently instead of generating
an exception.

Fixes: zephyrproject-rtos#21735

Signed-off-by: Andrew Boie <andrew.p.boie@intel.com>
nashif pushed a commit that referenced this issue Jan 8, 2020
On ARM irq_lock() simply fails silently instead of generating
an exception.

Fixes: #21735

Signed-off-by: Andrew Boie <andrew.p.boie@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: ARM ARM (32-bit) Architecture area: Userspace Userspace bug The issue is a bug, or the PR is fixing a bug priority: low Low impact/importance bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants