Re: [patch for review] Fwd: CURRENT: ifconfig tap0 results in core dump

• Previous message: Maksim Yevmenkin: "Re: [patch for review] Fwd: CURRENT: ifconfig tap0 results in core dump"
• In reply to: Peter Edwards: "[patch for review] Fwd: CURRENT: ifconfig tap0 results in core dump"
• Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Date: Tue, 24 May 2005 09:42:08 -0700
To: peadar@freebsd.org

On Tue, May 24, 2005 at 04:38:02PM +0100, Peter Edwards wrote:
> Does anyone have any objection to me committing the patch in this thread?

It's fine with me. Tap really should be converted to use interface
cloning instead of devfs cloning which would fix some of this, but
that's a problem for another day.

-- Brooks

> (Note: I inadvertently included a local change that no longer prevents
> non-root users from opening up /dev/tap*: I don't intend to commit
> that part of it)
>
>
> ---------- Forwarded message ----------
> From: Peter Edwards <peadar.edwards@gmail.com>
> Date: May 19, 2005 4:34 PM
> Subject: Re: CURRENT: ifconfig tap0 results in core dump
> To: Matti Saarinen <mjsaarin@cc.helsinki.fi>, Scot Hetzel
> <swhetzel@gmail.com>, freebsd-current@freebsd.org
> Cc: peadar@freebsd.org
>
>
> > > % ifconfig tap0
> > > tap0: flags=8802<BROADCAST,SIMPLEX,MULTICAST> mtu 1500
> > > inet6 fe80::2bd:9ff:fe7c:100%tap0 prefixlen 64 scopeid 0x5
> > > zsh: segmentation fault (core dumped) ifconfig tap0
> > >
> > >
> > > I remember that ifconfig didn't dump core when my laptop ran CURRENT
> > > from a few months ago.
> > >
> > You'll probably need to build a version of ifconfig with debugging
> > symbols. And then provide a backtrace of the core dump.
> >
> > How soon after killing openvpn, do you use the ifconfig command. It
> > might be possible that devfs was in the process of removing tap0, when
> > you used the ifconfig command.
> >
> Hm.
> It looks like the "close" code for if_tap clears out the addresses of
> the interface with a pretty blunt-edged "bzero", rather than removing
> them in any clean fashion. As a result, ifconfig gets confused over
> the address families in the tags it sees on the addresses it
> enumerates off the tap interface, and collapses with a corefile.
>
> if_tap's "close" seems to be trying to do part of what's done in
> if_detach, so I split out what I think are the relevant bits from
> there and used it in both places.
>
> Any networking experts care to take a look at the patch? I suspect
> there's a whole mess of locking I'm not doing for a start, but I think
> it might be an improvement over the current situation.
>
> Cheers,
> Peadar.

