Timer interrupt ax25_ds_timeout() could introduce double locks on
&ax25_list_lock.
ax25_ioctl()
--> ax25_ctl_ioctl()
--> ax25_dama_off()
--> ax25_dev_dama_off()
--> ax25_check_dama_slave()
--> spin_lock(&ax25_list_lock)
<timer interrupt>
--> ax25_ds_timeout()
--> spin_lock(&ax25_list_lock)
This flaw was found by an experimental static analysis tool I am
developing for irq-related deadlock.
To prevent the potential deadlock, the patch use spin_lock_bh()
on &ax25_list_lock inside ax25_check_dama_slave().
Fixes: c19c4b9c9acb ("[AX.25]: Optimize AX.25 socket list lock")
Signed-off-by: Chengfeng Ye <[email protected]>
---
V2: add fixes tag
net/ax25/ax25_ds_subr.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/ax25/ax25_ds_subr.c b/net/ax25/ax25_ds_subr.c
index f00e27df3c76..010b11303d32 100644
--- a/net/ax25/ax25_ds_subr.c
+++ b/net/ax25/ax25_ds_subr.c
@@ -156,13 +156,13 @@ static int ax25_check_dama_slave(ax25_dev *ax25_dev)
ax25_cb *ax25;
int res = 0;
- spin_lock(&ax25_list_lock);
+ spin_lock_bh(&ax25_list_lock);
ax25_for_each(ax25, &ax25_list)
if (ax25->ax25_dev == ax25_dev && (ax25->condition & AX25_COND_DAMA_MODE) && ax25->state > AX25_STATE_1) {
res = 1;
break;
}
- spin_unlock(&ax25_list_lock);
+ spin_unlock_bh(&ax25_list_lock);
return res;
}
--
2.17.1
On Thu, Oct 05, 2023 at 07:23:49AM +0000, Chengfeng Ye wrote:
> Timer interrupt ax25_ds_timeout() could introduce double locks on
> &ax25_list_lock.
>
> ax25_ioctl()
> --> ax25_ctl_ioctl()
> --> ax25_dama_off()
> --> ax25_dev_dama_off()
> --> ax25_check_dama_slave()
> --> spin_lock(&ax25_list_lock)
> <timer interrupt>
> --> ax25_ds_timeout()
> --> spin_lock(&ax25_list_lock)
>
> This flaw was found by an experimental static analysis tool I am
> developing for irq-related deadlock.
>
> To prevent the potential deadlock, the patch use spin_lock_bh()
> on &ax25_list_lock inside ax25_check_dama_slave().
>
> Fixes: c19c4b9c9acb ("[AX.25]: Optimize AX.25 socket list lock")
> Signed-off-by: Chengfeng Ye <[email protected]>
Hi Chengfeng Ye,
Are there other functions that spin_lock(&ax25_list_lock)
that also need to use spin_lock_bh()?
...
Hi Simon Horman,
I think maybe not. My static analysis tool only reported this function, I
also just manually checked the spin_lock(&ax25_list_lock) in other
functions, and it looks like they are basically under rcv callback or timer,
which already have BH disabled. I think the developers who wrote
the code should be aware of this so they used spin_lock() instead of
spin_lock_bh().
But the fixed function is a bit different, as it could be called from .ioctl(),
which is from userland syscall and executes under the process
context, and along the call chain BH is also not disabled explicitly. That's
the reason why only at this place I change to spin_lock_bh().
Thanks,
Chengfeng
On Sat, Oct 07, 2023 at 11:43:59PM +0800, Chengfeng Ye wrote:
> Hi Simon Horman,
>
> I think maybe not. My static analysis tool only reported this function, I
> also just manually checked the spin_lock(&ax25_list_lock) in other
> functions, and it looks like they are basically under rcv callback or timer,
> which already have BH disabled. I think the developers who wrote
> the code should be aware of this so they used spin_lock() instead of
> spin_lock_bh().
>
> But the fixed function is a bit different, as it could be called from .ioctl(),
> which is from userland syscall and executes under the process
> context, and along the call chain BH is also not disabled explicitly. That's
> the reason why only at this place I change to spin_lock_bh().
Thanks,
I agree that seems to be the case.
Reviewed-by: Simon Horman <[email protected]>