netdev - Re: [ovs-dev] [PATCH net-next 2/2] net: openvswitch: add drop action

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: <f7tpm4ych1b.fsf@redhat.com>
Date: Tue, 11 Jul 2023 16:46:56 -0400
From: Aaron Conole <aconole@...hat.com>
To: Eric Garver <eric@...ver.life>
Cc: Ilya Maximets <i.maximets@....org>,  Jakub Kicinski <kuba@...nel.org>,
  netdev@...r.kernel.org,  dev@...nvswitch.org,  Paolo Abeni
 <pabeni@...hat.com>,  Eric Dumazet <edumazet@...gle.com>,  "David S.
 Miller" <davem@...emloft.net>,  Adrian Moreno <amorenoz@...hat.com>,
  Eelco Chaudron <echaudro@...hat.com>
Subject: Re: [ovs-dev] [PATCH net-next 2/2] net: openvswitch: add drop action

Eric Garver <eric@...ver.life> writes:

> On Mon, Jul 10, 2023 at 06:51:19PM +0200, Ilya Maximets wrote:
>> On 7/8/23 00:06, Jakub Kicinski wrote:
>> > On Fri, 7 Jul 2023 18:04:36 +0200 Ilya Maximets wrote:
>> >>>> That already exists, right? Johannes added it in the last release for WiFi.  
>> >>>
>> >>> I'm not sure.  The SKB_DROP_REASON_SUBSYS_MAC80211_UNUSABLE behaves similarly
>> >>> to that on a surface.  However, looking closer, any value that can be passed
>> >>> into ieee80211_rx_handlers_result() and ends up in the kfree_skb_reason() is
>> >>> kind of defined in net/mac80211/drop.h, unless I'm missing something (very
>> >>> possible, because I don't really know wifi code).
>> >>>
>> >>> The difference, I guess, is that for openvswitch values will be provided by
>> >>> the userpsace application via netlink interface.  It'll be just a number not
>> >>> defined anywhere in the kernel.  Only the subsystem itself will be defined
>> >>> in order to occupy the range.  Garbage in, same garbage out, from the kernel's
>> >>> perspective.  
>> >>
>> >> To be clear, I think, not defining them in this particular case is better.
>> >> Definition of every reason that userspace can come up with will add extra
>> >> uAPI maintenance cost/issues with no practical benefits.  Values are not
>> >> going to be used for anything outside reporting a drop reason and subsystem
>> >> offset is not part of uAPI anyway.
>> > 
>> > Ah, I see. No, please don't stuff user space defined values into 
>> > the drop reason. The reasons are for debugging the kernel stack 
>> > itself. IOW it'd be abuse not reuse.
>> 
>> Makes sense.  I wasn't sure that's a good solution from a kernel perspective
>> either.  It's better than defining all these reasons, IMO, but it's not good
>> enough to be considered acceptable, I agree.
>> 
>> How about we define just 2 reasons, e.g. OVS_DROP_REASON_EXPLICIT_ACTION and
>> OVS_DROP_REASON_EXPLICIT_ACTION_WITH_ERROR (exact names can be different) ?
>> One for an explicit drop action with a zero argument and one for an explicit
>> drop with non-zero argument.
>> 
>> The exact reason for the error can be retrieved by other means, i.e by looking
>> at the datapath flow dump or OVS logs/traces.
>> 
>> This way we can give a user who is catching packet drop traces a signal that
>> there was something wrong with an OVS flow and they can look up exact details
>> from the userspace / flow dump.
>> 
>> The point being, most of the flows will have a zero as a drop action argument,
>> i.e. a regular explicit packet drop.  It will be hard to figure out which flow
>> exactly we're hitting without looking at the full flow dump.  And if the value
>> is non-zero, then it should be immediately obvious which flow is to blame from
>> the dump, as we should not have a lot of such flows.
>> 
>> This would still allow us to avoid a maintenance burden of defining every case,
>> which are fairly meaningless for the kernel itself, while having 99% of the
>> information we may need.
>> 
>> Jakub, do you think this will be acceptable?
>> 
>> Eric, Adrian, Aaron, do you see any problems with such implementation?
>
> I see no problems. I'm content with this approach.

+1

>> P.S. There is a plan to add more drop reasons for other places in openvswitch
>>      module to catch more regular types of drops like memory issues or upcall
>>      failures.  So, the drop reason subsystem can be extended later.
>>      The explicit drop action is a bit of an odd case here.
>> 
>> Best regards, Ilya Maximets.
>>