> Index: net/if.c
> ===================================================================
> RCS file: /usr/cvs/FreeBSD-CVS/src/sys/net/if.c,v
> retrieving revision 1.227
> diff -u -w -r1.227 if.c
> --- net/if.c 20 Apr 2005 09:30:54 -0000 1.227
> +++ net/if.c 9 May 2005 15:33:40 -0000
> @@ -530,13 +530,52 @@
> }
>
> /*
> + * Remove any network addresses from an interface.
> + */
> +
> +void
> +if_purgeaddrs(struct ifnet *ifp)
> +{
> + struct ifaddr *ifa, *next;
> +
> + TAILQ_FOREACH_SAFE(ifa, &ifp->if_addrhead, ifa_link, next) {
> +
> + if (ifa->ifa_addr->sa_family == AF_LINK)
> + continue;
> +#ifdef INET
> + /* XXX: Ugly!! ad hoc just for INET */
> + if (ifa->ifa_addr && ifa->ifa_addr->sa_family == AF_INET) {
> + struct ifaliasreq ifr;
> +
> + bzero(&ifr, sizeof(ifr));
> + ifr.ifra_addr = *ifa->ifa_addr;
> + if (ifa->ifa_dstaddr)
> + ifr.ifra_broadaddr = *ifa->ifa_dstaddr;
> + if (in_control(NULL, SIOCDIFADDR, (caddr_t)&ifr, ifp,
> + NULL) == 0)
> + continue;
> + }
> +#endif /* INET */
> +#ifdef INET6
> + if (ifa->ifa_addr && ifa->ifa_addr->sa_family == AF_INET6) {
> + in6_purgeaddr(ifa);
> + /* ifp_addrhead is already updated */
> + continue;
> + }
> +#endif /* INET6 */
> + TAILQ_REMOVE(&ifp->if_addrhead, ifa, ifa_link);
> + IFAFREE(ifa);
> + }
> +}
> +
> +/*
> * Detach an interface, removing it from the
> * list of "active" interfaces.
> */
> void
> if_detach(struct ifnet *ifp)
> {
> - struct ifaddr *ifa, *next;
> + struct ifaddr *ifa;
> struct radix_node_head *rnh;
> int s;
> int i;
> @@ -568,35 +607,9 @@
> altq_detach(&ifp->if_snd);
> #endif
>
> - for (ifa = TAILQ_FIRST(&ifp->if_addrhead); ifa; ifa = next) {
> - next = TAILQ_NEXT(ifa, ifa_link);
> + if_purgeaddrs(ifp);
>
> - if (ifa->ifa_addr->sa_family == AF_LINK)
> - continue;
> -#ifdef INET
> - /* XXX: Ugly!! ad hoc just for INET */
> - if (ifa->ifa_addr && ifa->ifa_addr->sa_family == AF_INET) {
> - struct ifaliasreq ifr;
>
> - bzero(&ifr, sizeof(ifr));
> - ifr.ifra_addr = *ifa->ifa_addr;
> - if (ifa->ifa_dstaddr)
> - ifr.ifra_broadaddr = *ifa->ifa_dstaddr;
> - if (in_control(NULL, SIOCDIFADDR, (caddr_t)&ifr, ifp,
> - NULL) == 0)
> - continue;
> - }
> -#endif /* INET */
> -#ifdef INET6
> - if (ifa->ifa_addr && ifa->ifa_addr->sa_family == AF_INET6) {
> - in6_purgeaddr(ifa);
> - /* ifp_addrhead is already updated */
> - continue;
> - }
> -#endif /* INET6 */
> - TAILQ_REMOVE(&ifp->if_addrhead, ifa, ifa_link);
> - IFAFREE(ifa);
> - }
>
> #ifdef INET6
> /*
> Index: net/if_tap.c
> ===================================================================
> RCS file: /usr/cvs/FreeBSD-CVS/src/sys/net/if_tap.c,v
> retrieving revision 1.53
> diff -u -w -r1.53 if_tap.c
> --- net/if_tap.c 4 May 2005 18:55:02 -0000 1.53
> +++ net/if_tap.c 9 May 2005 21:01:52 -0000
> @@ -356,9 +356,6 @@
> struct ifnet *ifp = NULL;
> int s;
>
> - if (tapuopen == 0 && suser(td) != 0)
> - return (EPERM);
> -
> if ((dev2unit(dev) & CLONE_UNITMASK) > TAPMAXUNIT)
> return (ENXIO);
>
> @@ -408,6 +405,7 @@
> int bar;
> struct thread *td;
> {
> + struct ifaddr *ifa;
> struct tap_softc *tp = dev->si_drv1;
> struct ifnet *ifp = &tp->tap_if;
> int s;
> @@ -426,24 +424,10 @@
> s = splimp();
> if_down(ifp);
> if (ifp->if_flags & IFF_RUNNING) {
> - /* find internet addresses and delete routes */
> - struct ifaddr *ifa = NULL;
> -
> - /* In desparate need of ifaddr locking. */
> TAILQ_FOREACH(ifa, &ifp->if_addrhead, ifa_link) {
> - if (ifa->ifa_addr->sa_family == AF_INET) {
> rtinit(ifa, (int)RTM_DELETE, 0);
> -
> - /* remove address from interface */
> - bzero(ifa->ifa_addr,
> - sizeof(*(ifa->ifa_addr)));
> - bzero(ifa->ifa_dstaddr,
> - sizeof(*(ifa->ifa_dstaddr)));
> - bzero(ifa->ifa_netmask,
> - sizeof(*(ifa->ifa_netmask)));
> }
> - }
> -
> + if_purgeaddrs(ifp);
> ifp->if_flags &= ~IFF_RUNNING;
> }
> splx(s);
> Index: net/if_var.h
> ===================================================================
> RCS file: /usr/cvs/FreeBSD-CVS/src/sys/net/if_var.h,v
> retrieving revision 1.95
> diff -u -w -r1.95 if_var.h
> --- net/if_var.h 20 Apr 2005 09:30:54 -0000 1.95
> +++ net/if_var.h 9 May 2005 15:33:41 -0000
> @@ -629,6 +629,7 @@
> void if_attach(struct ifnet *);
> int if_delmulti(struct ifnet *, struct sockaddr *);
> void if_detach(struct ifnet *);
> +void if_purgeaddrs(struct ifnet *);
> void if_down(struct ifnet *);
> void if_initname(struct ifnet *, const char *, int);
> void if_link_state_change(struct ifnet *, int);

> _______________________________________________
> freebsd-net@freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-net
> To unsubscribe, send any mail to "freebsd-net-unsubscribe@freebsd.org"

-- 
Any statement of the form "X is the one, true Y" is FALSE.
PGP fingerprint 655D 519C 26A7 82E7 2529  9BF0 5D8E 8BE9 F238 1AD4

• application/pgp-signature attachment: stored

• Previous message: Maksim Yevmenkin: "Re: [patch for review] Fwd: CURRENT: ifconfig tap0 results in core dump"
• In reply to: Peter Edwards: "[patch for review] Fwd: CURRENT: ifconfig tap0 results in core dump"
• Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]