Regression: Recent networking (qdisc?) patches break irda_get_next_speed()

Regression: Recent networking (qdisc?) patches break irda_get_next_speed()

[Alex Villací­s Lasso]

A regression has been introduced in 2.6.27 in the networking code, which 
breaks the irda_get_next_speed() function used by some IrDA drivers, 
including ks959-sir, written by me. I have filed a bug at:

http://bugzilla.kernel.org/show_bug.cgi?id=11795

to keep track of this. I am still performing bisection to locate the 
exact commit that broke the code, but the bug involves an overwriting of 
the beginning of a structure with extraneous data that makes the 
LAP_MAGIC check fail. I have tried searching the netdev archives, but 
there is no mention at all of regressions caused by this code. Milan 
Plzik at irda-users mentions the same problem affecting pxaficp_ir, and 
mentions recent qdisc patches as possible culprits.  Vasily Khoruzhick 
reported this bug at irda-users, and a quick hack (not a proper fix) 
proposed by him involves padding "struct irda_skb_cb" with 4 bytes at 
the beginning of the structure to move everything else past the buggy 
scribbling.


This URL contains an early reference to what is possibly this same bug
http://sourceforge.net/mailarchive/forum.php?thread_name=1223138254.21553.7.camel%40localhost&forum_name=irda-users

-- 
perl -e '$x=2.4;print sprintf("%.0f + %.0f = %.0f\n",$x,$x,$x+$x);'

Re: Regression: Recent networking (qdisc?) patches break irda_get_next_speed()

[Jarek Poplawski]

Alex Villací­s Lasso wrote, On 10/21/2008 08:20 PM:

> A regression has been introduced in 2.6.27 in the networking code, which 
> breaks the irda_get_next_speed() function used by some IrDA drivers, 
> including ks959-sir, written by me. I have filed a bug at:
> 
> http://bugzilla.kernel.org/show_bug.cgi?id=11795
> 
> to keep track of this. I am still performing bisection to locate the 
> exact commit that broke the code, but the bug involves an overwriting of 
> the beginning of a structure with extraneous data that makes the 
> LAP_MAGIC check fail. I have tried searching the netdev archives, but 
> there is no mention at all of regressions caused by this code. Milan 
> Plzik at irda-users mentions the same problem affecting pxaficp_ir, and 
> mentions recent qdisc patches as possible culprits.  Vasily Khoruzhick 
> reported this bug at irda-users, and a quick hack (not a proper fix) 
> proposed by him involves padding "struct irda_skb_cb" with 4 bytes at 
> the beginning of the structure to move everything else past the buggy 
> scribbling.


Looks like the patch below could fit to your description.

Jarek P.

commit 175f9c1bba9b825d22b142d183c9e175488b260c
Author: Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
Date:   Sun Jul 20 00:08:47 2008 -0700

    net_sched: Add size table for qdiscs

    Add size table functions for qdiscs and calculate packet size in
    qdisc_enqueue().

    Based on patch by Patrick McHardy
     http://marc.info/?l=linux-netdev&m=115201979221729&w=2

    Signed-off-by: Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
    Signed-off-by: David S. Miller <davem@davemloft.net>

Re: Regression: Recent networking (qdisc?) patches break irda_get_next_speed()

[Alex Villací­s Lasso]

Jarek Poplawski escribió:
> Alex Villací­s Lasso wrote, On 10/21/2008 08:20 PM:
>
>   
>> A regression has been introduced in 2.6.27 in the networking code, which 
>> breaks the irda_get_next_speed() function used by some IrDA drivers, 
>> including ks959-sir, written by me. I have filed a bug at:
>>
>> http://bugzilla.kernel.org/show_bug.cgi?id=11795
>>
>> to keep track of this. I am still performing bisection to locate the 
>> exact commit that broke the code, but the bug involves an overwriting of 
>> the beginning of a structure with extraneous data that makes the 
>> LAP_MAGIC check fail. I have tried searching the netdev archives, but 
>> there is no mention at all of regressions caused by this code. Milan 
>> Plzik at irda-users mentions the same problem affecting pxaficp_ir, and 
>> mentions recent qdisc patches as possible culprits.  Vasily Khoruzhick 
>> reported this bug at irda-users, and a quick hack (not a proper fix) 
>> proposed by him involves padding "struct irda_skb_cb" with 4 bytes at 
>> the beginning of the structure to move everything else past the buggy 
>> scribbling.
>>     
>
>
> Looks like the patch below could fit to your description.
>
> Jarek P.
>
> commit 175f9c1bba9b825d22b142d183c9e175488b260c
> Author: Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
> Date:   Sun Jul 20 00:08:47 2008 -0700
>
>     net_sched: Add size table for qdiscs
>
>     Add size table functions for qdiscs and calculate packet size in
>     qdisc_enqueue().
>
>     Based on patch by Patrick McHardy
>      http://marc.info/?l=linux-netdev&m=115201979221729&w=2
>
>     Signed-off-by: Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
>     Signed-off-by: David S. Miller <davem@davemloft.net>
>
>
>   
So then, the bug is that the cb field in the struct sk_buff is being 
interpreted as both a struct qdisc_skb_cb and an struct irda_skb_cb, for 
the same instance of struct sk_buff. I have just started to review the 
suggested patch, but it seems that 'struct qdisc_skb_cb' was meant to be 
aliased against the data for other layers (as suggested by the presence 
of a 'char data[]' field). If so, how come only IrDA is affected? How 
come UDP, TCP, etc. not affected by this? On the other hand, if 
qdisc_skb_cb was not meant to be aliased, then the IrDA case was left 
out while converting the rest of the layers so that they will skip over 
the member 'pkt_len' of the 'struct qdisc_skb_cb'.

