[PATCHv2] vhost-net: add dhclient work-around from userspace

[PATCHv2] vhost-net: add dhclient work-around from userspace

[Michael S. Tsirkin]

Userspace virtio server has the following hack
so guests rely on it, and we have to replicate it, too:

Use port number to detect incoming IPv4 DHCP response packets,
and fill in the checksum for these.

The issue we are solving is that on linux guests, some apps
that use recvmsg with AF_PACKET sockets, don't know how to
handle CHECKSUM_PARTIAL;
The interface to return the relevant information was added
in 8dc4194474159660d7f37c495e3fc3f10d0db8cc,
and older userspace does not use it.
One important user of recvmsg with AF_PACKET is dhclient,
so we add a work-around just for DHCP.

Don't bother applying the hack to IPv6 as userspace virtio does not
have a work-around for that - let's hope guests will do the right
thing wrt IPv6.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

Dave, I'm going to put this patch on the vhost tree,
no need for you to bother merging it - you'll get
it with a pull request.


 drivers/vhost/net.c |   44 +++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 43 insertions(+), 1 deletions(-)

diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index cc19595..03bba6a 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -24,6 +24,10 @@
 #include <linux/if_tun.h>
 #include <linux/if_macvlan.h>
 
+#include <linux/ip.h>
+#include <linux/udp.h>
+#include <linux/netdevice.h>
+
 #include <net/sock.h>
 
 #include "vhost.h"
@@ -186,6 +190,44 @@ static void handle_tx(struct vhost_net *net)
 	unuse_mm(net->dev.mm);
 }
 
+static int peek_head(struct sock *sk)
+{
+	struct sk_buff *skb;
+
+	lock_sock(sk);
+	skb = skb_peek(&sk->sk_receive_queue);
+	if (unlikely(!skb)) {
+		release_sock(sk);
+		return 0;
+	}
+	/* Userspace virtio server has the following hack so
+	 * guests rely on it, and we have to replicate it, too: */
+	/* Use port number to detect incoming IPv4 DHCP response packets,
+	 * and fill in the checksum. */
+
+	/* The issue we are solving is that on linux guests, some apps
+	 * that use recvmsg with AF_PACKET sockets, don't know how to
+	 * handle CHECKSUM_PARTIAL;
+	 * The interface to return the relevant information was added in
+	 * 8dc4194474159660d7f37c495e3fc3f10d0db8cc,
+	 * and older userspace does not use it.
+	 * One important user of recvmsg with AF_PACKET is dhclient,
+	 * so we add a work-around just for DHCP. */
+	if (skb->ip_summed == CHECKSUM_PARTIAL &&
+	    skb_headlen(skb) >= skb_transport_offset(skb) +
+				sizeof(struct udphdr) &&
+	    udp_hdr(skb)->dest == htons(68) &&
+	    skb_network_header_len(skb) >= sizeof(struct iphdr) &&
+	    ip_hdr(skb)->protocol == IPPROTO_UDP &&
+	    skb->protocol == htons(ETH_P_IP)) {
+		skb_checksum_help(skb);
+		/* Restore ip_summed value: tun passes it to user. */
+		skb->ip_summed = CHECKSUM_PARTIAL;
+	}
+	release_sock(sk);
+	return 1;
+}
+
 /* Expects to be always run from workqueue - which acts as
  * read-size critical section for our kind of RCU. */
 static void handle_rx(struct vhost_net *net)
@@ -222,7 +264,7 @@ static void handle_rx(struct vhost_net *net)
 	vq_log = unlikely(vhost_has_feature(&net->dev, VHOST_F_LOG_ALL)) ?
 		vq->log : NULL;
 
