Re: divert packet kernel lock
On Fri, May 06, 2022 at 10:16:35PM +0200, Mark Kettenis wrote:
> > Date: Fri, 6 May 2022 14:48:59 +0200
> > From: Alexander Bluhm
> >
> > On Thu, May 05, 2022 at 11:10:54PM +0200, Mark Kettenis wrote:
> > > > Date: Thu, 5 May 2022 22:41:01 +0200
> > > > From: Alexander Bluhm
> > > >
> > > > Hi,
> > > >
> > > > The easiest way to make divert_packet() MP safe for now, is to
> > > > protect sbappendaddr() with kernel lock.
> > >
> > > All other invocations of sbappendaddr() run with the kernel lock held?
> >
> > No. Only this place takes kernel lock.
> >
> > > If so, maybe that should be asserted inside sbappendaddr()?
> >
> > This is only a temporary hack. The clean solution would be a socket
> > mutex. I have marked it with XXXSMP. Maybe this is place is a
> > good start to implement and test such a lock.
> >
> > > If not, I don't understand how this would help...
> >
> > All other places call sbappendaddr() with exclusive net lock.
> > divert_packet() holds the shared net lock, so it cannot run in
> > parallel with the other callers.
> >
> > What is left is protection between multiple divert_packet() running
> > and calling sbappendaddr(). For that kernel lock helps.
> >
> > Of course that is a dirty hack. But we have a race in the commited
> > codebase that I want to plug quickly. A proper solution needs more
> > thought.
>
> Ouch. I suppose use the kernel lock here makes sense since you're
> going to take it after the call anyway.
>
> Maybe change the comment to state that in other places sbappendaddr()
> is always called with an exclusive net lock and therefore can't run
> while we're holding a shared net lock?
Is this better to understand?
Index: netinet/ip_divert.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_divert.c,v
retrieving revision 1.67
diff -u -p -r1.67 ip_divert.c
--- netinet/ip_divert.c 5 May 2022 16:44:22 - 1.67
+++ netinet/ip_divert.c 6 May 2022 20:45:43 -
@@ -222,11 +222,19 @@ divert_packet(struct mbuf *m, int dir, u
}
so = inp->inp_socket;
+ /*
+* XXXSMP sbappendaddr() is not MP safe and this function is called
+* from pf with shared netlock. To call only one sbappendaddr() from
+* divert_packet(), protect it with kernel lock. All other places
+* call sbappendaddr() with exclusive net lock. This blocks
+* divert_packet() as we have the shared lock.
+*/
+ KERNEL_LOCK();
if (sbappendaddr(so, >so_rcv, sintosa(), m, NULL) == 0) {
+ KERNEL_UNLOCK();
divstat_inc(divs_fullsock);
goto bad;
}
- KERNEL_LOCK();
sorwakeup(inp->inp_socket);
KERNEL_UNLOCK();
Index: netinet6/ip6_divert.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/ip6_divert.c,v
retrieving revision 1.66
diff -u -p -r1.66 ip6_divert.c
--- netinet6/ip6_divert.c 5 May 2022 16:44:22 - 1.66
+++ netinet6/ip6_divert.c 6 May 2022 20:45:11 -
@@ -228,11 +228,19 @@ divert6_packet(struct mbuf *m, int dir,
}
so = inp->inp_socket;
+ /*
+* XXXSMP sbappendaddr() is not MP safe and this function is called
+* from pf with shared netlock. To call only one sbappendaddr() from
+* divert_packet(), protect it with kernel lock. All other places
+* call sbappendaddr() with exclusive net lock. This blocks
+* divert_packet() as we have the shared lock.
+*/
+ KERNEL_LOCK();
if (sbappendaddr(so, >so_rcv, sin6tosa(), m, NULL) == 0) {
+ KERNEL_UNLOCK();
div6stat_inc(div6s_fullsock);
goto bad;
}
- KERNEL_LOCK();
sorwakeup(inp->inp_socket);
KERNEL_UNLOCK();
Re: divert packet kernel lock
> Date: Fri, 6 May 2022 14:48:59 +0200
> From: Alexander Bluhm
>
> On Thu, May 05, 2022 at 11:10:54PM +0200, Mark Kettenis wrote:
> > > Date: Thu, 5 May 2022 22:41:01 +0200
> > > From: Alexander Bluhm
> > >
> > > Hi,
> > >
> > > The easiest way to make divert_packet() MP safe for now, is to
> > > protect sbappendaddr() with kernel lock.
> >
> > All other invocations of sbappendaddr() run with the kernel lock held?
>
> No. Only this place takes kernel lock.
>
> > If so, maybe that should be asserted inside sbappendaddr()?
>
> This is only a temporary hack. The clean solution would be a socket
> mutex. I have marked it with XXXSMP. Maybe this is place is a
> good start to implement and test such a lock.
>
> > If not, I don't understand how this would help...
>
> All other places call sbappendaddr() with exclusive net lock.
> divert_packet() holds the shared net lock, so it cannot run in
> parallel with the other callers.
>
> What is left is protection between multiple divert_packet() running
> and calling sbappendaddr(). For that kernel lock helps.
>
> Of course that is a dirty hack. But we have a race in the commited
> codebase that I want to plug quickly. A proper solution needs more
> thought.
Ouch. I suppose use the kernel lock here makes sense since you're
going to take it after the call anyway.
Maybe change the comment to state that in other places sbappendaddr()
is always called with an exclusive net lock and therefore can't run
while we're holding a shared net lock?
> > > Index: netinet/ip_divert.c
> > > ===
> > > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_divert.c,v
> > > retrieving revision 1.67
> > > diff -u -p -r1.67 ip_divert.c
> > > --- netinet/ip_divert.c 5 May 2022 16:44:22 - 1.67
> > > +++ netinet/ip_divert.c 5 May 2022 20:36:23 -
> > > @@ -222,11 +222,18 @@ divert_packet(struct mbuf *m, int dir, u
> > > }
> > >
> > > so = inp->inp_socket;
> > > + /*
> > > + * XXXSMP sbappendaddr() is not MP safe and this function is called
> > > + * from pf with shared netlock. To run only one sbappendaddr()
> > > + * protect it with kernel lock. Socket buffer access from system
> > > + * call is protected with exclusive net lock.
> > > + */
> > > + KERNEL_LOCK();
> > > if (sbappendaddr(so, >so_rcv, sintosa(), m, NULL) == 0) {
> > > + KERNEL_UNLOCK();
> > > divstat_inc(divs_fullsock);
> > > goto bad;
> > > }
> > > - KERNEL_LOCK();
> > > sorwakeup(inp->inp_socket);
> > > KERNEL_UNLOCK();
> > >
> > > Index: netinet6/ip6_divert.c
> > > ===
> > > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/ip6_divert.c,v
> > > retrieving revision 1.66
> > > diff -u -p -r1.66 ip6_divert.c
> > > --- netinet6/ip6_divert.c 5 May 2022 16:44:22 - 1.66
> > > +++ netinet6/ip6_divert.c 5 May 2022 20:36:23 -
> > > @@ -228,11 +228,18 @@ divert6_packet(struct mbuf *m, int dir,
> > > }
> > >
> > > so = inp->inp_socket;
> > > + /*
> > > + * XXXSMP sbappendaddr() is not MP safe and this function is called
> > > + * from pf with shared netlock. To run only one sbappendaddr()
> > > + * protect it with kernel lock. Socket buffer access from system
> > > + * call is protected with exclusive net lock.
> > > + */
> > > + KERNEL_LOCK();
> > > if (sbappendaddr(so, >so_rcv, sin6tosa(), m, NULL) == 0) {
> > > + KERNEL_UNLOCK();
> > > div6stat_inc(div6s_fullsock);
> > > goto bad;
> > > }
> > > - KERNEL_LOCK();
> > > sorwakeup(inp->inp_socket);
> > > KERNEL_UNLOCK();
> > >
> > >
> > >
>
Re: divert packet kernel lock
sbappendaddr() has no sleep points, so this should work. I have no
objections to commit this as quick and dirty fix. Otherwise the
network stack parallelisation diff should be reverted.
> On 6 May 2022, at 15:48, Alexander Bluhm wrote:
>
> On Thu, May 05, 2022 at 11:10:54PM +0200, Mark Kettenis wrote:
>>> Date: Thu, 5 May 2022 22:41:01 +0200
>>> From: Alexander Bluhm
>>>
>>> Hi,
>>>
>>> The easiest way to make divert_packet() MP safe for now, is to
>>> protect sbappendaddr() with kernel lock.
>>
>> All other invocations of sbappendaddr() run with the kernel lock held?
>
> No. Only this place takes kernel lock.
>
>> If so, maybe that should be asserted inside sbappendaddr()?
>
> This is only a temporary hack. The clean solution would be a socket
> mutex. I have marked it with XXXSMP. Maybe this is place is a
> good start to implement and test such a lock.
>
>> If not, I don't understand how this would help...
>
> All other places call sbappendaddr() with exclusive net lock.
> divert_packet() holds the shared net lock, so it cannot run in
> parallel with the other callers.
>
> What is left is protection between multiple divert_packet() running
> and calling sbappendaddr(). For that kernel lock helps.
>
> Of course that is a dirty hack. But we have a race in the commited
> codebase that I want to plug quickly. A proper solution needs more
> thought.
>
>>> Index: netinet/ip_divert.c
>>> ===
>>> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_divert.c,v
>>> retrieving revision 1.67
>>> diff -u -p -r1.67 ip_divert.c
>>> --- netinet/ip_divert.c 5 May 2022 16:44:22 - 1.67
>>> +++ netinet/ip_divert.c 5 May 2022 20:36:23 -
>>> @@ -222,11 +222,18 @@ divert_packet(struct mbuf *m, int dir, u
>>> }
>>>
>>> so = inp->inp_socket;
>>> + /*
>>> +* XXXSMP sbappendaddr() is not MP safe and this function is called
>>> +* from pf with shared netlock. To run only one sbappendaddr()
>>> +* protect it with kernel lock. Socket buffer access from system
>>> +* call is protected with exclusive net lock.
>>> +*/
>>> + KERNEL_LOCK();
>>> if (sbappendaddr(so, >so_rcv, sintosa(), m, NULL) == 0) {
>>> + KERNEL_UNLOCK();
>>> divstat_inc(divs_fullsock);
>>> goto bad;
>>> }
>>> - KERNEL_LOCK();
>>> sorwakeup(inp->inp_socket);
>>> KERNEL_UNLOCK();
>>>
>>> Index: netinet6/ip6_divert.c
>>> ===
>>> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/ip6_divert.c,v
>>> retrieving revision 1.66
>>> diff -u -p -r1.66 ip6_divert.c
>>> --- netinet6/ip6_divert.c 5 May 2022 16:44:22 - 1.66
>>> +++ netinet6/ip6_divert.c 5 May 2022 20:36:23 -
>>> @@ -228,11 +228,18 @@ divert6_packet(struct mbuf *m, int dir,
>>> }
>>>
>>> so = inp->inp_socket;
>>> + /*
>>> +* XXXSMP sbappendaddr() is not MP safe and this function is called
>>> +* from pf with shared netlock. To run only one sbappendaddr()
>>> +* protect it with kernel lock. Socket buffer access from system
>>> +* call is protected with exclusive net lock.
>>> +*/
>>> + KERNEL_LOCK();
>>> if (sbappendaddr(so, >so_rcv, sin6tosa(), m, NULL) == 0) {
>>> + KERNEL_UNLOCK();
>>> div6stat_inc(div6s_fullsock);
>>> goto bad;
>>> }
>>> - KERNEL_LOCK();
>>> sorwakeup(inp->inp_socket);
>>> KERNEL_UNLOCK();
>>>
>>>
>>>
>
Re: divert packet kernel lock
On Thu, May 05, 2022 at 11:10:54PM +0200, Mark Kettenis wrote:
> > Date: Thu, 5 May 2022 22:41:01 +0200
> > From: Alexander Bluhm
> >
> > Hi,
> >
> > The easiest way to make divert_packet() MP safe for now, is to
> > protect sbappendaddr() with kernel lock.
>
> All other invocations of sbappendaddr() run with the kernel lock held?
No. Only this place takes kernel lock.
> If so, maybe that should be asserted inside sbappendaddr()?
This is only a temporary hack. The clean solution would be a socket
mutex. I have marked it with XXXSMP. Maybe this is place is a
good start to implement and test such a lock.
> If not, I don't understand how this would help...
All other places call sbappendaddr() with exclusive net lock.
divert_packet() holds the shared net lock, so it cannot run in
parallel with the other callers.
What is left is protection between multiple divert_packet() running
and calling sbappendaddr(). For that kernel lock helps.
Of course that is a dirty hack. But we have a race in the commited
codebase that I want to plug quickly. A proper solution needs more
thought.
> > Index: netinet/ip_divert.c
> > ===
> > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_divert.c,v
> > retrieving revision 1.67
> > diff -u -p -r1.67 ip_divert.c
> > --- netinet/ip_divert.c 5 May 2022 16:44:22 - 1.67
> > +++ netinet/ip_divert.c 5 May 2022 20:36:23 -
> > @@ -222,11 +222,18 @@ divert_packet(struct mbuf *m, int dir, u
> > }
> >
> > so = inp->inp_socket;
> > + /*
> > +* XXXSMP sbappendaddr() is not MP safe and this function is called
> > +* from pf with shared netlock. To run only one sbappendaddr()
> > +* protect it with kernel lock. Socket buffer access from system
> > +* call is protected with exclusive net lock.
> > +*/
> > + KERNEL_LOCK();
> > if (sbappendaddr(so, >so_rcv, sintosa(), m, NULL) == 0) {
> > + KERNEL_UNLOCK();
> > divstat_inc(divs_fullsock);
> > goto bad;
> > }
> > - KERNEL_LOCK();
> > sorwakeup(inp->inp_socket);
> > KERNEL_UNLOCK();
> >
> > Index: netinet6/ip6_divert.c
> > ===
> > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/ip6_divert.c,v
> > retrieving revision 1.66
> > diff -u -p -r1.66 ip6_divert.c
> > --- netinet6/ip6_divert.c 5 May 2022 16:44:22 - 1.66
> > +++ netinet6/ip6_divert.c 5 May 2022 20:36:23 -
> > @@ -228,11 +228,18 @@ divert6_packet(struct mbuf *m, int dir,
> > }
> >
> > so = inp->inp_socket;
> > + /*
> > +* XXXSMP sbappendaddr() is not MP safe and this function is called
> > +* from pf with shared netlock. To run only one sbappendaddr()
> > +* protect it with kernel lock. Socket buffer access from system
> > +* call is protected with exclusive net lock.
> > +*/
> > + KERNEL_LOCK();
> > if (sbappendaddr(so, >so_rcv, sin6tosa(), m, NULL) == 0) {
> > + KERNEL_UNLOCK();
> > div6stat_inc(div6s_fullsock);
> > goto bad;
> > }
> > - KERNEL_LOCK();
> > sorwakeup(inp->inp_socket);
> > KERNEL_UNLOCK();
> >
> >
> >
Re: divert packet kernel lock
> Date: Thu, 5 May 2022 22:41:01 +0200
> From: Alexander Bluhm
>
> Hi,
>
> The easiest way to make divert_packet() MP safe for now, is to
> protect sbappendaddr() with kernel lock.
All other invocations of sbappendaddr() run with the kernel lock held?
If so, maybe that should be asserted inside sbappendaddr()?
If not, I don't understand how this would help...
> ok?
>
> bluhm
>
> Index: netinet/ip_divert.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_divert.c,v
> retrieving revision 1.67
> diff -u -p -r1.67 ip_divert.c
> --- netinet/ip_divert.c 5 May 2022 16:44:22 - 1.67
> +++ netinet/ip_divert.c 5 May 2022 20:36:23 -
> @@ -222,11 +222,18 @@ divert_packet(struct mbuf *m, int dir, u
> }
>
> so = inp->inp_socket;
> + /*
> + * XXXSMP sbappendaddr() is not MP safe and this function is called
> + * from pf with shared netlock. To run only one sbappendaddr()
> + * protect it with kernel lock. Socket buffer access from system
> + * call is protected with exclusive net lock.
> + */
> + KERNEL_LOCK();
> if (sbappendaddr(so, >so_rcv, sintosa(), m, NULL) == 0) {
> + KERNEL_UNLOCK();
> divstat_inc(divs_fullsock);
> goto bad;
> }
> - KERNEL_LOCK();
> sorwakeup(inp->inp_socket);
> KERNEL_UNLOCK();
>
> Index: netinet6/ip6_divert.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/ip6_divert.c,v
> retrieving revision 1.66
> diff -u -p -r1.66 ip6_divert.c
> --- netinet6/ip6_divert.c 5 May 2022 16:44:22 - 1.66
> +++ netinet6/ip6_divert.c 5 May 2022 20:36:23 -
> @@ -228,11 +228,18 @@ divert6_packet(struct mbuf *m, int dir,
> }
>
> so = inp->inp_socket;
> + /*
> + * XXXSMP sbappendaddr() is not MP safe and this function is called
> + * from pf with shared netlock. To run only one sbappendaddr()
> + * protect it with kernel lock. Socket buffer access from system
> + * call is protected with exclusive net lock.
> + */
> + KERNEL_LOCK();
> if (sbappendaddr(so, >so_rcv, sin6tosa(), m, NULL) == 0) {
> + KERNEL_UNLOCK();
> div6stat_inc(div6s_fullsock);
> goto bad;
> }
> - KERNEL_LOCK();
> sorwakeup(inp->inp_socket);
> KERNEL_UNLOCK();
>
>
>
divert packet kernel lock
Hi,
The easiest way to make divert_packet() MP safe for now, is to
protect sbappendaddr() with kernel lock.
ok?
bluhm
Index: netinet/ip_divert.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_divert.c,v
retrieving revision 1.67
diff -u -p -r1.67 ip_divert.c
--- netinet/ip_divert.c 5 May 2022 16:44:22 - 1.67
+++ netinet/ip_divert.c 5 May 2022 20:36:23 -
@@ -222,11 +222,18 @@ divert_packet(struct mbuf *m, int dir, u
}
so = inp->inp_socket;
+ /*
+* XXXSMP sbappendaddr() is not MP safe and this function is called
+* from pf with shared netlock. To run only one sbappendaddr()
+* protect it with kernel lock. Socket buffer access from system
+* call is protected with exclusive net lock.
+*/
+ KERNEL_LOCK();
if (sbappendaddr(so, >so_rcv, sintosa(), m, NULL) == 0) {
+ KERNEL_UNLOCK();
divstat_inc(divs_fullsock);
goto bad;
}
- KERNEL_LOCK();
sorwakeup(inp->inp_socket);
KERNEL_UNLOCK();
Index: netinet6/ip6_divert.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/ip6_divert.c,v
retrieving revision 1.66
diff -u -p -r1.66 ip6_divert.c
--- netinet6/ip6_divert.c 5 May 2022 16:44:22 - 1.66
+++ netinet6/ip6_divert.c 5 May 2022 20:36:23 -
@@ -228,11 +228,18 @@ divert6_packet(struct mbuf *m, int dir,
}
so = inp->inp_socket;
+ /*
+* XXXSMP sbappendaddr() is not MP safe and this function is called
+* from pf with shared netlock. To run only one sbappendaddr()
+* protect it with kernel lock. Socket buffer access from system
+* call is protected with exclusive net lock.
+*/
+ KERNEL_LOCK();
if (sbappendaddr(so, >so_rcv, sin6tosa(), m, NULL) == 0) {
+ KERNEL_UNLOCK();
div6stat_inc(div6s_fullsock);
goto bad;
}
- KERNEL_LOCK();
sorwakeup(inp->inp_socket);
KERNEL_UNLOCK();
