[PATCH net 0/3][pull request] Intel Wired LAN Driver Updates 2022-03-15

[PATCH net 0/3][pull request] Intel Wired LAN Driver Updates 2022-03-15

[Tony Nguyen]

This series contains updates to ice and iavf drivers.

Maciej adjusts null check logic on Tx ring to prevent possible NULL
pointer dereference for ice.

Sudheer moves destruction of Flow Director lock as it was being accessed
after destruction for ice.

Przemyslaw removes an excess mutex unlock as it was being double
unlocked for iavf.

The following are changes since commit e9c14b59ea2ec19afe22d60b07583b7e08c74290:
  Add Paolo Abeni to networking maintainers
and are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/tnguy/net-queue 100GbE

Maciej Fijalkowski (1):
  ice: fix NULL pointer dereference in ice_update_vsi_tx_ring_stats()

Przemyslaw Patynowski (1):
  iavf: Fix double free in iavf_reset_task

Sudheer Mogilappagari (1):
  ice: destroy flow director filter mutex after releasing VSIs

 drivers/net/ethernet/intel/iavf/iavf_main.c | 8 +++++++-
 drivers/net/ethernet/intel/ice/ice_main.c   | 7 ++++---
 2 files changed, 11 insertions(+), 4 deletions(-)

-- 
2.31.1

[PATCH net 1/3] ice: fix NULL pointer dereference in ice_update_vsi_tx_ring_stats()

[Tony Nguyen]

From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>

It is possible to do NULL pointer dereference in routine that updates
Tx ring stats. Currently only stats and bytes are updated when ring
pointer is valid, but later on ring is accessed to propagate gathered Tx
stats onto VSI stats.

Change the existing logic to move to next ring when ring is NULL.

