Re: [Intel-wired-lan] [PATCH net v1] iavf: validate dest MAC and VLAN from tc-filter code path

[Maciej Fijalkowski <maciej.fijalkowski@intel.com>]

On Mon, Jun 27, 2022 at 09:21:59PM -0400, Michal Swiatkowski wrote:
> On Wed, Jun 22, 2022 at 10:31:42PM +0000, Zhang, Xuejun wrote:
> > Hi Michal, 
> > 
> > Thanks for the reply.
> > 
> > The lock is access lock for both mac_filter_list & vlan_filter_list. Access to anyone of the two filter lists requires holding the lock per current code design. 
> 
> Ok, thanks for explanation.

This doesn't explain why spin_lock_bh() is used and what contexts are
being protected.

> 
> > 
> > Regards 
> > 
> > Jun
> > 
> > -----Original Message-----
> > From: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> 
> > Sent: Tuesday, June 21, 2022 6:31 PM
> > To: Zhang, Xuejun <xuejun.zhang@intel.com>
> > Cc: intel-wired-lan@lists.osuosl.org; Kiran Patil <kiran.patil@intel.com>
> > Subject: Re: [Intel-wired-lan] [PATCH net v1] iavf: validate dest MAC and VLAN from tc-filter code path
> > 
> > On Tue, Jun 21, 2022 at 08:21:42PM +0000, Zhang, Xuejun wrote:
> > > Hi Michal,
> > 
> > Hi Jun
> > > 
> > > Pls add your comments whenever you have a chance.
> > 
> > Sorry, I am confused, comments about what? Did I miss replay to my previous one? I didn't receive any replay to my question about why we need this spin lock when there is only check for filter.
> > 
> > Please resend it to me if You can.
> > 
> > Regards
> > Michal
> > 
> > > 
> > > Regards,
> > > 
> > > Jun
> > > 
> > > -----Original Message-----
> > > From: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> > > Sent: Sunday, June 19, 2022 10:33 PM
> > > To: Zhang, Xuejun <xuejun.zhang@intel.com>
> > > Cc: intel-wired-lan@lists.osuosl.org; Kiran Patil 
> > > <kiran.patil@intel.com>
> > > Subject: Re: [Intel-wired-lan] [PATCH net v1] iavf: validate dest MAC 
> > > and VLAN from tc-filter code path
> > > 
> > > On Fri, Jun 17, 2022 at 01:50:00PM -0400, Jun Zhang wrote:
> > > > From: Kiran Patil <kiran.patil@intel.com>
> > > > 
> > > > Before allowing tc-filter using dest MAC, VLAN - check to make sure 
> > > > there is basic active filter using specified dest MAC and likewise 
> > > > for VLAN.
> > > > 
> > > > This check is must to allow only legit filter via tc-filter code 
> > > > path with or without ADQ.
> > > > 
> > > > Fixes: 0075fa0fadd0 ("i40evf: Add support to apply cloud filters")
> > > > Signed-off-by: Kiran Patil <kiran.patil@intel.com>
> > > > Signed-off-by: Jun Zhang <xuejun.zhang@intel.com>
> > > > ---
> > > >  drivers/net/ethernet/intel/iavf/iavf_main.c | 62
> > > > ++++++++++++++++++++-
> > > >  1 file changed, 61 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c
> > > > b/drivers/net/ethernet/intel/iavf/iavf_main.c
> > > > index 57c51a15bcbc..287c3e4bf8af 100644
> > > > --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> > > > +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> > > > @@ -3558,6 +3558,48 @@ static int __iavf_setup_tc(struct net_device *netdev, void *type_data)
> > > >  	return ret;
> > > >  }
> > > >  
> > > > +/**
> > > > + * iavf_is_vlan_tc_filter_allowed - allowed to add tc-filter using 
> > > > +VLAN
> > > > + * @adapter: board private structure
> > > > + * @vlan: VLAN to verify
> > > > + *
> > > > + * Using specified "vlan" ID, there must be active VLAN filter in 
> > > > +VF's
> > > > + * MAC-VLAN filter list.
> > > > + */
> > > > +static bool
> > > > +iavf_is_vlan_tc_filter_allowed(struct iavf_adapter *adapter, u16
> > > > +vlan) {
> > > > +	struct iavf_vlan_filter *f;
> > > > +	bool allowed;
> > > > +
> > > > +	spin_lock_bh(&adapter->mac_vlan_list_lock);
> > > Why do we need lock here?
> > > 
> > > > +	f = iavf_find_vlan(adapter, IAVF_VLAN(vlan, ETH_P_8021Q));
> > > > +	allowed = (f && f->add && !f->remove);
> > > > +	spin_unlock_bh(&adapter->mac_vlan_list_lock);
> > > > +	return allowed;
> > > > +}
> > > > +
> > > > +/**
> > > > + * iavf_is_mac_tc_filter_allowed - allowed to add tc-filter using 
> > > > +MAC addr
> > > > + * @adapter: board private structure
> > > > + * @macaddr: MAC address
> > > > + *
> > > > + * Using specified MAC address, there must be active MAC filter in 
> > > > +VF's
> > > > + * MAC-VLAN filter list.
> > > > + */
> > > > +static bool
> > > > +iavf_is_mac_tc_filter_allowed(struct iavf_adapter *adapter, const 
> > > > +u8
> > > > +*macaddr) {
> > > > +	struct iavf_mac_filter *f;
> > > > +	bool allowed;
> > > > +
> > > > +	spin_lock_bh(&adapter->mac_vlan_list_lock);
> > > > +	f = iavf_find_filter(adapter, macaddr);
> > > > +	allowed = (f && f->add && !f->is_new_mac && !f->remove);
> > > > +	spin_unlock_bh(&adapter->mac_vlan_list_lock);
> > > > +	return allowed;
> > > > +}
> > > > +
> > > >  /**
> > > >   * iavf_parse_cls_flower - Parse tc flower filters provided by kernel
> > > >   * @adapter: board private structure @@ -3651,7 +3693,15 @@ static 
> > > > int iavf_parse_cls_flower(struct iavf_adapter *adapter,
> > > >  			}
> > > >  		}
> > > >  
> > > > -		if (!is_zero_ether_addr(match.key->dst))
> > > > +		if (!is_zero_ether_addr(match.key->dst)) {
> > > > +			if (!iavf_is_mac_tc_filter_allowed(adapter,
> > > > +							   match.key->dst)) {
> > > > +				dev_err(&adapter->pdev->dev,
> > > > +					"Dest MAC %pM doesn't belong to this VF\n",
> > > > +					match.mask->dst);
> > > > +				return -EINVAL;
> > > > +			}
> > > > +
> > > >  			if (is_valid_ether_addr(match.key->dst) ||
> > > >  			    is_multicast_ether_addr(match.key->dst)) {
> > > >  				/* set the mask if a valid dst_mac address */ @@ -3660,6 
> > > > +3710,7 @@ static int iavf_parse_cls_flower(struct iavf_adapter *adapter,
> > > >  				ether_addr_copy(vf->data.tcp_spec.dst_mac,
> > > >  						match.key->dst);
> > > >  			}
> > > > +		}
> > > >  
> > > >  		if (!is_zero_ether_addr(match.key->src))
> > > >  			if (is_valid_ether_addr(match.key->src) || @@ -3677,6 +3728,8 @@ 
> > > > static int iavf_parse_cls_flower(struct iavf_adapter *adapter,
> > > >  
> > > >  		flow_rule_match_vlan(rule, &match);
> > > >  		if (match.mask->vlan_id) {
> > > > +			u16 vlan = match.key->vlan_id & VLAN_VID_MASK;
> > > > +
> > > >  			if (match.mask->vlan_id == VLAN_VID_MASK) {
> > > >  				field_flags |= IAVF_CLOUD_FIELD_IVLAN;
> > > >  			} else {
> > > > @@ -3684,6 +3737,13 @@ static int iavf_parse_cls_flower(struct iavf_adapter *adapter,
> > > >  					match.mask->vlan_id);
> > > >  				return -EINVAL;
> > > >  			}
> > > > +
> > > > +			if (!iavf_is_vlan_tc_filter_allowed(adapter, vlan)) {
> > > > +				dev_err(&adapter->pdev->dev,
> > > > +					"VLAN %u doesn't belong to this VF\n",
> > > > +					vlan);
> > > > +				return -EINVAL;
> > > > +			}
> > > >  		}
> > > >  		vf->mask.tcp_spec.vlan_id |= cpu_to_be16(0xffff);
> > > >  		vf->data.tcp_spec.vlan_id = cpu_to_be16(match.key->vlan_id);
> > > > --
> > > > 2.35.3
> > > > 
> > > > _______________________________________________
> > > > Intel-wired-lan mailing list
> > > > Intel-wired-lan@osuosl.org
> > > > https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan