Date: Mon, 17 Sep 2018 19:52:24 -0500 From: Mike Karels <mike@karels.net> To: Ryan Stone <rysto32@gmail.com> Cc: freebsd-net <freebsd-net@freebsd.org> Subject: Re: udp_notify() invalidates the routing cache without a write lock Message-ID: <201809180052.w8I0qOBO022229@mail.karels.net> In-Reply-To: Your message of Mon, 10 Sep 2018 17:00:33 -0400. <CAFMmRNzS0xFJg04-urmUdk=UWGSxMHcowfYow9fasQhaTj8ojA@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
> Recently at work I had a system crash while executing RTFREE() in
> udp_notify(). In looking at the system I discovered that two threads
> had called udp_notify() on the same pcb. This was possible because
> the threads only held a read lock on the socket. The obvious solution
> is to hold a write lock in this path. I haven't even compile-tested
> the patch below yet, but would anybody have any objections to this
> approach?
I think a write lock is definitely required. The approach seems fine.
Mike
> diff --git a/sys/netinet/udp_usrreq.c b/sys/netinet/udp_usrreq.c
> index cae044c066c3..429f195ee954 100644
> --- a/sys/netinet/udp_usrreq.c
> +++ b/sys/netinet/udp_usrreq.c
> @@ -756,13 +756,7 @@ struct inpcb *
> udp_notify(struct inpcb *inp, int errno)
> {
> - /*
> - * While udp_ctlinput() always calls udp_notify() with a read lo=
ck
> - * when invoking it directly, in_pcbnotifyall() currently uses w=
rite
> - * locks due to sharing code with TCP. For now, accept either a=
read
> - * or a write lock, but a read lock is sufficient.
> - */
> - INP_LOCK_ASSERT(inp);
> + INP_WLOCK_ASSERT(inp);
> if ((errno =3D=3D EHOSTUNREACH || errno =3D=3D ENETUNREACH ||
> errno =3D=3D EHOSTDOWN) && inp->inp_route.ro_rt) {
> RTFREE(inp->inp_route.ro_rt);
> @@ -808,13 +802,13 @@ udp_common_ctlinput(int cmd, struct sockaddr
> *sa, void *vip,
> if (ip !=3D NULL) {
> uh =3D (struct udphdr *)((caddr_t)ip + (ip->ip_hl << 2));
> inp =3D in_pcblookup(pcbinfo, faddr, uh->uh_dport,
> - ip->ip_src, uh->uh_sport, INPLOOKUP_RLOCKPCB, NULL);
> + ip->ip_src, uh->uh_sport, INPLOOKUP_WLOCKPCB, NULL);
> if (inp !=3D NULL) {
> - INP_RLOCK_ASSERT(inp);
> + INP_WLOCK_ASSERT(inp);
> if (inp->inp_socket !=3D NULL) {
> udp_notify(inp, inetctlerrmap[cmd]);
> }
> - INP_RUNLOCK(inp);
> + INP_WUNLOCK(inp);
> } else {
> inp =3D in_pcblookup(pcbinfo, faddr, uh->uh_dport=
,
> ip->ip_src, uh->uh_sport,
> _______________________________________________
> freebsd-net@freebsd.org mailing list
> https://lists.freebsd.org/mailman/listinfo/freebsd-net
> To unsubscribe, send any mail to "freebsd-net-unsubscribe@freebsd.org"
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201809180052.w8I0qOBO022229>