re: net: dsa: mv88e6xxx: Add devlink regions

re: net: dsa: mv88e6xxx: Add devlink regions

[Colin King (gmail)]

Hi Andrew,

Static analysis on linux-next has detected a potential issue with an 
function returning an uninitialized value from function 
mv88e6xxx_region_atu_snapshot in drivers/net/dsa/mv88e6xxx/devlink.c

The commit in question is:

commit bfb255428966e2ab2c406cf6c71d95e9e63241e4
Author: Andrew Lunn <andrew@lunn.ch>
Date:   Fri Sep 18 21:11:07 2020 +0200

     net: dsa: mv88e6xxx: Add devlink regions

Variable err is not being initialized at the start of the function. In 
the following while-loop err is not being assigned if id == 
MV88E6XXX_N_FID because of the early break out of the loop. This can end 
up with the function returning an uninitialized value in err.

I'm not sure of this is ever going to happen, or if in this case this is 
an error condition or not, so I'm unsure if err should be initialized to 
zero or some other value.

         while (1) {
                 fid = find_next_bit(chip->fid_bitmap, MV88E6XXX_N_FID, 
fid + 1);
                 if (fid == MV88E6XXX_N_FID)
                         break;

                 err =  mv88e6xxx_region_atu_snapshot_fid(chip, fid, table,
                                                          &count);
                 if (err) {
                         kfree(table);
                         goto out;
                 }
         }
         *data = (u8 *)table;
out:
         mv88e6xxx_reg_unlock(chip);

         return err;

Regards,

Colin

Re: net: dsa: mv88e6xxx: Add devlink regions

[Andrew Lunn]

On Wed, Oct 09, 2024 at 05:20:09PM +0100, Colin King (gmail) wrote:
> Hi Andrew,
> 
> Static analysis on linux-next has detected a potential issue with an
> function returning an uninitialized value from function
> mv88e6xxx_region_atu_snapshot in drivers/net/dsa/mv88e6xxx/devlink.c
> 
> The commit in question is:
> 
> commit bfb255428966e2ab2c406cf6c71d95e9e63241e4
> Author: Andrew Lunn <andrew@lunn.ch>
> Date:   Fri Sep 18 21:11:07 2020 +0200
> 
>     net: dsa: mv88e6xxx: Add devlink regions
> 
> Variable err is not being initialized at the start of the function. In the
> following while-loop err is not being assigned if id == MV88E6XXX_N_FID
> because of the early break out of the loop. This can end up with the
> function returning an uninitialized value in err.

Hi Colin

That is an old commit. I doubt that is the actual cause. What the real
problem is:

Commit ada5c3229b32e48f4c8e09b6937e5ad98cc3675f
Author: Aryan Srivastava <aryan.srivastava@alliedtelesis.co.nz>
Date:   Mon Oct 7 10:29:05 2024 +1300

    net: dsa: mv88e6xxx: Add FID map cache
    
    Add a cached FID bitmap. This mitigates the need to walk all VTU entries
    to find the next free FID.
    
    When flushing the VTU (during init), zero the FID bitmap. Use and
    manipulate this bitmap from now on, instead of reading HW for the FID
    map.
    
    The repeated VTU walks are costly and can take ~40 mins if ~4000 vlans
    are added. Caching the FID map reduces this time to <2 mins.
    
    Signed-off-by: Aryan Srivastava <aryan.srivastava@alliedtelesis.co.nz>
    Reviewed-by: Andrew Lunn <andrew@lunn.ch>
    Link: https://patch.msgid.link/20241006212905.3142976-1-aryan.srivastava@alliedtelesis.co.nz
    Signed-off-by: Jakub Kicinski <kuba@kernel.org>

diff --git a/drivers/net/dsa/mv88e6xxx/devlink.c b/drivers/net/dsa/mv88e6xxx/devlink.c
index a08dab75e0c0..ef3643bc43db 100644
--- a/drivers/net/dsa/mv88e6xxx/devlink.c
+++ b/drivers/net/dsa/mv88e6xxx/devlink.c
@@ -374,7 +374,6 @@ static int mv88e6xxx_region_atu_snapshot(struct devlink *dl,
                                         u8 **data)
 {
        struct dsa_switch *ds = dsa_devlink_to_ds(dl);
-       DECLARE_BITMAP(fid_bitmap, MV88E6XXX_N_FID);
        struct mv88e6xxx_devlink_atu_entry *table;
        struct mv88e6xxx_chip *chip = ds->priv;
        int fid = -1, count, err;
@@ -392,14 +391,8 @@ static int mv88e6xxx_region_atu_snapshot(struct devlink *dl,
 
        mv88e6xxx_reg_lock(chip);
 
-       err = mv88e6xxx_fid_map(chip, fid_bitmap);
-       if (err) {
-               kfree(table);
-               goto out;
-       }
-

The removed code used to ensure err was initialized.

Aryan, please could you fix this.

	Andrew