[PATCH] net: ixgbe: Fix memleak in ixgbe_configure_clsu32

When ixgbe_fdir_write_perfect_filter_82599() fails,
input allocated by kzalloc() has not been freed,
which leads to memleak.

Signed-off-by: Dinghao Liu <redacted>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 393d1c2cd853..e9c2d28efc81 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c

@@ -9582,8 +9582,10 @@ static int ixgbe_configure_clsu32(struct ixgbe_adapter *adapter,
 	ixgbe_atr_compute_perfect_hash_82599(&input->filter, mask);
 	err = ixgbe_fdir_write_perfect_filter_82599(hw, &input->filter,
 						    input->sw_idx, queue);
-	if (!err)
-		ixgbe_update_ethtool_fdir_entry(adapter, input, input->sw_idx);
+	if (err)
+		goto err_out_w_lock;
+
+	ixgbe_update_ethtool_fdir_entry(adapter, input, input->sw_idx);
 	spin_unlock(&adapter->fdir_perfect_lock);
 
 	if ((uhtid != 0x800) && (adapter->jump_tables[uhtid]))

-- 
2.17.1

Dear Dinghao,


Am 03.01.21 um 09:08 schrieb Dinghao Liu:

When ixgbe_fdir_write_perfect_filter_82599() fails,
input allocated by kzalloc() has not been freed,
which leads to memleak.

Nice find. Thank you for your patches. Out of curiosity, do you use an 
analysis tool to find these issues?

Reviewed-by: Paul Menzel <redacted>

I wonder, in the non-error case, how `input` and `jump` are freed.

Old code:

        if (!err)
                ixgbe_update_ethtool_fdir_entry(adapter, input, input->sw_idx);
        spin_unlock(&adapter->fdir_perfect_lock);

        if ((uhtid != 0x800) && (adapter->jump_tables[uhtid]))
                set_bit(loc - 1, (adapter->jump_tables[uhtid])->child_loc_map);

        kfree(mask);
        return err;

Should these two lines be replaced with `goto err_out`? (Though the name 
is confusing then, as it’s the non-error case.)

err_out_w_lock:
        spin_unlock(&adapter->fdir_perfect_lock);
err_out:
        kfree(mask);
free_input:
        kfree(input);
free_jump:
        kfree(jump);
        return err;
}

Kind regards,

Paul

Dear Dinghao,


Am 03.01.21 um 09:08 schrieb Dinghao Liu:

quoted

When ixgbe_fdir_write_perfect_filter_82599() fails,
input allocated by kzalloc() has not been freed,
which leads to memleak.

Nice find. Thank you for your patches. Out of curiosity, do you use an 
analysis tool to find these issues?

Yes, these bugs are suggested by my static analysis tool. 

quoted

Signed-off-by: Dinghao Liu <redacted>
---
  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 6 ++++--
  1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 393d1c2cd853..e9c2d28efc81 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c

@@ -9582,8 +9582,10 @@ static int ixgbe_configure_clsu32(struct ixgbe_adapter *adapter,
  	ixgbe_atr_compute_perfect_hash_82599(&input->filter, mask);
  	err = ixgbe_fdir_write_perfect_filter_82599(hw, &input->filter,
  						    input->sw_idx, queue);
-	if (!err)
-		ixgbe_update_ethtool_fdir_entry(adapter, input, input->sw_idx);
+	if (err)
+		goto err_out_w_lock;
+
+	ixgbe_update_ethtool_fdir_entry(adapter, input, input->sw_idx);
  	spin_unlock(&adapter->fdir_perfect_lock);
  
  	if ((uhtid != 0x800) && (adapter->jump_tables[uhtid]))

Reviewed-by: Paul Menzel <redacted>

I wonder, in the non-error case, how `input` and `jump` are freed.

I'm not sure if kfree(jump) will introduce bug. jump is allocated in a for 
loop and has been passed to adapter->jump_tables. input and mask have new
definitions (kzalloc) after this loop, it's reasonable to free them on failure.
But jump is different. Maybe we should not free jump after the loop?

Old code:

quoted

        if (!err)
                ixgbe_update_ethtool_fdir_entry(adapter, input, input->sw_idx);
        spin_unlock(&adapter->fdir_perfect_lock);

        if ((uhtid != 0x800) && (adapter->jump_tables[uhtid]))
                set_bit(loc - 1, (adapter->jump_tables[uhtid])->child_loc_map);

        kfree(mask);
        return err;

Should these two lines be replaced with `goto err_out`? (Though the name 
is confusing then, as it’s the non-error case.)

This also makes me confused. It seems that the check against untied is not error
handling code, but there is a kfree(mask) after it. Freeing allocated data on
success is not reasonable. 

Regards,
Dinghao

quoted

err_out_w_lock:
        spin_unlock(&adapter->fdir_perfect_lock);
err_out:
        kfree(mask);
free_input:
        kfree(input);
free_jump:
        kfree(jump);
        return err;
}

Kind regards,

Paul