* [PATCH 1/2] hp100: Fix a possible sleep-in-atomic bug in hp100_login_to_vg_hub
@ 2017-12-13 9:47 Jia-Ju Bai
2017-12-13 21:20 ` David Miller
2017-12-14 1:21 ` Joe Perches
0 siblings, 2 replies; 8+ messages in thread
From: Jia-Ju Bai @ 2017-12-13 9:47 UTC (permalink / raw)
To: perex, floeff, acme; +Cc: netdev, linux-kernel, Jia-Ju Bai
The driver may sleep under a spinlock.
The function call path is:
hp100_set_multicast_list (acquire the spinlock)
hp100_login_to_vg_hub
schedule_timeout_interruptible --> may sleep
To fix it, schedule_timeout_interruptible is replaced with udelay.
This bug is found by my static analysis tool(DSAC) and checked by my code review.
Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
---
drivers/net/ethernet/hp/hp100.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/hp/hp100.c b/drivers/net/ethernet/hp/hp100.c
index c8c7ad2..6addcbd 100644
--- a/drivers/net/ethernet/hp/hp100.c
+++ b/drivers/net/ethernet/hp/hp100.c
@@ -2636,8 +2636,7 @@ static int hp100_login_to_vg_hub(struct net_device *dev, u_short force_relogin)
do {
if (~(hp100_inb(VG_LAN_CFG_1) & HP100_LINK_UP_ST))
break;
- if (!in_interrupt())
- schedule_timeout_interruptible(1);
+ udelay(10);
} while (time_after(time, jiffies));
/* Start an addressed training and optionally request promiscuous port */
@@ -2672,8 +2671,7 @@ static int hp100_login_to_vg_hub(struct net_device *dev, u_short force_relogin)
do {
if (hp100_inb(VG_LAN_CFG_1) & HP100_LINK_CABLE_ST)
break;
- if (!in_interrupt())
- schedule_timeout_interruptible(1);
+ udelay(10);
} while (time_before(jiffies, time));
if (time_after_eq(jiffies, time)) {
@@ -2696,8 +2694,7 @@ static int hp100_login_to_vg_hub(struct net_device *dev, u_short force_relogin)
#endif
break;
}
- if (!in_interrupt())
- schedule_timeout_interruptible(1);
+ udelay(10);
} while (time_after(time, jiffies));
}
--
1.7.9.5
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] hp100: Fix a possible sleep-in-atomic bug in hp100_login_to_vg_hub
2017-12-13 9:47 [PATCH 1/2] hp100: Fix a possible sleep-in-atomic bug in hp100_login_to_vg_hub Jia-Ju Bai
@ 2017-12-13 21:20 ` David Miller
2017-12-14 3:13 ` Jia-Ju Bai
2017-12-14 1:21 ` Joe Perches
1 sibling, 1 reply; 8+ messages in thread
From: David Miller @ 2017-12-13 21:20 UTC (permalink / raw)
To: baijiaju1990; +Cc: perex, floeff, acme, netdev, linux-kernel
I want you to review all of your patches and resend them after you
have checked them carefully.
The first patch I even looked at, this one, is buggy.
You changed a schedule_timeout_interruptible(1) into a udelay(10)
That's not right.
schedule_timeout_interruptible() takes a "jiffies" argument, which
is a completely different unit than udelay() takes. You would have
to scale the argument to udelay() in some way using HZ.
Furthermore, the udelay argument you would come up with would
be way too long to be appropirate in this atomic context.
That's why the code tries to use a sleeping timeout, a long wait is
necessary here.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] hp100: Fix a possible sleep-in-atomic bug in hp100_login_to_vg_hub
2017-12-13 9:47 [PATCH 1/2] hp100: Fix a possible sleep-in-atomic bug in hp100_login_to_vg_hub Jia-Ju Bai
2017-12-13 21:20 ` David Miller
@ 2017-12-14 1:21 ` Joe Perches
1 sibling, 0 replies; 8+ messages in thread
From: Joe Perches @ 2017-12-14 1:21 UTC (permalink / raw)
To: Jia-Ju Bai, perex, floeff, acme; +Cc: netdev, linux-kernel
On Wed, 2017-12-13 at 17:47 +0800, Jia-Ju Bai wrote:
> The driver may sleep under a spinlock.
> The function call path is:
> hp100_set_multicast_list (acquire the spinlock)
> hp100_login_to_vg_hub
> schedule_timeout_interruptible --> may sleep
>
> To fix it, schedule_timeout_interruptible is replaced with udelay.
>
> This bug is found by my static analysis tool(DSAC) and checked by my code review.
Are there any current working VG/AnyLan network installations?
I rather doubt it.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] hp100: Fix a possible sleep-in-atomic bug in hp100_login_to_vg_hub
2017-12-13 21:20 ` David Miller
@ 2017-12-14 3:13 ` Jia-Ju Bai
2017-12-14 3:31 ` Jia-Ju Bai
2017-12-14 3:34 ` David Miller
0 siblings, 2 replies; 8+ messages in thread
From: Jia-Ju Bai @ 2017-12-14 3:13 UTC (permalink / raw)
To: David Miller; +Cc: perex, floeff, acme, netdev, linux-kernel
Thanks for reply :)
I think I should use "udelay(100000/HZ)" instead, do you think it is right?
Thanks,
Jia-Ju Bai
On 2017/12/14 5:20, David Miller wrote:
> I want you to review all of your patches and resend them after you
> have checked them carefully.
>
> The first patch I even looked at, this one, is buggy.
>
> You changed a schedule_timeout_interruptible(1) into a udelay(10)
>
> That's not right.
>
> schedule_timeout_interruptible() takes a "jiffies" argument, which
> is a completely different unit than udelay() takes. You would have
> to scale the argument to udelay() in some way using HZ.
>
> Furthermore, the udelay argument you would come up with would
> be way too long to be appropirate in this atomic context.
>
> That's why the code tries to use a sleeping timeout, a long wait is
> necessary here.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] hp100: Fix a possible sleep-in-atomic bug in hp100_login_to_vg_hub
2017-12-14 3:13 ` Jia-Ju Bai
@ 2017-12-14 3:31 ` Jia-Ju Bai
2017-12-14 3:34 ` David Miller
1 sibling, 0 replies; 8+ messages in thread
From: Jia-Ju Bai @ 2017-12-14 3:31 UTC (permalink / raw)
To: David Miller; +Cc: perex, floeff, acme, netdev, linux-kernel
Sorry, I made a mistake in last e-mail.
Maybe "mdelay(1000/HZ)" or "udelay(1000000/HZ)" .
Which one do you think is right?
Thanks,
Jia-Ju Bai
On 2017/12/14 11:13, Jia-Ju Bai wrote:
> Thanks for reply :)
> I think I should use "udelay(100000/HZ)" instead, do you think it is
> right?
>
>
> Thanks,
> Jia-Ju Bai
>
>
> On 2017/12/14 5:20, David Miller wrote:
>> I want you to review all of your patches and resend them after you
>> have checked them carefully.
>>
>> The first patch I even looked at, this one, is buggy.
>>
>> You changed a schedule_timeout_interruptible(1) into a udelay(10)
>>
>> That's not right.
>>
>> schedule_timeout_interruptible() takes a "jiffies" argument, which
>> is a completely different unit than udelay() takes. You would have
>> to scale the argument to udelay() in some way using HZ.
>>
>> Furthermore, the udelay argument you would come up with would
>> be way too long to be appropirate in this atomic context.
>>
>> That's why the code tries to use a sleeping timeout, a long wait is
>> necessary here.
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] hp100: Fix a possible sleep-in-atomic bug in hp100_login_to_vg_hub
2017-12-14 3:13 ` Jia-Ju Bai
2017-12-14 3:31 ` Jia-Ju Bai
@ 2017-12-14 3:34 ` David Miller
2017-12-14 3:56 ` Jia-Ju Bai
1 sibling, 1 reply; 8+ messages in thread
From: David Miller @ 2017-12-14 3:34 UTC (permalink / raw)
To: baijiaju1990; +Cc: perex, floeff, acme, netdev, linux-kernel
From: Jia-Ju Bai <baijiaju1990@gmail.com>
Date: Thu, 14 Dec 2017 11:13:15 +0800
> Thanks for reply :)
> I think I should use "udelay(100000/HZ)" instead, do you think it is
> right?
The delay is too long, please do not ignore that part of my critique
of your change.
You cannot delay so long under a lock, that's why the code is trying
to use a sleeping delay.
I'm not going to explain this problem another time.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] hp100: Fix a possible sleep-in-atomic bug in hp100_login_to_vg_hub
2017-12-14 3:34 ` David Miller
@ 2017-12-14 3:56 ` Jia-Ju Bai
2017-12-15 11:13 ` Siegfried Loeffler
0 siblings, 1 reply; 8+ messages in thread
From: Jia-Ju Bai @ 2017-12-14 3:56 UTC (permalink / raw)
To: David Miller; +Cc: perex, floeff, acme, netdev, linux-kernel
Sorry,
I think I know your meaning now.
Maybe we can unlock the spinlock before "schedule_timeout_interruptible"
and then lock again?
Like:
spin_unlock(...);
schedule_timeout_interruptible(1);
spin_lock(...);
Best wishes,
Jia-Ju Bai
On 2017/12/14 11:34, David Miller wrote:
> From: Jia-Ju Bai <baijiaju1990@gmail.com>
> Date: Thu, 14 Dec 2017 11:13:15 +0800
>
>> Thanks for reply :)
>> I think I should use "udelay(100000/HZ)" instead, do you think it is
>> right?
> The delay is too long, please do not ignore that part of my critique
> of your change.
>
> You cannot delay so long under a lock, that's why the code is trying
> to use a sleeping delay.
>
> I'm not going to explain this problem another time.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] hp100: Fix a possible sleep-in-atomic bug in hp100_login_to_vg_hub
2017-12-14 3:56 ` Jia-Ju Bai
@ 2017-12-15 11:13 ` Siegfried Loeffler
0 siblings, 0 replies; 8+ messages in thread
From: Siegfried Loeffler @ 2017-12-15 11:13 UTC (permalink / raw)
To: Jia-Ju Bai, David Miller; +Cc: perex, floeff, netdev, linux-kernel
I am sorry, I still have some of these 100VGAnyLan boards somewhere in
the attic but I am unable to test.
I haven't used 100VGAnyLan for the last 20 years ! :-) - I wonder if
anybody is still using it?
Cheers
Siegfried Loeffler, DG1SEK
On 14.12.17 04:56, Jia-Ju Bai wrote:
> Sorry,
>
> I think I know your meaning now.
>
> Maybe we can unlock the spinlock before
> "schedule_timeout_interruptible" and then lock again?
> Like:
> spin_unlock(...);
> schedule_timeout_interruptible(1);
> spin_lock(...);
>
>
> Best wishes,
> Jia-Ju Bai
>
>
> On 2017/12/14 11:34, David Miller wrote:
>> From: Jia-Ju Bai <baijiaju1990@gmail.com>
>> Date: Thu, 14 Dec 2017 11:13:15 +0800
>>
>>> Thanks for reply :)
>>> I think I should use "udelay(100000/HZ)" instead, do you think it is
>>> right?
>> The delay is too long, please do not ignore that part of my critique
>> of your change.
>>
>> You cannot delay so long under a lock, that's why the code is trying
>> to use a sleeping delay.
>>
>> I'm not going to explain this problem another time.
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-12-15 11:15 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-13 9:47 [PATCH 1/2] hp100: Fix a possible sleep-in-atomic bug in hp100_login_to_vg_hub Jia-Ju Bai
2017-12-13 21:20 ` David Miller
2017-12-14 3:13 ` Jia-Ju Bai
2017-12-14 3:31 ` Jia-Ju Bai
2017-12-14 3:34 ` David Miller
2017-12-14 3:56 ` Jia-Ju Bai
2017-12-15 11:13 ` Siegfried Loeffler
2017-12-14 1:21 ` Joe Perches
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.