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 <[email protected]>
> >
> > 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 <[email protected]>
> > > >
> > > > 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 -0000 1.67
+++ netinet/ip_divert.c 6 May 2022 20:45:43 -0000
@@ -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->so_rcv, sintosa(&sin), 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 -0000 1.66
+++ netinet6/ip6_divert.c 6 May 2022 20:45:11 -0000
@@ -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->so_rcv, sin6tosa(&sin6), m, NULL) == 0) {
+ KERNEL_UNLOCK();
div6stat_inc(div6s_fullsock);
goto bad;
}
- KERNEL_LOCK();
sorwakeup(inp->inp_socket);
KERNEL_UNLOCK();
