[PATCH net-next 0/3] bnxt_en: ntuple filter fixes

[PATCH net-next 0/3] bnxt_en: ntuple filter fixes

[Michael Chan]

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

The first patch is to remove an unneeded variable.  The next 2 patches
are to release RCU lock correctly after accesing the RCU protected
filter structure.  Patch 2 also re-arranges the code to look cleaner.

Michael Chan (3):
  bnxt_en: Remove unneeded variable in bnxt_hwrm_clear_vnic_filter()
  bnxt_en: Fix RCU locking for ntuple filters in bnxt_srxclsrldel()
  bnxt_en: Fix RCU locking for ntuple filters in bnxt_rx_flow_steer()

 drivers/net/ethernet/broadcom/bnxt/bnxt.c     |  7 ++-----
 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 21 ++++++++++---------
 2 files changed, 13 insertions(+), 15 deletions(-)

-- 
2.30.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

[PATCH net-next 1/3] bnxt_en: Remove unneeded variable in bnxt_hwrm_clear_vnic_filter()

[Michael Chan]

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

After recent refactoring, this function doesn't return error any
more.  Remove the unneeded rc variable and change the function to
void.  The caller is not checking for the return value.

Fixes: 96c9bedc755e ("bnxt_en: Refactor L2 filter alloc/free firmware commands.")
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202401041942.qrB1amZM-lkp@intel.com/
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index b70ddd33e9ed..fb5af8a34c8f 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -5752,10 +5752,9 @@ static int bnxt_hwrm_set_vnic_filter(struct bnxt *bp, u16 vnic_id, u16 idx,
 	return rc;
 }
 
-static int bnxt_hwrm_clear_vnic_filter(struct bnxt *bp)
+static void bnxt_hwrm_clear_vnic_filter(struct bnxt *bp)
 {
 	u16 i, j, num_of_vnics = 1; /* only vnic 0 supported */
-	int rc = 0;
 
 	/* Any associated ntuple filters will also be cleared by firmware. */
 	for (i = 0; i < num_of_vnics; i++) {
@@ -5769,8 +5768,6 @@ static int bnxt_hwrm_clear_vnic_filter(struct bnxt *bp)
 		}
 		vnic->uc_filter_count = 0;
 	}
-
-	return rc;
 }
 
 #define BNXT_DFLT_TUNL_TPA_BMAP				\
-- 
2.30.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

Re: [PATCH net-next 1/3] bnxt_en: Remove unneeded variable in bnxt_hwrm_clear_vnic_filter()

[Simon Horman]

On Fri, Jan 05, 2024 at 03:54:37PM -0800, Michael Chan wrote:
> After recent refactoring, this function doesn't return error any
> more.  Remove the unneeded rc variable and change the function to
> void.  The caller is not checking for the return value.
> 
> Fixes: 96c9bedc755e ("bnxt_en: Refactor L2 filter alloc/free firmware commands.")
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202401041942.qrB1amZM-lkp@intel.com/
> Signed-off-by: Michael Chan <michael.chan@broadcom.com>

Hi Michael,

I'm not sure this is a bug fix, so I might have cited
the commit using something like "Introduced by commit ..."
rather than a Fixes tag.

But the fix isn't going to propagate very far anyway,
as the cited commit is currently only in net-next.
So perhaps it is fine as is.

In any case, I agree that this is a nice update to the code.

Reviewed-by: Simon Horman <horms@kernel.org>

[PATCH net-next 2/3] bnxt_en: Fix RCU locking for ntuple filters in bnxt_srxclsrldel()

[Michael Chan]

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

After looking up an ntuple filter from a RCU hash list, the
rcu_read_unlock() call should be made after reading the structure,
or after determining that the filter cannot age out (by aRFS).
The existing code was calling rcu_read_unlock() too early in
bnxt_srxclsrldel().

As suggested by Simon Horman, change the code to handle the error
case of fltr_base not found in the if condition.  The code looks
cleaner this way.

