diff mbox series

[net-next,4/6,v6] net: dsa: rtl8366rb: Fix off-by-one bug

Message ID 20210925132311.2040272-5-linus.walleij@linaro.org
State Superseded
Headers show
Series RTL8366(RB) cleanups part 1 | expand

Commit Message

Linus Walleij Sept. 25, 2021, 1:23 p.m. UTC
The max VLAN number with non-4K VLAN activated is 15, and the
range is 0..15. Not 16.

The impact should be low since we by default have 4K VLAN and
thus have 4095 VLANs to play with in this switch. There will
not be a problem unless the code is rewritten to only use
16 VLANs.

Fixes: d8652956cf37 ("net: dsa: realtek-smi: Add Realtek SMI driver")
Cc: Vladimir Oltean <olteanv@gmail.com>
Cc: Mauri Sandberg <sandberg@mailfence.com>
Cc: Alvin Šipraga <alsi@bang-olufsen.dk>
Cc: DENG Qingfang <dqfext@gmail.com>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

---
ChangeLog v5->v6:
- No changes just resending with the rest of the
  patches.
ChangeLog v4->v5:
- Add some more text describing that this is not a critical bug.
- Add Fixes tag
ChangeLog v1->v4:
- New patch for a bug discovered when fixing the other issues.
---
 drivers/net/dsa/rtl8366rb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.31.1

Comments

Alvin Šipraga Sept. 25, 2021, 5:16 p.m. UTC | #1
On 9/25/21 3:23 PM, Linus Walleij wrote:
> The max VLAN number with non-4K VLAN activated is 15, and the

> range is 0..15. Not 16.

> 

> The impact should be low since we by default have 4K VLAN and

> thus have 4095 VLANs to play with in this switch. There will

> not be a problem unless the code is rewritten to only use

> 16 VLANs.

> 

> Fixes: d8652956cf37 ("net: dsa: realtek-smi: Add Realtek SMI driver")

> Cc: Vladimir Oltean <olteanv@gmail.com>

> Cc: Mauri Sandberg <sandberg@mailfence.com>

> Cc: Alvin Šipraga <alsi@bang-olufsen.dk>

> Cc: DENG Qingfang <dqfext@gmail.com>

> Cc: Florian Fainelli <f.fainelli@gmail.com>

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

> ---


Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk>


> ChangeLog v5->v6:

> - No changes just resending with the rest of the

>    patches.

> ChangeLog v4->v5:

> - Add some more text describing that this is not a critical bug.

> - Add Fixes tag

> ChangeLog v1->v4:

> - New patch for a bug discovered when fixing the other issues.

> ---

>   drivers/net/dsa/rtl8366rb.c | 2 +-

>   1 file changed, 1 insertion(+), 1 deletion(-)

> 

> diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c

> index 2c66a0c2ee50..6f25ee57069d 100644

> --- a/drivers/net/dsa/rtl8366rb.c

> +++ b/drivers/net/dsa/rtl8366rb.c

> @@ -1450,7 +1450,7 @@ static int rtl8366rb_set_mc_index(struct realtek_smi *smi, int port, int index)

>   

>   static bool rtl8366rb_is_vlan_valid(struct realtek_smi *smi, unsigned int vlan)

>   {

> -	unsigned int max = RTL8366RB_NUM_VLANS;

> +	unsigned int max = RTL8366RB_NUM_VLANS - 1;

>   

>   	if (smi->vlan4k_enabled)

>   		max = RTL8366RB_NUM_VIDS - 1;

>
Vladimir Oltean Sept. 25, 2021, 6:44 p.m. UTC | #2
On Sat, Sep 25, 2021 at 03:23:09PM +0200, Linus Walleij wrote:
> The max VLAN number with non-4K VLAN activated is 15, and the

> range is 0..15. Not 16.

> 

> The impact should be low since we by default have 4K VLAN and

> thus have 4095 VLANs to play with in this switch. There will

> not be a problem unless the code is rewritten to only use

> 16 VLANs.

> 

> Fixes: d8652956cf37 ("net: dsa: realtek-smi: Add Realtek SMI driver")

> Cc: Vladimir Oltean <olteanv@gmail.com>

> Cc: Mauri Sandberg <sandberg@mailfence.com>

> Cc: Alvin Šipraga <alsi@bang-olufsen.dk>

> Cc: DENG Qingfang <dqfext@gmail.com>

> Cc: Florian Fainelli <f.fainelli@gmail.com>

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

> ---

> ChangeLog v5->v6:

> - No changes just resending with the rest of the

>   patches.

> ChangeLog v4->v5:

> - Add some more text describing that this is not a critical bug.

> - Add Fixes tag

> ChangeLog v1->v4:

> - New patch for a bug discovered when fixing the other issues.

> ---

>  drivers/net/dsa/rtl8366rb.c | 2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)

> 

> diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c

> index 2c66a0c2ee50..6f25ee57069d 100644

> --- a/drivers/net/dsa/rtl8366rb.c

> +++ b/drivers/net/dsa/rtl8366rb.c

> @@ -1450,7 +1450,7 @@ static int rtl8366rb_set_mc_index(struct realtek_smi *smi, int port, int index)

>  

>  static bool rtl8366rb_is_vlan_valid(struct realtek_smi *smi, unsigned int vlan)

>  {

> -	unsigned int max = RTL8366RB_NUM_VLANS;

> +	unsigned int max = RTL8366RB_NUM_VLANS - 1;

>  

>  	if (smi->vlan4k_enabled)

>  		max = RTL8366RB_NUM_VIDS - 1;


Personally I would have preferred to make this "max" = RTL8366RB_NUM_VIDS,
and then the comparison right below:

	if (vlan >= max)
		return false;

because it's easier than to carry around "- 1" everywhere.
Anyway, this works just as well.

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
Florian Fainelli Sept. 26, 2021, 2:32 a.m. UTC | #3
On 9/25/2021 6:23 AM, Linus Walleij wrote:
> The max VLAN number with non-4K VLAN activated is 15, and the

> range is 0..15. Not 16.

> 

> The impact should be low since we by default have 4K VLAN and

> thus have 4095 VLANs to play with in this switch. There will

> not be a problem unless the code is rewritten to only use

> 16 VLANs.

> 

> Fixes: d8652956cf37 ("net: dsa: realtek-smi: Add Realtek SMI driver")

> Cc: Vladimir Oltean <olteanv@gmail.com>

> Cc: Mauri Sandberg <sandberg@mailfence.com>

> Cc: Alvin Šipraga <alsi@bang-olufsen.dk>

> Cc: DENG Qingfang <dqfext@gmail.com>

> Cc: Florian Fainelli <f.fainelli@gmail.com>

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>


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

-- 
Florian
diff mbox series

Patch

diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c
index 2c66a0c2ee50..6f25ee57069d 100644
--- a/drivers/net/dsa/rtl8366rb.c
+++ b/drivers/net/dsa/rtl8366rb.c
@@ -1450,7 +1450,7 @@  static int rtl8366rb_set_mc_index(struct realtek_smi *smi, int port, int index)
 
 static bool rtl8366rb_is_vlan_valid(struct realtek_smi *smi, unsigned int vlan)
 {
-	unsigned int max = RTL8366RB_NUM_VLANS;
+	unsigned int max = RTL8366RB_NUM_VLANS - 1;
 
 	if (smi->vlan4k_enabled)
 		max = RTL8366RB_NUM_VIDS - 1;