[ATM] who 'owns' the skb created by drivers/atm?

[ATM] who 'owns' the skb created by drivers/atm?

[chas williams]

when one of the atm drivers has a skb ready to pass up to the higher 
layers it may (optionally?) fill in skb->cb.  this usually just holds
a pointer to the atm_vcc that the skb arrived on.  if this skb is
destined for an atm socket, all is well.  the trouble arises when the skb
is bound for the ip layer via lec or clip.  lec or clip just push
the skb up to the ip layer via netif_rx().  sometimes (particularly true
on 64-bit platforms) the ip layer will interpret the skb->cb (for ip
the first 4 bytes of skb->cb are the next hop address which isnt used
much apparently).

its my understanding is that you can't use skb->cb unless you created
the skb.  well atm created the skb and filled in ->cb.  it seems ip
doesn't know its sharing this skb with the atm layer and doesnt clone
the skb in ip_rcv().  there seems to be an implicit understanding that
skb's created by ethernet drivers are 'owned' by the ip layer and shouldnt
touch skb->cb.

i would hazard that the atm drivers are not 'owned' by the ip layer --
any skb's that lec or clip send to the ip layer should first cloned and
the clone passed to the ip layer?

Re: [ATM] who 'owns' the skb created by drivers/atm?

[James Morris]

On Thu, 20 Feb 2003, chas williams wrote:

> its my understanding is that you can't use skb->cb unless you created
> the skb.  well atm created the skb and filled in ->cb.  it seems ip
> doesn't know its sharing this skb with the atm layer and doesnt clone
> the skb in ip_rcv().  there seems to be an implicit understanding that
> skb's created by ethernet drivers are 'owned' by the ip layer and shouldnt
> touch skb->cb.
> 

skb->cb is owned by whatever layer is currently processing the skb.


- James
-- 
James Morris
<jmorris@intercode.com.au>

Re: [ATM] who 'owns' the skb created by drivers/atm?

[David S. Miller]

On Thu, 2003-02-20 at 17:42, James Morris wrote:
> skb->cb is owned by whatever layer is currently processing the skb.

Furthermore, once you netif_rx() an SKB it is no longer yours.
It is owned by the networking stack.

If the ATM layer wants to do fancy things and still pass the SKB
to netif_rx(), _it_ should clone the SKB and give that clone to
the ATM layer directly.

Re: [ATM] who 'owns' the skb created by drivers/atm?

[Mitchell Blank Jr]

David S. Miller wrote:
> On Thu, 2003-02-20 at 17:42, James Morris wrote:
> > skb->cb is owned by whatever layer is currently processing the skb.
> 
> Furthermore, once you netif_rx() an SKB it is no longer yours.
> It is owned by the networking stack.
> 
> If the ATM layer wants to do fancy things and still pass the SKB
> to netif_rx(), _it_ should clone the SKB and give that clone to
> the ATM layer directly.

As far as I'm aware the ATM layer doesn't care what happens to the
SKB after it gets passed to netif_rx(), so I don't know why this would
be a problem.  Some people seem to be suggesting that we need to zero
out ->cb before passing the SKB to netif_rx() but I don't see why
that would be neccesary.

-Mitch

Re: [ATM] who 'owns' the skb created by drivers/atm?

[David S. Miller]

   From: Mitchell Blank Jr <mitch@sfgoth.com>
   Date: Thu, 20 Feb 2003 22:12:55 -0800
   
   Some people seem to be suggesting that we need to zero
   out ->cb before passing the SKB to netif_rx() but I don't see why
   that would be neccesary.

It is true, the whole input mechanism depends upon skb->cb[] being
clear on new skbs coming in via netif_rx().

Re: [ATM] who 'owns' the skb created by drivers/atm?

[Mitchell Blank Jr]

David S. Miller wrote:
>    Some people seem to be suggesting that we need to zero
>    out ->cb before passing the SKB to netif_rx() but I don't see why
>    that would be neccesary.
> 
> It is true, the whole input mechanism depends upon skb->cb[] being
> clear on new skbs coming in via netif_rx().

Hmmmm.. I guess we've just been getting lucky before in that case - we've
always just left the ATM_SKB() stuff in there.

Chas - I guess you should just do a memset(skb->cb, 0, sizeof(skb->cb))
just before the netif_rx() in {clip,lec,mpc}.c and before the ppp_input()
in pppoatm.c to make sure it's zeroed correctly.

-Mitch

Re: [ATM] who 'owns' the skb created by drivers/atm?

[chas williams]

In message <20030220222404.B11525@sfgoth.com>,Mitchell Blank Jr writes:
>Hmmmm.. I guess we've just been getting lucky before in that case - we've
>always just left the ATM_SKB() stuff in there.

i think fortunate might be a better word.  comparing the two:

struct atm_skb_data {
        struct atm_vcc  *vcc;       
        int             iovcnt;    
        unsigned long   atm_options;
};

struct inet_skb_parm
{
	struct ip_options {
	  __u32         faddr;
	  unsigned char optlen;
	  unsigned char srr;
	  unsigned char rr;
	  unsigned char ts;
...
}

surprising you only run into trouble on some 64bit platforms.  at this
point the atm_vcc* overlaps with optlen and chaos ensues.  on a somewhat
related note, i believe iovcnt is probably obselete.  skb's support
scatter/gather.

>Chas - I guess you should just do a memset(skb->cb, 0, sizeof(skb->cb))
>just before the netif_rx() in {clip,lec,mpc}.c and before the ppp_input()
>in pppoatm.c to make sure it's zeroed correctly.

this is one option.  the other would be to clone the skb and pass the
clone to the ip layer.  the last option, and the one i prefer, would
be to make the atm drivers not modify skb->cb (or reset it) when passing
up the skb.  the atm socket layer doesnt rely on it, and it would keep
the 'extra' processing to a minimum.

Re: [ATM] who 'owns' the skb created by drivers/atm?

[Werner Almesberger]

chas williams wrote:
> In message <20030220222404.B11525@sfgoth.com>,Mitchell Blank Jr writes:
>>Hmmmm.. I guess we've just been getting lucky before in that case - we've
>>always just left the ATM_SKB() stuff in there.

The "cb must be in virgin state" rule is indeed news to me. But
maybe the rule has always been there, and nobody really noticed :-)

> this is one option.  the other would be to clone the skb and pass the
> clone to the ip layer.  the last option, and the one i prefer, would
> be to make the atm drivers not modify skb->cb (or reset it) when passing
> up the skb.  the atm socket layer doesnt rely on it, and it would keep
> the 'extra' processing to a minimum.

I'm not sure this is the problem: as far as I remember, the ATM stack
doesn't assume that other layers leave skb->cb intact. In fact, it
shouldn't even touch an skb once it has been passed on.

BTW, I'm happy that ATM finally has a maintainer again. Thanks, Chas !

- Werner

-- 
  _________________________________________________________________________
 / Werner Almesberger, Buenos Aires, Argentina         wa@almesberger.net /
/_http://www.almesberger.net/____________________________________________/

Re: [ATM] who 'owns' the skb created by drivers/atm?

[chas williams]

In message <20030223001550.I2791@almesberger.net>,Werner Almesberger writes:
>The "cb must be in virgin state" rule is indeed news to me. But
>maybe the rule has always been there, and nobody really noticed :-)

its particularly a problem (on the ia64 anyway) in ip_options_echo().
optlen (in skb_inet_parm) winds up with values >>40 and it overwrites 
the stack.  it looks like the ip stack wants to process the options
block once and tuck it away in skb_inet_parm.