Re: [RFC PATCH 0/2] virtio-mmio: add irqfd support for vhost-net based on virtio-mmio

2014-10-26 Thread Michael S. Tsirkin
On Sat, Oct 25, 2014 at 04:24:52PM +0800, john.liuli wrote:
> From: Li Liu 
> 
> This set of patches try to implemet irqfd support of vhost-net 
> based on virtio-mmio.
> 
> I had posted a mail to talking about the status of vhost-net 
> on kvm-arm refer to http://www.spinics.net/lists/kvm-arm/msg10804.html.
> Some dependent patches are listed in the mail too. Basically the 
> vhost-net brings great performance improvements, almost 50%+.
> 
> It's easy to implement irqfd support with PCI MSI-X. But till 
> now arm32 do not provide equivalent mechanism to let a device 
> allocate multiple interrupts. And even the aarch64 provid LPI
> but also not available in a short time.
> 
> As Gauguey Remy said "Vhost does not emulate a complete virtio 
> adapter but only manage virtqueue operations". Vhost module
> don't update the ISR register, so if with only one irq then it's 
> no way to get the interrupt reason even we can inject the 
> irq correctly.  

Well guests don't read ISR in MSI-X mode so why does it help
to set the ISR bit?

> To get the interrupt reason to support such VIRTIO_NET_F_STATUS 
> features I add a new register offset VIRTIO_MMIO_ISRMEM which 
> will help to establish a shared memory region between qemu and 
> virtio-mmio device. Then the interrupt reason can be accessed by
> guest driver through this region. At the same time, the virtio-mmio 
> dirver check this region to see irqfd is supported or not during 
> the irq handler registration, and different handler will be assigned.
> 
> I want to know it's the right direction? Does it comply with the 
> virtio-mmio spec.? Or anyone have more good ideas to emulate mis-x 
> based on virtio-mmio? I hope to get feedback and guidance.
> Thx for any help.
> 
> Li Liu (2):
>   Add a new register offset let interrupt reason available
>   Assign a new irq handler while irqfd enabled
> 
>  drivers/virtio/virtio_mmio.c |   55 
> +++---
>  include/linux/virtio_mmio.h  |3 +++
>  2 files changed, 55 insertions(+), 3 deletions(-)
> 
> -- 
> 1.7.9.5
> 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [RFC PATCH 2/2] Assign a new irq handler while irqfd enabled

