Thanks for doing this Vishal ! Except for minor patch formatting issues (inline), this is fine and also tests fine.
On Fri, Sep 27, 2019 at 10:54 PM Vishal Deep Ajmera < [email protected]> wrote: > From: Darrell Ball <[email protected]> > > The ICMPv4 error data L4 length check was found to be too strict for TCP, > expecting a minimum of 20 rather than 8 bytes. This worked by > hapenstance for other inner protocols. The approach is to explicitly > handle the ICMPv4 error data L4 length check and to do this for all > supported inner protocols in the same way. Making the code common > between protocols also allows the existing ICMPv4 related UDP tests to > cover TCP and ICMP inner protocol cases. > Note that ICMPv6 does not have an 8 byte limit for error L4 data. > > Fixes: a489b16854b5 ("conntrack: New userspace connection tracker.") > CC: Daniele Di Proietto <[email protected]> > Reported-at: > https://mail.openvswitch.org/pipermail/ovs-dev/2019-August/361949.html > Reported-by: Vishal Deep Ajmera <[email protected]> > Signed-off-by: Vishal Deep Ajmera <[email protected]> > Co-authored-by: Vishal Deep Ajmera <[email protected]> > Signed-off-by: Darrell Ball <[email protected]> > > Signed-off-by: Ben Pfaff <[email protected]> > remove extra signoff when submitting the patch > (cherry picked from commit 6c2a93064afe8d812e4506880d1fd8f96108f92a) > > Conflicts: > lib/conntrack.c > move part starting from 'cherry' below the '---', although this info is not really needed > --- > lib/conntrack.c | 35 ++++++++++++++++++++--------------- > lib/packets.h | 3 +++ > 2 files changed, 23 insertions(+), 15 deletions(-) > > diff --git a/lib/conntrack.c b/lib/conntrack.c > index 8abaf7e..d59083e 100644 > --- a/lib/conntrack.c > +++ b/lib/conntrack.c > @@ -664,11 +664,12 @@ check_l4_icmp6(const struct conn_key *key, const > void *data, size_t size, > } > > static inline bool > -extract_l4_tcp(struct conn_key *key, const void *data, size_t size) > +extract_l4_tcp(struct conn_key *key, const void *data, size_t size, > + size_t *chk_len) > { > const struct tcp_header *tcp = data; > > - if (OVS_UNLIKELY(size < TCP_HEADER_LEN)) { > + if (OVS_UNLIKELY(size < (chk_len ? *chk_len : TCP_HEADER_LEN))) { > return false; > } > > @@ -680,11 +681,12 @@ extract_l4_tcp(struct conn_key *key, const void > *data, size_t size) > } > > static inline bool > -extract_l4_udp(struct conn_key *key, const void *data, size_t size) > +extract_l4_udp(struct conn_key *key, const void *data, size_t size, > + size_t *chk_len) > { > const struct udp_header *udp = data; > > - if (OVS_UNLIKELY(size < UDP_HEADER_LEN)) { > + if (OVS_UNLIKELY(size < (chk_len ? *chk_len : UDP_HEADER_LEN))) { > return false; > } > > @@ -696,7 +698,8 @@ extract_l4_udp(struct conn_key *key, const void *data, > size_t size) > } > > static inline bool extract_l4(struct conn_key *key, const void *data, > - size_t size, bool *related, const void *l3); > + size_t size, bool *related, const void *l3, > + size_t *chk_len); > > static uint8_t > reverse_icmp_type(uint8_t type) > @@ -728,11 +731,11 @@ reverse_icmp_type(uint8_t type) > * possible */ > static inline int > extract_l4_icmp(struct conn_key *key, const void *data, size_t size, > - bool *related) > + bool *related, size_t *chk_len) > { > const struct icmp_header *icmp = data; > > - if (OVS_UNLIKELY(size < ICMP_HEADER_LEN)) { > + if (OVS_UNLIKELY(size < (chk_len ? *chk_len : ICMP_HEADER_LEN))) { > return false; > } > > @@ -783,8 +786,9 @@ extract_l4_icmp(struct conn_key *key, const void > *data, size_t size, > key->src = inner_key.src; > key->dst = inner_key.dst; > key->nw_proto = inner_key.nw_proto; > + size_t check_len = ICMP_ERROR_DATA_L4_LEN; > > - ok = extract_l4(key, l4, tail - l4, NULL, l3); > + ok = extract_l4(key, l4, tail - l4, NULL, l3, &check_len); > if (ok) { > conn_key_reverse(key); > *related = true; > @@ -872,7 +876,7 @@ extract_l4_icmp6(struct conn_key *key, const void > *data, size_t size, > key->dst = inner_key.dst; > key->nw_proto = inner_key.nw_proto; > > - ok = extract_l4(key, l4, tail - l4, NULL, l3); > + ok = extract_l4(key, l4, tail - l4, NULL, l3, NULL); > if (ok) { > conn_key_reverse(key); > *related = true; > @@ -897,21 +901,22 @@ extract_l4_icmp6(struct conn_key *key, const void > *data, size_t size, > * an ICMP or ICMP6 header. * > * > * If 'related' is NULL, it means that we're already parsing a header > nested > - * in an ICMP error. In this case, we skip checksum and length > validation. */ > + * in an ICMP error. In this case, we skip the checksum and some length > + * validations. */ > static inline bool > extract_l4(struct conn_key *key, const void *data, size_t size, bool > *related, > - const void *l3) > + const void *l3, size_t *chk_len) > { > if (key->nw_proto == IPPROTO_TCP) { > return (!related || check_l4_tcp(key, data, size, l3)) > - && extract_l4_tcp(key, data, size); > + && extract_l4_tcp(key, data, size, chk_len); > } else if (key->nw_proto == IPPROTO_UDP) { > return (!related || check_l4_udp(key, data, size, l3)) > - && extract_l4_udp(key, data, size); > + && extract_l4_udp(key, data, size, chk_len); > } else if (key->dl_type == htons(ETH_TYPE_IP) > && key->nw_proto == IPPROTO_ICMP) { > return (!related || check_l4_icmp(data, size)) > - && extract_l4_icmp(key, data, size, related); > + && extract_l4_icmp(key, data, size, related, chk_len); > } else if (key->dl_type == htons(ETH_TYPE_IPV6) > && key->nw_proto == IPPROTO_ICMPV6) { > return (!related || check_l4_icmp6(key, data, size, l3)) > @@ -982,7 +987,7 @@ conn_key_extract(struct conntrack *ct, struct > dp_packet *pkt, ovs_be16 dl_type, > > if (ok) { > if (extract_l4(&ctx->key, l4, dp_packet_l4_size(pkt), > - &ctx->related, l3)) { > + &ctx->related, l3, NULL)) { > ctx->hash = conn_key_hash(&ctx->key, ct->hash_basis); > return true; > } > diff --git a/lib/packets.h b/lib/packets.h > index 0705576..adfa86c 100644 > --- a/lib/packets.h > +++ b/lib/packets.h > @@ -656,6 +656,9 @@ struct icmp_header { > }; > BUILD_ASSERT_DECL(ICMP_HEADER_LEN == sizeof(struct icmp_header)); > > +/* ICMPV4 */ > +#define ICMP_ERROR_DATA_L4_LEN 8 > + > #define IGMP_HEADER_LEN 8 > struct igmp_header { > uint8_t igmp_type; > -- > 1.9.1 > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
