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 <alexander.bl...@gmx.net> > > > > 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 <alexander.bl...@gmx.net> > > > > > > > > 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();