On 24/12/18(Mon) 17:31, Denis Fondras wrote:
> Thank you for the comments Martin. Here is an improved diff.
I think you should commit the icmp6_do_error() refactoring. It's ok mpi@
with some nits below.
It would be great if you could think the IPv4 version as well to take
a 'struct sockaddr' argument instead of a `struct in_ifaddr'.
I'm not happy with adding the IFF_MULTICAST flag and SIOC{ADD,DEL}MULTI
ioctls. It seems to be a common pattern between in existing pseudo-driver,
so this shouldn't block you. However I'd greatly appreciate if you
could explain to us which code is asking for this and if this could be
improved.
> Index: net/if_ethersubr.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_ethersubr.c,v
> retrieving revision 1.256
> diff -u -p -r1.256 if_ethersubr.c
> --- net/if_ethersubr.c 20 Dec 2018 23:00:55 -0000 1.256
> +++ net/if_ethersubr.c 24 Dec 2018 14:39:26 -0000
> @@ -243,7 +243,11 @@ ether_resolve(struct ifnet *ifp, struct
> if (!ISSET(ifp->if_xflags, IFXF_MPLS))
> senderr(ENETUNREACH);
>
> - switch (dst->sa_family) {
> + af = dst->sa_family;
> + if (af == AF_MPLS)
> + af = rt->rt_gateway->sa_family;
> +
> + switch (af) {
> case AF_LINK:
> if (satosdl(dst)->sdl_alen < sizeof(eh->ether_dhost))
> senderr(EHOSTUNREACH);
> @@ -258,7 +262,6 @@ ether_resolve(struct ifnet *ifp, struct
> break;
> #endif
> case AF_INET:
> - case AF_MPLS:
> error = arpresolve(ifp, rt, m, dst, eh->ether_dhost);
> if (error)
> return (error);
This is much better. ok mpi@
> Index: netinet6/icmp6.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet6/icmp6.c,v
> retrieving revision 1.228
> diff -u -p -r1.228 icmp6.c
> --- netinet6/icmp6.c 10 Dec 2018 23:00:01 -0000 1.228
> +++ netinet6/icmp6.c 24 Dec 2018 14:39:26 -0000
> @@ -231,11 +231,8 @@ icmp6_mtudisc_callback_register(void (*f
> LIST_INSERT_HEAD(&icmp6_mtudisc_callbacks, mc, mc_list);
> }
>
> -/*
> - * Generate an error packet of type error in response to bad IP6 packet.
> - */
> -void
> -icmp6_error(struct mbuf *m, int type, int code, int param)
> +struct mbuf *
> +icmp6_do_error(struct mbuf *m, int type, int code, int param)
> {
> struct ip6_hdr *oip6, *nip6;
> struct icmp6_hdr *icmp6;
> @@ -250,8 +247,9 @@ icmp6_error(struct mbuf *m, int type, in
>
> if (m->m_len < sizeof(struct ip6_hdr)) {
> m = m_pullup(m, sizeof(struct ip6_hdr));
> - if (m == NULL)
> - return;
> + if (m == NULL) {
> + return (NULL);
> + }
No needs for braces here.
> }
> oip6 = mtod(m, struct ip6_hdr *);
>
> @@ -294,7 +292,7 @@ icmp6_error(struct mbuf *m, int type, in
> sizeof(*icp));
> if (icp == NULL) {
> icmp6stat_inc(icp6s_tooshort);
> - return;
> + return (NULL);
> }
> if (icp->icmp6_type < ICMP6_ECHO_REQUEST ||
> icp->icmp6_type == ND_REDIRECT) {
> @@ -334,7 +332,7 @@ icmp6_error(struct mbuf *m, int type, in
> m = m_pullup(m, preplen);
> if (m == NULL) {
> nd6log((LOG_DEBUG, "ENOBUFS in icmp6_error %d\n", __LINE__));
> - return;
> + return (NULL);
> }
>
> nip6 = mtod(m, struct ip6_hdr *);
> @@ -361,18 +359,33 @@ icmp6_error(struct mbuf *m, int type, in
> m->m_pkthdr.ph_ifidx = 0;
>
> icmp6stat_inc(icp6s_outhist + type);
> - icmp6_reflect(m, sizeof(struct ip6_hdr)); /* header order: IPv6 -
> ICMPv6 */
>
> - return;
> + return (m);
>
> freeit:
> /*
> * If we can't tell wheter or not we can generate ICMP6, free it.
> */
> - m_freem(m);
> + return(m_freem(m));
Missing space after 'return'.
> }
>
> /*
> + * Generate an error packet of type error in response to bad IP6 packet.
> + */
> +void
> +icmp6_error(struct mbuf *m, int type, int code, int param)
> +{
> + struct mbuf *n;
> +
> + n = icmp6_do_error(m, type, code, param);
> + if (n != NULL) {
> + /* header order: IPv6 - ICMPv6 */
> + if (!icmp6_reflect(n, sizeof(struct ip6_hdr), NULL))
> + ip6_send(n);
> + }
> +}
> +
> +/*
> * Process a received ICMP6 message.
> */
> int
> @@ -597,7 +610,8 @@ icmp6_input(struct mbuf **mp, int *offp,
> nicmp6->icmp6_code = 0;
> icmp6stat_inc(icp6s_reflect);
> icmp6stat_inc(icp6s_outhist + ICMP6_ECHO_REPLY);
> - icmp6_reflect(n, noff);
> + if (!icmp6_reflect(n, noff, NULL))
> + ip6_send(n);
> }
> if (!m)
> goto freeit;
> @@ -1030,8 +1044,8 @@ icmp6_mtudisc_update(struct ip6ctlparam
> * Reflect the ip6 packet back to the source.
> * OFF points to the icmp6 header, counted from the top of the mbuf.
> */
> -void
> -icmp6_reflect(struct mbuf *m, size_t off)
> +int
> +icmp6_reflect(struct mbuf *m, size_t off, struct sockaddr *sa)
> {
> struct rtentry *rt = NULL;
> struct ip6_hdr *ip6;
> @@ -1051,8 +1065,10 @@ icmp6_reflect(struct mbuf *m, size_t off
> goto bad;
> }
>
> - if (m->m_pkthdr.ph_loopcnt++ >= M_MAXLOOP)
> - goto bad;
> + if (m->m_pkthdr.ph_loopcnt++ >= M_MAXLOOP) {
> + m_freem(m);
> + return (ELOOP);
> + }
> rtableid = m->m_pkthdr.ph_rtableid;
> m_resethdr(m);
> m->m_pkthdr.ph_rtableid = rtableid;
> @@ -1071,7 +1087,7 @@ icmp6_reflect(struct mbuf *m, size_t off
> l = sizeof(struct ip6_hdr) + sizeof(struct icmp6_hdr);
> if (m->m_len < l) {
> if ((m = m_pullup(m, l)) == NULL)
> - return;
> + return (EMSGSIZE);
> }
> memcpy(mtod(m, caddr_t), &nip6, sizeof(nip6));
> } else /* off == sizeof(struct ip6_hdr) */ {
> @@ -1079,7 +1095,7 @@ icmp6_reflect(struct mbuf *m, size_t off
> l = sizeof(struct ip6_hdr) + sizeof(struct icmp6_hdr);
> if (m->m_len < l) {
> if ((m = m_pullup(m, l)) == NULL)
> - return;
> + return (EMSGSIZE);
> }
> }
> ip6 = mtod(m, struct ip6_hdr *);
> @@ -1094,8 +1110,8 @@ icmp6_reflect(struct mbuf *m, size_t off
> ip6->ip6_dst = ip6->ip6_src;
>
> /*
> - * XXX: make sure to embed scope zone information, using
> - * already embedded IDs or the received interface (if any).
> + * XXX: make sure to embed scope zone information, using already
> + * embedded IDs or the received interface (if any).
> * Note that rcvif may be NULL.
> * TODO: scoped routing case (XXX).
> */
This change only obfuscate the blame, it's not needed :)
> @@ -1108,38 +1124,42 @@ icmp6_reflect(struct mbuf *m, size_t off
> sa6_dst.sin6_len = sizeof(sa6_dst);
> sa6_dst.sin6_addr = t;
>
> - /*
> - * If the incoming packet was addressed directly to us (i.e. unicast),
> - * use dst as the src for the reply.
> - * The IN6_IFF_TENTATIVE|IN6_IFF_DUPLICATED case would be VERY rare,
> - * but is possible (for example) when we encounter an error while
> - * forwarding procedure destined to a duplicated address of ours.
> - */
> - rt = rtalloc(sin6tosa(&sa6_dst), 0, rtableid);
> - if (rtisvalid(rt) && ISSET(rt->rt_flags, RTF_LOCAL) &&
> - !ISSET(ifatoia6(rt->rt_ifa)->ia6_flags,
> - IN6_IFF_ANYCAST|IN6_IFF_TENTATIVE|IN6_IFF_DUPLICATED)) {
> - src = &t;
> + if (sa == NULL) {
> + /*
> + * If the incoming packet was addressed directly to us (i.e.
> + * unicast), use dst as the src for the reply. The
> + * IN6_IFF_TENTATIVE|IN6_IFF_DUPLICATED case would be VERY rare,
> + * but is possible (for example) when we encounter an error
> + * while forwarding procedure destined to a duplicated address
> + * of ours.
> + */
> + rt = rtalloc(sin6tosa(&sa6_dst), 0, rtableid);
> + if (rtisvalid(rt) && ISSET(rt->rt_flags, RTF_LOCAL) &&
> + !ISSET(ifatoia6(rt->rt_ifa)->ia6_flags,
> + IN6_IFF_ANYCAST|IN6_IFF_TENTATIVE|IN6_IFF_DUPLICATED)) {
> + src = &t;
> + }
> + rtfree(rt);
> + rt = NULL;
> + sa = sin6tosa(&sa6_src);
> }
> - rtfree(rt);
> - rt = NULL;
>
> if (src == NULL) {
> struct in6_ifaddr *ia6;
>
> /*
> * This case matches to multicasts, our anycast, or unicasts
> - * that we do not own. Select a source address based on the
> + * that we do not own. Select a source address based on the
This is incorrect and useless :)
> * source address of the erroneous packet.
> */
> - rt = rtalloc(sin6tosa(&sa6_src), RT_RESOLVE, rtableid);
> + rt = rtalloc(sa, RT_RESOLVE, rtableid);
> if (!rtisvalid(rt)) {
> char addr[INET6_ADDRSTRLEN];
>
> nd6log((LOG_DEBUG,
> "%s: source can't be determined: dst=%s\n",
> __func__, inet_ntop(AF_INET6, &sa6_src.sin6_addr,
> - addr, sizeof(addr))));
> + addr, sizeof(addr))));
> rtfree(rt);
> goto bad;
> }
> @@ -1167,13 +1187,11 @@ icmp6_reflect(struct mbuf *m, size_t off
> */
>
> m->m_flags &= ~(M_BCAST|M_MCAST);
> -
> - ip6_send(m);
> - return;
> + return (0);
>
> bad:
> m_freem(m);
> - return;
> + return (EHOSTUNREACH);
> }
>
> void
