| Message ID | 20210414155023.27266-1-david.marchand@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] flow: Count and dump invalid IP packets. | expand |
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@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.? > 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? > + 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@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
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@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@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
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.marchand@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 >>> dev@openvswitch.org >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > -- > fbl
On Thu, Apr 15, 2021 at 01:32:06PM +0200, Eelco Chaudron wrote: > > > 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.marchand@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 > :) Perhaps calling the stats like these helps: miniflow_extract_ipv*_len_error miniflow_extract_ipv*_too_short It still requires you to know OVS internals though. fbl > > 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 > > > > dev@openvswitch.org > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > > _______________________________________________ > > > dev mailing list > > > dev@openvswitch.org > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > -- > > fbl > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Thu, Apr 15, 2021 at 1:45 PM Flavio Leitner <fbl@sysclose.org> wrote: > > > > > +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 > > :) > > Perhaps calling the stats like these helps: > miniflow_extract_ipv*_len_error > miniflow_extract_ipv*_too_short > > It still requires you to know OVS internals though. I agree. Whatever the name, you either know the internals, or you will grep the documentation or the sources to get the full meaning. Making life easier for people familiar with OVS is a nice bonus. So adding flow and/or extract inside the name seems the best proposal so far.
On Thu, Apr 15, 2021 at 1:12 PM Flavio Leitner <fbl@sysclose.org> wrote: > > > +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. Anormal padding, some corruption... When something is wrong with a packet, I prefer a full dump than being sorry I only have partial info.
On 15 Apr 2021, at 15:25, David Marchand wrote: > On Thu, Apr 15, 2021 at 1:45 PM Flavio Leitner <fbl@sysclose.org> > wrote: >>>>>> +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 >>> :) >> >> Perhaps calling the stats like these helps: >> miniflow_extract_ipv*_len_error >> miniflow_extract_ipv*_too_short >> >> It still requires you to know OVS internals though. > > I agree. > Whatever the name, you either know the internals, or you will grep the > documentation or the sources to get the full meaning. > Making life easier for people familiar with OVS is a nice bonus. > So adding flow and/or extract inside the name seems the best proposal > so far. So you will send a new version with Flavio’s name change above? If so, you can add my ack :)
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); 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); + VLOG_DBG("invalid packet for %s: port %"PRIu32", size %"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);
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. Signed-off-by: David Marchand <david.marchand@redhat.com> --- lib/flow.c | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+)