Patchwork e1000: Pad short frames to minimum size (60 bytes)

login
register
mail settings
Submitter Anthony Liguori
Date Sept. 20, 2010, 8:31 p.m.
Message ID <4C97C4A4.1030801@codemonkey.ws>
Download mbox | patch
Permalink /patch/65251/
State New
Headers show

Comments

Anthony Liguori - Sept. 20, 2010, 8:31 p.m.
On 09/20/2010 05:42 AM, Michael S. Tsirkin wrote:
> On Sun, Sep 19, 2010 at 07:36:51AM +0100, Stefan Hajnoczi wrote:
>    
>> On Sat, Sep 18, 2010 at 10:27 PM, Edgar E. Iglesias
>> <edgar.iglesias@gmail.com>  wrote:
>>      
>>> This doesn't look right. AFAIK, MAC's dont pad on receive.
>>>        
>> I agree.  NICs that do padding will do it on transmit, not receive.
>> Anything coming in on the wire should already have the minimum length.
>>      
> QEMU never gets access to the wire.
> Our APIs do not really pass complete ethernet packets:
> we forward packets without checksum and padding.
>
> I think it makes complete sense to keep this and
> handle padding in devices because we
> have devices that pass the frame to guest without padding and checksum.
> It should be easy to replace padding code in devices that
> need it with some kind of macro.
>    

Would this not also address the problem?  It sounds like the root cause 
is the tap code, not the devices..

Regards,

Anthony Liguori

>    
>> In QEMU that isn't true today and that's why rtl8139, pcnet, and
>> ne2000 already do this same padding.  This patch is the smallest
>> change to cover e1000.
>>
>>      
>>> IMO this kind of padding should somehow be done by the bridge that forwards
>>> packets into the qemu vlan (e.g slirp or the generic tap bridge).
>>>        
>> That should work and we can then drop the padding code from existing
>> NICs.  I'll take a look.
>>
>> Stefan
>>      
>
Michael S. Tsirkin - Sept. 20, 2010, 8:35 p.m.
On Mon, Sep 20, 2010 at 03:31:32PM -0500, Anthony Liguori wrote:
> On 09/20/2010 05:42 AM, Michael S. Tsirkin wrote:
> >On Sun, Sep 19, 2010 at 07:36:51AM +0100, Stefan Hajnoczi wrote:
> >>On Sat, Sep 18, 2010 at 10:27 PM, Edgar E. Iglesias
> >><edgar.iglesias@gmail.com>  wrote:
> >>>This doesn't look right. AFAIK, MAC's dont pad on receive.
> >>I agree.  NICs that do padding will do it on transmit, not receive.
> >>Anything coming in on the wire should already have the minimum length.
> >QEMU never gets access to the wire.
> >Our APIs do not really pass complete ethernet packets:
> >we forward packets without checksum and padding.
> >
> >I think it makes complete sense to keep this and
> >handle padding in devices because we
> >have devices that pass the frame to guest without padding and checksum.
> >It should be easy to replace padding code in devices that
> >need it with some kind of macro.
> 
> Would this not also address the problem?  It sounds like the root
> cause is the tap code, not the devices..
> 
> Regards,
> 
> Anthony Liguori

There are any number of ways to fix it.
virtio-net does not need the padding and tap does not
give it to us. So current API where we pass in the eth frame
without FCS or padding and devices add it if they like
seems more generic.

> >>In QEMU that isn't true today and that's why rtl8139, pcnet, and
> >>ne2000 already do this same padding.  This patch is the smallest
> >>change to cover e1000.
> >>
> >>>IMO this kind of padding should somehow be done by the bridge that forwards
> >>>packets into the qemu vlan (e.g slirp or the generic tap bridge).
> >>That should work and we can then drop the padding code from existing
> >>NICs.  I'll take a look.
> >>
> >>Stefan
> 

