diff mbox series

dsa: bcm_sf2_cfp: fix an incorrect NULL check on list iterator

Message ID 20220327055547.3938-1-xiam0nd.tong@gmail.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series dsa: bcm_sf2_cfp: fix an incorrect NULL check on list iterator | expand

Checks

Context Check Description
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix warning Target tree name not specified in the subject
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers fail 1 blamed authors not CCed: f.fainelli@gmail.com; 1 maintainers not CCed: f.fainelli@gmail.com
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 17 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/tree_selection success Guessing tree name failed - patch did not apply

Commit Message

Xiaomeng Tong March 27, 2022, 5:55 a.m. UTC
The bug is here:
	return rule;

The list iterator value 'rule' will *always* be set and non-NULL
by list_for_each_entry(), so it is incorrect to assume that the
iterator value will be NULL if the list is empty or no element
is found.

To fix the bug, return 'rule' when found, otherwise return NULL.

Cc: stable@vger.kernel.org
Fixes: ae7a5aff783c7 ("net: dsa: bcm_sf2: Keep copy of inserted rules")
Signed-off-by: Xiaomeng Tong <xiam0nd.tong@gmail.com>
---
 drivers/net/dsa/bcm_sf2_cfp.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Vladimir Oltean March 27, 2022, 6:58 p.m. UTC | #1
On Sun, Mar 27, 2022 at 01:55:47PM +0800, Xiaomeng Tong wrote:
> The bug is here:
> 	return rule;
> 
> The list iterator value 'rule' will *always* be set and non-NULL
> by list_for_each_entry(), so it is incorrect to assume that the
> iterator value will be NULL if the list is empty or no element
> is found.
> 
> To fix the bug, return 'rule' when found, otherwise return NULL.
> 
> Cc: stable@vger.kernel.org
> Fixes: ae7a5aff783c7 ("net: dsa: bcm_sf2: Keep copy of inserted rules")
> Signed-off-by: Xiaomeng Tong <xiam0nd.tong@gmail.com>
> ---

The change looks correct, but from a process standpoint for next time
(a) you should have copied Florian, the driver's maintainer (which I did now)
    who appears on the top of the list in the output of ./get_maintainer.pl
(b) networking bugfixes that apply to the "net" tree shouldn't need
    stable@vger.kernel.org copied, instead just target the patch against
    the https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git
    tree and mark the subject prefix as "[PATCH net]".

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

