Re: [dpdk-dev] [PATCH v2] driver/net/pcap fix: fd leak bug

[Ferruh Yigit]

On 3/1/2021 3:30 PM, ZhangTengfei wrote:
pcap fd was opend when vdev probed,
but not closed when vdev removed.
This bug appears in dpdk-pdump

The patch title should have "net/pcap: " as module name instead of 
"driver/net/pcap: ", suggested title:
net/pcap: fix fd leak on uninit


Also needs fixes line, I guess following can be used:
    Fixes: c956caa6eabf ("pcap: support port hotplug")
    Cc: sta...@dpdk.org



Signed-off-by: ZhangTengfei <zypsc...@outlook.com>

Would you mind using the sign-off as:
"Tengfei Zhang <zypsc...@outlook.com>

---
  drivers/net/pcap/rte_eth_pcap.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
index 90f5d75ea..396de29de 100644
--- a/drivers/net/pcap/rte_eth_pcap.c
+++ b/drivers/net/pcap/rte_eth_pcap.c
@@ -755,6 +755,8 @@ eth_dev_close(struct rte_eth_dev *dev)
        PMD_LOG(INFO, "Closing pcap ethdev on NUMA socket %d",
                        rte_socket_id());
  
+	eth_dev_stop(dev);
+
        rte_free(dev->process_private);
  
  	if (rte_eal_process_type() != RTE_PROC_PRIMARY)


Now since 'eth_dev_stop()' can be called multiple times with this change, 
'pcap_close()' is causing a crash when it gets NULL pointer, following check is 
required, can you please send a new version with it?


 --- a/drivers/net/pcap/rte_eth_pcap.c
 +++ b/drivers/net/pcap/rte_eth_pcap.c
 @@ -682,9 +682,11 @@ eth_dev_stop(struct rte_eth_dev *dev)
         /* Special iface case. Single pcap is open and shared between tx/rx. */
         if (internals->single_iface) {
                 queue_missed_stat_on_stop_update(dev, 0);
 -               pcap_close(pp->tx_pcap[0]);
 -               pp->tx_pcap[0] = NULL;
 -               pp->rx_pcap[0] = NULL;
 +               if (pp->tx_pcap[0] != NULL) {
 +                       pcap_close(pp->tx_pcap[0]);
 +                       pp->tx_pcap[0] = NULL;
 +                       pp->rx_pcap[0] = NULL;
 +               }
                 goto status_down;
         }