| 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
| ||
|
Date: Mon, 19 Nov 2018 21:44:52 +0000
From: "Chopra, Manish" <Manish.Chopra@...ium.com>
To: Pablo Neira Ayuso <pablo@...filter.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC: "davem@...emloft.net" <davem@...emloft.net>,
"thomas.lendacky@....com" <thomas.lendacky@....com>,
"f.fainelli@...il.com" <f.fainelli@...il.com>,
"Elior, Ariel" <Ariel.Elior@...ium.com>,
"michael.chan@...adcom.com" <michael.chan@...adcom.com>,
"santosh@...lsio.com" <santosh@...lsio.com>,
"madalin.bucur@....com" <madalin.bucur@....com>,
"yisen.zhuang@...wei.com" <yisen.zhuang@...wei.com>,
"salil.mehta@...wei.com" <salil.mehta@...wei.com>,
"jeffrey.t.kirsher@...el.com" <jeffrey.t.kirsher@...el.com>,
"tariqt@...lanox.com" <tariqt@...lanox.com>,
"saeedm@...lanox.com" <saeedm@...lanox.com>,
"jiri@...lanox.com" <jiri@...lanox.com>,
"idosch@...lanox.com" <idosch@...lanox.com>,
"jakub.kicinski@...ronome.com" <jakub.kicinski@...ronome.com>,
"peppe.cavallaro@...com" <peppe.cavallaro@...com>,
"grygorii.strashko@...com" <grygorii.strashko@...com>,
"andrew@...n.ch" <andrew@...n.ch>,
"vivien.didelot@...oirfairelinux.com"
<vivien.didelot@...oirfairelinux.com>,
"alexandre.torgue@...com" <alexandre.torgue@...com>,
"joabreu@...opsys.com" <joabreu@...opsys.com>,
"linux-net-drivers@...arflare.com" <linux-net-drivers@...arflare.com>,
"ganeshgr@...lsio.com" <ganeshgr@...lsio.com>,
"ogerlitz@...lanox.com" <ogerlitz@...lanox.com>
Subject: RE: [PATCH net-next 12/12] qede: use ethtool_rx_flow_rule() to remove
duplicated parser code
> -----Original Message-----
> From: netdev-owner@...r.kernel.org <netdev-owner@...r.kernel.org> On
> Behalf Of Pablo Neira Ayuso
> Sent: Monday, November 19, 2018 5:45 AM
> To: netdev@...r.kernel.org
> Cc: davem@...emloft.net; thomas.lendacky@....com;
> f.fainelli@...il.com; Elior, Ariel <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: [PATCH net-next 12/12] qede: use ethtool_rx_flow_rule() to remove
> duplicated parser code
>
> External Email
>
> The qede driver supports for ethtool_rx_flow_spec and flower, both
> codebases look very similar.
>
> This patch uses the ethtool_rx_flow_rule() infrastructure to remove the
> duplicated ethtool_rx_flow_spec parser and consolidate ACL offload support
> around the flow_rule infrastructure.
>
> Furthermore, more code can be consolidated by merging
> qede_add_cls_rule() and qede_add_tc_flower_fltr(), these two functions
> also look very similar.
>
> This driver currently provides simple ACL support, such as 5-tuple matching,
> drop policy and queue to CPU.
>
> Drivers that support more features can benefit from this infrastructure to
> save even more redundant codebase.
>
> Signed-off-by: Pablo Neira Ayuso <pablo@...filter.org>
> ---
> Note that, after this patch, qede_add_cls_rule() and
> qede_add_tc_flower_fltr() can be also consolidated since their code is
> redundant.
>
> drivers/net/ethernet/qlogic/qede/qede_filter.c | 246 ++++++-------------------
> 1 file changed, 53 insertions(+), 193 deletions(-)
>
> diff --git a/drivers/net/ethernet/qlogic/qede/qede_filter.c
> b/drivers/net/ethernet/qlogic/qede/qede_filter.c
> index aca302c3261b..f82b26ba8f80 100644
> --- a/drivers/net/ethernet/qlogic/qede/qede_filter.c
> +++ b/drivers/net/ethernet/qlogic/qede/qede_filter.c
> @@ -1578,30 +1578,6 @@ static void qede_flow_build_ipv6_hdr(struct
> qede_arfs_tuple *t,
> ports[1] = t->dst_port;
> }
>
> -/* Validate fields which are set and not accepted by the driver */ -static int
> qede_flow_spec_validate_unused(struct qede_dev *edev,
> - struct ethtool_rx_flow_spec *fs)
> -{
> - if (fs->flow_type & FLOW_MAC_EXT) {
> - DP_INFO(edev, "Don't support MAC extensions\n");
> - return -EOPNOTSUPP;
> - }
> -
> - if ((fs->flow_type & FLOW_EXT) &&
> - (fs->h_ext.vlan_etype || fs->h_ext.vlan_tci)) {
> - DP_INFO(edev, "Don't support vlan-based classification\n");
> - return -EOPNOTSUPP;
> - }
> -
> - if ((fs->flow_type & FLOW_EXT) &&
> - (fs->h_ext.data[0] || fs->h_ext.data[1])) {
> - DP_INFO(edev, "Don't support user defined data\n");
> - return -EOPNOTSUPP;
> - }
> -
> - return 0;
> -}
> -
> static int qede_set_v4_tuple_to_profile(struct qede_dev *edev,
> struct qede_arfs_tuple *t) { @@ -1665,132 +1641,6
> @@ static int qede_set_v6_tuple_to_profile(struct qede_dev *edev,
> return 0;
> }
>
> -static int qede_flow_spec_to_tuple_ipv4_common(struct qede_dev *edev,
> - struct qede_arfs_tuple *t,
> - struct ethtool_rx_flow_spec *fs)
> -{
> - if ((fs->h_u.tcp_ip4_spec.ip4src &
> - fs->m_u.tcp_ip4_spec.ip4src) != fs->h_u.tcp_ip4_spec.ip4src) {
> - DP_INFO(edev, "Don't support IP-masks\n");
> - return -EOPNOTSUPP;
> - }
> -
> - if ((fs->h_u.tcp_ip4_spec.ip4dst &
> - fs->m_u.tcp_ip4_spec.ip4dst) != fs->h_u.tcp_ip4_spec.ip4dst) {
> - DP_INFO(edev, "Don't support IP-masks\n");
> - return -EOPNOTSUPP;
> - }
> -
> - if ((fs->h_u.tcp_ip4_spec.psrc &
> - fs->m_u.tcp_ip4_spec.psrc) != fs->h_u.tcp_ip4_spec.psrc) {
> - DP_INFO(edev, "Don't support port-masks\n");
> - return -EOPNOTSUPP;
> - }
> -
> - if ((fs->h_u.tcp_ip4_spec.pdst &
> - fs->m_u.tcp_ip4_spec.pdst) != fs->h_u.tcp_ip4_spec.pdst) {
> - DP_INFO(edev, "Don't support port-masks\n");
> - return -EOPNOTSUPP;
> - }
> -
> - if (fs->h_u.tcp_ip4_spec.tos) {
> - DP_INFO(edev, "Don't support tos\n");
> - return -EOPNOTSUPP;
> - }
> -
> - t->eth_proto = htons(ETH_P_IP);
> - t->src_ipv4 = fs->h_u.tcp_ip4_spec.ip4src;
> - t->dst_ipv4 = fs->h_u.tcp_ip4_spec.ip4dst;
> - t->src_port = fs->h_u.tcp_ip4_spec.psrc;
> - t->dst_port = fs->h_u.tcp_ip4_spec.pdst;
> -
> - return qede_set_v4_tuple_to_profile(edev, t);
> -}
> -
> -static int qede_flow_spec_to_tuple_tcpv4(struct qede_dev *edev,
> - struct qede_arfs_tuple *t,
> - struct ethtool_rx_flow_spec *fs)
> -{
> - t->ip_proto = IPPROTO_TCP;
> -
> - if (qede_flow_spec_to_tuple_ipv4_common(edev, t, fs))
> - return -EINVAL;
> -
> - return 0;
> -}
> -
> -static int qede_flow_spec_to_tuple_udpv4(struct qede_dev *edev,
> - struct qede_arfs_tuple *t,
> - struct ethtool_rx_flow_spec *fs)
> -{
> - t->ip_proto = IPPROTO_UDP;
> -
> - if (qede_flow_spec_to_tuple_ipv4_common(edev, t, fs))
> - return -EINVAL;
> -
> - return 0;
> -}
> -
> -static int qede_flow_spec_to_tuple_ipv6_common(struct qede_dev *edev,
> - struct qede_arfs_tuple *t,
> - struct ethtool_rx_flow_spec *fs)
> -{
> - struct in6_addr zero_addr;
> -
> - memset(&zero_addr, 0, sizeof(zero_addr));
> -
> - if ((fs->h_u.tcp_ip6_spec.psrc &
> - fs->m_u.tcp_ip6_spec.psrc) != fs->h_u.tcp_ip6_spec.psrc) {
> - DP_INFO(edev, "Don't support port-masks\n");
> - return -EOPNOTSUPP;
> - }
> -
> - if ((fs->h_u.tcp_ip6_spec.pdst &
> - fs->m_u.tcp_ip6_spec.pdst) != fs->h_u.tcp_ip6_spec.pdst) {
> - DP_INFO(edev, "Don't support port-masks\n");
> - return -EOPNOTSUPP;
> - }
> -
> - if (fs->h_u.tcp_ip6_spec.tclass) {
> - DP_INFO(edev, "Don't support tclass\n");
> - return -EOPNOTSUPP;
> - }
> -
> - t->eth_proto = htons(ETH_P_IPV6);
> - memcpy(&t->src_ipv6, &fs->h_u.tcp_ip6_spec.ip6src,
> - sizeof(struct in6_addr));
> - memcpy(&t->dst_ipv6, &fs->h_u.tcp_ip6_spec.ip6dst,
> - sizeof(struct in6_addr));
> - t->src_port = fs->h_u.tcp_ip6_spec.psrc;
> - t->dst_port = fs->h_u.tcp_ip6_spec.pdst;
> -
> - return qede_set_v6_tuple_to_profile(edev, t, &zero_addr);
> -}
> -
> -static int qede_flow_spec_to_tuple_tcpv6(struct qede_dev *edev,
> - struct qede_arfs_tuple *t,
> - struct ethtool_rx_flow_spec *fs)
> -{
> - t->ip_proto = IPPROTO_TCP;
> -
> - if (qede_flow_spec_to_tuple_ipv6_common(edev, t, fs))
> - return -EINVAL;
> -
> - return 0;
> -}
> -
> -static int qede_flow_spec_to_tuple_udpv6(struct qede_dev *edev,
> - struct qede_arfs_tuple *t,
> - struct ethtool_rx_flow_spec *fs)
> -{
> - t->ip_proto = IPPROTO_UDP;
> -
> - if (qede_flow_spec_to_tuple_ipv6_common(edev, t, fs))
> - return -EINVAL;
> -
> - return 0;
> -}
> -
> /* Must be called while qede lock is held */ static struct
> qede_arfs_fltr_node * qede_flow_find_fltr(struct qede_dev *edev, struct
> qede_arfs_tuple *t) @@ -1875,25 +1725,32 @@ static int
> qede_parse_actions(struct qede_dev *edev,
> struct flow_action *flow_action) {
> const struct flow_action_key *act;
> - int rc = -EINVAL, num_act = 0, i;
> - bool is_drop = false;
> + int i;
>
> if (!flow_action_has_keys(flow_action)) {
> - DP_NOTICE(edev, "No tc actions received\n");
> - return rc;
> + DP_NOTICE(edev, "No actions received\n");
> + return -EINVAL;
> }
>
> flow_action_for_each(i, act, flow_action) {
> - num_act++;
> + switch (act->id) {
> + case FLOW_ACTION_KEY_DROP:
> + break;
> + case FLOW_ACTION_KEY_QUEUE:
> + if (ethtool_get_flow_spec_ring_vf(act->queue_index))
> + break;
>
> - if (act->id == FLOW_ACTION_KEY_DROP)
> - is_drop = true;
> + if (act->queue_index >= QEDE_RSS_COUNT(edev)) {
> + DP_INFO(edev, "Queue out-of-bounds\n");
> + return -EINVAL;
> + }
> + break;
> + default:
> + return -EINVAL;
> + }
> }
>
> - if (num_act == 1 && is_drop)
> - return 0;
> -
> - return rc;
> + return 0;
> }
>
> static int
> @@ -2147,16 +2004,17 @@ int qede_add_tc_flower_fltr(struct qede_dev
> *edev, __be16 proto, }
>
> static int qede_flow_spec_validate(struct qede_dev *edev,
> - struct ethtool_rx_flow_spec *fs,
> - struct qede_arfs_tuple *t)
> + struct flow_action *flow_action,
> + struct qede_arfs_tuple *t,
> + __u32 location)
> {
> - if (fs->location >= QEDE_RFS_MAX_FLTR) {
> + if (location >= QEDE_RFS_MAX_FLTR) {
> DP_INFO(edev, "Location out-of-bounds\n");
> return -EINVAL;
> }
>
> /* Check location isn't already in use */
> - if (test_bit(fs->location, edev->arfs->arfs_fltr_bmap)) {
> + if (test_bit(location, edev->arfs->arfs_fltr_bmap)) {
> DP_INFO(edev, "Location already in use\n");
> return -EINVAL;
> }
> @@ -2170,46 +2028,53 @@ static int qede_flow_spec_validate(struct
> qede_dev *edev,
> return -EINVAL;
> }
>
> - /* If drop requested then no need to validate other data */
> - if (fs->ring_cookie == RX_CLS_FLOW_DISC)
> - return 0;
> -
> - if (ethtool_get_flow_spec_ring_vf(fs->ring_cookie))
> - return 0;
> -
> - if (fs->ring_cookie >= QEDE_RSS_COUNT(edev)) {
> - DP_INFO(edev, "Queue out-of-bounds\n");
> + if (qede_parse_actions(edev, flow_action))
> return -EINVAL;
> - }
>
> return 0;
> }
>
> -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;
> +
> + if (qede_parse_flower_attr(edev, proto, &f, t)) {
> + err = -EINVAL;
> + goto err_out;
> + }
> +
> + /* Make sure location is valid and filter isn't already set */
> + err = qede_flow_spec_validate(edev, &f.rule.action, t,
> +fs->location);
> +err_out:
> + ethtool_rx_flow_rule_free(flow_rule);
> + return err;
> +
> }
>
> int qede_add_cls_rule(struct qede_dev *edev, struct ethtool_rxnfc *info)
> @@ -2227,12 +2092,7 @@ int qede_add_cls_rule(struct qede_dev *edev,
> struct ethtool_rxnfc *info)
> }
>
> /* Translate the flow specification into something fittign our DB */
> - rc = qede_flow_spec_to_tuple(edev, &t, fsp);
> - if (rc)
> - goto unlock;
> -
> - /* Make sure location is valid and filter isn't already set */
> - rc = qede_flow_spec_validate(edev, fsp, &t);
> + rc = qede_flow_spec_to_rule(edev, &t, fsp);
> if (rc)
> goto unlock;
>
> --
> 2.11.0
We will need some time to test these changes as it is changing this driver flows significantly.
Right now from the quick test of couple of ethtool commands below, which seems to be failing with this patch. They used to pass before this patch.
# ethtool -N eth0 flow-type tcp4 dst-port 5550 action -1
rmgr: Cannot insert RX class rule: Invalid argument
# dmesg -c
[12482.547161] [qede_parse_flower_attr:1930(eth0)]Invalid tc protocol request
It seem like ip_proto is not being set from below case, as flow_rule_match_key() seems to be returning false.
if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_BASIC)) {
struct flow_match_basic match;
flow_rule_match_basic(rule, &match);
ip_proto = match.key->ip_proto;
}
# ethtool -N eth0 flow-type tcp4 src-ip 192.168.40.200 dst-ip 192.168.50.200 src-port 6660 dst-port 5550 action -1
rmgr: Cannot insert RX class rule: Invalid argument
# dmesg -c
For this case, in function qede_tc_parse_ports() flow_rule_match_key() is returning false.
if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_PORTS)) {
struct flow_match_ports match;
--
--
}
Thanks.
Powered by blists - more mailing lists