Or maybe all the other layers *are* being overwritten, and IrDA is the 
only one that has a 'magic' member to check against corruption... As far 
as I can see, no other layer has a member with an equivalent function.

-- 
perl -e '$x=2.4;print sprintf("%.0f + %.0f = %.0f\n",$x,$x,$x+$x);'

Re: Regression: Recent networking (qdisc?) patches break irda_get_next_speed()

[David Miller]

From: Alex Villací­s Lasso <avillaci@ceibo.fiec.espol.edu.ec>
Date: Tue, 21 Oct 2008 18:37:56 -0500

> So then, the bug is that the cb field in the struct sk_buff is being
> interpreted as both a struct qdisc_skb_cb and an struct irda_skb_cb,
> for the same instance of struct sk_buff. I have just started to
> review the suggested patch, but it seems that 'struct qdisc_skb_cb'
> was meant to be aliased against the data for other layers (as
> suggested by the presence of a 'char data[]' field). If so, how come
> only IrDA is affected? How come UDP, TCP, etc. not affected by this?
> On the other hand, if qdisc_skb_cb was not meant to be aliased, then
> the IrDA case was left out while converting the rest of the layers
> so that they will skip over the member 'pkt_len' of the 'struct
> qdisc_skb_cb'.

The SKB control block is not aliased.

Once the packet is given to dev_queue_xmit() the packet scheduler
"owns" the control block of the SKB.

What IRDA is doing is illegal, and breaks in other ways without the
commit in question.

IRDA cannot depend upon the SKB control block not changing across
the dev_queue_xmit() call.

Re: Regression: Recent networking (qdisc?) patches break irda_get_next_speed()

[Alex Villací­s Lasso]

David Miller escribió:
> From: Alex Villací­s Lasso <avillaci-x0m+Mc+nT7uljOmnV8AmnkElSqmLX1BE@public.gmane.org>
> Date: Tue, 21 Oct 2008 18:37:56 -0500
>
>   
>> So then, the bug is that the cb field in the struct sk_buff is being
>> interpreted as both a struct qdisc_skb_cb and an struct irda_skb_cb,
>> for the same instance of struct sk_buff. I have just started to
>> review the suggested patch, but it seems that 'struct qdisc_skb_cb'
>> was meant to be aliased against the data for other layers (as
>> suggested by the presence of a 'char data[]' field). If so, how come
>> only IrDA is affected? How come UDP, TCP, etc. not affected by this?
>> On the other hand, if qdisc_skb_cb was not meant to be aliased, then
>> the IrDA case was left out while converting the rest of the layers
>> so that they will skip over the member 'pkt_len' of the 'struct
>> qdisc_skb_cb'.
>>     
>
> The SKB control block is not aliased.
>
> Once the packet is given to dev_queue_xmit() the packet scheduler
> "owns" the control block of the SKB.
>
> What IRDA is doing is illegal, and breaks in other ways without the
> commit in question.
>
> IRDA cannot depend upon the SKB control block not changing across
> the dev_queue_xmit() call.
>
>   
Let me see if I understood. So the particular illegal thing the IRDA 
stack is doing is the access of the control block in the middle of the 
driver transmit routine (via irda_get_next_speed() and friends). This 
information should be stored somewhere else. Exactly *where* to store it 
is the main problem to solve.

