netdev - Re: [PATCH net-next 12/12] qede: use ethtool_rx_flow

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:   Mon, 19 Nov 2018 17:12:40 +0100
From:   Pablo Neira Ayuso <pablo@...filter.org>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     netdev@...r.kernel.org, davem@...emloft.net,
        thomas.lendacky@....com, f.fainelli@...il.com,
        ariel.elior@...ium.com, michael.chan@...adcom.com,
        santosh@...lsio.com, madalin.bucur@....com,
        yisen.zhuang@...wei.com, salil.mehta@...wei.com,
        jeffrey.t.kirsher@...el.com, tariqt@...lanox.com,
        saeedm@...lanox.com, jiri@...lanox.com, idosch@...lanox.com,
        jakub.kicinski@...ronome.com, peppe.cavallaro@...com,
        grygorii.strashko@...com, andrew@...n.ch,
        vivien.didelot@...oirfairelinux.com, alexandre.torgue@...com,
        joabreu@...opsys.com, linux-net-drivers@...arflare.com,
        ganeshgr@...lsio.com, ogerlitz@...lanox.com
Subject: Re: [PATCH net-next 12/12] qede: use ethtool_rx_flow_rule() to
 remove duplicated parser code

On Mon, Nov 19, 2018 at 05:00:13PM +0100, Jiri Pirko wrote:
> Mon, Nov 19, 2018 at 01:15:19AM CET, pablo@...filter.org wrote:
[...]
> >-static int qede_flow_spec_to_tuple(struct qede_dev *edev,
> >-				   struct qede_arfs_tuple *t,
> >-				   struct ethtool_rx_flow_spec *fs)
> >+static int qede_flow_spec_to_rule(struct qede_dev *edev,
> >+				  struct qede_arfs_tuple *t,
> >+				  struct ethtool_rx_flow_spec *fs)
> > {
> >-	memset(t, 0, sizeof(*t));
> >-
> >-	if (qede_flow_spec_validate_unused(edev, fs))
> >-		return -EOPNOTSUPP;
> >+	struct tc_cls_flower_offload f = {};
> >+	struct flow_rule *flow_rule;
> >+	__be16 proto;
> >+	int err = 0;
> > 
> > 	switch ((fs->flow_type & ~FLOW_EXT)) {
> > 	case TCP_V4_FLOW:
> >-		return qede_flow_spec_to_tuple_tcpv4(edev, t, fs);
> > 	case UDP_V4_FLOW:
> >-		return qede_flow_spec_to_tuple_udpv4(edev, t, fs);
> >+		proto = htons(ETH_P_IP);
> >+		break;
> > 	case TCP_V6_FLOW:
> >-		return qede_flow_spec_to_tuple_tcpv6(edev, t, fs);
> > 	case UDP_V6_FLOW:
> >-		return qede_flow_spec_to_tuple_udpv6(edev, t, fs);
> >+		proto = htons(ETH_P_IPV6);
> >+		break;
> > 	default:
> > 		DP_VERBOSE(edev, NETIF_MSG_IFUP,
> > 			   "Can't support flow of type %08x\n", fs->flow_type);
> > 		return -EOPNOTSUPP;
> > 	}
> > 
> >-	return 0;
> >+	flow_rule = ethtool_rx_flow_rule(fs);
> >+	if (!flow_rule)
> >+		return -ENOMEM;
> >+
> >+	f.rule = *flow_rule;
> 
> This does not look right. I undersntand that you want to use the same
> driver code to parse same struct coming either from tc-flower or
> ethtool. That is why you introduced flow_rule as a intermediate layer.
> However, here, you use struct that is very tc-flower specific. Common
> parser should work on struct flow_rule.
>
> qede_parse_flower_attr() should accept struct flow_rule * and should be
> renamed to something like qede_parse_flow_rule().

That was intentional, to keep the number of changes in this drivers as
small as possible while introducing the flow_rule infrastructure.

I'll rework the driver as you're asking, so I can pass struct
flow_rule instead to all parser functions.

The patchset is already rather large, and involving many driver, I was
trying to keep changes to minimal but I'll update this driver as you
request, no problem.

Thanks.