Re: [PATCH] Work around dhclient brokenness
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 ~{PmVHI~} [EMAIL PROTECTED]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Work around dhclient brokenness
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. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Work around dhclient brokenness
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. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Work around dhclient brokenness
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 -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Work around dhclient brokenness
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. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Work around dhclient brokenness
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 ~{PmVHI~} [EMAIL PROTECTED]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Work around dhclient brokenness
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 -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Work around dhclient brokenness (v2)
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.
This allows unpatched dhclients to continue to work without needing to update
the guest kernels.
Since v1, we refined the search criteria to only consider packets originating
from a DHCP server. I also added a comment to note that we should disable this
routine when we introduce zero copy.
Signed-off-by: Anthony Liguori [EMAIL PROTECTED]
diff --git a/qemu/hw/virtio-net.c b/qemu/hw/virtio-net.c
index 61215b1..409960f 100644
--- a/qemu/hw/virtio-net.c
+++ b/qemu/hw/virtio-net.c
@@ -154,6 +154,34 @@ 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.
+ *
+ * N.B. if we introduce a zero-copy API, this operation is no longer free so
+ * we should provide a mechanism to disable it to avoid polluting the host
+ * cache.
+ */
+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 27 size 1500) /* normal sized MTU */
+(buf[12] == 0x08 buf[13] == 0x00) /* ethertype == IPv4 */
+(buf[23] == 17) /* ip.protocol == UDP */
+(buf[34] == 0 buf[35] == 67)) { /* udp.srcport == bootps */
+/* 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 +208,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 */
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Work around dhclient brokenness (v2)
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. This allows unpatched dhclients to continue to work without needing to update the guest kernels. Since v1, we refined the search criteria to only consider packets originating from a DHCP server. I also added a comment to note that we should disable this routine when we introduce zero copy. Applied, thanks. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Work around dhclient brokenness
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? -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Work around dhclient brokenness
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. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Work around dhclient brokenness
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 -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Work around dhclient brokenness
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 -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Work around dhclient brokenness
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 ~{PmVHI~} [EMAIL PROTECTED]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Work around dhclient brokenness
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. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Work around dhclient brokenness
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. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Work around dhclient brokenness
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. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Work around dhclient brokenness
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)? -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Work around dhclient brokenness
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 ~{PmVHI~} [EMAIL PROTECTED]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Work around dhclient brokenness
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? -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
