| Message ID | 1275481538.14363.10.camel@bigi |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
From: jamal <hadi@cyberus.ca> Date: Wed, 02 Jun 2010 08:25:38 -0400 > --- a/net/sched/cls_u32.c > +++ b/net/sched/cls_u32.c > @@ -135,6 +135,9 @@ next_knode: > > for (i = n->sel.nkeys; i>0; i--, key++) { > > + int toff = key->off+(off2&key->offmask)- 4; > + if (unlikely(toff > skb->len)) > + /* bailout here - needs some thought */ > if ((*(__be32*)(ptr+key->off+(off2&key->offmask))^key->v I don't think it's that simple. You can't dereference from the skb->data linear area if your offset is beyond "skb->len - skb->data_len" (aka. skb_headlen()) since that's where the paged or fragmented portion starts. We really need to use skb_copy_bits() if we want to allow any offset into the SKB, and because of all the ways packets can be transformed and constructed we absolutely have to. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 2, 2010 at 8:45 PM, David Miller <davem@davemloft.net> wrote: > From: jamal <hadi@cyberus.ca> > Date: Wed, 02 Jun 2010 08:25:38 -0400 > >> --- a/net/sched/cls_u32.c >> +++ b/net/sched/cls_u32.c >> @@ -135,6 +135,9 @@ next_knode: >> >> for (i = n->sel.nkeys; i>0; i--, key++) { >> >> + int toff = key->off+(off2&key->offmask)- 4; >> + if (unlikely(toff > skb->len)) >> + /* bailout here - needs some thought */ >> if ((*(__be32*)(ptr+key->off+(off2&key->offmask))^key->v > > I don't think it's that simple. > > You can't dereference from the skb->data linear area if your offset is > beyond "skb->len - skb->data_len" (aka. skb_headlen()) since that's > where the paged or fragmented portion starts. > > We really need to use skb_copy_bits() if we want to allow > any offset into the SKB, and because of all the ways > packets can be transformed and constructed we absolutely > have to. > Maybe skb_header_pointer() is lighter.
From: Changli Gao <xiaosuo@gmail.com> Date: Wed, 2 Jun 2010 21:14:55 +0800 > On Wed, Jun 2, 2010 at 8:45 PM, David Miller <davem@davemloft.net> wrote: >> From: jamal <hadi@cyberus.ca> >> Date: Wed, 02 Jun 2010 08:25:38 -0400 >> >>> --- a/net/sched/cls_u32.c >>> +++ b/net/sched/cls_u32.c >>> @@ -135,6 +135,9 @@ next_knode: >>> >>> for (i = n->sel.nkeys; i>0; i--, key++) { >>> >>> + int toff = key->off+(off2&key->offmask)- 4; >>> + if (unlikely(toff > skb->len)) >>> + /* bailout here - needs some thought */ >>> if ((*(__be32*)(ptr+key->off+(off2&key->offmask))^key->v >> >> I don't think it's that simple. >> >> You can't dereference from the skb->data linear area if your offset is >> beyond "skb->len - skb->data_len" (aka. skb_headlen()) since that's >> where the paged or fragmented portion starts. >> >> We really need to use skb_copy_bits() if we want to allow >> any offset into the SKB, and because of all the ways >> packets can be transformed and constructed we absolutely >> have to. >> > > Maybe skb_header_pointer() is lighter. Yes, it should be. In fact, it's designed for this kind of situation and that's why it's used extensively in netfilter. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2010-06-02 at 21:14 +0800, Changli Gao wrote:
> Maybe skb_header_pointer() is lighter.
A little worse than skb_copy_bits(). In any case, this change is going
to hurt.
Dave, can we assume the upper layers(qdiscs in this case) are
responsible for any linearizing?
Changli, if you have time - can you also audit tcf_pedit()
since it follows TheLinuxWay(tm).
cheers,
jamal
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 2, 2010 at 9:36 PM, jamal <hadi@cyberus.ca> wrote: > On Wed, 2010-06-02 at 21:14 +0800, Changli Gao wrote: > > >> Maybe skb_header_pointer() is lighter. > > A little worse than skb_copy_bits(). In any case, this change is going > to hurt. Why? it is an inline function, and in most cases, there isn't any function call. > Dave, can we assume the upper layers(qdiscs in this case) are > responsible for any linearizing? > > Changli, if you have time - can you also audit tcf_pedit() > since it follows TheLinuxWay(tm). > Yea, pedit has the same issue. Besides this issue, they should use get_unaligned() instead.
From: jamal <hadi@cyberus.ca> Date: Wed, 02 Jun 2010 09:36:37 -0400 > On Wed, 2010-06-02 at 21:14 +0800, Changli Gao wrote: > > >> Maybe skb_header_pointer() is lighter. > > A little worse than skb_copy_bits(). In any case, this change is going > to hurt. Umm, Jamal what are you talking about? Using skb_header_pointer(), if the offset is in range, there is no change from today other than a comparison. If it is not in range, we use skb_copy_bits(). It's only for the case where we have to fetch the value from the fragmented part of the SKB. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2010-06-02 at 06:43 -0700, David Miller wrote: > From: jamal <hadi@cyberus.ca> > > > > A little worse than skb_copy_bits(). In any case, this change is going > > to hurt. > > Umm, Jamal what are you talking about? ;-> > Using skb_header_pointer(), if the offset is in range, there is no > change from today other than a comparison. Thats the part i glossed over - My eyes just saw "it calls skb_copy_bits()" ;-> > If it is not in range, we use skb_copy_bits(). It's only for the > case where we have to fetch the value from the fragmented part > of the SKB. cheers, jamal -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2010-06-02 at 21:43 +0800, Changli Gao wrote: > Yea, pedit has the same issue. Besides this issue, they should use > get_unaligned() instead. sounds reasonable - although they are probably different patches with the offset fix being more important. cheers, jamal -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c index 9627542..dde7a23 100644 --- a/net/sched/cls_u32.c +++ b/net/sched/cls_u32.c @@ -135,6 +135,9 @@ next_knode: for (i = n->sel.nkeys; i>0; i--, key++) { + int toff = key->off+(off2&key->offmask)- 4; + if (unlikely(toff > skb->len)) + /* bailout here - needs some thought */ if ((*(__be32*)(ptr+key->off+(off2&key->offmask))^key->v n = n->next; goto next_knode;