On 15 Apr 2021, at 13:12, Flavio Leitner wrote:
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.
Guess you have to do something like grep “COVERAGE_DEFINE(<name>)”
anyway to figure out what it does due to lack of documantion on any
coverage counter :)
Maybe Ilya has some preference?
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.
Ack
+ 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