2014-10-26 Thread Michael S. Tsirkin
On Sat, Oct 25, 2014 at 04:24:54PM +0800, john.liuli wrote:
> From: Li Liu 
> 
> This irq handler will get the interrupt reason from a
> shared memory. And will be assigned only while irqfd
> enabled.
> 
> Signed-off-by: Li Liu 
> ---
>  drivers/virtio/virtio_mmio.c |   34 --
>  1 file changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> index 28ddb55..7229605 100644
> --- a/drivers/virtio/virtio_mmio.c
> +++ b/drivers/virtio/virtio_mmio.c
> @@ -259,7 +259,31 @@ static irqreturn_t vm_interrupt(int irq, void *opaque)
>   return ret;
>  }
>  
> +/* Notify all virtqueues on an interrupt. */
> +static irqreturn_t vm_interrupt_irqfd(int irq, void *opaque)
> +{
> + struct virtio_mmio_device *vm_dev = opaque;
> + struct virtio_mmio_vq_info *info;
> + unsigned long status;
> + unsigned long flags;
> + irqreturn_t ret = IRQ_NONE;
>  
> + /* Read the interrupt reason and reset it */
> + status = *vm_dev->isr_mem;
> + *vm_dev->isr_mem = 0x0;

you are reading and modifying shared memory
without atomics and any memory barriers.
Why is this safe?

> +
> + if (unlikely(status & VIRTIO_MMIO_INT_CONFIG)) {
> + virtio_config_changed(&vm_dev->vdev);
> + ret = IRQ_HANDLED;
> + }
> +
> + spin_lock_irqsave(&vm_dev->lock, flags);
> + list_for_each_entry(info, &vm_dev->virtqueues, node)
> + ret |= vring_interrupt(irq, info->vq);
> + spin_unlock_irqrestore(&vm_dev->lock, flags);
> +
> + return ret;
> +}
>  
>  static void vm_del_vq(struct virtqueue *vq)
>  {

So you invoke callbacks for all VQs.
This won't scale well as the number of VQs grows, will it?

> @@ -391,6 +415,7 @@ error_available:
>   return ERR_PTR(err);
>  }
>  
> +#define VIRTIO_MMIO_F_IRQFD(1 << 7)
>  static int vm_find_vqs(struct virtio_device *vdev, unsigned nvqs,
>  struct virtqueue *vqs[],
>  vq_callback_t *callbacks[],
> @@ -400,8 +425,13 @@ static int vm_find_vqs(struct virtio_device *vdev, 
> unsigned nvqs,
>   unsigned int irq = platform_get_irq(vm_dev->pdev, 0);
>   int i, err;
>  
> - err = request_irq(irq, vm_interrupt, IRQF_SHARED,
> - dev_name(&vdev->dev), vm_dev);
> + if (*vm_dev->isr_mem & VIRTIO_MMIO_F_IRQFD) {
> + err = request_irq(irq, vm_interrupt_irqfd, IRQF_SHARED,
> +   dev_name(&vdev->dev), vm_dev);
> + } else {
> + err = request_irq(irq, vm_interrupt, IRQF_SHARED,
> +   dev_name(&vdev->dev), vm_dev);
> + }
>   if (err)
>   return err;


So still a single interrupt for all VQs.
Again this doesn't scale: a single CPU has to handle
interrupts for all of them.
I think you need to find a way to get per-VQ interrupts.

> -- 
> 1.7.9.5
> 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [RFC PATCH 1/2] Add a new register offset let interrupt reason available

2014-10-26 Thread Michael S. Tsirkin
On Sat, Oct 25, 2014 at 04:24:53PM +0800, john.liuli wrote:
> From: Li Liu 
> 
> Add a new register offset VIRTIO_MMIO_ISRMEM which help to
> estblish a shared memory region between virtio-mmio driver
> and qemu with two purposes:
> 
> 1.Guest virtio-mmio driver can get the interrupt reason.
> 2.Check irqfd enabled or not to register different irq handler.
> 
> Signed-off-by: Li Liu 
> ---
>  drivers/virtio/virtio_mmio.c |   21 -
>  include/linux/virtio_mmio.h  |3 +++
>  2 files changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> index ef9a165..28ddb55 100644
> --- a/drivers/virtio/virtio_mmio.c
> +++ b/drivers/virtio/virtio_mmio.c
> @@ -122,6 +122,8 @@ struct virtio_mmio_device {
>   /* a list of queues so we can dispatch IRQs */
>   spinlock_t lock;
>   struct list_head virtqueues;
> +
> + uint8_t *isr_mem;
>  };
>  
>  struct virtio_mmio_vq_info {
> @@ -443,6 +445,7 @@ static int virtio_mmio_probe(struct platform_device *pdev)
>   struct virtio_mmio_device *vm_dev;
>   struct resource *mem;
>   unsigned long magic;
> + int err;
>  
>   mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>   if (!mem)
> @@ -481,6 +484,15 @@ static int virtio_mmio_probe(struct platform_device 
> *pdev)
>   return -ENXIO;
>   }
>  
> + vm_dev->isr_mem = alloc_pages_exact(PAGE_SIZE, GFP_KERNEL|__GFP_ZERO);
> + if (vm_dev->isr_mem == NULL) {
> + dev_err(&pdev->dev, "Allocate isr memory failed!\n");
> + return -ENOMEM;
> + }
> +
> + writel(virt_to_phys(vm_dev->isr_mem),
> +vm_dev->base + VIRTIO_MMIO_ISRMEM);
> +

What happens to existing devices?
then might not expect writes at this address.

>   vm_dev->vdev.id.device = readl(vm_dev->base + VIRTIO_MMIO_DEVICE_ID);
>   vm_dev->vdev.id.vendor = readl(vm_dev->base + VIRTIO_MMIO_VENDOR_ID);
>  
> @@ -488,13 +500,20 @@ static int virtio_mmio_probe(struct platform_device 
> *pdev)
>  
>   platform_set_drvdata(pdev, vm_dev);
>  
> - return register_virtio_device(&vm_dev->vdev);
> + err = register_virtio_device(&vm_dev->vdev);
> + if (err) {
> + free_pages_exact(vm_dev->isr_mem, PAGE_SIZE);
> + vm_dev->isr_mem = NULL;
> + }
> +
> + return err;
>  }
>  
>  static int virtio_mmio_remove(struct platform_device *pdev)
>  {
>   struct virtio_mmio_device *vm_dev = platform_get_drvdata(pdev);
>  
> + free_pages_exact(vm_dev->isr_mem, PAGE_SIZE);
>   unregister_virtio_device(&vm_dev->vdev);
>  
>   return 0;
> diff --git a/include/linux/virtio_mmio.h b/include/linux/virtio_mmio.h
> index 5c7b6f0..b1e3ec7 100644
> --- a/include/linux/virtio_mmio.h
> +++ b/include/linux/virtio_mmio.h
> @@ -95,6 +95,9 @@
>  /* Device status register - Read Write */
>  #define VIRTIO_MMIO_STATUS   0x070
>  
> +/* Allocate ISRMEM for interrupt reason - Write Only */
> +#define VIRTIO_MMIO_ISRMEM   0x080
> +
>  /* The config space is defined by each driver as
>   * the per-driver configuration space - Read Write */
>  #define VIRTIO_MMIO_CONFIG   0x100
> -- 
> 1.7.9.5
> 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: IPv6 UFO for VMs

2014-10-26 Thread Ben Hutchings
On Wed, 2014-10-22 at 11:35 +0200, Hannes Frederic Sowa wrote:
> On Mi, 2014-10-22 at 00:44 +0100, Ben Hutchings wrote:
> > There are several ways that VMs can take advantage of UFO and get the
> > host to do fragmentation for them:
> > 
> > drivers/net/macvtap.c:  gso_type = SKB_GSO_UDP;
> > drivers/net/tun.c:  skb_shinfo(skb)->gso_type = 
> > SKB_GSO_UDP;
> > drivers/net/virtio_net.c:   skb_shinfo(skb)->gso_type = 
> > SKB_GSO_UDP;
> > 
> > Our implementation of UFO for IPv6 does:
> > 
> > fptr = (struct frag_hdr *)(skb_network_header(skb) + 
> > unfrag_ip6hlen);
> > fptr->nexthdr = nexthdr;
> > fptr->reserved = 0;
> > fptr->identification = skb_shinfo(skb)->ip6_frag_id;
> > 
> > which assumes ip6_frag_id has been set.  That's only true if the local
> > stack constructed the skb; otherwise it appears we get zero.
> > 
> > This seems to be a regression as a result of:
> > 
> > commit 916e4cf46d0204806c062c8c6c4d1f633852c5b6
> > Author: Hannes Frederic Sowa 
> > Date:   Fri Feb 21 02:55:35 2014 +0100
> > 
> > ipv6: reuse ip6_frag_id from ip6_ufo_append_data
> > 
> > However, that change seems reasonable - we *shouldn't* be choosing IDs
> > for any other stack.  Any paravirt net driver that can use IPv6 UFO
> > needs to have some way of passing a fragmentation ID to put in
> > skb_shared_info::ip6_frag_id.
> 
> Do we really gain a lot of performance by enabling UFO on those devices
> or would it make sense to just drop support? It only helps fragmenting
> large UDP packets, so I don't think it is worth it.

It's not been important enough for anyone to bother implementing it in
hardware/firmware aside from Neterion.

I'll shortly post patches to disable it.

Ben.

> Otherwise I agree with Ben, we need to pass a fragmentation id from the
> host over to the system segmenting the gso frame. Fragmentation ids must
> be generated by the end system.
> 
> Hmm...

-- 
Ben Hutchings
Theory and practice are closer in theory than in practice.
- John Levine, moderator of comp.compilers


signature.asc
Description: This is a digitally signed message part
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH net 0/2] drivers/net,ipv6: Fix IPv6 fragment ID selection for virtio

2014-10-26 Thread Ben Hutchings
The virtio net protocol supports UFO but does not provide for passing a
fragment ID for fragmentation of IPv6 packets.  We used to generate a
fragment ID wherever such a packet was fragmented, but currently we
always use ID=0!

Ben.

Ben Hutchings (2):
  drivers/net: Disable UFO through virtio
  drivers/net,ipv6: Select IPv6 fragment idents for virtio UFO packets

 drivers/net/macvtap.c| 16 
 drivers/net/tun.c| 24 +++-
 drivers/net/virtio_net.c | 23 +--
 include/net/ipv6.h   |  2 ++
 net/ipv6/output_core.c   | 34 ++
 5 files changed, 72 insertions(+), 27 deletions(-)


-- 
Ben Hutchings
Theory and practice are closer in theory than in practice.
- John Levine, moderator of comp.compilers


signature.asc
Description: This is a digitally signed message part
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH net 1/2] drivers/net: Disable UFO through virtio

