[PATCH iwl-net 0/3] idpf: fix 3 bugs revealed by the Chapter I

[PATCH iwl-net 0/3] idpf: fix 3 bugs revealed by the Chapter I

[Alexander Lobakin]

The libeth conversion revealed 2 serious issues which lead to sporadic
crashes or WARNs under certain configurations. Additional one was found
while debugging these two with kmemleak.
This one is targeted stable, the rest can be backported manually later
if needed. They can be reproduced only after the conversion is applied
anyway.

Alexander Lobakin (2):
  idpf: fix memory leaks and crashes while performing a soft reset
  idpf: fix UAFs when destroying the queues

Michal Kubiak (1):
  idpf: fix memleak in vport interrupt configuration

 drivers/net/ethernet/intel/idpf/idpf_lib.c  | 48 ++++++++++-----------
 drivers/net/ethernet/intel/idpf/idpf_txrx.c | 43 ++++--------------
 2 files changed, 33 insertions(+), 58 deletions(-)

-- 
2.45.2

[PATCH iwl-net 1/3] idpf: fix memory leaks and crashes while performing a soft reset

[Alexander Lobakin]

The second tagged commit introduced a UAF, as it removed restoring
q_vector->vport pointers after reinitializating the structures.
This is due to that all queue allocation functions are performed here
with the new temporary vport structure and those functions rewrite
the backpointers to the vport. Then, this new struct is freed and
the pointers start leading to nowhere.

But generally speaking, the current logic is very fragile. It claims
to be more reliable when the system is low on memory, but in fact, it
consumes two times more memory as at the moment of running this
function, there are two vports allocated with their queues and vectors.
Moreover, it claims to prevent the driver from running into "bad state",
but in fact, any error during the rebuild leaves the old vport in the
partially allocated state.
Finally, if the interface is down when the function is called, it always
allocates a new queue set, but when the user decides to enable the
interface later on, vport_open() allocates them once again, IOW there's
a clear memory leak here.

Just don't allocate a new queue set when performing a reset, that solves
crashes and memory leaks. Readd the old queue number and reopen the
interface on rollback - that solves limbo states when the device is left
disabled and/or without HW queues enabled.

Fixes: 02cbfba1add5 ("idpf: add ethtool callbacks")
Fixes: e4891e4687c8 ("idpf: split &idpf_queue into 4 strictly-typed queue structures")
Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>
---
 drivers/net/ethernet/intel/idpf/idpf_lib.c | 30 +++++++++++-----------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c b/drivers/net/ethernet/intel/idpf/idpf_lib.c
index 5dbf2b4ba1b0..10b884dd3475 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_lib.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_lib.c
@@ -1335,9 +1335,8 @@ static void idpf_rx_init_buf_tail(struct idpf_vport *vport)
 /**
  * idpf_vport_open - Bring up a vport
  * @vport: vport to bring up
- * @alloc_res: allocate queue resources
  */
