[PATCH] Work around dhclient brokenness

[PATCH] Work around dhclient brokenness

[Anthony Liguori]

With the latest GSO/csum offload patches, any guest using an unpatched version
of dhclient (any Ubuntu guest, for instance), will no longer be able to get
a DHCP address.

dhclient is actually at fault here.  It uses AF_PACKET to receive DHCP responses
but does not check auxdata to see if the packet has a valid csum.  This causes
it to throw out the DHCP responses it gets from the virtio interface as there
is not a valid checksum.

Fedora has carried a patch to fix their dhclient (it's needed for Xen too) but
this patch has not made it into a release of dhclient.  AFAIK, the patch is in
the dhclient CVS but I cannot confirm since their CVS is not public.

This patch, suggested by Rusty, looks for UDP packets (of a normal MTU) and
explicitly adds a checksum to them if they are missing one.  We could further
refine the search criteria based on srcport but that's probably unnecessary.

This allows unpatched dhclients to continue to work without needing to update
the guest kernels.

diff --git a/qemu/hw/virtio-net.c b/qemu/hw/virtio-net.c
index 61215b1..2e2ff35 100644
--- a/qemu/hw/virtio-net.c
+++ b/qemu/hw/virtio-net.c
@@ -154,6 +154,29 @@ static int virtio_net_can_receive(void *opaque)
     return 1;
 }
 
+/* dhclient uses AF_PACKET but doesn't pass auxdata to the kernel so
+ * it never finds out that the packets don't have valid checksums.  This
+ * causes dhclient to get upset.  Fedora's carried a patch for ages to
+ * fix this with Xen but it hasn't appeared in an upstream release of
+ * dhclient yet.
+ *
+ * To avoid breaking existing guests, we catch udp packets and add
+ * checksums.  This is terrible but it's better than hacking the guest
+ * kernels.
+ */
+static void work_around_broken_dhclient(struct virtio_net_hdr *hdr,
+                                        const uint8_t *buf, size_t size)
+{
+    if ((hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) && /* missing csum */
+        (size > 18 && size < 1500) && /* normal sized MTU */
+        (buf[12] == 0x08 && buf[13] == 0x00) && /* ethertype == IPv4 */
+        (buf[23] == 17)) { /* ip.protocol == UDP */
+        /* FIXME this cast is evil */
+        net_checksum_calculate((uint8_t *)buf, size);
+        hdr->flags &= ~VIRTIO_NET_HDR_F_NEEDS_CSUM;
+    }
+}
+
 static void virtio_net_receive(void *opaque, const uint8_t *buf, int size)
 {
     VirtIONet *n = opaque;
@@ -180,6 +203,7 @@ static void virtio_net_receive(void *opaque, const uint8_t *buf, int size)
     if (tap_has_vnet_hdr(n->vc->vlan->first_client)) {
 	memcpy(hdr, buf, sizeof(*hdr));
 	offset += total;
+        work_around_broken_dhclient(hdr, buf + offset, size - offset);
     }
 
     /* copy in packet.  ugh */

Re: [PATCH] Work around dhclient brokenness

[Avi Kivity]

Anthony Liguori wrote:
> With the latest GSO/csum offload patches, any guest using an unpatched version
> of dhclient (any Ubuntu guest, for instance), will no longer be able to get
> a DHCP address.
>
> dhclient is actually at fault here.  It uses AF_PACKET to receive DHCP responses
> but does not check auxdata to see if the packet has a valid csum.  This causes
> it to throw out the DHCP responses it gets from the virtio interface as there
> is not a valid checksum.
>
> Fedora has carried a patch to fix their dhclient (it's needed for Xen too) but
> this patch has not made it into a release of dhclient.  AFAIK, the patch is in
> the dhclient CVS but I cannot confirm since their CVS is not public.
>
> This patch, suggested by Rusty, looks for UDP packets (of a normal MTU) and
> explicitly adds a checksum to them if they are missing one.  We could further
> refine the search criteria based on srcport but that's probably unnecessary.
>
>   

Won't this slow down nfs/udp?  I think a srcport check would be good here.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] Work around dhclient brokenness

[Herbert Xu]

On Mon, Aug 18, 2008 at 01:56:57PM +0300, Avi Kivity wrote:
>
> Won't this slow down nfs/udp?  I think a srcport check would be good here.

I still think that having the guests tell us that they can handle
it is the safest and most efficient way to proceed.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] Work around dhclient brokenness

[Avi Kivity]

Herbert Xu wrote:
> On Mon, Aug 18, 2008 at 01:56:57PM +0300, Avi Kivity wrote:
>   
>> Won't this slow down nfs/udp?  I think a srcport check would be good here.
>>     
>
> I still think that having the guests tell us that they can handle
> it is the safest and most efficient way to proceed.
>   

I thought this is a userspace problem?  Can we fix this in the guest kernel?

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] Work around dhclient brokenness