2014-10-26 Thread Ben Hutchings
IPv6 does not allow fragmentation by routers, so there is no
fragmentation ID in the fixed header.  UFO for IPv6 requires the ID to
be passed separately, but there is no provision for this in the virtio
net protocol.

Until recently our software implementation of UFO/IPv6 generated a new
ID, but this was a bug.  Now we will use ID=0 for any UFO/IPv6 packet
passed through a tap, which is even worse.

Unfortunately there is no distinction between UFO/IPv4 and v6
features, so disable UFO on taps and virtio_net completely until we
have a proper solution.

We cannot depend on VM managers respecting the tap feature flags, so
keep accepting UFO packets but log a warning the first time we do
this.

Signed-off-by: Ben Hutchings 
Fixes: 916e4cf46d02 ("ipv6: reuse ip6_frag_id from ip6_ufo_append_data")
---
I hoped that turning off the UFO feature completely would work, but when
I used libvirt and QEMU on Debian stable I found they still advertised
UFO to the guest.

Ben.

 drivers/net/macvtap.c| 13 +
 drivers/net/tun.c| 18 ++
 drivers/net/virtio_net.c | 23 +--
 3 files changed, 28 insertions(+), 26 deletions(-)

diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index 65e2892..2aeaa61 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -65,7 +65,7 @@ static struct cdev macvtap_cdev;
 static const struct proto_ops macvtap_socket_ops;
 
 #define TUN_OFFLOADS (NETIF_F_HW_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