Fixes: e72bba21355d ("ice: split ice_ring onto Tx/Rx separate structs")
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Acked-by: Alexander Lobakin <alexandr.lobakin@intel.com>
Tested-by: Gurucharan G <gurucharanx.g@intel.com> (A Contingent worker at Intel)
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_main.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 493942e910be..d4a7c39fd078 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -5962,8 +5962,9 @@ ice_update_vsi_tx_ring_stats(struct ice_vsi *vsi,
 		u64 pkts = 0, bytes = 0;
 
 		ring = READ_ONCE(rings[i]);
-		if (ring)
-			ice_fetch_u64_stats_per_ring(&ring->syncp, ring->stats, &pkts, &bytes);
+		if (!ring)
+			continue;
+		ice_fetch_u64_stats_per_ring(&ring->syncp, ring->stats, &pkts, &bytes);
 		vsi_stats->tx_packets += pkts;
 		vsi_stats->tx_bytes += bytes;
 		vsi->tx_restart += ring->tx_stats.restart_q;
-- 
2.31.1

[PATCH net 2/3] ice: destroy flow director filter mutex after releasing VSIs

[Tony Nguyen]

From: Sudheer Mogilappagari <sudheer.mogilappagari@intel.com>

Currently fdir_fltr_lock is accessed in ice_vsi_release_all() function
after it is destroyed. Instead destroy mutex after ice_vsi_release_all.

Fixes: 40319796b732 ("ice: Add flow director support for channel mode")
Signed-off-by: Sudheer Mogilappagari <sudheer.mogilappagari@intel.com>
Tested-by: Bharathi Sreenivas <bharathi.sreenivas@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index d4a7c39fd078..b7e8744b0c0a 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -4880,7 +4880,6 @@ static void ice_remove(struct pci_dev *pdev)
 	ice_devlink_unregister_params(pf);
 	set_bit(ICE_DOWN, pf->state);
 
-	mutex_destroy(&(&pf->hw)->fdir_fltr_lock);
 	ice_deinit_lag(pf);
 	if (test_bit(ICE_FLAG_PTP_SUPPORTED, pf->flags))
 		ice_ptp_release(pf);
@@ -4888,6 +4887,7 @@ static void ice_remove(struct pci_dev *pdev)
 		ice_remove_arfs(pf);
 	ice_setup_mc_magic_wake(pf);
 	ice_vsi_release_all(pf);
+	mutex_destroy(&(&pf->hw)->fdir_fltr_lock);
 	ice_set_wake(pf);
 	ice_free_irq_msix_misc(pf);
 	ice_for_each_vsi(pf, i) {
-- 
2.31.1

[PATCH net 3/3] iavf: Fix double free in iavf_reset_task

[Tony Nguyen]

From: Przemyslaw Patynowski <przemyslawx.patynowski@intel.com>

Fix double free possibility in iavf_disable_vf, as crit_lock is
freed in caller, iavf_reset_task. Add kernel-doc for iavf_disable_vf.
Remove mutex_unlock in iavf_disable_vf.
Without this patch there is double free scenario, when calling
iavf_reset_task.

Fixes: e85ff9c631e1 ("iavf: Fix deadlock in iavf_reset_task")
Signed-off-by: Przemyslaw Patynowski <przemyslawx.patynowski@intel.com>
Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Mateusz Palczewski <mateusz.palczewski@intel.com>
Tested-by: Konrad Jankowski <konrad0.jankowski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/iavf/iavf_main.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index 8e644e9ed8da..45570e3f782e 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -2541,6 +2541,13 @@ static void iavf_watchdog_task(struct work_struct *work)
 		queue_delayed_work(iavf_wq, &adapter->watchdog_task, HZ * 2);
 }
 
+/**
+ * iavf_disable_vf - disable VF
+ * @adapter: board private structure
+ *
+ * Set communication failed flag and free all resources.
+ * NOTE: This function is expected to be called with crit_lock being held.
+ **/
 static void iavf_disable_vf(struct iavf_adapter *adapter)
 {
 	struct iavf_mac_filter *f, *ftmp;
@@ -2595,7 +2602,6 @@ static void iavf_disable_vf(struct iavf_adapter *adapter)
 	memset(adapter->vf_res, 0, IAVF_VIRTCHNL_VF_RESOURCE_SIZE);
 	iavf_shutdown_adminq(&adapter->hw);
 	adapter->netdev->flags &= ~IFF_UP;
-	mutex_unlock(&adapter->crit_lock);
 	adapter->flags &= ~IAVF_FLAG_RESET_PENDING;
 	iavf_change_state(adapter, __IAVF_DOWN);
 	wake_up(&adapter->down_waitqueue);
-- 
2.31.1

Re: [PATCH net 1/3] ice: fix NULL pointer dereference in ice_update_vsi_tx_ring_stats()

[Jakub Kicinski]

On Tue, 15 Mar 2022 14:12:23 -0700 Tony Nguyen wrote:
> From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> 
> It is possible to do NULL pointer dereference in routine that updates
> Tx ring stats. Currently only stats and bytes are updated when ring

s/stats/packets/ ?

> pointer is valid, but later on ring is accessed to propagate gathered Tx
> stats onto VSI stats.
> 
> Change the existing logic to move to next ring when ring is NULL.
> 
> Fixes: e72bba21355d ("ice: split ice_ring onto Tx/Rx separate structs")
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> Acked-by: Alexander Lobakin <alexandr.lobakin@intel.com>
> Tested-by: Gurucharan G <gurucharanx.g@intel.com> (A Contingent worker at Intel)
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_main.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> index 493942e910be..d4a7c39fd078 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> @@ -5962,8 +5962,9 @@ ice_update_vsi_tx_ring_stats(struct ice_vsi *vsi,
>  		u64 pkts = 0, bytes = 0;
>  
>  		ring = READ_ONCE(rings[i]);

Not really related to this patch but why is there a read_once() here?
Aren't stats read under rtnl_lock? What is this protecting against?

> -		if (ring)
> -			ice_fetch_u64_stats_per_ring(&ring->syncp, ring->stats, &pkts, &bytes);
> +		if (!ring)
> +			continue;
> +		ice_fetch_u64_stats_per_ring(&ring->syncp, ring->stats, &pkts, &bytes);

Re: [PATCH net 3/3] iavf: Fix double free in iavf_reset_task

[Jakub Kicinski]

On Tue, 15 Mar 2022 14:12:25 -0700 Tony Nguyen wrote:
> Fix double free possibility in iavf_disable_vf, as crit_lock is
> freed in caller, iavf_reset_task. Add kernel-doc for iavf_disable_vf.
> Remove mutex_unlock in iavf_disable_vf.
> Without this patch there is double free scenario, when calling
> iavf_reset_task.

s/free/unlock/ or s/free/release/

Re: [PATCH net 2/3] ice: destroy flow director filter mutex after releasing VSIs

[Jakub Kicinski]

On Tue, 15 Mar 2022 14:12:24 -0700 Tony Nguyen wrote:
> +	mutex_destroy(&(&pf->hw)->fdir_fltr_lock);

nit: maybe uncrazify this line while moving it?

Re: [PATCH net 2/3] ice: destroy flow director filter mutex after releasing VSIs

[Tony Nguyen]

On 3/15/2022 8:32 PM, Jakub Kicinski wrote:
> On Tue, 15 Mar 2022 14:12:24 -0700 Tony Nguyen wrote:
>> +	mutex_destroy(&(&pf->hw)->fdir_fltr_lock);
> nit: maybe uncrazify this line while moving it?
Yea, will uncrazify :)

Re: [PATCH net 3/3] iavf: Fix double free in iavf_reset_task

[Tony Nguyen]

On 3/15/2022 8:30 PM, Jakub Kicinski wrote:
> On Tue, 15 Mar 2022 14:12:25 -0700 Tony Nguyen wrote:
>> Fix double free possibility in iavf_disable_vf, as crit_lock is
>> freed in caller, iavf_reset_task. Add kernel-doc for iavf_disable_vf.
>> Remove mutex_unlock in iavf_disable_vf.
>> Without this patch there is double free scenario, when calling
>> iavf_reset_task.
> s/free/unlock/ or s/free/release/
Will fix.

Re: [PATCH net 1/3] ice: fix NULL pointer dereference in ice_update_vsi_tx_ring_stats()

[Nguyen, Anthony L]

On Tue, 2022-03-15 at 20:29 -0700, Jakub Kicinski wrote:
> On Tue, 15 Mar 2022 14:12:23 -0700 Tony Nguyen wrote:
> > From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> > 
> > It is possible to do NULL pointer dereference in routine that
> > updates
> > Tx ring stats. Currently only stats and bytes are updated when ring
> 
> s/stats/packets/ ?

Will fix.

> 
> > pointer is valid, but later on ring is accessed to propagate
> > gathered Tx
> > stats onto VSI stats.
> > 
> > Change the existing logic to move to next ring when ring is NULL.
> > 
> > Fixes: e72bba21355d ("ice: split ice_ring onto Tx/Rx separate
> > structs")
> > Reported-by: kernel test robot <lkp@intel.com>
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> > Acked-by: Alexander Lobakin <alexandr.lobakin@intel.com>
> > Tested-by: Gurucharan G <gurucharanx.g@intel.com> (A Contingent
> > worker at Intel)
> > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> > ---
> >  drivers/net/ethernet/intel/ice/ice_main.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/intel/ice/ice_main.c
> > b/drivers/net/ethernet/intel/ice/ice_main.c
> > index 493942e910be..d4a7c39fd078 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_main.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> > @@ -5962,8 +5962,9 @@ ice_update_vsi_tx_ring_stats(struct ice_vsi
> > *vsi,
> >                 u64 pkts = 0, bytes = 0;
> >  
> >                 ring = READ_ONCE(rings[i]);
> 
> Not really related to this patch but why is there a read_once() here?
> Aren't stats read under rtnl_lock? What is this protecting against?

It looks like it was based on a patch from i40e [1]. From the commit, I
gather this is the reason:

"Previously the stats were 64 bit but highly racy due to the fact that
64 bit transactions are not atomic on 32 bit systems."

Thanks,

Tony

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=980e9b1186424fa3eb766d59fc91003d0ed1ed6a


(Resending as some non-text formatting snuck in to my reply. Sorry for
the spam)

Re: [PATCH net 1/3] ice: fix NULL pointer dereference in ice_update_vsi_tx_ring_stats()

[Alexander Duyck]

On Wed, Mar 16, 2022 at 12:01 PM Nguyen, Anthony L
<anthony.l.nguyen@intel.com> wrote:
>
> On Tue, 2022-03-15 at 20:29 -0700, Jakub Kicinski wrote:
> > On Tue, 15 Mar 2022 14:12:23 -0700 Tony Nguyen wrote:
> > > From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>

<snip>

> > > pointer is valid, but later on ring is accessed to propagate
> > > gathered Tx
> > > stats onto VSI stats.
> > >
> > > Change the existing logic to move to next ring when ring is NULL.
> > >
> > > Fixes: e72bba21355d ("ice: split ice_ring onto Tx/Rx separate
> > > structs")
> > > Reported-by: kernel test robot <lkp@intel.com>
> > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> > > Acked-by: Alexander Lobakin <alexandr.lobakin@intel.com>
> > > Tested-by: Gurucharan G <gurucharanx.g@intel.com> (A Contingent
> > > worker at Intel)
> > > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> > > ---
> > >  drivers/net/ethernet/intel/ice/ice_main.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/intel/ice/ice_main.c
> > > b/drivers/net/ethernet/intel/ice/ice_main.c
> > > index 493942e910be..d4a7c39fd078 100644
> > > --- a/drivers/net/ethernet/intel/ice/ice_main.c
> > > +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> > > @@ -5962,8 +5962,9 @@ ice_update_vsi_tx_ring_stats(struct ice_vsi
> > > *vsi,
> > >                 u64 pkts = 0, bytes = 0;
> > >
> > >                 ring = READ_ONCE(rings[i]);
> >
> > Not really related to this patch but why is there a read_once() here?
> > Aren't stats read under rtnl_lock? What is this protecting against?
>
> It looks like it was based on a patch from i40e [1]. From the commit, I
> gather this is the reason:
>
> "Previously the stats were 64 bit but highly racy due to the fact that
> 64 bit transactions are not atomic on 32 bit systems."
>
> Thanks,
>
> Tony
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=980e9b1186424fa3eb766d59fc91003d0ed1ed6a
>
>
> (Resending as some non-text formatting snuck in to my reply. Sorry for
> the spam)

Actually the rcu locking and READ_ONCE has to do with the fact that
the driver code for the igb/ixgbe/i40e driver had a bunch of code that
could kick in from the sysfs and/or PCIe paths that would start
tearing things down without necessarily holding the rtnl_lock as it
should have.

It might make sense to reevaluate the usage of the READ_ONCE and rcu
locking to determine if it is actually needed since much of that is a
hold over from when we were doing this in the watchdog and not while
holding the rtnl_lock in the stats call.

Re: [PATCH net 1/3] ice: fix NULL pointer dereference in ice_update_vsi_tx_ring_stats()

[Tony Nguyen]

On 3/16/2022 1:24 PM, Alexander Duyck wrote:
> On Wed, Mar 16, 2022 at 12:01 PM Nguyen, Anthony L
> <anthony.l.nguyen@intel.com> wrote:
>> On Tue, 2022-03-15 at 20:29 -0700, Jakub Kicinski wrote:
>>> On Tue, 15 Mar 2022 14:12:23 -0700 Tony Nguyen wrote:
>>>> From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> <snip>
>
>>>> pointer is valid, but later on ring is accessed to propagate
>>>> gathered Tx
>>>> stats onto VSI stats.
>>>>
>>>> Change the existing logic to move to next ring when ring is NULL.
>>>>
>>>> Fixes: e72bba21355d ("ice: split ice_ring onto Tx/Rx separate
>>>> structs")
>>>> Reported-by: kernel test robot <lkp@intel.com>
>>>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>> Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
>>>> Acked-by: Alexander Lobakin <alexandr.lobakin@intel.com>
>>>> Tested-by: Gurucharan G <gurucharanx.g@intel.com> (A Contingent
>>>> worker at Intel)
>>>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
>>>> ---
>>>>   drivers/net/ethernet/intel/ice/ice_main.c | 5 +++--
>>>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c
>>>> b/drivers/net/ethernet/intel/ice/ice_main.c
>>>> index 493942e910be..d4a7c39fd078 100644
>>>> --- a/drivers/net/ethernet/intel/ice/ice_main.c
>>>> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
>>>> @@ -5962,8 +5962,9 @@ ice_update_vsi_tx_ring_stats(struct ice_vsi
>>>> *vsi,
>>>>                  u64 pkts = 0, bytes = 0;
>>>>
>>>>                  ring = READ_ONCE(rings[i]);
>>> Not really related to this patch but why is there a read_once() here?
>>> Aren't stats read under rtnl_lock? What is this protecting against?
>> It looks like it was based on a patch from i40e [1]. From the commit, I
>> gather this is the reason:
>>
>> "Previously the stats were 64 bit but highly racy due to the fact that
>> 64 bit transactions are not atomic on 32 bit systems."
>>
>> Thanks,
>>
>> Tony
>>
>> [1]
>> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=980e9b1186424fa3eb766d59fc91003d0ed1ed6a
>>
>>
>> (Resending as some non-text formatting snuck in to my reply. Sorry for
>> the spam)
> Actually the rcu locking and READ_ONCE has to do with the fact that
> the driver code for the igb/ixgbe/i40e driver had a bunch of code that
> could kick in from the sysfs and/or PCIe paths that would start
> tearing things down without necessarily holding the rtnl_lock as it
> should have.
>
> It might make sense to reevaluate the usage of the READ_ONCE and rcu
> locking to determine if it is actually needed since much of that is a
> hold over from when we were doing this in the watchdog and not while
> holding the rtnl_lock in the stats call.

Thanks for clarifying Alex. I'll ask the teams to look into this.