[PATCH net 0/2] nfp: MPLS and shared blocks TC offload fixes

[PATCH net 0/2] nfp: MPLS and shared blocks TC offload fixes

[Jakub Kicinski]

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

[PATCH net 1/2] nfp: flower: fix mpls ether type detection

[Jakub Kicinski]

From: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>

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 <pieter.jansenvanvuuren@netronome.com>
Reviewed-by: Simon Horman <simon.horman@netronome.com>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 drivers/net/ethernet/netronome/nfp/flower/match.c  | 14 ++++++++++++++
 .../net/ethernet/netronome/nfp/flower/offload.c    |  8 ++++++++
 2 files changed, 22 insertions(+)

diff --git a/drivers/net/ethernet/netronome/nfp/flower/match.c b/drivers/net/ethernet/netronome/nfp/flower/match.c
index 91935405f586..84f7a5dbea9d 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/match.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/match.c
@@ -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);
+	} else if (dissector_uses_key(flow->dissector,
+				      FLOW_DISSECTOR_KEY_BASIC)) {
+		/* Check for mpls ether type and set NFP_FLOWER_MASK_MPLS_Q
+		 * bit, which indicates an mpls ether type but without any
+		 * mpls fields.
+		 */
+		struct flow_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);
 	}
 }
 
diff --git a/drivers/net/ethernet/netronome/nfp/flower/offload.c b/drivers/net/ethernet/netronome/nfp/flower/offload.c
index c42e64f32333..477f584f6d28 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/offload.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c
@@ -264,6 +264,14 @@ nfp_flower_calculate_key_layers(struct nfp_app *app,
 		case cpu_to_be16(ETH_P_ARP):
 			return -EOPNOTSUPP;
 
+		case cpu_to_be16(ETH_P_MPLS_UC):
+		case cpu_to_be16(ETH_P_MPLS_MC):
+			if (!(key_layer & NFP_FLOWER_LAYER_MAC)) {
+				key_layer |= NFP_FLOWER_LAYER_MAC;
+				key_size += sizeof(struct nfp_flower_mac_mpls);
+			}
+			break;
+
 		/* Will be included in layer 2. */
 		case cpu_to_be16(ETH_P_8021Q):
 			break;
-- 
2.17.1

[PATCH net 2/2] nfp: reject binding to shared blocks

[Jakub Kicinski]

From: John Hurley <john.hurley@netronome.com>

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 <john.hurley@netronome.com>
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Simon Horman <simon.horman@netronome.com>
---
 drivers/net/ethernet/netronome/nfp/bpf/main.c       | 3 +++
 drivers/net/ethernet/netronome/nfp/flower/offload.c | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/netronome/nfp/bpf/main.c b/drivers/net/ethernet/netronome/nfp/bpf/main.c
index fcdfb8e7fdea..6b15e3b11956 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/main.c
+++ b/drivers/net/ethernet/netronome/nfp/bpf/main.c
@@ -202,6 +202,9 @@ static int nfp_bpf_setup_tc_block(struct net_device *netdev,
 	if (f->binder_type != TCF_BLOCK_BINDER_TYPE_CLSACT_INGRESS)
 		return -EOPNOTSUPP;
 
+	if (tcf_block_shared(f->block))
+		return -EOPNOTSUPP;
+
 	switch (f->command) {
 	case TC_BLOCK_BIND:
 		return tcf_block_cb_register(f->block,
diff --git a/drivers/net/ethernet/netronome/nfp/flower/offload.c b/drivers/net/ethernet/netronome/nfp/flower/offload.c
index 477f584f6d28..525057bee0ed 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/offload.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c
@@ -631,6 +631,9 @@ static int nfp_flower_setup_tc_block(struct net_device *netdev,
 	if (f->binder_type != TCF_BLOCK_BINDER_TYPE_CLSACT_INGRESS)
 		return -EOPNOTSUPP;
 
+	if (tcf_block_shared(f->block))
+		return -EOPNOTSUPP;
+
 	switch (f->command) {
 	case TC_BLOCK_BIND:
 		return tcf_block_cb_register(f->block,
-- 
2.17.1

Re: [PATCH net 2/2] nfp: reject binding to shared blocks

[Jiri Pirko]

Mon, Jun 25, 2018 at 10:22:46PM CEST, jakub.kicinski@netronome.com wrote:
>From: John Hurley <john.hurley@netronome.com>
>
>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?


>
>Fixes: 4861738775d7 ("net: sched: introduce shared filter blocks infrastructure")
>Signed-off-by: John Hurley <john.hurley@netronome.com>
>Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>Reviewed-by: Simon Horman <simon.horman@netronome.com>
>---
> drivers/net/ethernet/netronome/nfp/bpf/main.c       | 3 +++
> drivers/net/ethernet/netronome/nfp/flower/offload.c | 3 +++
> 2 files changed, 6 insertions(+)
>
>diff --git a/drivers/net/ethernet/netronome/nfp/bpf/main.c b/drivers/net/ethernet/netronome/nfp/bpf/main.c
>index fcdfb8e7fdea..6b15e3b11956 100644
>--- a/drivers/net/ethernet/netronome/nfp/bpf/main.c
>+++ b/drivers/net/ethernet/netronome/nfp/bpf/main.c
>@@ -202,6 +202,9 @@ static int nfp_bpf_setup_tc_block(struct net_device *netdev,
> 	if (f->binder_type != TCF_BLOCK_BINDER_TYPE_CLSACT_INGRESS)
> 		return -EOPNOTSUPP;
> 
>+	if (tcf_block_shared(f->block))
>+		return -EOPNOTSUPP;
>+
> 	switch (f->command) {
> 	case TC_BLOCK_BIND:
> 		return tcf_block_cb_register(f->block,
>diff --git a/drivers/net/ethernet/netronome/nfp/flower/offload.c b/drivers/net/ethernet/netronome/nfp/flower/offload.c
>index 477f584f6d28..525057bee0ed 100644
>--- a/drivers/net/ethernet/netronome/nfp/flower/offload.c
>+++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c
>@@ -631,6 +631,9 @@ static int nfp_flower_setup_tc_block(struct net_device *netdev,
> 	if (f->binder_type != TCF_BLOCK_BINDER_TYPE_CLSACT_INGRESS)
> 		return -EOPNOTSUPP;
> 
>+	if (tcf_block_shared(f->block))
>+		return -EOPNOTSUPP;
>+
> 	switch (f->command) {
> 	case TC_BLOCK_BIND:
> 		return tcf_block_cb_register(f->block,
>-- 
>2.17.1
>

Re: [PATCH net 2/2] nfp: reject binding to shared blocks

[Jakub Kicinski]

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:
> >From: John Hurley <john.hurley@netronome.com>
> >
> >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.

Re: [PATCH net 2/2] nfp: reject binding to shared blocks

[Jiri Pirko]

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:
>> Mon, Jun 25, 2018 at 10:22:46PM CEST, jakub.kicinski@netronome.com wrote:
>> >From: John Hurley <john.hurley@netronome.com>
>> >
>> >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 <jiri@mellanox.com>

Re: [PATCH net 2/2] nfp: reject binding to shared blocks

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1883 bytes --]

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':
>> 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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 33423 bytes --]

Re: [PATCH net 2/2] nfp: reject binding to shared blocks

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1803 bytes --]

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':
>> 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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 33599 bytes --]