netdev - Re: [PATCH net-next 2/7] i40e: Add placeholder for ndo set VLANs

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: <20220106203254.1c6159fb@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Thu, 6 Jan 2022 20:32:54 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Tony Nguyen <anthony.l.nguyen@...el.com>
Cc:     davem@...emloft.net, Karen Sornek <karen.sornek@...el.com>,
        netdev@...r.kernel.org, sassmann@...hat.com,
        Przemyslaw Patynowski <przemyslawx.patynowski@...el.com>,
        Konrad Jankowski <konrad0.jankowski@...el.com>
Subject: Re: [PATCH net-next 2/7] i40e: Add placeholder for ndo set VLANs

On Thu,  6 Jan 2022 13:32:56 -0800 Tony Nguyen wrote:
> From: Karen Sornek <karen.sornek@...el.com>
> 
> VLANs set by ndo, were not accounted.
> Implement placeholder, by which driver can account VLANs set by
> ndo. Ensure that once PF changes trunk, every guest filter
> is removed from the list 'vm_vlan_list'.
> Implement logic for deletion/addition of guest(from VM) filters.

I could not understand what this change is achieving from reading this.

> +/**
> + * i40e_add_vmvlan_to_list
> + * @vf: pointer to the VF info
> + * @vfl:  pointer to the VF VLAN tag filters list
> + * @vlan_idx: vlan_id index in VLAN tag filters list
> + *
> + * add VLAN tag into the VLAN list for VM
> + **/
> +static i40e_status
> +i40e_add_vmvlan_to_list(struct i40e_vf *vf,
> +			struct virtchnl_vlan_filter_list *vfl,
> +			u16 vlan_idx)
> +{
> +	struct i40e_vm_vlan *vlan_elem;
> +
> +	vlan_elem = kzalloc(sizeof(*vlan_elem), GFP_KERNEL);
> +	if (!vlan_elem)
> +		return I40E_ERR_NO_MEMORY;
> +	vlan_elem->vlan = vfl->vlan_id[vlan_idx];
> +	vlan_elem->vsi_id = vfl->vsi_id;
> +	INIT_LIST_HEAD(&vlan_elem->list);
> +	vf->num_vlan++;
> +	list_add(&vlan_elem->list, &vf->vm_vlan_list);
> +	return 0;

Why not call i40e_vsi_add_vlan() here?

i40e_del_vmvlan_from_list() calls i40e_vsi_kill_vlan(), the functions
are not symmetric.

> +}
> +
> +/**
> + * i40e_del_vmvlan_from_list
> + * @vsi: pointer to the VSI structure
> + * @vf: pointer to the VF info
> + * @vlan: VLAN tag to be removed from the list
> + *
> + * delete VLAN tag from the VLAN list for VM
> + **/
> +static void i40e_del_vmvlan_from_list(struct i40e_vsi *vsi,
> +				      struct i40e_vf *vf, u16 vlan)
> +{
> +	struct i40e_vm_vlan *entry, *tmp;
> +
> +	list_for_each_entry_safe(entry, tmp, &vf->vm_vlan_list, list) {
> +		if (vlan == entry->vlan) {
> +			i40e_vsi_kill_vlan(vsi, vlan);
> +			vf->num_vlan--;
> +			list_del(&entry->list);
> +			kfree(entry);
> +			break;
> +		}
> +	}
> +}
> +
> +/**
> + * i40e_free_vmvlan_list
> + * @vsi: pointer to the VSI structure
> + * @vf: pointer to the VF info
> + *
> + * remove whole list of VLAN tags for VM
> + **/
> +static void i40e_free_vmvlan_list(struct i40e_vsi *vsi, struct i40e_vf *vf)
> +{
> +	struct i40e_vm_vlan *entry, *tmp;
> +
> +	if (list_empty(&vf->vm_vlan_list))
> +		return;
> +
> +	list_for_each_entry_safe(entry, tmp, &vf->vm_vlan_list, list) {
> +		if (vsi)

This function is only called with vsi = NULL AFAICT.

Please remove all dead code.

> +			i40e_vsi_kill_vlan(vsi, entry->vlan);
> +		list_del(&entry->list);
> +		kfree(entry);
> +	}
> +	vf->num_vlan = 0;
> +}
> +
>  /**
>   * i40e_free_vf_res
>   * @vf: pointer to the VF info


> @@ -2969,12 +3046,13 @@ static int i40e_vc_add_vlan_msg(struct i40e_vf *vf, u8 *msg)
>  	struct i40e_pf *pf = vf->pf;
>  	struct i40e_vsi *vsi = NULL;
>  	i40e_status aq_ret = 0;
> -	int i;
> +	u16 i;
>  
> -	if ((vf->num_vlan >= I40E_VC_MAX_VLAN_PER_VF) &&
> +	if ((vf->num_vlan + vfl->num_elements > I40E_VC_MAX_VLAN_PER_VF) &&
>  	    !test_bit(I40E_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps)) {
>  		dev_err(&pf->pdev->dev,
>  			"VF is not trusted, switch the VF to trusted to add more VLAN addresses\n");
> +		aq_ret = I40E_ERR_CONFIG;

seems unrelated

>  		goto error_param;
>  	}
>  	if (!test_bit(I40E_VF_STATE_ACTIVE, &vf->vf_states) ||