| Message ID | cover.1649310812.git.duoming@zju.edu.cn |
|---|---|
| Headers | show |
| Series | Fix deadlocks caused by del_timer_sync() | expand |
On Thu, Apr 07, 2022 at 02:37:12PM +0800, Duoming Zhou wrote: > There is a deadlock in irdma_cleanup_cm_core(), which is shown > below: > > (Thread 1) | (Thread 2) > | irdma_schedule_cm_timer() > irdma_cleanup_cm_core() | add_timer() > spin_lock_irqsave() //(1) | (wait a time) > ... | irdma_cm_timer_tick() > del_timer_sync() | spin_lock_irqsave() //(2) > (wait timer to stop) | ... > > We hold cm_core->ht_lock in position (1) of thread 1 and > use del_timer_sync() to wait timer to stop, but timer handler > also need cm_core->ht_lock in position (2) of thread 2. > As a result, irdma_cleanup_cm_core() will block forever. > > This patch extracts del_timer_sync() from the protection of > spin_lock_irqsave(), which could let timer handler to obtain > the needed lock. > > Signed-off-by: Duoming Zhou <duoming@zju.edu.cn> > --- > drivers/infiniband/hw/irdma/cm.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/infiniband/hw/irdma/cm.c b/drivers/infiniband/hw/irdma/cm.c > index dedb3b7edd8..019dd8bfe08 100644 > --- a/drivers/infiniband/hw/irdma/cm.c > +++ b/drivers/infiniband/hw/irdma/cm.c > @@ -3252,8 +3252,11 @@ void irdma_cleanup_cm_core(struct irdma_cm_core *cm_core) > return; > > spin_lock_irqsave(&cm_core->ht_lock, flags); > - if (timer_pending(&cm_core->tcp_timer)) > + if (timer_pending(&cm_core->tcp_timer)) { > + spin_unlock_irqrestore(&cm_core->ht_lock, flags); > del_timer_sync(&cm_core->tcp_timer); > + spin_lock_irqsave(&cm_core->ht_lock, flags); > + } > spin_unlock_irqrestore(&cm_core->ht_lock, flags); This lock doesn't seem to be protecting anything. Also do we need to check timer_pending()? I think the del_timer_sync() function will just return directly if there isn't a pending lock? regards, dan carpenter
Hello, On Thu, 7 Apr 2022 14:24:56 +0300 Dan Carpenter wrote: > > There is a deadlock in irdma_cleanup_cm_core(), which is shown > > below: > > > > (Thread 1) | (Thread 2) > > | irdma_schedule_cm_timer() > > irdma_cleanup_cm_core() | add_timer() > > spin_lock_irqsave() //(1) | (wait a time) > > ... | irdma_cm_timer_tick() > > del_timer_sync() | spin_lock_irqsave() //(2) > > (wait timer to stop) | ... > > > > We hold cm_core->ht_lock in position (1) of thread 1 and > > use del_timer_sync() to wait timer to stop, but timer handler > > also need cm_core->ht_lock in position (2) of thread 2. > > As a result, irdma_cleanup_cm_core() will block forever. > > > > This patch extracts del_timer_sync() from the protection of > > spin_lock_irqsave(), which could let timer handler to obtain > > the needed lock. > > > > Signed-off-by: Duoming Zhou <duoming@zju.edu.cn> > > --- > > drivers/infiniband/hw/irdma/cm.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/infiniband/hw/irdma/cm.c b/drivers/infiniband/hw/irdma/cm.c > > index dedb3b7edd8..019dd8bfe08 100644 > > --- a/drivers/infiniband/hw/irdma/cm.c > > +++ b/drivers/infiniband/hw/irdma/cm.c > > @@ -3252,8 +3252,11 @@ void irdma_cleanup_cm_core(struct irdma_cm_core *cm_core) > > return; > > > > spin_lock_irqsave(&cm_core->ht_lock, flags); > > - if (timer_pending(&cm_core->tcp_timer)) > > + if (timer_pending(&cm_core->tcp_timer)) { > > + spin_unlock_irqrestore(&cm_core->ht_lock, flags); > > del_timer_sync(&cm_core->tcp_timer); > > + spin_lock_irqsave(&cm_core->ht_lock, flags); > > + } > > spin_unlock_irqrestore(&cm_core->ht_lock, flags); > > This lock doesn't seem to be protecting anything. Also do we need to > check timer_pending()? I think the del_timer_sync() function will just > return directly if there isn't a pending lock? Thanks a lot for your advice, I will remove the timer_pending() and the redundant lock. Best regards, Duoming Zhou
Hello, On Thu, 7 Apr 2022 11:23:55 -0300 Jason Gunthorpe wrote: > > > > diff --git a/drivers/infiniband/hw/irdma/cm.c b/drivers/infiniband/hw/irdma/cm.c > > > > index dedb3b7edd8..019dd8bfe08 100644 > > > > +++ b/drivers/infiniband/hw/irdma/cm.c > > > > @@ -3252,8 +3252,11 @@ void irdma_cleanup_cm_core(struct irdma_cm_core *cm_core) > > > > return; > > > > > > > > spin_lock_irqsave(&cm_core->ht_lock, flags); > > > > - if (timer_pending(&cm_core->tcp_timer)) > > > > + if (timer_pending(&cm_core->tcp_timer)) { > > > > + spin_unlock_irqrestore(&cm_core->ht_lock, flags); > > > > del_timer_sync(&cm_core->tcp_timer); > > > > + spin_lock_irqsave(&cm_core->ht_lock, flags); > > > > + } > > > > spin_unlock_irqrestore(&cm_core->ht_lock, flags); > > > > > > This lock doesn't seem to be protecting anything. Also do we need to > > > check timer_pending()? I think the del_timer_sync() function will just > > > return directly if there isn't a pending lock? > > > > Thanks a lot for your advice, I will remove the timer_pending() and the > > redundant lock. > > Does del_timer_sync work with a self-rescheduling timer like this has? The del_timer_sync() will kill the timer although it is self-rescheduling. We could use other functions to arouse timer again besides timer handler itself. Best regards, Duoming Zhou
On Thu, Apr 07, 2022 at 08:54:13PM +0800, duoming@zju.edu.cn wrote: > Hello, > > On Thu, 7 Apr 2022 14:24:56 +0300 Dan Carpenter wrote: > > > > There is a deadlock in irdma_cleanup_cm_core(), which is shown > > > below: > > > > > > (Thread 1) | (Thread 2) > > > | irdma_schedule_cm_timer() > > > irdma_cleanup_cm_core() | add_timer() > > > spin_lock_irqsave() //(1) | (wait a time) > > > ... | irdma_cm_timer_tick() > > > del_timer_sync() | spin_lock_irqsave() //(2) > > > (wait timer to stop) | ... > > > > > > We hold cm_core->ht_lock in position (1) of thread 1 and > > > use del_timer_sync() to wait timer to stop, but timer handler > > > also need cm_core->ht_lock in position (2) of thread 2. > > > As a result, irdma_cleanup_cm_core() will block forever. > > > > > > This patch extracts del_timer_sync() from the protection of > > > spin_lock_irqsave(), which could let timer handler to obtain > > > the needed lock. > > > > > > Signed-off-by: Duoming Zhou <duoming@zju.edu.cn> > > > --- > > > drivers/infiniband/hw/irdma/cm.c | 5 ++++- > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/infiniband/hw/irdma/cm.c b/drivers/infiniband/hw/irdma/cm.c > > > index dedb3b7edd8..019dd8bfe08 100644 > > > --- a/drivers/infiniband/hw/irdma/cm.c > > > +++ b/drivers/infiniband/hw/irdma/cm.c > > > @@ -3252,8 +3252,11 @@ void irdma_cleanup_cm_core(struct irdma_cm_core *cm_core) > > > return; > > > > > > spin_lock_irqsave(&cm_core->ht_lock, flags); > > > - if (timer_pending(&cm_core->tcp_timer)) > > > + if (timer_pending(&cm_core->tcp_timer)) { > > > + spin_unlock_irqrestore(&cm_core->ht_lock, flags); > > > del_timer_sync(&cm_core->tcp_timer); > > > + spin_lock_irqsave(&cm_core->ht_lock, flags); > > > + } > > > spin_unlock_irqrestore(&cm_core->ht_lock, flags); > > > > This lock doesn't seem to be protecting anything. Also do we need to > > check timer_pending()? I think the del_timer_sync() function will just > > return directly if there isn't a pending lock? > > Thanks a lot for your advice, I will remove the timer_pending() and the > redundant lock. I didn't give any advice. :P I only ask questions when I don't know the answers. Someone probably needs to look at &cm_core->ht_lock and figure out what it's protecting. regards, dan carpenter
> Subject: Re: Re: Re: [PATCH 09/11] drivers: infiniband: hw: Fix deadlock in > irdma_cleanup_cm_core() > > Hello, > > On Thu, 7 Apr 2022 11:23:55 -0300 Jason Gunthorpe wrote: > > > > > > diff --git a/drivers/infiniband/hw/irdma/cm.c > > > > > b/drivers/infiniband/hw/irdma/cm.c > > > > > index dedb3b7edd8..019dd8bfe08 100644 > > > > > +++ b/drivers/infiniband/hw/irdma/cm.c > > > > > @@ -3252,8 +3252,11 @@ void irdma_cleanup_cm_core(struct > irdma_cm_core *cm_core) > > > > > return; > > > > > > > > > > spin_lock_irqsave(&cm_core->ht_lock, flags); > > > > > - if (timer_pending(&cm_core->tcp_timer)) > > > > > + if (timer_pending(&cm_core->tcp_timer)) { > > > > > + spin_unlock_irqrestore(&cm_core->ht_lock, flags); > > > > > del_timer_sync(&cm_core->tcp_timer); > > > > > + spin_lock_irqsave(&cm_core->ht_lock, flags); > > > > > + } > > > > > spin_unlock_irqrestore(&cm_core->ht_lock, flags); > > > > > > > > This lock doesn't seem to be protecting anything. Also do we need > > > > to check timer_pending()? I think the del_timer_sync() function > > > > will just return directly if there isn't a pending lock? > > > > > > Thanks a lot for your advice, I will remove the timer_pending() and > > > the redundant lock. > > > > Does del_timer_sync work with a self-rescheduling timer like this has? > > The del_timer_sync() will kill the timer although it is self-rescheduling. > We could use other functions to arouse timer again besides timer handler itself. > By the time we execute, irdma_cleanup_cm_core all cm_nodes should be culled and there will be no timer add from the timer handler. And the secondary path to add timer, irdma_schedule_timer is guaranteed to not run.
Hello, On Thu, 7 Apr 2022 17:36:12 +0000 Saleem, Shiraz wrote: > > Subject: Re: Re: [PATCH 09/11] drivers: infiniband: hw: Fix deadlock in > > irdma_cleanup_cm_core() > > > > On Thu, Apr 07, 2022 at 08:54:13PM +0800, duoming@zju.edu.cn wrote: > > > Hello, > > > > > > On Thu, 7 Apr 2022 14:24:56 +0300 Dan Carpenter wrote: > > > > > > > > There is a deadlock in irdma_cleanup_cm_core(), which is shown > > > > > below: > > > > > > > > > > (Thread 1) | (Thread 2) > > > > > | irdma_schedule_cm_timer() > > > > > irdma_cleanup_cm_core() | add_timer() > > > > > spin_lock_irqsave() //(1) | (wait a time) > > > > > ... | irdma_cm_timer_tick() > > > > > del_timer_sync() | spin_lock_irqsave() //(2) > > > > > (wait timer to stop) | ... > > > > > > > > > > We hold cm_core->ht_lock in position (1) of thread 1 and use > > > > > del_timer_sync() to wait timer to stop, but timer handler also > > > > > need cm_core->ht_lock in position (2) of thread 2. > > > > > As a result, irdma_cleanup_cm_core() will block forever. > > > > > > > > > > This patch extracts del_timer_sync() from the protection of > > > > > spin_lock_irqsave(), which could let timer handler to obtain the > > > > > needed lock. > > > > > > > > > > Signed-off-by: Duoming Zhou <duoming@zju.edu.cn> > > > > > --- > > > > > drivers/infiniband/hw/irdma/cm.c | 5 ++++- > > > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/infiniband/hw/irdma/cm.c > > > > > b/drivers/infiniband/hw/irdma/cm.c > > > > > index dedb3b7edd8..019dd8bfe08 100644 > > > > > --- a/drivers/infiniband/hw/irdma/cm.c > > > > > +++ b/drivers/infiniband/hw/irdma/cm.c > > > > > @@ -3252,8 +3252,11 @@ void irdma_cleanup_cm_core(struct > > irdma_cm_core *cm_core) > > > > > return; > > > > > > > > > > spin_lock_irqsave(&cm_core->ht_lock, flags); > > > > > - if (timer_pending(&cm_core->tcp_timer)) > > > > > + if (timer_pending(&cm_core->tcp_timer)) { > > > > > + spin_unlock_irqrestore(&cm_core->ht_lock, flags); > > > > > del_timer_sync(&cm_core->tcp_timer); > > > > > + spin_lock_irqsave(&cm_core->ht_lock, flags); > > > > > + } > > > > > spin_unlock_irqrestore(&cm_core->ht_lock, flags); > > > > > > > > This lock doesn't seem to be protecting anything. Also do we need > > > > to check timer_pending()? I think the del_timer_sync() function > > > > will just return directly if there isn't a pending lock? > > > > > > Thanks a lot for your advice, I will remove the timer_pending() and > > > the redundant lock. > > > > I didn't give any advice. :P I only ask questions when I don't know the answers. > > Someone probably needs to look at &cm_core->ht_lock and figure out what it's > > protecting. > > > Agreed on this fix. > > We should not lock around del_timer_sync or need to check on timer_pending. > > However, we do need serialize addition of a timer which can be called from multiple paths, i.e. the timer handler and irdma_schedule_cm_timer. I think we should replace the check "if (!timer_pending(&cm_core->tcp_timer))" to "if (timer_pending(&cm_core->tcp_timer))" in irdma_cm_timer_tick(), and replace "if (!was_timer_set)" to "if (was_timer_set)" in irdma_schedule_cm_timer() in order to guarantee the timer could be executed. I will send the modified patch as soon as possible. Best regards, Duoming Zhou
> Subject: Re: RE: Re: [PATCH 09/11] drivers: infiniband: hw: Fix deadlock in > irdma_cleanup_cm_core() > > Hello, > > On Thu, 7 Apr 2022 17:36:12 +0000 Saleem, Shiraz wrote: > > > > Subject: Re: Re: [PATCH 09/11] drivers: infiniband: hw: Fix deadlock > > > in > > > irdma_cleanup_cm_core() > > > > > > On Thu, Apr 07, 2022 at 08:54:13PM +0800, duoming@zju.edu.cn wrote: > > > > Hello, > > > > > > > > On Thu, 7 Apr 2022 14:24:56 +0300 Dan Carpenter wrote: > > > > > > > > > > There is a deadlock in irdma_cleanup_cm_core(), which is shown > > > > > > below: > > > > > > > > > > > > (Thread 1) | (Thread 2) > > > > > > | irdma_schedule_cm_timer() > > > > > > irdma_cleanup_cm_core() | add_timer() > > > > > > spin_lock_irqsave() //(1) | (wait a time) > > > > > > ... | irdma_cm_timer_tick() > > > > > > del_timer_sync() | spin_lock_irqsave() //(2) > > > > > > (wait timer to stop) | ... > > > > > > > > > > > > We hold cm_core->ht_lock in position (1) of thread 1 and use > > > > > > del_timer_sync() to wait timer to stop, but timer handler also > > > > > > need cm_core->ht_lock in position (2) of thread 2. > > > > > > As a result, irdma_cleanup_cm_core() will block forever. > > > > > > > > > > > > This patch extracts del_timer_sync() from the protection of > > > > > > spin_lock_irqsave(), which could let timer handler to obtain > > > > > > the needed lock. > > > > > > > > > > > > Signed-off-by: Duoming Zhou <duoming@zju.edu.cn> > > > > > > --- > > > > > > drivers/infiniband/hw/irdma/cm.c | 5 ++++- > > > > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/drivers/infiniband/hw/irdma/cm.c > > > > > > b/drivers/infiniband/hw/irdma/cm.c > > > > > > index dedb3b7edd8..019dd8bfe08 100644 > > > > > > --- a/drivers/infiniband/hw/irdma/cm.c > > > > > > +++ b/drivers/infiniband/hw/irdma/cm.c > > > > > > @@ -3252,8 +3252,11 @@ void irdma_cleanup_cm_core(struct > > > irdma_cm_core *cm_core) > > > > > > return; > > > > > > > > > > > > spin_lock_irqsave(&cm_core->ht_lock, flags); > > > > > > - if (timer_pending(&cm_core->tcp_timer)) > > > > > > + if (timer_pending(&cm_core->tcp_timer)) { > > > > > > + spin_unlock_irqrestore(&cm_core->ht_lock, flags); > > > > > > del_timer_sync(&cm_core->tcp_timer); > > > > > > + spin_lock_irqsave(&cm_core->ht_lock, flags); > > > > > > + } > > > > > > spin_unlock_irqrestore(&cm_core->ht_lock, flags); > > > > > > > > > > This lock doesn't seem to be protecting anything. Also do we > > > > > need to check timer_pending()? I think the del_timer_sync() > > > > > function will just return directly if there isn't a pending lock? > > > > > > > > Thanks a lot for your advice, I will remove the timer_pending() > > > > and the redundant lock. > > > > > > I didn't give any advice. :P I only ask questions when I don't know the answers. > > > Someone probably needs to look at &cm_core->ht_lock and figure out > > > what it's protecting. > > > > > Agreed on this fix. > > > > We should not lock around del_timer_sync or need to check on timer_pending. > > > > However, we do need serialize addition of a timer which can be called from > multiple paths, i.e. the timer handler and irdma_schedule_cm_timer. > > I think we should replace the check "if (!timer_pending(&cm_core->tcp_timer))" to > "if (timer_pending(&cm_core->tcp_timer))" in irdma_cm_timer_tick(), and replace "if > (!was_timer_set)" to "if (was_timer_set)" in irdma_schedule_cm_timer() in order to > guarantee the timer could be executed. I will send the modified patch as soon as > possible. > No we don’t arm the timer if there's is one pending. Its also a bug to do so. https://elixir.bootlin.com/linux/v5.18-rc1/source/kernel/time/timer.c#L1143
Hello, On Fri, 8 Apr 2022 02:21:59 +0000 Saleem, Shiraz wrote: > > > > > > > There is a deadlock in irdma_cleanup_cm_core(), which is shown > > > > > > > below: > > > > > > > > > > > > > > (Thread 1) | (Thread 2) > > > > > > > | irdma_schedule_cm_timer() > > > > > > > irdma_cleanup_cm_core() | add_timer() > > > > > > > spin_lock_irqsave() //(1) | (wait a time) > > > > > > > ... | irdma_cm_timer_tick() > > > > > > > del_timer_sync() | spin_lock_irqsave() //(2) > > > > > > > (wait timer to stop) | ... > > > > > > > > > > > > > > We hold cm_core->ht_lock in position (1) of thread 1 and use > > > > > > > del_timer_sync() to wait timer to stop, but timer handler also > > > > > > > need cm_core->ht_lock in position (2) of thread 2. > > > > > > > As a result, irdma_cleanup_cm_core() will block forever. > > > > > > > > > > > > > > This patch extracts del_timer_sync() from the protection of > > > > > > > spin_lock_irqsave(), which could let timer handler to obtain > > > > > > > the needed lock. > > > > > > > > > > > > > > Signed-off-by: Duoming Zhou <duoming@zju.edu.cn> > > > > > > > --- > > > > > > > drivers/infiniband/hw/irdma/cm.c | 5 ++++- > > > > > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > diff --git a/drivers/infiniband/hw/irdma/cm.c > > > > > > > b/drivers/infiniband/hw/irdma/cm.c > > > > > > > index dedb3b7edd8..019dd8bfe08 100644 > > > > > > > --- a/drivers/infiniband/hw/irdma/cm.c > > > > > > > +++ b/drivers/infiniband/hw/irdma/cm.c > > > > > > > @@ -3252,8 +3252,11 @@ void irdma_cleanup_cm_core(struct > > > > irdma_cm_core *cm_core) > > > > > > > return; > > > > > > > > > > > > > > spin_lock_irqsave(&cm_core->ht_lock, flags); > > > > > > > - if (timer_pending(&cm_core->tcp_timer)) > > > > > > > + if (timer_pending(&cm_core->tcp_timer)) { > > > > > > > + spin_unlock_irqrestore(&cm_core->ht_lock, flags); > > > > > > > del_timer_sync(&cm_core->tcp_timer); > > > > > > > + spin_lock_irqsave(&cm_core->ht_lock, flags); > > > > > > > + } > > > > > > > spin_unlock_irqrestore(&cm_core->ht_lock, flags); > > > > > > > > > > > > This lock doesn't seem to be protecting anything. Also do we > > > > > > need to check timer_pending()? I think the del_timer_sync() > > > > > > function will just return directly if there isn't a pending lock? > > > > > > > > > > Thanks a lot for your advice, I will remove the timer_pending() > > > > > and the redundant lock. > > > > > > > > I didn't give any advice. :P I only ask questions when I don't know the answers. > > > > Someone probably needs to look at &cm_core->ht_lock and figure out > > > > what it's protecting. > > > > > > > Agreed on this fix. > > > > > > We should not lock around del_timer_sync or need to check on timer_pending. > > > > > > However, we do need serialize addition of a timer which can be called from > > multiple paths, i.e. the timer handler and irdma_schedule_cm_timer. > > > > I think we should replace the check "if (!timer_pending(&cm_core->tcp_timer))" to > > "if (timer_pending(&cm_core->tcp_timer))" in irdma_cm_timer_tick(), and replace "if > > (!was_timer_set)" to "if (was_timer_set)" in irdma_schedule_cm_timer() in order to > > guarantee the timer could be executed. I will send the modified patch as soon as > > possible. > > > > No we don’t arm the timer if there's is one pending. Its also a bug to do so. > > https://elixir.bootlin.com/linux/v5.18-rc1/source/kernel/time/timer.c#L1143 You are right, I think we could add "mod_timer" in irdma_schedule_cm_timer and irdma_cm_timer_tick() in order to start timer. I will send [PATCH V4 09/11] drivers: infiniband: hw: Fix deadlock in irdma_cleanup_cm_core(). Best regards, Duoming Zhou
If the timer handlers need a lock owned by the thread calling del_timer_sync(), then, the caller thread will block forever. Duoming Zhou (11): drivers: tty: serial: Fix deadlock in sa1100_set_termios() drivers: usb: host: Fix deadlock in oxu_bus_suspend() drivers: staging: rtl8192u: Fix deadlock in ieee80211_beacons_stop() drivers: staging: rtl8723bs: Fix deadlock in rtw_surveydone_event_callback() drivers: staging: rtl8192e: Fix deadlock in rtllib_beacons_stop() drivers: staging: rtl8192e: Fix deadlock in rtw_joinbss_event_prehandle() drivers: net: hippi: Fix deadlock in rr_close() drivers: net: can: Fix deadlock in grcan_close() drivers: infiniband: hw: Fix deadlock in irdma_cleanup_cm_core() arch: xtensa: platforms: Fix deadlock in iss_net_close() arch: xtensa: platforms: Fix deadlock in rs_close() arch/xtensa/platforms/iss/console.c | 4 +++- arch/xtensa/platforms/iss/network.c | 2 ++ drivers/infiniband/hw/irdma/cm.c | 5 ++++- drivers/net/can/grcan.c | 2 ++ drivers/net/hippi/rrunner.c | 2 ++ drivers/staging/rtl8192e/rtllib_softmac.c | 2 +- drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c | 2 +- drivers/staging/rtl8723bs/core/rtw_mlme.c | 4 ++++ drivers/tty/serial/sa1100.c | 2 ++ drivers/usb/host/oxu210hp-hcd.c | 2 ++ 10 files changed, 23 insertions(+), 4 deletions(-)