| Message ID | 1513158468-14382-1-git-send-email-baijiaju1990@gmail.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [1/2] hp100: Fix a possible sleep-in-atomic bug in hp100_login_to_vg_hub | expand |
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.
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.
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.
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. >
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.
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.
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. > >
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)); }
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(-)