[ovs-dev] [PATCH] flow: Count and dump invalid IP packets.
Eelco Chaudron
echaudro at redhat.com
Thu Apr 15 11:32:06 UTC 2021
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 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.
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 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