-static int idpf_vport_open(struct idpf_vport *vport, bool alloc_res)
+static int idpf_vport_open(struct idpf_vport *vport)
 {
 	struct idpf_netdev_priv *np = netdev_priv(vport->netdev);
 	struct idpf_adapter *adapter = vport->adapter;
@@ -1350,11 +1349,9 @@ static int idpf_vport_open(struct idpf_vport *vport, bool alloc_res)
 	/* we do not allow interface up just yet */
 	netif_carrier_off(vport->netdev);
 
-	if (alloc_res) {
-		err = idpf_vport_queues_alloc(vport);
-		if (err)
-			return err;
-	}
+	err = idpf_vport_queues_alloc(vport);
+	if (err)
+		return err;
 
 	err = idpf_vport_intr_alloc(vport);
 	if (err) {
@@ -1539,7 +1536,7 @@ void idpf_init_task(struct work_struct *work)
 	np = netdev_priv(vport->netdev);
 	np->state = __IDPF_VPORT_DOWN;
 	if (test_and_clear_bit(IDPF_VPORT_UP_REQUESTED, vport_config->flags))
-		idpf_vport_open(vport, true);
+		idpf_vport_open(vport);
 
 	/* Spawn and return 'idpf_init_task' work queue until all the
 	 * default vports are created
@@ -1898,9 +1895,6 @@ int idpf_initiate_soft_reset(struct idpf_vport *vport,
 		goto free_vport;
 	}
 
-	err = idpf_vport_queues_alloc(new_vport);
-	if (err)
-		goto free_vport;
 	if (current_state <= __IDPF_VPORT_DOWN) {
 		idpf_send_delete_queues_msg(vport);
 	} else {
@@ -1932,17 +1926,23 @@ int idpf_initiate_soft_reset(struct idpf_vport *vport,
 
 	err = idpf_set_real_num_queues(vport);
 	if (err)
-		goto err_reset;
+		goto err_open;
 
 	if (current_state == __IDPF_VPORT_UP)
-		err = idpf_vport_open(vport, false);
+		err = idpf_vport_open(vport);
 
 	kfree(new_vport);
 
 	return err;
 
 err_reset:
-	idpf_vport_queues_rel(new_vport);
+	idpf_send_add_queues_msg(vport, vport->num_txq, vport->num_complq,
+				 vport->num_rxq, vport->num_bufq);
+
+err_open:
+	if (current_state == __IDPF_VPORT_UP)
+		idpf_vport_open(vport);
+
 free_vport:
 	kfree(new_vport);
 
@@ -2171,7 +2171,7 @@ static int idpf_open(struct net_device *netdev)
 	idpf_vport_ctrl_lock(netdev);
 	vport = idpf_netdev_to_vport(netdev);
 
-	err = idpf_vport_open(vport, true);
+	err = idpf_vport_open(vport);
 
 	idpf_vport_ctrl_unlock(netdev);
 
-- 
2.45.2

Re: [PATCH iwl-net 1/3] idpf: fix memory leaks and crashes while performing a soft reset

[Simon Horman]

On Wed, Jul 24, 2024 at 03:40:22PM +0200, Alexander Lobakin wrote:
> The second tagged commit introduced a UAF, as it removed restoring
> q_vector->vport pointers after reinitializating the structures.
> This is due to that all queue allocation functions are performed here
> with the new temporary vport structure and those functions rewrite
> the backpointers to the vport. Then, this new struct is freed and
> the pointers start leading to nowhere.
> 
> But generally speaking, the current logic is very fragile. It claims
> to be more reliable when the system is low on memory, but in fact, it
> consumes two times more memory as at the moment of running this
> function, there are two vports allocated with their queues and vectors.
> Moreover, it claims to prevent the driver from running into "bad state",
> but in fact, any error during the rebuild leaves the old vport in the
> partially allocated state.
> Finally, if the interface is down when the function is called, it always
> allocates a new queue set, but when the user decides to enable the
> interface later on, vport_open() allocates them once again, IOW there's
> a clear memory leak here.
> 
> Just don't allocate a new queue set when performing a reset, that solves
> crashes and memory leaks. Readd the old queue number and reopen the
> interface on rollback - that solves limbo states when the device is left
> disabled and/or without HW queues enabled.
> 
> Fixes: 02cbfba1add5 ("idpf: add ethtool callbacks")
> Fixes: e4891e4687c8 ("idpf: split &idpf_queue into 4 strictly-typed queue structures")
> Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>
> ---
>  drivers/net/ethernet/intel/idpf/idpf_lib.c | 30 +++++++++++-----------
>  1 file changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c b/drivers/net/ethernet/intel/idpf/idpf_lib.c

...

> @@ -1932,17 +1926,23 @@ int idpf_initiate_soft_reset(struct idpf_vport *vport,
>  
>  	err = idpf_set_real_num_queues(vport);
>  	if (err)
> -		goto err_reset;
> +		goto err_open;
>  
>  	if (current_state == __IDPF_VPORT_UP)
> -		err = idpf_vport_open(vport, false);
> +		err = idpf_vport_open(vport);
>  
>  	kfree(new_vport);
>  
>  	return err;
>  
>  err_reset:
> -	idpf_vport_queues_rel(new_vport);
> +	idpf_send_add_queues_msg(vport, vport->num_txq, vport->num_complq,
> +				 vport->num_rxq, vport->num_bufq);
> +
> +err_open:
> +	if (current_state == __IDPF_VPORT_UP)
> +		idpf_vport_open(vport);

Hi Alexander,

Can the system end up in an odd state if this call to idpf_vport_open(), or
the one above, fails. Likewise if the above call to
idpf_send_add_queues_msg() fails.

> +
>  free_vport:
>  	kfree(new_vport);
>  

...

Re: [PATCH iwl-net 1/3] idpf: fix memory leaks and crashes while performing a soft reset

[Alexander Lobakin]

From: Simon Horman <horms@kernel.org>
Date: Fri, 26 Jul 2024 17:09:54 +0100

> On Wed, Jul 24, 2024 at 03:40:22PM +0200, Alexander Lobakin wrote:
>> The second tagged commit introduced a UAF, as it removed restoring
>> q_vector->vport pointers after reinitializating the structures.
>> This is due to that all queue allocation functions are performed here
>> with the new temporary vport structure and those functions rewrite
>> the backpointers to the vport. Then, this new struct is freed and
>> the pointers start leading to nowhere.

[...]

>>  err_reset:
>> -	idpf_vport_queues_rel(new_vport);
>> +	idpf_send_add_queues_msg(vport, vport->num_txq, vport->num_complq,
>> +				 vport->num_rxq, vport->num_bufq);
>> +
>> +err_open:
>> +	if (current_state == __IDPF_VPORT_UP)
>> +		idpf_vport_open(vport);
> 
> Hi Alexander,
> 
> Can the system end up in an odd state if this call to idpf_vport_open(), or
> the one above, fails. Likewise if the above call to
> idpf_send_add_queues_msg() fails.

Adding the queues with the parameters that were before changing them
almost can't fail. But if any of these two fails, it really will be in
an odd state...

Perhaps we need to do a more powerful reset then? Can we somehow tell
the kernel that in fact our iface is down, so that the user could try
to enable it manually once again?
Anyway, feels like a separate series or patch to -next, what do you think?

> 
>> +
>>  free_vport:
>>  	kfree(new_vport);

Thanks,
Olek

Re: [PATCH iwl-net 1/3] idpf: fix memory leaks and crashes while performing a soft reset

[Simon Horman]

On Mon, Jul 29, 2024 at 10:54:50AM +0200, Alexander Lobakin wrote:
> From: Simon Horman <horms@kernel.org>
> Date: Fri, 26 Jul 2024 17:09:54 +0100
> 
> > On Wed, Jul 24, 2024 at 03:40:22PM +0200, Alexander Lobakin wrote:
> >> The second tagged commit introduced a UAF, as it removed restoring
> >> q_vector->vport pointers after reinitializating the structures.
> >> This is due to that all queue allocation functions are performed here
> >> with the new temporary vport structure and those functions rewrite
> >> the backpointers to the vport. Then, this new struct is freed and
> >> the pointers start leading to nowhere.
> 
> [...]
> 
> >>  err_reset:
> >> -	idpf_vport_queues_rel(new_vport);
> >> +	idpf_send_add_queues_msg(vport, vport->num_txq, vport->num_complq,
> >> +				 vport->num_rxq, vport->num_bufq);
> >> +
> >> +err_open:
> >> +	if (current_state == __IDPF_VPORT_UP)
> >> +		idpf_vport_open(vport);
> > 
> > Hi Alexander,
> > 
> > Can the system end up in an odd state if this call to idpf_vport_open(), or
> > the one above, fails. Likewise if the above call to
> > idpf_send_add_queues_msg() fails.
> 
> Adding the queues with the parameters that were before changing them
> almost can't fail. But if any of these two fails, it really will be in
> an odd state...
> 
> Perhaps we need to do a more powerful reset then? Can we somehow tell
> the kernel that in fact our iface is down, so that the user could try
> to enable it manually once again?
> Anyway, feels like a separate series or patch to -next, what do you think?
> 
> > 
> >> +
> >>  free_vport:
> >>  	kfree(new_vport);
> 
> Thanks,
> Olek
> 

Re: [PATCH iwl-net 1/3] idpf: fix memory leaks and crashes while performing a soft reset

[Simon Horman]

On Mon, Jul 29, 2024 at 10:54:50AM +0200, Alexander Lobakin wrote:
> From: Simon Horman <horms@kernel.org>
> Date: Fri, 26 Jul 2024 17:09:54 +0100
> 
> > On Wed, Jul 24, 2024 at 03:40:22PM +0200, Alexander Lobakin wrote:
> >> The second tagged commit introduced a UAF, as it removed restoring
> >> q_vector->vport pointers after reinitializating the structures.
> >> This is due to that all queue allocation functions are performed here
> >> with the new temporary vport structure and those functions rewrite
> >> the backpointers to the vport. Then, this new struct is freed and
> >> the pointers start leading to nowhere.
> 
> [...]
> 
> >>  err_reset:
> >> -	idpf_vport_queues_rel(new_vport);
> >> +	idpf_send_add_queues_msg(vport, vport->num_txq, vport->num_complq,
> >> +				 vport->num_rxq, vport->num_bufq);
> >> +
> >> +err_open:
> >> +	if (current_state == __IDPF_VPORT_UP)
> >> +		idpf_vport_open(vport);
> > 
> > Hi Alexander,
> > 
> > Can the system end up in an odd state if this call to idpf_vport_open(), or
> > the one above, fails. Likewise if the above call to
> > idpf_send_add_queues_msg() fails.
> 
> Adding the queues with the parameters that were before changing them
> almost can't fail. But if any of these two fails, it really will be in
> an odd state...

Thanks for the clarification, this is my concern.

> Perhaps we need to do a more powerful reset then? Can we somehow tell
> the kernel that in fact our iface is down, so that the user could try
> to enable it manually once again?
> Anyway, feels like a separate series or patch to -next, what do you think?

Yes, sure. I agree that this patch improves things, and more extreme
corner cases can be addressed separately.

With the above in mind, I'm happy with this patch.

Reviewed-by: Simon Horman <horms@kernel.org>

RE: [PATCH iwl-net 1/3] idpf: fix memory leaks and crashes while performing a soft reset

[Singh, Krishneil K]

> -----Original Message-----
> From: Simon Horman <horms@kernel.org>
> Sent: Tuesday, July 30, 2024 9:37 AM
> To: Lobakin, Aleksander <aleksander.lobakin@intel.com>
> Cc: intel-wired-lan@lists.osuosl.org; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; David S. Miller <davem@davemloft.net>; Eric
> Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo
> Abeni <pabeni@redhat.com>; NEX SW NCIS OSDT ITP Upstreaming
> <nex.sw.ncis.osdt.itp.upstreaming@intel.com>; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH iwl-net 1/3] idpf: fix memory leaks and crashes while
> performing a soft reset
> 
> On Mon, Jul 29, 2024 at 10:54:50AM +0200, Alexander Lobakin wrote:
> > From: Simon Horman <horms@kernel.org>
> > Date: Fri, 26 Jul 2024 17:09:54 +0100
> >
> > > On Wed, Jul 24, 2024 at 03:40:22PM +0200, Alexander Lobakin wrote:
> > >> The second tagged commit introduced a UAF, as it removed restoring
> > >> q_vector->vport pointers after reinitializating the structures.
> > >> This is due to that all queue allocation functions are performed here
> > >> with the new temporary vport structure and those functions rewrite
> > >> the backpointers to the vport. Then, this new struct is freed and
> > >> the pointers start leading to nowhere.
> >
> > [...]
> >
> > >>  err_reset:
> > >> -	idpf_vport_queues_rel(new_vport);
> > >> +	idpf_send_add_queues_msg(vport, vport->num_txq, vport-
> >num_complq,
> > >> +				 vport->num_rxq, vport->num_bufq);
> > >> +
> > >> +err_open:
> > >> +	if (current_state == __IDPF_VPORT_UP)
> > >> +		idpf_vport_open(vport);
> > >
> > > Hi Alexander,
> > >
> > > Can the system end up in an odd state if this call to idpf_vport_open(), or
> > > the one above, fails. Likewise if the above call to
> > > idpf_send_add_queues_msg() fails.
> >
> > Adding the queues with the parameters that were before changing them
> > almost can't fail. But if any of these two fails, it really will be in
> > an odd state...
> 
> Thanks for the clarification, this is my concern.
> 
> > Perhaps we need to do a more powerful reset then? Can we somehow tell
> > the kernel that in fact our iface is down, so that the user could try
> > to enable it manually once again?
> > Anyway, feels like a separate series or patch to -next, what do you think?
> 
> Yes, sure. I agree that this patch improves things, and more extreme
> corner cases can be addressed separately.
> 
> With the above in mind, I'm happy with this patch.
> 
> Reviewed-by: Simon Horman <horms@kernel.org>
> 

Tested-by: Krishneil Singh <krishneil.k.singh@intel.com>

[PATCH iwl-net 2/3] idpf: fix memleak in vport interrupt configuration

[Alexander Lobakin]

From: Michal Kubiak <michal.kubiak@intel.com>

The initialization of vport interrupt consists of two functions:
 1) idpf_vport_intr_init() where a generic configuration is done
 2) idpf_vport_intr_req_irq() where the irq for each q_vector is
   requested.

The first function used to create a base name for each interrupt using
"kasprintf()" call. Unfortunately, although that call allocated memory
for a text buffer, that memory was never released.

Fix this by removing creating the interrupt base name in 1).
Instead, always create a full interrupt name in the function 2), because
there is no need to create a base name separately, considering that the
function 2) is never called out of idpf_vport_intr_init() context.

Fixes: d4d558718266 ("idpf: initialize interrupts and enable vport")
Cc: stable@vger.kernel.org # 6.7
Signed-off-by: Michal Kubiak <michal.kubiak@intel.com>
Reviewed-by: Pavan Kumar Linga <pavan.kumar.linga@intel.com>
Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>
---
 drivers/net/ethernet/intel/idpf/idpf_txrx.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/intel/idpf/idpf_txrx.c b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
index af2879f03b8d..a2f9f252694a 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_txrx.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
@@ -3780,13 +3780,15 @@ void idpf_vport_intr_update_itr_ena_irq(struct idpf_q_vector *q_vector)
 /**
  * idpf_vport_intr_req_irq - get MSI-X vectors from the OS for the vport
  * @vport: main vport structure
- * @basename: name for the vector
  */
-static int idpf_vport_intr_req_irq(struct idpf_vport *vport, char *basename)
+static int idpf_vport_intr_req_irq(struct idpf_vport *vport)
 {
 	struct idpf_adapter *adapter = vport->adapter;
+	const char *drv_name, *if_name, *vec_name;
 	int vector, err, irq_num, vidx;
-	const char *vec_name;
+
+	drv_name = dev_driver_string(&adapter->pdev->dev);
+	if_name = netdev_name(vport->netdev);
 
 	for (vector = 0; vector < vport->num_q_vectors; vector++) {
 		struct idpf_q_vector *q_vector = &vport->q_vectors[vector];
@@ -3804,8 +3806,8 @@ static int idpf_vport_intr_req_irq(struct idpf_vport *vport, char *basename)
 		else
 			continue;
 
-		name = kasprintf(GFP_KERNEL, "%s-%s-%d", basename, vec_name,
-				 vidx);
+		name = kasprintf(GFP_KERNEL, "%s-%s-%s-%d", drv_name, if_name,
+				 vec_name, vidx);
 
 		err = request_irq(irq_num, idpf_vport_intr_clean_queues, 0,
 				  name, q_vector);
@@ -4326,7 +4328,6 @@ int idpf_vport_intr_alloc(struct idpf_vport *vport)
  */
 int idpf_vport_intr_init(struct idpf_vport *vport)
 {
-	char *int_name;
 	int err;
 
 	err = idpf_vport_intr_init_vec_idx(vport);
@@ -4340,11 +4341,7 @@ int idpf_vport_intr_init(struct idpf_vport *vport)
 	if (err)
 		goto unroll_vectors_alloc;
 
-	int_name = kasprintf(GFP_KERNEL, "%s-%s",
-			     dev_driver_string(&vport->adapter->pdev->dev),
-			     vport->netdev->name);
-
-	err = idpf_vport_intr_req_irq(vport, int_name);
+	err = idpf_vport_intr_req_irq(vport);
 	if (err)
 		goto unroll_vectors_alloc;
 
-- 
2.45.2

Re: [PATCH iwl-net 2/3] idpf: fix memleak in vport interrupt configuration

[Simon Horman]

On Wed, Jul 24, 2024 at 03:40:23PM +0200, Alexander Lobakin wrote:
> From: Michal Kubiak <michal.kubiak@intel.com>
> 
> The initialization of vport interrupt consists of two functions:
>  1) idpf_vport_intr_init() where a generic configuration is done
>  2) idpf_vport_intr_req_irq() where the irq for each q_vector is
>    requested.
> 
> The first function used to create a base name for each interrupt using
> "kasprintf()" call. Unfortunately, although that call allocated memory
> for a text buffer, that memory was never released.
> 
> Fix this by removing creating the interrupt base name in 1).
> Instead, always create a full interrupt name in the function 2), because
> there is no need to create a base name separately, considering that the
> function 2) is never called out of idpf_vport_intr_init() context.
> 
> Fixes: d4d558718266 ("idpf: initialize interrupts and enable vport")
> Cc: stable@vger.kernel.org # 6.7
> Signed-off-by: Michal Kubiak <michal.kubiak@intel.com>
> Reviewed-by: Pavan Kumar Linga <pavan.kumar.linga@intel.com>
> Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>

