Date: Sat, 8 Dec 2007 19:42:48 +0100 From: Johannes 5 Joemann <joemann@beefree.free.de> To: "Bruce M. Simpson" <bms@FreeBSD.org> Cc: boris@tagnet.ru, current@freebsd.org Subject: Re: Multicast problems [PATCH] Message-ID: <20071208194248.5cfcf0fd.joemann@beefree.free.de> In-Reply-To: <468A84FA.2090703@FreeBSD.org> References: <E1I5iuL-0006kH-6s@clue.co.za> <468A5B9D.6030401@FreeBSD.org> <468A84FA.2090703@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
This is a multi-part message in MIME format. --Multipart=_Sat__8_Dec_2007_19_42_48_+0100_/Fu51QzRhA5OoiZa Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Hi! On Tue, 03 Jul 2007 18:18:50 +0100 "Bruce M. Simpson" <bms@FreeBSD.org> wrote: > Bruce M. Simpson wrote: > > I see now that Linux also supports ip_mreqn in its > > IP_ADD_MEMBERSHIP path. I could potentially change the ASM API > > ioctl paths (IP_ADD_MEMBERSHIP, IP_DEL_MEMBERSHIP) to detect and > > support the ip_mreqn structure -- however -- I am loathe to do > > this as it introduces another bunch of nested conditionals, as > > the same code now has to support IP_ADD_SOURCE_MEMBERSHIP in > > FreeBSD, which has the same structure size. It is also a > > retrograde change. > > I have attached a diff which emulates the Linux ip_mreqn kludge in > the IP_ADD_MEMBERSHIP and IP_DROP_MEMBERSHIP paths; this includes > the changes from the previous patch to workaround the non-existence > of a default route on boot. Thanx! > I do not plan to commit it at the moment and have not tested it. I tested your patch [1] on 7.0-BETA4 in order to make quagga's ospfd functional again (over here it is multicasting on 5 interfaces (3 of them point-to-point) - without the fix it only works on one interface). Your patch solves the problem that if quagga discovers ip_mreqn it also uses it for IP_ADD_MEMBERSHIP. Although you consider your patch to in_mcast.c to be a retrograde kludge, it made my day:-) For the convenience of other 7.0 users being lost in the Bermuda Triangle between RFC1724, Linuxisms, and RFC3678 I've attached a trivial modification of your patch, which makes it apply to the RELENG_7 version 1.3.2.1 of src/sys/netinet/in_mcast.c. > The right thing for applications to do is to use the RFC 3678 API Meanwhile users of ospfd might survive even without your kernel patch as outlined in [2]. I've attached that patch suggestion as a drop in for ports/net/quagga/files. It worked for me with an unpatched (and with a patched) 7.0-BETA4 kernel. > if they need to join an interface by index, the legacy ASM API can > only be relied upon if interfaces are assigned IPv4 addresses, and > it breaks for point-to-point because of legacy BSD behaviour. As long as FreeBSD supplies ip_mreqn in src/sys/netinet/in.h it seems reasonable to support associated operations like IP_ADD_MEMBERSHIP. It's certainly better to eradicate kludges and go with RFC3678, but meanwhile I'd support your patch being committed to RELENG_7 in order to make the ip_mreqn kludge smooth for production. (This is just a naive comment from a humble user who's not in the multicast business.-) Thanx for your work! Johannes ps I've Cc:ed the quagga maintainer (I hope that's OK) as I consider it undesirable to have 7.0 shipped with a broken quagga-ospfd. Whether this should be fixed in the kernel or the port (or both:) is another question, but something should happen ... [1] <http://lists.freebsd.org/pipermail/freebsd-current/attachments/20070703/d4f903b6/add_drop_mreqn-0001.bin>; [2] <http://bugzilla.quagga.net/show_bug.cgi?id=420#c8>; --Multipart=_Sat__8_Dec_2007_19_42_48_+0100_/Fu51QzRhA5OoiZa Content-Type: text/x-diff; name="sys-netinet-in_mcast.c.patch" Content-Disposition: attachment; filename="sys-netinet-in_mcast.c.patch" Content-Transfer-Encoding: 7bit --- sys/netinet/in_mcast.c.orig 2007-11-30 18:01:05.000000000 +0100 +++ sys/netinet/in_mcast.c 2007-12-07 21:34:28.000000000 +0100 @@ -980,11 +980,40 @@ struct ip_mreq_source mreqs; if (sopt->sopt_name == IP_ADD_MEMBERSHIP) { - error = sooptcopyin(sopt, &mreqs, - sizeof(struct ip_mreq), - sizeof(struct ip_mreq)); /* - * Do argument switcharoo from ip_mreq into + * Handle the case where a struct ip_mreqn + * was passed to the IP_ADD_MEMBERSHIP ioctl to + * specify an interface index. This is an + * undocumented modification of the IPv4 ASM API + * obtained from Linux. + */ + if (sopt->sopt_valsize == sizeof(struct ip_mreqn)) { + struct ip_mreqn *mreqn = + (struct ip_mreqn *)&mreqs; + + error = sooptcopyin(sopt, mreqn, + sizeof(struct ip_mreqn), + sizeof(struct ip_mreqn)); + if (error) + return (error); + + if (mreqn->imr_ifindex < 0 || + if_index < mreqn->imr_ifindex) + return (EINVAL); + if (mreqn->imr_ifindex == 0) { + ifp = NULL; + } else { + ifp = ifnet_byindex(mreqn->imr_ifindex); + if (ifp == NULL) + return (EADDRNOTAVAIL); + } + } else { + error = sooptcopyin(sopt, &mreqs, + sizeof(struct ip_mreq), + sizeof(struct ip_mreq)); + } + /* + * Do argument switcharoo from ip_mreq[n] into * ip_mreq_source to avoid using two instances. */ mreqs.imr_interface = mreqs.imr_sourceaddr; @@ -1008,10 +1037,12 @@ } /* - * Obtain ifp. If no interface address was provided, - * use the interface of the route in the unicast FIB for - * the given multicast destination; usually, this is the - * default route. + * If ifp was not already overridden, obtain it. + * + * If no interface address was provided, use the interface + * of the route in the unicast FIB for the given multicast + * destination; usually, this is the default route. + * * If this lookup fails, attempt to use the first non-loopback * interface with multicast capability in the system as a * last resort. The legacy IPv4 ASM API requires that we do @@ -1020,9 +1051,9 @@ * If all of these conditions fail, return EADDRNOTAVAIL, and * reject the IPv4 multicast join. */ - if (mreqs.imr_interface.s_addr != INADDR_ANY) { - ifp = ip_multicast_if(&mreqs.imr_interface); - } else { + if (ifp == NULL && mreqs.imr_interface.s_addr != INADDR_ANY) { + INADDR_TO_IFP(mreqs.imr_interface, ifp); + } else if (ifp == NULL) { struct route ro; ro.ro_rt = NULL; @@ -1235,12 +1266,42 @@ case IP_DROP_MEMBERSHIP: case IP_DROP_SOURCE_MEMBERSHIP: if (sopt->sopt_name == IP_DROP_MEMBERSHIP) { - error = sooptcopyin(sopt, &mreqs, - sizeof(struct ip_mreq), - sizeof(struct ip_mreq)); + /* + * Handle the case where a struct ip_mreqn + * was passed to the IP_DROP_MEMBERSHIP ioctl to + * specify an interface index. This is an + * undocumented modification of the IPv4 ASM API + * obtained from Linux. + */ + if (sopt->sopt_valsize == sizeof(struct ip_mreqn)) { + struct ip_mreqn *mreqn = + (struct ip_mreqn *)&mreqs; + + error = sooptcopyin(sopt, mreqn, + sizeof(struct ip_mreqn), + sizeof(struct ip_mreqn)); + if (error) + return (error); + + if (mreqn->imr_ifindex < 0 || + if_index < mreqn->imr_ifindex) + return (EINVAL); + if (mreqn->imr_ifindex == 0) { + ifp = NULL; + } else { + ifp = ifnet_byindex(mreqn->imr_ifindex); + if (ifp == NULL) + return (EADDRNOTAVAIL); + } + } else { + error = sooptcopyin(sopt, &mreqs, + sizeof(struct ip_mreq), + sizeof(struct ip_mreq)); + } + /* * Swap interface and sourceaddr arguments, - * as ip_mreq and ip_mreq_source are laid + * as ip_mreq[n] and ip_mreq_source are laid * out differently. */ mreqs.imr_interface = mreqs.imr_sourceaddr; @@ -1263,7 +1324,7 @@ ssa->sin.sin_addr = mreqs.imr_sourceaddr; } - if (gsa->sin.sin_addr.s_addr != INADDR_ANY) + if (ifp == NULL && gsa->sin.sin_addr.s_addr != INADDR_ANY) INADDR_TO_IFP(mreqs.imr_interface, ifp); #ifdef DIAGNOSTIC --Multipart=_Sat__8_Dec_2007_19_42_48_+0100_/Fu51QzRhA5OoiZa Content-Type: text/x-diff; name="patch-lib_sockopt.c" Content-Disposition: attachment; filename="patch-lib_sockopt.c" Content-Transfer-Encoding: 7bit --- lib/sockopt.c.orig 2007-08-22 18:22:54.000000000 +0200 +++ lib/sockopt.c 2007-12-07 20:46:35.000000000 +0100 @@ -211,70 +211,12 @@ used instead of if_addr */) { -#ifdef HAVE_STRUCT_IP_MREQN_IMR_IFINDEX - /* This is better because it uses ifindex directly */ - struct ip_mreqn mreqn; - int ret; - - switch (optname) - { - case IP_MULTICAST_IF: - case IP_ADD_MEMBERSHIP: - case IP_DROP_MEMBERSHIP: - memset (&mreqn, 0, sizeof(mreqn)); - - if (mcast_addr) - mreqn.imr_multiaddr.s_addr = mcast_addr; - - if (ifindex) - mreqn.imr_ifindex = ifindex; - else - mreqn.imr_address = if_addr; - - ret = setsockopt(sock, IPPROTO_IP, optname, - (void *)&mreqn, sizeof(mreqn)); - if ((ret < 0) && (optname == IP_ADD_MEMBERSHIP) && (errno == EADDRINUSE)) - { - /* see above: handle possible problem when interface comes back up */ - char buf[2][INET_ADDRSTRLEN]; - zlog_info("setsockopt_multicast_ipv4 attempting to drop and " - "re-add (fd %d, ifaddr %s, mcast %s, ifindex %u)", - sock, - inet_ntop(AF_INET, &if_addr, buf[0], sizeof(buf[0])), - inet_ntop(AF_INET, &mreqn.imr_multiaddr, - buf[1], sizeof(buf[1])), ifindex); - setsockopt(sock, IPPROTO_IP, IP_DROP_MEMBERSHIP, - (void *)&mreqn, sizeof(mreqn)); - ret = setsockopt(sock, IPPROTO_IP, IP_ADD_MEMBERSHIP, - (void *)&mreqn, sizeof(mreqn)); - } - return ret; - break; - - default: - /* Can out and give an understandable error */ - errno = EINVAL; - return -1; - break; - } - - /* Example defines for another OS, boilerplate off other code in this - function, AND handle optname as per other sections for consistency !! */ - /* #elif defined(BOGON_NIX) && EXAMPLE_VERSION_CODE > -100000 */ - /* Add your favourite OS here! */ - -#else /* #if OS_TYPE */ /* standard BSD API */ struct in_addr m; struct ip_mreq mreq; int ret; -#ifdef HAVE_BSD_STRUCT_IP_MREQ_HACK - if (ifindex) - m.s_addr = htonl(ifindex); - else -#endif m = if_addr; switch (optname) @@ -314,7 +256,6 @@ return -1; break; } -#endif /* #if OS_TYPE */ } --Multipart=_Sat__8_Dec_2007_19_42_48_+0100_/Fu51QzRhA5OoiZa--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20071208194248.5cfcf0fd.joemann>