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.
> > 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();
> >
> >
> >