[Herbert Xu]

On Mon, Aug 18, 2008 at 02:06:46PM +0300, Avi Kivity wrote:
>
> >I still think that having the guests tell us that they can handle
> >it is the safest and most efficient way to proceed.
> >  
> 
> I thought this is a userspace problem?  Can we fix this in the guest kernel?

Right, I meant that it's best to have the guest's user-space tell
us that it can handle checksum offload through ethtool via virtio.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] Work around dhclient brokenness

[Avi Kivity]

Herbert Xu wrote:
> On Mon, Aug 18, 2008 at 02:06:46PM +0300, Avi Kivity wrote:
>   
>>> I still think that having the guests tell us that they can handle
>>> it is the safest and most efficient way to proceed.
>>>  
>>>       
>> I thought this is a userspace problem?  Can we fix this in the guest kernel?
>>     
>
> Right, I meant that it's best to have the guest's user-space tell
> us that it can handle checksum offload through ethtool via virtio.
>   

Isn't that turned on automatically for real hardware?  And what's to 
prevent a broken dhclient together with the (presumably) hacked up 
initscripts that call ethtool?

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] Work around dhclient brokenness

[Herbert Xu]

On Mon, Aug 18, 2008 at 02:40:55PM +0300, Avi Kivity wrote:
>
> Isn't that turned on automatically for real hardware?  And what's to 
> prevent a broken dhclient together with the (presumably) hacked up 
> initscripts that call ethtool?

Well the idea is that only a fixed guest would even know about
enabling this.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] Work around dhclient brokenness

[Avi Kivity]

Herbert Xu wrote:
> On Mon, Aug 18, 2008 at 02:40:55PM +0300, Avi Kivity wrote:
>   
>> Isn't that turned on automatically for real hardware?  And what's to 
>> prevent a broken dhclient together with the (presumably) hacked up 
>> initscripts that call ethtool?
>>     
>
> Well the idea is that only a fixed guest would even know about
> enabling this.
>   

I can think of problems with hotplugged virtio devices, but yes, this 
may be better.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] Work around dhclient brokenness

[Rusty Russell]

On Monday 18 August 2008 21:44:25 Herbert Xu wrote:
> On Mon, Aug 18, 2008 at 02:40:55PM +0300, Avi Kivity wrote:
> > Isn't that turned on automatically for real hardware?  And what's to
> > prevent a broken dhclient together with the (presumably) hacked up
> > initscripts that call ethtool?
>
> Well the idea is that only a fixed guest would even know about
> enabling this.

For those not following closely: We already have a method for the guest to 
accept or reject features.  Our problem is that the guest is already 
accepting the CSUM feature: but one critical userspace app (dhcp-client) can't 
actually handle it due to a bug.

