From: "Brandeburg, Jesse" <[EMAIL PROTECTED]>
Date: Wed, 29 Mar 2006 18:53:57 -0800
> To do this we have code like so in e1000_tso:
> 2529 if (skb_shinfo(skb)->tso_size) {
> 2530 if (skb_header_cloned(skb)) {
> 2531 err = pskb_expand_head(skb, 0, 0,
> GFP_ATOMIC);
> 2532 if (err)
> 2533 return err;
> 2534 }
I was wondering if that call could somehow mess up the
sk->sk_forward_alloc value later on.
But it can't, sk_forward_alloc is modified based upon the
skb->truesize value, but pskb_expand_head() does not change that.
So the things left to check in the generic networking are the
skb_shinfo() contents and ->dataref handling.
I considered whether pskb_expand_head() could corrupt the TSO
information in skb_shinfo(). But that's clearly not the case because
pskb_expand_head() explicitly copies it over:
memcpy(data + size, skb->end, sizeof(struct skb_shared_info));
And skb->end is set appropriately:
skb->end = data + size;
because skb_shinfo() is:
#define skb_shinfo(SKB) ((struct skb_shared_info *)((SKB)->end))
The only skb_shared_info that has to be explicitly setup is the
dataref, and pskb_expand_head() does that:
atomic_set(&skb_shinfo(skb)->dataref, 1);
So that all checks out.
I wonder if something funky is going on wrt. the skb_release_data()
that pskb_expand_head() does. We have that SKB_DATAREF_SHIFT thingy,
which will trigger in this case.
if (!skb->cloned ||
!atomic_sub_return(skb->nohdr ? (1 << SKB_DATAREF_SHIFT) + 1 : 1,
&skb_shinfo(skb)->dataref)) {
When we enqueue a new TCP frame we do skb_header_release() which goes:
skb->nohdr = 1;
atomic_add(1 << SKB_DATAREF_SHIFT, &skb_shinfo(skb)->dataref);
Presumably the dataref is "1" already when we get here and do this.
We will clone, the clone will set ->nohdr to 0 and will increment the
dataref.
So at this point the dataref should be:
1 /* initial reference */
+ (1 << SKB_DATAREF_SHIFT) /* from skb_header_release() */
+ 1 /* from skb_clone */
This all works out because when the clone is freed up, skb->nohdr will
be zero, so we will subtract "1" from dataref. Later when the ACK
arrives we'll free up the non-clone and this will have skb->nohdr set
to "1" and thus we'll subtract
(1 << SKB_DATAREF_SHIFT) + 1
from dataref, as per skb_release_data().
Although maybe relevant here, I just noticed that __skb_linearize()
does not clear skb->nohdr. I bet that will cause a bunch of trouble
if the original SKB had skb->nohdr set, but I cannot see how that
can occur, we only send clones out to the device and those have
skb->nohdr clear (Herbert, double check this for me please).
Luckily that thing is used rarely. Only in the dev_queue_xmit()
path when the SKB has been configured in such a way that the
transmitting device does not support so it should not be relevant
here. Also I note that __skb_linearize() is not used at all
outside of net/core/dev.c, so we should mark it static some point
soon. In fact we should do that while fixing this fringe "nohdr"
bug in __skb_linearize().
All the other dataref accesses look safe.
Herbert do you see any holes here?
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