- NETIF_F_TSO6 | NETIF_F_UFO)
+ NETIF_F_TSO6)
 #define RX_OFFLOADS (NETIF_F_GRO | NETIF_F_LRO)
 #define TAP_FEATURES (NETIF_F_GSO | NETIF_F_SG)
 
@@ -569,6 +569,8 @@ static int macvtap_skb_from_vnet_hdr(struct sk_buff *skb,
gso_type = SKB_GSO_TCPV6;
break;
case VIRTIO_NET_HDR_GSO_UDP:
+   pr_warn_once("macvtap: %s: using disabled UFO feature; 
please fix this program\n",
+current->comm);
gso_type = SKB_GSO_UDP;
break;
default:
@@ -614,8 +616,6 @@ static void macvtap_skb_to_vnet_hdr(const struct sk_buff 
*skb,
vnet_hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
else if (sinfo->gso_type & SKB_GSO_TCPV6)
vnet_hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
-   else if (sinfo->gso_type & SKB_GSO_UDP)
-   vnet_hdr->gso_type = VIRTIO_NET_HDR_GSO_UDP;
else
BUG();
if (sinfo->gso_type & SKB_GSO_TCP_ECN)
@@ -950,9 +950,6 @@ static int set_offload(struct macvtap_queue *q, unsigned 
long arg)
if (arg & TUN_F_TSO6)
feature_mask |= NETIF_F_TSO6;
}
-
-   if (arg & TUN_F_UFO)
-   feature_mask |= NETIF_F_UFO;
}
 
/* tun/tap driver inverts the usage for TSO offloads, where
@@ -963,7 +960,7 @@ static int set_offload(struct macvtap_queue *q, unsigned 
long arg)
 * When user space turns off TSO, we turn off GSO/LRO so that
 * user-space will not receive TSO frames.
 */
-   if (feature_mask & (NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_UFO))
+   if (feature_mask & (NETIF_F_TSO | NETIF_F_TSO6))
features |= RX_OFFLOADS;
else
features &= ~RX_OFFLOADS;
@@ -1064,7 +1061,7 @@ static long macvtap_ioctl(struct file *file, unsigned int 
cmd,
case TUNSETOFFLOAD:
/* let the user check for future flags */
if (arg & ~(TUN_F_CSUM | TUN_F_TSO4 | TUN_F_TSO6 |
-   TUN_F_TSO_ECN | TUN_F_UFO))
+   TUN_F_TSO_ECN))
return -EINVAL;
 
rtnl_lock();
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 186ce54..e8b949a 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -174,7 +174,7 @@ struct tun_struct {
struct net_device   *dev;
netdev_features_t   set_features;
 #define TUN_USER_FEATURES (NETIF_F_HW_CSUM|NETIF_F_TSO_ECN|NETIF_F_TSO| \
- NETIF_F_TSO6|NETIF_F_UFO)
+ NETIF_F_TSO6)
 
