[PATCH net 1/2] nfp: flower: release metadata on offload failure

[PATCH net 1/2] nfp: flower: release metadata on offload failure

[Jakub Kicinski]

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

Calling nfp_compile_flow_metadata both assigns a stats context and
increments a ref counter on (or allocates) a mask id table entry. These
are released by the nfp_modify_flow_metadata call on flow deletion,
however, if a flow add fails after metadata is set then the flow entry
will be deleted but the metadata assignments leaked.

Add an error path to the flow add offload function to ensure allocated
metadata is released in the event of an offload fail.

Fixes: 81f3ddf2547d ("nfp: add control message passing capabilities to flower offloads")
Signed-off-by: John Hurley <john.hurley@netronome.com>
Reviewed-by: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 drivers/net/ethernet/netronome/nfp/flower/offload.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/flower/offload.c b/drivers/net/ethernet/netronome/nfp/flower/offload.c
index 29c95423ab64..c3ad8d737cf0 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/offload.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c
@@ -479,13 +479,13 @@ nfp_flower_add_offload(struct nfp_app *app, struct net_device *netdev,
 	err = nfp_flower_xmit_flow(netdev, flow_pay,
 				   NFP_FLOWER_CMSG_TYPE_FLOW_ADD);
 	if (err)
-		goto err_destroy_flow;
+		goto err_release_metadata;
 
 	flow_pay->tc_flower_cookie = flow->cookie;
 	err = rhashtable_insert_fast(&priv->flow_table, &flow_pay->fl_node,
 				     nfp_flower_table_params);
 	if (err)
-		goto err_destroy_flow;
+		goto err_release_metadata;
 
 	port->tc_offload_cnt++;
 
@@ -494,6 +494,8 @@ nfp_flower_add_offload(struct nfp_app *app, struct net_device *netdev,
 
 	return 0;
 
+err_release_metadata:
+	nfp_modify_flow_metadata(app, flow_pay);
 err_destroy_flow:
 	kfree(flow_pay->action_data);
 	kfree(flow_pay->mask_data);
-- 
2.17.1

[PATCH net 2/2] nfp: flower: prevent offload if rhashtable insert fails

[Jakub Kicinski]

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

For flow offload adds, if the rhash insert code fails, the flow will still
have been offloaded but the reference to it in the driver freed.

Re-order the offload setup calls to ensure that a flow will only be written
to FW if a kernel reference is held and stored in the rhashtable. Remove
this hashtable entry if the offload fails.

Fixes: c01d0efa5136 ("nfp: flower: use rhashtable for flow caching")
Signed-off-by: John Hurley <john.hurley@netronome.com>
Reviewed-by: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
Merge note: there will be a slight merge conflict with net-next
here:
 - the first argument of nfp_flower_xmit_flow() changed from 'netdev'
   to 'app';
 - we only bump the port offload cnt if (port).

FWIW the net-next version of the patch can be found at:

     git://git.kernel.org/pub/scm/linux/kernel/git/kuba/linux.git

     17ed95873d51 nfp: flower: prevent offload if rhashtable insert fails
     d857fc8f472b nfp: flower: release metadata on offload failure

CC: Stephen Rothwell <sfr@canb.auug.org.au> # for linux-next
---
 .../net/ethernet/netronome/nfp/flower/offload.c    | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/flower/offload.c b/drivers/net/ethernet/netronome/nfp/flower/offload.c
index c3ad8d737cf0..2f49eb75f3cc 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/offload.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c
@@ -476,17 +476,17 @@ nfp_flower_add_offload(struct nfp_app *app, struct net_device *netdev,
 	if (err)
 		goto err_destroy_flow;
 
-	err = nfp_flower_xmit_flow(netdev, flow_pay,
-				   NFP_FLOWER_CMSG_TYPE_FLOW_ADD);
-	if (err)
-		goto err_release_metadata;
-
 	flow_pay->tc_flower_cookie = flow->cookie;
 	err = rhashtable_insert_fast(&priv->flow_table, &flow_pay->fl_node,
 				     nfp_flower_table_params);
 	if (err)
 		goto err_release_metadata;
 
+	err = nfp_flower_xmit_flow(netdev, flow_pay,
+				   NFP_FLOWER_CMSG_TYPE_FLOW_ADD);
+	if (err)
+		goto err_remove_rhash;
+
 	port->tc_offload_cnt++;
 
 	/* Deallocate flow payload when flower rule has been destroyed. */
@@ -494,6 +494,10 @@ nfp_flower_add_offload(struct nfp_app *app, struct net_device *netdev,
 
 	return 0;
 
+err_remove_rhash:
+	WARN_ON_ONCE(rhashtable_remove_fast(&priv->flow_table,
+					    &flow_pay->fl_node,
+					    nfp_flower_table_params));
 err_release_metadata:
 	nfp_modify_flow_metadata(app, flow_pay);
 err_destroy_flow:
-- 
2.17.1

Re: [PATCH net 1/2] nfp: flower: release metadata on offload failure

[David Miller]

From: Jakub Kicinski <jakub.kicinski@netronome.com>
Date: Tue, 27 Nov 2018 14:04:11 -0800

> From: John Hurley <john.hurley@netronome.com>
> 
> Calling nfp_compile_flow_metadata both assigns a stats context and
> increments a ref counter on (or allocates) a mask id table entry. These
> are released by the nfp_modify_flow_metadata call on flow deletion,
> however, if a flow add fails after metadata is set then the flow entry
> will be deleted but the metadata assignments leaked.
> 
> Add an error path to the flow add offload function to ensure allocated
> metadata is released in the event of an offload fail.
> 
> Fixes: 81f3ddf2547d ("nfp: add control message passing capabilities to flower offloads")
> Signed-off-by: John Hurley <john.hurley@netronome.com>
> Reviewed-by: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>

Applied.

Re: [PATCH net 2/2] nfp: flower: prevent offload if rhashtable insert fails

[David Miller]

From: Jakub Kicinski <jakub.kicinski@netronome.com>
Date: Tue, 27 Nov 2018 14:04:12 -0800

> From: John Hurley <john.hurley@netronome.com>
> 
> For flow offload adds, if the rhash insert code fails, the flow will still
> have been offloaded but the reference to it in the driver freed.
> 
> Re-order the offload setup calls to ensure that a flow will only be written
> to FW if a kernel reference is held and stored in the rhashtable. Remove
> this hashtable entry if the offload fails.
> 
> Fixes: c01d0efa5136 ("nfp: flower: use rhashtable for flow caching")
> Signed-off-by: John Hurley <john.hurley@netronome.com>
> Reviewed-by: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> ---
> Merge note: there will be a slight merge conflict with net-next
> here:
>  - the first argument of nfp_flower_xmit_flow() changed from 'netdev'
>    to 'app';
>  - we only bump the port offload cnt if (port).
> 
> FWIW the net-next version of the patch can be found at:
> 
>      git://git.kernel.org/pub/scm/linux/kernel/git/kuba/linux.git
> 
>      17ed95873d51 nfp: flower: prevent offload if rhashtable insert fails
>      d857fc8f472b nfp: flower: release metadata on offload failure
> 
> CC: Stephen Rothwell <sfr@canb.auug.org.au> # for linux-next

Applied.