netdev - Re: [PATCH net-next 1/8] pkt_cls: add new tc cls helper to check offload flag and chain index

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: <20180125080348.GA2159@nanopsycho>
Date:   Thu, 25 Jan 2018 09:03:48 +0100
From:   Jiri Pirko <jiri@...nulli.us>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc:     davem@...emloft.net, dsahern@...il.com, netdev@...r.kernel.org,
        oss-drivers@...ronome.com
Subject: Re: [PATCH net-next 1/8] pkt_cls: add new tc cls helper to check
 offload flag and chain index

Thu, Jan 25, 2018 at 01:17:46AM CET, jakub.kicinski@...ronome.com wrote:
>Very few (mlxsw) upstream drivers seem to allow offload of chains
>other than 0.  Save driver developers typing and add a helper for
>checking both if ethtool's TC offload flag is on and if chain is 0.
>This helper will set the extack appropriately in both error cases.
>
>Signed-off-by: Jakub Kicinski <jakub.kicinski@...ronome.com>
>Reviewed-by: Simon Horman <simon.horman@...ronome.com>
>---
> drivers/net/ethernet/netronome/nfp/bpf/main.c |  4 +---
> drivers/net/netdevsim/bpf.c                   |  5 +----
> include/net/pkt_cls.h                         | 12 ++++++++++++
> 3 files changed, 14 insertions(+), 7 deletions(-)
>
>diff --git a/drivers/net/ethernet/netronome/nfp/bpf/main.c b/drivers/net/ethernet/netronome/nfp/bpf/main.c
>index b3206855535a..322027792fe8 100644
>--- a/drivers/net/ethernet/netronome/nfp/bpf/main.c
>+++ b/drivers/net/ethernet/netronome/nfp/bpf/main.c
>@@ -130,7 +130,7 @@ static int nfp_bpf_setup_tc_block_cb(enum tc_setup_type type,
> 				   "only offload of BPF classifiers supported");
> 		return -EOPNOTSUPP;
> 	}
>-	if (!tc_can_offload_extack(nn->dp.netdev, cls_bpf->common.extack))
>+	if (!tc_cls_can_offload_and_chain0(nn->dp.netdev, &cls_bpf->common))
> 		return -EOPNOTSUPP;
> 	if (!nfp_net_ebpf_capable(nn)) {
> 		NL_SET_ERR_MSG_MOD(cls_bpf->common.extack,
>@@ -142,8 +142,6 @@ static int nfp_bpf_setup_tc_block_cb(enum tc_setup_type type,
> 				   "only ETH_P_ALL supported as filter protocol");
> 		return -EOPNOTSUPP;
> 	}
>-	if (cls_bpf->common.chain_index)
>-		return -EOPNOTSUPP;
> 
> 	/* Only support TC direct action */
> 	if (!cls_bpf->exts_integrated ||
>diff --git a/drivers/net/netdevsim/bpf.c b/drivers/net/netdevsim/bpf.c
>index 8166f121bbcc..de73c1ff0939 100644
>--- a/drivers/net/netdevsim/bpf.c
>+++ b/drivers/net/netdevsim/bpf.c
>@@ -135,7 +135,7 @@ int nsim_bpf_setup_tc_block_cb(enum tc_setup_type type,
> 		return -EOPNOTSUPP;
> 	}
> 
>-	if (!tc_can_offload_extack(ns->netdev, cls_bpf->common.extack))
>+	if (!tc_cls_can_offload_and_chain0(ns->netdev, &cls_bpf->common))
> 		return -EOPNOTSUPP;
> 
> 	if (cls_bpf->common.protocol != htons(ETH_P_ALL)) {
>@@ -144,9 +144,6 @@ int nsim_bpf_setup_tc_block_cb(enum tc_setup_type type,
> 		return -EOPNOTSUPP;
> 	}
> 
>-	if (cls_bpf->common.chain_index)
>-		return -EOPNOTSUPP;
>-

For nfp and netdevsim you do this in a patch that introduces the helper,
for the rest you have a separate patch? Why this inconsistency?
Again, from my perspective, this can be done in a single patch for all
drivers. All the same hunks.



> 	if (!ns->bpf_tc_accept) {
> 		NSIM_EA(cls_bpf->common.extack,
> 			"netdevsim configured to reject BPF TC offload");
>diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
>index 1a41513cec7f..4db08d7dd22c 100644
>--- a/include/net/pkt_cls.h
>+++ b/include/net/pkt_cls.h
>@@ -656,6 +656,18 @@ static inline bool tc_can_offload_extack(const struct net_device *dev,
> 	return can;
> }
> 
>+static inline bool
>+tc_cls_can_offload_and_chain0(const struct net_device *dev,
>+			      struct tc_cls_common_offload *common)
>+{
>+	if (common->chain_index) {
>+		NL_SET_ERR_MSG(common->extack,
>+			       "Driver supports only offload of chain 0");

No need for a line-wrap.


>+		return false;
>+	}
>+	return tc_can_offload_extack(dev, common->extack);
>+}
>+
> static inline bool tc_skip_hw(u32 flags)
> {
> 	return (flags & TCA_CLS_FLAGS_SKIP_HW) ? true : false;
>-- 
>2.15.1
>