diff mbox series

[net-next,v2,13/13] bnxt_en: Add support for ntuple filter deletion by ethtool.

Message ID 20231223042210.102485-14-michael.chan@broadcom.com (mailing list archive)
State Accepted
Commit 8d7ba028aa9ab6570fe233ae026b3609b46f1eb7
Delegated to: Netdev Maintainers
Headers show
Series bnxt_en: Add basic ntuple filter support | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1113 this patch: 1113
netdev/cc_maintainers success CCed 4 of 4 maintainers
netdev/build_clang success Errors and warnings before: 1140 this patch: 1140
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1141 this patch: 1141
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 41 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Michael Chan Dec. 23, 2023, 4:22 a.m. UTC 
Add logic to delete a user specified ntuple filter from ethtool.

Reviewed-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 29 +++++++++++++++++++
 1 file changed, 29 insertions(+)

Comments

Simon Horman Dec. 25, 2023, 5:41 p.m. UTC | #1 
On Fri, Dec 22, 2023 at 08:22:10PM -0800, Michael Chan wrote:
> Add logic to delete a user specified ntuple filter from ethtool.
> 
> Reviewed-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
> Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
> Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
> Signed-off-by: Michael Chan <michael.chan@broadcom.com>
> ---
>  .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 29 +++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> index c3b9be328b87..5629ba9f4b2e 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> @@ -1341,6 +1341,31 @@ static int bnxt_srxclsrlins(struct bnxt *bp, struct ethtool_rxnfc *cmd)
>  	return rc;
>  }
>  
> +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;
> +
> +	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);
> +		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;
> +	}
> +
> +	rcu_read_unlock();
> +	return -ENOENT;
> +}
> +

Hi Michael,

FWIIW, I think it would be more idoiomatic to handle
the error case inside the if condition.

(Completely untested!)

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) {
		rcu_read_unlock();
		return -ENOENT;
	}

	fltr = container_of(fltr_base, struct bnxt_ntuple_filter, 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;
}

...
Jakub Kicinski Jan. 4, 2024, 10:59 p.m. UTC | #2 
On Fri, 22 Dec 2023 20:22:10 -0800 Michael Chan wrote:
> +	if (fltr_base) {
> +		struct bnxt_ntuple_filter *fltr;
> +
> +		fltr = container_of(fltr_base, struct bnxt_ntuple_filter, base);
> +		rcu_read_unlock();
> +		if (!(fltr->base.flags & BNXT_ACT_NO_AGING))
> +			return -EINVAL;

This looks pretty suspicious, you drop the RCU lock before ever using
the object. I'm guessing the filter may be form aRFS and that's why
we need RCU? Shouldn't you hold the RCU lock when checking that
NO_AGING is set? If it's an aRFS flow it may disappear..
Michael Chan Jan. 4, 2024, 11:37 p.m. UTC | #3 
On Thu, Jan 4, 2024 at 2:59 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri, 22 Dec 2023 20:22:10 -0800 Michael Chan wrote:
> > +     if (fltr_base) {
> > +             struct bnxt_ntuple_filter *fltr;
> > +
> > +             fltr = container_of(fltr_base, struct bnxt_ntuple_filter, base);
> > +             rcu_read_unlock();
> > +             if (!(fltr->base.flags & BNXT_ACT_NO_AGING))
> > +                     return -EINVAL;
>
> This looks pretty suspicious, you drop the RCU lock before ever using
> the object. I'm guessing the filter may be form aRFS and that's why
> we need RCU? Shouldn't you hold the RCU lock when checking that
> NO_AGING is set? If it's an aRFS flow it may disappear..

I think you are right.  The RCU lock should be released after checking
the flags.

There is also another unused variable detected by the kernel test
robot.  I will post the fixes in a day or two.  Thanks.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
index c3b9be328b87..5629ba9f4b2e 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
@@ -1341,6 +1341,31 @@  static int bnxt_srxclsrlins(struct bnxt *bp, struct ethtool_rxnfc *cmd)
 	return rc;
 }
 
+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;
+
+	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);
+		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;
+	}
+
+	rcu_read_unlock();
+	return -ENOENT;
+}
+
 static u64 get_ethtool_ipv4_rss(struct bnxt *bp)
 {
 	if (bp->rss_hash_cfg & VNIC_RSS_CFG_REQ_HASH_TYPE_IPV4)
@@ -1532,6 +1557,10 @@  static int bnxt_set_rxnfc(struct net_device *dev, struct ethtool_rxnfc *cmd)
 		rc = bnxt_srxclsrlins(bp, cmd);
 		break;
 
+	case ETHTOOL_SRXCLSRLDEL:
+		rc = bnxt_srxclsrldel(bp, cmd);
+		break;
+
 	default:
 		rc = -EOPNOTSUPP;
 		break;