* [PATCH 0/4] Move copy_from_user out of preempt disabled context
@ 2025-09-02 9:49 Bibo Mao
2025-09-02 9:49 ` [PATCH 1/4] LoongArch: KVM: Avoid use copy_from_user with lock hold in kvm_eiointc_regs_access Bibo Mao
` (3 more replies)
0 siblings, 4 replies; 8+ messages in thread
From: Bibo Mao @ 2025-09-02 9:49 UTC (permalink / raw)
To: Huacai Chen, Xianglai Li; +Cc: WANG Xuerui, kvm, loongarch, linux-kernel
Function copy_from_user() and copy_to_user() may sleep because of page
fault, and they cannot be called with preempt disabled context, such as
spin_lock hold context.
Here this patch set move copy_from_user out of preempt disabled context,
local variable is added in spinlock context at first and then put to user
memory from local variable without spinlock.
Bibo Mao (4):
LoongArch: KVM: Avoid use copy_from_user with lock hold in
kvm_eiointc_regs_access
LoongArch: KVM: Avoid use copy_from_user with lock hold in
kvm_eiointc_sw_status_access
LoongArch: KVM: Avoid use copy_from_user with lock hold in
kvm_eiointc_ctrl_access
LoongArch: KVM: Avoid use copy_from_user with lock hold in
kvm_pch_pic_regs_access
arch/loongarch/kvm/intc/eiointc.c | 87 +++++++++++++++++++------------
arch/loongarch/kvm/intc/pch_pic.c | 22 +++++---
2 files changed, 68 insertions(+), 41 deletions(-)
base-commit: b320789d6883cc00ac78ce83bccbfe7ed58afcf0
--
2.39.3
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/4] LoongArch: KVM: Avoid use copy_from_user with lock hold in kvm_eiointc_regs_access
2025-09-02 9:49 [PATCH 0/4] Move copy_from_user out of preempt disabled context Bibo Mao
@ 2025-09-02 9:49 ` Bibo Mao
2025-09-02 11:58 ` Huacai Chen
2025-09-02 9:49 ` [PATCH 2/4] LoongArch: KVM: Avoid use copy_from_user with lock hold in kvm_eiointc_sw_status_access Bibo Mao
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: Bibo Mao @ 2025-09-02 9:49 UTC (permalink / raw)
To: Huacai Chen, Xianglai Li; +Cc: WANG Xuerui, kvm, loongarch, linux-kernel
Function copy_from_user() and copy_to_user() may sleep because of page
fault, and they cannot be called in spin_lock hold context. Otherwise there
will be possible warning such as:
BUG: sleeping function called from invalid context at include/linux/uaccess.h:192
in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 6292, name: qemu-system-loo
preempt_count: 1, expected: 0
RCU nest depth: 0, expected: 0
INFO: lockdep is turned off.
irq event stamp: 0
hardirqs last enabled at (0): [<0000000000000000>] 0x0
hardirqs last disabled at (0): [<9000000004c4a554>] copy_process+0x90c/0x1d40
softirqs last enabled at (0): [<9000000004c4a554>] copy_process+0x90c/0x1d40
softirqs last disabled at (0): [<0000000000000000>] 0x0
CPU: 41 UID: 0 PID: 6292 Comm: qemu-system-loo Tainted: G W 6.17.0-rc3+ #31 PREEMPT(full)
Tainted: [W]=WARN
Stack : 0000000000000076 0000000000000000 9000000004c28264 9000100092ff4000
9000100092ff7b80 9000100092ff7b88 0000000000000000 9000100092ff7cc8
9000100092ff7cc0 9000100092ff7cc0 9000100092ff7a00 0000000000000001
0000000000000001 9000100092ff7b88 947d2f9216a5e8b9 900010008773d880
00000000ffff8b9f fffffffffffffffe 0000000000000ba1 fffffffffffffffe
000000000000003e 900000000825a15b 000010007ad38000 9000100092ff7ec0
0000000000000000 0000000000000000 9000000006f3ac60 9000000007252000
0000000000000000 00007ff746ff2230 0000000000000053 9000200088a021b0
0000555556c9d190 0000000000000000 9000000004c2827c 000055556cfb5f40
00000000000000b0 0000000000000007 0000000000000007 0000000000071c1d
Call Trace:
[<9000000004c2827c>] show_stack+0x5c/0x180
[<9000000004c20fac>] dump_stack_lvl+0x94/0xe4
[<9000000004c99c7c>] __might_resched+0x26c/0x290
[<9000000004f68968>] __might_fault+0x20/0x88
[<ffff800002311de0>] kvm_eiointc_regs_access.isra.0+0x88/0x380 [kvm]
[<ffff8000022f8514>] kvm_device_ioctl+0x194/0x290 [kvm]
[<900000000506b0d8>] sys_ioctl+0x388/0x1010
[<90000000063ed210>] do_syscall+0xb0/0x2d8
[<9000000004c25ef8>] handle_syscall+0xb8/0x158
Fixes: 1ad7efa552fd5 ("LoongArch: KVM: Add EIOINTC user mode read and write functions")
Signed-off-by: Bibo Mao <maobibo@loongson.cn>
---
arch/loongarch/kvm/intc/eiointc.c | 33 ++++++++++++++++++++-----------
1 file changed, 21 insertions(+), 12 deletions(-)
diff --git a/arch/loongarch/kvm/intc/eiointc.c b/arch/loongarch/kvm/intc/eiointc.c
index 026b139dcff2..2fb5b9c6e8ad 100644
--- a/arch/loongarch/kvm/intc/eiointc.c
+++ b/arch/loongarch/kvm/intc/eiointc.c
@@ -462,19 +462,17 @@ static int kvm_eiointc_ctrl_access(struct kvm_device *dev,
static int kvm_eiointc_regs_access(struct kvm_device *dev,
struct kvm_device_attr *attr,
- bool is_write)
+ bool is_write, int *data)
{
int addr, cpu, offset, ret = 0;
unsigned long flags;
void *p = NULL;
- void __user *data;
struct loongarch_eiointc *s;
s = dev->kvm->arch.eiointc;
addr = attr->attr;
cpu = addr >> 16;
addr &= 0xffff;
- data = (void __user *)attr->addr;
switch (addr) {
case EIOINTC_NODETYPE_START ... EIOINTC_NODETYPE_END:
offset = (addr - EIOINTC_NODETYPE_START) / 4;
@@ -513,13 +511,10 @@ static int kvm_eiointc_regs_access(struct kvm_device *dev,
}
spin_lock_irqsave(&s->lock, flags);
- if (is_write) {
- if (copy_from_user(p, data, 4))
- ret = -EFAULT;
- } else {
- if (copy_to_user(data, p, 4))
- ret = -EFAULT;
- }
+ if (is_write)
+ memcpy(p, data, 4);
+ else
+ memcpy(data, p, 4);
spin_unlock_irqrestore(&s->lock, flags);
return ret;
@@ -576,9 +571,18 @@ static int kvm_eiointc_sw_status_access(struct kvm_device *dev,
static int kvm_eiointc_get_attr(struct kvm_device *dev,
struct kvm_device_attr *attr)
{
+ int ret, data;
+
switch (attr->group) {
case KVM_DEV_LOONGARCH_EXTIOI_GRP_REGS:
- return kvm_eiointc_regs_access(dev, attr, false);
+ ret = kvm_eiointc_regs_access(dev, attr, false, &data);
+ if (ret)
+ return ret;
+
+ if (copy_to_user((void __user *)attr->addr, &data, 4))
+ ret = -EFAULT;
+
+ return ret;
case KVM_DEV_LOONGARCH_EXTIOI_GRP_SW_STATUS:
return kvm_eiointc_sw_status_access(dev, attr, false);
default:
@@ -589,11 +593,16 @@ static int kvm_eiointc_get_attr(struct kvm_device *dev,
static int kvm_eiointc_set_attr(struct kvm_device *dev,
struct kvm_device_attr *attr)
{
+ int data;
+
switch (attr->group) {
case KVM_DEV_LOONGARCH_EXTIOI_GRP_CTRL:
return kvm_eiointc_ctrl_access(dev, attr);
case KVM_DEV_LOONGARCH_EXTIOI_GRP_REGS:
- return kvm_eiointc_regs_access(dev, attr, true);
+ if (copy_from_user(&data, (void __user *)attr->addr, 4))
+ return -EFAULT;
+
+ return kvm_eiointc_regs_access(dev, attr, true, &data);
case KVM_DEV_LOONGARCH_EXTIOI_GRP_SW_STATUS:
return kvm_eiointc_sw_status_access(dev, attr, true);
default:
--
2.39.3
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/4] LoongArch: KVM: Avoid use copy_from_user with lock hold in kvm_eiointc_sw_status_access
2025-09-02 9:49 [PATCH 0/4] Move copy_from_user out of preempt disabled context Bibo Mao
2025-09-02 9:49 ` [PATCH 1/4] LoongArch: KVM: Avoid use copy_from_user with lock hold in kvm_eiointc_regs_access Bibo Mao
@ 2025-09-02 9:49 ` Bibo Mao
2025-09-02 9:49 ` [PATCH 3/4] LoongArch: KVM: Avoid use copy_from_user with lock hold in kvm_eiointc_ctrl_access Bibo Mao
2025-09-02 9:49 ` [PATCH 4/4] LoongArch: KVM: Avoid use copy_from_user with lock hold in kvm_pch_pic_regs_access Bibo Mao
3 siblings, 0 replies; 8+ messages in thread
From: Bibo Mao @ 2025-09-02 9:49 UTC (permalink / raw)
To: Huacai Chen, Xianglai Li; +Cc: WANG Xuerui, kvm, loongarch, linux-kernel
Function copy_from_user() and copy_to_user() may sleep because of page
fault, and they cannot be called in spin_lock hold context. Here move
funtcion calling with copy_from_user() and copy_to_user() out of function
kvm_eiointc_sw_status_access().
Fixes: 1ad7efa552fd5 ("LoongArch: KVM: Add EIOINTC user mode read and write functions")
Signed-off-by: Bibo Mao <maobibo@loongson.cn>
---
arch/loongarch/kvm/intc/eiointc.c | 29 +++++++++++++++++------------
1 file changed, 17 insertions(+), 12 deletions(-)
diff --git a/arch/loongarch/kvm/intc/eiointc.c b/arch/loongarch/kvm/intc/eiointc.c
index 2fb5b9c6e8ad..dd0477faf8e0 100644
--- a/arch/loongarch/kvm/intc/eiointc.c
+++ b/arch/loongarch/kvm/intc/eiointc.c
@@ -522,19 +522,17 @@ static int kvm_eiointc_regs_access(struct kvm_device *dev,
static int kvm_eiointc_sw_status_access(struct kvm_device *dev,
struct kvm_device_attr *attr,
- bool is_write)
+ bool is_write, int *data)
{
int addr, ret = 0;
unsigned long flags;
void *p = NULL;
- void __user *data;
struct loongarch_eiointc *s;
s = dev->kvm->arch.eiointc;
addr = attr->attr;
addr &= 0xffff;
- data = (void __user *)attr->addr;
switch (addr) {
case KVM_DEV_LOONGARCH_EXTIOI_SW_STATUS_NUM_CPU:
if (is_write)
@@ -556,13 +554,10 @@ static int kvm_eiointc_sw_status_access(struct kvm_device *dev,
return -EINVAL;
}
spin_lock_irqsave(&s->lock, flags);
- if (is_write) {
- if (copy_from_user(p, data, 4))
- ret = -EFAULT;
- } else {
- if (copy_to_user(data, p, 4))
- ret = -EFAULT;
- }
+ if (is_write)
+ memcpy(p, data, 4);
+ else
+ memcpy(data, p, 4);
spin_unlock_irqrestore(&s->lock, flags);
return ret;
@@ -584,7 +579,14 @@ static int kvm_eiointc_get_attr(struct kvm_device *dev,
return ret;
case KVM_DEV_LOONGARCH_EXTIOI_GRP_SW_STATUS:
- return kvm_eiointc_sw_status_access(dev, attr, false);
+ ret = kvm_eiointc_sw_status_access(dev, attr, false, &data);
+ if (ret)
+ return ret;
+
+ if (copy_to_user((void __user *)attr->addr, &data, 4))
+ ret = -EFAULT;
+
+ return ret;
default:
return -EINVAL;
}
@@ -604,7 +606,10 @@ static int kvm_eiointc_set_attr(struct kvm_device *dev,
return kvm_eiointc_regs_access(dev, attr, true, &data);
case KVM_DEV_LOONGARCH_EXTIOI_GRP_SW_STATUS:
- return kvm_eiointc_sw_status_access(dev, attr, true);
+ if (copy_from_user(&data, (void __user *)attr->addr, 4))
+ return -EFAULT;
+
+ return kvm_eiointc_sw_status_access(dev, attr, true, &data);
default:
return -EINVAL;
}
--
2.39.3
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 3/4] LoongArch: KVM: Avoid use copy_from_user with lock hold in kvm_eiointc_ctrl_access
2025-09-02 9:49 [PATCH 0/4] Move copy_from_user out of preempt disabled context Bibo Mao
2025-09-02 9:49 ` [PATCH 1/4] LoongArch: KVM: Avoid use copy_from_user with lock hold in kvm_eiointc_regs_access Bibo Mao
2025-09-02 9:49 ` [PATCH 2/4] LoongArch: KVM: Avoid use copy_from_user with lock hold in kvm_eiointc_sw_status_access Bibo Mao
@ 2025-09-02 9:49 ` Bibo Mao
2025-09-02 9:49 ` [PATCH 4/4] LoongArch: KVM: Avoid use copy_from_user with lock hold in kvm_pch_pic_regs_access Bibo Mao
3 siblings, 0 replies; 8+ messages in thread
From: Bibo Mao @ 2025-09-02 9:49 UTC (permalink / raw)
To: Huacai Chen, Xianglai Li; +Cc: WANG Xuerui, kvm, loongarch, linux-kernel
Function copy_from_user() and copy_to_user() may sleep because of page
fault, and they cannot be called in spin_lock hold context. Here move
function calling with copy_from_user() and copy_to_user() before spinlock
context in function kvm_eiointc_ctrl_access().
Fixes: 1ad7efa552fd5 ("LoongArch: KVM: Add EIOINTC user mode read and write functions")
Signed-off-by: Bibo Mao <maobibo@loongson.cn>
---
arch/loongarch/kvm/intc/eiointc.c | 25 +++++++++++++++----------
1 file changed, 15 insertions(+), 10 deletions(-)
diff --git a/arch/loongarch/kvm/intc/eiointc.c b/arch/loongarch/kvm/intc/eiointc.c
index dd0477faf8e0..c32333695381 100644
--- a/arch/loongarch/kvm/intc/eiointc.c
+++ b/arch/loongarch/kvm/intc/eiointc.c
@@ -426,21 +426,26 @@ static int kvm_eiointc_ctrl_access(struct kvm_device *dev,
struct loongarch_eiointc *s = dev->kvm->arch.eiointc;
data = (void __user *)attr->addr;
- spin_lock_irqsave(&s->lock, flags);
switch (type) {
case KVM_DEV_LOONGARCH_EXTIOI_CTRL_INIT_NUM_CPU:
+ case KVM_DEV_LOONGARCH_EXTIOI_CTRL_INIT_FEATURE:
if (copy_from_user(&val, data, 4))
- ret = -EFAULT;
- else {
- if (val >= EIOINTC_ROUTE_MAX_VCPUS)
- ret = -EINVAL;
- else
- s->num_cpu = val;
- }
+ return -EFAULT;
+ break;
+ default:
+ break;
+ }
+
+ spin_lock_irqsave(&s->lock, flags);
+ switch (type) {
+ case KVM_DEV_LOONGARCH_EXTIOI_CTRL_INIT_NUM_CPU:
+ if (val >= EIOINTC_ROUTE_MAX_VCPUS)
+ ret = -EINVAL;
+ else
+ s->num_cpu = val;
break;
case KVM_DEV_LOONGARCH_EXTIOI_CTRL_INIT_FEATURE:
- if (copy_from_user(&s->features, data, 4))
- ret = -EFAULT;
+ s->features = val;
if (!(s->features & BIT(EIOINTC_HAS_VIRT_EXTENSION)))
s->status |= BIT(EIOINTC_ENABLE);
break;
--
2.39.3
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 4/4] LoongArch: KVM: Avoid use copy_from_user with lock hold in kvm_pch_pic_regs_access
2025-09-02 9:49 [PATCH 0/4] Move copy_from_user out of preempt disabled context Bibo Mao
` (2 preceding siblings ...)
2025-09-02 9:49 ` [PATCH 3/4] LoongArch: KVM: Avoid use copy_from_user with lock hold in kvm_eiointc_ctrl_access Bibo Mao
@ 2025-09-02 9:49 ` Bibo Mao
3 siblings, 0 replies; 8+ messages in thread
From: Bibo Mao @ 2025-09-02 9:49 UTC (permalink / raw)
To: Huacai Chen, Xianglai Li; +Cc: WANG Xuerui, kvm, loongarch, linux-kernel
Function copy_from_user() and copy_to_user() may sleep because of page
fault, and they cannot be called in spin_lock hold context. Here move
function calling with copy_from_user() and copy_to_user() out of spinlock
context in function kvm_pch_pic_regs_access().
Fixes: d206d95148732 ("LoongArch: KVM: Add PCHPIC user mode read and write functions")
Signed-off-by: Bibo Mao <maobibo@loongson.cn>
---
arch/loongarch/kvm/intc/pch_pic.c | 22 +++++++++++++++-------
1 file changed, 15 insertions(+), 7 deletions(-)
diff --git a/arch/loongarch/kvm/intc/pch_pic.c b/arch/loongarch/kvm/intc/pch_pic.c
index 119290bcea79..71706e24a1c5 100644
--- a/arch/loongarch/kvm/intc/pch_pic.c
+++ b/arch/loongarch/kvm/intc/pch_pic.c
@@ -352,6 +352,7 @@ static int kvm_pch_pic_regs_access(struct kvm_device *dev,
void __user *data;
void *p = NULL;
struct loongarch_pch_pic *s;
+ char buf[8];
s = dev->kvm->arch.pch_pic;
addr = attr->attr;
@@ -397,17 +398,24 @@ static int kvm_pch_pic_regs_access(struct kvm_device *dev,
return -EINVAL;
}
- spin_lock(&s->lock);
- /* write or read value according to is_write */
if (is_write) {
- if (copy_from_user(p, data, len))
- ret = -EFAULT;
- } else {
- if (copy_to_user(data, p, len))
- ret = -EFAULT;
+ if (copy_from_user(buf, data, len))
+ return -EFAULT;
}
+
+ spin_lock(&s->lock);
+ /* write or read value according to is_write */
+ if (is_write)
+ memcpy(p, buf, len);
+ else
+ memcpy(buf, p, len);
spin_unlock(&s->lock);
+ if (!is_write) {
+ if (copy_to_user(data, buf, len))
+ return -EFAULT;
+ }
+
return ret;
}
--
2.39.3
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/4] LoongArch: KVM: Avoid use copy_from_user with lock hold in kvm_eiointc_regs_access
2025-09-02 9:49 ` [PATCH 1/4] LoongArch: KVM: Avoid use copy_from_user with lock hold in kvm_eiointc_regs_access Bibo Mao
@ 2025-09-02 11:58 ` Huacai Chen
2025-09-02 12:15 ` Bibo Mao
0 siblings, 1 reply; 8+ messages in thread
From: Huacai Chen @ 2025-09-02 11:58 UTC (permalink / raw)
To: Bibo Mao; +Cc: Xianglai Li, WANG Xuerui, kvm, loongarch, linux-kernel
Hi, Bibo,
On Tue, Sep 2, 2025 at 5:49 PM Bibo Mao <maobibo@loongson.cn> wrote:
>
> Function copy_from_user() and copy_to_user() may sleep because of page
> fault, and they cannot be called in spin_lock hold context. Otherwise there
> will be possible warning such as:
>
> BUG: sleeping function called from invalid context at include/linux/uaccess.h:192
> in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 6292, name: qemu-system-loo
> preempt_count: 1, expected: 0
> RCU nest depth: 0, expected: 0
> INFO: lockdep is turned off.
> irq event stamp: 0
> hardirqs last enabled at (0): [<0000000000000000>] 0x0
> hardirqs last disabled at (0): [<9000000004c4a554>] copy_process+0x90c/0x1d40
> softirqs last enabled at (0): [<9000000004c4a554>] copy_process+0x90c/0x1d40
> softirqs last disabled at (0): [<0000000000000000>] 0x0
> CPU: 41 UID: 0 PID: 6292 Comm: qemu-system-loo Tainted: G W 6.17.0-rc3+ #31 PREEMPT(full)
> Tainted: [W]=WARN
> Stack : 0000000000000076 0000000000000000 9000000004c28264 9000100092ff4000
> 9000100092ff7b80 9000100092ff7b88 0000000000000000 9000100092ff7cc8
> 9000100092ff7cc0 9000100092ff7cc0 9000100092ff7a00 0000000000000001
> 0000000000000001 9000100092ff7b88 947d2f9216a5e8b9 900010008773d880
> 00000000ffff8b9f fffffffffffffffe 0000000000000ba1 fffffffffffffffe
> 000000000000003e 900000000825a15b 000010007ad38000 9000100092ff7ec0
> 0000000000000000 0000000000000000 9000000006f3ac60 9000000007252000
> 0000000000000000 00007ff746ff2230 0000000000000053 9000200088a021b0
> 0000555556c9d190 0000000000000000 9000000004c2827c 000055556cfb5f40
> 00000000000000b0 0000000000000007 0000000000000007 0000000000071c1d
> Call Trace:
> [<9000000004c2827c>] show_stack+0x5c/0x180
> [<9000000004c20fac>] dump_stack_lvl+0x94/0xe4
> [<9000000004c99c7c>] __might_resched+0x26c/0x290
> [<9000000004f68968>] __might_fault+0x20/0x88
> [<ffff800002311de0>] kvm_eiointc_regs_access.isra.0+0x88/0x380 [kvm]
> [<ffff8000022f8514>] kvm_device_ioctl+0x194/0x290 [kvm]
> [<900000000506b0d8>] sys_ioctl+0x388/0x1010
> [<90000000063ed210>] do_syscall+0xb0/0x2d8
> [<9000000004c25ef8>] handle_syscall+0xb8/0x158
>
> Fixes: 1ad7efa552fd5 ("LoongArch: KVM: Add EIOINTC user mode read and write functions")
> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> ---
> arch/loongarch/kvm/intc/eiointc.c | 33 ++++++++++++++++++++-----------
> 1 file changed, 21 insertions(+), 12 deletions(-)
>
> diff --git a/arch/loongarch/kvm/intc/eiointc.c b/arch/loongarch/kvm/intc/eiointc.c
> index 026b139dcff2..2fb5b9c6e8ad 100644
> --- a/arch/loongarch/kvm/intc/eiointc.c
> +++ b/arch/loongarch/kvm/intc/eiointc.c
> @@ -462,19 +462,17 @@ static int kvm_eiointc_ctrl_access(struct kvm_device *dev,
>
> static int kvm_eiointc_regs_access(struct kvm_device *dev,
> struct kvm_device_attr *attr,
> - bool is_write)
> + bool is_write, int *data)
> {
> int addr, cpu, offset, ret = 0;
> unsigned long flags;
> void *p = NULL;
> - void __user *data;
> struct loongarch_eiointc *s;
>
> s = dev->kvm->arch.eiointc;
> addr = attr->attr;
> cpu = addr >> 16;
> addr &= 0xffff;
> - data = (void __user *)attr->addr;
> switch (addr) {
> case EIOINTC_NODETYPE_START ... EIOINTC_NODETYPE_END:
> offset = (addr - EIOINTC_NODETYPE_START) / 4;
> @@ -513,13 +511,10 @@ static int kvm_eiointc_regs_access(struct kvm_device *dev,
> }
>
> spin_lock_irqsave(&s->lock, flags);
> - if (is_write) {
> - if (copy_from_user(p, data, 4))
> - ret = -EFAULT;
> - } else {
> - if (copy_to_user(data, p, 4))
> - ret = -EFAULT;
> - }
> + if (is_write)
> + memcpy(p, data, 4);
> + else
> + memcpy(data, p, 4);
p is a local variable, data is a parameter, they both have nothing to
do with s, why memcpy need to be protected?
After some thinking I found the code was wrong at the first time. The
real code that needs to be protected is not copy_from_user() or
memcpy(), but the above switch block.
Other patches have similar problems.
Huacai
> spin_unlock_irqrestore(&s->lock, flags);
>
> return ret;
> @@ -576,9 +571,18 @@ static int kvm_eiointc_sw_status_access(struct kvm_device *dev,
> static int kvm_eiointc_get_attr(struct kvm_device *dev,
> struct kvm_device_attr *attr)
> {
> + int ret, data;
> +
> switch (attr->group) {
> case KVM_DEV_LOONGARCH_EXTIOI_GRP_REGS:
> - return kvm_eiointc_regs_access(dev, attr, false);
> + ret = kvm_eiointc_regs_access(dev, attr, false, &data);
> + if (ret)
> + return ret;
> +
> + if (copy_to_user((void __user *)attr->addr, &data, 4))
> + ret = -EFAULT;
> +
> + return ret;
> case KVM_DEV_LOONGARCH_EXTIOI_GRP_SW_STATUS:
> return kvm_eiointc_sw_status_access(dev, attr, false);
> default:
> @@ -589,11 +593,16 @@ static int kvm_eiointc_get_attr(struct kvm_device *dev,
> static int kvm_eiointc_set_attr(struct kvm_device *dev,
> struct kvm_device_attr *attr)
> {
> + int data;
> +
> switch (attr->group) {
> case KVM_DEV_LOONGARCH_EXTIOI_GRP_CTRL:
> return kvm_eiointc_ctrl_access(dev, attr);
> case KVM_DEV_LOONGARCH_EXTIOI_GRP_REGS:
> - return kvm_eiointc_regs_access(dev, attr, true);
> + if (copy_from_user(&data, (void __user *)attr->addr, 4))
> + return -EFAULT;
> +
> + return kvm_eiointc_regs_access(dev, attr, true, &data);
> case KVM_DEV_LOONGARCH_EXTIOI_GRP_SW_STATUS:
> return kvm_eiointc_sw_status_access(dev, attr, true);
> default:
> --
> 2.39.3
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/4] LoongArch: KVM: Avoid use copy_from_user with lock hold in kvm_eiointc_regs_access
2025-09-02 11:58 ` Huacai Chen
@ 2025-09-02 12:15 ` Bibo Mao
2025-09-03 8:34 ` Huacai Chen
0 siblings, 1 reply; 8+ messages in thread
From: Bibo Mao @ 2025-09-02 12:15 UTC (permalink / raw)
To: Huacai Chen; +Cc: Xianglai Li, WANG Xuerui, kvm, loongarch, linux-kernel
On 2025/9/2 下午7:58, Huacai Chen wrote:
> Hi, Bibo,
>
> On Tue, Sep 2, 2025 at 5:49 PM Bibo Mao <maobibo@loongson.cn> wrote:
>>
>> Function copy_from_user() and copy_to_user() may sleep because of page
>> fault, and they cannot be called in spin_lock hold context. Otherwise there
>> will be possible warning such as:
>>
>> BUG: sleeping function called from invalid context at include/linux/uaccess.h:192
>> in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 6292, name: qemu-system-loo
>> preempt_count: 1, expected: 0
>> RCU nest depth: 0, expected: 0
>> INFO: lockdep is turned off.
>> irq event stamp: 0
>> hardirqs last enabled at (0): [<0000000000000000>] 0x0
>> hardirqs last disabled at (0): [<9000000004c4a554>] copy_process+0x90c/0x1d40
>> softirqs last enabled at (0): [<9000000004c4a554>] copy_process+0x90c/0x1d40
>> softirqs last disabled at (0): [<0000000000000000>] 0x0
>> CPU: 41 UID: 0 PID: 6292 Comm: qemu-system-loo Tainted: G W 6.17.0-rc3+ #31 PREEMPT(full)
>> Tainted: [W]=WARN
>> Stack : 0000000000000076 0000000000000000 9000000004c28264 9000100092ff4000
>> 9000100092ff7b80 9000100092ff7b88 0000000000000000 9000100092ff7cc8
>> 9000100092ff7cc0 9000100092ff7cc0 9000100092ff7a00 0000000000000001
>> 0000000000000001 9000100092ff7b88 947d2f9216a5e8b9 900010008773d880
>> 00000000ffff8b9f fffffffffffffffe 0000000000000ba1 fffffffffffffffe
>> 000000000000003e 900000000825a15b 000010007ad38000 9000100092ff7ec0
>> 0000000000000000 0000000000000000 9000000006f3ac60 9000000007252000
>> 0000000000000000 00007ff746ff2230 0000000000000053 9000200088a021b0
>> 0000555556c9d190 0000000000000000 9000000004c2827c 000055556cfb5f40
>> 00000000000000b0 0000000000000007 0000000000000007 0000000000071c1d
>> Call Trace:
>> [<9000000004c2827c>] show_stack+0x5c/0x180
>> [<9000000004c20fac>] dump_stack_lvl+0x94/0xe4
>> [<9000000004c99c7c>] __might_resched+0x26c/0x290
>> [<9000000004f68968>] __might_fault+0x20/0x88
>> [<ffff800002311de0>] kvm_eiointc_regs_access.isra.0+0x88/0x380 [kvm]
>> [<ffff8000022f8514>] kvm_device_ioctl+0x194/0x290 [kvm]
>> [<900000000506b0d8>] sys_ioctl+0x388/0x1010
>> [<90000000063ed210>] do_syscall+0xb0/0x2d8
>> [<9000000004c25ef8>] handle_syscall+0xb8/0x158
>>
>> Fixes: 1ad7efa552fd5 ("LoongArch: KVM: Add EIOINTC user mode read and write functions")
>> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
>> ---
>> arch/loongarch/kvm/intc/eiointc.c | 33 ++++++++++++++++++++-----------
>> 1 file changed, 21 insertions(+), 12 deletions(-)
>>
>> diff --git a/arch/loongarch/kvm/intc/eiointc.c b/arch/loongarch/kvm/intc/eiointc.c
>> index 026b139dcff2..2fb5b9c6e8ad 100644
>> --- a/arch/loongarch/kvm/intc/eiointc.c
>> +++ b/arch/loongarch/kvm/intc/eiointc.c
>> @@ -462,19 +462,17 @@ static int kvm_eiointc_ctrl_access(struct kvm_device *dev,
>>
>> static int kvm_eiointc_regs_access(struct kvm_device *dev,
>> struct kvm_device_attr *attr,
>> - bool is_write)
>> + bool is_write, int *data)
>> {
>> int addr, cpu, offset, ret = 0;
>> unsigned long flags;
>> void *p = NULL;
>> - void __user *data;
>> struct loongarch_eiointc *s;
>>
>> s = dev->kvm->arch.eiointc;
>> addr = attr->attr;
>> cpu = addr >> 16;
>> addr &= 0xffff;
>> - data = (void __user *)attr->addr;
>> switch (addr) {
>> case EIOINTC_NODETYPE_START ... EIOINTC_NODETYPE_END:
>> offset = (addr - EIOINTC_NODETYPE_START) / 4;
>> @@ -513,13 +511,10 @@ static int kvm_eiointc_regs_access(struct kvm_device *dev,
>> }
>>
>> spin_lock_irqsave(&s->lock, flags);
>> - if (is_write) {
>> - if (copy_from_user(p, data, 4))
>> - ret = -EFAULT;
>> - } else {
>> - if (copy_to_user(data, p, 4))
>> - ret = -EFAULT;
>> - }
>> + if (is_write)
>> + memcpy(p, data, 4);
>> + else
>> + memcpy(data, p, 4);
> p is a local variable, data is a parameter, they both have nothing to
> do with s, why memcpy need to be protected?
p is pointer to register buffer rather than local variable. When dump
extioi register to user space, maybe one vCPU is writing extioi register
at the same time, so there needs spinlock protection.
>
> After some thinking I found the code was wrong at the first time. The
> real code that needs to be protected is not copy_from_user() or
> memcpy(), but the above switch block.
For switch block in function kvm_eiointc_regs_access() for example, it
is only to get register buffer pointer, not register content. Spinlock
protection is not necessary in switch block.
Regards
Bibo Mao
>
> Other patches have similar problems.
>
> Huacai
>
>> spin_unlock_irqrestore(&s->lock, flags);
>>
>> return ret;
>> @@ -576,9 +571,18 @@ static int kvm_eiointc_sw_status_access(struct kvm_device *dev,
>> static int kvm_eiointc_get_attr(struct kvm_device *dev,
>> struct kvm_device_attr *attr)
>> {
>> + int ret, data;
>> +
>> switch (attr->group) {
>> case KVM_DEV_LOONGARCH_EXTIOI_GRP_REGS:
>> - return kvm_eiointc_regs_access(dev, attr, false);
>> + ret = kvm_eiointc_regs_access(dev, attr, false, &data);
>> + if (ret)
>> + return ret;
>> +
>> + if (copy_to_user((void __user *)attr->addr, &data, 4))
>> + ret = -EFAULT;
>> +
>> + return ret;
>> case KVM_DEV_LOONGARCH_EXTIOI_GRP_SW_STATUS:
>> return kvm_eiointc_sw_status_access(dev, attr, false);
>> default:
>> @@ -589,11 +593,16 @@ static int kvm_eiointc_get_attr(struct kvm_device *dev,
>> static int kvm_eiointc_set_attr(struct kvm_device *dev,
>> struct kvm_device_attr *attr)
>> {
>> + int data;
>> +
>> switch (attr->group) {
>> case KVM_DEV_LOONGARCH_EXTIOI_GRP_CTRL:
>> return kvm_eiointc_ctrl_access(dev, attr);
>> case KVM_DEV_LOONGARCH_EXTIOI_GRP_REGS:
>> - return kvm_eiointc_regs_access(dev, attr, true);
>> + if (copy_from_user(&data, (void __user *)attr->addr, 4))
>> + return -EFAULT;
>> +
>> + return kvm_eiointc_regs_access(dev, attr, true, &data);
>> case KVM_DEV_LOONGARCH_EXTIOI_GRP_SW_STATUS:
>> return kvm_eiointc_sw_status_access(dev, attr, true);
>> default:
>> --
>> 2.39.3
>>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/4] LoongArch: KVM: Avoid use copy_from_user with lock hold in kvm_eiointc_regs_access
2025-09-02 12:15 ` Bibo Mao
@ 2025-09-03 8:34 ` Huacai Chen
0 siblings, 0 replies; 8+ messages in thread
From: Huacai Chen @ 2025-09-03 8:34 UTC (permalink / raw)
To: Bibo Mao; +Cc: Xianglai Li, WANG Xuerui, kvm, loongarch, linux-kernel
On Tue, Sep 2, 2025 at 8:17 PM Bibo Mao <maobibo@loongson.cn> wrote:
>
>
>
> On 2025/9/2 下午7:58, Huacai Chen wrote:
> > Hi, Bibo,
> >
> > On Tue, Sep 2, 2025 at 5:49 PM Bibo Mao <maobibo@loongson.cn> wrote:
> >>
> >> Function copy_from_user() and copy_to_user() may sleep because of page
> >> fault, and they cannot be called in spin_lock hold context. Otherwise there
> >> will be possible warning such as:
> >>
> >> BUG: sleeping function called from invalid context at include/linux/uaccess.h:192
> >> in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 6292, name: qemu-system-loo
> >> preempt_count: 1, expected: 0
> >> RCU nest depth: 0, expected: 0
> >> INFO: lockdep is turned off.
> >> irq event stamp: 0
> >> hardirqs last enabled at (0): [<0000000000000000>] 0x0
> >> hardirqs last disabled at (0): [<9000000004c4a554>] copy_process+0x90c/0x1d40
> >> softirqs last enabled at (0): [<9000000004c4a554>] copy_process+0x90c/0x1d40
> >> softirqs last disabled at (0): [<0000000000000000>] 0x0
> >> CPU: 41 UID: 0 PID: 6292 Comm: qemu-system-loo Tainted: G W 6.17.0-rc3+ #31 PREEMPT(full)
> >> Tainted: [W]=WARN
> >> Stack : 0000000000000076 0000000000000000 9000000004c28264 9000100092ff4000
> >> 9000100092ff7b80 9000100092ff7b88 0000000000000000 9000100092ff7cc8
> >> 9000100092ff7cc0 9000100092ff7cc0 9000100092ff7a00 0000000000000001
> >> 0000000000000001 9000100092ff7b88 947d2f9216a5e8b9 900010008773d880
> >> 00000000ffff8b9f fffffffffffffffe 0000000000000ba1 fffffffffffffffe
> >> 000000000000003e 900000000825a15b 000010007ad38000 9000100092ff7ec0
> >> 0000000000000000 0000000000000000 9000000006f3ac60 9000000007252000
> >> 0000000000000000 00007ff746ff2230 0000000000000053 9000200088a021b0
> >> 0000555556c9d190 0000000000000000 9000000004c2827c 000055556cfb5f40
> >> 00000000000000b0 0000000000000007 0000000000000007 0000000000071c1d
> >> Call Trace:
> >> [<9000000004c2827c>] show_stack+0x5c/0x180
> >> [<9000000004c20fac>] dump_stack_lvl+0x94/0xe4
> >> [<9000000004c99c7c>] __might_resched+0x26c/0x290
> >> [<9000000004f68968>] __might_fault+0x20/0x88
> >> [<ffff800002311de0>] kvm_eiointc_regs_access.isra.0+0x88/0x380 [kvm]
> >> [<ffff8000022f8514>] kvm_device_ioctl+0x194/0x290 [kvm]
> >> [<900000000506b0d8>] sys_ioctl+0x388/0x1010
> >> [<90000000063ed210>] do_syscall+0xb0/0x2d8
> >> [<9000000004c25ef8>] handle_syscall+0xb8/0x158
> >>
> >> Fixes: 1ad7efa552fd5 ("LoongArch: KVM: Add EIOINTC user mode read and write functions")
> >> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> >> ---
> >> arch/loongarch/kvm/intc/eiointc.c | 33 ++++++++++++++++++++-----------
> >> 1 file changed, 21 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/arch/loongarch/kvm/intc/eiointc.c b/arch/loongarch/kvm/intc/eiointc.c
> >> index 026b139dcff2..2fb5b9c6e8ad 100644
> >> --- a/arch/loongarch/kvm/intc/eiointc.c
> >> +++ b/arch/loongarch/kvm/intc/eiointc.c
> >> @@ -462,19 +462,17 @@ static int kvm_eiointc_ctrl_access(struct kvm_device *dev,
> >>
> >> static int kvm_eiointc_regs_access(struct kvm_device *dev,
> >> struct kvm_device_attr *attr,
> >> - bool is_write)
> >> + bool is_write, int *data)
> >> {
> >> int addr, cpu, offset, ret = 0;
> >> unsigned long flags;
> >> void *p = NULL;
> >> - void __user *data;
> >> struct loongarch_eiointc *s;
> >>
> >> s = dev->kvm->arch.eiointc;
> >> addr = attr->attr;
> >> cpu = addr >> 16;
> >> addr &= 0xffff;
> >> - data = (void __user *)attr->addr;
> >> switch (addr) {
> >> case EIOINTC_NODETYPE_START ... EIOINTC_NODETYPE_END:
> >> offset = (addr - EIOINTC_NODETYPE_START) / 4;
> >> @@ -513,13 +511,10 @@ static int kvm_eiointc_regs_access(struct kvm_device *dev,
> >> }
> >>
> >> spin_lock_irqsave(&s->lock, flags);
> >> - if (is_write) {
> >> - if (copy_from_user(p, data, 4))
> >> - ret = -EFAULT;
> >> - } else {
> >> - if (copy_to_user(data, p, 4))
> >> - ret = -EFAULT;
> >> - }
> >> + if (is_write)
> >> + memcpy(p, data, 4);
> >> + else
> >> + memcpy(data, p, 4);
> > p is a local variable, data is a parameter, they both have nothing to
> > do with s, why memcpy need to be protected?
> p is pointer to register buffer rather than local variable. When dump
> extioi register to user space, maybe one vCPU is writing extioi register
> at the same time, so there needs spinlock protection.
Make sense, applied.
Huacai
>
> >
> > After some thinking I found the code was wrong at the first time. The
> > real code that needs to be protected is not copy_from_user() or
> > memcpy(), but the above switch block.
> For switch block in function kvm_eiointc_regs_access() for example, it
> is only to get register buffer pointer, not register content. Spinlock
> protection is not necessary in switch block.
>
> Regards
> Bibo Mao
> >
> > Other patches have similar problems.
> >
> > Huacai
> >
> >> spin_unlock_irqrestore(&s->lock, flags);
> >>
> >> return ret;
> >> @@ -576,9 +571,18 @@ static int kvm_eiointc_sw_status_access(struct kvm_device *dev,
> >> static int kvm_eiointc_get_attr(struct kvm_device *dev,
> >> struct kvm_device_attr *attr)
> >> {
> >> + int ret, data;
> >> +
> >> switch (attr->group) {
> >> case KVM_DEV_LOONGARCH_EXTIOI_GRP_REGS:
> >> - return kvm_eiointc_regs_access(dev, attr, false);
> >> + ret = kvm_eiointc_regs_access(dev, attr, false, &data);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + if (copy_to_user((void __user *)attr->addr, &data, 4))
> >> + ret = -EFAULT;
> >> +
> >> + return ret;
> >> case KVM_DEV_LOONGARCH_EXTIOI_GRP_SW_STATUS:
> >> return kvm_eiointc_sw_status_access(dev, attr, false);
> >> default:
> >> @@ -589,11 +593,16 @@ static int kvm_eiointc_get_attr(struct kvm_device *dev,
> >> static int kvm_eiointc_set_attr(struct kvm_device *dev,
> >> struct kvm_device_attr *attr)
> >> {
> >> + int data;
> >> +
> >> switch (attr->group) {
> >> case KVM_DEV_LOONGARCH_EXTIOI_GRP_CTRL:
> >> return kvm_eiointc_ctrl_access(dev, attr);
> >> case KVM_DEV_LOONGARCH_EXTIOI_GRP_REGS:
> >> - return kvm_eiointc_regs_access(dev, attr, true);
> >> + if (copy_from_user(&data, (void __user *)attr->addr, 4))
> >> + return -EFAULT;
> >> +
> >> + return kvm_eiointc_regs_access(dev, attr, true, &data);
> >> case KVM_DEV_LOONGARCH_EXTIOI_GRP_SW_STATUS:
> >> return kvm_eiointc_sw_status_access(dev, attr, true);
> >> default:
> >> --
> >> 2.39.3
> >>
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2025-09-03 8:35 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-09-02 9:49 [PATCH 0/4] Move copy_from_user out of preempt disabled context Bibo Mao
2025-09-02 9:49 ` [PATCH 1/4] LoongArch: KVM: Avoid use copy_from_user with lock hold in kvm_eiointc_regs_access Bibo Mao
2025-09-02 11:58 ` Huacai Chen
2025-09-02 12:15 ` Bibo Mao
2025-09-03 8:34 ` Huacai Chen
2025-09-02 9:49 ` [PATCH 2/4] LoongArch: KVM: Avoid use copy_from_user with lock hold in kvm_eiointc_sw_status_access Bibo Mao
2025-09-02 9:49 ` [PATCH 3/4] LoongArch: KVM: Avoid use copy_from_user with lock hold in kvm_eiointc_ctrl_access Bibo Mao
2025-09-02 9:49 ` [PATCH 4/4] LoongArch: KVM: Avoid use copy_from_user with lock hold in kvm_pch_pic_regs_access Bibo Mao
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).