The proposal is to add another mechanism, whereby the host doesn't advertise 
CSUM, but advertises a new CSUM2 feature.  The driver doesn't accept this by 
default: then guest userspace says "hey, I *really can* handle CSUM".  This 
would have to be done dby resetting the device in the ethtool callback 
(that's how we renegotiate features).  And guests need a special virtio hack 
in their init scripts.

This leaves the small number of current users without CSUM (and hence GSO 
etc).  Yet they might not use dhcp with bridging anyway.  Worst of all, we 
have to document this embarrassing workaround.

Neither solution is good.  But I don't think Anthony's hack looks so bad after 
this.

Rusty.

Re: [PATCH] Work around dhclient brokenness

[Chris Wedgwood]

On Tue, Aug 19, 2008 at 10:45:20AM +1000, Rusty Russell wrote:

> For those not following closely: We already have a method for the
> guest to accept or reject features.  Our problem is that the guest
> is already accepting the CSUM feature: but one critical userspace
> app (dhcp-client) can't actually handle it due to a bug.

Can't we just get dhcp-client client fixed upstream and let the
distro's update in a couple of months?

> The proposal is to add another mechanism, whereby the host doesn't
> advertise CSUM, but advertises a new CSUM2 feature.  The driver
> doesn't accept this by default: then guest userspace says "hey, I
> *really can* handle CSUM".  This would have to be done dby resetting
> the device in the ethtool callback (that's how we renegotiate
> features).  And guests need a special virtio hack in their init
> scripts.

If guest can have modified scripts for virtio and what not, they can
have a fixed dhcp-client.

> This leaves the small number of current users without CSUM (and
> hence GSO etc).  Yet they might not use dhcp with bridging anyway.

I think for now that's fine, it's a performance hit that people can
tweak away explicitly without having to add something like a CSUM2
feature.

Re: [PATCH] Work around dhclient brokenness

[Rusty Russell]

On Tuesday 19 August 2008 13:17:57 Chris Wedgwood wrote:
> On Tue, Aug 19, 2008 at 10:45:20AM +1000, Rusty Russell wrote:
> > For those not following closely: We already have a method for the
> > guest to accept or reject features.  Our problem is that the guest
> > is already accepting the CSUM feature: but one critical userspace
> > app (dhcp-client) can't actually handle it due to a bug.
>
> Can't we just get dhcp-client client fixed upstream and let the
> distro's update in a couple of months?

Herbert has already fixed the client, but it seems upstream hasn't released 
yet.

> > The proposal is to add another mechanism, whereby the host doesn't
> > advertise CSUM, but advertises a new CSUM2 feature.  The driver
> > doesn't accept this by default: then guest userspace says "hey, I
> > *really can* handle CSUM".  This would have to be done dby resetting
> > the device in the ethtool callback (that's how we renegotiate
> > features).  And guests need a special virtio hack in their init
> > scripts.
>
> If guest can have modified scripts for virtio and what not, they can
> have a fixed dhcp-client.

They need to do both.  This way if they don't, it still works, but networking 
is at a penalty (no CSUM offload).

Rusty.

Re: [PATCH] Work around dhclient brokenness

[Chris Wedgwood]

On Tue, Aug 19, 2008 at 02:41:01PM +1000, Rusty Russell wrote:

> They need to do both.  This way if they don't, it still works, but
> networking is at a penalty (no CSUM offload).

CSUM2 sounds so ugly though.  Features seem to get added and never
removed....  how about if this had a documented short lifetime (if it
really must go in)?

Re: [PATCH] Work around dhclient brokenness

[Herbert Xu]

On Mon, Aug 18, 2008 at 10:13:55PM -0700, Chris Wedgwood wrote:
> 
> CSUM2 sounds so ugly though.  Features seem to get added and never
> removed....  how about if this had a documented short lifetime (if it
> really must go in)?

All we need is a simple toggle to disable checksum offload.  Every
NIC that offers receive checksum offload allows it to be disabled.
virtio shouldn't be any different.

Resetting the NIC seems a bit over the top.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] Work around dhclient brokenness

[Chris Wedgwood]

On Tue, Aug 19, 2008 at 03:17:08PM +1000, Herbert Xu wrote:

> All we need is a simple toggle to disable checksum offload.  Every
> NIC that offers receive checksum offload allows it to be disabled.
> virtio shouldn't be any different.

So why CSUM2 and not an ethtool interface then?

Re: [PATCH] Work around dhclient brokenness

[Rusty Russell]

On Tuesday 19 August 2008 15:17:08 Herbert Xu wrote:
> On Mon, Aug 18, 2008 at 10:13:55PM -0700, Chris Wedgwood wrote:
> > CSUM2 sounds so ugly though.  Features seem to get added and never
> > removed....  how about if this had a documented short lifetime (if it
> > really must go in)?
>
> All we need is a simple toggle to disable checksum offload.  Every
> NIC that offers receive checksum offload allows it to be disabled.
> virtio shouldn't be any different.
>
> Resetting the NIC seems a bit over the top.

Not really.  We could extend the protocol, but that's currently how feature 
negotiation works: you can't do it while the device is live.  That seemed 
simplest.  I learnt from Xen :)