> >From f77c3143f3fbefdfa2f0cc873c2665b5aa78e8c9 Mon Sep 17 00:00:00 2001
> From: Anthony Liguori <aliguori@us.ibm.com>
> Date: Mon, 20 Sep 2010 15:29:31 -0500
> Subject: [PATCH] tap: make sure packets are at least 40 bytes long
> 
> This is required by ethernet drivers but not enforced in the Linux tap code so
> we need to fix it up ourselves.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> 
> diff --git a/net/tap.c b/net/tap.c
> index 4afb314..822241a 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -179,7 +179,13 @@ static int tap_can_send(void *opaque)
>  #ifndef __sun__
>  ssize_t tap_read_packet(int tapfd, uint8_t *buf, int maxlen)
>  {
> -    return read(tapfd, buf, maxlen);
> +    ssize_t len;
> +
> +    len = read(tapfd, buf, maxlen);
> +    if (len > 0) {
> +        len = MAX(MIN(maxlen, 40), len);
> +    }
> +    return len;
>  }
>  #endif
>  
> -- 
> 1.7.0.4
>
Edgar Iglesias - Sept. 20, 2010, 8:40 p.m.
On Mon, Sep 20, 2010 at 03:31:32PM -0500, Anthony Liguori wrote:
> On 09/20/2010 05:42 AM, Michael S. Tsirkin wrote:
> > On Sun, Sep 19, 2010 at 07:36:51AM +0100, Stefan Hajnoczi wrote:
> >    
> >> On Sat, Sep 18, 2010 at 10:27 PM, Edgar E. Iglesias
> >> <edgar.iglesias@gmail.com>  wrote:
> >>      
> >>> This doesn't look right. AFAIK, MAC's dont pad on receive.
> >>>        
> >> I agree.  NICs that do padding will do it on transmit, not receive.
> >> Anything coming in on the wire should already have the minimum length.
> >>      
> > QEMU never gets access to the wire.
> > Our APIs do not really pass complete ethernet packets:
> > we forward packets without checksum and padding.
> >
> > I think it makes complete sense to keep this and
> > handle padding in devices because we
> > have devices that pass the frame to guest without padding and checksum.
> > It should be easy to replace padding code in devices that
> > need it with some kind of macro.
> >    
> 
> Would this not also address the problem?  It sounds like the root cause 
> is the tap code, not the devices..
> 
> Regards,
> 
> Anthony Liguori
> 
> >    
> >> In QEMU that isn't true today and that's why rtl8139, pcnet, and
> >> ne2000 already do this same padding.  This patch is the smallest
> >> change to cover e1000.
> >>
> >>      
> >>> IMO this kind of padding should somehow be done by the bridge that forwards
> >>> packets into the qemu vlan (e.g slirp or the generic tap bridge).
> >>>        
> >> That should work and we can then drop the padding code from existing
> >> NICs.  I'll take a look.
> >>
> >> Stefan
> >>      
> >    
> 

> From f77c3143f3fbefdfa2f0cc873c2665b5aa78e8c9 Mon Sep 17 00:00:00 2001
> From: Anthony Liguori <aliguori@us.ibm.com>
> Date: Mon, 20 Sep 2010 15:29:31 -0500
> Subject: [PATCH] tap: make sure packets are at least 40 bytes long
> 
> This is required by ethernet drivers but not enforced in the Linux tap code so
> we need to fix it up ourselves.


This enforces ethernet semantics on the internal links (which is probably
not good), but it's IMO much better than changing the devices. It also
moves the workaround closer to the root of the problem. IMO, it's a step
in the right direction.

Acked-by: Edgar E. Iglesias <edgar.iglesias@gmail.com>


> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> 
> diff --git a/net/tap.c b/net/tap.c
> index 4afb314..822241a 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -179,7 +179,13 @@ static int tap_can_send(void *opaque)
>  #ifndef __sun__
>  ssize_t tap_read_packet(int tapfd, uint8_t *buf, int maxlen)
>  {
> -    return read(tapfd, buf, maxlen);
> +    ssize_t len;
> +
> +    len = read(tapfd, buf, maxlen);
> +    if (len > 0) {
> +        len = MAX(MIN(maxlen, 40), len);
> +    }
> +    return len;
>  }
>  #endif
>  
> -- 
> 1.7.0.4
>
Michael S. Tsirkin - Sept. 20, 2010, 8:44 p.m.
On Mon, Sep 20, 2010 at 10:40:35PM +0200, Edgar E. Iglesias wrote:
> On Mon, Sep 20, 2010 at 03:31:32PM -0500, Anthony Liguori wrote:
> > On 09/20/2010 05:42 AM, Michael S. Tsirkin wrote:
> > > On Sun, Sep 19, 2010 at 07:36:51AM +0100, Stefan Hajnoczi wrote:
> > >    
> > >> On Sat, Sep 18, 2010 at 10:27 PM, Edgar E. Iglesias
> > >> <edgar.iglesias@gmail.com>  wrote:
> > >>      
> > >>> This doesn't look right. AFAIK, MAC's dont pad on receive.
> > >>>        
> > >> I agree.  NICs that do padding will do it on transmit, not receive.
> > >> Anything coming in on the wire should already have the minimum length.
> > >>      
> > > QEMU never gets access to the wire.
> > > Our APIs do not really pass complete ethernet packets:
> > > we forward packets without checksum and padding.
> > >
> > > I think it makes complete sense to keep this and
> > > handle padding in devices because we
> > > have devices that pass the frame to guest without padding and checksum.
> > > It should be easy to replace padding code in devices that
> > > need it with some kind of macro.
> > >    
> > 
> > Would this not also address the problem?  It sounds like the root cause 
> > is the tap code, not the devices..
> > 
> > Regards,
> > 
> > Anthony Liguori
> > 
> > >    
> > >> In QEMU that isn't true today and that's why rtl8139, pcnet, and
> > >> ne2000 already do this same padding.  This patch is the smallest
> > >> change to cover e1000.
> > >>
> > >>      
> > >>> IMO this kind of padding should somehow be done by the bridge that forwards
> > >>> packets into the qemu vlan (e.g slirp or the generic tap bridge).
> > >>>        
> > >> That should work and we can then drop the padding code from existing
> > >> NICs.  I'll take a look.
> > >>
> > >> Stefan
> > >>      
> > >    
> > 
> 
> > From f77c3143f3fbefdfa2f0cc873c2665b5aa78e8c9 Mon Sep 17 00:00:00 2001
> > From: Anthony Liguori <aliguori@us.ibm.com>
> > Date: Mon, 20 Sep 2010 15:29:31 -0500
> > Subject: [PATCH] tap: make sure packets are at least 40 bytes long
> > 
> > This is required by ethernet drivers but not enforced in the Linux tap code so
> > we need to fix it up ourselves.
> 
> 
> This enforces ethernet semantics on the internal links (which is probably
> not good),

Plus plus ungood.
When we do add e.g. ipoib support, we'll have to go and hunt these bugs down again.
Also will make it impossible to implement any devices that pass in guest buffers
without FCS and padding.

> but it's IMO much better than changing the devices.

How much better?

> It also
> moves the workaround closer to the root of the problem.
> IMO, it's a step in the right direction.
> 
> Acked-by: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> 
> 
> > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> > 
> > diff --git a/net/tap.c b/net/tap.c
> > index 4afb314..822241a 100644
> > --- a/net/tap.c
> > +++ b/net/tap.c
> > @@ -179,7 +179,13 @@ static int tap_can_send(void *opaque)
> >  #ifndef __sun__
> >  ssize_t tap_read_packet(int tapfd, uint8_t *buf, int maxlen)
> >  {
> > -    return read(tapfd, buf, maxlen);
> > +    ssize_t len;
> > +
> > +    len = read(tapfd, buf, maxlen);
> > +    if (len > 0) {
> > +        len = MAX(MIN(maxlen, 40), len);
> > +    }

Let's at least add a comment explaining what does this do?
Also - does tcp backend need this as well? Other backends?


> > +    return len;
> >  }
> >  #endif
> >  
> > -- 
> > 1.7.0.4
> >
Edgar Iglesias - Sept. 20, 2010, 8:51 p.m.
On Mon, Sep 20, 2010 at 03:31:32PM -0500, Anthony Liguori wrote:
> On 09/20/2010 05:42 AM, Michael S. Tsirkin wrote:
> > On Sun, Sep 19, 2010 at 07:36:51AM +0100, Stefan Hajnoczi wrote:
> >    
> >> On Sat, Sep 18, 2010 at 10:27 PM, Edgar E. Iglesias
> >> <edgar.iglesias@gmail.com>  wrote:
> >>      
> >>> This doesn't look right. AFAIK, MAC's dont pad on receive.
> >>>        
> >> I agree.  NICs that do padding will do it on transmit, not receive.
> >> Anything coming in on the wire should already have the minimum length.
> >>      
> > QEMU never gets access to the wire.
> > Our APIs do not really pass complete ethernet packets:
> > we forward packets without checksum and padding.
> >
> > I think it makes complete sense to keep this and
> > handle padding in devices because we
> > have devices that pass the frame to guest without padding and checksum.
> > It should be easy to replace padding code in devices that
> > need it with some kind of macro.
> >    
> 
> Would this not also address the problem?  It sounds like the root cause 
> is the tap code, not the devices..
> 
> Regards,
> 
> Anthony Liguori
> 
> >    
> >> In QEMU that isn't true today and that's why rtl8139, pcnet, and
> >> ne2000 already do this same padding.  This patch is the smallest
> >> change to cover e1000.
> >>
> >>      
> >>> IMO this kind of padding should somehow be done by the bridge that forwards
> >>> packets into the qemu vlan (e.g slirp or the generic tap bridge).
> >>>        
> >> That should work and we can then drop the padding code from existing
> >> NICs.  I'll take a look.
> >>
> >> Stefan
> >>      
> >    
> 