int vnet_hdr_sz;
int sndbuf;
@@ -1149,8 +1149,17 @@ static ssize_t tun_get_user(struct tun_struct *tun, 
struct tun_file *tfile,
skb_shinfo(skb)->gso_type = SKB_GSO_TCPV6;
break;
case VIRTIO_NET_HDR_GSO_UDP:
+   {
+   static bool warned;
+   if (!warned) {
+   warned = true;
+   netdev_warn(tun->dev,
+   "%s: using disabled UFO feature; 
please fix this program\n",
+ 

[PATCH net 2/2] drivers/net,ipv6: Select IPv6 fragment idents for virtio UFO packets

2014-10-26 Thread Ben Hutchings
UFO is now disabled on all drivers that work with virtio net headers,
but userland may try to send UFO/IPv6 packets anyway.  Instead of
sending with ID=0, we should select identifiers on their behalf (as we
used to).

Signed-off-by: Ben Hutchings 
Fixes: 916e4cf46d02 ("ipv6: reuse ip6_frag_id from ip6_ufo_append_data")
---
 drivers/net/macvtap.c  |  3 +++
 drivers/net/tun.c  |  6 +-
 include/net/ipv6.h |  2 ++
 net/ipv6/output_core.c | 34 ++
 4 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index 2aeaa61..6f226de 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -16,6 +16,7 @@
 #include 
 #include 
 
+#include 
 #include 
 #include 
 #include 
@@ -572,6 +573,8 @@ static int macvtap_skb_from_vnet_hdr(struct sk_buff *skb,
pr_warn_once("macvtap: %s: using disabled UFO feature; 
please fix this program\n",
 current->comm);
gso_type = SKB_GSO_UDP;
+   if (skb->protocol == htons(ETH_P_IPV6))
+   ipv6_proxy_select_ident(skb);
break;
default:
return -EINVAL;
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index e8b949a..f8356b0 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -65,6 +65,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -1139,6 +1140,8 @@ static ssize_t tun_get_user(struct tun_struct *tun, 
struct tun_file *tfile,
break;
}
 
+   skb_reset_network_header(skb);
+
if (gso.gso_type != VIRTIO_NET_HDR_GSO_NONE) {
pr_debug("GSO!\n");
switch (gso.gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
@@ -1158,6 +1161,8 @@ static ssize_t tun_get_user(struct tun_struct *tun, 
struct tun_file *tfile,
current->comm);
}
skb_shinfo(skb)->gso_type = SKB_GSO_UDP;
+   if (skb->protocol == htons(ETH_P_IPV6))
+   ipv6_proxy_select_ident(skb);
break;
}
default:
@@ -1188,7 +1193,6 @@ static ssize_t tun_get_user(struct tun_struct *tun, 
struct tun_file *tfile,
skb_shinfo(skb)->tx_flags |= SKBTX_SHARED_FRAG;
}
 
-   skb_reset_network_header(skb);
skb_probe_transport_header(skb, 0);
 
rxhash = skb_get_hash(skb);
diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index 97f4720..4292929 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -671,6 +671,8 @@ static inline int ipv6_addr_diff(const struct in6_addr *a1, 
const struct in6_add
return __ipv6_addr_diff(a1, a2, sizeof(struct in6_addr));
 }
 
+void ipv6_proxy_select_ident(struct sk_buff *skb);
+
 int ip6_dst_hoplimit(struct dst_entry *dst);
 
 static inline int ip6_sk_dst_hoplimit(struct ipv6_pinfo *np, struct flowi6 
*fl6,
diff --git a/net/ipv6/output_core.c b/net/ipv6/output_core.c
index fc24c39..97f41a3 100644
--- a/net/ipv6/output_core.c
+++ b/net/ipv6/output_core.c
@@ -3,11 +3,45 @@
  * not configured or static.  These functions are needed by GSO/GRO 
implementation.
  */
 #include 
+#include 
 #include 
 #include 
 #include 
 #include 
 
+/* This function exists only for tap drivers that must support broken
+ * clients requesting UFO without specifying an IPv6 fragment ID.
+ *
+ * This is similar to ipv6_select_ident() but we use an independent hash
+ * seed to limit information leakage.
+ *
+ * The network header must be set before calling this.
+ */
+void ipv6_proxy_select_ident(struct sk_buff *skb)
+{
+   static u32 ip6_proxy_idents_hashrnd __read_mostly;
+   struct in6_addr buf[2];
+   struct in6_addr *addrs;
+   u32 hash, id;
+
+   addrs = skb_header_pointer(skb,
+  skb_network_offset(skb) +
+  offsetof(struct ipv6hdr, saddr),
+  sizeof(buf), buf);
+   if (!addrs)
+   return;
+
+   net_get_random_once(&ip6_proxy_idents_hashrnd,
+   sizeof(ip6_proxy_idents_hashrnd));
+
+   hash = __ipv6_addr_jhash(&addrs[1], ip6_proxy_idents_hashrnd);
+   hash = __ipv6_addr_jhash(&addrs[0], hash);
+
+   id = ip_idents_reserve(hash, 1);
+   skb_shinfo(skb)->ip6_frag_id = htonl(id);
+}
+EXPORT_SYMBOL_GPL(ipv6_proxy_select_ident);
+
 int ip6_find_1stfragopt(struct sk_buff *skb, u8 **nexthdr)
 {
u16 offset = sizeof(struct ipv6hdr);

-- 
Ben Hutchings
Theory and practice are closer in theory than in practice.
- John Levine, moderator of comp.compilers


signature.asc
Description: This is a digitally signed message part
___
Virtualization mailing list
Virtualization@lis

Re: [PATCH net 1/2] drivers/net: Disable UFO through virtio

2014-10-26 Thread Varka Bhadram

On 10/27/2014 09:52 AM, Ben Hutchings wrote:

IPv6 does not allow fragmentation by routers, so there is no
fragmentation ID in the fixed header.  UFO for IPv6 requires the ID to
be passed separately, but there is no provision for this in the virtio
net protocol.

Until recently our software implementation of UFO/IPv6 generated a new
ID, but this was a bug.  Now we will use ID=0 for any UFO/IPv6 packet
passed through a tap, which is even worse.

Unfortunately there is no distinction between UFO/IPv4 and v6
features, so disable UFO on taps and virtio_net completely until we
have a proper solution.

We cannot depend on VM managers respecting the tap feature flags, so
keep accepting UFO packets but log a warning the first time we do
this.


(...)


diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 186ce54..e8b949a 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -174,7 +174,7 @@ struct tun_struct {
struct net_device   *dev;
netdev_features_t   set_features;
  #define TUN_USER_FEATURES (NETIF_F_HW_CSUM|NETIF_F_TSO_ECN|NETIF_F_TSO| \
- NETIF_F_TSO6|NETIF_F_UFO)
+ NETIF_F_TSO6)
  
  	int			vnet_hdr_sz;

int sndbuf;
@@ -1149,8 +1149,17 @@ static ssize_t tun_get_user(struct tun_struct *tun, 
struct tun_file *tfile,
skb_shinfo(skb)->gso_type = SKB_GSO_TCPV6;
break;
case VIRTIO_NET_HDR_GSO_UDP:
+   {
+   static bool warned;


one line space after declaration..


+   if (!warned) {
+   warned = true;
+   netdev_warn(tun->dev,
+   "%s: using disabled UFO feature; please 
fix this program\n",
+   current->comm);
+   }
skb_shinfo(skb)->gso_type = SKB_GSO_UDP;
break;
+   }
default:
tun->dev->stats.rx_frame_errors++;
kfree_skb(skb);
@@ -1251,8 +1260,6 @@ static ssize_t tun_put_user(struct tun_struct *tun,
gso.gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
else if (sinfo->gso_type & SKB_GSO_TCPV6)
gso.gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
-   else if (sinfo->gso_type & SKB_GSO_UDP)
-   gso.gso_type = VIRTIO_NET_HDR_GSO_UDP;
else {
pr_err("unexpected GSO type: "
   "0x%x, gso_size %d, hdr_len %d\n",
@@ -1762,11 +1769,6 @@ static int set_offload(struct tun_struct *tun, unsigned 
long arg)
features |= NETIF_F_TSO6;
arg &= ~(TUN_F_TSO4|TUN_F_TSO6);
}
-
-   if (arg & TUN_F_UFO) {
-   features |= NETIF_F_UFO;
-   arg &= ~TUN_F_UFO;
-   }
}
  
  	/* This gives the user a way to test for new features in future by

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index d75256bd..e9e269d 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -491,8 +491,16 @@ static void receive_buf(struct receive_queue *rq, void 
*buf, unsigned int len)
skb_shinfo(skb)->gso_type = SKB_GSO_TCPV4;
break;
case VIRTIO_NET_HDR_GSO_UDP:
+   {
+   static bool warned;


dto...


+   if (!warned) {
+   warned = true;
+   netdev_warn(dev,
+   "host using disabled UFO feature; please 
fix it\n");
+   }
skb_shinfo(skb)->gso_type = SKB_GSO_UDP;
break;
+   }
  


(...)





--
Thanks and Regards,
Varka Bhadram.

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization