Patchwork slirp: ensure minimum packet size

login
register
mail settings
Submitter Bruce Rogers
Date Feb. 10, 2011, 10:54 p.m.
Message ID <4D540A3402000048000A9C8C@novprvoes0310.provo.novell.com>
Download mbox | patch
Permalink /patch/82680/
State New
Headers show

Comments

Bruce Rogers - Feb. 10, 2011, 10:54 p.m.
With recent gpxe eepro100 drivers, short packets are rejected,
so ensure the minimum ethernet packet size.

Signed-off-by: Bruce Rogers <brogers@novell.com>
---
 slirp/slirp.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Anthony Liguori - Feb. 11, 2011, 8:26 a.m.
On 02/10/2011 11:54 PM, Bruce Rogers wrote:
> With recent gpxe eepro100 drivers, short packets are rejected,
> so ensure the minimum ethernet packet size.
>
> Signed-off-by: Bruce Rogers<brogers@novell.com>
>    

This doesn't make much sense.  I think this is more likely a case where 
we're incorrectly calculating packet size.  Michael fixed an issue 
similar to this in e1000 relating to FCR if I recall correctly.  Maybe 
eepro100 has the same problem?

Regards,

Anthony Liguori

> ---
>   slirp/slirp.c |    4 ++--
>   1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/slirp/slirp.c b/slirp/slirp.c
> index 332d83b..b611cf7 100644
> --- a/slirp/slirp.c
> +++ b/slirp/slirp.c
> @@ -697,7 +697,7 @@ void if_encap(Slirp *slirp, const uint8_t *ip_data, int ip_data_len)
>           return;
>
>       if (!memcmp(slirp->client_ethaddr, zero_ethaddr, ETH_ALEN)) {
> -        uint8_t arp_req[ETH_HLEN + sizeof(struct arphdr)];
> +        uint8_t arp_req[max(ETH_HLEN + sizeof(struct arphdr), 64)];
>           struct ethhdr *reh = (struct ethhdr *)arp_req;
>           struct arphdr *rah = (struct arphdr *)(arp_req + ETH_HLEN);
>           const struct ip *iph = (const struct ip *)ip_data;
> @@ -734,7 +734,7 @@ void if_encap(Slirp *slirp, const uint8_t *ip_data, int ip_data_len)
>           memcpy(&eh->h_source[2],&slirp->vhost_addr, 4);
>           eh->h_proto = htons(ETH_P_IP);
>           memcpy(buf + sizeof(struct ethhdr), ip_data, ip_data_len);
> -        slirp_output(slirp->opaque, buf, ip_data_len + ETH_HLEN);
> +        slirp_output(slirp->opaque, buf, max(ip_data_len + ETH_HLEN, 64));
>       }
>   }
>
>
Bruce Rogers - Feb. 11, 2011, 5:46 p.m.
>>> On 2/11/2011 at 01:26 AM, Anthony Liguori <anthony@codemonkey.ws> wrote: 
> On 02/10/2011 11:54 PM, Bruce Rogers wrote:
>> With recent gpxe eepro100 drivers, short packets are rejected,
>> so ensure the minimum ethernet packet size.
>>
>> Signed-off-by: Bruce Rogers<brogers@novell.com>
>>    
> 
> This doesn't make much sense.  I think this is more likely a case where 
> we're incorrectly calculating packet size.  Michael fixed an issue 
> similar to this in e1000 relating to FCR if I recall correctly.  Maybe 
> eepro100 has the same problem?
> 

I just took a clue from the other, similar code in slirp.c (arp_input()), where it ensured a minimum of 64 bytes. I now see that that is the wrong approach, and the way this is handled is that the nic emulation adds padding to short packets in its receive handler.

I'll submit a patch with that approach instead.

Thanks for the review.

Bruce

Patch

diff --git a/slirp/slirp.c b/slirp/slirp.c
index 332d83b..b611cf7 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -697,7 +697,7 @@  void if_encap(Slirp *slirp, const uint8_t *ip_data, int ip_data_len)
         return;
     
     if (!memcmp(slirp->client_ethaddr, zero_ethaddr, ETH_ALEN)) {
-        uint8_t arp_req[ETH_HLEN + sizeof(struct arphdr)];
+        uint8_t arp_req[max(ETH_HLEN + sizeof(struct arphdr), 64)];
         struct ethhdr *reh = (struct ethhdr *)arp_req;
         struct arphdr *rah = (struct arphdr *)(arp_req + ETH_HLEN);
         const struct ip *iph = (const struct ip *)ip_data;
@@ -734,7 +734,7 @@  void if_encap(Slirp *slirp, const uint8_t *ip_data, int ip_data_len)
         memcpy(&eh->h_source[2], &slirp->vhost_addr, 4);
         eh->h_proto = htons(ETH_P_IP);
         memcpy(buf + sizeof(struct ethhdr), ip_data, ip_data_len);
-        slirp_output(slirp->opaque, buf, ip_data_len + ETH_HLEN);
+        slirp_output(slirp->opaque, buf, max(ip_data_len + ETH_HLEN, 64));
     }
 }