-	for (;;) {
+	while (peek_head(sock->sk)) {
 		head = vhost_get_vq_desc(&net->dev, vq, vq->iov,
 					 ARRAY_SIZE(vq->iov),
 					 &out, &in,
-- 
1.7.1.12.g42b7f

Re: [PATCHv2] vhost-net: add dhclient work-around from userspace

[Michael S. Tsirkin]

On Mon, Jun 28, 2010 at 01:08:07PM +0300, Michael S. Tsirkin wrote:
> Userspace virtio server has the following hack
> so guests rely on it, and we have to replicate it, too:
> 
> Use port number to detect incoming IPv4 DHCP response packets,
> and fill in the checksum for these.
> 
> The issue we are solving is that on linux guests, some apps
> that use recvmsg with AF_PACKET sockets, don't know how to
> handle CHECKSUM_PARTIAL;
> The interface to return the relevant information was added
> in 8dc4194474159660d7f37c495e3fc3f10d0db8cc,
> and older userspace does not use it.
> One important user of recvmsg with AF_PACKET is dhclient,
> so we add a work-around just for DHCP.
> 
> Don't bother applying the hack to IPv6 as userspace virtio does not
> have a work-around for that - let's hope guests will do the right
> thing wrt IPv6.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Tested-by: Laine Stump <laine@redhat.com>

Re: [PATCHv2] vhost-net: add dhclient work-around from userspace

[Sridhar Samudrala]

On Mon, 2010-06-28 at 13:08 +0300, Michael S. Tsirkin wrote:
> Userspace virtio server has the following hack
> so guests rely on it, and we have to replicate it, too:
> 
> Use port number to detect incoming IPv4 DHCP response packets,
> and fill in the checksum for these.
> 
> The issue we are solving is that on linux guests, some apps
> that use recvmsg with AF_PACKET sockets, don't know how to
> handle CHECKSUM_PARTIAL;
> The interface to return the relevant information was added
> in 8dc4194474159660d7f37c495e3fc3f10d0db8cc,
> and older userspace does not use it.
> One important user of recvmsg with AF_PACKET is dhclient,
> so we add a work-around just for DHCP.
> 
> Don't bother applying the hack to IPv6 as userspace virtio does not
> have a work-around for that - let's hope guests will do the right
> thing wrt IPv6.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> 
> Dave, I'm going to put this patch on the vhost tree,
> no need for you to bother merging it - you'll get
> it with a pull request.
> 
> 
>  drivers/vhost/net.c |   44 +++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 43 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index cc19595..03bba6a 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -24,6 +24,10 @@
>  #include <linux/if_tun.h>
>  #include <linux/if_macvlan.h>
> 
> +#include <linux/ip.h>
> +#include <linux/udp.h>
> +#include <linux/netdevice.h>
> +
>  #include <net/sock.h>
> 
>  #include "vhost.h"
> @@ -186,6 +190,44 @@ static void handle_tx(struct vhost_net *net)
>  	unuse_mm(net->dev.mm);
>  }
> 
> +static int peek_head(struct sock *sk)

This routine is doing more than just peeking the head of sk's receive
queue. May be this should be named similar to what qemu calls
'work_around_broken_dhclient()'
> +{
> +	struct sk_buff *skb;
> +
> +	lock_sock(sk);
> +	skb = skb_peek(&sk->sk_receive_queue);
> +	if (unlikely(!skb)) {
> +		release_sock(sk);
> +		return 0;
> +	}
> +	/* Userspace virtio server has the following hack so
> +	 * guests rely on it, and we have to replicate it, too: */
> +	/* Use port number to detect incoming IPv4 DHCP response packets,
> +	 * and fill in the checksum. */
> +
> +	/* The issue we are solving is that on linux guests, some apps
> +	 * that use recvmsg with AF_PACKET sockets, don't know how to
> +	 * handle CHECKSUM_PARTIAL;
> +	 * The interface to return the relevant information was added in
> +	 * 8dc4194474159660d7f37c495e3fc3f10d0db8cc,
> +	 * and older userspace does not use it.
> +	 * One important user of recvmsg with AF_PACKET is dhclient,
> +	 * so we add a work-around just for DHCP. */
> +	if (skb->ip_summed == CHECKSUM_PARTIAL &&
> +	    skb_headlen(skb) >= skb_transport_offset(skb) +
> +				sizeof(struct udphdr) &&
> +	    udp_hdr(skb)->dest == htons(68) &&
> +	    skb_network_header_len(skb) >= sizeof(struct iphdr) &&
> +	    ip_hdr(skb)->protocol == IPPROTO_UDP &&
> +	    skb->protocol == htons(ETH_P_IP)) {

Isn't it more logical to check for skb->protocol, followed by ip_hdr and
then udp_hdr?

> +		skb_checksum_help(skb);
> +		/* Restore ip_summed value: tun passes it to user. */
> +		skb->ip_summed = CHECKSUM_PARTIAL;
> +	}
> +	release_sock(sk);
> +	return 1;
> +}
> +
>  /* Expects to be always run from workqueue - which acts as
>   * read-size critical section for our kind of RCU. */
>  static void handle_rx(struct vhost_net *net)
> @@ -222,7 +264,7 @@ static void handle_rx(struct vhost_net *net)
>  	vq_log = unlikely(vhost_has_feature(&net->dev, VHOST_F_LOG_ALL)) ?
>  		vq->log : NULL;
> 
> -	for (;;) {
> +	while (peek_head(sock->sk)) {
>  		head = vhost_get_vq_desc(&net->dev, vq, vq->iov,
>  					 ARRAY_SIZE(vq->iov),
>  					 &out, &in,

Re: [PATCHv2] vhost-net: add dhclient work-around from userspace

[Michael S. Tsirkin]

On Mon, Jun 28, 2010 at 03:19:41PM -0700, Sridhar Samudrala wrote:
> On Mon, 2010-06-28 at 13:08 +0300, Michael S. Tsirkin wrote:
> > Userspace virtio server has the following hack
> > so guests rely on it, and we have to replicate it, too:
> > 
> > Use port number to detect incoming IPv4 DHCP response packets,
> > and fill in the checksum for these.
> > 
> > The issue we are solving is that on linux guests, some apps
> > that use recvmsg with AF_PACKET sockets, don't know how to
> > handle CHECKSUM_PARTIAL;
> > The interface to return the relevant information was added
> > in 8dc4194474159660d7f37c495e3fc3f10d0db8cc,
> > and older userspace does not use it.
> > One important user of recvmsg with AF_PACKET is dhclient,
> > so we add a work-around just for DHCP.
> > 
> > Don't bother applying the hack to IPv6 as userspace virtio does not
> > have a work-around for that - let's hope guests will do the right
> > thing wrt IPv6.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > 
> > Dave, I'm going to put this patch on the vhost tree,
> > no need for you to bother merging it - you'll get
> > it with a pull request.
> > 
> > 
> >  drivers/vhost/net.c |   44 +++++++++++++++++++++++++++++++++++++++++++-
> >  1 files changed, 43 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > index cc19595..03bba6a 100644
> > --- a/drivers/vhost/net.c
> > +++ b/drivers/vhost/net.c
> > @@ -24,6 +24,10 @@
> >  #include <linux/if_tun.h>
> >  #include <linux/if_macvlan.h>
> > 
> > +#include <linux/ip.h>
> > +#include <linux/udp.h>
> > +#include <linux/netdevice.h>
> > +
> >  #include <net/sock.h>
> > 
> >  #include "vhost.h"
> > @@ -186,6 +190,44 @@ static void handle_tx(struct vhost_net *net)
> >  	unuse_mm(net->dev.mm);
> >  }
> > 
> > +static int peek_head(struct sock *sk)
> 
> This routine is doing more than just peeking the head of sk's receive
> queue. May be this should be named similar to what qemu calls
> 'work_around_broken_dhclient()'
> > +{
> > +	struct sk_buff *skb;
> > +
> > +	lock_sock(sk);
> > +	skb = skb_peek(&sk->sk_receive_queue);
> > +	if (unlikely(!skb)) {
> > +		release_sock(sk);
> > +		return 0;
> > +	}
> > +	/* Userspace virtio server has the following hack so
> > +	 * guests rely on it, and we have to replicate it, too: */
> > +	/* Use port number to detect incoming IPv4 DHCP response packets,
> > +	 * and fill in the checksum. */
> > +
> > +	/* The issue we are solving is that on linux guests, some apps
> > +	 * that use recvmsg with AF_PACKET sockets, don't know how to
> > +	 * handle CHECKSUM_PARTIAL;
> > +	 * The interface to return the relevant information was added in
> > +	 * 8dc4194474159660d7f37c495e3fc3f10d0db8cc,
> > +	 * and older userspace does not use it.
> > +	 * One important user of recvmsg with AF_PACKET is dhclient,
> > +	 * so we add a work-around just for DHCP. */
> > +	if (skb->ip_summed == CHECKSUM_PARTIAL &&
> > +	    skb_headlen(skb) >= skb_transport_offset(skb) +
> > +				sizeof(struct udphdr) &&
> > +	    udp_hdr(skb)->dest == htons(68) &&
> > +	    skb_network_header_len(skb) >= sizeof(struct iphdr) &&
> > +	    ip_hdr(skb)->protocol == IPPROTO_UDP &&
> > +	    skb->protocol == htons(ETH_P_IP)) {
> 
> Isn't it more logical to check for skb->protocol, followed by ip_hdr and
> then udp_hdr?


Yes, but then we'll only exit after checking them all.
My way we'll almost always exit after port check.

> > +		skb_checksum_help(skb);
> > +		/* Restore ip_summed value: tun passes it to user. */
> > +		skb->ip_summed = CHECKSUM_PARTIAL;
> > +	}
> > +	release_sock(sk);
> > +	return 1;
> > +}
> > +
> >  /* Expects to be always run from workqueue - which acts as
> >   * read-size critical section for our kind of RCU. */
> >  static void handle_rx(struct vhost_net *net)
> > @@ -222,7 +264,7 @@ static void handle_rx(struct vhost_net *net)
> >  	vq_log = unlikely(vhost_has_feature(&net->dev, VHOST_F_LOG_ALL)) ?
> >  		vq->log : NULL;
> > 
> > -	for (;;) {
> > +	while (peek_head(sock->sk)) {
> >  		head = vhost_get_vq_desc(&net->dev, vq, vq->iov,
> >  					 ARRAY_SIZE(vq->iov),
> >  					 &out, &in,

Re: [PATCHv2] vhost-net: add dhclient work-around from userspace

[David Miller]

From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Mon, 28 Jun 2010 13:08:07 +0300

> Userspace virtio server has the following hack
> so guests rely on it, and we have to replicate it, too:
> 
> Use port number to detect incoming IPv4 DHCP response packets,
> and fill in the checksum for these.
> 
> The issue we are solving is that on linux guests, some apps
> that use recvmsg with AF_PACKET sockets, don't know how to
> handle CHECKSUM_PARTIAL;
> The interface to return the relevant information was added
> in 8dc4194474159660d7f37c495e3fc3f10d0db8cc,
> and older userspace does not use it.
> One important user of recvmsg with AF_PACKET is dhclient,
> so we add a work-around just for DHCP.
> 
> Don't bother applying the hack to IPv6 as userspace virtio does not
> have a work-around for that - let's hope guests will do the right
> thing wrt IPv6.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Yikes, this is awful too.

Nothing in the kernel should be mucking around with procotol packets
like this by default.  In particular, what the heck does port 67 mean?
Locally I can use it for whatever I want for my own purposes, I don't
have to follow the conventions for service ports as specified by the
IETF.

But I can't have the packet checksum state be left alone for port 67
traffic on a box using virtio because you have this hack there.

And yes it's broken on machines using the qemu thing, but at least the
hack there is restricted to userspace.

I really don't want anything in the kernel that looks like this.

These applications are broken, and we've provided a way for them to
work properly.  What's the point of having fixed applications if
all of these hacks grow like fungus over every virtualization transport?

It just means that people won't fix the apps, since they don't have
to.  There is no incentive, and the mechanism we created to properly
handle this loses it's value.

At best, you can write a netfilter module that mucks up the packet
checksum state in these situations.  At least in that case, you can
make it generic (it mangles iff a packet matches a certain rule,
so for your virtio guests you'd make it match for DHCP frames) instead
of being some hard-coded DHCP thing by design.

And since this is so cleanly seperated and portable you don't even
need to push it upstream.  It's a temporary workaround for a temporary
problem.  You can just delete it as soon as the majority of guests
have the fixed dhcp.  The qemu crap should disappear similarly.

Re: [PATCHv2] vhost-net: add dhclient work-around from userspace

[Michael S. Tsirkin]

On Tue, Jun 29, 2010 at 12:36:47AM -0700, David Miller wrote:
> From: "Michael S. Tsirkin" <mst@redhat.com>
> Date: Mon, 28 Jun 2010 13:08:07 +0300
> 
> > Userspace virtio server has the following hack
> > so guests rely on it, and we have to replicate it, too:
> > 
> > Use port number to detect incoming IPv4 DHCP response packets,
> > and fill in the checksum for these.
> > 
> > The issue we are solving is that on linux guests, some apps
> > that use recvmsg with AF_PACKET sockets, don't know how to
> > handle CHECKSUM_PARTIAL;
> > The interface to return the relevant information was added
> > in 8dc4194474159660d7f37c495e3fc3f10d0db8cc,
> > and older userspace does not use it.
> > One important user of recvmsg with AF_PACKET is dhclient,
> > so we add a work-around just for DHCP.
> > 
> > Don't bother applying the hack to IPv6 as userspace virtio does not
> > have a work-around for that - let's hope guests will do the right
> > thing wrt IPv6.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> Yikes, this is awful too.
> 
> Nothing in the kernel should be mucking around with procotol packets
> like this by default.  In particular, what the heck does port 67 mean?
> Locally I can use it for whatever I want for my own purposes, I don't
> have to follow the conventions for service ports as specified by the
> IETF.
> 
> But I can't have the packet checksum state be left alone for port 67
> traffic on a box using virtio because you have this hack there.
> 
> And yes it's broken on machines using the qemu thing, but at least the
> hack there is restricted to userspace.

Yes, and I think it was a mistake to add the hack there. This is what
prevented applications from using the new interface in the 3 years
since it was first introduced.

> I really don't want anything in the kernel that looks like this.
> 
> These applications are broken, and we've provided a way for them to
> work properly.  What's the point of having fixed applications if
> all of these hacks grow like fungus over every virtualization transport?
> 
> It just means that people won't fix the apps, since they don't have
> to.  There is no incentive, and the mechanism we created to properly
> handle this loses it's value.
> 
> At best, you can write a netfilter module that mucks up the packet
> checksum state in these situations.  At least in that case, you can
> make it generic (it mangles iff a packet matches a certain rule,
> so for your virtio guests you'd make it match for DHCP frames) instead
> of being some hard-coded DHCP thing by design.

Nod.
One question on implementation:
why does skb_checksum_help set the checksum state to
CHECKSUM_NONE? Shouldn't it be CHECKSUM_COMPLETE?



> And since this is so cleanly seperated and portable you don't even
> need to push it upstream.  It's a temporary workaround for a temporary
> problem.  You can just delete it as soon as the majority of guests
> have the fixed dhcp.  The qemu crap should disappear similarly.

Since using the module involves updating the management tools
as well, if we go down this route it will be much less painful
for everyone to do push it upstream.

-- 
MST

Re: [PATCHv2] vhost-net: add dhclient work-around from userspace

[David Miller]

From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Tue, 29 Jun 2010 16:04:39 +0300

> Since using the module involves updating the management tools
> as well, if we go down this route it will be much less painful
> for everyone to do push it upstream.

Ok, you can make your case to Patrick McHardy and if he'll merge
it into his netfilter GIT tree I guess I'll have to take it :)

Re: [PATCHv2] vhost-net: add dhclient work-around from userspace

[Anthony Liguori]

On 06/29/2010 08:04 AM, Michael S. Tsirkin wrote:
> On Tue, Jun 29, 2010 at 12:36:47AM -0700, David Miller wrote:
>    
>> From: "Michael S. Tsirkin"<mst@redhat.com>
>> Date: Mon, 28 Jun 2010 13:08:07 +0300
>>
>>      
>>> Userspace virtio server has the following hack
>>> so guests rely on it, and we have to replicate it, too:
>>>
>>> Use port number to detect incoming IPv4 DHCP response packets,
>>> and fill in the checksum for these.
>>>
>>> The issue we are solving is that on linux guests, some apps
>>> that use recvmsg with AF_PACKET sockets, don't know how to
>>> handle CHECKSUM_PARTIAL;
>>> The interface to return the relevant information was added
>>> in 8dc4194474159660d7f37c495e3fc3f10d0db8cc,
>>> and older userspace does not use it.
>>> One important user of recvmsg with AF_PACKET is dhclient,
>>> so we add a work-around just for DHCP.
>>>
>>> Don't bother applying the hack to IPv6 as userspace virtio does not
>>> have a work-around for that - let's hope guests will do the right
>>> thing wrt IPv6.
>>>
>>> Signed-off-by: Michael S. Tsirkin<mst@redhat.com>
>>>        
>> Yikes, this is awful too.
>>
>> Nothing in the kernel should be mucking around with procotol packets
>> like this by default.  In particular, what the heck does port 67 mean?
>> Locally I can use it for whatever I want for my own purposes, I don't
>> have to follow the conventions for service ports as specified by the
>> IETF.
>>
>> But I can't have the packet checksum state be left alone for port 67
>> traffic on a box using virtio because you have this hack there.
>>
>> And yes it's broken on machines using the qemu thing, but at least the
>> hack there is restricted to userspace.
>>      
> Yes, and I think it was a mistake to add the hack there. This is what
> prevented applications from using the new interface in the 3 years
> since it was first introduced.
>    

It's far more complicated than that.  dhclient is part of ISC's DHCP 
package.  They do not have a public SCM and instead require you to join 
their Software Guild to get access to it.

This problem was identified in one distribution and the patch was pushed 
upstream but because they did not have a public SCM, most other 
distributions did not see the fix until it appeared in a release.  ISC 
has a pretty long release cycle historically.

ISC's had the fix for a long time but there was a 3-year gap in their 
releases and since their SCM isn't public, users are stuck with the last 
release.

This hack makes sense in QEMU as we have a few hacks like this to fix 
broken guests.  A primary use of virtualization is to run old 
applications so it makes sense for us to do that.

I don't think it makes sense for vhost to do this.  These guests are so 
old that they don't have the requisite features to achieve really high 
performance anyway.

I've always thought making vhost totally transparent was a bad idea and 
this is one of the reasons.  We can do a lot of ugly things in userspace 
that we shouldn't be doing in the kernel.

Regards,

Anthony Liguori

Re: [PATCHv2] vhost-net: add dhclient work-around from userspace

[Michael S. Tsirkin]

On Wed, Jun 30, 2010 at 05:08:11PM -0500, Anthony Liguori wrote:
> On 06/29/2010 08:04 AM, Michael S. Tsirkin wrote:
> >On Tue, Jun 29, 2010 at 12:36:47AM -0700, David Miller wrote:
> >>From: "Michael S. Tsirkin"<mst@redhat.com>
> >>Date: Mon, 28 Jun 2010 13:08:07 +0300
> >>
> >>>Userspace virtio server has the following hack
> >>>so guests rely on it, and we have to replicate it, too:
> >>>
> >>>Use port number to detect incoming IPv4 DHCP response packets,
> >>>and fill in the checksum for these.
> >>>
> >>>The issue we are solving is that on linux guests, some apps
> >>>that use recvmsg with AF_PACKET sockets, don't know how to
> >>>handle CHECKSUM_PARTIAL;
> >>>The interface to return the relevant information was added
> >>>in 8dc4194474159660d7f37c495e3fc3f10d0db8cc,
> >>>and older userspace does not use it.
> >>>One important user of recvmsg with AF_PACKET is dhclient,
> >>>so we add a work-around just for DHCP.
> >>>
> >>>Don't bother applying the hack to IPv6 as userspace virtio does not
> >>>have a work-around for that - let's hope guests will do the right
> >>>thing wrt IPv6.
> >>>
> >>>Signed-off-by: Michael S. Tsirkin<mst@redhat.com>
> >>Yikes, this is awful too.
> >>
> >>Nothing in the kernel should be mucking around with procotol packets
> >>like this by default.  In particular, what the heck does port 67 mean?
> >>Locally I can use it for whatever I want for my own purposes, I don't
> >>have to follow the conventions for service ports as specified by the
> >>IETF.
> >>
> >>But I can't have the packet checksum state be left alone for port 67
> >>traffic on a box using virtio because you have this hack there.
> >>
> >>And yes it's broken on machines using the qemu thing, but at least the
> >>hack there is restricted to userspace.
> >Yes, and I think it was a mistake to add the hack there. This is what
> >prevented applications from using the new interface in the 3 years
> >since it was first introduced.
> 
> It's far more complicated than that.  dhclient is part of ISC's DHCP
> package.  They do not have a public SCM and instead require you to
> join their Software Guild to get access to it.
> 
> This problem was identified in one distribution and the patch was
> pushed upstream but because they did not have a public SCM, most
> other distributions did not see the fix until it appeared in a
> release.  ISC has a pretty long release cycle historically.
> 
> ISC's had the fix for a long time but there was a 3-year gap in
> their releases and since their SCM isn't public, users are stuck
> with the last release.
> 
> This hack makes sense in QEMU as we have a few hacks like this to
> fix broken guests.
>  A primary use of virtualization is to run old
> applications so it makes sense for us to do that.

IMO it was wrong to put it in qemu: originally, if a distro shipped
a broken virtio/dhclient combo, it was it's own bug to fix.
But now that qemu has shipped the work-around for so long,
broken guests seemed work. So we *still* see the bug re-surface in new guests.

And since they are fairly new, it is interesting to
get decent performance from them now.

> 
> I don't think it makes sense for vhost to do this.  These guests are
> so old that they don't have the requisite features to achieve really
> high performance anyway.
> 
> I've always thought making vhost totally transparent was a bad idea
> and this is one of the reasons.

It does not have to be fully transparent. You can insert your own ring
in the middle, and copy descriptors around.  And we stop on errors and
let userspace handle.  This will come handy if we get e.g. virtio bug
that we need to work around.

> We can do a lot of ugly things in
> userspace that we shouldn't be doing in the kernel.
> 
> Regards,
> 
> Anthony Liguori

QEMU is only userspace for the host. It is the hardware for the guest.
So IMO we should not be doing the ugly things there either.

-- 
MST

Re: [PATCHv2] vhost-net: add dhclient work-around from userspace

[Anthony Liguori]

On 06/30/2010 05:31 PM, Michael S. Tsirkin wrote:
> On Wed, Jun 30, 2010 at 05:08:11PM -0500, Anthony Liguori wrote:
>    
>> On 06/29/2010 08:04 AM, Michael S. Tsirkin wrote:
>>      
>>> On Tue, Jun 29, 2010 at 12:36:47AM -0700, David Miller wrote:
>>>        
>>>> From: "Michael S. Tsirkin"<mst@redhat.com>
>>>> Date: Mon, 28 Jun 2010 13:08:07 +0300
>>>>
>>>>          
>>>>> Userspace virtio server has the following hack
>>>>> so guests rely on it, and we have to replicate it, too:
>>>>>
>>>>> Use port number to detect incoming IPv4 DHCP response packets,
>>>>> and fill in the checksum for these.
>>>>>
>>>>> The issue we are solving is that on linux guests, some apps
>>>>> that use recvmsg with AF_PACKET sockets, don't know how to
>>>>> handle CHECKSUM_PARTIAL;
>>>>> The interface to return the relevant information was added
>>>>> in 8dc4194474159660d7f37c495e3fc3f10d0db8cc,
>>>>> and older userspace does not use it.
>>>>> One important user of recvmsg with AF_PACKET is dhclient,
>>>>> so we add a work-around just for DHCP.
>>>>>
>>>>> Don't bother applying the hack to IPv6 as userspace virtio does not
>>>>> have a work-around for that - let's hope guests will do the right
>>>>> thing wrt IPv6.
>>>>>
>>>>> Signed-off-by: Michael S. Tsirkin<mst@redhat.com>
>>>>>            
>>>> Yikes, this is awful too.
>>>>
>>>> Nothing in the kernel should be mucking around with procotol packets
>>>> like this by default.  In particular, what the heck does port 67 mean?
>>>> Locally I can use it for whatever I want for my own purposes, I don't
>>>> have to follow the conventions for service ports as specified by the
>>>> IETF.
>>>>
>>>> But I can't have the packet checksum state be left alone for port 67
>>>> traffic on a box using virtio because you have this hack there.
>>>>
>>>> And yes it's broken on machines using the qemu thing, but at least the
>>>> hack there is restricted to userspace.
>>>>          
>>> Yes, and I think it was a mistake to add the hack there. This is what
>>> prevented applications from using the new interface in the 3 years
>>> since it was first introduced.
>>>        
>> It's far more complicated than that.  dhclient is part of ISC's DHCP
>> package.  They do not have a public SCM and instead require you to
>> join their Software Guild to get access to it.
>>
>> This problem was identified in one distribution and the patch was
>> pushed upstream but because they did not have a public SCM, most
>> other distributions did not see the fix until it appeared in a
>> release.  ISC has a pretty long release cycle historically.
>>
>> ISC's had the fix for a long time but there was a 3-year gap in
>> their releases and since their SCM isn't public, users are stuck
>> with the last release.
>>
>> This hack makes sense in QEMU as we have a few hacks like this to
>> fix broken guests.
>>   A primary use of virtualization is to run old
>> applications so it makes sense for us to do that.
>>      
> IMO it was wrong to put it in qemu: originally, if a distro shipped
> a broken virtio/dhclient combo, it was it's own bug to fix.
> But now that qemu has shipped the work-around for so long,
> broken guests seemed work.

The guests were broken before qemu implemented this.

virtio-net had checksum offload long before it was ever implemented in 
qemu.  Not even lguest implemented it because the interfaces weren't 
available in tun/tap.  I'm not sure how Rusty ever tested it.  We only 
discovered this bug after checksum offload was implemented in tun/tap 
and we were able to enable it in QEMU.  At that point, the guests had 
shipped and were in the wild.

The real problem was that we implemented a feature in a guest driver 
without having a backend implementation and then shipped the code.  
Shipping untested code is a recipe for failure.

If we had implemented the front-end feature only when a backend 
implementation was available, we would have caught this, fixed it in the 
guests, and not be in the situation because there wouldn't be these 
broken guests.


>   So we *still* see the bug re-surface in new guests.
>    

Which guests?  Newer versions of dhclient should work as expected.

> And since they are fairly new, it is interesting to
> get decent performance from them now.
>
>    
>> I don't think it makes sense for vhost to do this.  These guests are
>> so old that they don't have the requisite features to achieve really
>> high performance anyway.
>>
>> I've always thought making vhost totally transparent was a bad idea
>> and this is one of the reasons.
>>      
> It does not have to be fully transparent. You can insert your own ring
> in the middle, and copy descriptors around.  And we stop on errors and
> let userspace handle.  This will come handy if we get e.g. virtio bug
> that we need to work around.
>    

I mean from a UI perspective.  IOW, if users have to explicitly choose 
to use vhost-net, then it's okay to force them to use newer guests that 
support vhost-net.  However, if we make it transparent, then it has to 
support everything that QEMU virtio has ever supported which is 
problematic for exactly the reasons you are now encountering.

>> We can do a lot of ugly things in
>> userspace that we shouldn't be doing in the kernel.
>>
>> Regards,
>>
>> Anthony Liguori
>>      
> QEMU is only userspace for the host. It is the hardware for the guest.
> So IMO we should not be doing the ugly things there either.
>    

Shouldn't we apply the same argument to the Windows RTC implementation 
and say that Windows should not rely on counting interrupts?  Or that it 
shouldn't spin in a tight loop checking interrupt status with interrupts 
disabled after receiving an interrupt?

Supporting broken guests is a big part of what we do in QEMU.  We do 
what we need to do to make guests that we cannot change work.  When this 
first was implemented, there were a good number of pre-existing guests 
that broke because we enabled checksum offload.

If we can fix the guests to avoid doing ugly things in QEMU, we should, 
but we can't regress an otherwise working guest just because we think 
the solution is ugly in QEMU.

Regards,

Anthony Liguori

Re: [PATCHv2] vhost-net: add dhclient work-around from userspace

[xming]

Hi,

I think I am hitting this, in a weird way :D. Running 0.12.90,
vhost_net and kernel 2.6.35.4, I have troubles to get dhcp clients to
work except an old dhcp client "pump" seems to work out of the box.
Below are the output of dhcpcd, isc's dhcp client and busybox' udhcpc.

My dhcp server is in a guest with vhost_net running dnsmasq. Clients
which do not use vhost_net (plain virtio, e1000 and physical machines)
do not have any problems. The dhcp server itself is a dhcp client and
uses dhcpcd to get lease from ADSL modem w/o issues.
When the dhcp server is not using vhost_net, then all clients can get a lease.

So my conclusion so far is that when the server is using vhost_net and
clients are using vhost_net (and above mentioned software), then it's
getting invalid/checksum error UDP. the only working DHCP client is
pump.

# dhcpcd
dhcpcd[9089]: version 5.2.7 starting
dhcpcd[9089]: eth0: broadcasting for a lease
dhcpcd[9089]: eth0: invalid UDP packet from 192.168.1.1
dhcpcd[9089]: eth0: invalid UDP packet from 192.168.1.1
dhcpcd[9089]: eth0: invalid UDP packet from 192.168.1.1
dhcpcd[9089]: eth0: invalid UDP packet from 192.168.1.1
dhcpcd[9089]: timed out
dhcpcd[9089]: allowing 8 seconds for IPv4LL timeout
dhcpcd[9089]: eth0: probing for an IPv4LL address
dhcpcd[9089]: eth0: checking for 169.254.195.227
dhcpcd[9089]: eth0: using IPv4LL address 169.254.195.227
dhcpcd[9089]: forked to background, child pid 9109

# dhclient
Internet Systems Consortium DHCP Client V3.1.2p1-Gentoo
Copyright 2004-2009 Internet Systems Consortium.
All rights reserved.
For info, please visit http://www.isc.org/sw/dhcp/

Listening on LPF/eth0/00:16:3e:00:07:01
Sending on   LPF/eth0/00:16:3e:00:07:01
Sending on   Socket/fallback
DHCPDISCOVER on eth0 to 255.255.255.255 port 67 interval 7
DHCPDISCOVER on eth0 to 255.255.255.255 port 67 interval 7
DHCPDISCOVER on eth0 to 255.255.255.255 port 67 interval 7
DHCPDISCOVER on eth0 to 255.255.255.255 port 67 interval 13
DHCPDISCOVER on eth0 to 255.255.255.255 port 67 interval 13
5 bad udp checksums in 5 packets
DHCPDISCOVER on eth0 to 255.255.255.255 port 67 interval 14
No DHCPOFFERS received.
No working leases in persistent database - sleeping.


# busybox udhcpc
### adapter index 2
### adapter hardware address 00:16:3e:00:07:01
udhcpc (v1.13.2) started
### vfork'ing and execle'ing /usr/share/udhcpc/default.script
udhcpc: exec /usr/share/udhcpc/default.script: No such file or directory
### entering raw listen mode
### opening raw socket on ifindex 2
### got raw socket fd 5
### attached filter to raw socket fd 5
### bound to raw socket fd 5
### adding option 0x35
### adding option 0x3d
### adding option 0x3c
### adding option 0x39
Sending discover...
### Waiting on select...
udhcpc: packet with bad UDP checksum received, ignoring
### Waiting on select...
### adding option 0x35
### adding option 0x3d
### adding option 0x3c
### adding option 0x39
Sending discover...
### Waiting on select...
udhcpc: packet with bad UDP checksum received, ignoring
### Waiting on select...
### adding option 0x35
### adding option 0x3d
### adding option 0x3c
### adding option 0x39
Sending discover...
### Waiting on select...
udhcpc: packet with bad UDP checksum received, ignoring
### Waiting on select...
### vfork'ing and execle'ing /usr/share/udhcpc/default.script
udhcpc: exec /usr/share/udhcpc/default.script: No such file or directory
### Waiting on select...
### adding option 0x35
### adding option 0x3d
### adding option 0x3c
### adding option 0x39
Sending discover...
### Waiting on select...
udhcpc: packet with bad UDP checksum received, ignoring
### Waiting on select...

Re: [PATCHv2] vhost-net: add dhclient work-around from userspace

[Michael S. Tsirkin]

On Mon, Sep 20, 2010 at 02:12:40PM +0200, xming wrote:
> Hi,
> 
> I think I am hitting this, in a weird way :D. Running 0.12.90,
> vhost_net and kernel 2.6.35.4, I have troubles to get dhcp clients to
> work except an old dhcp client "pump" seems to work out of the box.
> Below are the output of dhcpcd, isc's dhcp client and busybox' udhcpc.
> 
> My dhcp server is in a guest with vhost_net running dnsmasq. Clients
> which do not use vhost_net (plain virtio, e1000 and physical machines)
> do not have any problems. The dhcp server itself is a dhcp client and
> uses dhcpcd to get lease from ADSL modem w/o issues.
> When the dhcp server is not using vhost_net, then all clients can get a lease.
> 
> So my conclusion so far is that when the server is using vhost_net and
> clients are using vhost_net (and above mentioned software), then it's
> getting invalid/checksum error UDP. the only working DHCP client is
> pump.

Newer versions of dhclient should also be OK: they detect
that checksum is missing in the packet. Try it e.g. with
a recent fedora guest as a client.

To solve the problem for old clients, recent kernels and iptables have
support for CHECKSUM target.

You can use this target to compute and fill in the checksum in
a packet that lacks a checksum.

Typical expected use:
iptables -A POSTROUTING -t mangle -p udp --dport bootpc \
-j CHECKSUM --checksum-fill

libvirt will program these for you if it sets up the server,
maybe there needs a flag to tell it that server is local.

> # dhcpcd
> dhcpcd[9089]: version 5.2.7 starting
> dhcpcd[9089]: eth0: broadcasting for a lease
> dhcpcd[9089]: eth0: invalid UDP packet from 192.168.1.1
> dhcpcd[9089]: eth0: invalid UDP packet from 192.168.1.1
> dhcpcd[9089]: eth0: invalid UDP packet from 192.168.1.1
> dhcpcd[9089]: eth0: invalid UDP packet from 192.168.1.1
> dhcpcd[9089]: timed out
> dhcpcd[9089]: allowing 8 seconds for IPv4LL timeout
> dhcpcd[9089]: eth0: probing for an IPv4LL address
> dhcpcd[9089]: eth0: checking for 169.254.195.227
> dhcpcd[9089]: eth0: using IPv4LL address 169.254.195.227
> dhcpcd[9089]: forked to background, child pid 9109
> 
> # dhclient
> Internet Systems Consortium DHCP Client V3.1.2p1-Gentoo
> Copyright 2004-2009 Internet Systems Consortium.
> All rights reserved.
> For info, please visit http://www.isc.org/sw/dhcp/
> 
> Listening on LPF/eth0/00:16:3e:00:07:01
> Sending on   LPF/eth0/00:16:3e:00:07:01
> Sending on   Socket/fallback
> DHCPDISCOVER on eth0 to 255.255.255.255 port 67 interval 7
> DHCPDISCOVER on eth0 to 255.255.255.255 port 67 interval 7
> DHCPDISCOVER on eth0 to 255.255.255.255 port 67 interval 7
> DHCPDISCOVER on eth0 to 255.255.255.255 port 67 interval 13
> DHCPDISCOVER on eth0 to 255.255.255.255 port 67 interval 13
> 5 bad udp checksums in 5 packets
> DHCPDISCOVER on eth0 to 255.255.255.255 port 67 interval 14
> No DHCPOFFERS received.
> No working leases in persistent database - sleeping.
> 
> 
> # busybox udhcpc
> ### adapter index 2
> ### adapter hardware address 00:16:3e:00:07:01
> udhcpc (v1.13.2) started
> ### vfork'ing and execle'ing /usr/share/udhcpc/default.script
> udhcpc: exec /usr/share/udhcpc/default.script: No such file or directory
> ### entering raw listen mode
> ### opening raw socket on ifindex 2
> ### got raw socket fd 5
> ### attached filter to raw socket fd 5
> ### bound to raw socket fd 5
> ### adding option 0x35
> ### adding option 0x3d
> ### adding option 0x3c
> ### adding option 0x39
> Sending discover...
> ### Waiting on select...
> udhcpc: packet with bad UDP checksum received, ignoring
> ### Waiting on select...
> ### adding option 0x35
> ### adding option 0x3d
> ### adding option 0x3c
> ### adding option 0x39
> Sending discover...
> ### Waiting on select...
> udhcpc: packet with bad UDP checksum received, ignoring
> ### Waiting on select...
> ### adding option 0x35
> ### adding option 0x3d
> ### adding option 0x3c
> ### adding option 0x39
> Sending discover...
> ### Waiting on select...
> udhcpc: packet with bad UDP checksum received, ignoring
> ### Waiting on select...
> ### vfork'ing and execle'ing /usr/share/udhcpc/default.script
> udhcpc: exec /usr/share/udhcpc/default.script: No such file or directory
> ### Waiting on select...
> ### adding option 0x35
> ### adding option 0x3d
> ### adding option 0x3c
> ### adding option 0x39
> Sending discover...
> ### Waiting on select...
> udhcpc: packet with bad UDP checksum received, ignoring
> ### Waiting on select...
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2] vhost-net: add dhclient work-around from userspace

[xming]

> Newer versions of dhclient should also be OK: they detect
> that checksum is missing in the packet. Try it e.g. with
> a recent fedora guest as a client.

I don't have fedora, but with the latest release (4.1.1-P1) on isc.org
it still behaves the same (see output at the bottom).

> To solve the problem for old clients, recent kernels and iptables have
> support for CHECKSUM target.
>
> You can use this target to compute and fill in the checksum in
> a packet that lacks a checksum.
>
> Typical expected use:
> iptables -A POSTROUTING -t mangle -p udp --dport bootpc \
> -j CHECKSUM --checksum-fill

Nice trick :D

> libvirt will program these for you if it sets up the server,
> maybe there needs a flag to tell it that server is local.

I don't use libvirt.

My point is, there doesn't seem to be much working client and the only
working client is a ver very old one (pump), newer client do not work,
as opposite to what you have explained.

To repeat myself, here is the situation:

- DHCP server with vhost_net, all client w/o vhost_net work, clients
with vhost_net do not work except pump
- DHCP server w/o vhost_net, all clients work
- physical DHCP server, client with vhost *do* work.


--- output of the lates DHCP client ---

Internet Systems Consortium DHCP Client 4.1.1-P1
Copyright 2004-2010 Internet Systems Consortium.
All rights reserved.
For info, please visit https://www.isc.org/software/dhcp/

Listening on LPF/eth0/00:16:3e:00:07:01
Sending on   LPF/eth0/00:16:3e:00:07:01
Sending on   Socket/fallback
DHCPDISCOVER on eth0 to 255.255.255.255 port 67 interval 6
DHCPDISCOVER on eth0 to 255.255.255.255 port 67 interval 13
DHCPDISCOVER on eth0 to 255.255.255.255 port 67 interval 14
DHCPDISCOVER on eth0 to 255.255.255.255 port 67 interval 10
DHCPDISCOVER on eth0 to 255.255.255.255 port 67 interval 18
5 bad udp checksums in 5 packets
No DHCPOFFERS received.
No working leases in persistent database - sleeping.

Re: [PATCHv2] vhost-net: add dhclient work-around from userspace

[Michael S. Tsirkin]

On Tue, Sep 21, 2010 at 10:27:45AM +0200, xming wrote:
> > Newer versions of dhclient should also be OK: they detect
> > that checksum is missing in the packet. Try it e.g. with
> > a recent fedora guest as a client.
> 
> I don't have fedora, but with the latest release (4.1.1-P1) on isc.org
> it still behaves the same (see output at the bottom).
> 
> > To solve the problem for old clients, recent kernels and iptables have
> > support for CHECKSUM target.
> >
> > You can use this target to compute and fill in the checksum in
> > a packet that lacks a checksum.
> >
> > Typical expected use:
> > iptables -A POSTROUTING -t mangle -p udp --dport bootpc \
> > -j CHECKSUM --checksum-fill
> 
> Nice trick :D

Does it help?

> > libvirt will program these for you if it sets up the server,
> > maybe there needs a flag to tell it that server is local.
> 
> I don't use libvirt.

Does it help if you program the rule above?

> My point is, there doesn't seem to be much working client and the only
> working client is a ver very old one (pump), newer client do not work,
> as opposite to what you have explained.

Sorry if I misled you. dhclient in Fedora is ok and has been for
a long while but the bugfix does not appear to be present in released
clients from isc.org, or so it seems from examining the source.
I don't know why and whether is was ever sent their way:
it might be a good idea to talk to them on why they don't
use the PACKET_AUXDATA checksum API linux has had since 2007.
Meanwhile, you can try adding the patch from fedora.

> To repeat myself, here is the situation:
> 
> - DHCP server with vhost_net, all client w/o vhost_net work, clients
> with vhost_net do not work except pump
> - DHCP server w/o vhost_net, all clients work

Hmm are you sure? I expect vhost net client to still not work.
At least, the hack in qemu (that vhost-net disables) is for dhcp response
packets, I do not see how it will help the server ...

> - physical DHCP server, client with vhost *do* work.
> 

Yes, qemu has a hack that catches typical use and will
attach a checksum on receive. Unfortunately keeping
that around made it all too easy for vendors to keep
shipping buggy clients (and I presume server has a similar
issue).

So the only quick solution I have is to work around this
in kernel: in this case by programming the iptables rule.

-- 
MST