ibmveth: fix open-fail unwind after LAN registration
Subsystems (5)
cover ibmveth: fix open-fail unwind after LAN registration
Hi,
ibmveth_open() has two independent unwind holes after the logical LAN is set up. Neither needs the MQ RX series. This posting is against current net.git.
Patch 1 issues h_free_logical_lan() on every post-register failure. A pool allocation failure jumped to out_free_buffer_pools without the hypercall, so PHYP still owned the buffer-list page when it was unmapped. request_irq() failure already issued H_FREE before taking the same label.
Patch 2 gives TX LTBs their own walk. out_free_buffer_pools reuses open()'s loop index, so after the pool unwind i is -1 and the TX buffers leak. request_irq() failure has the same leak. The TX-fail goto on that walk also skips dma_unmap of the filter list (pre-existing since the LTB was added).
The MQ RX series on net-next keeps the helper versions of these guards and does not depend on this pair. If both land, keep the helpers in that series; these two patches are the current single-queue open-fail path only.
Mingming
Mingming Cao (2):
ibmveth: h_free logical LAN on open-fail after register
ibmveth: fix TX LTB and filter unwind on open-fail
drivers/net/ethernet/ibm/ibmveth.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)
base-commit: 11536ee3d3e0b1bd35b6f3f8df55a6053eb0c71d
Signature · 1 line
2.50.1 (Apple Git-155)
PATCH 1/2 ibmveth: h_free logical LAN on open-fail after register
ibmveth_open() registers the logical LAN, then allocates the RX buffer pools. A pool failure jumps to out_free_buffer_pools without h_free_logical_lan(), so PHYP still owns the buffer-list page when it is unmapped. request_irq() failure already issued the hypercall before taking the same label.
Move that h_free to the shared post-register unwind so both paths deregister before the buffer list is unmapped.
--- drivers/net/ethernet/ibm/ibmveth.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
drivers/net/ethernet/ibm/ibmveth.c +3 -4
@@ -718,10 +718,6 @@ static int ibmveth_open(struct net_device *netdev)if (rc !=0 ) {netdev_err (netdev,"unable to request irq 0x%x, rc %d\n" , netdev->irq, rc); -do { - lpar_rc =h_free_logical_lan (adapter->vdev->unit_address); - }while (H_IS_LONG_BUSY (lpar_rc) || (lpar_rc ==H_BUSY )); - goto out_free_buffer_pools; }
@@ -737,6 +733,9 @@ static int ibmveth_open(struct net_device *netdev)return 0 ; out_free_buffer_pools: +do { + lpar_rc =h_free_logical_lan (adapter->vdev->unit_address); + }while (H_IS_LONG_BUSY (lpar_rc) || (lpar_rc ==H_BUSY ));while (--i >=0 ) {if (adapter->rx_buff_pool[i].active) ibmveth_free_buffer_pool(adapter,
Signature · 1 line
2.50.1 (Apple Git-155)
Re: [PATCH net 1/2] ibmveth: h_free logical LAN on open-fail after register
Quoted 21 lines · Mingming Cao
ibmveth_open()registers the logical LAN, then allocates the RX buffer pools. A pool failure jumps toout_free_buffer_poolswithouth_free_logical_lan(), so PHYP still owns the buffer-list page when it is unmapped.request_irq()failure already issued the hypercall before taking the same label.Move that
h_freeto the shared post-register unwind so both paths deregister before the buffer list is unmapped.Fixes d43732ce021f ("ibmveth: properly unwind on init errors")Signed-off-by Mingming Cao <[email protected]>--- drivers/net/ethernet/ibm/ibmveth.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)drivers/net/ethernet/ibm/ibmveth.c +0
-0
drivers/net/ethernet/ibm/ibmveth.c static int ibmveth_open(struct net_device *netdev) · line 722
Quoted 9 lines
drivers/net/ethernet/ibm/ibmveth.c +0
-4@@ -718,10 +718,6 @@ static int ibmveth_open(struct net_device *netdev)if (rc !=0 ) {netdev_err (netdev,"unable to request irq 0x%x, rc %d\n" , netdev->irq, rc); -do { - lpar_rc =h_free_logical_lan (adapter->vdev->unit_address); - }while (H_IS_LONG_BUSY (lpar_rc) || (lpar_rc ==H_BUSY )); - goto out_free_buffer_pools; }
This code change is good. But I noticed another thing that's a pretty minor bug of useless code just after this:
rc = -ENOMEM;
rc isn't used in that code path after it is set. Is there some reason I do not understand to have this here?
drivers/net/ethernet/ibm/ibmveth.c static int ibmveth_open(struct net_device *netdev) · line 741
Quoted 10 lines
drivers/net/ethernet/ibm/ibmveth.c +3
-0@@ -737,6 +733,9 @@ static int ibmveth_open(struct net_device *netdev)return 0 ; out_free_buffer_pools: +do { + lpar_rc =h_free_logical_lan (adapter->vdev->unit_address); + }while (H_IS_LONG_BUSY (lpar_rc) || (lpar_rc ==H_BUSY ));while (--i >=0 ) {if (adapter->rx_buff_pool[i].active) ibmveth_free_buffer_pool(adapter,
Re: [PATCH net 1/2] ibmveth: h_free logical LAN on open-fail after register
Quoted 7 lines · Dave Marquardt
This code change is good. But I noticed another thing that's a pretty minor bug of useless code just after this:
rc = -ENOMEM;rc isn't used in that code path after it is set. Is there some reason I do not understand to have this here?
Yeah, that |rc = -ENOMEM| at line 728 is dead — set right before |return 0| and never read again. Pre-existing junk. |d43732| set it for the bounce alloc that used to run next. |d6832| moved the TX LTB earlier and dropped the bounce, but left the line. After |request_irq| succeeds we replenish, start TX, and return 0.
It's harmless (not user-visible) — I prefer leaving it out of this Fixes and letting the MQ series clean it up naturally when it rewrites that whole open() path. But fine to respin a new version if desired.
Thanks for the review.
Mingming
Re: [PATCH net 1/2] ibmveth: h_free logical LAN on open-fail after register
Quoted 10 lines · mingming cao
Quoted 7 lines · Dave Marquardt
This code change is good. But I noticed another thing that's a pretty minor bug of useless code just after this:
rc = -ENOMEM;rc isn't used in that code path after it is set. Is there some reason I do not understand to have this here?
Yeah, that |rc = -ENOMEM| at line 728 is dead — set right before
Quoted 8 lines
|return 0| and never read again. Pre-existing junk. |d43732| set it for the bounce alloc that used to run next. |d6832| moved the TX LTB earlier and dropped the bounce, but left the line. After |request_irq|succeeds we replenish, start TX, and return 0.
It's harmless (not user-visible) — I prefer leaving it out of this Fixes and letting the MQ series clean it up naturally when it rewrites that whole
open()path. But fine to respin a new version if desired.
Agreed.
-Dave
PATCH 2/2 ibmveth: fix TX LTB and filter unwind on open-fail
In reply to Mingming Caoout_free_buffer_pools reuses open()'s loop index for the TX LTB walk. After the pool unwind i is -1, so the TX buffers allocated earlier are leaked. request_irq() failure has the same leak.
The TX-fail goto on that walk also skips dma_unmap of the filter list (pre-existing since the LTB was added). Rewrite the TX walk to real_num_tx_queues with a pointer check, and unmap the filter list before those frees.
--- drivers/net/ethernet/ibm/ibmveth.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)
drivers/net/ethernet/ibm/ibmveth.c +9 -6
@@ -553,10 +553,15 @@ static int ibmveth_rxq_harvest_buffer(struct ibmveth_adapter *adapter,static void ibmveth_free_tx_ltb (struct ibmveth_adapter *adapter,int idx) { +void *ptr = adapter->tx_ltb_ptr[idx]; + +if (!ptr) +return ; + + adapter->tx_ltb_ptr[idx] =NULL ;dma_unmap_single (&adapter->vdev->dev, adapter->tx_ltb_dma[idx], adapter->tx_ltb_size,DMA_TO_DEVICE ); -kfree (adapter->tx_ltb_ptr[idx]); - adapter->tx_ltb_ptr[idx] =NULL ; +kfree (ptr); }static int ibmveth_allocate_tx_ltb (struct ibmveth_adapter *adapter,int idx)
@@ -667,7 +672,7 @@ static int ibmveth_open(struct net_device *netdev)for (i =0 ; i < netdev->real_num_tx_queues; i++) {if (ibmveth_allocate_tx_ltb (adapter, i)) - goto out_free_tx_ltb; + goto out_unmap_filter_list; } adapter->rx_queue.index =0 ;
@@ -745,10 +750,8 @@ static int ibmveth_open(struct net_device *netdev)dma_unmap_single (dev, adapter->filter_list_dma,4096 ,DMA_BIDIRECTIONAL ); -out_free_tx_ltb: -while (--i >=0 ) { +for (i = netdev->real_num_tx_queues -1 ; i >=0 ; i--)ibmveth_free_tx_ltb (adapter, i); - } out_unmap_buffer_list: dma_unmap_single(dev, adapter->buffer_list_dma, 4096,
Signature · 1 line
2.50.1 (Apple Git-155)
Re: [PATCH net 2/2] ibmveth: fix TX LTB and filter unwind on open-fail
Quoted 49 lines · Mingming Cao
out_free_buffer_poolsreuses open()'s loop index for the TX LTB walk. After the pool unwind i is -1, so the TX buffers allocated earlier are leaked.request_irq()failure has the same leak.The TX-fail goto on that walk also skips
dma_unmapof the filter list (pre-existing since the LTB was added). Rewrite the TX walk toreal_num_tx_queueswith a pointer check, and unmap the filter list before those frees.Fixes d926793c1de9 ("ibmveth: Implement multi queue on xmit")Signed-off-by Mingming Cao <[email protected]>--- drivers/net/ethernet/ibm/ibmveth.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)drivers/net/ethernet/ibm/ibmveth.c +8
-3@@ -553,10 +553,15 @@ static int ibmveth_rxq_harvest_buffer(struct ibmveth_adapter *adapter,static void ibmveth_free_tx_ltb (struct ibmveth_adapter *adapter,int idx) { +void *ptr = adapter->tx_ltb_ptr[idx]; + +if (!ptr) +return ; + + adapter->tx_ltb_ptr[idx] =NULL ;dma_unmap_single (&adapter->vdev->dev, adapter->tx_ltb_dma[idx], adapter->tx_ltb_size,DMA_TO_DEVICE ); -kfree (adapter->tx_ltb_ptr[idx]); - adapter->tx_ltb_ptr[idx] =NULL ; +kfree (ptr); }static int ibmveth_allocate_tx_ltb (struct ibmveth_adapter *adapter,int idx)@@ -667,7 +672,7 @@ static int ibmveth_open(struct net_device *netdev)for (i =0 ; i < netdev->real_num_tx_queues; i++) {if (ibmveth_allocate_tx_ltb (adapter, i)) - goto out_free_tx_ltb; + goto out_unmap_filter_list; } adapter->rx_queue.index =0 ;@@ -745,10 +750,8 @@ static int ibmveth_open(struct net_device *netdev) dma_unmap_single(dev, adapter->filter_list_dma, 4096,
drivers/net/ethernet/ibm/ibmveth.c static int ibmveth_open(struct net_device *netdev) · line 757
Quoted 10 lines
drivers/net/ethernet/ibm/ibmveth.c +1
-3DMA_BIDIRECTIONAL); -out_free_tx_ltb: -while (--i >=0 ) { +for (i = netdev->real_num_tx_queues -1 ; i >=0 ; i--)ibmveth_free_tx_ltb (adapter, i); - } out_unmap_buffer_list: dma_unmap_single(dev, adapter->buffer_list_dma, 4096,
This series was applied to netdev/net.git (main) by Jakub Kicinski <[email protected]>:
Hello:
This series was applied to netdev/net.git (main)
by Jakub Kicinski <[email protected]>:
Quoted 6 lines · On Fri, 25 Sep 2026 11:05:28 -0700 you wrote:
Hi,
ibmveth_open()has two independent unwind holes after the logical LAN is set up. Neither needs the MQ RX series. This posting is against current net.git.
Quoted 7 lines
Patch 1 issues
h_free_logical_lan()on every post-register failure. A pool allocation failure jumped toout_free_buffer_poolswithout the hypercall, so PHYP still owned the buffer-list page when it was unmapped.request_irq()failure already issuedH_FREEbefore taking the same label.[...]
Here is the summary with links:
- [net,1/2] ibmveth: h_free logical LAN on open-fail after register
https://git.kernel.org/netdev/net/c/af0524bf4ce1 - [net,2/2] ibmveth: fix TX LTB and filter unwind on open-fail https://git.kernel.org/netdev/net/c/84bec0bf0352
You are awesome, thank you!
Signature · 2 lines
Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html