>  drivers/net/dsa/bcm_sf2_cfp.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/dsa/bcm_sf2_cfp.c b/drivers/net/dsa/bcm_sf2_cfp.c
> index a7e2fcf2df2c..edbe5e7f1cb6 100644
> --- a/drivers/net/dsa/bcm_sf2_cfp.c
> +++ b/drivers/net/dsa/bcm_sf2_cfp.c
> @@ -567,14 +567,14 @@ static void bcm_sf2_cfp_slice_ipv6(struct bcm_sf2_priv *priv,
>  static struct cfp_rule *bcm_sf2_cfp_rule_find(struct bcm_sf2_priv *priv,
>  					      int port, u32 location)
>  {
> -	struct cfp_rule *rule = NULL;
> +	struct cfp_rule *rule;
>  
>  	list_for_each_entry(rule, &priv->cfp.rules_list, next) {
>  		if (rule->port == port && rule->fs.location == location)
> -			break;
> +			return rule;
>  	}
>  
> -	return rule;
> +	return NULL;
>  }
>  
>  static int bcm_sf2_cfp_rule_cmp(struct bcm_sf2_priv *priv, int port,
> -- 
> 2.17.1
>
Xiaomeng Tong March 28, 2022, 2:35 a.m. UTC | #2
On Sun, 27 Mar 2022 21:58:05 +0300, Vladimir Oltean wrote:
> On Sun, Mar 27, 2022 at 01:55:47PM +0800, Xiaomeng Tong wrote:
> > The bug is here:
> > 	return rule;
> > 
> > The list iterator value 'rule' will *always* be set and non-NULL
> > by list_for_each_entry(), so it is incorrect to assume that the
> > iterator value will be NULL if the list is empty or no element
> > is found.
> > 
> > To fix the bug, return 'rule' when found, otherwise return NULL.
> > 
> > Cc: stable@vger.kernel.org
> > Fixes: ae7a5aff783c7 ("net: dsa: bcm_sf2: Keep copy of inserted rules")
> > Signed-off-by: Xiaomeng Tong <xiam0nd.tong@gmail.com>
> > ---
> 
> The change looks correct, but from a process standpoint for next time
> (a) you should have copied Florian, the driver's maintainer (which I did now)
>     who appears on the top of the list in the output of ./get_maintainer.pl
> (b) networking bugfixes that apply to the "net" tree shouldn't need
>     stable@vger.kernel.org copied, instead just target the patch against
>     the https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git
>     tree and mark the subject prefix as "[PATCH net]".
> 
> Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

Thank you for your patient review and good suggestion, i got it.
I'm sorry I forgot to copy Florian.

--
Xiaomeng tong
Florian Fainelli March 28, 2022, 2:51 a.m. UTC | #3
On 3/27/2022 11:58 AM, Vladimir Oltean wrote:
> On Sun, Mar 27, 2022 at 01:55:47PM +0800, Xiaomeng Tong wrote:
>> The bug is here:
>> 	return rule;
>>
>> The list iterator value 'rule' will *always* be set and non-NULL
>> by list_for_each_entry(), so it is incorrect to assume that the
>> iterator value will be NULL if the list is empty or no element
>> is found.
>>
>> To fix the bug, return 'rule' when found, otherwise return NULL.
>>
>> Cc: stable@vger.kernel.org
>> Fixes: ae7a5aff783c7 ("net: dsa: bcm_sf2: Keep copy of inserted rules")
>> Signed-off-by: Xiaomeng Tong <xiam0nd.tong@gmail.com>
>> ---
> 
> The change looks correct, but from a process standpoint for next time
> (a) you should have copied Florian, the driver's maintainer (which I did now)
>      who appears on the top of the list in the output of ./get_maintainer.pl
> (b) networking bugfixes that apply to the "net" tree shouldn't need
>      stable@vger.kernel.org copied, instead just target the patch against
>      the https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git
>      tree and mark the subject prefix as "[PATCH net]".
> 
> Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

If you could please resubmit with the subject being:

net: dsa: bcm_sf2_cfp: fix an incorrect NULL check on list iterator

and add Vladimir's and my tag below:

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

That would be great! Thanks
Xiaomeng Tong March 28, 2022, 3:34 a.m. UTC | #4
On Sun, 27 Mar 2022 19:51:39 -0700, Florian Fainelli wrote:
> If you could please resubmit with the subject being:
> 
> net: dsa: bcm_sf2_cfp: fix an incorrect NULL check on list iterator
> 
> and add Vladimir's and my tag below:
> 
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> 
> That would be great! Thanks

I have resubmited as your suggestion, please check it, thank you.

--
Xiaomeng Tong
diff mbox series

Patch

diff --git a/drivers/net/dsa/bcm_sf2_cfp.c b/drivers/net/dsa/bcm_sf2_cfp.c
index a7e2fcf2df2c..edbe5e7f1cb6 100644
--- a/drivers/net/dsa/bcm_sf2_cfp.c
+++ b/drivers/net/dsa/bcm_sf2_cfp.c
@@ -567,14 +567,14 @@  static void bcm_sf2_cfp_slice_ipv6(struct bcm_sf2_priv *priv,
 static struct cfp_rule *bcm_sf2_cfp_rule_find(struct bcm_sf2_priv *priv,
 					      int port, u32 location)
 {
-	struct cfp_rule *rule = NULL;
+	struct cfp_rule *rule;
 
 	list_for_each_entry(rule, &priv->cfp.rules_list, next) {
 		if (rule->port == port && rule->fs.location == location)
-			break;
+			return rule;
 	}
 
-	return rule;
+	return NULL;
 }
 
 static int bcm_sf2_cfp_rule_cmp(struct bcm_sf2_priv *priv, int port,