Reviewed-by: Simon Horman <horms@kernel.org>

RE: [Intel-wired-lan] [PATCH iwl-net 2/3] idpf: fix memleak in vport interrupt configuration

[Singh, Krishneil K]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Simon Horman
> Sent: Friday, July 26, 2024 9:16 AM
> To: Lobakin, Aleksander <aleksander.lobakin@intel.com>
> Cc: Linga, Pavan Kumar <pavan.kumar.linga@intel.com>; NEX SW NCIS OSDT ITP
> Upstreaming <nex.sw.ncis.osdt.itp.upstreaming@intel.com>;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org; stable@vger.kernel.org;
> Eric Dumazet <edumazet@google.com>; Kubiak, Michal
> <michal.kubiak@intel.com>; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; Jakub Kicinski <kuba@kernel.org>; intel-wired-
> lan@lists.osuosl.org; Paolo Abeni <pabeni@redhat.com>; David S. Miller
> <davem@davemloft.net>
> Subject: Re: [Intel-wired-lan] [PATCH iwl-net 2/3] idpf: fix memleak in vport
> interrupt configuration
> 
> On Wed, Jul 24, 2024 at 03:40:23PM +0200, Alexander Lobakin wrote:
> > From: Michal Kubiak <michal.kubiak@intel.com>
> >
> > The initialization of vport interrupt consists of two functions:
> >  1) idpf_vport_intr_init() where a generic configuration is done
> >  2) idpf_vport_intr_req_irq() where the irq for each q_vector is
> >    requested.
> >
> > The first function used to create a base name for each interrupt using
> > "kasprintf()" call. Unfortunately, although that call allocated memory
> > for a text buffer, that memory was never released.
> >
> > Fix this by removing creating the interrupt base name in 1).
> > Instead, always create a full interrupt name in the function 2), because
> > there is no need to create a base name separately, considering that the
> > function 2) is never called out of idpf_vport_intr_init() context.
> >
> > Fixes: d4d558718266 ("idpf: initialize interrupts and enable vport")
> > Cc: stable@vger.kernel.org # 6.7
> > Signed-off-by: Michal Kubiak <michal.kubiak@intel.com>
> > Reviewed-by: Pavan Kumar Linga <pavan.kumar.linga@intel.com>
> > Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>
> 
> Reviewed-by: Simon Horman <horms@kernel.org>

