| Message ID | 1380209227.3165.176.camel@edumazet-glaptop |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On 09/26/2013 05:27 PM, Eric Dumazet wrote: > On Thu, 2013-09-26 at 16:09 +0200, Nikolay Aleksandrov wrote: >> Factor out the code that extracts the ports from skb_flow_dissect and >> add a new function skb_flow_get_ports which can be re-used. >> >> Suggested-by: Veaceslav Falico <vfalico@redhat.com> >> Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com> >> --- >> v2: new patch >> v3: fix a bug in skb_flow_dissect where thoff didn't have poff added by >> modifying thoff directly in skb_flow_get_ports as it's done anyway. >> Also add the necessary export symbol for skb_flow_get_ports. >> This seems like a good idea because there're other users that can re-use >> it later as well. > > Wait a minute.... existing code seems buggy. > > Daniel, any objection if I submit this fix ? > > (commit 8ed781668dd49b608f) > 1 question, I might be missing something but proto_ports_offset() gets the SPI with that 4 byte offset as is written in the comments, in every other case proto_ports_offset() is 0, so why would we want the SPI in the ->ports field ? And even then isn't it supposed to be 16 bits (2 bytes) and not 4, since we need to pass over "next header" (8 bits) and length (8 bits) ? Thanks, Nik -- 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 09/26/2013 05:40 PM, Nikolay Aleksandrov wrote: > On 09/26/2013 05:27 PM, Eric Dumazet wrote: >> On Thu, 2013-09-26 at 16:09 +0200, Nikolay Aleksandrov wrote: >>> Factor out the code that extracts the ports from skb_flow_dissect and >>> add a new function skb_flow_get_ports which can be re-used. >>> >>> Suggested-by: Veaceslav Falico <vfalico@redhat.com> >>> Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com> >>> --- >>> v2: new patch >>> v3: fix a bug in skb_flow_dissect where thoff didn't have poff added by >>> modifying thoff directly in skb_flow_get_ports as it's done anyway. >>> Also add the necessary export symbol for skb_flow_get_ports. >>> This seems like a good idea because there're other users that can re-use >>> it later as well. >> >> Wait a minute.... existing code seems buggy. >> >> Daniel, any objection if I submit this fix ? >> >> (commit 8ed781668dd49b608f) >> > 1 question, I might be missing something but proto_ports_offset() gets the SPI > with that 4 byte offset as is written in the comments, in every other case > proto_ports_offset() is 0, so why would we want the SPI in the ->ports field ? > And even then isn't it supposed to be 16 bits (2 bytes) and not 4, since we need > to pass over "next header" (8 bits) and length (8 bits) ? > Nevermind the second part, I forgot about the 16-bit 0 region :-) > Thanks, > Nik -- 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 Thu, 2013-09-26 at 17:40 +0200, Nikolay Aleksandrov wrote: > > > 1 question, I might be missing something but proto_ports_offset() gets the SPI > with that 4 byte offset as is written in the comments, in every other case > proto_ports_offset() is 0, so why would we want the SPI in the ->ports field ? > And even then isn't it supposed to be 16 bits (2 bytes) and not 4, since we need > to pass over "next header" (8 bits) and length (8 bits) ? struct ip_auth_hdr { __u8 nexthdr; __u8 hdrlen; /* This one is measured in 32 bit units! */ __be16 reserved; __be32 spi; __be32 seq_no; /* Sequence number */ __u8 auth_data[0]; /* Variable len but >=4. Mind the 64 bit alignment! */ }; offsetof(spi, struct ...) = 4 -- 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/core/flow_dissector.c b/net/core/flow_dissector.c index 1929af8..8d7d0dd 100644 --- a/net/core/flow_dissector.c +++ b/net/core/flow_dissector.c @@ -154,8 +154,8 @@ ipv6: if (poff >= 0) { __be32 *ports, _ports; - nhoff += poff; - ports = skb_header_pointer(skb, nhoff, sizeof(_ports), &_ports); + ports = skb_header_pointer(skb, nhoff + poff, + sizeof(_ports), &_ports); if (ports) flow->ports = *ports; }