From owner-freebsd-net@freebsd.org Tue Sep 18 01:09:48 2018 Return-Path: Delivered-To: freebsd-net@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id D846B10ABBF4 for ; Tue, 18 Sep 2018 01:09:48 +0000 (UTC) (envelope-from mike@karels.net) Received: from mail.karels.net (mail.karels.net [216.160.39.52]) by mx1.freebsd.org (Postfix) with ESMTP id 57912744E5 for ; Tue, 18 Sep 2018 01:09:48 +0000 (UTC) (envelope-from mike@karels.net) Received: from mail.karels.net (localhost [127.0.0.1]) by mail.karels.net (8.15.2/8.15.2) with ESMTP id w8I0qOBO022229; Mon, 17 Sep 2018 19:52:24 -0500 (CDT) (envelope-from mike@karels.net) Message-Id: <201809180052.w8I0qOBO022229@mail.karels.net> To: Ryan Stone cc: freebsd-net From: Mike Karels Reply-to: mike@karels.net Subject: Re: udp_notify() invalidates the routing cache without a write lock In-reply-to: Your message of Mon, 10 Sep 2018 17:00:33 -0400. MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-ID: <22227.1537231944.1@mail.karels.net> Content-Transfer-Encoding: quoted-printable Date: Mon, 17 Sep 2018 19:52:24 -0500 X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.27 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 18 Sep 2018 01:09:49 -0000 > 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"