[PATCH 1/2] hp100: Fix a possible sleep-in-atomic bug in hp100_login_to_vg_hub

[PATCH 1/2] hp100: Fix a possible sleep-in-atomic bug in hp100_login_to_vg_hub

[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

Re: [PATCH 1/2] hp100: Fix a possible sleep-in-atomic bug in hp100_login_to_vg_hub

[David Miller]

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.

Re: [PATCH 1/2] hp100: Fix a possible sleep-in-atomic bug in hp100_login_to_vg_hub

[Jia-Ju Bai]

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.

Re: [PATCH 1/2] hp100: Fix a possible sleep-in-atomic bug in hp100_login_to_vg_hub

[Jia-Ju Bai]

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.
>

Re: [PATCH 1/2] hp100: Fix a possible sleep-in-atomic bug in hp100_login_to_vg_hub

[David Miller]

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.

Re: [PATCH 1/2] hp100: Fix a possible sleep-in-atomic bug in hp100_login_to_vg_hub

[Jia-Ju Bai]

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.

Re: [PATCH 1/2] hp100: Fix a possible sleep-in-atomic bug in hp100_login_to_vg_hub

[Siegfried Loeffler]

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.
>
>

Re: [PATCH 1/2] hp100: Fix a possible sleep-in-atomic bug in hp100_login_to_vg_hub

[Joe Perches]

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.