netdev - Re: [PATCH net-next 03/11] mlxsw: spectrum

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200818064151.GA214959@shredder>
Date:   Tue, 18 Aug 2020 09:41:51 +0300
From:   Ido Schimmel <idosch@...sch.org>
To:     Bjorn Helgaas <helgaas@...nel.org>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
        jiri@...lanox.com, petrm@...lanox.com, mlxsw@...lanox.com,
        Ido Schimmel <idosch@...lanox.com>
Subject: Re: [PATCH net-next 03/11] mlxsw: spectrum_policer: Add policer core

On Mon, Aug 17, 2020 at 10:38:24AM -0500, Bjorn Helgaas wrote:
> You've likely seen this already, but Coverity found this problem:
> 
>   *** CID 1466147:  Control flow issues  (DEADCODE)
>   /drivers/net/ethernet/mellanox/mlxsw/spectrum_policer.c: 380 in mlxsw_sp_policers_init()
>   374     	}
>   375     
>   376     	return 0;
>   377     
>   378     err_family_register:
>   379     	for (i--; i >= 0; i--) {
>   >>>     CID 1466147:  Control flow issues  (DEADCODE)
>   >>>     Execution cannot reach this statement: "struct mlxsw_sp_policer_fam...".
>   380     		struct mlxsw_sp_policer_family *family;
>   381     
>   382     		family = mlxsw_sp->policer_core->family_arr[i];
>   383     		mlxsw_sp_policer_family_unregister(mlxsw_sp, family);
>   384     	}
>   385     err_init:
> 
> I think the problem is that MLXSW_SP_POLICER_TYPE_MAX is 0 because
> 
> > +enum mlxsw_sp_policer_type {
> > +	MLXSW_SP_POLICER_TYPE_SINGLE_RATE,
> > +
> > +	__MLXSW_SP_POLICER_TYPE_MAX,
> > +	MLXSW_SP_POLICER_TYPE_MAX = __MLXSW_SP_POLICER_TYPE_MAX - 1,
> > +};
> 
> so we can only execute the family_register loop once, with i == 0,
> and if we get to err_family_register via the error exit:
> 
> > +	for (i = 0; i < MLXSW_SP_POLICER_TYPE_MAX + 1; i++) {
> > +		err = mlxsw_sp_policer_family_register(mlxsw_sp, mlxsw_sp_policer_family_arr[i]);
> > +		if (err)
> > +			goto err_family_register;
> 
> i will be 0, so i-- sets i to -1, so we don't enter the
> family_unregister loop body since -1 is not >= 0.

Thanks for the report, but isn't the code doing the right thing here? I
mean, it's dead code now, but as soon as we add another family it will
be executed. It seems error prone to remove it only to please Coverity
and then add it back when it's actually needed.

> 
> This code is now upstream as 8d3fbae70d8d ("mlxsw: spectrum_policer:
> Add policer core").
> 
> Bjorn