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