From: Jakub Kicinski <hidden> Date: 2018-06-25 20:23:21
Hi!
This series brings two fixes to TC filter/action offload code.
Pieter fixes matching MPLS packets when the match is purely on
the MPLS ethertype and none of the MPLS fields are used.
John provides a fix for offload of shared blocks. Unfortunately,
with shared blocks there is currently no guarantee that filters
which were added by the core will be removed before block unbind.
Our simple fix is to not support offload of rules on shared blocks
at all, a revert of this fix will be send for -next once the
reoffload infrastructure lands. The shared blocks became important
as we are trying to use them for bonding offload (managed from user
space) and lack of remove calls leads to resource leaks.
John Hurley (1):
nfp: reject binding to shared blocks
Pieter Jansen van Vuuren (1):
nfp: flower: fix mpls ether type detection
drivers/net/ethernet/netronome/nfp/bpf/main.c | 3 +++
drivers/net/ethernet/netronome/nfp/flower/match.c | 14 ++++++++++++++
.../net/ethernet/netronome/nfp/flower/offload.c | 12 ++++++++++++
3 files changed, 29 insertions(+)
--
2.17.1
From: Jakub Kicinski <hidden> Date: 2018-06-25 20:23:23
From: Pieter Jansen van Vuuren <redacted>
Previously it was not possible to distinguish between mpls ether types and
other ether types. This leads to incorrect classification of offloaded
filters that match on mpls ether type. For example the following two
filters overlap:
# tc filter add dev eth0 parent ffff: \
protocol 0x8847 flower \
action mirred egress redirect dev eth1
# tc filter add dev eth0 parent ffff: \
protocol 0x0800 flower \
action mirred egress redirect dev eth2
The driver now correctly includes the mac_mpls layer where HW stores mpls
fields, when it detects an mpls ether type. It also sets the MPLS_Q bit to
indicate that the filter should match mpls packets.
Fixes: bb055c198d9b ("nfp: add mpls match offloading support")
Signed-off-by: Pieter Jansen van Vuuren <redacted>
Reviewed-by: Simon Horman <redacted>
Reviewed-by: Jakub Kicinski <redacted>
---
drivers/net/ethernet/netronome/nfp/flower/match.c | 14 ++++++++++++++
.../net/ethernet/netronome/nfp/flower/offload.c | 8 ++++++++
2 files changed, 22 insertions(+)
@@ -123,6 +123,20 @@ nfp_flower_compile_mac(struct nfp_flower_mac_mpls *frame,NFP_FLOWER_MASK_MPLS_Q;frame->mpls_lse=cpu_to_be32(t_mpls);+}elseif(dissector_uses_key(flow->dissector,+FLOW_DISSECTOR_KEY_BASIC)){+/* Check for mpls ether type and set NFP_FLOWER_MASK_MPLS_Q+*bit,whichindicatesanmplsethertypebutwithoutany+*mplsfields.+*/+structflow_dissector_key_basic*key_basic;++key_basic=skb_flow_dissector_target(flow->dissector,+FLOW_DISSECTOR_KEY_BASIC,+flow->key);+if(key_basic->n_proto==cpu_to_be16(ETH_P_MPLS_UC)||+key_basic->n_proto==cpu_to_be16(ETH_P_MPLS_MC))+frame->mpls_lse=cpu_to_be32(NFP_FLOWER_MASK_MPLS_Q);}}
@@ -264,6 +264,14 @@ nfp_flower_calculate_key_layers(struct nfp_app *app,casecpu_to_be16(ETH_P_ARP):return-EOPNOTSUPP;+casecpu_to_be16(ETH_P_MPLS_UC):+casecpu_to_be16(ETH_P_MPLS_MC):+if(!(key_layer&NFP_FLOWER_LAYER_MAC)){+key_layer|=NFP_FLOWER_LAYER_MAC;+key_size+=sizeof(structnfp_flower_mac_mpls);+}+break;+/* Will be included in layer 2. */casecpu_to_be16(ETH_P_8021Q):break;
From: Jakub Kicinski <hidden> Date: 2018-06-25 20:23:25
From: John Hurley <redacted>
TC shared blocks allow multiple qdiscs to be grouped together and filters
shared between them. Currently the chains of filters attached to a block
are only flushed when the block is removed. If a qdisc is removed from a
block but the block still exists, flow del messages are not passed to the
callback registered for that qdisc. For the NFP, this presents the
possibility of rules still existing in hw when they should be removed.
Prevent binding to shared blocks until the kernel can send per qdisc del
messages when block unbinds occur.
Fixes: 4861738775d7 ("net: sched: introduce shared filter blocks infrastructure")
Signed-off-by: John Hurley <redacted>
Signed-off-by: Jakub Kicinski <redacted>
Reviewed-by: Simon Horman <redacted>
---
drivers/net/ethernet/netronome/nfp/bpf/main.c | 3 +++
drivers/net/ethernet/netronome/nfp/flower/offload.c | 3 +++
2 files changed, 6 insertions(+)
Mon, Jun 25, 2018 at 10:22:46PM CEST, jakub.kicinski@netronome.com wrote:
From: John Hurley <redacted>
TC shared blocks allow multiple qdiscs to be grouped together and filters
shared between them. Currently the chains of filters attached to a block
are only flushed when the block is removed. If a qdisc is removed from a
block but the block still exists, flow del messages are not passed to the
callback registered for that qdisc. For the NFP, this presents the
possibility of rules still existing in hw when they should be removed.
Prevent binding to shared blocks until the kernel can send per qdisc del
messages when block unbinds occur.
This is not nfp-specific problem. Should be handled differently. The
driver has information about offloaded filters. On unbind, it have
enough info to do the flush, doesn't it?
From: Jakub Kicinski <hidden> Date: 2018-06-25 20:50:20
On Mon, 25 Jun 2018 22:40:21 +0200, Jiri Pirko wrote:
Mon, Jun 25, 2018 at 10:22:46PM CEST, jakub.kicinski@netronome.com wrote:
quoted
From: John Hurley <redacted>
TC shared blocks allow multiple qdiscs to be grouped together and filters
shared between them. Currently the chains of filters attached to a block
are only flushed when the block is removed. If a qdisc is removed from a
block but the block still exists, flow del messages are not passed to the
callback registered for that qdisc. For the NFP, this presents the
possibility of rules still existing in hw when they should be removed.
Prevent binding to shared blocks until the kernel can send per qdisc del
messages when block unbinds occur.
This is not nfp-specific problem. Should be handled differently. The
driver has information about offloaded filters. On unbind, it have
enough info to do the flush, doesn't it?
Certainly. But this fix is simpler and sufficient. We need to
backport it back to 4.16. If we have to go through driver tables
and flush filters we may as well merge the reoffload series to net.
Mon, Jun 25, 2018 at 10:50:14PM CEST, jakub.kicinski@netronome.com wrote:
On Mon, 25 Jun 2018 22:40:21 +0200, Jiri Pirko wrote:
quoted
Mon, Jun 25, 2018 at 10:22:46PM CEST, jakub.kicinski@netronome.com wrote:
quoted
From: John Hurley <redacted>
TC shared blocks allow multiple qdiscs to be grouped together and filters
shared between them. Currently the chains of filters attached to a block
are only flushed when the block is removed. If a qdisc is removed from a
block but the block still exists, flow del messages are not passed to the
callback registered for that qdisc. For the NFP, this presents the
possibility of rules still existing in hw when they should be removed.
Prevent binding to shared blocks until the kernel can send per qdisc del
messages when block unbinds occur.
This is not nfp-specific problem. Should be handled differently. The
driver has information about offloaded filters. On unbind, it have
enough info to do the flush, doesn't it?
Certainly. But this fix is simpler and sufficient. We need to
backport it back to 4.16. If we have to go through driver tables
and flush filters we may as well merge the reoffload series to net.
Oh, I missed this is for net. Sorry.
Acked-by: Jiri Pirko <redacted>
From: kbuild test robot <hidden> Date: 2018-06-25 23:38:08
Hi John,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on net/master]
url: https://github.com/0day-ci/linux/commits/Jakub-Kicinski/nfp-MPLS-and-shared-blocks-TC-offload-fixes/20180626-042358
config: i386-randconfig-x001-201825 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
drivers/net/ethernet/netronome/nfp/flower/offload.c: In function 'nfp_flower_setup_tc_block':
quoted
drivers/net/ethernet/netronome/nfp/flower/offload.c:634:6: error: implicit declaration of function 'tcf_block_shared'; did you mean 'tcf_block_dev'? [-Werror=implicit-function-declaration]
if (tcf_block_shared(f->block))
^~~~~~~~~~~~~~~~
tcf_block_dev
cc1: some warnings being treated as errors
vim +634 drivers/net/ethernet/netronome/nfp/flower/offload.c
625
626 static int nfp_flower_setup_tc_block(struct net_device *netdev,
627 struct tc_block_offload *f)
628 {
629 struct nfp_repr *repr = netdev_priv(netdev);
630
631 if (f->binder_type != TCF_BLOCK_BINDER_TYPE_CLSACT_INGRESS)
632 return -EOPNOTSUPP;
633
> 634 if (tcf_block_shared(f->block))
635 return -EOPNOTSUPP;
636
637 switch (f->command) {
638 case TC_BLOCK_BIND:
639 return tcf_block_cb_register(f->block,
640 nfp_flower_setup_tc_block_cb,
641 repr, repr);
642 case TC_BLOCK_UNBIND:
643 tcf_block_cb_unregister(f->block,
644 nfp_flower_setup_tc_block_cb,
645 repr);
646 return 0;
647 default:
648 return -EOPNOTSUPP;
649 }
650 }
651
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
From: kbuild test robot <hidden> Date: 2018-06-26 02:05:45
Hi John,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on net/master]
url: https://github.com/0day-ci/linux/commits/Jakub-Kicinski/nfp-MPLS-and-shared-blocks-TC-offload-fixes/20180626-042358
config: x86_64-randconfig-u0-06260533 (attached as .config)
compiler: gcc-5 (Debian 5.5.0-3) 5.4.1 20171010
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
drivers/net/ethernet/netronome/nfp/bpf/main.c: In function 'nfp_bpf_setup_tc_block':
quoted
drivers/net/ethernet/netronome/nfp/bpf/main.c:205:6: error: implicit declaration of function 'tcf_block_shared' [-Werror=implicit-function-declaration]
if (tcf_block_shared(f->block))
^
cc1: some warnings being treated as errors
vim +/tcf_block_shared +205 drivers/net/ethernet/netronome/nfp/bpf/main.c
196
197 static int nfp_bpf_setup_tc_block(struct net_device *netdev,
198 struct tc_block_offload *f)
199 {
200 struct nfp_net *nn = netdev_priv(netdev);
201
202 if (f->binder_type != TCF_BLOCK_BINDER_TYPE_CLSACT_INGRESS)
203 return -EOPNOTSUPP;
204
> 205 if (tcf_block_shared(f->block))
206 return -EOPNOTSUPP;
207
208 switch (f->command) {
209 case TC_BLOCK_BIND:
210 return tcf_block_cb_register(f->block,
211 nfp_bpf_setup_tc_block_cb,
212 nn, nn);
213 case TC_BLOCK_UNBIND:
214 tcf_block_cb_unregister(f->block,
215 nfp_bpf_setup_tc_block_cb,
216 nn);
217 return 0;
218 default:
219 return -EOPNOTSUPP;
220 }
221 }
222
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation