SKB truesize bug

SKB truesize bug

[Stephen Hemminger]

Is this bug fixed?  is it in the 2.6.26.8 kernel?

http://bugzilla.kernel.org/show_bug.cgi?id=10996

Re: SKB truesize bug

[Herbert Xu]

On Tue, Nov 11, 2008 at 09:25:13AM -0800, Stephen Hemminger wrote:
> Is this bug fixed?  is it in the 2.6.26.8 kernel?

No and yes.

We need to fix up the truesize when resizing packets for IPsec
processing.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[PATCH][IPv4] UFO: prevent generation of chained skb

[Kostya B]

[IPv4] UFO: prevent generation of chained skb destined to UFO device

Problem:
ip_append_data() could wrongly generate a chained skb for devices which support UFO.
When sk_write_queue is not empty (e.g. MSG_MORE), __instead__ of appending data 
into the next nr_frag of the queued skb, a new chained skb is created.

I would normally assume UFO device should get data in nr_frags and not in frag_list.
Later the udp4_hwcsum_outgoing() resets csum to NONE and skb_gso_segment() has oops.

Proposal:
1. Even length is less than mtu, employ ip_ufo_append_data() and append data to the  __existed__ skb in the sk_write_queue.

2. ip_ufo_append_data() is fixed due to a wrong manipulation of peek-ing and later enqueue-ing of the same skb.
Now, enqueuing is always performed, because on error the further ip_flush_pending_frames() would release the queued skb.


Signed-off-by: Kostya B 

---

net/ipv4/ip_output.c |   25 +++++++++----------------
1 file changed, 9 insertions(+), 16 deletions(-)

diff -uprN -X linux-2.6.25-git11/Documentation/dontdiff linux-2.6.25-git11/net/ipv4/ip_output.c linux-2.6.25-git11-fix/net/ipv4/ip_output.c
--- linux-2.6.25-git11/net/ipv4/ip_output.c     2008-04-28 12:22:00.000000000 +0300
+++ linux-2.6.25-git11-fix/net/ipv4/ip_output.c 2008-04-28 14:02:41.000000000 +0300
@@ -753,23 +753,15 @@ static inline int ip_ufo_append_data(str
                skb->ip_summed = CHECKSUM_PARTIAL;
                skb->csum = 0;
                sk->sk_sndmsg_off = 0;
+
+                /* specify the length of each IP datagram fragment*/
+                skb_shinfo(skb)->gso_size = mtu - fragheaderlen;
+                skb_shinfo(skb)->gso_type = SKB_GSO_UDP;
+                __skb_queue_tail(&sk->sk_write_queue, skb);
        }