> From f77c3143f3fbefdfa2f0cc873c2665b5aa78e8c9 Mon Sep 17 00:00:00 2001
> From: Anthony Liguori <aliguori@us.ibm.com>
> Date: Mon, 20 Sep 2010 15:29:31 -0500
> Subject: [PATCH] tap: make sure packets are at least 40 bytes long
> 
> This is required by ethernet drivers but not enforced in the Linux tap code so
> we need to fix it up ourselves.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> 
> diff --git a/net/tap.c b/net/tap.c
> index 4afb314..822241a 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -179,7 +179,13 @@ static int tap_can_send(void *opaque)
>  #ifndef __sun__
>  ssize_t tap_read_packet(int tapfd, uint8_t *buf, int maxlen)
>  {
> -    return read(tapfd, buf, maxlen);
> +    ssize_t len;
> +
> +    len = read(tapfd, buf, maxlen);
> +    if (len > 0) {
> +        len = MAX(MIN(maxlen, 40), len);


A small detail :)
40 -> 64 (including a dummy FCS).


> +    }
> +    return len;
>  }
>  #endif
>  
> -- 
> 1.7.0.4
>
Anthony Liguori - Sept. 20, 2010, 8:57 p.m.
On 09/20/2010 03:44 PM, Michael S. Tsirkin wrote:
>>>  From f77c3143f3fbefdfa2f0cc873c2665b5aa78e8c9 Mon Sep 17 00:00:00 2001
>>> From: Anthony Liguori<aliguori@us.ibm.com>
>>> Date: Mon, 20 Sep 2010 15:29:31 -0500
>>> Subject: [PATCH] tap: make sure packets are at least 40 bytes long
>>>
>>> This is required by ethernet drivers but not enforced in the Linux tap code so
>>> we need to fix it up ourselves.
>>>        
>>
>> This enforces ethernet semantics on the internal links (which is probably
>> not good),
>>      
> Plus plus ungood.
> When we do add e.g. ipoib support, we'll have to go and hunt these bugs down again.
> Also will make it impossible to implement any devices that pass in guest buffers
> without FCS and padding.
>    

That's actually a good point which strongly is in favor of making the 
devices do the padding themselves.

Regards,

