Hi Eelco, On Thu, Apr 15, 2021 at 10:07:24AM +0200, Eelco Chaudron wrote: > > > On 14 Apr 2021, at 17:50, David Marchand wrote: > > > Skipping further processing of invalid IP packets helps avoid crashes > > but it does not help to figure out if the malformed packets are still > > present on the network. > > > > Add coverage counters for IPv4 and IPv6 sanity checks so that we know > > there are some invalid packets. > > > > Dump such whole packets in debug mode. > > Looks good to me, some small nits below. > > > Signed-off-by: David Marchand <david.march...@redhat.com> > > --- > > lib/flow.c | 42 ++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 42 insertions(+) > > > > diff --git a/lib/flow.c b/lib/flow.c > > index 729d59b1b3..2b55244190 100644 > > --- a/lib/flow.c > > +++ b/lib/flow.c > > @@ -44,8 +44,15 @@ > > #include "openvswitch/nsh.h" > > #include "ovs-router.h" > > #include "lib/netdev-provider.h" > > +#include "openvswitch/vlog.h" > > + > > +VLOG_DEFINE_THIS_MODULE(flow); > > > > COVERAGE_DEFINE(flow_extract); > > +COVERAGE_DEFINE(ipv4_check_too_short); > > +COVERAGE_DEFINE(ipv4_check_length_error); > > +COVERAGE_DEFINE(ipv6_check_too_short); > > +COVERAGE_DEFINE(ipv6_check_length_error); > > The check keyword is a bit confusing to me, maybe something like > ipv4_pkt_too_short, etc.?
David may have a different idea, but to me this works to pinpoint the packet failure to ipv*_sanity_check(). Perhaps the name could be better. However, a generic name that can be used in more places would make it harder to pinpoint. > > COVERAGE_DEFINE(miniflow_malloc); > > > > /* U64 indices for segmented flow classification. */ > > @@ -645,17 +652,20 @@ ipv4_sanity_check(const struct ip_header *nh, > > size_t size, > > uint16_t tot_len; > > > > if (OVS_UNLIKELY(size < IP_HEADER_LEN)) { > > + COVERAGE_INC(ipv4_check_too_short); > > return false; > > } > > ip_len = IP_IHL(nh->ip_ihl_ver) * 4; > > > > if (OVS_UNLIKELY(ip_len < IP_HEADER_LEN || size < ip_len)) { > > + COVERAGE_INC(ipv4_check_length_error); > > return false; > > } > > > > tot_len = ntohs(nh->ip_tot_len); > > if (OVS_UNLIKELY(tot_len > size || ip_len > tot_len || > > size - tot_len > UINT16_MAX)) { > > + COVERAGE_INC(ipv4_check_length_error); > > return false; > > } > > > > @@ -686,21 +696,41 @@ ipv6_sanity_check(const struct > > ovs_16aligned_ip6_hdr *nh, size_t size) > > uint16_t plen; > > > > if (OVS_UNLIKELY(size < sizeof *nh)) { > > + COVERAGE_INC(ipv6_check_too_short); > > return false; > > } > > > > plen = ntohs(nh->ip6_plen); > > if (OVS_UNLIKELY(plen + IPV6_HEADER_LEN > size)) { > > + COVERAGE_INC(ipv6_check_length_error); > > return false; > > } > > > > if (OVS_UNLIKELY(size - (plen + IPV6_HEADER_LEN) > UINT16_MAX)) { > > + COVERAGE_INC(ipv6_check_length_error); > > return false; > > } > > > > return true; > > } > > > > +static void > > +dump_invalid_packet(struct dp_packet *packet, const char *reason) > > +{ > > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); > > + struct ds ds = DS_EMPTY_INITIALIZER; > > + size_t size; > > + > > + if (VLOG_DROP_DBG(&rl)) { > > + return; > > + } > > + size = dp_packet_size(packet); > > + ds_put_hex_dump(&ds, dp_packet_data(packet), size, 0, false); > > Are we sure we need to dump the entire packet, or are we ok with let’s say > the first 128 bytes? For normal packets yes, that would be the case. But the packet might be corrupted and this is only used when debugging is enabled, so having a full dump is useful. fbl > > > + VLOG_DBG("invalid packet for %s: port %"PRIu32", size > > %"PRIuSIZE"\n%s", > > Do we want to indent the next line a bit, so we know they are together, > i.e., “PRIuSIZE”\n %s” > > > + reason, packet->md.in_port.odp_port, size, ds_cstr(&ds)); > > + ds_destroy(&ds); > > +} > > + > > /* Initializes 'dst' from 'packet' and 'md', taking the packet type > > into > > * account. 'dst' must have enough space for FLOW_U64S * 8 bytes. > > * > > @@ -850,6 +880,9 @@ miniflow_extract(struct dp_packet *packet, struct > > miniflow *dst) > > uint16_t tot_len; > > > > if (OVS_UNLIKELY(!ipv4_sanity_check(nh, size, &ip_len, > > &tot_len))) { > > + if (OVS_UNLIKELY(VLOG_IS_DBG_ENABLED())) { > > + dump_invalid_packet(packet, "ipv4_sanity_check"); > > + } > > goto out; > > } > > dp_packet_set_l2_pad_size(packet, size - tot_len); > > @@ -880,6 +913,9 @@ miniflow_extract(struct dp_packet *packet, struct > > miniflow *dst) > > uint16_t plen; > > > > if (OVS_UNLIKELY(!ipv6_sanity_check(nh, size))) { > > + if (OVS_UNLIKELY(VLOG_IS_DBG_ENABLED())) { > > + dump_invalid_packet(packet, "ipv6_sanity_check"); > > + } > > goto out; > > } > > data_pull(&data, &size, sizeof *nh); > > @@ -1114,6 +1150,9 @@ parse_tcp_flags(struct dp_packet *packet) > > uint16_t tot_len; > > > > if (OVS_UNLIKELY(!ipv4_sanity_check(nh, size, &ip_len, > > &tot_len))) { > > + if (OVS_UNLIKELY(VLOG_IS_DBG_ENABLED())) { > > + dump_invalid_packet(packet, "ipv4_sanity_check"); > > + } > > return 0; > > } > > dp_packet_set_l2_pad_size(packet, size - tot_len); > > @@ -1127,6 +1166,9 @@ parse_tcp_flags(struct dp_packet *packet) > > uint16_t plen; > > > > if (OVS_UNLIKELY(!ipv6_sanity_check(nh, size))) { > > + if (OVS_UNLIKELY(VLOG_IS_DBG_ENABLED())) { > > + dump_invalid_packet(packet, "ipv6_sanity_check"); > > + } > > return 0; > > } > > data_pull(&data, &size, sizeof *nh); > > -- > > 2.23.0 > > > > _______________________________________________ > > dev mailing list > > d...@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev -- fbl _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev