[PATCH net 0/2] ibmvnic: Fix TX skb leak after device reset

[PATCH net 0/2] ibmvnic: Fix TX skb leak after device reset

[Nick Child]

These 2 patches focus on resolving a possible skb leak after
a subset of the ibmvnic reset processes.

Essentially, the driver maintains a free_map which contains indexes to a
list of tracked skb's addresses on xmit. Due to a mistake during reset,
the free_map did not accurately map to free indexes in the skb list.
This resulted in a leak in skb because the index in free_map was blindly
trusted to contain a NULL pointer. So this patchset addresses 2 issues:
  1. We shouldn't blindly trust our free_map (lets not do this again)
  2. We need to ensure that our free_map is accurate in the first place

The first patch is more cautionary to detect these leaks in any future
bugs (while also helping to justify the leak fixed in the second patch).
In this case it is due to device resets which free the tx complete irq
but do not free the outstanding skb's which would have been freed by the
irq handler ibmvnic_complete_tx().

These outstanding SKB's MUST be freed any time we free the IRQ. We are
not going to get an IRQ to free them later on! Also, further in the
reset path init_tx_pools() is going to mark all buffers free! This is
addressed by the second patch.

Nick Child (2):
  ibmvnic: Add tx check to prevent skb leak
  ibmvnic: Free any outstanding tx skbs during scrq reset

 drivers/net/ethernet/ibm/ibmvnic.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

-- 
2.39.3

[PATCH net 1/2] ibmvnic: Add tx check to prevent skb leak

[Nick Child]

Below is a summary of how the driver stores a reference to an skb during
transmit:
    tx_buff[free_map[consumer_index]]->skb = new_skb;
    free_map[consumer_index] = IBMVNIC_INVALID_MAP;
    consumer_index ++;
Where variable data looks like this:
    free_map == [4, IBMVNIC_INVALID_MAP, IBMVNIC_INVALID_MAP, 0, 3]
                                               	consumer_index^
    tx_buff == [skb=null, skb=<ptr>, skb=<ptr>, skb=null, skb=null]

The driver has checks to ensure that free_map[consumer_index] pointed to
a valid index but there was no check to ensure that this index pointed
to an unused/null skb address. So, if, by some chance, our free_map and
tx_buff lists become out of sync then we were previously risking an
skb memory leak. This could then cause tcp congestion control to stop
sending packets, eventually leading to ETIMEDOUT.

Therefore, add a conditional to ensure that the skb address is null. If
not then warn the user (because this is still a bug that should be
patched) and free the old pointer to prevent memleak/tcp problems.

Signed-off-by: Nick Child <nnac123@linux.ibm.com>
---
 drivers/net/ethernet/ibm/ibmvnic.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index 5e9a93bdb518..887d92a88403 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -2482,6 +2482,18 @@ static netdev_tx_t ibmvnic_xmit(struct sk_buff *skb, struct net_device *netdev)
 	    (tx_pool->consumer_index + 1) % tx_pool->num_buffers;
 
 	tx_buff = &tx_pool->tx_buff[bufidx];
+
+	/* Sanity checks on our free map to make sure it points to an index
+	 * that is not being occupied by another skb. If skb memory is
+	 * not freed then we see congestion control kick in and halt tx.
+	 */
+	if (unlikely(tx_buff->skb)) {
+		dev_warn_ratelimited(dev, "TX free map points to untracked skb (%s %d idx=%d)\n",
+				     skb_is_gso(skb) ? "tso_pool" : "tx_pool",
+				     queue_num, bufidx);
+		dev_kfree_skb_any(tx_buff->skb);
+	}
+
 	tx_buff->skb = skb;
 	tx_buff->index = bufidx;
 	tx_buff->pool_index = queue_num;
-- 
2.39.3

Re: [PATCH net 1/2] ibmvnic: Add tx check to prevent skb leak

[Paolo Abeni]

On Thu, 2024-06-20 at 10:23 -0500, Nick Child wrote:
> Below is a summary of how the driver stores a reference to an skb during
> transmit:
>     tx_buff[free_map[consumer_index]]->skb = new_skb;
>     free_map[consumer_index] = IBMVNIC_INVALID_MAP;
>     consumer_index ++;
> Where variable data looks like this:
>     free_map == [4, IBMVNIC_INVALID_MAP, IBMVNIC_INVALID_MAP, 0, 3]
>                                                	consumer_index^
>     tx_buff == [skb=null, skb=<ptr>, skb=<ptr>, skb=null, skb=null]
> 
> The driver has checks to ensure that free_map[consumer_index] pointed to
> a valid index but there was no check to ensure that this index pointed
> to an unused/null skb address. So, if, by some chance, our free_map and
> tx_buff lists become out of sync then we were previously risking an
> skb memory leak. This could then cause tcp congestion control to stop
> sending packets, eventually leading to ETIMEDOUT.
> 
> Therefore, add a conditional to ensure that the skb address is null. If
> not then warn the user (because this is still a bug that should be
> patched) and free the old pointer to prevent memleak/tcp problems.
> 
> Signed-off-by: Nick Child <nnac123@linux.ibm.com>
> ---
>  drivers/net/ethernet/ibm/ibmvnic.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
> index 5e9a93bdb518..887d92a88403 100644

For some reasons, this one was not applied together with patch 2/2.

I'm applying it now.

Cheers,

Paolo

[PATCH net 2/2] ibmvnic: Free any outstanding tx skbs during scrq reset

[Nick Child]

There are 2 types of outstanding tx skb's:
Type 1: Packets that are sitting in the drivers ind_buff that are
waiting to be batch sent to the NIC. During a device reset, these are
freed with a call to ibmvnic_tx_scrq_clean_buffer()
Type 2: Packets that have been sent to the NIC and are awaiting a TX
completion IRQ. These are free'd during a reset with a call to
clean_tx_pools()

During any reset which requires us to free the tx irq, ensure that the
Type 2 skb references are freed. Since the irq is released, it is
impossible for the NIC to inform of any completions.

Furthermore, later in the reset process is a call to init_tx_pools()
which marks every entry in the tx pool as free (ie not outstanding).
So if the driver is to make a call to init_tx_pools(), it must first
be sure that the tx pool is empty of skb references.

This issue was discovered by observing the following in the logs during
EEH testing:
	TX free map points to untracked skb (tso_pool 0 idx=4)
	TX free map points to untracked skb (tso_pool 0 idx=5)
	TX free map points to untracked skb (tso_pool 1 idx=36)

Fixes: 65d6470d139a ("ibmvnic: clean pending indirect buffs during reset")
Signed-off-by: Nick Child <nnac123@linux.ibm.com>
---
 drivers/net/ethernet/ibm/ibmvnic.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index 887d92a88403..23ebeb143987 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -4073,6 +4073,12 @@ static void release_sub_crqs(struct ibmvnic_adapter *adapter, bool do_h_free)
 		adapter->num_active_tx_scrqs = 0;
 	}
 
+	/* Clean any remaining outstanding SKBs
+	 * we freed the irq so we won't be hearing
+	 * from them
+	 */
+	clean_tx_pools(adapter);
+
 	if (adapter->rx_scrq) {
 		for (i = 0; i < adapter->num_active_rx_scrqs; i++) {
 			if (!adapter->rx_scrq[i])
-- 
2.39.3

Re: [PATCH net 0/2] ibmvnic: Fix TX skb leak after device reset

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

On Thu, 20 Jun 2024 10:23:10 -0500 you wrote:
> These 2 patches focus on resolving a possible skb leak after
> a subset of the ibmvnic reset processes.
> 
> Essentially, the driver maintains a free_map which contains indexes to a
> list of tracked skb's addresses on xmit. Due to a mistake during reset,
> the free_map did not accurately map to free indexes in the skb list.
> This resulted in a leak in skb because the index in free_map was blindly
> trusted to contain a NULL pointer. So this patchset addresses 2 issues:
>   1. We shouldn't blindly trust our free_map (lets not do this again)
>   2. We need to ensure that our free_map is accurate in the first place
> 
> [...]

Here is the summary with links:
  - [net,1/2] ibmvnic: Add tx check to prevent skb leak
    (no matching commit)
  - [net,2/2] ibmvnic: Free any outstanding tx skbs during scrq reset
    https://git.kernel.org/netdev/net/c/49bbeb5719c2

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net 0/2] ibmvnic: Fix TX skb leak after device reset

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Thu, 20 Jun 2024 10:23:10 -0500 you wrote:
> These 2 patches focus on resolving a possible skb leak after
> a subset of the ibmvnic reset processes.
> 
> Essentially, the driver maintains a free_map which contains indexes to a
> list of tracked skb's addresses on xmit. Due to a mistake during reset,
> the free_map did not accurately map to free indexes in the skb list.
> This resulted in a leak in skb because the index in free_map was blindly
> trusted to contain a NULL pointer. So this patchset addresses 2 issues:
>   1. We shouldn't blindly trust our free_map (lets not do this again)
>   2. We need to ensure that our free_map is accurate in the first place
> 
> [...]

Here is the summary with links:
  - [net,1/2] ibmvnic: Add tx check to prevent skb leak
    https://git.kernel.org/netdev/net/c/0983d288caf9
  - [net,2/2] ibmvnic: Free any outstanding tx skbs during scrq reset
    (no matching commit)

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html