virtio_net: Fix skb->csum_start computation
Subsystems (4)
PATCH 1/4 virtio_net: Fix skb->csum_start computation
From: Mark McLoughlin <[email protected]>
hdr->csum_start is the offset from the start of the ethernet header to the transport layer checksum field. skb->csum_start is the offset from skb->head.
skb_partial_csum_set() assumes that skb->data points to the ethernet header - i.e. it computes skb->csum_start by adding the headroom to hdr->csum_start.
Since eth_type_trans() skb_pull()s the ethernet header, skb_partial_csum_set() should be called before eth_type_trans().
(Without this patch, GSO packets from a guest to the world outside the host are corrupted).
--- drivers/net/virtio_net.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
drivers/net/virtio_net.c +5 -3
@@ -86,9 +86,7 @@ static void receive_skb(struct net_device *dev, struct sk_buff *skb,BUG_ON (len >MAX_PACKET_LEN );skb_trim (skb, len); - skb->protocol =eth_type_trans (skb, dev); -pr_debug ("Receiving skb proto 0x%04x len %i type %i\n" , -ntohs (skb->protocol), skb->len, skb->pkt_type); + dev->stats.rx_bytes += skb->len; dev->stats.rx_packets++;
@@ -98,6 +96,10 @@ static void receive_skb(struct net_device *dev, struct sk_buff *skb, goto frame_err; } + skb->protocol =eth_type_trans (skb, dev); +pr_debug ("Receiving skb proto 0x%04x len %i type %i\n" , +ntohs (skb->protocol), skb->len, skb->pkt_type); +if (hdr->gso_type !=VIRTIO_NET_HDR_GSO_NONE ) {pr_debug ("GSO!\n" );switch (hdr->gso_type & ~VIRTIO_NET_HDR_GSO_ECN ) {
PATCH 2/4 virtio: Fix typo in virtio_net_hdr comments
From: Mark McLoughlin <[email protected]>
--- include/linux/virtio_net.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
include/linux/virtio_net.h +1 -1
@@ -38,7 +38,7 @@ struct virtio_net_hdr#define VIRTIO_NET_HDR_GSO_ECN 0x80 // TCP has ECN set__u8 gso_type;__u16 hdr_len;/* Ethernet + IP + tcp/udp hdrs */ -__u16 gso_size;/* Bytes to append to gso_hdr_len per frame */ +__u16 gso_size;/* Bytes to append to hdr_len per frame */ __u16 csum_start;/* Position to start checksumming from */ __u16 csum_offset;/* Offset after that to place checksum */ };
PATCH 2/4 virtio: Fix typo in virtio_net_hdr comments
In reply to Rusty RussellFrom: Mark McLoughlin <[email protected]>
--- include/linux/virtio_net.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
include/linux/virtio_net.h +1 -1
@@ -38,7 +38,7 @@ struct virtio_net_hdr#define VIRTIO_NET_HDR_GSO_ECN 0x80 // TCP has ECN set__u8 gso_type;__u16 hdr_len;/* Ethernet + IP + tcp/udp hdrs */ -__u16 gso_size;/* Bytes to append to gso_hdr_len per frame */ +__u16 gso_size;/* Bytes to append to hdr_len per frame */ __u16 csum_start;/* Position to start checksumming from */ __u16 csum_offset;/* Offset after that to place checksum */ };
PATCH 3/4 virtio: virtio_net free transmit skbs in a timer
From: Mark McLoughlin <[email protected]>
virtio_net currently only frees old transmit skbs just before queueing new ones. If the queue is full, it then enables interrupts and waits for notification that more work has been performed.
However, a side-effect of this scheme is that there are always xmit skbs left dangling when no new packets are sent, against the Documentation/networking/driver.txt guideline:
"... it is not allowed for your TX mitigation scheme
to let TX packets "hang out" in the TX ring unreclaimed forever if no new TX packets are sent."
Add a timer to ensure that any time we queue new TX skbs, we will shortly free them again.
This fixes an easily reproduced hang at shutdown where iptables attempts to unload nf_conntrack and nf_conntrack waits for an skb it is tracking to be freed, but virtio_net never frees it.
--- drivers/net/virtio_net.c | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-)
drivers/net/virtio_net.c +26 -2
@@ -43,6 +43,8 @@ struct virtnet_info/* The skb we couldn't send because buffers were full. */ struct sk_buff *last_xmit_skb; + +struct timer_list xmit_free_timer;/* Number of input buffers, and max we've ever had. */ unsigned int num, max;
@@ -238,9 +240,23 @@ static void free_old_xmit_skbs(struct vi } } +static void xmit_free (unsigned long data) +{ +struct virtnet_info *vi = (void *)data; + +netif_tx_lock (vi->dev); + +free_old_xmit_skbs (vi); + +if (!skb_queue_empty (&vi->send)) +mod_timer (&vi->xmit_free_timer, jiffies + (HZ /10 )); + +netif_tx_unlock (vi->dev); +} +static int xmit_skb (struct virtnet_info *vi,struct sk_buff *skb) { -int num; +int num, err;struct scatterlist sg[2 +MAX_SKB_FRAGS ];struct virtio_net_hdr *hdr;const unsigned char *dest = ((struct ethhdr *)skb->data)->h_dest;
@@ -283,7 +299,11 @@ static int xmit_skb(struct virtnet_infovnet_hdr_to_sg (sg, skb); num =skb_to_sgvec (skb, sg+1 ,0 , skb->len) +1 ; -return vi->svq->vq_ops->add_buf (vi->svq, sg, num,0 , skb); + err = vi->svq->vq_ops->add_buf (vi->svq, sg, num,0 , skb); +if (!err) +mod_timer (&vi->xmit_free_timer, jiffies + (HZ /10 )); + +return err; }static void xmit_tasklet (unsigned long data)
@@ -452,6 +472,8 @@ static int virtnet_probe(struct virtio_dtasklet_init (&vi->tasklet, xmit_tasklet, (unsigned long )vi); +setup_timer (&vi->xmit_free_timer, xmit_free, (unsigned long )vi); + err =register_netdev (dev);if (err) {pr_debug ("virtio_net: registering device failed\n" );
@@ -488,6 +510,8 @@ static void virtnet_remove(struct virtio/* Stop all the virtqueues. */ vdev->config->reset (vdev); + +del_timer_sync (&vi->xmit_free_timer);/* Free our skbs in send and recv queues, if any. */ while ((skb =__skb_dequeue (&vi->recv)) !=NULL ) {
PATCH 3/4 virtio: virtio_net free transmit skbs in a timer
In reply to Rusty RussellFrom: Mark McLoughlin <[email protected]>
virtio_net currently only frees old transmit skbs just before queueing new ones. If the queue is full, it then enables interrupts and waits for notification that more work has been performed.
However, a side-effect of this scheme is that there are always xmit skbs left dangling when no new packets are sent, against the Documentation/networking/driver.txt guideline:
"... it is not allowed for your TX mitigation scheme
to let TX packets "hang out" in the TX ring unreclaimed forever if no new TX packets are sent."
Add a timer to ensure that any time we queue new TX skbs, we will shortly free them again.
This fixes an easily reproduced hang at shutdown where iptables attempts to unload nf_conntrack and nf_conntrack waits for an skb it is tracking to be freed, but virtio_net never frees it.
--- drivers/net/virtio_net.c | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-)
drivers/net/virtio_net.c +26 -2
@@ -43,6 +43,8 @@ struct virtnet_info/* The skb we couldn't send because buffers were full. */ struct sk_buff *last_xmit_skb; + +struct timer_list xmit_free_timer;/* Number of input buffers, and max we've ever had. */ unsigned int num, max;
@@ -238,9 +240,23 @@ static void free_old_xmit_skbs(struct vi } } +static void xmit_free (unsigned long data) +{ +struct virtnet_info *vi = (void *)data; + +netif_tx_lock (vi->dev); + +free_old_xmit_skbs (vi); + +if (!skb_queue_empty (&vi->send)) +mod_timer (&vi->xmit_free_timer, jiffies + (HZ /10 )); + +netif_tx_unlock (vi->dev); +} +static int xmit_skb (struct virtnet_info *vi,struct sk_buff *skb) { -int num; +int num, err;struct scatterlist sg[2 +MAX_SKB_FRAGS ];struct virtio_net_hdr *hdr;const unsigned char *dest = ((struct ethhdr *)skb->data)->h_dest;
@@ -283,7 +299,11 @@ static int xmit_skb(struct virtnet_infovnet_hdr_to_sg (sg, skb); num =skb_to_sgvec (skb, sg+1 ,0 , skb->len) +1 ; -return vi->svq->vq_ops->add_buf (vi->svq, sg, num,0 , skb); + err = vi->svq->vq_ops->add_buf (vi->svq, sg, num,0 , skb); +if (!err) +mod_timer (&vi->xmit_free_timer, jiffies + (HZ /10 )); + +return err; }static void xmit_tasklet (unsigned long data)
@@ -452,6 +472,8 @@ static int virtnet_probe(struct virtio_dtasklet_init (&vi->tasklet, xmit_tasklet, (unsigned long )vi); +setup_timer (&vi->xmit_free_timer, xmit_free, (unsigned long )vi); + err =register_netdev (dev);if (err) {pr_debug ("virtio_net: registering device failed\n" );
@@ -488,6 +510,8 @@ static void virtnet_remove(struct virtio/* Stop all the virtqueues. */ vdev->config->reset (vdev); + +del_timer_sync (&vi->xmit_free_timer);/* Free our skbs in send and recv queues, if any. */ while ((skb =__skb_dequeue (&vi->recv)) !=NULL ) {
PATCH 4/4 virtio: use callback on empty in virtio_net
virtio_net uses a timer to free old transmitted packets, rather than leaving callbacks enabled all the time. If the host promises to always notify us when the transmit ring is empty, we can free packets at that point and avoid the timer.
--- drivers/net/virtio_net.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-)
drivers/net/virtio_net.c +17 -5
@@ -44,6 +44,7 @@ struct virtnet_info/* The skb we couldn't send because buffers were full. */ struct sk_buff *last_xmit_skb; +/* If we need to free in a timer, this is it. */ struct timer_list xmit_free_timer;/* Number of input buffers, and max we've ever had. */
@@ -51,6 +52,7 @@ struct virtnet_info/* For cleaning up after transmission. */ struct tasklet_struct tasklet; +bool free_in_tasklet;/* Receive & send queues. */ struct sk_buff_head recv;
@@ -74,7 +76,7 @@ static void skb_xmit_done(struct virtque/* Suppress further interrupts. */ svq->vq_ops->disable_cb (svq); -/* We were waiting for more output buffers. */ +/* We were probably waiting for more output buffers. */ netif_wake_queue (vi->dev);/* Make sure we re-xmit last_xmit_skb: if there are no more packets
@@ -240,6 +242,8 @@ static void free_old_xmit_skbs(struct vi } } +/* If the virtio transport doesn't always notify us when all in-flight packets +* are consumed, we fall back to using this function on a timer to free them. */ static void xmit_free (unsigned long data) {struct virtnet_info *vi = (void *)data;
@@ -300,7 +304,7 @@ static int xmit_skb(struct virtnet_info num =skb_to_sgvec (skb, sg+1 ,0 , skb->len) +1 ; err = vi->svq->vq_ops->add_buf (vi->svq, sg, num,0 , skb); -if (!err) +if (!err && !vi->free_in_tasklet)mod_timer (&vi->xmit_free_timer, jiffies + (HZ /10 ));return err;
@@ -315,6 +319,8 @@ static void xmit_tasklet(unsigned long d vi->svq->vq_ops->kick (vi->svq); vi->last_xmit_skb =NULL ; } +if (vi->free_in_tasklet) +free_old_xmit_skbs (vi);netif_tx_unlock_bh (vi->dev); }
@@ -455,6 +461,10 @@ static int virtnet_probe(struct virtio_d vi->vdev = vdev; vdev->priv = vi; +/* If they give us a callback when all buffers are done, we don't need +* the timer. */ + vi->free_in_tasklet =virtio_has_feature (vdev,VIRTIO_F_NOTIFY_ON_EMPTY ); +/* We expect two virtqueues, receive then send. */ vi->rvq = vdev->config->find_vq (vdev,0 , skb_recv_done);if (IS_ERR (vi->rvq)) {
@@ -474,7 +487,8 @@ static int virtnet_probe(struct virtio_dtasklet_init (&vi->tasklet, xmit_tasklet, (unsigned long )vi); -setup_timer (&vi->xmit_free_timer, xmit_free, (unsigned long )vi); +if (!vi->free_in_tasklet) +setup_timer (&vi->xmit_free_timer, xmit_free, (unsigned long )vi); err =register_netdev (dev);if (err) {
@@ -513,7 +527,8 @@ static void virtnet_remove(struct virtio/* Stop all the virtqueues. */ vdev->config->reset (vdev); -del_timer_sync (&vi->xmit_free_timer); +if (!vi->free_in_tasklet) +del_timer_sync (&vi->xmit_free_timer);/* Free our skbs in send and recv queues, if any. */ while ((skb =__skb_dequeue (&vi->recv)) !=NULL ) {
@@ -538,7 +553,7 @@ static unsigned int features[] = {static unsigned int features[] = {VIRTIO_NET_F_CSUM ,VIRTIO_NET_F_GSO ,VIRTIO_NET_F_MAC ,VIRTIO_NET_F_HOST_TSO4 ,VIRTIO_NET_F_HOST_UFO ,VIRTIO_NET_F_HOST_TSO6 , -VIRTIO_NET_F_HOST_ECN , +VIRTIO_NET_F_HOST_ECN ,VIRTIO_F_NOTIFY_ON_EMPTY , };static struct virtio_driver virtio_net = {
PATCH 4/4 virtio: use callback on empty in virtio_net
In reply to Rusty Russellvirtio_net uses a timer to free old transmitted packets, rather than leaving callbacks enabled all the time. If the host promises to always notify us when the transmit ring is empty, we can free packets at that point and avoid the timer.
--- drivers/net/virtio_net.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-)
drivers/net/virtio_net.c +17 -5
@@ -44,6 +44,7 @@ struct virtnet_info/* The skb we couldn't send because buffers were full. */ struct sk_buff *last_xmit_skb; +/* If we need to free in a timer, this is it. */ struct timer_list xmit_free_timer;/* Number of input buffers, and max we've ever had. */
@@ -51,6 +52,7 @@ struct virtnet_info/* For cleaning up after transmission. */ struct tasklet_struct tasklet; +bool free_in_tasklet;/* Receive & send queues. */ struct sk_buff_head recv;
@@ -74,7 +76,7 @@ static void skb_xmit_done(struct virtque/* Suppress further interrupts. */ svq->vq_ops->disable_cb (svq); -/* We were waiting for more output buffers. */ +/* We were probably waiting for more output buffers. */ netif_wake_queue (vi->dev);/* Make sure we re-xmit last_xmit_skb: if there are no more packets
@@ -240,6 +242,8 @@ static void free_old_xmit_skbs(struct vi } } +/* If the virtio transport doesn't always notify us when all in-flight packets +* are consumed, we fall back to using this function on a timer to free them. */ static void xmit_free (unsigned long data) {struct virtnet_info *vi = (void *)data;
@@ -300,7 +304,7 @@ static int xmit_skb(struct virtnet_info num =skb_to_sgvec (skb, sg+1 ,0 , skb->len) +1 ; err = vi->svq->vq_ops->add_buf (vi->svq, sg, num,0 , skb); -if (!err) +if (!err && !vi->free_in_tasklet)mod_timer (&vi->xmit_free_timer, jiffies + (HZ /10 ));return err;
@@ -315,6 +319,8 @@ static void xmit_tasklet(unsigned long d vi->svq->vq_ops->kick (vi->svq); vi->last_xmit_skb =NULL ; } +if (vi->free_in_tasklet) +free_old_xmit_skbs (vi);netif_tx_unlock_bh (vi->dev); }
@@ -455,6 +461,10 @@ static int virtnet_probe(struct virtio_d vi->vdev = vdev; vdev->priv = vi; +/* If they give us a callback when all buffers are done, we don't need +* the timer. */ + vi->free_in_tasklet =virtio_has_feature (vdev,VIRTIO_F_NOTIFY_ON_EMPTY ); +/* We expect two virtqueues, receive then send. */ vi->rvq = vdev->config->find_vq (vdev,0 , skb_recv_done);if (IS_ERR (vi->rvq)) {
@@ -474,7 +487,8 @@ static int virtnet_probe(struct virtio_dtasklet_init (&vi->tasklet, xmit_tasklet, (unsigned long )vi); -setup_timer (&vi->xmit_free_timer, xmit_free, (unsigned long )vi); +if (!vi->free_in_tasklet) +setup_timer (&vi->xmit_free_timer, xmit_free, (unsigned long )vi); err =register_netdev (dev);if (err) {
@@ -513,7 +527,8 @@ static void virtnet_remove(struct virtio/* Stop all the virtqueues. */ vdev->config->reset (vdev); -del_timer_sync (&vi->xmit_free_timer); +if (!vi->free_in_tasklet) +del_timer_sync (&vi->xmit_free_timer);/* Free our skbs in send and recv queues, if any. */ while ((skb =__skb_dequeue (&vi->recv)) !=NULL ) {
@@ -538,7 +553,7 @@ static unsigned int features[] = {static unsigned int features[] = {VIRTIO_NET_F_CSUM ,VIRTIO_NET_F_GSO ,VIRTIO_NET_F_MAC ,VIRTIO_NET_F_HOST_TSO4 ,VIRTIO_NET_F_HOST_UFO ,VIRTIO_NET_F_HOST_TSO6 , -VIRTIO_NET_F_HOST_ECN , +VIRTIO_NET_F_HOST_ECN ,VIRTIO_F_NOTIFY_ON_EMPTY , };static struct virtio_driver virtio_net = {
PATCH 4/4 virtio: use callback on empty in virtio_net
In reply to Rusty Russellvirtio_net uses a timer to free old transmitted packets, rather than leaving callbacks enabled all the time. If the host promises to always notify us when the transmit ring is empty, we can free packets at that point and avoid the timer.
--- drivers/net/virtio_net.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-)
drivers/net/virtio_net.c +17 -5
@@ -44,6 +44,7 @@ struct virtnet_info/* The skb we couldn't send because buffers were full. */ struct sk_buff *last_xmit_skb; +/* If we need to free in a timer, this is it. */ struct timer_list xmit_free_timer;/* Number of input buffers, and max we've ever had. */
@@ -51,6 +52,7 @@ struct virtnet_info/* For cleaning up after transmission. */ struct tasklet_struct tasklet; +bool free_in_tasklet;/* Receive & send queues. */ struct sk_buff_head recv;
@@ -74,7 +76,7 @@ static void skb_xmit_done(struct virtque/* Suppress further interrupts. */ svq->vq_ops->disable_cb (svq); -/* We were waiting for more output buffers. */ +/* We were probably waiting for more output buffers. */ netif_wake_queue (vi->dev);/* Make sure we re-xmit last_xmit_skb: if there are no more packets
@@ -240,6 +242,8 @@ static void free_old_xmit_skbs(struct vi } } +/* If the virtio transport doesn't always notify us when all in-flight packets +* are consumed, we fall back to using this function on a timer to free them. */ static void xmit_free (unsigned long data) {struct virtnet_info *vi = (void *)data;
@@ -300,7 +304,7 @@ static int xmit_skb(struct virtnet_info num =skb_to_sgvec (skb, sg+1 ,0 , skb->len) +1 ; err = vi->svq->vq_ops->add_buf (vi->svq, sg, num,0 , skb); -if (!err) +if (!err && !vi->free_in_tasklet)mod_timer (&vi->xmit_free_timer, jiffies + (HZ /10 ));return err;
@@ -315,6 +319,8 @@ static void xmit_tasklet(unsigned long d vi->svq->vq_ops->kick (vi->svq); vi->last_xmit_skb =NULL ; } +if (vi->free_in_tasklet) +free_old_xmit_skbs (vi);netif_tx_unlock_bh (vi->dev); }
@@ -455,6 +461,10 @@ static int virtnet_probe(struct virtio_d vi->vdev = vdev; vdev->priv = vi; +/* If they give us a callback when all buffers are done, we don't need +* the timer. */ + vi->free_in_tasklet =virtio_has_feature (vdev,VIRTIO_F_NOTIFY_ON_EMPTY ); +/* We expect two virtqueues, receive then send. */ vi->rvq = vdev->config->find_vq (vdev,0 , skb_recv_done);if (IS_ERR (vi->rvq)) {
@@ -474,7 +487,8 @@ static int virtnet_probe(struct virtio_dtasklet_init (&vi->tasklet, xmit_tasklet, (unsigned long )vi); -setup_timer (&vi->xmit_free_timer, xmit_free, (unsigned long )vi); +if (!vi->free_in_tasklet) +setup_timer (&vi->xmit_free_timer, xmit_free, (unsigned long )vi); err =register_netdev (dev);if (err) {
@@ -513,7 +527,8 @@ static void virtnet_remove(struct virtio/* Stop all the virtqueues. */ vdev->config->reset (vdev); -del_timer_sync (&vi->xmit_free_timer); +if (!vi->free_in_tasklet) +del_timer_sync (&vi->xmit_free_timer);/* Free our skbs in send and recv queues, if any. */ while ((skb =__skb_dequeue (&vi->recv)) !=NULL ) {
@@ -538,7 +553,7 @@ static unsigned int features[] = {static unsigned int features[] = {VIRTIO_NET_F_CSUM ,VIRTIO_NET_F_GSO ,VIRTIO_NET_F_MAC ,VIRTIO_NET_F_HOST_TSO4 ,VIRTIO_NET_F_HOST_UFO ,VIRTIO_NET_F_HOST_TSO6 , -VIRTIO_NET_F_HOST_ECN , +VIRTIO_NET_F_HOST_ECN ,VIRTIO_F_NOTIFY_ON_EMPTY , };static struct virtio_driver virtio_net = {
Quoted 17 lines · Rusty Russell
From: Mark McLoughlin <[email protected]>
hdr->csum_startis the offset from the start of the ethernet header to the transport layer checksum field.skb->csum_startis the offset fromskb->head.
skb_partial_csum_set()assumes thatskb->datapoints to the ethernet header - i.e. it computesskb->csum_startby adding the headroom tohdr->csum_start.Since
eth_type_trans()skb_pull()s the ethernet header,skb_partial_csum_set()should be called beforeeth_type_trans().(Without this patch, GSO packets from a guest to the world outside the host are corrupted).
Quoted 6 lines
Signed-off-by Mark McLoughlin <[email protected]>Acked-by Herbert Xu <[email protected]>Signed-off-by Rusty Russell <[email protected]>--- drivers/net/virtio_net.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
applied 1-4
Quoted 17 lines · Rusty Russell
From: Mark McLoughlin <[email protected]>
hdr->csum_startis the offset from the start of the ethernet header to the transport layer checksum field.skb->csum_startis the offset fromskb->head.
skb_partial_csum_set()assumes thatskb->datapoints to the ethernet header - i.e. it computesskb->csum_startby adding the headroom tohdr->csum_start.Since
eth_type_trans()skb_pull()s the ethernet header,skb_partial_csum_set()should be called beforeeth_type_trans().(Without this patch, GSO packets from a guest to the world outside the host are corrupted).
Quoted 6 lines
Signed-off-by Mark McLoughlin <[email protected]>Acked-by Herbert Xu <[email protected]>Signed-off-by Rusty Russell <[email protected]>--- drivers/net/virtio_net.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
applied 1-4