On 26/12/18(Wed) 17:13, Denis Fondras wrote:
> On Mon, Dec 24, 2018 at 08:43:10PM -0200, Martin Pieuchot wrote:
> > 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.
> >
>
> Interface needs IFF_MULTICAST when enabling IPv6 as per in6_ifattach().
> When an address is configured, the interface joins multicast groups with
> in6_joingroup() (hence use SIOCADDMULTI) but only if IFF_MULTICAST is set. It
> is
> useful for interfaces extern-facing interfaces, not so much for internal (like
> mpe) as they won't process all-{nodes, routers} packets.
>
> We may remove the test in in6_ifattach() because there are many tests through
> the stack. It works if I disable the test in in6_ifattach() and remove the
> IFF_MULTICAST. However I haven't tested further so it may break elsewhere.
I think we could start by relaxing this check for pseudo-interfaces or
point-to-point :)
Do you know any other pseudo-interface that set IFF_MULTICAST and handle
the ioctls just because of this check?
> And here is a new version of the icmp_reflect() diff, it is closer to what is
> done with icmp6_reflect().
I like it, on suggestion below.
> Index: netinet/ip_icmp.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet/ip_icmp.c,v
> retrieving revision 1.181
> diff -u -p -r1.181 ip_icmp.c
> --- netinet/ip_icmp.c 28 Nov 2018 08:15:29 -0000 1.181
> +++ netinet/ip_icmp.c 26 Dec 2018 14:03:50 -0000
> @@ -676,11 +676,12 @@ freeit:
> * Reflect the ip packet back to the source
> */
> int
> -icmp_reflect(struct mbuf *m, struct mbuf **op, struct in_ifaddr *ia)
> +icmp_reflect(struct mbuf *m, struct mbuf **op, struct sockaddr *sa)
> {
> struct ip *ip = mtod(m, struct ip *);
> struct mbuf *opts = NULL;
> struct sockaddr_in sin;
> + struct in_ifaddr *ia = NULL;
> struct rtentry *rt = NULL;
> int optlen = (ip->ip_hl << 2) - sizeof(struct ip);
> u_int rtableid;
> @@ -700,12 +701,12 @@ icmp_reflect(struct mbuf *m, struct mbuf
> m_resethdr(m);
> m->m_pkthdr.ph_rtableid = rtableid;
>
> - /*
> - * If the incoming packet was addressed directly to us,
> - * use dst as the src for the reply. For broadcast, use
> - * the address which corresponds to the incoming interface.
> - */
> - if (ia == NULL) {
> + if (sa == NULL) {
> + /*
> + * If the incoming packet was addressed directly to us,
> + * use dst as the src for the reply. For broadcast, use
> + * the address which corresponds to the incoming interface.
> + */
> memset(&sin, 0, sizeof(sin));
> sin.sin_len = sizeof(sin);
> sin.sin_family = AF_INET;
> @@ -715,8 +716,15 @@ icmp_reflect(struct mbuf *m, struct mbuf
> if (rtisvalid(rt) &&
> ISSET(rt->rt_flags, RTF_LOCAL|RTF_BROADCAST))
> ia = ifatoia(rt->rt_ifa);
> - }
>
> + else {
> + memset(&sin, 0, sizeof(sin));
> + sin.sin_len = sizeof(sin);
> + sin.sin_family = AF_INET;
> + sin.sin_addr = ip->ip_src;
> + sa = sintosa(&sin);
> + }
> + }
> /*
> * The following happens if the packet was not addressed to us.
> * Use the new source address and do a route lookup. If it fails
> @@ -725,19 +733,14 @@ icmp_reflect(struct mbuf *m, struct mbuf
> if (ia == NULL) {
> rtfree(rt);
>
> - memset(&sin, 0, sizeof(sin));
> - sin.sin_len = sizeof(sin);
> - sin.sin_family = AF_INET;
> - sin.sin_addr = ip->ip_src;
> -
> /* keep packet in the original virtual instance */
> - rt = rtalloc(sintosa(&sin), RT_RESOLVE, rtableid);
> - if (rt == NULL) {
> + rt = rtalloc(sa, RT_RESOLVE, rtableid);
> + if (rtisvalid(rt) &&
> + rt->rt_ifa->ifa_addr->sa_family == AF_INET) {
Could you use `sa_family' to decide if you need to increment the
counter? This has the advantage of clearly explaining that this
logic is here for MPLS which is the only code path passing an `sa'
argument.
> ipstat_inc(ips_noroute);
> m_freem(m);
> return (EHOSTUNREACH);
> }
> -
> ia = ifatoia(rt->rt_ifa);
> }
>
> Index: netinet/ip_icmp.h
> ===================================================================
> RCS file: /cvs/src/sys/netinet/ip_icmp.h,v
> retrieving revision 1.31
> diff -u -p -r1.31 ip_icmp.h
> --- netinet/ip_icmp.h 5 Nov 2018 21:50:39 -0000 1.31
> +++ netinet/ip_icmp.h 26 Dec 2018 14:03:50 -0000
> @@ -235,7 +235,7 @@ struct mbuf *
> void icmp_error(struct mbuf *, int, int, u_int32_t, int);
> int icmp_input(struct mbuf **, int *, int, int);
> void icmp_init(void);
> -int icmp_reflect(struct mbuf *, struct mbuf **, struct in_ifaddr *);
> +int icmp_reflect(struct mbuf *, struct mbuf **, struct sockaddr *);
> void icmp_send(struct mbuf *, struct mbuf *);
> int icmp_sysctl(int *, u_int, void *, size_t *, void *, size_t);
> struct rtentry *
> Index: netmpls/mpls_input.c
> ===================================================================
> RCS file: /cvs/src/sys/netmpls/mpls_input.c,v
> retrieving revision 1.68
> diff -u -p -r1.68 mpls_input.c
> --- netmpls/mpls_input.c 12 Jan 2018 06:57:56 -0000 1.68
> +++ netmpls/mpls_input.c 26 Dec 2018 14:03:50 -0000
> @@ -339,9 +339,7 @@ mpls_do_error(struct mbuf *m, int type,
> struct shim_hdr stack[MPLS_INKERNEL_LOOP_MAX];
> struct sockaddr_mpls sa_mpls;
> struct sockaddr_mpls *smpls;
> - struct rtentry *rt = NULL;
> struct shim_hdr *shim;
> - struct in_ifaddr *ia;
> struct icmp *icp;
> struct ip *ip;
> int nstk, error;
> @@ -380,26 +378,7 @@ mpls_do_error(struct mbuf *m, int type,
> smpls->smpls_len = sizeof(*smpls);
> smpls->smpls_label = shim->shim_label & MPLS_LABEL_MASK;
>
> - rt = rtalloc(smplstosa(smpls), RT_RESOLVE, 0);
> - if (rt == NULL) {
> - /* no entry for this label */
> - m_freem(m);
> - return (NULL);
> - }
> - if (rt->rt_ifa->ifa_addr->sa_family == AF_INET)
> - ia = ifatoia(rt->rt_ifa);
> - else {
> - /* XXX this needs fixing, if the MPLS is on an IP
> - * less interface we need to find some other IP to
> - * use as source.
> - */
> - rtfree(rt);
> - m_freem(m);
> - return (NULL);
> - }
> - /* It is safe to dereference ``ia'' iff ``rt'' is valid. */
> - error = icmp_reflect(m, NULL, ia);
> - rtfree(rt);
> + error = icmp_reflect(m, NULL, smplstosa(smpls));
> if (error)
> return (NULL);
>
