From: Stephen Hemminger <stephen.hemminger@vyatta.com>
To: "Winkler, Tomas" <tomas.winkler@intel.com>,
Stephen Hemminger <shemminger@vyatta.com>,
Johannes Berg <johannes@sipsolutions.net>
Cc: "davem@davemloft.net" <davem@davemloft.net>,
"netdev@vger.kernel.org " <netdev@vger.kernel.org>,
"linux-wireless@vger.kernel.org "
<linux-wireless@vger.kernel.org>
Subject: RE: [PATCH net-2.6] bridge: fix br_multicast_ipv6_rcv for paged skbs
Date: Thu, 30 Dec 2010 15:06:16 -0800 [thread overview]
Message-ID: <9mk4bme8o97ir27xi8a9fbsd.1293750376929@email.android.com> (raw)
QWx0aG91Z2ggY29weSBpcyBzbG93ZXIgZm9yIGxhcmdlIHBhY2tldHMsIHRoaXMgaXMgYSBub24g
cGVyZm9ybWFuY2UgcGF0aC4gVGhlIGNvZGUgaW4gcXVlc3Rpb24gaXMgZm9yIGJyaWRnZWQgbXVs
dGljYXN0IElwdjYgSUNNUCBwYWNrZXRzLiBUaGlzIGNhc2UgaXMgc28gdW5jcml0aWNhbCBpdCBj
b3VsZCBiZSBkb25lIGluIEJBU0lDIGFuZCBubyBvbmUgY291bGQgcG9zc2libHkgY2FyZSEKCiJX
aW5rbGVyLCBUb21hcyIgPHRvbWFzLndpbmtsZXJAaW50ZWwuY29tPiB3cm90ZToKCj4KPgo+PiAt
LS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQo+PiBGcm9tOiBTdGVwaGVuIEhlbW1pbmdlciBbbWFp
bHRvOnNoZW1taW5nZXJAdnlhdHRhLmNvbV0KPj4gU2VudDogVGh1cnNkYXksIERlY2VtYmVyIDMw
LCAyMDEwIDk6MDYgUE0KPj4gVG86IEpvaGFubmVzIEJlcmcKPj4gQ2M6IFdpbmtsZXIsIFRvbWFz
OyBkYXZlbUBkYXZlbWxvZnQubmV0OyBuZXRkZXZAdmdlci5rZXJuZWwub3JnOyBsaW51eC0KPj4g
d2lyZWxlc3NAdmdlci5rZXJuZWwub3JnCj4+IFN1YmplY3Q6IFJlOiBbUEFUQ0ggbmV0LTIuNl0g
YnJpZGdlOiBmaXggYnJfbXVsdGljYXN0X2lwdjZfcmN2IGZvciBwYWdlZAo+PiBza2JzCj4+IAo+
PiBPbiBUaHUsIDMwIERlYyAyMDEwIDE5OjUyOjE0ICswMTAwCj4+IEpvaGFubmVzIEJlcmcgPGpv
aGFubmVzQHNpcHNvbHV0aW9ucy5uZXQ+IHdyb3RlOgo+PiAKPj4gPiBPbiBUaHUsIDIwMTAtMTIt
MzAgYXQgMTA6NDYgLTA4MDAsIFN0ZXBoZW4gSGVtbWluZ2VyIHdyb3RlOgo+PiA+Cj4+ID4gPiBU
aGlzIGRvZXNuJ3QgbG9vayBjb3JyZWN0LiBUaGUgY2FsY3VsYXRpb24gb2YgdGhlIG9mZnNldCBk
b2Vzbid0IGxvb2sKPj4gY29ycmVjdC4KPj4gPiA+IEp1c3QgZm9sbG93aW5nIHRoZSBza2JfY2xv
bmUoKSwgdGhlIHNrYl9wdWxsIHZhbHVlIGlzICJvZmZzZXQiLgo+PiA+ID4gQWxzbywgdGhlIG90
aGVyIGNoZWNrcyByZXR1cm4gLUVJTlZBTCBmb3IgaW5jb3JyZWN0bHkgZm9ybWVkIHBhY2tldC4K
Pj4gPiA+Cj4+ID4gPiAtLS0gYS9uZXQvYnJpZGdlL2JyX211bHRpY2FzdC5jCTIwMTAtMTItMzAg
MTA6Mjk6NTguNTc5NTEwNDg4IC0wODAwCj4+ID4gPiArKysgYi9uZXQvYnJpZGdlL2JyX211bHRp
Y2FzdC5jCTIwMTAtMTItMzAgMTA6NDM6MjcuMjczMzg2NjkxIC0wODAwCj4+ID4gPiBAQCAtMTQ2
NCw2ICsxNDY0LDkgQEAgc3RhdGljIGludCBicl9tdWx0aWNhc3RfaXB2Nl9yY3Yoc3RydWN0Cj4+
ID4gPiAgCWlmIChvZmZzZXQgPCAwIHx8IG5leHRoZHIgIT0gSVBQUk9UT19JQ01QVjYpCj4+ID4g
PiAgCQlyZXR1cm4gMDsKPj4gPiA+Cj4+ID4gPiArCWlmICghcHNrYl9tYXlfcHVsbChza2IsIG9m
ZnNldCkpCj4+ID4gPiArCQlyZXR1cm4gLUVJTlZBTDsKPj4gPiA+ICsKPj4gPiA+ICAJLyogT2th
eSwgd2UgZm91bmQgSUNNUHY2IGhlYWRlciAqLwo+PiA+ID4gIAlza2IyID0gc2tiX2Nsb25lKHNr
YiwgR0ZQX0FUT01JQyk7Cj4+ID4gPiAgCWlmICghc2tiMikKPj4gPgo+PiA+IFdvdWxkbid0IHRo
YXQgbWFrZSBtb3JlIHNlbnNlIGFmdGVyIHRoZSBjbG9uZSBhbnl3YXk/IEJ1dCBpZiB5b3UgbG9v
ayBhdAo+PiA+IG15IGVtYWlsLCB5b3UnbGwgZmluZCB0aGF0IHRoZXJlJ3MgcG90ZW50aWFsbHks
IGFuZCBjb25kaXRpb25hbGx5LCBtb3JlCj4+ID4gc3R1ZmYgdGhhdCB3aWxsIGJlIHJlYWQgZnJv
bSB0aGUgc2tiJ3MgaGVhZGVyLCB3aGljaCBoYXNuJ3QgbmVjZXNzYXJpbHkKPj4gPiBiZWVuIHB1
bGxlZCBpbiwgc28gSSB0aGluayB0aGlzIHN0aWxsIHdvbid0IGZpeCBhbGwgdGhlIGlzc3Vlcy4K
Pj4gPgo+PiA+IFNlZWluZyBob3cgdGhpcyBvbmx5IGFmZmVjdHMgc29tZSBJQ01QdjYgcGFja2V0
cywgbWF5YmUgd2Ugc2hvdWxkIGp1c3QKPj4gPiB1c2Ugc2tiX2NvcHkoKSBpbnN0ZWFkPwo+PiAK
Pj4gSXQgY29tZXMgb3V0IGNsZWFuZXIsIGFuZCB0aGUgY2hlY2sgY2FuIGJlIHNpbXBsaWZpZWQu
Cj4+IAo+PiAtLS0gYS9uZXQvYnJpZGdlL2JyX211bHRpY2FzdC5jCTIwMTAtMTItMzAgMTA6NDc6
MTIuMDMxNzMzODU1IC0wODAwCj4+ICsrKyBiL25ldC9icmlkZ2UvYnJfbXVsdGljYXN0LmMJMjAx
MC0xMi0zMCAxMTowMDoxMi4xMzU4MDEyNjYgLTA4MDAKPj4gQEAgLTE0NjUsMTkgKzE0NjUsMTkg
QEAgc3RhdGljIGludCBicl9tdWx0aWNhc3RfaXB2Nl9yY3Yoc3RydWN0Cj4+ICAJCXJldHVybiAw
Owo+PiAKPj4gIAkvKiBPa2F5LCB3ZSBmb3VuZCBJQ01QdjYgaGVhZGVyICovCj4+IC0Jc2tiMiA9
IHNrYl9jbG9uZShza2IsIEdGUF9BVE9NSUMpOwo+PiArCXNrYjIgPSBza2JfY29weShza2IsIEdG
UF9BVE9NSUMpOwo+PiAgCWlmICghc2tiMikKPj4gIAkJcmV0dXJuIC1FTk9NRU07Cj4+IAo+PiAr
CWVyciA9IC1FSU5WQUw7Cj4+ICsJaWYgKHNrYjItPmxlbiA8IG9mZnNldCArIHNpemVvZigqaWNt
cDZoKSkKPj4gKwkJZ290byBvdXQ7Cj4+ICsKPj4gIAlsZW4gLT0gb2Zmc2V0IC0gc2tiX25ldHdv
cmtfb2Zmc2V0KHNrYjIpOwo+PiAKPj4gIAlfX3NrYl9wdWxsKHNrYjIsIG9mZnNldCk7Cj4+ICAJ
c2tiX3Jlc2V0X3RyYW5zcG9ydF9oZWFkZXIoc2tiMik7Cj4+IAo+PiAtCWVyciA9IC1FSU5WQUw7
Cj4+IC0JaWYgKCFwc2tiX21heV9wdWxsKHNrYjIsIHNpemVvZigqaWNtcDZoKSkpCj4+IC0JCWdv
dG8gb3V0Owo+PiAtCj4+ICAJaWNtcDZoID0gaWNtcDZfaGRyKHNrYjIpOwo+PiAKPj4gIAlzd2l0
Y2ggKGljbXA2aC0+aWNtcDZfdHlwZSkgewo+PiAKPj4KPlNvcnJ5IGZvciBkdW1wIHF1ZXN0aW9u
IGJ1dCBpc24ndCB0aGVyZSBwZXJmb3JtYW5jZSBwZW5hbHR5IG9uIHVzaW5nIHNrYl9jb3B5IHZz
LiBza2JfY2xvbmU/Cj4KPkFueWhvdyBCZWxvdyBpcyBhIGNvZGUgc25pcHBldCBmcm9tIGlwNl9p
bnB1dC5jIHNvIHlvdSBwcm9iYWJseSB3b3VsZCB3YW50IHRvIGZpeCBpdCBhbGwgb3Zlci4gCj5C
VFcgb2Zmc2V0IGFuZCB0aGUgcG9pbnRlciBhcml0aG1ldGljIHJlYWxseSBnaXZlcyB0aGUgc2Ft
ZSBudW1iZXIgKzEsIEknbSBub3Qgc3VybHkgd2h5IHRoZSBvcmlnaW5hbCBhdXRob3Igd291bGQg
dGhvdWdodCBpdCBiZSBzYWZlciB0aGFuIGp1c3QgdXNpbmcgb2Zmc2V0Lgo+Cj4JCQkJCW9mZnNl
dCA9IGlwdjZfc2tpcF9leHRoZHIoc2tiLCBzaXplb2YoKmhkciksCj4gICAgICAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgJm5leHRoZHIpOwo+ICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICBpZiAob2Zmc2V0IDwgMCkKPiAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICBnb3RvIG91dDsKPiAKPiAgICAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgaWYgKG5leHRoZHIgIT0gSVBQUk9UT19JQ01QVjYpCj4gICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgZ290byBvdXQ7Cj4gCj4gICAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgIGlmICghcHNrYl9tYXlfcHVsbChza2IsIChza2JfbmV0d29y
a19oZWFkZXIoc2tiKSArCj4gICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICBvZmZzZXQgKyAxIC0gc2tiLT5kYXRhKSkpCj4gICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgZ290byBvdXQ7Cj4gCj4gICAgICAgICAgICAgICAgICAgICAg
ICAgICAgICAgIGljbXA2ID0gKHN0cnVjdCBpY21wNmhkciAqKShza2JfbmV0d29ya19oZWFkZXIo
c2tiKSArIG9mZnNldCk7Cj4KPgo+Cj5UaGFua3MKPlRvbWFzCj4KPgo+LS0tLS0tLS0tLS0tLS0t
LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tCj5J
bnRlbCBJc3JhZWwgKDc0KSBMaW1pdGVkCj4KPlRoaXMgZS1tYWlsIGFuZCBhbnkgYXR0YWNobWVu
dHMgbWF5IGNvbnRhaW4gY29uZmlkZW50aWFsIG1hdGVyaWFsIGZvcgo+dGhlIHNvbGUgdXNlIG9m
IHRoZSBpbnRlbmRlZCByZWNpcGllbnQocykuIEFueSByZXZpZXcgb3IgZGlzdHJpYnV0aW9uCj5i
eSBvdGhlcnMgaXMgc3RyaWN0bHkgcHJvaGliaXRlZC4gSWYgeW91IGFyZSBub3QgdGhlIGludGVu
ZGVkCj5yZWNpcGllbnQsIHBsZWFzZSBjb250YWN0IHRoZSBzZW5kZXIgYW5kIGRlbGV0ZSBhbGwg
Y29waWVzLgo+Cg==
WARNING: multiple messages have this Message-ID (diff)
From: Stephen Hemminger <stephen.hemminger-ZtmgI6mnKB3QT0dZR+AlfA@public.gmane.org>
To: "Winkler,
Tomas" <tomas.winkler-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>,
Stephen Hemminger
<shemminger-ZtmgI6mnKB3QT0dZR+AlfA@public.gmane.org>,
Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
Cc: "davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org"
<davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>,
"netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org "
<netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
"linux-wireless-u79uwXL29TY76Z2rM5mHXA@public.gmane.org "
<linux-wireless-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Subject: RE: [PATCH net-2.6] bridge: fix br_multicast_ipv6_rcv for paged skbs
Date: Thu, 30 Dec 2010 15:06:16 -0800 [thread overview]
Message-ID: <9mk4bme8o97ir27xi8a9fbsd.1293750376929@email.android.com> (raw)
Although copy is slower for large packets, this is a non performance path. The code in question is for bridged multicast Ipv6 ICMP packets. This case is so uncritical it could be done in BASIC and no one could possibly care!
"Winkler, Tomas" <tomas.winkler@intel.com> wrote:
>
>
>> -----Original Message-----
>> From: Stephen Hemminger [mailto:shemminger@vyatta.com]
>> Sent: Thursday, December 30, 2010 9:06 PM
>> To: Johannes Berg
>> Cc: Winkler, Tomas; davem@davemloft.net; netdev@vger.kernel.org; linux-
>> wireless@vger.kernel.org
>> Subject: Re: [PATCH net-2.6] bridge: fix br_multicast_ipv6_rcv for paged
>> skbs
>>
>> On Thu, 30 Dec 2010 19:52:14 +0100
>> Johannes Berg <johannes@sipsolutions.net> wrote:
>>
>> > On Thu, 2010-12-30 at 10:46 -0800, Stephen Hemminger wrote:
>> >
>> > > This doesn't look correct. The calculation of the offset doesn't look
>> correct.
>> > > Just following the skb_clone(), the skb_pull value is "offset".
>> > > Also, the other checks return -EINVAL for incorrectly formed packet.
>> > >
>> > > --- a/net/bridge/br_multicast.c 2010-12-30 10:29:58.579510488 -0800
>> > > +++ b/net/bridge/br_multicast.c 2010-12-30 10:43:27.273386691 -0800
>> > > @@ -1464,6 +1464,9 @@ static int br_multicast_ipv6_rcv(struct
>> > > if (offset < 0 || nexthdr != IPPROTO_ICMPV6)
>> > > return 0;
>> > >
>> > > + if (!pskb_may_pull(skb, offset))
>> > > + return -EINVAL;
>> > > +
>> > > /* Okay, we found ICMPv6 header */
>> > > skb2 = skb_clone(skb, GFP_ATOMIC);
>> > > if (!skb2)
>> >
>> > Wouldn't that make more sense after the clone anyway? But if you look at
>> > my email, you'll find that there's potentially, and conditionally, more
>> > stuff that will be read from the skb's header, which hasn't necessarily
>> > been pulled in, so I think this still won't fix all the issues.
>> >
>> > Seeing how this only affects some ICMPv6 packets, maybe we should just
>> > use skb_copy() instead?
>>
>> It comes out cleaner, and the check can be simplified.
>>
>> --- a/net/bridge/br_multicast.c 2010-12-30 10:47:12.031733855 -0800
>> +++ b/net/bridge/br_multicast.c 2010-12-30 11:00:12.135801266 -0800
>> @@ -1465,19 +1465,19 @@ static int br_multicast_ipv6_rcv(struct
>> return 0;
>>
>> /* Okay, we found ICMPv6 header */
>> - skb2 = skb_clone(skb, GFP_ATOMIC);
>> + skb2 = skb_copy(skb, GFP_ATOMIC);
>> if (!skb2)
>> return -ENOMEM;
>>
>> + err = -EINVAL;
>> + if (skb2->len < offset + sizeof(*icmp6h))
>> + goto out;
>> +
>> len -= offset - skb_network_offset(skb2);
>>
>> __skb_pull(skb2, offset);
>> skb_reset_transport_header(skb2);
>>
>> - err = -EINVAL;
>> - if (!pskb_may_pull(skb2, sizeof(*icmp6h)))
>> - goto out;
>> -
>> icmp6h = icmp6_hdr(skb2);
>>
>> switch (icmp6h->icmp6_type) {
>>
>>
>Sorry for dump question but isn't there performance penalty on using skb_copy vs. skb_clone?
>
>Anyhow Below is a code snippet from ip6_input.c so you probably would want to fix it all over.
>BTW offset and the pointer arithmetic really gives the same number +1, I'm not surly why the original author would thought it be safer than just using offset.
>
> offset = ipv6_skip_exthdr(skb, sizeof(*hdr),
> &nexthdr);
> if (offset < 0)
> goto out;
>
> if (nexthdr != IPPROTO_ICMPV6)
> goto out;
>
> if (!pskb_may_pull(skb, (skb_network_header(skb) +
> offset + 1 - skb->data)))
> goto out;
>
> icmp6 = (struct icmp6hdr *)(skb_network_header(skb) + offset);
>
>
>
>Thanks
>Tomas
>
>
>---------------------------------------------------------------------
>Intel Israel (74) Limited
>
>This e-mail and any attachments may contain confidential material for
>the sole use of the intended recipient(s). Any review or distribution
>by others is strictly prohibited. If you are not the intended
>recipient, please contact the sender and delete all copies.
>
next reply other threads:[~2010-12-30 23:06 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-12-30 23:06 Stephen Hemminger [this message]
2010-12-30 23:06 ` [PATCH net-2.6] bridge: fix br_multicast_ipv6_rcv for paged skbs Stephen Hemminger
2010-12-30 23:29 ` Winkler, Tomas
2010-12-30 23:29 ` Winkler, Tomas
2010-12-31 10:18 ` Johannes Berg
2010-12-31 10:18 ` Johannes Berg
2010-12-31 20:45 ` David Miller
2010-12-31 21:16 ` Winkler, Tomas
-- strict thread matches above, loose matches on Subject: below --
2010-12-29 16:12 BUG: while bridging Ethernet and wireless device: Tomas Winkler
2010-12-30 11:32 ` [PATCH net-2.6] bridge: fix br_multicast_ipv6_rcv for paged skbs Tomas Winkler
2010-12-30 18:46 ` Stephen Hemminger
2010-12-30 18:52 ` Johannes Berg
2010-12-30 18:52 ` Johannes Berg
2010-12-30 19:06 ` Stephen Hemminger
2010-12-30 19:06 ` Stephen Hemminger
2010-12-30 21:00 ` Winkler, Tomas
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9mk4bme8o97ir27xi8a9fbsd.1293750376929@email.android.com \
--to=stephen.hemminger@vyatta.com \
--cc=davem@davemloft.net \
--cc=johannes@sipsolutions.net \
--cc=linux-wireless@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=shemminger@vyatta.com \
--cc=tomas.winkler@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.