> 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?
> > > 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 5 May 2022 20:36:23 -0000
> > > @@ -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->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 5 May 2022 20:36:23 -0000
> > > @@ -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->so_rcv, sin6tosa(&sin6), m, NULL) == 0) {
> > > + KERNEL_UNLOCK();
> > > div6stat_inc(div6s_fullsock);
> > > goto bad;
> > > }
> > > - KERNEL_LOCK();
> > > sorwakeup(inp->inp_socket);
> > > KERNEL_UNLOCK();
> > >
> > >
> > >
>
