On 25/12/18(Tue) 21:16, Denis Fondras wrote:
> On Mon, Dec 24, 2018 at 08:43:10PM -0200, Martin Pieuchot wrote:
> > 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'.
> >
>
> Here is a diff to convert "struct in_ifaddr" to "struct sockaddr"
Comments 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 25 Dec 2018 20:11:37 -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,18 @@ icmp_reflect(struct mbuf *m, struct mbuf
> m_resethdr(m);
> m->m_pkthdr.ph_rtableid = rtableid;
>
> + if (sa != NULL) {
> + rt = rtalloc(sa, RT_RESOLVE, rtableid);
> + if (rtisvalid(rt) && rt->rt_ifa->ifa_addr->sa_family == AF_INET)
> + ia = ifatoia(rt->rt_ifa);
> + }
This is worst than what we had. Because this chunk of code is now only
here because of a single caller of icmp_reflect().
Your IPv6 change doesn't use any `ia6' instead it uses `sa', can't we do
the same here?
> /*
> * 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) {
> + rtfree(rt);
> memset(&sin, 0, sizeof(sin));
> sin.sin_len = sizeof(sin);
> sin.sin_family = AF_INET;
> @@ -732,7 +739,7 @@ icmp_reflect(struct mbuf *m, struct mbuf
>
> /* keep packet in the original virtual instance */
> rt = rtalloc(sintosa(&sin), RT_RESOLVE, rtableid);
> - if (rt == NULL) {
> + if (!rtisvalid(rt)) {
This is correct. Sadly like many correct changes they expose existing
bugs. So it's better to commit them separately. This function is used
in many code paths, so I wouldn't be surprised to see a regression :o)
(Some em(4) have a link detection bug that prevent us to do the same
thing in ip_output()).
