Re: [Qemu-devel] slirp, incoming packets get truncated

[Philippe Mathieu-Daudé]

Hi Samuel,

On 9/8/19 1:29 AM, Samuel Thibault wrote:
>
> Now, with MTU set to 9000, the packets just don't go at all. Could you
> try the attached patch? The lowest layer of slirp was indeed limited to
> 1600-byte frames for no good reason. With this and the virtio driver, I
> could exchange 9000-byte packets.
> 
> Samuel
> 
> diff --git a/src/slirp.c b/src/slirp.c
> index b0194cb..3fd6f68 100644
> --- a/src/slirp.c
> +++ b/src/slirp.c
> @@ -890,20 +890,22 @@ static int if_encap6(Slirp *slirp, struct mbuf
*ifm, struct ethhdr *eh,
>   */
>  int if_encap(Slirp *slirp, struct mbuf *ifm)
>  {
> -    uint8_t buf[1600];
> -    struct ethhdr *eh = (struct ethhdr *)buf;
> +    uint8_t *buf;
> +    struct ethhdr *eh;
>      uint8_t ethaddr[ETH_ALEN];
>      const struct ip *iph = (const struct ip *)ifm->m_data;
>      int ret;
>
> -    if (ifm->m_len + ETH_HLEN > sizeof(buf)) {
> -        return 1;
> -    }
> +    buf = g_malloc(ifm->m_len + ETH_HLEN);

Since g_malloc() aborts on failure, you want g_try_malloc() here.

> +    if (!buf)
> +        return 0;
> +    eh = (struct ethhdr *)buf;
>
>      switch (iph->ip_v) {
>      case IPVERSION:
>          ret = if_encap4(slirp, ifm, eh, ethaddr);
>          if (ret < 2) {
> +            g_free(buf);
>              return ret;
>          }
>          break;
> @@ -911,6 +913,7 @@ int if_encap(Slirp *slirp, struct mbuf *ifm)
>      case IP6VERSION:
>          ret = if_encap6(slirp, ifm, eh, ethaddr);
>          if (ret < 2) {
> +            g_free(buf);
>              return ret;
>          }
>          break;
> @@ -929,6 +932,7 @@ int if_encap(Slirp *slirp, struct mbuf *ifm)
>                eh->h_dest[5]);
>      memcpy(buf + sizeof(struct ethhdr), ifm->m_data, ifm->m_len);
>      slirp_send_packet_all(slirp, buf, ifm->m_len + ETH_HLEN);
> +    g_free(buf);
>      return 1;
>  }

Eventually easier to review using less exit points, i.e.:

-- >8 --
@@ -903,16 +903,10 @@ int if_encap(Slirp *slirp, struct mbuf *ifm)
     switch (iph->ip_v) {
     case IPVERSION:
         ret = if_encap4(slirp, ifm, eh, ethaddr);
-        if (ret < 2) {
-            return ret;
-        }
         break;

     case IP6VERSION:
         ret = if_encap6(slirp, ifm, eh, ethaddr);
-        if (ret < 2) {
-            return ret;
-        }
         break;

     default:
@@ -920,16 +914,21 @@ int if_encap(Slirp *slirp, struct mbuf *ifm)
         break;
     }

-    memcpy(eh->h_dest, ethaddr, ETH_ALEN);
-    DEBUG_ARG("src = %02x:%02x:%02x:%02x:%02x:%02x", eh->h_source[0],
-              eh->h_source[1], eh->h_source[2], eh->h_source[3],
-              eh->h_source[4], eh->h_source[5]);
-    DEBUG_ARG("dst = %02x:%02x:%02x:%02x:%02x:%02x", eh->h_dest[0],
-              eh->h_dest[1], eh->h_dest[2], eh->h_dest[3], eh->h_dest[4],
-              eh->h_dest[5]);
-    memcpy(buf + sizeof(struct ethhdr), ifm->m_data, ifm->m_len);
-    slirp_send_packet_all(slirp, buf, ifm->m_len + ETH_HLEN);
-    return 1;
+    if (ret >= 2) {
+        memcpy(eh->h_dest, ethaddr, ETH_ALEN);
+        DEBUG_ARG("src = %02x:%02x:%02x:%02x:%02x:%02x", eh->h_source[0],
+                  eh->h_source[1], eh->h_source[2], eh->h_source[3],
+                  eh->h_source[4], eh->h_source[5]);
+        DEBUG_ARG("dst = %02x:%02x:%02x:%02x:%02x:%02x", eh->h_dest[0],
+                  eh->h_dest[1], eh->h_dest[2], eh->h_dest[3],
eh->h_dest[4],
+                  eh->h_dest[5]);
+        memcpy(buf + sizeof(struct ethhdr), ifm->m_data, ifm->m_len);
+        slirp_send_packet_all(slirp, buf, ifm->m_len + ETH_HLEN);
+        ret = 1;
+    }
+
+    g_free(buf);
+    return ret;
 }
---

Anyhow, if you plan to properly (with your S-o-b) commit your patch to
the libslirp repository, using g_try_malloc() you can add:

Reviewed-by: Philippe Mathieu-Daudé <address@hidden>

Regards,

Phil.