Skip to content

Commit

Permalink
net: Fix possible wrong checksum generation.
Browse files Browse the repository at this point in the history 
Patch cef401d (net: fix possible wrong checksum
generation) fixed wrong checksum calculation but it broke TSO by
defining new GSO type but not a netdev feature for that type.
net_gso_ok() would not allow hardware checksum/segmentation
offload of such packets without the feature.

Following patch fixes TSO and wrong checksum. This patch uses
same logic that Eric Dumazet used. Patch introduces new flag
SKBTX_SHARED_FRAG if at least one frag can be modified by
the user. but SKBTX_SHARED_FRAG flag is kept in skb shared
info tx_flags rather than gso_type.

tx_flags is better compared to gso_type since we can have skb with
shared frag without gso packet. It does not link SHARED_FRAG to
GSO, So there is no need to define netdev feature for this.

Signed-off-by: Pravin B Shelar <pshelar@nicira.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
  • Loading branch information
@davem330
Pravin B Shelar authored and davem330 committed Feb 13, 2013
1 parent b8fa410 commit c9af6db
Show file tree
Hide file tree
Showing 11 changed files with 29 additions and 38 deletions.
4 changes: 2 additions & 2 deletions drivers/net/macvtap.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -543,7 +543,6 @@ static int zerocopy_sg_from_iovec(struct sk_buff *skb, const struct iovec *from,
skb->data_len += len;
skb->len += len;
skb->truesize += truesize;
skb_shinfo(skb)->gso_type |= SKB_GSO_SHARED_FRAG;
atomic_add(truesize, &skb->sk->sk_wmem_alloc);
while (len) {
int off = base & ~PAGE_MASK;
Expand Down Expand Up @@ -599,7 +598,7 @@ static int macvtap_skb_from_vnet_hdr(struct sk_buff *skb,

if (vnet_hdr->gso_type != VIRTIO_NET_HDR_GSO_NONE) {
skb_shinfo(skb)->gso_size = vnet_hdr->gso_size;
skb_shinfo(skb)->gso_type |= gso_type;
skb_shinfo(skb)->gso_type = gso_type;

/* Header must be checked, and gso_segs computed. */
skb_shinfo(skb)->gso_type |= SKB_GSO_DODGY;
Expand Down Expand Up @@ -743,6 +742,7 @@ static ssize_t macvtap_get_user(struct macvtap_queue *q, struct msghdr *m,
if (zerocopy) {
skb_shinfo(skb)->destructor_arg = m->msg_control;
skb_shinfo(skb)->tx_flags |= SKBTX_DEV_ZEROCOPY;
skb_shinfo(skb)->tx_flags |= SKBTX_SHARED_FRAG;
}
if (vlan)
macvlan_start_xmit(skb, vlan->dev);
Expand Down
13 changes: 5 additions & 8 deletions drivers/net/tun.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1019,7 +1019,6 @@ static int zerocopy_sg_from_iovec(struct sk_buff *skb, const struct iovec *from,
skb->data_len += len;
skb->len += len;
skb->truesize += truesize;
skb_shinfo(skb)->gso_type |= SKB_GSO_SHARED_FRAG;
atomic_add(truesize, &skb->sk->sk_wmem_alloc);
while (len) {
int off = base & ~PAGE_MASK;
Expand Down Expand Up @@ -1165,18 +1164,16 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
}

if (gso.gso_type != VIRTIO_NET_HDR_GSO_NONE) {
unsigned short gso_type = 0;

pr_debug("GSO!\n");
switch (gso.gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
case VIRTIO_NET_HDR_GSO_TCPV4:
gso_type = SKB_GSO_TCPV4;
skb_shinfo(skb)->gso_type = SKB_GSO_TCPV4;
break;
case VIRTIO_NET_HDR_GSO_TCPV6:
gso_type = SKB_GSO_TCPV6;
skb_shinfo(skb)->gso_type = SKB_GSO_TCPV6;
break;
case VIRTIO_NET_HDR_GSO_UDP:
gso_type = SKB_GSO_UDP;
skb_shinfo(skb)->gso_type = SKB_GSO_UDP;
break;
default:
tun->dev->stats.rx_frame_errors++;
Expand All @@ -1185,10 +1182,9 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
}

if (gso.gso_type & VIRTIO_NET_HDR_GSO_ECN)
gso_type |= SKB_GSO_TCP_ECN;
skb_shinfo(skb)->gso_type |= SKB_GSO_TCP_ECN;

skb_shinfo(skb)->gso_size = gso.gso_size;
skb_shinfo(skb)->gso_type |= gso_type;
if (skb_shinfo(skb)->gso_size == 0) {
tun->dev->stats.rx_frame_errors++;
kfree_skb(skb);
Expand All @@ -1204,6 +1200,7 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
if (zerocopy) {
skb_shinfo(skb)->destructor_arg = msg_control;
skb_shinfo(skb)->tx_flags |= SKBTX_DEV_ZEROCOPY;
skb_shinfo(skb)->tx_flags |= SKBTX_SHARED_FRAG;
}

skb_reset_network_header(skb);
Expand Down
13 changes: 5 additions & 8 deletions drivers/net/virtio_net.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ static void set_skb_frag(struct sk_buff *skb, struct page *page,
skb->len += size;
skb->truesize += PAGE_SIZE;
skb_shinfo(skb)->nr_frags++;
skb_shinfo(skb)->gso_type |= SKB_GSO_SHARED_FRAG;
skb_shinfo(skb)->tx_flags |= SKBTX_SHARED_FRAG;
*len -= size;
}

Expand Down Expand Up @@ -387,18 +387,16 @@ static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len)
ntohs(skb->protocol), skb->len, skb->pkt_type);

if (hdr->hdr.gso_type != VIRTIO_NET_HDR_GSO_NONE) {
unsigned short gso_type = 0;

pr_debug("GSO!\n");
switch (hdr->hdr.gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
case VIRTIO_NET_HDR_GSO_TCPV4:
gso_type = SKB_GSO_TCPV4;
skb_shinfo(skb)->gso_type = SKB_GSO_TCPV4;
break;
case VIRTIO_NET_HDR_GSO_UDP:
gso_type = SKB_GSO_UDP;
skb_shinfo(skb)->gso_type = SKB_GSO_UDP;
break;
case VIRTIO_NET_HDR_GSO_TCPV6:
gso_type = SKB_GSO_TCPV6;
skb_shinfo(skb)->gso_type = SKB_GSO_TCPV6;
break;
default:
net_warn_ratelimited("%s: bad gso type %u.\n",
Expand All @@ -407,15 +405,14 @@ static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len)
}

if (hdr->hdr.gso_type & VIRTIO_NET_HDR_GSO_ECN)
gso_type |= SKB_GSO_TCP_ECN;
skb_shinfo(skb)->gso_type |= SKB_GSO_TCP_ECN;

skb_shinfo(skb)->gso_size = hdr->hdr.gso_size;
if (skb_shinfo(skb)->gso_size == 0) {
net_warn_ratelimited("%s: zero gso size.\n", dev->name);
goto frame_err;
}

skb_shinfo(skb)->gso_type |= gso_type;
/* Header must be checked, and gso_segs computed. */
skb_shinfo(skb)->gso_type |= SKB_GSO_DODGY;
skb_shinfo(skb)->gso_segs = 0;
Expand Down
17 changes: 9 additions & 8 deletions include/linux/skbuff.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,13 @@ enum {

/* generate wifi status information (where possible) */
SKBTX_WIFI_STATUS = 1 << 4,

/* This indicates at least one fragment might be overwritten
* (as in vmsplice(), sendfile() ...)
* If we need to compute a TX checksum, we'll need to copy
* all frags to avoid possible bad checksum
*/
SKBTX_SHARED_FRAG = 1 << 5,
};

/*
Expand Down Expand Up @@ -307,13 +314,6 @@ enum {
SKB_GSO_TCPV6 = 1 << 4,

SKB_GSO_FCOE = 1 << 5,

/* This indicates at least one fragment might be overwritten
* (as in vmsplice(), sendfile() ...)
* If we need to compute a TX checksum, we'll need to copy
* all frags to avoid possible bad checksum
*/
SKB_GSO_SHARED_FRAG = 1 << 6,
};

#if BITS_PER_LONG > 32
Expand Down Expand Up @@ -2220,7 +2220,8 @@ static inline int skb_linearize(struct sk_buff *skb)
*/
static inline bool skb_has_shared_frag(const struct sk_buff *skb)
{
return skb_shinfo(skb)->gso_type & SKB_GSO_SHARED_FRAG;
return skb_is_nonlinear(skb) &&
skb_shinfo(skb)->tx_flags & SKBTX_SHARED_FRAG;
}

/**
Expand Down
5 changes: 2 additions & 3 deletions net/core/skbuff.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2326,8 +2326,7 @@ void skb_split(struct sk_buff *skb, struct sk_buff *skb1, const u32 len)
{
int pos = skb_headlen(skb);

skb_shinfo(skb1)->gso_type = skb_shinfo(skb)->gso_type;

skb_shinfo(skb)->tx_flags = skb_shinfo(skb1)->tx_flags & SKBTX_SHARED_FRAG;
if (len < pos) /* Split line is inside header. */
skb_split_inside_header(skb, skb1, len, pos);
else /* Second chunk has no header, nothing to copy. */
Expand Down Expand Up @@ -2833,7 +2832,7 @@ struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features)
skb_copy_from_linear_data_offset(skb, offset,
skb_put(nskb, hsize), hsize);

skb_shinfo(nskb)->gso_type = skb_shinfo(skb)->gso_type;
skb_shinfo(nskb)->tx_flags = skb_shinfo(skb)->tx_flags & SKBTX_SHARED_FRAG;

while (pos < offset + len && i < nfrags) {
*frag = skb_shinfo(skb)->frags[i];
Expand Down
1 change: 0 additions & 1 deletion net/ipv4/af_inet.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1287,7 +1287,6 @@ static struct sk_buff *inet_gso_segment(struct sk_buff *skb,
SKB_GSO_UDP |
SKB_GSO_DODGY |
SKB_GSO_TCP_ECN |
SKB_GSO_SHARED_FRAG |
0)))
goto out;

Expand Down
1 change: 1 addition & 0 deletions net/ipv4/ip_output.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -598,6 +598,7 @@ int ip_fragment(struct sk_buff *skb, int (*output)(struct sk_buff *))
/* for offloaded checksums cleanup checksum before fragmentation */
if ((skb->ip_summed == CHECKSUM_PARTIAL) && skb_checksum_help(skb))
goto fail;
iph = ip_hdr(skb);

left = skb->len - hlen; /* Space per frame */
ptr = hlen; /* Where to start from */
Expand Down
4 changes: 1 addition & 3 deletions net/ipv4/tcp.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -897,8 +897,7 @@ static ssize_t do_tcp_sendpages(struct sock *sk, struct page *page, int offset,
get_page(page);
skb_fill_page_desc(skb, i, page, offset, copy);
}

skb_shinfo(skb)->gso_type |= SKB_GSO_SHARED_FRAG;
skb_shinfo(skb)->tx_flags |= SKBTX_SHARED_FRAG;

skb->len += copy;
skb->data_len += copy;
Expand Down Expand Up @@ -3044,7 +3043,6 @@ struct sk_buff *tcp_tso_segment(struct sk_buff *skb,
SKB_GSO_DODGY |
SKB_GSO_TCP_ECN |
SKB_GSO_TCPV6 |
SKB_GSO_SHARED_FRAG |
0) ||
!(type & (SKB_GSO_TCPV4 | SKB_GSO_TCPV6))))
goto out;
Expand Down
4 changes: 2 additions & 2 deletions net/ipv4/tcp_input.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1239,13 +1239,13 @@ static bool tcp_shifted_skb(struct sock *sk, struct sk_buff *skb,
*/
if (!skb_shinfo(prev)->gso_size) {
skb_shinfo(prev)->gso_size = mss;
skb_shinfo(prev)->gso_type |= sk->sk_gso_type;
skb_shinfo(prev)->gso_type = sk->sk_gso_type;
}

/* CHECKME: To clear or not to clear? Mimics normal skb currently */
if (skb_shinfo(skb)->gso_segs <= 1) {
skb_shinfo(skb)->gso_size = 0;
skb_shinfo(skb)->gso_type &= SKB_GSO_SHARED_FRAG;
skb_shinfo(skb)->gso_type = 0;
}

/* Difference in this won't matter, both ACKed by the same cumul. ACK */
Expand Down
4 changes: 2 additions & 2 deletions net/ipv4/tcp_output.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1133,18 +1133,18 @@ static void tcp_queue_skb(struct sock *sk, struct sk_buff *skb)
static void tcp_set_skb_tso_segs(const struct sock *sk, struct sk_buff *skb,
unsigned int mss_now)
{
skb_shinfo(skb)->gso_type &= SKB_GSO_SHARED_FRAG;
if (skb->len <= mss_now || !sk_can_gso(sk) ||
skb->ip_summed == CHECKSUM_NONE) {
/* Avoid the costly divide in the normal
* non-TSO case.
*/
skb_shinfo(skb)->gso_segs = 1;
skb_shinfo(skb)->gso_size = 0;
skb_shinfo(skb)->gso_type = 0;
} else {
skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(skb->len, mss_now);
skb_shinfo(skb)->gso_size = mss_now;
skb_shinfo(skb)->gso_type |= sk->sk_gso_type;
skb_shinfo(skb)->gso_type = sk->sk_gso_type;
}
}

Expand Down
1 change: 0 additions & 1 deletion net/ipv6/ip6_offload.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,6 @@ static struct sk_buff *ipv6_gso_segment(struct sk_buff *skb,
SKB_GSO_DODGY |
SKB_GSO_TCP_ECN |
SKB_GSO_TCPV6 |
SKB_GSO_SHARED_FRAG |
0)))
goto out;

Expand Down

0 comments on commit c9af6db

Please sign in to comment.