Skip to content

Commit

Permalink
net: take care of cloned skbs in tcp_try_coalesce()
Browse files Browse the repository at this point in the history
Before stealing fragments or skb head, we must make sure skbs are not
cloned.

Alexander was worried about destination skb being cloned : In bridge
setups, a driver could be fooled if skb->data_len would not match skb
nr_frags.

If source skb is cloned, we must take references on pages instead.

Bug happened using tcpdump (if not using mmap())

Introduce kfree_skb_partial() helper to cleanup code.

Reported-by: Alexander Duyck <alexander.h.duyck@intel.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
  • Loading branch information
Eric Dumazet authored and David S. Miller committed May 3, 2012
1 parent eeb7fc7 commit 923dd34
Showing 1 changed file with 28 additions and 14 deletions.
42 changes: 28 additions & 14 deletions net/ipv4/tcp_input.c
Original file line number Diff line number Diff line change
Expand Up @@ -4532,6 +4532,7 @@ static inline int tcp_try_rmem_schedule(struct sock *sk, unsigned int size)
* @sk: socket
* @to: prior buffer
* @from: buffer to add in queue
* @fragstolen: pointer to boolean
*
* Before queueing skb @from after @to, try to merge them
* to reduce overall memory use and queue lengths, if cost is small.
Expand All @@ -4544,10 +4545,10 @@ static bool tcp_try_coalesce(struct sock *sk,
struct sk_buff *from,
bool *fragstolen)
{
int delta, len = from->len;
int i, delta, len = from->len;

*fragstolen = false;
if (tcp_hdr(from)->fin)
if (tcp_hdr(from)->fin || skb_cloned(to))
return false;
if (len <= skb_tailroom(to)) {
BUG_ON(skb_copy_bits(from, 0, skb_put(to, len), len));
Expand All @@ -4574,7 +4575,13 @@ static bool tcp_try_coalesce(struct sock *sk,
skb_shinfo(from)->frags,
skb_shinfo(from)->nr_frags * sizeof(skb_frag_t));
skb_shinfo(to)->nr_frags += skb_shinfo(from)->nr_frags;
skb_shinfo(from)->nr_frags = 0;

if (skb_cloned(from))
for (i = 0; i < skb_shinfo(from)->nr_frags; i++)
skb_frag_ref(from, i);
else
skb_shinfo(from)->nr_frags = 0;

to->truesize += delta;
atomic_add(delta, &sk->sk_rmem_alloc);
sk_mem_charge(sk, delta);
Expand All @@ -4592,13 +4599,26 @@ static bool tcp_try_coalesce(struct sock *sk,
offset = from->data - (unsigned char *)page_address(page);
skb_fill_page_desc(to, skb_shinfo(to)->nr_frags,
page, offset, skb_headlen(from));
*fragstolen = true;

if (skb_cloned(from))
get_page(page);
else
*fragstolen = true;

delta = len; /* we dont know real truesize... */
goto copyfrags;
}
return false;
}

static void kfree_skb_partial(struct sk_buff *skb, bool head_stolen)
{
if (head_stolen)
kmem_cache_free(skbuff_head_cache, skb);
else
__kfree_skb(skb);
}

static void tcp_data_queue_ofo(struct sock *sk, struct sk_buff *skb)
{
struct tcp_sock *tp = tcp_sk(sk);
Expand Down Expand Up @@ -4642,10 +4662,7 @@ static void tcp_data_queue_ofo(struct sock *sk, struct sk_buff *skb)
if (!tcp_try_coalesce(sk, skb1, skb, &fragstolen)) {
__skb_queue_after(&tp->out_of_order_queue, skb1, skb);
} else {
if (fragstolen)
kmem_cache_free(skbuff_head_cache, skb);
else
__kfree_skb(skb);
kfree_skb_partial(skb, fragstolen);
skb = NULL;
}

Expand Down Expand Up @@ -4804,12 +4821,9 @@ static void tcp_data_queue(struct sock *sk, struct sk_buff *skb)

tcp_fast_path_check(sk);

if (eaten > 0) {
if (fragstolen)
kmem_cache_free(skbuff_head_cache, skb);
else
__kfree_skb(skb);
} else if (!sock_flag(sk, SOCK_DEAD))
if (eaten > 0)
kfree_skb_partial(skb, fragstolen);
else if (!sock_flag(sk, SOCK_DEAD))
sk->sk_data_ready(sk, 0);
return;
}
Expand Down

0 comments on commit 923dd34

Please sign in to comment.