-       err = skb_append_datato_frags(sk,skb, getfrag, from,
+       return skb_append_datato_frags(sk,skb, getfrag, from,
                               (length - transhdrlen));
-       if (!err) {
-               /* specify the length of each IP datagram fragment*/
-               skb_shinfo(skb)->gso_size = mtu - fragheaderlen;
-               skb_shinfo(skb)->gso_type = SKB_GSO_UDP;
-               __skb_queue_tail(&sk->sk_write_queue, skb);
-
-               return 0;
-       }
-       /* There is not enough support do UFO ,
-        * so follow normal path
-        */
-       kfree_skb(skb);
-       return err;
 }

 /*
@@ -863,8 +855,9 @@ int ip_append_data(struct sock *sk,
                csummode = CHECKSUM_PARTIAL;

        inet->cork.length += length;
-       if (((length> mtu) && (sk->sk_protocol == IPPROTO_UDP)) &&
-                       (rt->u.dst.dev->features & NETIF_F_UFO)) {
+       if (((length> mtu) || !skb_queue_empty(&sk->sk_write_queue)) &&
+           (sk->sk_protocol == IPPROTO_UDP) &&
+           (rt->u.dst.dev->features & NETIF_F_UFO)) {

                err = ip_ufo_append_data(sk, getfrag, from, length, hh_len,
                                         fragheaderlen, transhdrlen, mtu,
---
_________________________________________________________________
Explore the seven wonders of the world
http://search.msn.com/results.aspx?q=7+wonders+world&mkt=en-US&form=QBRE

Re: [PATCH][IPv4] UFO: prevent generation of chained skb

[David Miller]

From: Kostya B <bkostya@hotmail.com>
Date: Mon, 28 Apr 2008 12:13:27 +0000

> 
> [IPv4] UFO: prevent generation of chained skb destined to UFO device
> 
> Problem:
> ip_append_data() could wrongly generate a chained skb for devices which support UFO.
> When sk_write_queue is not empty (e.g. MSG_MORE), __instead__ of appending data 
> into the next nr_frag of the queued skb, a new chained skb is created.
> 
> I would normally assume UFO device should get data in nr_frags and not in frag_list.
> Later the udp4_hwcsum_outgoing() resets csum to NONE and skb_gso_segment() has oops.
> 
> Proposal:
> 1. Even length is less than mtu, employ ip_ufo_append_data() and append data to the  __existed__ skb in the sk_write_queue.
> 
> 2. ip_ufo_append_data() is fixed due to a wrong manipulation of peek-ing and later enqueue-ing of the same skb.
> Now, enqueuing is always performed, because on error the further ip_flush_pending_frames() would release the queued skb.
> 
> 
> Signed-off-by: Kostya B 

Mostly this looks fine to me.

Herbert, can you take a quick look at this?

Thanks.

> 
> ---
> 
> net/ipv4/ip_output.c |   25 +++++++++----------------
> 1 file changed, 9 insertions(+), 16 deletions(-)
> 
> diff -uprN -X linux-2.6.25-git11/Documentation/dontdiff linux-2.6.25-git11/net/ipv4/ip_output.c linux-2.6.25-git11-fix/net/ipv4/ip_output.c
> --- linux-2.6.25-git11/net/ipv4/ip_output.c     2008-04-28 12:22:00.000000000 +0300
> +++ linux-2.6.25-git11-fix/net/ipv4/ip_output.c 2008-04-28 14:02:41.000000000 +0300
> @@ -753,23 +753,15 @@ static inline int ip_ufo_append_data(str
>                 skb->ip_summed = CHECKSUM_PARTIAL;
>                 skb->csum = 0;
>                 sk->sk_sndmsg_off = 0;
> +
> +                /* specify the length of each IP datagram fragment*/
> +                skb_shinfo(skb)->gso_size = mtu - fragheaderlen;
> +                skb_shinfo(skb)->gso_type = SKB_GSO_UDP;
> +                __skb_queue_tail(&sk->sk_write_queue, skb);
>         }
> 
> -       err = skb_append_datato_frags(sk,skb, getfrag, from,
> +       return skb_append_datato_frags(sk,skb, getfrag, from,
>                                (length - transhdrlen));
> -       if (!err) {
> -               /* specify the length of each IP datagram fragment*/
> -               skb_shinfo(skb)->gso_size = mtu - fragheaderlen;
> -               skb_shinfo(skb)->gso_type = SKB_GSO_UDP;
> -               __skb_queue_tail(&sk->sk_write_queue, skb);
> -
> -               return 0;
> -       }
> -       /* There is not enough support do UFO ,
> -        * so follow normal path
> -        */
> -       kfree_skb(skb);
> -       return err;
>  }
> 
>  /*
> @@ -863,8 +855,9 @@ int ip_append_data(struct sock *sk,
>                 csummode = CHECKSUM_PARTIAL;
> 
>         inet->cork.length += length;
> -       if (((length> mtu) && (sk->sk_protocol == IPPROTO_UDP)) &&
> -                       (rt->u.dst.dev->features & NETIF_F_UFO)) {
> +       if (((length> mtu) || !skb_queue_empty(&sk->sk_write_queue)) &&
> +           (sk->sk_protocol == IPPROTO_UDP) &&
> +           (rt->u.dst.dev->features & NETIF_F_UFO)) {
> 
>                 err = ip_ufo_append_data(sk, getfrag, from, length, hh_len,
>                                          fragheaderlen, transhdrlen, mtu,
> ---
> _________________________________________________________________
> Explore the seven wonders of the world
> http://search.msn.com/results.aspx?q=7+wonders+world&mkt=en-US&form=QBRE
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH][IPv4] UFO: prevent generation of chained skb

[Herbert Xu]

On Tue, Apr 29, 2008 at 03:27:14AM -0700, David Miller wrote:
>
> Mostly this looks fine to me.
> 
> Herbert, can you take a quick look at this?

Yep looks good to me too.

Acked-by: Herbert Xu <herbert@gondor.apana.org.au>

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH][IPv4] UFO: prevent generation of chained skb

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Tue, 29 Apr 2008 20:07:13 +0800

> On Tue, Apr 29, 2008 at 03:27:14AM -0700, David Miller wrote:
> >
> > Mostly this looks fine to me.
> > 
> > Herbert, can you take a quick look at this?
> 
> Yep looks good to me too.
> 
> Acked-by: Herbert Xu <herbert@gondor.apana.org.au>

Applied, thanks everyone.

K, I had to fix up your patch by hand because your email
client corrupted the patch, changing tabs into space
characters, and things of that nature.

Also, your patch description had many lines longer than
80 columns, which I had to fix up as well.

I'm happy to do this before you are aware of the problem,
like this time, but now that I've made you aware I will
push back future patches which have these errors.

Thank you.

RE: SKB truesize bug

[Kostya B]

Hi,

Perhaps we should take a closer look on pskb_expand_head()

And add 

+skb->truesize = size + sizeof(struct sk_buff);


Kostya

_________________________________________________________________
Invite your mail contacts to join your friends list with Windows Live Spaces. It's easy!
http://spaces.live.com/spacesapi.aspx?wx_action=create&wx_url=/friends.aspx&mkt=en-us

Re: SKB truesize bug

[David Miller]

From: Kostya B <bkostya@hotmail.com>
Date: Thu, 20 Nov 2008 11:59:28 +0000

> Perhaps we should take a closer look on pskb_expand_head()
> 
> And add 
> 
> +skb->truesize = size + sizeof(struct sk_buff);

You can't.  If a socket is attached to the skb it's buffer
accounting will get corrupted if you modify skb->truesize.

This has been explained a thousand times on this list.