Anthony Liguori
Edgar Iglesias - Sept. 20, 2010, 9:02 p.m.
On Mon, Sep 20, 2010 at 10:44:34PM +0200, Michael S. Tsirkin wrote:
> On Mon, Sep 20, 2010 at 10:40:35PM +0200, Edgar E. Iglesias wrote:
> > On Mon, Sep 20, 2010 at 03:31:32PM -0500, Anthony Liguori wrote:
> > > On 09/20/2010 05:42 AM, Michael S. Tsirkin wrote:
> > > > On Sun, Sep 19, 2010 at 07:36:51AM +0100, Stefan Hajnoczi wrote:
> > > >    
> > > >> On Sat, Sep 18, 2010 at 10:27 PM, Edgar E. Iglesias
> > > >> <edgar.iglesias@gmail.com>  wrote:
> > > >>      
> > > >>> This doesn't look right. AFAIK, MAC's dont pad on receive.
> > > >>>        
> > > >> I agree.  NICs that do padding will do it on transmit, not receive.
> > > >> Anything coming in on the wire should already have the minimum length.
> > > >>      
> > > > QEMU never gets access to the wire.
> > > > Our APIs do not really pass complete ethernet packets:
> > > > we forward packets without checksum and padding.
> > > >
> > > > I think it makes complete sense to keep this and
> > > > handle padding in devices because we
> > > > have devices that pass the frame to guest without padding and checksum.
> > > > It should be easy to replace padding code in devices that
> > > > need it with some kind of macro.
> > > >    
> > > 
> > > Would this not also address the problem?  It sounds like the root cause 
> > > is the tap code, not the devices..
> > > 
> > > Regards,
> > > 
> > > Anthony Liguori
> > > 
> > > >    
> > > >> In QEMU that isn't true today and that's why rtl8139, pcnet, and
> > > >> ne2000 already do this same padding.  This patch is the smallest
> > > >> change to cover e1000.
> > > >>
> > > >>      
> > > >>> IMO this kind of padding should somehow be done by the bridge that forwards
> > > >>> packets into the qemu vlan (e.g slirp or the generic tap bridge).
> > > >>>        
> > > >> That should work and we can then drop the padding code from existing
> > > >> NICs.  I'll take a look.
> > > >>
> > > >> Stefan
> > > >>      
> > > >    
> > > 
> > 
> > > From f77c3143f3fbefdfa2f0cc873c2665b5aa78e8c9 Mon Sep 17 00:00:00 2001
> > > From: Anthony Liguori <aliguori@us.ibm.com>
> > > Date: Mon, 20 Sep 2010 15:29:31 -0500
> > > Subject: [PATCH] tap: make sure packets are at least 40 bytes long
> > > 
> > > This is required by ethernet drivers but not enforced in the Linux tap code so
> > > we need to fix it up ourselves.
> > 
> > 
> > This enforces ethernet semantics on the internal links (which is probably
> > not good),
> 
> Plus plus ungood.
> When we do add e.g. ipoib support, we'll have to go and hunt these bugs down again.
> Also will make it impossible to implement any devices that pass in guest buffers
> without FCS and padding.

If we dont remove the padding from the device models rx paths, we
will continue with code that relies on it and it is IMO wrong.
Ethernet MAC's don't padd nor append checksum on receive.

I agree with you that it's not great that the internal link
protocol has to be strictly ethernet but it seems to me like
if that is reality today, with or without Anthonys patch.
slirp and tap both require ethernet semantics (except possibly
padding and FCS). The addressing and packet headers are ethernet.

In the long run, I'd rather see a more flexible internal interconnect
that supports mutiple heterogenous link types. In the meantime, I
think Anthonys patch is a better workaround than patching the
device models.

> > but it's IMO much better than changing the devices.
> 
> How much better?

OK, s/much better/better/ :)

