Re: ip_wccp.c for WCCP v2 question

Looking at net/ipv4/ipip.c as a guide, what Joe has done looks
reasonable. Can you send
me a dump of an oops (using the ksymoops tool) Joe?

Roger.

Henrik Nordstrom wrote:

> Exacly, but I do not think skb_pull is very happy if you try to pull
> more data than there is.. hmm.. after reading the source I realize that
> this is not exacly the case but almost. skb_pull can be used with any
> size, regardless of the amount actually received, BUT if a too large
> length is used then it returns NULL and does nothing.
>
> One other similar bug:
>
> The added if statement might look outside the received packet if an
> "empty" WCCP packet is received.
>
> Also, I am a bit unsure if the use of skb->h and skb->nh is correct. I
> think some of these are supposed to be skb->data. The skb_pull "magic"
> (skb->h.raw - skb->data) does not make much sense given that skb_pull
> increases skb->data with the given offset.. but potential problem is in
> the original ip_wccp.c as well.. unless ofcourse skb->data does not
> point to the GRE header when this is called..
>
> I think it should read something like this:
>
> if (skb->len < WCCP_GRE_LEN)
> goto drop;
>
> gre_hdr = (unsigned long *)skb->data;
> if (*gre_hdr != htonl(WCCP_PROTOCOL_TYPE))
> goto drop;
>
> /* is MAC header assignment really correct at this layer? It
> * feels quite malplaced... and looks odd... */
> skb->mac.raw = skb->nh.raw;
>
> /* Decapsulate the packet.
> * WCCP2 puts an extra 4 octets into the header, but uses the
> * same encapsulation type; if it looks as if the first octet
> * of the packet isn't the beginning of an IPv4 header, assume
> * it's WCCP2.
> */
> if ((skb->len >= WCCP_GRE_LEN + WCCP2_GRE_EXTRA) &&
> ((*(skb->data+WCCP_GRE_LEN) & 0xF0 )!= 0x40)) {
> /* WCCP2 */
> skb->nh.raw = skb_pull(skb, WCCP_GRE_LEN + WCCP2_GRE_EXTRA);
> } else {
> /* WCCP1 */
> skb->nh.raw = skb_pull(skb, WCCP_GRE_LEN);
> }
>
> if (!skb->nh.raw || skb->len <= 0)
> goto drop;
>
> memset(....
>
>
> The important changes besides the ->nh/h/data changes is
> 1. Added length check to make sure there is a WCCP GRE header
> 2. Added a length check before looking for "WCCP2 magic"
> 3. Verify the result of skb_pull
>
>
> Dropping packets you do not understand should not be a harm, but I am
> not 100% sure the method used here is fully correct.
>
> Hmm... need to read a bit about Linux kernel network programming I
> think. Too many loose ends on skb processing here...
>
> /Henrik
>
>
> Joe Cooper wrote:
>
>> Oh, yeah... As for the negative size possibility. Maybe I'm misreading
>> it, but aren't they being dropped if there is a size problem (the two
>> 'goto drop' bits)?
>>
>> Is there harm in dropping them (as has always been done in the module)?
>> I certainly don't have the knowledge to argue one way or the other. I
>> know just enough to figure out what the code is doing...not enough to
>> know what it /should/ do.
>>
>> Thanks again for your input.
>>
>> Henrik Nordstrom wrote:
>>
>>> I am not yet familar with how Linux skb processing, but:
>>>
>>> One thing I notice is that you do not verify the size of the GRE packets
>>> before trying to grab the encapsulated one, so if the router sends you a
>>> bad WCCP packet (no encapsulated packet) then oops a negative packet
>>> size... Bbad WCCP packets has been seen from certain IOS versions in
>>> certain path configurations.
>>>
>>> and why is the module cleanup code removed?
>>>
>>> /Henrik
>>>
>>> Joe Cooper wrote:
>>>
>>>> Hi folks,
>>>>
>>>> We've got WCCP v2 working with Squid, thanks to some fast coding by
>>>> Kevin Wormington.
>>>>
>>>> Would someone (hopefully with more Linux kernel hacking knowledge than
>>>> I) take a look at this ip_wccp.c module:
>>>>
>>>> http://www.swelltech.com/pengies/joe/patches/ip_wccp.c
>>>>
>>>> It's only a couple of lines bigger than the original ip_wccp.c for v1 of
>>>> WCCP, where it checks for the extra 4 octets of the v2 GRE header and
>>>> then skips them for the decapsulation...but something is causing an oops
>>>> on our test box. We're not real sure exactly what is going on, but this
>>>> seems as likely a candidate as any, since the box doesn't crash until it
>>>> starts seeing WCCP traffic. Unfortunately...because Kevin's router only
>>>> has WCCP v2 and my router only has v1, we can't do any cross-testing.
>>>> And the Squid in testing has not yet been made both v2 and v1 aware (it
>>>> can currently do v2 just fine...but v1 is broken, I'll be fixing that
>>>> over the next few days).
>>>>
>>>> I'd welcome some comments from you guys, if you see anything problematic
>>>> in the module...
>>>> --
>>>> Joe Cooper <joe@swelltech.com>
>>>> Affordable Web Caching Proxy Appliances
>>>> http://www.swelltech.com
>>>
>> --
>>
>> --
>> Joe Cooper <joe@swelltech.com>
>> Affordable Web Caching Proxy Appliances
>> http://www.swelltech.com
>

-- 
-------------------------------------------------------------
Roger Venning	\ Do not go gentle into that good night
Melbourne        \ Rage, rage against the dying of the light.
Australia <r.venning@bipond.com>                 Dylan Thomas