udp_notify() invalidates the routing cache without a write lock

[Mike Karels]

> 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 lock
> -        * when invoking it directly, in_pcbnotifyall() currently uses write
> -        * 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 == EHOSTUNREACH || errno == ENETUNREACH ||
>             errno == 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 != NULL) {
>                uh = (struct udphdr *)((caddr_t)ip + (ip->ip_hl << 2));
>                inp = 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 != NULL) {
> -                       INP_RLOCK_ASSERT(inp);
> +                       INP_WLOCK_ASSERT(inp);
>                        if (inp->inp_socket != NULL) {
>                                udp_notify(inp, inetctlerrmap[cmd]);
>                        }
> -                       INP_RUNLOCK(inp);
> +                       INP_WUNLOCK(inp);
>                } else {
>                        inp = in_pcblookup(pcbinfo, faddr, uh->uh_dport,
>                                           ip->ip_src, uh->uh_sport,
> _______________________________________________
> freebsd-net at freebsd.org mailing list
> https://lists.freebsd.org/mailman/listinfo/freebsd-net
> To unsubscribe, send any mail to "freebsd-net-unsubscribe at freebsd.org"