RE: [Intel-wired-lan] [PATCH net] i40e: Fix VF MAC filter removal

[Romanowski, Rafal]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@xxxxxxxxxx> On Behalf Of
> Brett Creeley
> Sent: Thursday, March 14, 2024 4:54 PM
> To: ivecera <ivecera@xxxxxxxxxx>; netdev@xxxxxxxxxxxxxxx
> Cc: moderated list:INTEL ETHERNET DRIVERS <intel-wired-
> lan@xxxxxxxxxxxxxxxx>; open list <linux-kernel@xxxxxxxxxxxxxxx>; Loktionov,
> Aleksandr <aleksandr.loktionov@xxxxxxxxx>; Eric Dumazet
> <edumazet@xxxxxxxxxx>; Nguyen, Anthony L
> <anthony.l.nguyen@xxxxxxxxx>; horms@xxxxxxxxxx; Jakub Kicinski
> <kuba@xxxxxxxxxx>; Paolo Abeni <pabeni@xxxxxxxxxx>; David S. Miller
> <davem@xxxxxxxxxxxxx>
> Subject: Re: [Intel-wired-lan] [PATCH net] i40e: Fix VF MAC filter removal
> 
> On 3/13/2024 6:56 AM, Ivan Vecera wrote:
> > Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
> >
> >
> > Commit 73d9629e1c8c ("i40e: Do not allow untrusted VF to remove
> > administratively set MAC") fixed an issue where untrusted VF was
> > allowed to remove its own MAC address although this was assigned
> > administratively from PF. Unfortunately the introduced check is wrong
> > because it causes that MAC filters for other MAC addresses including
> > multi-cast ones are not removed.
> >
> > <snip>
> >          if (ether_addr_equal(addr, vf->default_lan_addr.addr) &&
> >              i40e_can_vf_change_mac(vf))
> >                  was_unimac_deleted = true;
> >          else
> >                  continue;
> >
> >          if (i40e_del_mac_filter(vsi, al->list[i].addr)) {
> >          ...
> > </snip>
> >
> > The else path with `continue` effectively skips any MAC filter removal
> > except one for primary MAC addr when VF is allowed to do so.
> > Fix the check condition so the `continue` is only done for primary MAC
> > address.
> >
> > Fixes: 73d9629e1c8c ("i40e: Do not allow untrusted VF to remove
> > administratively set MAC")
> > Signed-off-by: Ivan Vecera <ivecera@xxxxxxxxxx>
> > ---
> >   drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 11 ++++++-----
> >   1 file changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> > b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> > index b34c71770887..10267a300770 100644
> > --- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> > @@ -3143,11 +3143,12 @@ static int i40e_vc_del_mac_addr_msg(struct
> i40e_vf *vf, u8 *msg)
> >                  /* Allow to delete VF primary MAC only if it was not set
> >                   * administratively by PF or if VF is trusted.
> >                   */
> > -               if (ether_addr_equal(addr, vf->default_lan_addr.addr) &&
> > -                   i40e_can_vf_change_mac(vf))
> > -                       was_unimac_deleted = true;
> > -               else
> > -                       continue;
> > +               if (ether_addr_equal(addr, vf->default_lan_addr.addr)) {
> > +                       if (i40e_can_vf_change_mac(vf))
> > +                               was_unimac_deleted = true;
> > +                       else
> > +                               continue;
> > +               }
> 
> Seems okay to me.
> 
> Reviewed-by: Brett Creeley <brett.creeley@xxxxxxx>
> 
> >
> >                  if (i40e_del_mac_filter(vsi, al->list[i].addr)) {
> >                          ret = -EINVAL;
> > --
> > 2.43.0
> >
> >

Tested-by: Rafal Romanowski <rafal.romanowski@xxxxxxxxx>