Tested-by: Krishneil Singh <krishneil.k.singh@intel.com>

[PATCH iwl-net 3/3] idpf: fix UAFs when destroying the queues

[Alexander Lobakin]

The second tagged commit started sometimes (very rarely, but possible)
throwing WARNs from
net/core/page_pool.c:page_pool_disable_direct_recycling().
Turned out idpf frees interrupt vectors with embedded NAPIs *before*
freeing the queues making page_pools' NAPI pointers lead to freed
memory before these pools are destroyed by libeth.
It's not clear whether there are other accesses to the freed vectors
when destroying the queues, but anyway, we usually free queue/interrupt
vectors only when the queues are destroyed and the NAPIs are guaranteed
to not be referenced anywhere.

Invert the allocation and freeing logic making queue/interrupt vectors
be allocated first and freed last. Vectors don't require queues to be
present, so this is safe. Additionally, this change allows to remove
that useless queue->q_vector pointer cleanup, as vectors are still
valid when freeing the queues (+ both are freed within one function,
so it's not clear why nullify the pointers at all).

Fixes: 1c325aac10a8 ("idpf: configure resources for TX queues")
Fixes: 90912f9f4f2d ("idpf: convert header split mode to libeth + napi_build_skb()")
Reported-by: Michal Kubiak <michal.kubiak@intel.com>
Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>
---
 drivers/net/ethernet/intel/idpf/idpf_lib.c  | 24 ++++++++++-----------
 drivers/net/ethernet/intel/idpf/idpf_txrx.c | 24 +--------------------
 2 files changed, 13 insertions(+), 35 deletions(-)

diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c b/drivers/net/ethernet/intel/idpf/idpf_lib.c
index 10b884dd3475..0b6c8fd5bc90 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_lib.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_lib.c
@@ -900,8 +900,8 @@ static void idpf_vport_stop(struct idpf_vport *vport)
 
 	vport->link_up = false;
 	idpf_vport_intr_deinit(vport);
-	idpf_vport_intr_rel(vport);
 	idpf_vport_queues_rel(vport);
+	idpf_vport_intr_rel(vport);
 	np->state = __IDPF_VPORT_DOWN;
 }
 