What is the proper way (if any) to store per-packet parameters (other 
than the payload itself) which are specific to a particular layer (IrDA 
in this case) and which are needed by drivers in order to work 
correctly? The control block gets overwritten by the time the driver 
proc (hard_start_xmit) is called, so this approach is now ruled out. I 
was thinking about storing a copy of the parameters (struct irda_skb_cb) 
as a header within the payload itself (skb->data[]), but I am not sure 
about whether this approach is a good design decision. I am open to 
suggestions on where to place the parameters.

-- 
perl -e '$x=2.4;print sprintf("%.0f + %.0f = %.0f\n",$x,$x,$x+$x);'


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

RE: Regression: Recent networking (qdisc?) patches break irda_get_next_speed()

[Brandeburg, Jesse]

Alex Villací­s Lasso wrote:
<snip>

>> The SKB control block is not aliased.
>> 

...
>> IRDA cannot depend upon the SKB control block not changing across
>> the dev_queue_xmit() call. 
>> 
>> 
> Let me see if I understood. So the particular illegal thing the IRDA
> stack is doing is the access of the control block in the middle of the
> driver transmit routine (via irda_get_next_speed() and friends). This
> information should be stored somewhere else. Exactly *where* to store
> it is the main problem to solve.
> 
> What is the proper way (if any) to store per-packet parameters (other
> than the payload itself) which are specific to a particular layer
> (IrDA in this case) and which are needed by drivers in order to work
> correctly? The control block gets overwritten by the time the driver
> proc (hard_start_xmit) is called, so this approach is now ruled out. I
> was thinking about storing a copy of the parameters (struct
> irda_skb_cb) as a header within the payload itself (skb->data[]), but
> I am not sure about whether this approach is a good design decision.
> I am open to suggestions on where to place the parameters.

Isn't this what the data that is skb_reserve'd at the beginning of skb's allocated by netdev_alloc_skb is for?  If you take an extra reference to the skb and/or use a destructor hook you should be good, right?

you should just be able to push ->data using skb_reserve(sizeof your private data) in the beginning of the skb, or is that a horrible idea Dave?

Re: Regression: Recent networking (qdisc?) patches break irda_get_next_speed()

[Stephen Hemminger]

On Thu, 23 Oct 2008 15:13:02 -0700
"Brandeburg, Jesse" <jesse.brandeburg@intel.com> wrote:

> Alex Villací­s Lasso wrote:
> <snip>
> 
> >> The SKB control block is not aliased.
> >> 
> 
> ...
> >> IRDA cannot depend upon the SKB control block not changing across
> >> the dev_queue_xmit() call. 
> >> 
> >> 
> > Let me see if I understood. So the particular illegal thing the IRDA
> > stack is doing is the access of the control block in the middle of the
> > driver transmit routine (via irda_get_next_speed() and friends). This
> > information should be stored somewhere else. Exactly *where* to store
> > it is the main problem to solve.
> > 
> > What is the proper way (if any) to store per-packet parameters (other
> > than the payload itself) which are specific to a particular layer
> > (IrDA in this case) and which are needed by drivers in order to work
> > correctly? The control block gets overwritten by the time the driver
> > proc (hard_start_xmit) is called, so this approach is now ruled out. I
> > was thinking about storing a copy of the parameters (struct
> > irda_skb_cb) as a header within the payload itself (skb->data[]), but
> > I am not sure about whether this approach is a good design decision.
> > I am open to suggestions on where to place the parameters.
> 
> Isn't this what the data that is skb_reserve'd at the beginning of skb's allocated by netdev_alloc_skb is for?  If you take an extra reference to the skb and/or use a destructor hook you should be good, right?
> 
> you should just be able to push ->data using skb_reserve(sizeof your private data) in the beginning of the skb, or is that a horrible idea Dave?

That space is reserved for a copy of the ethernet header when doing bridge/filtering.
Yes, its a stupid, undocumented hack. 

If irda needs additional protocol space, it could advertise a larger hardware header
size and use the additional space for hidden protocol info. 

Re: Regression: Recent networking (qdisc?) patches break irda_get_next_speed()

[David Miller]

From: "Brandeburg, Jesse" <jesse.brandeburg@intel.com>
Date: Thu, 23 Oct 2008 15:13:02 -0700

> Isn't this what the data that is skb_reserve'd at the beginning of
> skb's allocated by netdev_alloc_skb is for?  If you take an extra
> reference to the skb and/or use a destructor hook you should be
> good, right?
>
> you should just be able to push ->data using skb_reserve(sizeof your
> private data) in the beginning of the skb, or is that a horrible
> idea Dave?

This area of the SKB data is still volatile.

Let's say a packet scheduler classifier causes a TC action
to execute which pushes stuff there, it'll get overwritten.

Really, this idea won't work.