> 
> > It also
> > moves the workaround closer to the root of the problem.
> > IMO, it's a step in the right direction.
> > 
> > Acked-by: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> > 
> > 
> > > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> > > 
> > > diff --git a/net/tap.c b/net/tap.c
> > > index 4afb314..822241a 100644
> > > --- a/net/tap.c
> > > +++ b/net/tap.c
> > > @@ -179,7 +179,13 @@ static int tap_can_send(void *opaque)
> > >  #ifndef __sun__
> > >  ssize_t tap_read_packet(int tapfd, uint8_t *buf, int maxlen)
> > >  {
> > > -    return read(tapfd, buf, maxlen);
> > > +    ssize_t len;
> > > +
> > > +    len = read(tapfd, buf, maxlen);
> > > +    if (len > 0) {
> > > +        len = MAX(MIN(maxlen, 40), len);
> > > +    }
> 
> Let's at least add a comment explaining what does this do?
> Also - does tcp backend need this as well? Other backends?

A comment sounds good.

Cheers,
Edgar
Edgar E. Iglesias - Sept. 21, 2010, 9:16 a.m.
On Mon, Sep 20, 2010 at 9:31 PM, Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 09/20/2010 05:42 AM, Michael S. Tsirkin wrote:
>>
>> On Sun, Sep 19, 2010 at 07:36:51AM +0100, Stefan Hajnoczi wrote:
>>
>>>
>>> On Sat, Sep 18, 2010 at 10:27 PM, Edgar E. Iglesias
>>> <edgar.iglesias@gmail.com>  wrote:
>>>
>>>>
>>>> This doesn't look right. AFAIK, MAC's dont pad on receive.
>>>>
>>>
>>> I agree.  NICs that do padding will do it on transmit, not receive.
>>> Anything coming in on the wire should already have the minimum length.
>>>
>>
>> QEMU never gets access to the wire.
>> Our APIs do not really pass complete ethernet packets:
>> we forward packets without checksum and padding.
>>
>> I think it makes complete sense to keep this and
>> handle padding in devices because we
>> have devices that pass the frame to guest without padding and checksum.
>> It should be easy to replace padding code in devices that
>> need it with some kind of macro.
>>
>
> Would this not also address the problem?  It sounds like the root cause is
> the tap code, not the devices..

This won't work when s->has_vnet_hdr is 1 because the virtio-net
header consumes buffer space and reduces the amount we pad.  The
padding size should be 60 + (s->has_vnet_hdr ? sizeof(struct
virtio_net_hdr) : 0).

Adjusting the length without clearing the untouched buffer space is
probably fine.  I'm trying to think of a scenario where this becomes
an information leak (security issue).  Perhaps if the guest has vlans
enabled and allows different users to sniff traffic only on their
vlans?  Then you may be able to read part of another vlan's traffic by
sending short packets to your vlan and gathering the padding data.
This is pretty contrived but doing a <60 byte memset would prevent the
issue for sure.

Stefan
Michael S. Tsirkin - Sept. 21, 2010, 9:17 a.m.
On Mon, Sep 20, 2010 at 10:51:36PM +0200, Edgar E. Iglesias wrote:
> On Mon, Sep 20, 2010 at 03:31:32PM -0500, Anthony Liguori wrote:
> > On 09/20/2010 05:42 AM, Michael S. Tsirkin wrote:
> > > On Sun, Sep 19, 2010 at 07:36:51AM +0100, Stefan Hajnoczi wrote:
> > >    
> > >> On Sat, Sep 18, 2010 at 10:27 PM, Edgar E. Iglesias
> > >> <edgar.iglesias@gmail.com>  wrote:
> > >>      
> > >>> This doesn't look right. AFAIK, MAC's dont pad on receive.
> > >>>        
> > >> I agree.  NICs that do padding will do it on transmit, not receive.
> > >> Anything coming in on the wire should already have the minimum length.
> > >>      
> > > QEMU never gets access to the wire.
> > > Our APIs do not really pass complete ethernet packets:
> > > we forward packets without checksum and padding.
> > >
> > > I think it makes complete sense to keep this and
> > > handle padding in devices because we
> > > have devices that pass the frame to guest without padding and checksum.
> > > It should be easy to replace padding code in devices that
> > > need it with some kind of macro.
> > >    
> > 
> > Would this not also address the problem?  It sounds like the root cause 
> > is the tap code, not the devices..
> > 
> > Regards,
> > 
> > Anthony Liguori
> > 
> > >    
> > >> In QEMU that isn't true today and that's why rtl8139, pcnet, and
> > >> ne2000 already do this same padding.  This patch is the smallest
> > >> change to cover e1000.
> > >>
> > >>      
> > >>> IMO this kind of padding should somehow be done by the bridge that forwards
> > >>> packets into the qemu vlan (e.g slirp or the generic tap bridge).
> > >>>        
> > >> That should work and we can then drop the padding code from existing
> > >> NICs.  I'll take a look.
> > >>
> > >> Stefan
> > >>      
> > >    
> > 
> 
> > From f77c3143f3fbefdfa2f0cc873c2665b5aa78e8c9 Mon Sep 17 00:00:00 2001
> > From: Anthony Liguori <aliguori@us.ibm.com>
> > Date: Mon, 20 Sep 2010 15:29:31 -0500
> > Subject: [PATCH] tap: make sure packets are at least 40 bytes long
> > 
> > This is required by ethernet drivers but not enforced in the Linux tap code so
> > we need to fix it up ourselves.
> > 
> > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> > 
> > diff --git a/net/tap.c b/net/tap.c
> > index 4afb314..822241a 100644
> > --- a/net/tap.c
> > +++ b/net/tap.c
> > @@ -179,7 +179,13 @@ static int tap_can_send(void *opaque)
> >  #ifndef __sun__
> >  ssize_t tap_read_packet(int tapfd, uint8_t *buf, int maxlen)
> >  {
> > -    return read(tapfd, buf, maxlen);
> > +    ssize_t len;
> > +
> > +    len = read(tapfd, buf, maxlen);
> > +    if (len > 0) {
> > +        len = MAX(MIN(maxlen, 40), len);
> 
> 
> A small detail :)
> 40 -> 64 (including a dummy FCS).

I don't think so: e1000 at least has code to tack the FCS on,
so we'll end up with a 68 bytes.

> > +    }
> > +    return len;
> >  }
> >  #endif
> >  
> > -- 
> > 1.7.0.4
> >
Edgar Iglesias - Sept. 21, 2010, 9:21 a.m.
On Tue, Sep 21, 2010 at 11:17:07AM +0200, Michael S. Tsirkin wrote:
> On Mon, Sep 20, 2010 at 10:51:36PM +0200, Edgar E. Iglesias wrote:
> > On Mon, Sep 20, 2010 at 03:31:32PM -0500, Anthony Liguori wrote:
> > > On 09/20/2010 05:42 AM, Michael S. Tsirkin wrote:
> > > > On Sun, Sep 19, 2010 at 07:36:51AM +0100, Stefan Hajnoczi wrote:
> > > >    
> > > >> On Sat, Sep 18, 2010 at 10:27 PM, Edgar E. Iglesias
> > > >> <edgar.iglesias@gmail.com>  wrote:
> > > >>      
> > > >>> This doesn't look right. AFAIK, MAC's dont pad on receive.
> > > >>>        
> > > >> I agree.  NICs that do padding will do it on transmit, not receive.
> > > >> Anything coming in on the wire should already have the minimum length.
> > > >>      
> > > > QEMU never gets access to the wire.
> > > > Our APIs do not really pass complete ethernet packets:
> > > > we forward packets without checksum and padding.
> > > >
> > > > I think it makes complete sense to keep this and
> > > > handle padding in devices because we
> > > > have devices that pass the frame to guest without padding and checksum.
> > > > It should be easy to replace padding code in devices that
> > > > need it with some kind of macro.
> > > >    
> > > 
> > > Would this not also address the problem?  It sounds like the root cause 
> > > is the tap code, not the devices..
> > > 
> > > Regards,
> > > 
> > > Anthony Liguori
> > > 
> > > >    
> > > >> In QEMU that isn't true today and that's why rtl8139, pcnet, and
> > > >> ne2000 already do this same padding.  This patch is the smallest
> > > >> change to cover e1000.
> > > >>
> > > >>      
> > > >>> IMO this kind of padding should somehow be done by the bridge that forwards
> > > >>> packets into the qemu vlan (e.g slirp or the generic tap bridge).
> > > >>>        
> > > >> That should work and we can then drop the padding code from existing
> > > >> NICs.  I'll take a look.
> > > >>
> > > >> Stefan
> > > >>      
> > > >    
> > > 
> > 
> > > From f77c3143f3fbefdfa2f0cc873c2665b5aa78e8c9 Mon Sep 17 00:00:00 2001
> > > From: Anthony Liguori <aliguori@us.ibm.com>
> > > Date: Mon, 20 Sep 2010 15:29:31 -0500
> > > Subject: [PATCH] tap: make sure packets are at least 40 bytes long
> > > 
> > > This is required by ethernet drivers but not enforced in the Linux tap code so
> > > we need to fix it up ourselves.
> > > 
> > > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> > > 
> > > diff --git a/net/tap.c b/net/tap.c
> > > index 4afb314..822241a 100644
> > > --- a/net/tap.c
> > > +++ b/net/tap.c
> > > @@ -179,7 +179,13 @@ static int tap_can_send(void *opaque)
> > >  #ifndef __sun__
> > >  ssize_t tap_read_packet(int tapfd, uint8_t *buf, int maxlen)
> > >  {
> > > -    return read(tapfd, buf, maxlen);
> > > +    ssize_t len;
> > > +
> > > +    len = read(tapfd, buf, maxlen);
> > > +    if (len > 0) {
> > > +        len = MAX(MIN(maxlen, 40), len);
> > 
> > 
> > A small detail :)
> > 40 -> 64 (including a dummy FCS).
> 
> I don't think so: e1000 at least has code to tack the FCS on,
> so we'll end up with a 68 bytes.

And at the moment e1000 also has padding, both padding
and FCS appending should go away from ethernet models before
this goes in.

Anyway, if you guys maintaining the networking parts are in
agreement that padding and FCS appending should be done in
the device models (at least for the moment), I'll accept
that and back-off. In that case, I think your suggestion
of hiding things behind some kind of generic macro or
function would be good. At least it will clarify things.

Cheers
Stefan Weil - Feb. 21, 2011, 9:13 p.m.
Am 21.09.2010 11:16, schrieb Stefan Hajnoczi:
> On Mon, Sep 20, 2010 at 9:31 PM, Anthony Liguori 
> <anthony@codemonkey.ws> wrote:
>> On 09/20/2010 05:42 AM, Michael S. Tsirkin wrote:
>>> On Sun, Sep 19, 2010 at 07:36:51AM +0100, Stefan Hajnoczi wrote:
>>>> On Sat, Sep 18, 2010 at 10:27 PM, Edgar E. Iglesias
>>>> <edgar.iglesias@gmail.com>  wrote:
>>>>> This doesn't look right. AFAIK, MAC's dont pad on receive.
>>>> I agree.  NICs that do padding will do it on transmit, not receive.
>>>> Anything coming in on the wire should already have the minimum length.
>>> QEMU never gets access to the wire.
>>> Our APIs do not really pass complete ethernet packets:
>>> we forward packets without checksum and padding.
>>>
>>> I think it makes complete sense to keep this and
>>> handle padding in devices because we
>>> have devices that pass the frame to guest without padding and checksum.
>>> It should be easy to replace padding code in devices that
>>> need it with some kind of macro.
>> Would this not also address the problem?  It sounds like the root 
>> cause is
>> the tap code, not the devices..
> This won't work when s->has_vnet_hdr is 1 because the virtio-net
> header consumes buffer space and reduces the amount we pad. The
> padding size should be 60 + (s->has_vnet_hdr ? sizeof(struct
> virtio_net_hdr) : 0).
>
> Adjusting the length without clearing the untouched buffer space is
> probably fine. I'm trying to think of a scenario where this becomes
> an information leak (security issue). Perhaps if the guest has vlans
> enabled and allows different users to sniff traffic only on their
> vlans? Then you may be able to read part of another vlan's traffic by
> sending short packets to your vlan and gathering the padding data.
> This is pretty contrived but doing a <60 byte memset would prevent the
> issue for sure.
>
> Stefan


The latest patch which was sent was for eepro100.c (Bruce Rogers),
but any ethernet NIC has the same kind of problem.

Does the majority still think that patching the MAC
emulation is the right way (although real MACs don't
pad on receive, as Edgar already explained)?

Then all ethernet NIC emulations should
handle the padding in the same way.
The code should be marked as a workaround
which has nothing to do with the MAC emulation.
MAC emulation code for short frames should not be
removed.

If there is consensus on this, I'll send a modified
patch for eepro100.c (or Bruce modifies his patch
so it does add the workaround comment without
removing the short frame code).

The better way would be padding in qemu's network
code for those devices which need it (that means
adding a new flag "min_frame_length" for ethernet
devices).

Stefan W.

Patch

From f77c3143f3fbefdfa2f0cc873c2665b5aa78e8c9 Mon Sep 17 00:00:00 2001
From: Anthony Liguori <aliguori@us.ibm.com>
Date: Mon, 20 Sep 2010 15:29:31 -0500
Subject: [PATCH] tap: make sure packets are at least 40 bytes long

This is required by ethernet drivers but not enforced in the Linux tap code so
we need to fix it up ourselves.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

diff --git a/net/tap.c b/net/tap.c
index 4afb314..822241a 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -179,7 +179,13 @@  static int tap_can_send(void *opaque)
 #ifndef __sun__
 ssize_t tap_read_packet(int tapfd, uint8_t *buf, int maxlen)
 {
-    return read(tapfd, buf, maxlen);
+    ssize_t len;
+
+    len = read(tapfd, buf, maxlen);
+    if (len > 0) {
+        len = MAX(MIN(maxlen, 40), len);
+    }
+    return len;
 }
 #endif
 
-- 
1.7.0.4