@@ -1349,43 +1349,43 @@ static int idpf_vport_open(struct idpf_vport *vport)
 	/* we do not allow interface up just yet */
 	netif_carrier_off(vport->netdev);
 
-	err = idpf_vport_queues_alloc(vport);
-	if (err)
-		return err;
-
 	err = idpf_vport_intr_alloc(vport);
 	if (err) {
 		dev_err(&adapter->pdev->dev, "Failed to allocate interrupts for vport %u: %d\n",
 			vport->vport_id, err);
-		goto queues_rel;
+		return err;
 	}
 
+	err = idpf_vport_queues_alloc(vport);
+	if (err)
+		goto intr_rel;
+
 	err = idpf_vport_queue_ids_init(vport);
 	if (err) {
 		dev_err(&adapter->pdev->dev, "Failed to initialize queue ids for vport %u: %d\n",
 			vport->vport_id, err);
-		goto intr_rel;
+		goto queues_rel;
 	}
 
 	err = idpf_vport_intr_init(vport);
 	if (err) {
 		dev_err(&adapter->pdev->dev, "Failed to initialize interrupts for vport %u: %d\n",
 			vport->vport_id, err);
-		goto intr_rel;
+		goto queues_rel;
 	}
 
 	err = idpf_rx_bufs_init_all(vport);
 	if (err) {
 		dev_err(&adapter->pdev->dev, "Failed to initialize RX buffers for vport %u: %d\n",
 			vport->vport_id, err);
-		goto intr_rel;
+		goto queues_rel;
 	}
 
 	err = idpf_queue_reg_init(vport);
 	if (err) {
 		dev_err(&adapter->pdev->dev, "Failed to initialize queue registers for vport %u: %d\n",
 			vport->vport_id, err);
-		goto intr_rel;
+		goto queues_rel;
 	}
 
 	idpf_rx_init_buf_tail(vport);
