netdev - Re: [net-next 2/9] i40e: make visible changed vf mac on host

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: <705015a6-fefe-04fd-12f0-a28e2ba1fb79@pensando.io>
Date:   Fri, 2 Aug 2019 10:00:14 -0700
From:   Shannon Nelson <snelson@...sando.io>
To:     "Loktionov, Aleksandr" <aleksandr.loktionov@...el.com>,
        "Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>,
        "davem@...emloft.net" <davem@...emloft.net>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "nhorman@...hat.com" <nhorman@...hat.com>,
        "sassmann@...hat.com" <sassmann@...hat.com>,
        "Bowers, AndrewX" <andrewx.bowers@...el.com>
Subject: Re: [net-next 2/9] i40e: make visible changed vf mac on host

On 8/2/19 1:14 AM, Loktionov, Aleksandr wrote:
> Good day Nelson

Please don't top post.  The custom on this mailing list is to answer 
inline in order to be sure we're answering in context.  As it is, I 
believe you missed answering one of my questions.

> In 99% cases VF has _only one_ unicast mac anyway, and the last MAC has been chosen because of VF mac address change algo -  it marks unicast filter for deletion and appends a new unicast filter to the list.
> The implementation has been chosen because of simplicity /* Just 3 more lines to solve the issue */, from one point it may look wasteful for some 1% of VF corner cases.
> But from another point of view, more complicated code will affect 99% normal cases. Modern cpu are sensitive to cache thrash by code size and pipeline stalls by conditionals

Yes, absolutely.  So it follows that (a) we don't want to leave things 
in a loop if not necessary to repeat them, (b) we'd like to keep loops 
small as possible, (c) we want to keep our spin_lock sections small, and 
(d) we don't want to do things that later don't matter if an error 
happens when writing to the firmware.  So it seems to me that you should 
move that copy line from the loop and outside of the spin_lock, and put 
it after the call sync the filters.  Perhaps track the good mac index 
with "good_mac = i" at the end of the loop code and use that later to 
know which mac to copy into the vf struct.

I also noticed that you're checking the mac addresses for validity, but 
only before copying it to the local vf struct.  If you need to check the 
addresses, shouldn't you check them before you add them to the vf's 
filter list so you don't try to sync bad addresses to the FW?

If the sync to the FW fails, you send the error status to the VF but you 
still have this new address copied into the vf struct.  I think the copy 
line should be after the FW sync, and only if the sync succeeds.

sln


>
> Alex
>
> -----Original Message-----
> From: Shannon Nelson [mailto:snelson@...sando.io]
> Sent: Friday, August 2, 2019 2:11 AM
> To: Kirsher, Jeffrey T <jeffrey.t.kirsher@...el.com>; davem@...emloft.net
> Cc: Loktionov, Aleksandr <aleksandr.loktionov@...el.com>; netdev@...r.kernel.org; nhorman@...hat.com; sassmann@...hat.com; Bowers, AndrewX <andrewx.bowers@...el.com>
> Subject: Re: [net-next 2/9] i40e: make visible changed vf mac on host
>
> On 8/1/19 1:51 PM, Jeff Kirsher wrote:
>> From: Aleksandr Loktionov <aleksandr.loktionov@...el.com>
>>
>> This patch makes changed VM mac address visible on host via ip link
>> show command. This problem is fixed by copying last unicast mac filter
>> to vf->default_lan_addr.addr. Without this patch if VF MAC was not set
>> from host side and if you run ip link show $pf, on host side you'd
>> always see a zero MAC, not the real VF MAC that VF assigned to itself.
>>
>> Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@...el.com>
>> Tested-by: Andrew Bowers <andrewx.bowers@...el.com>
>> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@...el.com>
>> ---
>>    drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 3 +++
>>    1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
>> b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
>> index 02b09a8ad54c..21f7ac514d1f 100644
>> --- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
>> +++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
>> @@ -2629,6 +2629,9 @@ static int i40e_vc_add_mac_addr_msg(struct i40e_vf *vf, u8 *msg)
>>    			} else {
>>    				vf->num_mac++;
>>    			}
>> +			if (is_valid_ether_addr(al->list[i].addr))
>> +				ether_addr_copy(vf->default_lan_addr.addr,
>> +						al->list[i].addr);
>>    		}
>>    	}
>>    	spin_unlock_bh(&vsi->mac_filter_hash_lock);
> Since this copy is done inside the for-loop, it looks like you are copying every address in the list, not just the last one.  This seems wasteful and unnecessary.
>
> Since it is possible, altho' unlikely, that the filter sync that happens a little later could fail, might it be better to do the copy after you know that the sync has succeeded?
>
> Why is the last mac chosen for display rather than the first?  Is there anything special about the last mac as opposed to the first mac?
>
> sln
>