frag6 splassert net lock

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

frag6 splassert net lock

Alexander Bluhm
Hi,

On my regress test machine I see this trace:

splassert: rt_match: want 2 have 0
Starting stack trace...
rt_match(2,0,d0bb032e,604a56c3) at rt_match+0x45
rt_match(f5475540,0,0,0) at rt_match+0x45
rtalloc(f5475540,0,0) at rtalloc+0x13
icmp6_reflect(d818de00,28) at icmp6_reflect+0x1f7
icmp6_error(d8181900,3,1,0) at icmp6_error+0x39a
frag6_freef(d717b0b4) at frag6_freef+0xb0
frag6_slowtimo() at frag6_slowtimo+0x24c
pfslowtimo(d0c4cf78) at pfslowtimo+0x40
softclock_thread(d77f5590) at softclock_thread+0xc2
End of stack trace.

It is triggered by regress/sys/netinet6/frag6.  A net lock is missing
when we send the icmp6 reply.

ok?

bluhm

Index: netinet6/frag6.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/frag6.c,v
retrieving revision 1.80
diff -u -p -r1.80 frag6.c
--- netinet6/frag6.c 13 Nov 2017 07:16:35 -0000 1.80
+++ netinet6/frag6.c 14 Nov 2017 12:00:37 -0000
@@ -593,8 +593,12 @@ frag6_slowtimo(void)
 
  mtx_leave(&frag6_mutex);
 
- while ((q6 = TAILQ_FIRST(&rmq6)) != NULL) {
- TAILQ_REMOVE(&rmq6, q6, ip6q_queue);
- frag6_freef(q6);
+ if (!TAILQ_EMPTY(&rmq6)) {
+ NET_LOCK();
+ while ((q6 = TAILQ_FIRST(&rmq6)) != NULL) {
+ TAILQ_REMOVE(&rmq6, q6, ip6q_queue);
+ frag6_freef(q6);
+ }
+ NET_UNLOCK();
  }
 }

Reply | Threaded
Open this post in threaded view
|

Re: frag6 splassert net lock

Alexandr Nedvedicky
Hello,

I would go for NET_RLOCK() instead of NET_LOCK(), which is currently alias
to NET_WLOCK(). My point is the icmp6_reflect(), which is the workhorse for
icmp6_error(), is a typical 'READER-user' of network stack. It does not
change any network configuration. So it should be fine to let it run
as a reader.

I'm sure mpi@ can provide authoritative OK here as nothing should go wrong
with pure NET_LOCK() you have in your diff.

regards
sasha

On Tue, Nov 14, 2017 at 01:44:18PM +0100, Alexander Bluhm wrote:

> Hi,
>
> On my regress test machine I see this trace:
>
> splassert: rt_match: want 2 have 0
> Starting stack trace...
> rt_match(2,0,d0bb032e,604a56c3) at rt_match+0x45
> rt_match(f5475540,0,0,0) at rt_match+0x45
> rtalloc(f5475540,0,0) at rtalloc+0x13
> icmp6_reflect(d818de00,28) at icmp6_reflect+0x1f7
> icmp6_error(d8181900,3,1,0) at icmp6_error+0x39a
> frag6_freef(d717b0b4) at frag6_freef+0xb0
> frag6_slowtimo() at frag6_slowtimo+0x24c
> pfslowtimo(d0c4cf78) at pfslowtimo+0x40
> softclock_thread(d77f5590) at softclock_thread+0xc2
> End of stack trace.
>
> It is triggered by regress/sys/netinet6/frag6.  A net lock is missing
> when we send the icmp6 reply.
>
> ok?
>
> bluhm
>
> Index: netinet6/frag6.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/frag6.c,v
> retrieving revision 1.80
> diff -u -p -r1.80 frag6.c
> --- netinet6/frag6.c 13 Nov 2017 07:16:35 -0000 1.80
> +++ netinet6/frag6.c 14 Nov 2017 12:00:37 -0000
> @@ -593,8 +593,12 @@ frag6_slowtimo(void)
>  
>   mtx_leave(&frag6_mutex);
>  
> - while ((q6 = TAILQ_FIRST(&rmq6)) != NULL) {
> - TAILQ_REMOVE(&rmq6, q6, ip6q_queue);
> - frag6_freef(q6);
> + if (!TAILQ_EMPTY(&rmq6)) {
> + NET_LOCK();
> + while ((q6 = TAILQ_FIRST(&rmq6)) != NULL) {
> + TAILQ_REMOVE(&rmq6, q6, ip6q_queue);
> + frag6_freef(q6);
> + }
> + NET_UNLOCK();
>   }
>  }
>

Reply | Threaded
Open this post in threaded view
|

Re: frag6 splassert net lock

Martin Pieuchot
On 14/11/17(Tue) 14:20, Alexandr Nedvedicky wrote:
> Hello,
>
> I would go for NET_RLOCK() instead of NET_LOCK(), which is currently alias
> to NET_WLOCK(). My point is the icmp6_reflect(), which is the workhorse for
> icmp6_error(), is a typical 'READER-user' of network stack. It does not
> change any network configuration. So it should be fine to let it run
> as a reader.

I'd prefer we do not use any NET_RLOCK() until PF_LOCK() is in and
enabled.  Such code path are not performance critical :)

> I'm sure mpi@ can provide authoritative OK here as nothing should go wrong
> with pure NET_LOCK() you have in your diff.

I'm ok with the diff as is.