Re: [Qemu-devel] [PATCHv7 2/9] slirp: Adding ICMPv6 error sending

[Thomas Huth]

On 08.02.2016 11:28, Samuel Thibault wrote:
> From: Yann Bordenave <address@hidden>
> 
> Disambiguation : icmp_error is renamed into icmp_send_error, since it
> doesn't manage errors, but only sends ICMP Error messages.

You could maybe also put the icmp_error (for IPv4) related stuff into a
separate patch ... but I don't mind too much, this patch is also still
readable as it is right now.

> Adding icmp6_send_error to send ICMPv6 Error messages. This function is
> simpler than the v4 version.
> Adding some calls in various functions to send ICMP errors, when a
> received packet is too big, or when its hop limit is 0.
> 
> Signed-off-by: Yann Bordenave <address@hidden>
> Signed-off-by: Samuel Thibault <address@hidden>
> ---
>  slirp/ip6_icmp.c  | 60 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  slirp/ip6_icmp.h  | 10 ++++++++++
>  slirp/ip6_input.c | 15 ++++++++------
>  slirp/ip_icmp.c   | 12 +++++------
>  slirp/ip_icmp.h   |  4 ++--
>  slirp/ip_input.c  |  8 ++++----
>  slirp/socket.c    |  4 ++--
>  slirp/tcp_input.c |  2 +-
>  slirp/udp.c       |  3 ++-
>  9 files changed, 96 insertions(+), 22 deletions(-)
> 
> diff --git a/slirp/ip6_icmp.c b/slirp/ip6_icmp.c
> index 13f89af..41e034b 100644
> --- a/slirp/ip6_icmp.c
> +++ b/slirp/ip6_icmp.c
> @@ -67,6 +67,66 @@ static void icmp6_send_echoreply(struct mbuf *m, Slirp 
> *slirp, struct ip6 *ip,
>      ip6_output(NULL, t, 0);
>  }
>  
> +void icmp6_send_error(struct mbuf *m, uint8_t type, uint8_t code)
> +{
> +    Slirp *slirp = m->slirp;
> +    struct mbuf *t = m_get(slirp);
> +    struct ip6 *ip = mtod(m, struct ip6 *);
> +
> +    char addrstr[INET6_ADDRSTRLEN];
> +    DEBUG_CALL("icmp_send_error");
> +    DEBUG_ARGS((dfd, " type = %d, code = %d\n", type, code));
> +
> +    /* IPv6 packet */
> +    struct ip6 *rip = mtod(t, struct ip6 *);
> +    rip->ip_src = (struct in6_addr)LINKLOCAL_ADDR;
> +    if (in6_multicast(ip->ip_src) || in6_unspecified(ip->ip_src)) {
> +        /* TODO icmp error? */
> +        return;

I think you're leaking the buffer from "struct mbuf *t = m_get(slirp)"
here ... so "t" should be assigned after the above check instead.

> +    }
> +    rip->ip_dst = ip->ip_src;
> +    inet_ntop(AF_INET6, &rip->ip_dst, addrstr, INET6_ADDRSTRLEN);
> +    DEBUG_ARG("target = %s", addrstr);
> +
> +    rip->ip_nh = IPPROTO_ICMPV6;
> +    const int error_data_len = min(m->m_len,
> +            IF_MTU - (sizeof(struct ip6) + ICMP6_ERROR_MINLEN));
> +    rip->ip_pl = htons(ICMP6_ERROR_MINLEN + error_data_len);
> +    t->m_len = sizeof(struct ip6) + ntohs(rip->ip_pl);
> +
> +    /* ICMPv6 packet */
> +    t->m_data += sizeof(struct ip6);
> +    struct icmp6 *ricmp = mtod(t, struct icmp6 *);
> +    ricmp->icmp6_type = type;
> +    ricmp->icmp6_code = code;
> +    ricmp->icmp6_cksum = 0;
> +
> +    switch (type) {
> +    case ICMP6_UNREACH:
> +    case ICMP6_TIMXCEED:
> +        ricmp->icmp6_err.unused = 0;
> +        break;
> +    case ICMP6_TOOBIG:
> +        ricmp->icmp6_err.mtu = htonl(IF_MTU);
> +        break;
> +    case ICMP6_PARAMPROB:
> +        /* TODO: Handle this case */
> +        break;
> +    default:
> +        g_assert_not_reached();
> +        break;
> +    }
> +    t->m_data += ICMP6_ERROR_MINLEN;
> +    memcpy(t->m_data, m->m_data, error_data_len);
> +
> +    /* Checksum */
> +    t->m_data -= ICMP6_ERROR_MINLEN;
> +    t->m_data -= sizeof(struct ip6);
> +    ricmp->icmp6_cksum = ip6_cksum(t);
> +
> +    ip6_output(NULL, t, 0);
> +}
> +
>  /*
>   * Process a NDP message
>   */
> diff --git a/slirp/ip6_icmp.h b/slirp/ip6_icmp.h
> index 1ae003b..f47c29f 100644
> --- a/slirp/ip6_icmp.h
> +++ b/slirp/ip6_icmp.h
> @@ -22,6 +22,12 @@ struct icmp6_echo { /* Echo Messages */
>      uint16_t seq_num;
>  };
>  
> +union icmp6_error_body {
> +    uint32_t unused;
> +    uint32_t pointer;
> +    uint32_t mtu;
> +};
> +
>  /*
>   * NDP Messages
>   */
> @@ -85,6 +91,7 @@ struct icmp6 {
>      uint8_t     icmp6_code;         /* type sub code */
>      uint16_t    icmp6_cksum;        /* ones complement cksum of struct */
>      union {
> +        union icmp6_error_body error_body;
>          struct icmp6_echo echo;
>          struct ndp_rs ndp_rs;
>          struct ndp_ra ndp_ra;
> @@ -92,6 +99,7 @@ struct icmp6 {
>          struct ndp_na ndp_na;
>          struct ndp_redirect ndp_redirect;
>      } icmp6_body;
> +#define icmp6_err icmp6_body.error_body
>  #define icmp6_echo icmp6_body.echo
>  #define icmp6_nrs icmp6_body.ndp_rs
>  #define icmp6_nra icmp6_body.ndp_ra
> @@ -101,6 +109,7 @@ struct icmp6 {
>  } QEMU_PACKED;
>  
>  #define ICMP6_MINLEN    4
> +#define ICMP6_ERROR_MINLEN  8
>  #define ICMP6_ECHO_MINLEN   8
>  #define ICMP6_NDP_RS_MINLEN 8
>  #define ICMP6_NDP_RA_MINLEN 16
> @@ -238,6 +247,7 @@ struct ndpopt {
>  void icmp6_init(Slirp *slirp);
>  void icmp6_cleanup(Slirp *slirp);
>  void icmp6_input(struct mbuf *);
> +void icmp6_send_error(struct mbuf *m, uint8_t type, uint8_t code);
>  void ndp_send_ra(Slirp *slirp);
>  void ndp_send_ns(Slirp *slirp, struct in6_addr addr);
>  
> diff --git a/slirp/ip6_input.c b/slirp/ip6_input.c
> index e1e6229..828f47c 100644
> --- a/slirp/ip6_input.c
> +++ b/slirp/ip6_input.c
> @@ -33,7 +33,7 @@ void ip6_input(struct mbuf *m)
>      DEBUG_ARG("m_len = %d", m->m_len);
>  
>      if (m->m_len < sizeof(struct ip6)) {
> -        return;
> +        goto bad;
>      }

I think you could also merge this hunk into the previous patch instead.

>      ip6 = mtod(m, struct ip6 *);
> @@ -42,9 +42,14 @@ void ip6_input(struct mbuf *m)
>          goto bad;
>      }
>  
> +    if (ntohs(ip6->ip_pl) > IF_MTU) {
> +        icmp6_send_error(m, ICMP6_TOOBIG, 0);
> +        goto bad;
> +    }
> +
>      /* check ip_ttl for a correct ICMP reply */
>      if (ip6->ip_hl == 0) {
> -        /*icmp_error(m, ICMP_TIMXCEED,ICMP_TIMXCEED_INTRANS, 0,"ttl");*/
> +        icmp6_send_error(m, ICMP6_TIMXCEED, ICMP6_TIMXCEED_INTRANS);
>          goto bad;
>      }
>  
> @@ -52,14 +57,12 @@ void ip6_input(struct mbuf *m)
>       * Switch out to protocol's input routine.
>       */
>      switch (ip6->ip_nh) {
> -#if 0
>      case IPPROTO_TCP:
> -        tcp_input(m, hlen, (struct socket *)NULL);
> +        icmp6_send_error(m, ICMP6_UNREACH, ICMP6_UNREACH_NO_ROUTE);
>          break;
>      case IPPROTO_UDP:
> -        udp_input(m, hlen);
> +        icmp6_send_error(m, ICMP6_UNREACH, ICMP6_UNREACH_NO_ROUTE);
>          break;
> -#endif
>      case IPPROTO_ICMPV6:
>          icmp6_input(m);
>          break;
> diff --git a/slirp/ip_icmp.c b/slirp/ip_icmp.c
> index ace3982..590dada 100644
> --- a/slirp/ip_icmp.c
> +++ b/slirp/ip_icmp.c
> @@ -38,7 +38,7 @@
>  /* Be nice and tell them it's just a pseudo-ping packet */
>  static const char icmp_ping_msg[] = "This is a pseudo-PING packet used by 
> Slirp to emulate ICMP ECHO-REQUEST packets.\n";
>  
> -/* list of actions for icmp_error() on RX of an icmp message */
> +/* list of actions for icmp_send_error() on RX of an icmp message */
>  static const int icmp_flush[19] = {
>  /*  ECHO REPLY (0)  */   0,
>                        1,
> @@ -101,7 +101,7 @@ static int icmp_send(struct socket *so, struct mbuf *m, 
> int hlen)
>                 (struct sockaddr *)&addr, sizeof(addr)) == -1) {
>          DEBUG_MISC((dfd, "icmp_input icmp sendto tx errno = %d-%s\n",
>                      errno, strerror(errno)));
> -        icmp_error(m, ICMP_UNREACH, ICMP_UNREACH_NET, 0, strerror(errno));
> +        icmp_send_error(m, ICMP_UNREACH, ICMP_UNREACH_NET, 0, 
> strerror(errno));
>          icmp_detach(so);
>      }
>  
> @@ -189,7 +189,7 @@ icmp_input(struct mbuf *m, int hlen)
>               (struct sockaddr *)&addr, sizeof(addr)) == -1) {
>       DEBUG_MISC((dfd,"icmp_input udp sendto tx errno = %d-%s\n",
>                   errno,strerror(errno)));
> -     icmp_error(m, ICMP_UNREACH,ICMP_UNREACH_NET, 0,strerror(errno));
> +     icmp_send_error(m, ICMP_UNREACH, ICMP_UNREACH_NET, 0, strerror(errno));
>       udp_detach(so);
>        }
>      } /* if ip->ip_dst.s_addr == alias_addr.s_addr */
> @@ -235,7 +235,7 @@ end_error:
>  
>  #define ICMP_MAXDATALEN (IP_MSS-28)
>  void
> -icmp_error(struct mbuf *msrc, u_char type, u_char code, int minsize,
> +icmp_send_error(struct mbuf *msrc, u_char type, u_char code, int minsize,
>             const char *message)
>  {
>    unsigned hlen, shlen, s_ip_len;
> @@ -243,7 +243,7 @@ icmp_error(struct mbuf *msrc, u_char type, u_char code, 
> int minsize,
>    register struct icmp *icp;
>    register struct mbuf *m;
>  
> -  DEBUG_CALL("icmp_error");
> +  DEBUG_CALL("icmp_send_error");
>    DEBUG_ARG("msrc = %p", msrc);
>    DEBUG_ARG("msrc_len = %d", msrc->m_len);
>  
> @@ -433,7 +433,7 @@ void icmp_receive(struct socket *so)
>          }
>          DEBUG_MISC((dfd, " udp icmp rx errno = %d-%s\n", errno,
>                      strerror(errno)));
> -        icmp_error(so->so_m, ICMP_UNREACH, error_code, 0, strerror(errno));
> +        icmp_send_error(so->so_m, ICMP_UNREACH, error_code, 0, 
> strerror(errno));
>      } else {
>          icmp_reflect(so->so_m);
>          so->so_m = NULL; /* Don't m_free() it again! */
> diff --git a/slirp/ip_icmp.h b/slirp/ip_icmp.h
> index be4426b..846761d 100644
> --- a/slirp/ip_icmp.h
> +++ b/slirp/ip_icmp.h
> @@ -156,8 +156,8 @@ struct icmp {
>  void icmp_init(Slirp *slirp);
>  void icmp_cleanup(Slirp *slirp);
>  void icmp_input(struct mbuf *, int);
> -void icmp_error(struct mbuf *msrc, u_char type, u_char code, int minsize,
> -                const char *message);
> +void icmp_send_error(struct mbuf *msrc, u_char type, u_char code, int 
> minsize,
> +                     const char *message);
>  void icmp_reflect(struct mbuf *);
>  void icmp_receive(struct socket *so);
>  void icmp_detach(struct socket *so);
> diff --git a/slirp/ip_input.c b/slirp/ip_input.c
> index e4855ae..16fb2cb 100644
> --- a/slirp/ip_input.c
> +++ b/slirp/ip_input.c
> @@ -132,9 +132,9 @@ ip_input(struct mbuf *m)
>          m_adj(m, ip->ip_len - m->m_len);
>  
>       /* check ip_ttl for a correct ICMP reply */
> -     if(ip->ip_ttl==0) {
> -       icmp_error(m, ICMP_TIMXCEED,ICMP_TIMXCEED_INTRANS, 0,"ttl");
> -       goto bad;
> +     if (ip->ip_ttl == 0) {
> +         icmp_send_error(m, ICMP_TIMXCEED, ICMP_TIMXCEED_INTRANS, 0, "ttl");
> +         goto bad;
>       }
>  
>       /*
> @@ -637,7 +637,7 @@ typedef uint32_t n_time;
>       }
>       return (0);
>  bad:
> -     icmp_error(m, type, code, 0, 0);
> +     icmp_send_error(m, type, code, 0, 0);
>  
>       return (1);
>  }
> diff --git a/slirp/socket.c b/slirp/socket.c
> index 2b5453e..32b1ba3 100644
> --- a/slirp/socket.c
> +++ b/slirp/socket.c
> @@ -463,7 +463,7 @@ sorecvfrom(struct socket *so)
>  
>           DEBUG_MISC((dfd," udp icmp rx errno = %d-%s\n",
>                       errno,strerror(errno)));
> -         icmp_error(so->so_m, ICMP_UNREACH,code, 0,strerror(errno));
> +         icmp_send_error(so->so_m, ICMP_UNREACH, code, 0, strerror(errno));
>         } else {
>           icmp_reflect(so->so_m);
>              so->so_m = NULL; /* Don't m_free() it again! */
> @@ -511,7 +511,7 @@ sorecvfrom(struct socket *so)
>           else if(errno == ENETUNREACH) code=ICMP_UNREACH_NET;
>  
>           DEBUG_MISC((dfd," rx error, tx icmp ICMP_UNREACH:%i\n", code));
> -         icmp_error(so->so_m, ICMP_UNREACH,code, 0,strerror(errno));
> +         icmp_send_error(so->so_m, ICMP_UNREACH, code, 0, strerror(errno));
>           m_free(m);
>         } else {
>         /*
> diff --git a/slirp/tcp_input.c b/slirp/tcp_input.c
> index 2027a75..5f845da 100644
> --- a/slirp/tcp_input.c
> +++ b/slirp/tcp_input.c
> @@ -608,7 +608,7 @@ findso:
>             m->m_data -= sizeof(struct tcpiphdr)+off-sizeof(struct tcphdr);
>             m->m_len  += sizeof(struct tcpiphdr)+off-sizeof(struct tcphdr);
>             *ip=save_ip;
> -           icmp_error(m, ICMP_UNREACH,code, 0,strerror(errno));
> +           icmp_send_error(m, ICMP_UNREACH, code, 0, strerror(errno));
>           }
>              tcp_close(tp);
>           m_free(m);
> diff --git a/slirp/udp.c b/slirp/udp.c
> index 6b39cab..be012fb 100644
> --- a/slirp/udp.c
> +++ b/slirp/udp.c
> @@ -209,7 +209,8 @@ udp_input(register struct mbuf *m, int iphlen)
>         m->m_data -= iphlen;
>         *ip=save_ip;
>         DEBUG_MISC((dfd,"udp tx errno = %d-%s\n",errno,strerror(errno)));
> -       icmp_error(m, ICMP_UNREACH,ICMP_UNREACH_NET, 0,strerror(errno));
> +       icmp_send_error(m, ICMP_UNREACH, ICMP_UNREACH_NET, 0,
> +                       strerror(errno));
>         goto bad;
>       }
>  
>