[ovs-dev] [PATCH] flow: Count and dump invalid IP packets.
Flavio Leitner
fbl at sysclose.org
Thu Apr 15 11:12:09 UTC 2021
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.marchand at 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
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
--
fbl
More information about the dev
mailing list