@@ -1452,10 +1452,10 @@ static int idpf_vport_open(struct idpf_vport *vport)
 	idpf_send_map_unmap_queue_vector_msg(vport, false);
 intr_deinit:
 	idpf_vport_intr_deinit(vport);
-intr_rel:
-	idpf_vport_intr_rel(vport);
 queues_rel:
 	idpf_vport_queues_rel(vport);
+intr_rel:
+	idpf_vport_intr_rel(vport);
 
 	return err;
 }
diff --git a/drivers/net/ethernet/intel/idpf/idpf_txrx.c b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
index a2f9f252694a..585c3dadd9bf 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_txrx.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
@@ -3576,9 +3576,7 @@ static void idpf_vport_intr_napi_dis_all(struct idpf_vport *vport)
  */
 void idpf_vport_intr_rel(struct idpf_vport *vport)
 {
-	int i, j, v_idx;
-
-	for (v_idx = 0; v_idx < vport->num_q_vectors; v_idx++) {
+	for (u32 v_idx = 0; v_idx < vport->num_q_vectors; v_idx++) {
 		struct idpf_q_vector *q_vector = &vport->q_vectors[v_idx];
 
 		kfree(q_vector->complq);
@@ -3593,26 +3591,6 @@ void idpf_vport_intr_rel(struct idpf_vport *vport)
 		free_cpumask_var(q_vector->affinity_mask);
 	}
 
-	/* Clean up the mapping of queues to vectors */
-	for (i = 0; i < vport->num_rxq_grp; i++) {
-		struct idpf_rxq_group *rx_qgrp = &vport->rxq_grps[i];
-
-		if (idpf_is_queue_model_split(vport->rxq_model))
-			for (j = 0; j < rx_qgrp->splitq.num_rxq_sets; j++)
-				rx_qgrp->splitq.rxq_sets[j]->rxq.q_vector = NULL;
-		else
-			for (j = 0; j < rx_qgrp->singleq.num_rxq; j++)
-				rx_qgrp->singleq.rxqs[j]->q_vector = NULL;
-	}
-
-	if (idpf_is_queue_model_split(vport->txq_model))
-		for (i = 0; i < vport->num_txq_grp; i++)
-			vport->txq_grps[i].complq->q_vector = NULL;
-	else
-		for (i = 0; i < vport->num_txq_grp; i++)
-			for (j = 0; j < vport->txq_grps[i].num_txq; j++)
-				vport->txq_grps[i].txqs[j]->q_vector = NULL;
-
 	kfree(vport->q_vectors);
 	vport->q_vectors = NULL;
 }
-- 
2.45.2

Re: [PATCH iwl-net 3/3] idpf: fix UAFs when destroying the queues

[Simon Horman]

On Wed, Jul 24, 2024 at 03:40:24PM +0200, Alexander Lobakin wrote:
> The second tagged commit started sometimes (very rarely, but possible)
> throwing WARNs from
> net/core/page_pool.c:page_pool_disable_direct_recycling().
> Turned out idpf frees interrupt vectors with embedded NAPIs *before*
> freeing the queues making page_pools' NAPI pointers lead to freed
> memory before these pools are destroyed by libeth.
> It's not clear whether there are other accesses to the freed vectors
> when destroying the queues, but anyway, we usually free queue/interrupt
> vectors only when the queues are destroyed and the NAPIs are guaranteed
> to not be referenced anywhere.
> 
> Invert the allocation and freeing logic making queue/interrupt vectors
> be allocated first and freed last. Vectors don't require queues to be
> present, so this is safe. Additionally, this change allows to remove
> that useless queue->q_vector pointer cleanup, as vectors are still
> valid when freeing the queues (+ both are freed within one function,
> so it's not clear why nullify the pointers at all).
> 
> Fixes: 1c325aac10a8 ("idpf: configure resources for TX queues")
> Fixes: 90912f9f4f2d ("idpf: convert header split mode to libeth + napi_build_skb()")
> Reported-by: Michal Kubiak <michal.kubiak@intel.com>
> Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>

Reviewed-by: Simon Horman <horms@kernel.org>

RE: [PATCH iwl-net 3/3] idpf: fix UAFs when destroying the queues

[Singh, Krishneil K]

> -----Original Message-----
> From: Simon Horman <horms@kernel.org>
> Sent: Friday, July 26, 2024 9:22 AM
> To: Lobakin, Aleksander <aleksander.lobakin@intel.com>
> Cc: intel-wired-lan@lists.osuosl.org; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; David S. Miller <davem@davemloft.net>; Eric
> Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo
> Abeni <pabeni@redhat.com>; NEX SW NCIS OSDT ITP Upstreaming
> <nex.sw.ncis.osdt.itp.upstreaming@intel.com>; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; Kubiak, Michal <michal.kubiak@intel.com>
> Subject: Re: [PATCH iwl-net 3/3] idpf: fix UAFs when destroying the queues
> 
> On Wed, Jul 24, 2024 at 03:40:24PM +0200, Alexander Lobakin wrote:
> > The second tagged commit started sometimes (very rarely, but possible)
> > throwing WARNs from
> > net/core/page_pool.c:page_pool_disable_direct_recycling().
> > Turned out idpf frees interrupt vectors with embedded NAPIs *before*
> > freeing the queues making page_pools' NAPI pointers lead to freed
> > memory before these pools are destroyed by libeth.
> > It's not clear whether there are other accesses to the freed vectors
> > when destroying the queues, but anyway, we usually free queue/interrupt
> > vectors only when the queues are destroyed and the NAPIs are guaranteed
> > to not be referenced anywhere.
> >
> > Invert the allocation and freeing logic making queue/interrupt vectors
> > be allocated first and freed last. Vectors don't require queues to be
> > present, so this is safe. Additionally, this change allows to remove
> > that useless queue->q_vector pointer cleanup, as vectors are still
> > valid when freeing the queues (+ both are freed within one function,
> > so it's not clear why nullify the pointers at all).
> >
> > Fixes: 1c325aac10a8 ("idpf: configure resources for TX queues")
> > Fixes: 90912f9f4f2d ("idpf: convert header split mode to libeth +
> napi_build_skb()")
> > Reported-by: Michal Kubiak <michal.kubiak@intel.com>
> > Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>
> 
> Reviewed-by: Simon Horman <horms@kernel.org>
> 

Tested-by: Krishneil Singh <krishneil.k.singh@intel.com>