netdev - Re: [net-next v3 1/7] net: openvswitch: add datapath flow drop reason

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Tue, 8 Aug 2023 20:02:24 +0200
From: Ilya Maximets <i.maximets@....org>
To: Adrian Moreno <amorenoz@...hat.com>, netdev@...r.kernel.org
Cc: i.maximets@....org, aconole@...hat.com, eric@...ver.life,
 dev@...nvswitch.org
Subject: Re: [net-next v3 1/7] net: openvswitch: add datapath flow drop reason

On 8/7/23 18:45, Adrian Moreno wrote:
> Create a new drop reason subsystem for openvswitch and add the first
> drop reason to represent flow drops.
> 
> A flow drop happens when a flow has an empty action-set or there is no
> action that consumes the packet (output, userspace, recirc, etc).
> 
> Implementation-wise, most of these skb-consuming actions already call
> "consume_skb" internally and return directly from within the
> do_execute_actions() loop so with minimal changes we can assume that
> any skb that exits the loop normally is a packet drop.
> 
> Signed-off-by: Adrian Moreno <amorenoz@...hat.com>
> ---
>  include/net/dropreason.h   |  6 ++++++
>  net/openvswitch/actions.c  | 12 ++++++++++--
>  net/openvswitch/datapath.c | 16 ++++++++++++++++
>  net/openvswitch/drop.h     | 24 ++++++++++++++++++++++++
>  4 files changed, 56 insertions(+), 2 deletions(-)
>  create mode 100644 net/openvswitch/drop.h

<snip>

> diff --git a/net/openvswitch/drop.h b/net/openvswitch/drop.h
> new file mode 100644
> index 000000000000..cdd10629c6be
> --- /dev/null
> +++ b/net/openvswitch/drop.h
> @@ -0,0 +1,24 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * OpenvSwitch drop reason list.
> + */
> +
> +#ifndef OPENVSWITCH_DROP_H
> +#define OPENVSWITCH_DROP_H
> +#include <net/dropreason.h>
> +
> +#define OVS_DROP_REASONS(R)			\
> +	R(OVS_DROP_FLOW)		        \

Hi, Adrian.  Not a full review, just complaining about names. :)

The OVS_DROP_FLOW seems a bit confusing and unclear.  A "flow drop"
is also a strange term to use.  Maybe we can somehow express in the
name that this drop reason is used when there are no actions left
to execute?  e.g. OVS_DROP_NO_MORE_ACTIONS or OVS_DROP_LAST_ACTION
or OVS_DROP_END_OF_ACTION_LIST or something of that sort?  These may
seem long, but they are not longer than some other names introduced
later in the set.  What do yo think?

Best regards, Ilya Maximets.