Re: the TSO packet mangling issue

[Alexey Kuznetsov <kuznet@xxxxxxxxxxxxx>]

Hello!

> This patch adds tcp_skb_cow() and uses it in e1000/tg3.

Looks almost nice to me.

Actually, I would prefer to unbind this from tcp, it can be used
for something else, sctp or anything else who sends clones of skbs
and wants to protect only data part.

Also, tcp_skb_cow() is not good. F.e. the first thing, which I thought
of, was WLAN drivers which rewrite ethernet header and either screw up
tcpdumps (and break net/bridge) or have to copy each tcp skb.
I used to patch it with:

diff -ru ../madwifi-cvs.current/madwifi/driver/if_ath.c madwifi/driver/if_ath.c
--- ../madwifi-cvs.current/madwifi/driver/if_ath.c      2004-06-08 
17:38:20.587760856 +0400
+++ madwifi/driver/if_ath.c     2004-06-04 11:06:43.000000000 +0400
@@ -759,7 +759,9 @@

        if (ic->ic_flags & IEEE80211_F_WEPON)
                len += IEEE80211_WEP_IVLEN + IEEE80211_WEP_KIDLEN;
-       if ((skb_headroom(skb) < len) &&
+
+       if (len < skb_headroom(skb)) len=skb_headroom(skb);
+       if ((skb_cloned(skb) || (skb_headroom(skb) < len)) &&
            pskb_expand_head(skb, len - skb_headroom(skb), 0, GFP_ATOMIC)) {
                dev_kfree_skb(skb);
                return -ENOMEM;

I think it would be good to make a function sort of:

static inline int skb_header_cloned(struct sk_buff *skb)
{
        int dataref;

        if (!skb->cloned)
                return 0;

        dataref = atomic_read(&skb_shinfo(skb)->dataref);
        dataref -= !!(dataref & SKB_DATAREF_TCP);
        dataref &= SKB_DATAREF_MASK;
        return (dataref != 1);
}

In this case many of skb_cloned() could be just replaced with
skb_header_cloned().

Alexey