netdev - Re: [patch net-next 10/13] openvswitch: add support for datapath hardware offload

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: <20140904124837.GI1867@nanopsycho.lan>
Date:	Thu, 4 Sep 2014 14:48:37 +0200
From:	Jiri Pirko <jiri@...nulli.us>
To:	John Fastabend <john.fastabend@...il.com>
Cc:	netdev@...r.kernel.org, davem@...emloft.net, nhorman@...driver.com,
	andy@...yhouse.net, tgraf@...g.ch, dborkman@...hat.com,
	ogerlitz@...lanox.com, jesse@...ira.com, pshelar@...ira.com,
	azhou@...ira.com, ben@...adent.org.uk, stephen@...workplumber.org,
	jeffrey.t.kirsher@...el.com, vyasevic@...hat.com,
	xiyou.wangcong@...il.com, john.r.fastabend@...el.com,
	edumazet@...gle.com, jhs@...atatu.com, sfeldma@...ulusnetworks.com,
	f.fainelli@...il.com, roopa@...ulusnetworks.com,
	linville@...driver.com, dev@...nvswitch.org, jasowang@...hat.com,
	ebiederm@...ssion.com, nicolas.dichtel@...nd.com,
	ryazanov.s.a@...il.com, buytenh@...tstofly.org,
	aviadr@...lanox.com, nbd@...nwrt.org, alexei.starovoitov@...il.com,
	Neil.Jerram@...aswitch.com, ronye@...lanox.com
Subject: Re: [patch net-next 10/13] openvswitch: add support for datapath
 hardware offload

Wed, Sep 03, 2014 at 06:37:08PM CEST, john.fastabend@...il.com wrote:
>On 09/03/2014 02:24 AM, Jiri Pirko wrote:
>>Benefit from the possibility to work with flows in switch devices and
>>use the swdev api to offload flow datapath.
>>
>>Signed-off-by: Jiri Pirko <jiri@...nulli.us>
>>---
>>  net/openvswitch/Makefile       |   3 +-
>>  net/openvswitch/datapath.c     |  33 ++++++
>>  net/openvswitch/datapath.h     |   3 +
>>  net/openvswitch/flow_table.c   |   1 +
>>  net/openvswitch/hw_offload.c   | 245 +++++++++++++++++++++++++++++++++++++++++
>>  net/openvswitch/hw_offload.h   |  22 ++++
>>  net/openvswitch/vport-netdev.c |   3 +
>>  net/openvswitch/vport.h        |   2 +
>>  8 files changed, 311 insertions(+), 1 deletion(-)
>>  create mode 100644 net/openvswitch/hw_offload.c
>>  create mode 100644 net/openvswitch/hw_offload.h
>>
>>diff --git a/net/openvswitch/Makefile b/net/openvswitch/Makefile
>>index 3591cb5..5152437 100644
>>--- a/net/openvswitch/Makefile
>>+++ b/net/openvswitch/Makefile
>>@@ -13,7 +13,8 @@ openvswitch-y := \
>>  	flow_table.o \
>>  	vport.o \
>>  	vport-internal_dev.o \
>>-	vport-netdev.o
>>+	vport-netdev.o \
>>+	hw_offload.o
>>
>>  ifneq ($(CONFIG_OPENVSWITCH_VXLAN),)
>>  openvswitch-y += vport-vxlan.o
>>diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
>>index 75bb07f..3e43e1d 100644
>>--- a/net/openvswitch/datapath.c
>>+++ b/net/openvswitch/datapath.c
>>@@ -57,6 +57,7 @@
>>  #include "flow_netlink.h"
>>  #include "vport-internal_dev.h"
>>  #include "vport-netdev.h"
>>+#include "hw_offload.h"
>>
>>  int ovs_net_id __read_mostly;
>>
>>@@ -864,6 +865,9 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct genl_info *info)
>>  			acts = NULL;
>>  			goto err_unlock_ovs;
>>  		}
>>+		error = ovs_hw_flow_insert(dp, new_flow);
>>+		if (error)
>>+			pr_warn("failed to insert flow into hw\n");
>
>This is really close to silently failing. I think we need to
>hard fail here somehow and push it back to userspace as part of
>the reply and ovs_notify.

Yes, I agree. My plan was to handle this in ovs hw/sw/both netlink attr
implementation.


>
>Otherwise I don't know how to manage the hardware correctly. Consider
>the hardware table is full. In this case user space will continue to
>add rules and they will be silently discarded. Similarly if user space
>adds a flow/action that can not be supported by the hardware it will
>be silently ignored.
>
>Even if we do careful accounting on resources in user space we could
>still get an ENOMEM error from sw_flow_action_create.
>
>Same comment for the other hw commands flush/remove.
>
>>  		if (unlikely(reply)) {
>>  			error = ovs_flow_cmd_fill_info(new_flow,
>>@@ -896,10 +900,18 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct genl_info *info)
>>  				goto err_unlock_ovs;
>>  			}
>>  		}
>
>
>[...]
>
>
>Thanks,
>John
>
>-- 
>John Fastabend         Intel Corporation
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html