2023-10-10 01:08:36

by Edward AD

Subject: [PATCH] rfkill: fix deadlock in rfkill_send_events

syzbot report:
syz-executor675/5132 is trying to acquire lock:
ffff8880297ee088 (&data->mtx){+.+.}-{3:3}, at: rfkill_send_events+0x226/0x3f0 net/rfkill/core.c:286

but task is already holding lock:
ffff88801bfc0088 (&data->mtx){+.+.}-{3:3}, at: rfkill_fop_open+0x146/0x750 net/rfkill/core.c:1183

other info that might help us debug this:
Possible unsafe locking scenario:

CPU0
----
lock(&data->mtx);
lock(&data->mtx);

*** DEADLOCK ***

In 2c3dfba4cf84 insert rfkill_sync() to rfkill_fop_open(), it will call
rfkill_send_events() and then triger this issue.

Fixes: 2c3dfba4cf84 ("rfkill: sync before userspace visibility/changes")
Reported-and-tested-by: [email protected]
Signed-off-by: Edward AD <[email protected]>
---
net/rfkill/core.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/rfkill/core.c b/net/rfkill/core.c
index 08630896b6c8..a14e0d4a0b00 100644
--- a/net/rfkill/core.c
+++ b/net/rfkill/core.c
@@ -1180,7 +1180,6 @@ static int rfkill_fop_open(struct inode *inode, struct file *file)
init_waitqueue_head(&data->read_wait);

mutex_lock(&rfkill_global_mutex);
- mutex_lock(&data->mtx);
/*
* start getting events from elsewhere but hold mtx to get
* startup events added first
@@ -1191,9 +1190,12 @@ static int rfkill_fop_open(struct inode *inode, struct file *file)
if (!ev)
goto free;
rfkill_sync(rfkill);
+ mutex_lock(&data->mtx);
rfkill_fill_event(&ev->ev, rfkill, RFKILL_OP_ADD);
list_add_tail(&ev->list, &data->events);
+ mutex_unlock(&data->mtx);
}
+ mutex_lock(&data->mtx);
list_add(&data->list, &rfkill_fds);
mutex_unlock(&data->mtx);
mutex_unlock(&rfkill_global_mutex);
--
2.25.1

2023-10-14 02:54:13

by Edward AD

Subject: Re: [PATCH] rfkill: fix deadlock in rfkill_send_events

Hi Simon Horman,
On Fri, 13 Oct 2023 13:06:38 +0200, Simon Horman wrote:
> I am wondering if you considered moving the rfkill_sync() calls
> to before &data->mtx is taken, to avoid the need to drop and
> retake it?
If you move rfkill_sync() before calling &data->mtx, more code will be added
because rfkill_sync() is in the loop body.
>
> Perhaps it doesn't work for some reason (compile tested only!).
> But this does seem somehow cleaner for me.
BR,
edward