Re: [PATCH net-next 4/4] i40e: Avoid repeating RX filter warning

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] The fix for repeating RX filter warnings is incomplete and leaves SRIOV/VMDQ VSIs vulnerable to log spam, exacerbated by unconditional state clearing for untrusted VFs.

Pre-existing issues:
- [High] The function `i40e_sync_vsi_filters` implements an ad-hoc sleeping spinlock using a bit flag (`__I40E_VSI_SYNCING_FILTERS`), violating subsystem locking guidelines.
--

commit e12ee63ee282f8dda3570d0ba1ab6d4fc31ab63e
Author: Chris Packham <chris.packham@alliedtelesis.co.nz>
Subject: i40e: Avoid repeating RX filter warning

This patch introduces a test_and_set_bit check on the overflow promiscuous
state flag to suppress repetitive log messages when the driver forces
overflow promiscuous mode due to a lack of space for RX filters. It updates
the warning logic in both i40e_aqc_add_filters and i40e_aqc_broadcast_filter.

> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index 719f45b5cb409..abbc71e815ae3 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
[ ... ]
> @@ -2412,10 +2412,12 @@ void i40e_aqc_add_filters(struct i40e_vsi *vsi, const char *vsi_name,
>  
>  	if (fcnt != num_add) {
>  		if (vsi->type == I40E_VSI_MAIN) {
> -			set_bit(__I40E_VSI_OVERFLOW_PROMISC, vsi->state);
> -			dev_warn(&vsi->back->pdev->dev,
> -				 "Error %s adding RX filters on %s, promiscuous mode forced on\n",
> -				 libie_aq_str(aq_status), vsi_name);
> +			if (!test_and_set_bit(__I40E_VSI_OVERFLOW_PROMISC,
> +					      vsi->state)) {
> +				dev_warn(&vsi->back->pdev->dev,
> +					 "Error %s adding RX filters on %s, promiscuous mode forced on\n",
> +					 libie_aq_str(aq_status), vsi_name);
> +			}
>  		} else if (vsi->type == I40E_VSI_SRIOV ||
>  			   vsi->type == I40E_VSI_VMDQ1 ||
>  			   vsi->type == I40E_VSI_VMDQ2) {

[Severity: High]
Does this logic leave virtual function and vmdq VSIs vulnerable to log spam?
The patch suppresses the warning for the main VSI by checking
test_and_set_bit, but the else if branch for the other VSI types immediately
below it does not use a state check.

Should the state-based warning suppression be enforced equally across all VSI
types?

This regression seems exacerbated by the unconditional state clearing for
untrusted virtual functions in i40e_sync_vsi_filters:

drivers/net/ethernet/intel/i40e/i40e_main.c:i40e_sync_vsi_filters() {
    ...
	/* if the VF is not trusted do not do promisc */
	if (vsi->type == I40E_VSI_SRIOV && pf->vf &&
	    !pf->vf[vsi->vf_id].trusted) {
		clear_bit(__I40E_VSI_OVERFLOW_PROMISC, vsi->state);
		goto out;
	}
    ...
}

Does this unconditional state clearing destroy the warning suppression logic
for untrusted virtual functions, potentially leading to a denial of service
via log spam?

[Severity: High]
This is a pre-existing issue, but while reviewing the synchronization of
these filters I noticed the following pattern in i40e_sync_vsi_filters:

drivers/net/ethernet/intel/i40e/i40e_main.c:i40e_sync_vsi_filters() {
	...
	while (test_and_set_bit(__I40E_VSI_SYNCING_FILTERS, vsi->state))
		usleep_range(1000, 2000);
	...
out:
	...
	clear_bit(__I40E_VSI_SYNCING_FILTERS, vsi->state);
	return retval;
	...
}

Does this implement an ad-hoc sleeping spinlock using a bit flag, violating
the subsystem guidelines for synchronization?

Would it be better to directly replace this custom polling loop with a
standard mutex to ensure strict lockdep tracking and memory ordering?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901211752.1922993-1-anthony.l.nguyen@intel.com?part=4