Re: [PATCH net 8/8] ixgbe: only access vfinfo and mv_list under RCU lock

[Corinna Vinschen <vinschen@redhat.com>]

Hi Jakub, hi others,

sorry for the late reply, I was AFK for two months.

I scanned the review points and most of them raise important questions.

This mail is only to let you know that I started looking into them
and I will follow up with a new patch in the next few weeks.


Thanks,
Corinna


On May 22 17:16, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> ixgbe: only access vfinfo and mv_list under RCU lock
> 
> This change drops the vfs_lock spinlock in favour of RCU protection
> for adapter->vfinfo and adapter->mv_list, marking them __rcu and
> converting readers to rcu_dereference()/rcu_read_lock() and writers
> to rcu_replace_pointer()/kfree_rcu(). The goal is to address a
> crash observed when ixgbe_disable_sriov() races with
> ixgbe_service_task() / ixgbe_msg_task() readers.
> 
> > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> > index 9b8217523fd2..8849b9f42bf6 100644
> > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> > @@ -808,10 +810,10 @@ struct ixgbe_adapter {
> >  	/* SR-IOV */
> >  	DECLARE_BITMAP(active_vfs, IXGBE_MAX_VF_FUNCTIONS);
> >  	unsigned int num_vfs;
> > -	struct vf_data_storage *vfinfo;
> > +	struct vf_data_storage __rcu *vfinfo;
> >  	int vf_rate_link_speed;
> >  	struct vf_macvlans vf_mvs;
> > -	struct vf_macvlans *mv_list;
> > +	struct vf_macvlans __rcu *mv_list;
> 
> [Medium]
> After this conversion, is ixgbe_handle_mdd_event() in
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c still consistent
> with the new annotations? It contains:
> 
>     if (adapter->vfinfo && ixgbe_check_mdd_event(adapter)) {
> 
> That bare load of an __rcu pointer is not wrapped in
> rcu_read_lock() and does not use rcu_access_pointer() or
> rcu_dereference(), so sparse will flag it and the access
> bypasses the RCU discipline used elsewhere in the file. The
> commit message says RCU is used "throughout the driver" — was
> this site simply missed during the conversion?
> 
> [ ... ]
> 
> > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> > index 2646ee6f295f..d82c7dfc6580 100644
> > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> 
> [ ... ]
> 
> > @@ -8267,22 +8293,27 @@ static void ixgbe_watchdog_flush_tx(struct ixgbe_adapter *adapter)
> > @@ -8309,9 +8340,15 @@ static void ixgbe_check_for_bad_vf(struct ixgbe_adapter *adapter)
> >  
> >  	/* check status reg for all VFs owned by this PF */
> >  	for (vf = 0; vf < adapter->num_vfs; ++vf) {
> > -		struct pci_dev *vfdev = adapter->vfinfo[vf].vfdev;
> > +		struct vf_data_storage *vfinfo;
> > +		struct pci_dev *vfdev = NULL;
> >  		u16 status_reg;
> >  
> > +		rcu_read_lock();
> > +		vfinfo = rcu_dereference(adapter->vfinfo);
> > +		if (vfinfo)
> > +			vfdev = vfinfo[vf].vfdev;
> > +		rcu_read_unlock();
> >  		if (!vfdev)
> >  			continue;
> >  		pci_read_config_word(vfdev, PCI_STATUS, &status_reg);
> 
> [High]
> Can vfdev still be used safely after rcu_read_unlock() here?
> 
> The RCU section only protects the lifetime of the vfinfo array,
> not the lifetime of the pci_dev whose pointer was stored in it.
> ixgbe_disable_sriov() drops the reference taken by
> ixgbe_get_vfs() with pci_dev_put(vfdev) and then proceeds to
> pci_disable_sriov(), either of which can free the pci_dev
> concurrently with the subsequent pci_read_config_word() and the
> pcie_flr(vfdev) reached through ixgbe_bad_vf_abort().
> 
> Should this path take pci_dev_get(vfdev) before dropping the
> rcu_read_unlock(), and pci_dev_put() after the config-space
> access and FLR are complete?
> 
> This appears to predate this patch (the prior code dereferenced
> vfdev outside any lifetime-extending section as well), but the
> RCU conversion seems like a natural opportunity to address it.
> 
> [ ... ]
> 
> > @@ -10071,20 +10114,26 @@ static int handle_redirect_action(struct ixgbe_adapter *adapter, int ifindex,
> >  {
> >  	struct ixgbe_ring_feature *vmdq = &adapter->ring_feature[RING_F_VMDQ];
> >  	unsigned int num_vfs = adapter->num_vfs, vf;
> > +	struct vf_data_storage *vfinfo;
> >  	struct netdev_nested_priv priv;
> >  	struct upper_walk_data data;
> >  	struct net_device *upper;
> >  
> >  	/* redirect to a SRIOV VF */
> > -	for (vf = 0; vf < num_vfs; ++vf) {
> > -		upper = pci_get_drvdata(adapter->vfinfo[vf].vfdev);
> > -		if (upper->ifindex == ifindex) {
> > -			*queue = vf * __ALIGN_MASK(1, ~vmdq->mask);
> > -			*action = vf + 1;
> > -			*action <<= ETHTOOL_RX_FLOW_SPEC_RING_VF_OFF;
> > -			return 0;
> > +	rcu_read_lock();
> > +	vfinfo = rcu_dereference(adapter->vfinfo);
> > +	if (vfinfo)
> > +		for (vf = 0; vf < num_vfs; ++vf) {
> > +			upper = pci_get_drvdata(vfinfo[vf].vfdev);
> 
> [High]
> Can the captured num_vfs and the freshly-dereferenced vfinfo
> disagree in size here?
> 
> num_vfs is sampled from adapter->num_vfs before rcu_read_lock(),
> while vfinfo is observed inside the RCU section. ixgbe_disable_sriov()
> runs under PCI device_lock (not rtnl), sets adapter->num_vfs = 0,
> and rcu_replace_pointer()s vfinfo. A subsequent
> __ixgbe_enable_sriov(N2) can publish a smaller vfinfo array of
> size N2 < num_vfs before the loop runs. The access vfinfo[vf]
> for vf >= N2 is then a heap out-of-bounds read.
> 
> Should num_vfs be sampled inside the rcu_read_lock() and bounded
> by the actual size of the published vfinfo, or should
> adapter->num_vfs be updated as part of the same rcu_assign_pointer
> publication so readers can rely on a consistent pair?
> 
> [ ... ]
> 
> > @@ -9744,15 +9781,21 @@ static int ixgbe_ndo_get_vf_stats(struct net_device *netdev, int vf,
> >  				  struct ifla_vf_stats *vf_stats)
> >  {
> >  	struct ixgbe_adapter *adapter = ixgbe_from_netdev(netdev);
> > +	struct vf_data_storage *vfinfo;
> >  
> >  	if (vf < 0 || vf >= adapter->num_vfs)
> >  		return -EINVAL;
> >  
> > -	vf_stats->rx_packets = adapter->vfinfo[vf].vfstats.gprc;
> > -	vf_stats->rx_bytes   = adapter->vfinfo[vf].vfstats.gorc;
> > -	vf_stats->tx_packets = adapter->vfinfo[vf].vfstats.gptc;
> > -	vf_stats->tx_bytes   = adapter->vfinfo[vf].vfstats.gotc;
> > -	vf_stats->multicast  = adapter->vfinfo[vf].vfstats.mprc;
> > +	rcu_read_lock();
> > +	vfinfo = rcu_dereference(adapter->vfinfo);
> > +	if (vfinfo) {
> > +		vf_stats->rx_packets = vfinfo[vf].vfstats.gprc;
> > +		vf_stats->rx_bytes   = vfinfo[vf].vfstats.gorc;
> > +		vf_stats->tx_packets = vfinfo[vf].vfstats.gptc;
> > +		vf_stats->tx_bytes   = vfinfo[vf].vfstats.gotc;
> > +		vf_stats->multicast  = vfinfo[vf].vfstats.mprc;
> > +	}
> > +	rcu_read_unlock();
> 
> [High]
> This pattern recurs in many of the rtnl ndo handlers (set_vf_mac,
> set_vf_spoofchk, set_vf_rss_query_en, set_vf_trust, set_vf_vlan,
> set_vf_bw, get_vf_config, get_vf_stats): the bound check
> 
>     if (vf >= adapter->num_vfs)
>         return -EINVAL;
> 
> is performed before rcu_read_lock(). Between that check and the
> later rcu_dereference, ixgbe_disable_sriov() (under PCI device_lock,
> not rtnl) can set num_vfs = 0 and __ixgbe_enable_sriov() can
> republish vfinfo at a smaller size N2.
> 
> For ixgbe_ndo_get_vf_config() the resulting OOB read is then
> memcpy'd into struct ifla_vf_info and returned to userspace via
> netlink, which would expose adjacent kernel heap. For the setter
> ndos and the mailbox-driven setters, this is an out-of-bounds
> write.
> 
> Should the vf range check be repeated against the size of the
> freshly observed vfinfo, ideally with num_vfs and vfinfo
> published together so the pair is self-consistent?
> 
> > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
> > index 431d77da15a5..80f22a8e7af4 100644
> > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
> > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
> 
> [ ... ]
> 
> > @@ -206,31 +214,28 @@ int ixgbe_disable_sriov(struct ixgbe_adapter *adapter)
> >  {
> >  	unsigned int num_vfs = adapter->num_vfs, vf;
> >  	struct ixgbe_hw *hw = &adapter->hw;
> > -	unsigned long flags;
> > +	struct vf_data_storage *vfinfo;
> > +	struct vf_macvlans *mv_list;
> >  	int rss;
> >  
> > -	spin_lock_irqsave(&adapter->vfs_lock, flags);
> > -	/* set num VFs to 0 to prevent access to vfinfo */
> > +	/* set num VFs to 0 so readers bail out early */
> >  	adapter->num_vfs = 0;
> > -	spin_unlock_irqrestore(&adapter->vfs_lock, flags);
> > +
> > +	vfinfo = rcu_replace_pointer(adapter->vfinfo, NULL, 1);
> > +	mv_list = rcu_replace_pointer(adapter->mv_list, NULL, 1);
> 
> [Low]
> The lockdep predicate passed to rcu_replace_pointer() is the
> literal 1, which makes the writer-side assertion always succeed
> and provides no verification. The patch simultaneously removes
> vfs_lock and shifts to an implicit (undocumented) reliance on
> some caller serialization for the writer.
> 
> Could lockdep_is_held() of the actual writer-side lock (for
> example the PCI device_lock or rtnl_lock, whichever is intended)
> be used here so a future regression that calls
> ixgbe_disable_sriov() outside that context is caught by lockdep?
> 
> [ ... ]
> 
> > @@ -643,10 +668,16 @@ static void ixgbe_clear_vf_vlans(struct ixgbe_adapter *adapter, u32 vf)
> >  static int ixgbe_set_vf_macvlan(struct ixgbe_adapter *adapter,
> >  				int vf, int index, unsigned char *mac_addr)
> >  {
> > -	struct vf_macvlans *entry;
> > +	struct vf_macvlans *mv_list, *entry;
> >  	bool found = false;
> >  	int retval = 0;
> >  
> > +	lockdep_assert_in_rcu_read_lock();
> > +	/* vf_mvs entries point into the mv_list array */
> > +	mv_list = rcu_dereference(adapter->mv_list);
> > +	if (!mv_list)
> > +		return 0;
> > +
> >  	if (index <= 1) {
> >  		list_for_each_entry(entry, &adapter->vf_mvs.l, l) {
> 
> [High]
> Is non-RCU list iteration safe here while only rcu_read_lock()
> is held?
> 
> The list head &adapter->vf_mvs.l is mutated by the writer side
> without RCU-aware primitives. ixgbe_alloc_vf_macvlans() does:
> 
>     INIT_LIST_HEAD(&adapter->vf_mvs.l);
>     ...
>     list_add(&mv_list[i].l, &adapter->vf_mvs.l);
> 
> and ixgbe_disable_sriov() kfree_rcu()s mv_list but never
> reinitialises &adapter->vf_mvs.l, so on a subsequent
> re-enable the writer concurrently zeros and re-links the head
> while a reader walks it.
> 
> Should list_for_each_entry() be list_for_each_entry_rcu(), and
> the writer use INIT_LIST_HEAD_RCU() / list_add_rcu()? Otherwise
> the reader can observe torn next/prev loads or follow into
> freed/re-used entries.
> 
> [ ... ]
> 
> > @@ -780,16 +816,25 @@ static void ixgbe_vf_clear_mbx(struct ixgbe_adapter *adapter, u32 vf)
> >  static int ixgbe_set_vf_mac(struct ixgbe_adapter *adapter,
> >  			    int vf, unsigned char *mac_addr)
> >  {
> > +	struct vf_data_storage *vfinfo;
> >  	int retval;
> >  
> > -	ixgbe_del_mac_filter(adapter, adapter->vfinfo[vf].vf_mac_addresses, vf);
> > +	rcu_read_lock();
> > +	vfinfo = rcu_dereference(adapter->vfinfo);
> > +	if (!vfinfo) {
> > +		rcu_read_unlock();
> > +		return -EINVAL;
> > +	}
> > +
> > +	ixgbe_del_mac_filter(adapter, vfinfo[vf].vf_mac_addresses, vf);
> >  	retval = ixgbe_add_mac_filter(adapter, mac_addr, vf);
> >  	if (retval >= 0)
> > -		memcpy(adapter->vfinfo[vf].vf_mac_addresses, mac_addr,
> > +		memcpy(vfinfo[vf].vf_mac_addresses, mac_addr,
> >  		       ETH_ALEN);
> 
> [ ... ]
> 
> > @@ -1537,21 +1677,34 @@ void ixgbe_ping_all_vfs(struct ixgbe_adapter *adapter)
> >  int ixgbe_ndo_set_vf_mac(struct net_device *netdev, int vf, u8 *mac)
> >  {
> >  	struct ixgbe_adapter *adapter = ixgbe_from_netdev(netdev);
> > +	struct vf_data_storage *vfinfo;
> >  	int retval;
> >  
> >  	if (vf >= adapter->num_vfs)
> >  		return -EINVAL;
> >  
> > +	rcu_read_lock();
> > +	vfinfo = rcu_dereference(adapter->vfinfo);
> > +	if (!vfinfo) {
> > +		rcu_read_unlock();
> > +		return 0;
> > +	}
> > +
> >  	if (is_valid_ether_addr(mac)) {
> >  		dev_info(&adapter->pdev->dev, "setting MAC %pM on VF %d\n",
> >  			 mac, vf);
> >  		dev_info(&adapter->pdev->dev, "Reload the VF driver to make this change effective.");
> >  
> >  		retval = ixgbe_set_vf_mac(adapter, vf, mac);
> >  		if (retval >= 0) {
> > -			adapter->vfinfo[vf].pf_set_mac = true;
> > +			vfinfo[vf].pf_set_mac = true;
> 
> [High]
> Can vfinfo here and the vfinfo observed inside ixgbe_set_vf_mac()
> end up pointing at different objects?
> 
> ixgbe_ndo_set_vf_mac() captures vfinfo via its own rcu_dereference,
> then calls ixgbe_set_vf_mac() which independently does:
> 
>     rcu_read_lock();
>     vfinfo = rcu_dereference(adapter->vfinfo);
>     ...
>     memcpy(vfinfo[vf].vf_mac_addresses, mac_addr, ETH_ALEN);
> 
> Nesting rcu_read_lock() is fine, but each rcu_dereference()
> observes whatever pointer is currently published. If
> ixgbe_disable_sriov() followed by __ixgbe_enable_sriov() races
> between the two dereferences, the inner copy lands in the new
> array while the outer pf_set_mac = true write lands in the old
> array. The two pieces of state then disagree, which seems to
> break the "attempted to override administratively set MAC"
> enforcement on subsequent VF mailbox messages. If the new
> vfinfo is shorter than vf, the inner memcpy is also a heap
> out-of-bounds write.
> 
> Should ixgbe_ndo_set_vf_mac() pass its already-captured vfinfo
> into ixgbe_set_vf_mac() so both writes go to the same object,
> or otherwise structure the call so a single rcu_dereference
> covers all writes for one operation?