Fixes: 8d7ba028aa9a ("bnxt_en: Add support for ntuple filter deletion by ethtool.")
Suggested-by: Simon Horman <horms@kernel.org>
Reported-by: Jakub Kicinski <kuba@kernel.org>
Link: https://lore.kernel.org/netdev/20240104145955.5a6df702@kernel.org/
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 21 ++++++++++---------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
index 5629ba9f4b2e..27b983c0a8a9 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
@@ -1345,25 +1345,26 @@ static int bnxt_srxclsrldel(struct bnxt *bp, struct ethtool_rxnfc *cmd)
 {
 	struct ethtool_rx_flow_spec *fs = &cmd->fs;
 	struct bnxt_filter_base *fltr_base;
+	struct bnxt_ntuple_filter *fltr;
 
 	rcu_read_lock();
 	fltr_base = bnxt_get_one_fltr_rcu(bp, bp->ntp_fltr_hash_tbl,
 					  BNXT_NTP_FLTR_HASH_SIZE,
 					  fs->location);
-	if (fltr_base) {
-		struct bnxt_ntuple_filter *fltr;
-
-		fltr = container_of(fltr_base, struct bnxt_ntuple_filter, base);
+	if (!fltr_base) {
 		rcu_read_unlock();
-		if (!(fltr->base.flags & BNXT_ACT_NO_AGING))
-			return -EINVAL;
-		bnxt_hwrm_cfa_ntuple_filter_free(bp, fltr);
-		bnxt_del_ntp_filter(bp, fltr);
-		return 0;
+		return -ENOENT;
 	}
 
+	fltr = container_of(fltr_base, struct bnxt_ntuple_filter, base);
+	if (!(fltr->base.flags & BNXT_ACT_NO_AGING)) {
+		rcu_read_unlock();
+		return -EINVAL;
+	}
 	rcu_read_unlock();
-	return -ENOENT;
+	bnxt_hwrm_cfa_ntuple_filter_free(bp, fltr);
+	bnxt_del_ntp_filter(bp, fltr);
+	return 0;
 }
 
 static u64 get_ethtool_ipv4_rss(struct bnxt *bp)
-- 
2.30.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

Re: [PATCH net-next 2/3] bnxt_en: Fix RCU locking for ntuple filters in bnxt_srxclsrldel()

[Simon Horman]

On Fri, Jan 05, 2024 at 03:54:38PM -0800, Michael Chan wrote:
> After looking up an ntuple filter from a RCU hash list, the
> rcu_read_unlock() call should be made after reading the structure,
> or after determining that the filter cannot age out (by aRFS).
> The existing code was calling rcu_read_unlock() too early in
> bnxt_srxclsrldel().
> 
> As suggested by Simon Horman, change the code to handle the error
> case of fltr_base not found in the if condition.  The code looks
> cleaner this way.
> 
> Fixes: 8d7ba028aa9a ("bnxt_en: Add support for ntuple filter deletion by ethtool.")
> Suggested-by: Simon Horman <horms@kernel.org>
> Reported-by: Jakub Kicinski <kuba@kernel.org>
> Link: https://lore.kernel.org/netdev/20240104145955.5a6df702@kernel.org/
> Signed-off-by: Michael Chan <michael.chan@broadcom.com>

Thanks Michael,

I agree that this addresses the issue flagged at the Link above.
That it is a bug-fix, and should have a Fixes tag.
And that as the cited commit has not propagated beyond net-next
it is appropriate to target this patch at net-next.

FWIIW, I might have separated the bug fix from the code re-arrangement that
I suggested. But perhaps that is over doing things as the patch is for
net-next anyway.

Reviewed-by: Simon Horman <horms@kernel.org>

[PATCH net-next 3/3] bnxt_en: Fix RCU locking for ntuple filters in bnxt_rx_flow_steer()

[Michael Chan]

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

Similar to the previous patch, RCU locking was released too early
in bnxt_rx_flow_steer().  Fix it to unlock after reading fltr->base.sw_id
to guarantee that fltr won't be freed while we are still reading it.

Fixes: cb5bdd292dc0 ("bnxt_en: Add bnxt_lookup_ntp_filter_from_idx() function")
Reported-by: Simon Horman <horms@kernel.org>
Link: https://lore.kernel.org/netdev/20231225165653.GH5962@kernel.org/
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index fb5af8a34c8f..0aacd3c6ed5c 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -14020,8 +14020,8 @@ static int bnxt_rx_flow_steer(struct net_device *dev, const struct sk_buff *skb,
 	rcu_read_lock();
 	fltr = bnxt_lookup_ntp_filter_from_idx(bp, new_fltr, idx);
 	if (fltr) {
-		rcu_read_unlock();
 		rc = fltr->base.sw_id;
+		rcu_read_unlock();
 		goto err_free;
 	}
 	rcu_read_unlock();
-- 
2.30.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

Re: [PATCH net-next 3/3] bnxt_en: Fix RCU locking for ntuple filters in bnxt_rx_flow_steer()

[Simon Horman]

On Fri, Jan 05, 2024 at 03:54:39PM -0800, Michael Chan wrote:
> Similar to the previous patch, RCU locking was released too early
> in bnxt_rx_flow_steer().  Fix it to unlock after reading fltr->base.sw_id
> to guarantee that fltr won't be freed while we are still reading it.
> 
> Fixes: cb5bdd292dc0 ("bnxt_en: Add bnxt_lookup_ntp_filter_from_idx() function")
> Reported-by: Simon Horman <horms@kernel.org>
> Link: https://lore.kernel.org/netdev/20231225165653.GH5962@kernel.org/
> Signed-off-by: Michael Chan <michael.chan@broadcom.com>

Thanks Michael,

I agree that this addresses the issue flagged at the Link above.
That it is a bug-fix, and should have a Fixes tag.
And that as the cited commit has not propagated beyond net-next
it is appropriate to target this patch at net-next.

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next 0/3] bnxt_en: ntuple filter fixes

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Fri,  5 Jan 2024 15:54:36 -0800 you wrote:
> The first patch is to remove an unneeded variable.  The next 2 patches
> are to release RCU lock correctly after accesing the RCU protected
> filter structure.  Patch 2 also re-arranges the code to look cleaner.
> 
> Michael Chan (3):
>   bnxt_en: Remove unneeded variable in bnxt_hwrm_clear_vnic_filter()
>   bnxt_en: Fix RCU locking for ntuple filters in bnxt_srxclsrldel()
>   bnxt_en: Fix RCU locking for ntuple filters in bnxt_rx_flow_steer()
> 
> [...]

Here is the summary with links:
  - [net-next,1/3] bnxt_en: Remove unneeded variable in bnxt_hwrm_clear_vnic_filter()
    https://git.kernel.org/netdev/net-next/c/1ef4cacaae2f
  - [net-next,2/3] bnxt_en: Fix RCU locking for ntuple filters in bnxt_srxclsrldel()
    https://git.kernel.org/netdev/net-next/c/fd7769798de8
  - [net-next,3/3] bnxt_en: Fix RCU locking for ntuple filters in bnxt_rx_flow_steer()
    https://git.kernel.org/netdev/net-next/c/d8214d0f0135

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html