Adding data to SKB - odd checksum errors

Adding data to SKB - odd checksum errors

[Kristian Evensen]

Hello,

I am working on an algorithm to add data from the previous skb (on the 
queue) to the front of the current skb. This should be beneficial for a 
certain kind of TCP-traffic, and I am curious as to wether it will work 
or not.

Currently I have implemented a small algorithm to copy the data that 
works most of the time. It is called from write_xmit (right before the 
while-loop) and performs a number of checks (does the skb fit in the 
window, does it have enought space - mostly the same as the 
retrans_try_collapse-function) before it copies the data. I first 
"allocate" new data at the back of the skb using skb_put, and if that is 
succsessfull I copy the new data (using first memmove to move the old 
data and then memcpy), update the seq-number of this skb and calculate a 
new checksum. The algorithm will (currently) not work with non-linear 
skbs. I have pasted the code at the bottom of this mail.

The weird thing about this algorithm is that something will suddenly 
make it go wrong/it makes something else go wrong, and the packets that 
I send have the wrong TCP-checksum. I have looked around the code for 
any counters I might have missed or similar, but I cant find any. If I 
have understod skb's correctly, I shouldn't have to update any counters 
except skb->len (which put does) since I dont expand the SKB I only use 
the space already reserved for it.

Does anyone have any ideas to what might be wrong or can spot any 
errors/misunderstandings in my code?

Thanks,
Kristian

The code:

This goes into write_xmit before the loop:
if(sysctl_tcp_thin_aggressive_bundling && tcp_stream_is_thin(tp)){
    if(skb->prev != (struct sk_buff*) &(sk)->sk_write_queue
        && !(TCP_SKB_CB(skb)->flags & TCPCB_FLAG_SYN)
        && (skb_shinfo(skb)->nr_frags == 0 &&
        skb_shinfo(skb->prev)->nr_frags == 0)){
            tcp_trans_try_collapse2(sk, skb, tcp_current_mss(sk, 0))
        }
}

This is the code that copies the data:
static int tcp_trans_try_collapse2(struct sock *sk, struct sk_buff *skb, 
int mss_now)
{
    struct tcp_sock *tp = tcp_sk(sk);

    /* Make sure that this isnt refereced by somebody else
     */
    if(!skb_cloned(skb)){
        struct sk_buff *prev_skb = skb_copy(skb->prev, GFP_ATOMIC);
        int skb_size = skb->len, prev_skb_size = prev_skb->len;
        u16 flags = TCP_SKB_CB(prev_skb)->flags;

        /* Since this technique currently does not support SACK, I
            * return -1 if the previous has been SACK'd. */
        if(TCP_SKB_CB(prev_skb)->sacked & TCPCB_SACKED_ACKED){
            return -1;
        }

        /* Current skb is out of window. */
        if (after(TCP_SKB_CB(skb)->end_seq, tp->snd_una+tp->snd_wnd)){
            return -1;
        }

        /* Punt if not enough space exists in the first SKB for
         * the data in the second, or the total combined payload
         * would exceed the MSS.
         */
        if ((prev_skb_size > skb_tailroom(skb)) ||
            ((skb_size + prev_skb_size) > mss_now)){
            return -1;
        }

        /*To avoid duplicate copies.*/
        if(TCP_SKB_CB(skb)->seq <= TCP_SKB_CB(prev_skb)->seq)
            return -1;

        /*First, check I have enough room*/
        if(skb_tailroom(skb) < prev_skb->len)
            return -1;

        copy = skb_put(skb, prev_skb->len);

        if(copy){
            memmove(skb->data + prev_skb->len, skb->data, skb->len - 
prev_skb->len);
            memcpy(skb->data, prev_skb->data, prev_skb->len);
            TCP_SKB_CB(skb)->seq = TCP_SKB_CB(prev_skb)->seq;
            skb->csum = csum_partial(skb->data, skb->len, 0);
        }

        __kfree_skb(prev_skb);
    }

    return 1;
}

Re: Adding data to SKB - odd checksum errors

[Sergio Paracuellos]

Hi,

This is because if you change the skb you must calcutale checksum for
tcp and ip headers changes again. For example:

struct iphdr *new_iph = NULL;
struct tcphdr *new_th = NULL;

/* copy skb to another skb with skb_copy */
new_skb = skb_copy_expand(skb, skb_headroom(skb), MY_SIZE, GFP_ATOMIC);

/* more bytes for buffer */
skb_put(new_skb, MY_SIZE);

/* now get pointer to the the new ip and tcp headers (should be the same
in the other skb */
new_th = new_skb->h.th;
new_iph = new_skb->nh.iph;

/* suppose I change the len */
new_ip_tot_len = htons(ntohs(new_iph->tot_len) + MY_SIZE);
new_iph->check = new_check(new_iph->tot_len ^ 0xFFFF, new_ip_tot_len,
iph->check)

where new_check can be something similar to:

