[PATCH iwl-net v1] idpf: fix potential memory leak on kcalloc() failure

[PATCH iwl-net v1] idpf: fix potential memory leak on kcalloc() failure

[Michal Swiatkowski]

In case of failing on rss_data->rss_key allocation the function is
freeing vport without freeing earlier allocated q_vector_idxs. Fix it.

Move from freeing in error branch to goto scheme.

Fixes: 95af467d9a4e ("idpf: configure resources for RX queues")
Reviewed-by: Pavan Kumar Linga <pavan.kumar.linga@intel.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Suggested-by: Pavan Kumar Linga <pavan.kumar.linga@intel.com>
Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
---
 drivers/net/ethernet/intel/idpf/idpf_lib.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c b/drivers/net/ethernet/intel/idpf/idpf_lib.c
index aa755dedb41d..329ba53e86fd 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_lib.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_lib.c
@@ -1132,11 +1132,9 @@ static struct idpf_vport *idpf_vport_alloc(struct idpf_adapter *adapter,
 
 	num_max_q = max(max_q->max_txq, max_q->max_rxq);
 	vport->q_vector_idxs = kcalloc(num_max_q, sizeof(u16), GFP_KERNEL);
-	if (!vport->q_vector_idxs) {
-		kfree(vport);
+	if (!vport->q_vector_idxs)
+		goto free_vport;
 
-		return NULL;
-	}
 	idpf_vport_init(vport, max_q);
 
 	/* This alloc is done separate from the LUT because it's not strictly
@@ -1146,11 +1144,9 @@ static struct idpf_vport *idpf_vport_alloc(struct idpf_adapter *adapter,
 	 */
 	rss_data = &adapter->vport_config[idx]->user_config.rss_data;
 	rss_data->rss_key = kzalloc(rss_data->rss_key_size, GFP_KERNEL);
-	if (!rss_data->rss_key) {
-		kfree(vport);
+	if (!rss_data->rss_key)
+		goto free_vector_idxs;
 
-		return NULL;
-	}
 	/* Initialize default rss key */
 	netdev_rss_key_fill((void *)rss_data->rss_key, rss_data->rss_key_size);
 
@@ -1163,6 +1159,13 @@ static struct idpf_vport *idpf_vport_alloc(struct idpf_adapter *adapter,
 	adapter->next_vport = idpf_get_free_slot(adapter);
 
 	return vport;
+
+free_vector_idxs:
+	kfree(vport->q_vector_idxs);
+free_vport:
+	kfree(vport);
+
+	return NULL;
 }
 
 /**
-- 
2.42.0

Re: [PATCH iwl-net v1] idpf: fix potential memory leak on kcalloc() failure

[Simon Horman]

On Fri, Apr 04, 2025 at 12:54:21PM +0200, Michal Swiatkowski wrote:
> In case of failing on rss_data->rss_key allocation the function is
> freeing vport without freeing earlier allocated q_vector_idxs. Fix it.
> 
> Move from freeing in error branch to goto scheme.
> 
> Fixes: 95af467d9a4e ("idpf: configure resources for RX queues")

Hi Michal,

WRT leaking q_vector_indxs, that allocation is not present at
the commit cited above, so I think the correct Fixes tag for
that problem is the following, where that allocation was added:

Fixes: d4d558718266 ("idpf: initialize interrupts and enable vport")

I do note that adapter->vport_config[idx] may be allocated but
not freed on error in idpf_vport_alloc(). But I assume that this
is not a leak as it will eventually be cleaned up by idpf_remove().

So the Fixes tag not withstanding this looks good to me.

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

> Reviewed-by: Pavan Kumar Linga <pavan.kumar.linga@intel.com>
> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> Suggested-by: Pavan Kumar Linga <pavan.kumar.linga@intel.com>
> Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>

...

Re: [Intel-wired-lan] [PATCH iwl-net v1] idpf: fix potential memory leak on kcalloc() failure

[Tony Nguyen]

On 4/7/2025 3:43 AM, Simon Horman wrote:
> On Fri, Apr 04, 2025 at 12:54:21PM +0200, Michal Swiatkowski wrote:
>> In case of failing on rss_data->rss_key allocation the function is
>> freeing vport without freeing earlier allocated q_vector_idxs. Fix it.
>>
>> Move from freeing in error branch to goto scheme.
>>
>> Fixes: 95af467d9a4e ("idpf: configure resources for RX queues")
> 
> Hi Michal,
> 
> WRT leaking q_vector_indxs, that allocation is not present at
> the commit cited above, so I think the correct Fixes tag for
> that problem is the following, where that allocation was added:
> 
> Fixes: d4d558718266 ("idpf: initialize interrupts and enable vport")

Patch applied. I do agree with Simon's assessment so plan to use this fixes.

Thanks,
Tony

Re: [Intel-wired-lan] [PATCH iwl-net v1] idpf: fix potential memory leak on kcalloc() failure

[Michal Swiatkowski]

On Thu, Apr 10, 2025 at 03:04:16PM -0700, Tony Nguyen wrote:
> On 4/7/2025 3:43 AM, Simon Horman wrote:
> > On Fri, Apr 04, 2025 at 12:54:21PM +0200, Michal Swiatkowski wrote:
> > > In case of failing on rss_data->rss_key allocation the function is
> > > freeing vport without freeing earlier allocated q_vector_idxs. Fix it.
> > > 
> > > Move from freeing in error branch to goto scheme.
> > > 
> > > Fixes: 95af467d9a4e ("idpf: configure resources for RX queues")
> > 
> > Hi Michal,
> > 
> > WRT leaking q_vector_indxs, that allocation is not present at
> > the commit cited above, so I think the correct Fixes tag for
> > that problem is the following, where that allocation was added:
> > 
> > Fixes: d4d558718266 ("idpf: initialize interrupts and enable vport")
> 
> Patch applied. I do agree with Simon's assessment so plan to use this fixes.
> 

Thanks Tony, I also agree.

> Thanks,
> Tony
> 

Re: [PATCH iwl-net v1] idpf: fix potential memory leak on kcalloc() failure

[Michal Swiatkowski]

On Mon, Apr 07, 2025 at 11:43:50AM +0100, Simon Horman wrote:
> On Fri, Apr 04, 2025 at 12:54:21PM +0200, Michal Swiatkowski wrote:
> > In case of failing on rss_data->rss_key allocation the function is
> > freeing vport without freeing earlier allocated q_vector_idxs. Fix it.
> > 
> > Move from freeing in error branch to goto scheme.
> > 
> > Fixes: 95af467d9a4e ("idpf: configure resources for RX queues")
> 
> Hi Michal,
> 
> WRT leaking q_vector_indxs, that allocation is not present at
> the commit cited above, so I think the correct Fixes tag for
> that problem is the following, where that allocation was added:
> 
> Fixes: d4d558718266 ("idpf: initialize interrupts and enable vport")

Thanks for checking that. I agree, my fixes is wrong.

> 
> I do note that adapter->vport_config[idx] may be allocated but
> not freed on error in idpf_vport_alloc(). But I assume that this
> is not a leak as it will eventually be cleaned up by idpf_remove().

Right, it will be better to free it directly for better readable.
Probably candidate for net-next changes.

> 
> So the Fixes tag not withstanding this looks good to me.
> 
> Reviewed-by: Simon Horman <horms@kernel.org>
>

Thanks for review.

> > Reviewed-by: Pavan Kumar Linga <pavan.kumar.linga@intel.com>
> > Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> > Suggested-by: Pavan Kumar Linga <pavan.kumar.linga@intel.com>
> > Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> 
> ...

Re: [PATCH iwl-net v1] idpf: fix potential memory leak on kcalloc() failure

[Simon Horman]

On Fri, Apr 11, 2025 at 07:09:12AM +0200, Michal Swiatkowski wrote:
> On Mon, Apr 07, 2025 at 11:43:50AM +0100, Simon Horman wrote:
> > On Fri, Apr 04, 2025 at 12:54:21PM +0200, Michal Swiatkowski wrote:
> > > In case of failing on rss_data->rss_key allocation the function is
> > > freeing vport without freeing earlier allocated q_vector_idxs. Fix it.
> > > 
> > > Move from freeing in error branch to goto scheme.
> > > 
> > > Fixes: 95af467d9a4e ("idpf: configure resources for RX queues")
> > 
> > Hi Michal,
> > 
> > WRT leaking q_vector_indxs, that allocation is not present at
> > the commit cited above, so I think the correct Fixes tag for
> > that problem is the following, where that allocation was added:
> > 
> > Fixes: d4d558718266 ("idpf: initialize interrupts and enable vport")
> 
> Thanks for checking that. I agree, my fixes is wrong.
> 
> > 
> > I do note that adapter->vport_config[idx] may be allocated but
> > not freed on error in idpf_vport_alloc(). But I assume that this
> > is not a leak as it will eventually be cleaned up by idpf_remove().
> 
> Right, it will be better to free it directly for better readable.
> Probably candidate for net-next changes.

Thanks, that does sound like a nice idea.

...

RE: [Intel-wired-lan] [PATCH iwl-net v1] idpf: fix potential memory leak on kcalloc() failure

[Salin, Samuel]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Michal Swiatkowski
> Sent: Friday, April 4, 2025 3:54 AM
> To: intel-wired-lan@lists.osuosl.org
> Cc: netdev@vger.kernel.org; Michal Swiatkowski
> <michal.swiatkowski@linux.intel.com>; Linga, Pavan Kumar
> <pavan.kumar.linga@intel.com>; Loktionov, Aleksandr
> <aleksandr.loktionov@intel.com>
> Subject: [Intel-wired-lan] [PATCH iwl-net v1] idpf: fix potential memory leak
> on kcalloc() failure
> 
> In case of failing on rss_data->rss_key allocation the function is freeing vport
> without freeing earlier allocated q_vector_idxs. Fix it.
> 
> Move from freeing in error branch to goto scheme.
> 
> Fixes: 95af467d9a4e ("idpf: configure resources for RX queues")
> Reviewed-by: Pavan Kumar Linga <pavan.kumar.linga@intel.com>
> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> Suggested-by: Pavan Kumar Linga <pavan.kumar.linga@intel.com>
> Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> ---
> 2.42.0

Tested-by: Samuel Salin <Samuel.salin@intel.com>