netdev - Re: [net v6 1/8] i40e: main driver core

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]

Date:	Sat, 7 Sep 2013 19:20:37 +0000
From:	"Brandeburg, Jesse" <jesse.brandeburg@...el.com>
To:	Daniel Borkmann <dborkman@...hat.com>
CC:	"Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>,
	"davem@...emloft.net" <davem@...emloft.net>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"gospo@...hat.com" <gospo@...hat.com>,
	"sassmann@...hat.com" <sassmann@...hat.com>,
	"Nelson, Shannon" <shannon.nelson@...el.com>,
	"Waskiewicz Jr, Peter P" <peter.p.waskiewicz.jr@...el.com>,
	"e1000-devel@...ts.sourceforge.net" 
	<e1000-devel@...ts.sourceforge.net>
Subject: Re: [net v6 1/8] i40e: main driver core

On Sat, 2013-09-07 at 08:44 +0200, Daniel Borkmann wrote:

> Nitpicking ... at some point in time i40e_status should be removed plus
> I40E_ERR_PARAM, I40E_SUCCESS, I40E_ERR_NO_MEMORY and the like, as we have
> int and -EINVAL, 0, -ENOMEM for that. ;-)

First, thanks Daniel for taking the time to review.

Those are a result of our files that are shared across OSes, as not all
OSes have -ENOMEM etc, we also have a lot of status codes the kernel
doesn't have.  That said, when there is a 1-1 relationship the
replacements should be made.  

> 
> [...]
> +i40e_status i40e_put_mac_in_vlan(struct i40e_vsi *vsi, u8 *macaddr,
> +				 bool is_vf, bool is_netdev)
> +{
> +	struct i40e_mac_filter *f, *add_f;
> +
> +	list_for_each_entry(f, &vsi->mac_filter_list, list) {
> [...]
> +			if (!add_f) {
> +				dev_info(&vsi->back->pdev->dev, "Could not add filter %d for %pM\n",
> +					 f->vlan, f->macaddr);
> +				return -ENOMEM;
> +			}
> +		}
> +	}
> +	return I40E_SUCCESS;
> +}
> 
> Their usage seems also to be mixed anyway: -ENOMEM vs. I40E_SUCCESS.

that's just plain buggy. 


> [...]
> +void i40e_vsi_reset_stats(struct i40e_vsi *vsi)
> +{
> +	struct rtnl_link_stats64 *ns;
> +	int i;
> +
> +	if (!vsi)
> +		return;
> +
> [...]
> +static struct i40e_mac_filter *i40e_find_filter(struct i40e_vsi *vsi,
> +						u8 *macaddr, s16 vlan,
> +						bool is_vf, bool is_netdev)
> +{
> +	struct i40e_mac_filter *f;
> +
> +	if (!vsi || !macaddr)
> +		return NULL;
> [...]
> 
> Probably the code could also be scanned to remove such checks as well ...

do you mean we should not be checking for NULL vsi or NULL macaddr?  We
have several structures in the driver that by design don't have all
elements filled in for certain cases.

I went and looked at the !vsi case and we can definitely remove that in
most cases, as there are several nested checks for null vsi.

thanks again, 
Jesse