As &info->lock is acquired by slgt_interrupt() under irq context, other
process context code acquiring the lock should disable irq, otherwise
deadlock could happen if the irq preempt the execution while the
lock is held in process context on the same CPU.
Lock acquisition inside set_params32() does not disable irq, and this
function is called by slgt_compat_ioctl() from process context.
Possible deadlock scenario:
slgt_compat_ioctl()
-> set_params32()
-> spin_lock(&info->lock)
<irq>
-> slgt_interrupt()
-> spin_lock(&info->lock); (deadlock here)
This flaw was found by an experimental static analysis tool I am developing
for irq-related deadlock. x86_64 allmodconfig using gcc shows no new
warning.
The patch fixes the potential deadlock by spin_lock_irqsave() like other
lock acquisition sites.
Signed-off-by: Chengfeng Ye <[email protected]>
---
drivers/tty/synclink_gt.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/synclink_gt.c b/drivers/tty/synclink_gt.c
index 16e469e581ec..e86bb6228e0f 100644
--- a/drivers/tty/synclink_gt.c
+++ b/drivers/tty/synclink_gt.c
@@ -1088,12 +1088,13 @@ static long get_params32(struct slgt_info *info, struct MGSL_PARAMS32 __user *us
static long set_params32(struct slgt_info *info, struct MGSL_PARAMS32 __user *new_params)
{
struct MGSL_PARAMS32 tmp_params;
+ unsigned long flags;
DBGINFO(("%s set_params32\n", info->device_name));
if (copy_from_user(&tmp_params, new_params, sizeof(struct MGSL_PARAMS32)))
return -EFAULT;
- spin_lock(&info->lock);
+ spin_lock_irqsave(&info->lock, flags);
if (tmp_params.mode == MGSL_MODE_BASE_CLOCK) {
info->base_clock = tmp_params.clock_speed;
} else {
@@ -1111,7 +1112,7 @@ static long set_params32(struct slgt_info *info, struct MGSL_PARAMS32 __user *ne
info->params.stop_bits = tmp_params.stop_bits;
info->params.parity = tmp_params.parity;
}
- spin_unlock(&info->lock);
+ spin_unlock_irqrestore(&info->lock, flags);
program_hw(info);
--
2.17.1
On 28. 07. 23, 14:39, Chengfeng Ye wrote:
> As &info->lock is acquired by slgt_interrupt() under irq context, other
> process context code acquiring the lock should disable irq, otherwise
> deadlock could happen if the irq preempt the execution while the
> lock is held in process context on the same CPU.
>
> Lock acquisition inside set_params32() does not disable irq, and this
> function is called by slgt_compat_ioctl() from process context.
>
> Possible deadlock scenario:
> slgt_compat_ioctl()
> -> set_params32()
> -> spin_lock(&info->lock)
> <irq>
> -> slgt_interrupt()
> -> spin_lock(&info->lock); (deadlock here)
>
> This flaw was found by an experimental static analysis tool I am developing
> for irq-related deadlock. x86_64 allmodconfig using gcc shows no new
> warning.
>
> The patch fixes the potential deadlock by spin_lock_irqsave() like other
> lock acquisition sites.
>
> Signed-off-by: Chengfeng Ye <[email protected]>
Oh well, this driver deserves to die (or a massive rewrite).
Reviewed-by: Jiri Slaby <[email protected]>
thanks,
--
js
suse labs
Hi,
This is the semi-friendly patch-bot of Greg Kroah-Hartman.
Markus, you seem to have sent a nonsensical or otherwise pointless
review comment to a patch submission on a Linux kernel developer mailing
list. I strongly suggest that you not do this anymore. Please do not
bother developers who are actively working to produce patches and
features with comments that, in the end, are a waste of time.
Patch submitter, please ignore Markus's suggestion; you do not need to
follow it at all. The person/bot/AI that sent it is being ignored by
almost all Linux kernel maintainers for having a persistent pattern of
behavior of producing distracting and pointless commentary, and
inability to adapt to feedback. Please feel free to also ignore emails
from them.
thanks,
greg k-h's patch email bot