(Of course, we don't need to *disable* it, we need to *enable* it).

Cheers,
Rusty.

Re: [PATCH] Work around dhclient brokenness

[Rusty Russell]

On Tuesday 19 August 2008 15:28:40 Chris Wedgwood wrote:
> On Tue, Aug 19, 2008 at 03:17:08PM +1000, Herbert Xu wrote:
> > All we need is a simple toggle to disable checksum offload.  Every
> > NIC that offers receive checksum offload allows it to be disabled.
> > virtio shouldn't be any different.
>
> So why CSUM2 and not an ethtool interface then?

We need both.  CSUM2 is the new virtio-level feature.  The ethtool interface 
allows you to toggle it (we don't support that currently, but that's simply 
slackness).

Rusty.

Re: [PATCH] Work around dhclient brokenness

[Avi Kivity]

Rusty Russell wrote:
> On Monday 18 August 2008 21:44:25 Herbert Xu wrote:
>   
>> On Mon, Aug 18, 2008 at 02:40:55PM +0300, Avi Kivity wrote:
>>     
>>> Isn't that turned on automatically for real hardware?  And what's to
>>> prevent a broken dhclient together with the (presumably) hacked up
>>> initscripts that call ethtool?
>>>       
>> Well the idea is that only a fixed guest would even know about
>> enabling this.
>>     
>
> For those not following closely: We already have a method for the guest to 
> accept or reject features.  Our problem is that the guest is already 
> accepting the CSUM feature: but one critical userspace app (dhcp-client) can't 
> actually handle it due to a bug.
>
> The proposal is to add another mechanism, whereby the host doesn't advertise 
> CSUM, but advertises a new CSUM2 feature.  The driver doesn't accept this by 
> default: then guest userspace says "hey, I *really can* handle CSUM".  This 
> would have to be done dby resetting the device in the ethtool callback 
> (that's how we renegotiate features).  And guests need a special virtio hack 
> in their init scripts.
>
> This leaves the small number of current users without CSUM (and hence GSO 
> etc).  Yet they might not use dhcp with bridging anyway.  Worst of all, we 
> have to document this embarrassing workaround.
>
> Neither solution is good.  But I don't think Anthony's hack looks so bad after 
> this.
>   

Well, if changed to avoid random udp packets and focus on dhcp, okay.

I'd still like a way to disable it from the host.  Even when it does 
nothing it will force the header into the host cache, which may be 
different from the guest cache.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] Work around dhclient brokenness

[Herbert Xu]

On Tue, Aug 19, 2008 at 07:08:23PM +1000, Rusty Russell wrote:
> 
> Not really.  We could extend the protocol, but that's currently how feature 
> negotiation works: you can't do it while the device is live.  That seemed 
> simplest.  I learnt from Xen :)
> 
> (Of course, we don't need to *disable* it, we need to *enable* it).

I don't see why we shouldn't support disabling it.  After all, any
NIC that supports receive checksum offload allows it to be disabled.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] Work around dhclient brokenness

[Rusty Russell]

On Tuesday 19 August 2008 19:56:13 Herbert Xu wrote:
> On Tue, Aug 19, 2008 at 07:08:23PM +1000, Rusty Russell wrote:
> > Not really.  We could extend the protocol, but that's currently how
> > feature negotiation works: you can't do it while the device is live. 
> > That seemed simplest.  I learnt from Xen :)
> >
> > (Of course, we don't need to *disable* it, we need to *enable* it).
>
> I don't see why we shouldn't support disabling it.  After all, any
> NIC that supports receive checksum offload allows it to be disabled.

Yes, we might as well support both.  But you said we need to disable it, 
whereas in fact we need to enable it (for this immediate problem).

I was being pedantic because some people are getting themselves very confused 
here :)

Rusty.

Re: [PATCH] Work around dhclient brokenness

[Anthony Liguori]

Avi Kivity wrote:
> Rusty Russell wrote:
>
> Well, if changed to avoid random udp packets and focus on dhcp, okay.

I'll update the patch.

> I'd still like a way to disable it from the host.  Even when it does 
> nothing it will force the header into the host cache, which may be 
> different from the guest cache.

It's already in the host cache as we don't have a zero copy API right now.

Regards,

Anthony Liguori

Re: [PATCH] Work around dhclient brokenness

[Avi Kivity]

Anthony Liguori wrote:
>> I'd still like a way to disable it from the host.  Even when it does 
>> nothing it will force the header into the host cache, which may be 
>> different from the guest cache.
>
> It's already in the host cache as we don't have a zero copy API right 
> now.

I'm thinking of the possibility that we will have a zero copy API one day.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] Work around dhclient brokenness

