netdev - Re: [PATCH] PATCH: i40e Improve trusted VF MAC addresses logging when limit is reached

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250805093145.GV8494@horms.kernel.org>
Date: Tue, 5 Aug 2025 10:31:45 +0100
From: Simon Horman <horms@...nel.org>
To: David Hill <dhill@...hat.com>
Cc: netdev@...r.kernel.org, anthony.l.nguyen@...el.com,
	przemyslaw.kitszel@...el.com, andrew+netdev@...n.ch,
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com
Subject: Re: [PATCH] PATCH: i40e Improve trusted VF MAC addresses logging
 when limit is reached

On Sat, Aug 02, 2025 at 10:13:22AM -0400, David Hill wrote:
> When a VF reaches the limit introduced in this commit [1], the host reports
> an error in the syslog but doesn't mention which VF reached its limit and
> what the limit is actually is which makes troubleshooting of networking
> issue a bit tedious.   This commit simply improves this error reporting
> by adding which VF number has reached a limit and what that limit is.
> 
> Signed-off-by: David Hill <dhill@...hat.com>
> 
> [1] commit cfb1d572c986a39fd288f48a6305d81e6f8d04a3
> Author: Karen Sornek <karen.sornek@...el.com>
> Date:   Thu Jun 17 09:19:26 2021 +0200

Hi David,

Your Signed-off-by, and any other tags, should come at the end of
the commit message. In this case, immediately before the scissors ("---").

And the correct form for a commit citation in a commit message is like
this. Note: There should be 12 or more characters of hash - usually 12 is
enough to prevent a collision with current hashes; And, unlike a Fixes
tag it should be line wrapped as appropriate.

commit cfb1d572c986 ("i40e: Add ensurance of MacVlan resources for every
trusted VF")

> ---
>  drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> index 9b8efdeafbcf..44e3e75e8fb0 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> @@ -2953,7 +2953,8 @@ static inline int i40e_check_vf_permission(struct i40e_vf *vf,
>  		    I40E_VC_MAX_MACVLAN_PER_TRUSTED_VF(pf->num_alloc_vfs,
>  						       hw->num_ports)) {
>  			dev_err(&pf->pdev->dev,
> -				"Cannot add more MAC addresses, trusted VF exhausted it's resources\n");
> +				"Cannot add more MAC addresses, trusted VF %d uses %d out of %d MAC addresses\n", vf->vf_id, i40e_count_filters(vsi) +
> +          mac2add_cnt, I40E_VC_MAX_MACVLAN_PER_TRUSTED_VF(pf->num_alloc_vfs,hw->num_ports));
>  			return -EPERM;
>  		}
>  	}

I am wondering if we can achieve the same in a slightly less verbose way.
And follow more closely the preference in Networking code to keep lines
to 80 columns wide or less, unless it reduces readability (subjective to to
be sure).

Something like this (compile tested only!):

diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
index 9b8efdeafbcf..874a0d907496 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
@@ -2896,8 +2896,8 @@ static inline int i40e_check_vf_permission(struct i40e_vf *vf,
 {
 	struct i40e_pf *pf = vf->pf;
 	struct i40e_vsi *vsi = pf->vsi[vf->lan_vsi_idx];
+	int mac2add_cnt = i40e_count_filters(vsi);
 	struct i40e_hw *hw = &pf->hw;
-	int mac2add_cnt = 0;
 	int i;
 
 	for (i = 0; i < al->num_elements; i++) {
@@ -2937,8 +2937,7 @@ static inline int i40e_check_vf_permission(struct i40e_vf *vf,
 	 * push us over the limit.
 	 */
 	if (!test_bit(I40E_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps)) {
-		if ((i40e_count_filters(vsi) + mac2add_cnt) >
-		    I40E_VC_MAX_MAC_ADDR_PER_VF) {
+		if (mac2add_cnt > I40E_VC_MAX_MAC_ADDR_PER_VF) {
 			dev_err(&pf->pdev->dev,
 				"Cannot add more MAC addresses, VF is not trusted, switch the VF to trusted to add more functionality\n");
 			return -EPERM;
@@ -2949,11 +2948,14 @@ static inline int i40e_check_vf_permission(struct i40e_vf *vf,
 	 * all VFs.
 	 */
 	} else {
-		if ((i40e_count_filters(vsi) + mac2add_cnt) >
-		    I40E_VC_MAX_MACVLAN_PER_TRUSTED_VF(pf->num_alloc_vfs,
-						       hw->num_ports)) {
+		int add_max;
+
+		add_max = I40E_VC_MAX_MACVLAN_PER_TRUSTED_VF(pf->num_alloc_vfs,
+							     hw->num_ports);
+		if (mac2add_cnt > add_max) {
 			dev_err(&pf->pdev->dev,
-				"Cannot add more MAC addresses, trusted VF exhausted it's resources\n");
+				"Cannot add more MAC addresses, trusted VF %d uses %d out of %d MAC addresses\n",
+				vf->vf_id, mac2add_cnt, add_max);
 			return -EPERM;
 		}
 	}

While sketching-out the above I noticed the dev_err() relating to
undtrusted VFs (around line 2940). And I'm wondering if you think
it makes sense to give it the same treatment that your patch
gives to the dev_err for trusted VFS (around line 2955).