Re: Interrupt race in NET_LOCK/NET_UNLOCK

[Alexander Bluhm]

On Fri, Dec 23, 2016 at 11:41:00AM +0100, Martin Pieuchot wrote:
> On 23/12/16(Fri) 06:08, Visa Hankala wrote:
> > NET_LOCK() should raise IPL before acquiring the lock, and NET_UNLOCK()
> > should restore the level after releasing the lock. Otherwise, lock
> > recursion can occur, most likely right after the splx(). An example:
> > 
> > nd6_slowtimo  <- NET_LOCK()
> > timeout_run
> > softclock
> > softintr_dispatch
> > dosoftint
> > interrupt
> > k_intr
> > if_netisr  <- NET_LOCK()
> > taskq_thread
> > 
> > OK?
> 
> This should never happen.  Simply because the NET_LOCK() MUST NOT be
> taken in (soft) interrupt context.
> 
> The real problem is that nd6_slowtimo() is set twice, once with
> timeout_set_proc(9) and once with timeout_set(9).  Diff below fixes
> that.
> 
> ok?

OK bluhm@

> 
> Index: netinet6/nd6.c
> ===
> RCS file: /cvs/src/sys/netinet6/nd6.c,v
> retrieving revision 1.200
> diff -u -p -r1.200 nd6.c
> --- netinet6/nd6.c22 Dec 2016 13:39:32 -  1.200
> +++ netinet6/nd6.c23 Dec 2016 10:37:33 -
> @@ -1479,7 +1479,6 @@ nd6_slowtimo(void *ignored_arg)
>  
>   NET_LOCK(s);
>  
> - timeout_set(_slowtimo_ch, nd6_slowtimo, NULL);
>   timeout_add_sec(_slowtimo_ch, ND6_SLOWTIMER_INTERVAL);
>  
>   TAILQ_FOREACH(ifp, , if_list) {

Re: Interrupt race in NET_LOCK/NET_UNLOCK

[Mike Belopuhov]

On 23 December 2016 at 11:41, Martin Pieuchot  wrote:
> On 23/12/16(Fri) 06:08, Visa Hankala wrote:
>> NET_LOCK() should raise IPL before acquiring the lock, and NET_UNLOCK()
>> should restore the level after releasing the lock. Otherwise, lock
>> recursion can occur, most likely right after the splx(). An example:
>>
>> nd6_slowtimo  <- NET_LOCK()
>> timeout_run
>> softclock
>> softintr_dispatch
>> dosoftint
>> interrupt
>> k_intr
>> if_netisr  <- NET_LOCK()
>> taskq_thread
>>
>> OK?
>
> This should never happen.  Simply because the NET_LOCK() MUST NOT be
> taken in (soft) interrupt context.
>
> The real problem is that nd6_slowtimo() is set twice, once with
> timeout_set_proc(9) and once with timeout_set(9).  Diff below fixes
> that.
>
> ok?
>

Most definitely.

Re: Interrupt race in NET_LOCK/NET_UNLOCK

[Martin Pieuchot]

On 23/12/16(Fri) 06:08, Visa Hankala wrote:
> NET_LOCK() should raise IPL before acquiring the lock, and NET_UNLOCK()
> should restore the level after releasing the lock. Otherwise, lock
> recursion can occur, most likely right after the splx(). An example:
> 
> nd6_slowtimo  <- NET_LOCK()
> timeout_run
> softclock
> softintr_dispatch
> dosoftint
> interrupt
> k_intr
> if_netisr  <- NET_LOCK()
> taskq_thread
> 
> OK?

This should never happen.  Simply because the NET_LOCK() MUST NOT be
taken in (soft) interrupt context.

The real problem is that nd6_slowtimo() is set twice, once with
timeout_set_proc(9) and once with timeout_set(9).  Diff below fixes
that.

ok?

Index: netinet6/nd6.c
===
RCS file: /cvs/src/sys/netinet6/nd6.c,v
retrieving revision 1.200
diff -u -p -r1.200 nd6.c
--- netinet6/nd6.c  22 Dec 2016 13:39:32 -  1.200
+++ netinet6/nd6.c  23 Dec 2016 10:37:33 -
@@ -1479,7 +1479,6 @@ nd6_slowtimo(void *ignored_arg)
 
NET_LOCK(s);
 
-   timeout_set(_slowtimo_ch, nd6_slowtimo, NULL);
timeout_add_sec(_slowtimo_ch, ND6_SLOWTIMER_INTERVAL);
 
TAILQ_FOREACH(ifp, , if_list) {

Interrupt race in NET_LOCK/NET_UNLOCK

[Visa Hankala]

NET_LOCK() should raise IPL before acquiring the lock, and NET_UNLOCK()
should restore the level after releasing the lock. Otherwise, lock
recursion can occur, most likely right after the splx(). An example:

nd6_slowtimo  <- NET_LOCK()
timeout_run
softclock
softintr_dispatch
dosoftint
interrupt
k_intr
if_netisr  <- NET_LOCK()
taskq_thread

OK?

Index: sys/systm.h
===
RCS file: src/sys/sys/systm.h,v
retrieving revision 1.120
diff -u -p -r1.120 systm.h
--- sys/systm.h 19 Dec 2016 08:36:50 -  1.120
+++ sys/systm.h 23 Dec 2016 05:59:26 -
@@ -297,14 +297,14 @@ extern struct rwlock netlock;
 
 #defineNET_LOCK(s) 
\
 do {   \
-   rw_enter_write();   \
s = splsoftnet();   \
+   rw_enter_write();   \
 } while (0)
 
-#defineNET_UNLOCK(s)   \
+#defineNET_UNLOCK(s)   
\
 do {   \
-   splx(s);\
rw_exit_write();\
+   splx(s);\
 } while (0)
 
 #defineNET_ASSERT_LOCKED() 
\