[Anthony Liguori]

Avi Kivity wrote:
> Anthony Liguori wrote:
>>> I'd still like a way to disable it from the host.  Even when it does 
>>> nothing it will force the header into the host cache, which may be 
>>> different from the guest cache.
>>
>> It's already in the host cache as we don't have a zero copy API right 
>> now.
>
> I'm thinking of the possibility that we will have a zero copy API one 
> day.

Yes, and we can add an option to disable this hack when we implement a 
zero copy API.  It's much easier to add an option than it is to remove 
one.  I'm hoping that over time we can come up with a more clever 
solution to this problem eliminating the need to have an explicit option 
altogether.

Regards,

Anthony Liguori

Re: [PATCH] Work around dhclient brokenness

[Avi Kivity]

Anthony Liguori wrote:
> Avi Kivity wrote:
>> Anthony Liguori wrote:
>>>> I'd still like a way to disable it from the host.  Even when it 
>>>> does nothing it will force the header into the host cache, which 
>>>> may be different from the guest cache.
>>>
>>> It's already in the host cache as we don't have a zero copy API 
>>> right now.
>>
>> I'm thinking of the possibility that we will have a zero copy API one 
>> day.
>
> Yes, and we can add an option to disable this hack when we implement a 
> zero copy API.  It's much easier to add an option than it is to remove 
> one.  I'm hoping that over time we can come up with a more clever 
> solution to this problem eliminating the need to have an explicit 
> option altogether.
>

Makes sense.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] Work around dhclient brokenness

[Chris Wedgwood]

On Tue, Aug 19, 2008 at 07:10:44PM +1000, Rusty Russell wrote:

> We need both.  CSUM2 is the new virtio-level feature.

Perhaps that's what I'm misisng.  How is this different to CSUM?

Re: [PATCH] Work around dhclient brokenness

[Anthony Liguori]

Rusty Russell wrote:
> On Tuesday 19 August 2008 15:17:08 Herbert Xu wrote:
>   
>> On Mon, Aug 18, 2008 at 10:13:55PM -0700, Chris Wedgwood wrote:
>>     
>>> CSUM2 sounds so ugly though.  Features seem to get added and never
>>> removed....  how about if this had a documented short lifetime (if it
>>> really must go in)?
>>>       
>> All we need is a simple toggle to disable checksum offload.  Every
>> NIC that offers receive checksum offload allows it to be disabled.
>> virtio shouldn't be any different.
>>
>> Resetting the NIC seems a bit over the top.
>>     
>
> Not really.  We could extend the protocol, but that's currently how feature 
> negotiation works: you can't do it while the device is live.  That seemed 
> simplest.  I learnt from Xen :)
>
> (Of course, we don't need to *disable* it, we need to *enable* it).
>   

Checksum offload enabled could be a config flag.  Then it could be 
toggled while the device is live.  Of course, you need a feature flag 
for the config flag.

Regards,

Anthony Liguori

> Cheers,
> Rusty.
>   

Re: [PATCH] Work around dhclient brokenness

[Herbert Xu]

On Fri, Aug 15, 2008 at 02:47:12PM -0500, Anthony Liguori wrote:
>
> +static void work_around_broken_dhclient(struct virtio_net_hdr *hdr,
> +                                        const uint8_t *buf, size_t size)
> +{
> +    if ((hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) && /* missing csum */
> +        (size > 18 && size < 1500) && /* normal sized MTU */
> +        (buf[12] == 0x08 && buf[13] == 0x00) && /* ethertype == IPv4 */
> +        (buf[23] == 17)) { /* ip.protocol == UDP */
> +        /* FIXME this cast is evil */
> +        net_checksum_calculate((uint8_t *)buf, size);

If we're going to do this, how about just setting the checksum
to zero?

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] Work around dhclient brokenness

[Rusty Russell]

On Wednesday 20 August 2008 01:05:55 Chris Wedgwood wrote:
> On Tue, Aug 19, 2008 at 07:10:44PM +1000, Rusty Russell wrote:
> > We need both.  CSUM2 is the new virtio-level feature.
>
> Perhaps that's what I'm misisng.  How is this different to CSUM?

Because current guests which say they can handle CSUM can't *really* handle 
it.  That's the entire point.  We need a new one, so that existing guests 
don't ack it (because they don't understand it), and new guests can ack it 
later.

Hope that clarifies,
Rusty.