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]

Message-ID: <a4dd0cf5-716a-feb9-5ba1-ec09636735a0@redhat.com>
Date: Wed, 9 Aug 2023 08:47:12 +0200
From: Adrian Moreno <amorenoz@...hat.com>
To: Ilya Maximets <i.maximets@....org>, netdev@...r.kernel.org
Cc: 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/8/23 20:02, Ilya Maximets wrote:
> 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?
> 

Hi Ilya,

Thanks for the suggestion. It looks reasonable. I did consider something similar 
but then it felt like having a bit of an "unexpected" or "involuntary" 
connotation. Given that there are other drop reasons that are involutary I 
wanted to somehow differentiate this one from the rest.

Semantically it'd mean something like: When a flow is deliberately installed 
with an empty action list it means "it" (the flow?) _wants_ to drop the packet, 
that's why I ended at that name.

OVS_DROP_LAST_ACTION seems to convey this intentionality as well. I'll can send 
another version changing all names.

Thanks.

-- 
Adrián Moreno