netdev - Re: [PATCH net-next 6/8] net/mlx5e: Introduce tc offload support

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: <20160302155845.GD2122@nanopsycho.orion>
Date:	Wed, 2 Mar 2016 16:58:45 +0100
From:	Jiri Pirko <jiri@...nulli.us>
To:	Amir Vadai <amir@...ai.me>
Cc:	John Fastabend <john.fastabend@...il.com>,
	"David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
	Or Gerlitz <ogerlitz@...lanox.com>,
	John Fastabend <john.r.fastabend@...el.com>,
	Saeed Mahameed <saeedm@...lanox.com>,
	Hadar Har-Zion <hadarh@...lanox.com>,
	Jiri Pirko <jiri@...lanox.com>
Subject: Re: [PATCH net-next 6/8] net/mlx5e: Introduce tc offload support

Wed, Mar 02, 2016 at 04:53:37PM CET, amir@...ai.me wrote:
>On Tue, Mar 01, 2016 at 09:13:25AM -0800, John Fastabend wrote:
>> On 16-03-01 09:00 AM, Amir Vadai wrote:
>> > On Tue, Mar 01, 2016 at 03:52:08PM +0100, Jiri Pirko wrote:
>> >> Tue, Mar 01, 2016 at 03:24:48PM CET, amir@...ai.me wrote:
>> >>> Extend ndo_setup_tc() to support ingress tc offloading. Will be used by
>> >>> later patches to offload tc flower filter.
>> >>>
>> >>> Feature is off by default and could be enabled by issuing:
>> >>> # ethtool  -K eth0 hw-tc-offload on
>> >>>
>> >>> Offloads flow table is dynamically created when first filter is
>> >>> added.
>> >>> Rules are saved in a hash table that is maintained by the consumer (for
>> >>> example - the flower offload in the next patch).
>> >>> When last filter is removed and no filters exist in the hash table, the
>> >>> offload flow table is destroyed.
>> >>
>> >> <snip>	
>> >> 	
>> >>> @@ -1880,6 +1883,17 @@ static int mlx5e_setup_tc(struct net_device *netdev, u8 tc)
>> >>> static int mlx5e_ndo_setup_tc(struct net_device *dev, u32 handle,
>> >>> 			      __be16 proto, struct tc_to_netdev *tc)
>> >>> {
>> >>> +	struct mlx5e_priv *priv = netdev_priv(dev);
>> >>> +
>> >>> +	if (TC_H_MAJ(handle) != TC_H_MAJ(TC_H_INGRESS))
>> >>> +		goto mqprio;
>> >>> +
>> >>> +	switch (tc->type) {
>> >>> +	default:
>> >>> +		return -EINVAL;
>> >>
>> >> -EOPNOTSUPP would be better here perhaps?
>> >>
>> >>
>> >>> +	}
>> >>> +
>> >>> +mqprio:
>> >>> 	if (handle != TC_H_ROOT || tc->type != TC_SETUP_MQPRIO)
>> >>> 		return -EINVAL;
>> >>>
>> >>> @@ -1963,6 +1977,13 @@ static int mlx5e_set_features(struct net_device *netdev,
>> >>> 			mlx5e_disable_vlan_filter(priv);
>> >>> 	}
>> >>>
>> >>> +	if ((changes & NETIF_F_HW_TC) && !(features & NETIF_F_HW_TC) &&
>> >>> +	    mlx5e_tc_num_filters(priv)) {
>> >>> +		netdev_err(netdev,
>> >>> +			   "Active offloaded tc filters, can't turn hw_tc_offload off\n");
>> >>> +		return -EINVAL;
>> >>
>> >> This should not fail I believe. Just disable it in hw. I would even toss
>> >> away the rules if necessary.
>> > It depends on the answer regarding your comment on the previous patch.
>> > If we have the rule in both SW and HW, and remove it from the HW it is
>> > ok (although, currently I don't understand why would anyone want in both
>> > places).
>> > If the rule is processed by HW only - turning off this flag, will
>> > disable the offloaded rules - it might be misleading, so I prefered not
>> > to allow it and print a message.
>> 
>> When we get the HW only mode we will need to also flush the hardware
>> representation in software as well as the hardware state.
>
>Yep, I do think that just failing the operation is the best appraoch.
>Will make the design simpler, and from the user point of view, less
>surprises.
>
>Jiri?

I don't feel it is ok, but at the same time, it is probably the best
solution for now. Other solutions would be too complicated.