IP layer bug?

IP layer bug?

[Oleg Drokin]

Hello!

   2.4.x kernel. have not tried 2.2
   I just found somethig, I believe is kernel bug.
   I am working with usbnet.c driver, which stores some of its
   internal state in sk_buff.cb area. But once such skb passed to
   upper layer with netif_rx, net/ipv4/ip_input.c reuses content of cb
   (line #345), and all packets that should go outside of beyond hosts
   we have direct routes to, fails, because we think, they have source routing
   enabled.
   For now I workarounded it with filling skb->cb with zeroes before
   netif_rx(), but I believe it is a kludge and networking layer should be fixed
   instead.

   Thank you.

Bye,
    Oleg

Re: IP layer bug?

[Manoj Sontakke]

Hi
On Sun, 25 Mar 2001, Oleg Drokin wrote:

> Hello!
> 
>    2.4.x kernel. have not tried 2.2
>    I just found somethig, I believe is kernel bug.
>    I am working with usbnet.c driver, which stores some of its
>    internal state in sk_buff.cb area. But once such skb passed to
>    upper layer with netif_rx, net/ipv4/ip_input.c reuses content of cb
>    (line #345), 
ip_options_compile() when called with first argument NULL resets cb to 0.
This is probably because the cb is supposed to be used IP and above. The
underlying layer(link and phy) could be anything so where from the
ip_options should start will depend upon the underlying layer.

>    and all packets that should go outside of beyond hosts
>    we have direct routes to, fails, because we think, they have source routing
>    enabled.
>    For now I workarounded it with filling skb->cb with zeroes before
>    netif_rx(), but I believe it is a kludge and networking layer should be fixed
>    instead.
> 
>    Thank you.
> 
> Bye,
>     Oleg
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

-- 
Regards,
Manoj Sontakke

Re: IP layer bug?

[Oleg Drokin]

Hello!

On Mon, Mar 26, 2001 at 04:06:19PM +0530, Manoj Sontakke wrote:
> >    2.4.x kernel. have not tried 2.2
> >    I just found somethig, I believe is kernel bug.
> >    I am working with usbnet.c driver, which stores some of its
> >    internal state in sk_buff.cb area. But once such skb passed to
> >    upper layer with netif_rx, net/ipv4/ip_input.c reuses content of cb
> >    (line #345), 
> ip_options_compile() when called with first argument NULL resets cb to 0.
I have found that already.

> This is probably because the cb is supposed to be used IP and above. The
Sure.

> underlying layer(link and phy) could be anything so where from the
> ip_options should start will depend upon the underlying layer.
But here's the problem!
If I won't zero cb in my driver before netif_rx() call,
IP layer thinks that all my packets have various ip options set
(source routing most notable)

Bye,
    Oleg

Re: IP layer bug?

[Manoj Sontakke]

Hi,
On Mon, 26 Mar 2001, Oleg Drokin wrote:

> Hello!
> 
> On Mon, Mar 26, 2001 at 04:06:19PM +0530, Manoj Sontakke wrote:
> > >    2.4.x kernel. have not tried 2.2
> > >    I just found somethig, I believe is kernel bug.
> > >    I am working with usbnet.c driver, which stores some of its
> > >    internal state in sk_buff.cb area. But once such skb passed to
> > >    upper layer with netif_rx, net/ipv4/ip_input.c reuses content of cb
> > >    (line #345), 
> > ip_options_compile() when called with first argument NULL resets cb to 0.
> I have found that already.
> 
> > This is probably because the cb is supposed to be used IP and above. The
> Sure.
> 
> > underlying layer(link and phy) could be anything so where from the
> > ip_options should start will depend upon the underlying layer.
> But here's the problem!
> If I won't zero cb in my driver before netif_rx() call,
> IP layer thinks that all my packets have various ip options set
> (source routing most notable)

U can set cb to zero, but u also plan to use cb for storing ur data. If
that happens then u need to modify the way the macro IPCB
(probably in net/ip.h) is defined. Also make sure to increase the size 
of cb to accomodate ur data. This will solve ur problem but making this
general (part of the standard kernel code) needs a lot of work in the ip
layer. The cb is also used by TCP. The same needs to be done in IP layer
but this will again largly depend on the layers below and hence the
complexity.

The alternative to this is that u can add another buffer like cb in
sk_buff.

Manoj Sontakke

Re: IP layer bug?

[Oleg Drokin]

Hello!

On Mon, Mar 26, 2001 at 05:51:43PM +0530, Manoj Sontakke wrote:
> > > ip_options_compile() when called with first argument NULL resets cb to 0.
> > I have found that already.
> > > underlying layer(link and phy) could be anything so where from the
> > > ip_options should start will depend upon the underlying layer.
> > But here's the problem!
> > If I won't zero cb in my driver before netif_rx() call,
> > IP layer thinks that all my packets have various ip options set
> > (source routing most notable)
> U can set cb to zero, but u also plan to use cb for storing ur data. If
I use it for storing data, but when I do netif_rx() on skb, I cannot touch it
anymore, so no data there is belong to me, and if I read that code in
ip_input.c, it gets zeroed.
BTW, I found that it's not ip_input.c that sends me 'source routing rejected'
message, because no message is logged. It might be some other place.
But anyway something is wrong in IP layer.

> that happens then u need to modify the way the macro IPCB
> (probably in net/ip.h) is defined. Also make sure to increase the size 
> of cb to accomodate ur data. This will solve ur problem but making this
My data perfectly fits in cb of current size.

> general (part of the standard kernel code) needs a lot of work in the ip
> layer. The cb is also used by TCP. The same needs to be done in IP layer
> but this will again largly depend on the layers below and hence the
> complexity.
But comment in skbuff.h claims that any owner of skb can use cb for its own needs, till it passes ownership to whoever else.

> The alternative to this is that u can add another buffer like cb in
> sk_buff.
What for?

Bye,
    Oleg

Re: IP layer bug?

[kuznet]

Hello!

>    For now I workarounded it with filling skb->cb with zeroes before
>    netif_rx(),

This is right. For another examples look into tunnels.

> but I believe it is a kludge and networking layer should be fixed instead.

No.

alloc_skb() creates skb with clean cb. ip_rcv() and other protocol handlers
do not redo this work. If device uses cb internally, it must clear it
before handing skb to netif_rx().

Alexey

Re: IP layer bug?

[Oleg Drokin]

Hello!

On Fri, Mar 30, 2001 at 09:13:40PM +0400, kuznet@ms2.inr.ac.ru wrote:
> >    For now I workarounded it with filling skb->cb with zeroes before
> >    netif_rx(),
> This is right. For another examples look into tunnels.
Hm. But comment in linux/skbuff.h says:
        /*
         * This is the control buffer. It is free to use for every
         * layer. Please put your private variables there. If you
         * want to keep them across layers you have to do a skb_clone()
         * first. This is owned by whoever has the skb queued ATM.
         */
Which does not imply I should clear buffer after I am passing ownership.

> > but I believe it is a kludge and networking layer should be fixed instead.
> No.

> alloc_skb() creates skb with clean cb. ip_rcv() and other protocol handlers
> do not redo this work. If device uses cb internally, it must clear it
> before handing skb to netif_rx().
Why not document it somewhere, so that others will not fall into the same trap?

Bye,
    Oleg

Re: IP layer bug?

[kuznet]

Hello!

> Hm. But comment in linux/skbuff.h says:

The comment is about more difficult case: transmit path,
where cb is used both by top level protocol and lower layers:
f.e. TCP -> IP -> device. cb is dirty from the moment of skb
creation in this case.

Also, note that the second sentence in the comment is obsolete.
Passing not cloned skbs between layers is strongly deprecated
practice (I hope it is not used in any place) and cb of skb entering
to lower layer is property of the layer.

RX path is simpler: cb must be kept clean, that's all.

General rule is minimization redundant clearings of the area.

> Why not document it somewhere, so that others will not fall into the same trap?

Indeed. 8) You got the experience, which you expect to be useful
for people, it is time to prepare some note recording this. 8)

Alexey

Re: IP layer bug?

[Oleg Drokin]

Hello!

On Sat, Mar 31, 2001 at 07:32:48PM +0400, kuznet@ms2.inr.ac.ru wrote:

> General rule is minimization redundant clearings of the area.
> > Why not document it somewhere, so that others will not fall into the same trap?
> Indeed. 8) You got the experience, which you expect to be useful
> for people, it is time to prepare some note recording this. 8)
I cannot think of something better that piece below, so I think
you may want to change it, anyway ;)


--- include/linux/skbuff.h.orig	Mon Apr  2 13:13:46 2001
+++ include/linux/skbuff.h	Mon Apr  2 13:24:18 2001
@@ -102,7 +102,10 @@
 	 * This is the control buffer. It is free to use for every
 	 * layer. Please put your private variables there. If you
 	 * want to keep them across layers you have to do a skb_clone()
-	 * first. This is owned by whoever has the skb queued ATM.
+	 * first (which is a must, anyway). This is owned by whoever
+	 * has the skb queued ATM. 
+	 * Driver writers: notice you should zero cb before netif_rx()
+	 * if you used it.
 	 */ 
 	char		cb[48];	 
 

Bye,
    Oleg