static inline u_int16_t
new_check(u_int32_t oldvalinv, u_int32_t newval, u_int16_t oldcheck)
{
	u_int32_t diffs[] = { oldvalinv, newval };
	return csum_fold(csum_partial((char *)diffs, sizeof(diffs),
				oldcheck^0xFFFF));
}

Am I in a mistake? I suppose this can be done in this way, but I am not
sure.

Good luck!

Cheers,
	Sergio

El dom, 25-02-2007 a las 15:34 +0200, Kristian Evensen escribió:
> Hello,
> 
> I am working on an algorithm to add data from the previous skb (on the 
> queue) to the front of the current skb. This should be beneficial for a 
> certain kind of TCP-traffic, and I am curious as to wether it will work 
> or not.
> 
> Currently I have implemented a small algorithm to copy the data that 
> works most of the time. It is called from write_xmit (right before the 
> while-loop) and performs a number of checks (does the skb fit in the 
> window, does it have enought space - mostly the same as the 
> retrans_try_collapse-function) before it copies the data. I first 
> "allocate" new data at the back of the skb using skb_put, and if that is 
> succsessfull I copy the new data (using first memmove to move the old 
> data and then memcpy), update the seq-number of this skb and calculate a 
> new checksum. The algorithm will (currently) not work with non-linear 
> skbs. I have pasted the code at the bottom of this mail.
> 
> The weird thing about this algorithm is that something will suddenly 
> make it go wrong/it makes something else go wrong, and the packets that 
> I send have the wrong TCP-checksum. I have looked around the code for 
> any counters I might have missed or similar, but I cant find any. If I 
> have understod skb's correctly, I shouldn't have to update any counters 
> except skb->len (which put does) since I dont expand the SKB I only use 
> the space already reserved for it.
> 
> Does anyone have any ideas to what might be wrong or can spot any 
> errors/misunderstandings in my code?
> 
> Thanks,
> Kristian
> 
> The code:
> 
> This goes into write_xmit before the loop:
> if(sysctl_tcp_thin_aggressive_bundling && tcp_stream_is_thin(tp)){
>     if(skb->prev != (struct sk_buff*) &(sk)->sk_write_queue
>         && !(TCP_SKB_CB(skb)->flags & TCPCB_FLAG_SYN)
>         && (skb_shinfo(skb)->nr_frags == 0 &&
>         skb_shinfo(skb->prev)->nr_frags == 0)){
>             tcp_trans_try_collapse2(sk, skb, tcp_current_mss(sk, 0))
>         }
> }
> 
> This is the code that copies the data:
> static int tcp_trans_try_collapse2(struct sock *sk, struct sk_buff *skb, 
> int mss_now)
> {
>     struct tcp_sock *tp = tcp_sk(sk);
> 
>     /* Make sure that this isnt refereced by somebody else
>      */
>     if(!skb_cloned(skb)){
>         struct sk_buff *prev_skb = skb_copy(skb->prev, GFP_ATOMIC);
>         int skb_size = skb->len, prev_skb_size = prev_skb->len;
>         u16 flags = TCP_SKB_CB(prev_skb)->flags;
> 
>         /* Since this technique currently does not support SACK, I
>             * return -1 if the previous has been SACK'd. */
>         if(TCP_SKB_CB(prev_skb)->sacked & TCPCB_SACKED_ACKED){
>             return -1;
>         }
> 
>         /* Current skb is out of window. */
>         if (after(TCP_SKB_CB(skb)->end_seq, tp->snd_una+tp->snd_wnd)){
>             return -1;
>         }
> 
>         /* Punt if not enough space exists in the first SKB for
>          * the data in the second, or the total combined payload
>          * would exceed the MSS.
>          */
>         if ((prev_skb_size > skb_tailroom(skb)) ||
>             ((skb_size + prev_skb_size) > mss_now)){
>             return -1;
>         }
> 
>         /*To avoid duplicate copies.*/
>         if(TCP_SKB_CB(skb)->seq <= TCP_SKB_CB(prev_skb)->seq)
>             return -1;
> 
>         /*First, check I have enough room*/
>         if(skb_tailroom(skb) < prev_skb->len)
>             return -1;
> 
>         copy = skb_put(skb, prev_skb->len);
> 
>         if(copy){
>             memmove(skb->data + prev_skb->len, skb->data, skb->len - 
> prev_skb->len);
>             memcpy(skb->data, prev_skb->data, prev_skb->len);
>             TCP_SKB_CB(skb)->seq = TCP_SKB_CB(prev_skb)->seq;
>             skb->csum = csum_partial(skb->data, skb->len, 0);
>         }
> 
>         __kfree_skb(prev_skb);
>     }
> 
>     return 1;
> }
> -
> To unsubscribe from this list: send the line "unsubscribe linux-net" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
-- 
“No quiero mandar en naide ni que me manden a mi. Me gusta vivir
errante, hoy aquí y mañana allí, y mi vida sigue adelante.” 

Camarón