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.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
> :)
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
> > > > 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
--
fbl
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
