net/af_packet: fix socket close on device stop

Message ID 20250204164508.864080-1-tudor.cornea@gmail.com (mailing list archive)
State Accepted
Delegated to: Stephen Hemminger
Headers
Series net/af_packet: fix socket close on device stop |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/iol-marvell-Functional success Functional Testing PASS
ci/github-robot: build success github build: passed
ci/iol-unit-amd64-testing success Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/iol-abi-testing success Testing PASS
ci/intel-Functional success Functional PASS
ci/iol-compile-arm64-testing success Testing PASS
ci/iol-sample-apps-testing success Testing PASS
ci/iol-compile-amd64-testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-unit-arm64-testing success Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS

Commit Message

Tudor Cornea Feb. 4, 2025, 4:45 p.m. UTC
Currently, if we call rte_eth_dev_stop(), the sockets are closed.
If we attempt to start the port again, socket related operations
will not work correctly.

This can be alleviated by closing the socket at the same place in
which we currently free the memory, in eth_dev_close().

If an application calls rte_eth_dev_stop() on a port managed
by the af_packet PMD, the port becomes unusable. This is in contrast
with ports managed by other drivers (e.g virtio).

I also managed to reproduce the issue using testpmd.

sudo ip link add test-veth0 type veth peer name test-veth1

sudo ip link set test-veth0 up
sudo ip link set test-veth1 up

AF_PACKET_ARGS=\
"blocksz=4096,framesz=2048,framecnt=512,qpairs=1,qdisc_bypass=0"

sudo ./dpdk-testpmd \
        -l 0-3 \
        -m 1024 \
        --no-huge \
        --no-shconf \
        --no-pci \
        --vdev=net_af_packet0,iface=test-veth0,${AF_PACKET_ARGS} \
        --vdev=net_af_packet1,iface=test-veth1,${AF_PACKET_ARGS} \
        -- \
        -i

testpmd> start tx_first

Forwarding will start, and we will see traffic on the interfaces.

testpmd> stop
testpmd> port stop 0
Stopping ports...
Checking link statuses...
Done
testpmd> port stop 1
Stopping ports...
Checking link statuses...
Done

testpmd> port start 0
AFPACKET: eth_dev_macaddr_set(): receive socket not found
Port 0: CA:65:81:63:81:B2
Checking link statuses...
Done
testpmd> port start 1
AFPACKET: eth_dev_macaddr_set(): receive socket not found
Port 1: CA:12:D0:BE:93:3F
Checking link statuses...
Done

testpmd> start tx_first

When we start forwarding again, we can see that there is no traffic
on the interfaces. This does not happen when testing with other PMD
drivers (e.g virtio).

With the patch, the port should re-initialize correctly.

testpmd> port start 0
Port 0: CA:65:81:63:81:B2
Checking link statuses...
Done

Fixes: 364e08f2bbc0 ("af_packet: add PMD for AF_PACKET-based virtual devices")

Signed-off-by: Tudor Cornea <tudor.cornea@gmail.com>
---
 drivers/net/af_packet/rte_eth_af_packet.c | 30 +++++++++++------------
 1 file changed, 15 insertions(+), 15 deletions(-)
  

Comments

Stephen Hemminger Feb. 7, 2025, 1:26 a.m. UTC | #1
On Tue,  4 Feb 2025 18:45:08 +0200
Tudor Cornea <tudor.cornea@gmail.com> wrote:

> Currently, if we call rte_eth_dev_stop(), the sockets are closed.
> If we attempt to start the port again, socket related operations
> will not work correctly.
> 
> This can be alleviated by closing the socket at the same place in
> which we currently free the memory, in eth_dev_close().
> 
> If an application calls rte_eth_dev_stop() on a port managed
> by the af_packet PMD, the port becomes unusable. This is in contrast
> with ports managed by other drivers (e.g virtio).
> 
> I also managed to reproduce the issue using testpmd.
> 
> sudo ip link add test-veth0 type veth peer name test-veth1
> 
> sudo ip link set test-veth0 up
> sudo ip link set test-veth1 up
> 
> AF_PACKET_ARGS=\
> "blocksz=4096,framesz=2048,framecnt=512,qpairs=1,qdisc_bypass=0"
> 
> sudo ./dpdk-testpmd \
>         -l 0-3 \
>         -m 1024 \
>         --no-huge \
>         --no-shconf \
>         --no-pci \
>         --vdev=net_af_packet0,iface=test-veth0,${AF_PACKET_ARGS} \
>         --vdev=net_af_packet1,iface=test-veth1,${AF_PACKET_ARGS} \
>         -- \
>         -i
> 
> testpmd> start tx_first  
> 
> Forwarding will start, and we will see traffic on the interfaces.
> 
> testpmd> stop
> testpmd> port stop 0  
> Stopping ports...
> Checking link statuses...
> Done
> testpmd> port stop 1  
> Stopping ports...
> Checking link statuses...
> Done
> 
> testpmd> port start 0  
> AFPACKET: eth_dev_macaddr_set(): receive socket not found
> Port 0: CA:65:81:63:81:B2
> Checking link statuses...
> Done
> testpmd> port start 1  
> AFPACKET: eth_dev_macaddr_set(): receive socket not found
> Port 1: CA:12:D0:BE:93:3F
> Checking link statuses...
> Done
> 
> testpmd> start tx_first  
> 
> When we start forwarding again, we can see that there is no traffic
> on the interfaces. This does not happen when testing with other PMD
> drivers (e.g virtio).
> 
> With the patch, the port should re-initialize correctly.
> 
> testpmd> port start 0  
> Port 0: CA:65:81:63:81:B2
> Checking link statuses...
> Done
> 
> Fixes: 364e08f2bbc0 ("af_packet: add PMD for AF_PACKET-based virtual devices")
> 
> Signed-off-by: Tudor Cornea <tudor.cornea@gmail.com>

Makes sense, applied to next-net
  
Stephen Hemminger Feb. 7, 2025, 5:57 p.m. UTC | #2
On Tue,  4 Feb 2025 18:45:08 +0200
Tudor Cornea <tudor.cornea@gmail.com> wrote:

> Currently, if we call rte_eth_dev_stop(), the sockets are closed.
> If we attempt to start the port again, socket related operations
> will not work correctly.
> 
> This can be alleviated by closing the socket at the same place in
> which we currently free the memory, in eth_dev_close().
> 
> If an application calls rte_eth_dev_stop() on a port managed
> by the af_packet PMD, the port becomes unusable. This is in contrast
> with ports managed by other drivers (e.g virtio).
> 
> I also managed to reproduce the issue using testpmd.
> 
> sudo ip link add test-veth0 type veth peer name test-veth1
> 
> sudo ip link set test-veth0 up
> sudo ip link set test-veth1 up
> 
> AF_PACKET_ARGS=\
> "blocksz=4096,framesz=2048,framecnt=512,qpairs=1,qdisc_bypass=0"
> 
> sudo ./dpdk-testpmd \
>         -l 0-3 \
>         -m 1024 \
>         --no-huge \
>         --no-shconf \
>         --no-pci \
>         --vdev=net_af_packet0,iface=test-veth0,${AF_PACKET_ARGS} \
>         --vdev=net_af_packet1,iface=test-veth1,${AF_PACKET_ARGS} \
>         -- \
>         -i
> 
> testpmd> start tx_first  
> 
> Forwarding will start, and we will see traffic on the interfaces.
> 
> testpmd> stop
> testpmd> port stop 0  
> Stopping ports...
> Checking link statuses...
> Done
> testpmd> port stop 1  
> Stopping ports...
> Checking link statuses...
> Done
> 
> testpmd> port start 0  
> AFPACKET: eth_dev_macaddr_set(): receive socket not found
> Port 0: CA:65:81:63:81:B2
> Checking link statuses...
> Done
> testpmd> port start 1  
> AFPACKET: eth_dev_macaddr_set(): receive socket not found
> Port 1: CA:12:D0:BE:93:3F
> Checking link statuses...
> Done
> 
> testpmd> start tx_first  
> 
> When we start forwarding again, we can see that there is no traffic
> on the interfaces. This does not happen when testing with other PMD
> drivers (e.g virtio).
> 
> With the patch, the port should re-initialize correctly.
> 
> testpmd> port start 0  
> Port 0: CA:65:81:63:81:B2
> Checking link statuses...
> Done
> 
> Fixes: 364e08f2bbc0 ("af_packet: add PMD for AF_PACKET-based virtual devices")
> 
> Signed-off-by: Tudor Cornea <tudor.cornea@gmail.com>


Applied to next-net
Should this go to stable as well
  
Tudor Cornea Feb. 10, 2025, 8:20 a.m. UTC | #3
> Applied to next-net
> Should this go to stable as well

I think it would probably make sense to have it in stable.
  

Patch

diff --git a/drivers/net/af_packet/rte_eth_af_packet.c b/drivers/net/af_packet/rte_eth_af_packet.c
index ceb8d9356a..efae15a493 100644
--- a/drivers/net/af_packet/rte_eth_af_packet.c
+++ b/drivers/net/af_packet/rte_eth_af_packet.c
@@ -357,27 +357,12 @@  static int
 eth_dev_stop(struct rte_eth_dev *dev)
 {
 	unsigned i;
-	int sockfd;
 	struct pmd_internals *internals = dev->data->dev_private;
 
 	for (i = 0; i < internals->nb_queues; i++) {
-		sockfd = internals->rx_queue[i].sockfd;
-		if (sockfd != -1)
-			close(sockfd);
-
-		/* Prevent use after free in case tx fd == rx fd */
-		if (sockfd != internals->tx_queue[i].sockfd) {
-			sockfd = internals->tx_queue[i].sockfd;
-			if (sockfd != -1)
-				close(sockfd);
-		}
-
-		internals->rx_queue[i].sockfd = -1;
-		internals->tx_queue[i].sockfd = -1;
 		dev->data->rx_queue_state[i] = RTE_ETH_QUEUE_STATE_STOPPED;
 		dev->data->tx_queue_state[i] = RTE_ETH_QUEUE_STATE_STOPPED;
 	}
-
 	dev->data->dev_link.link_status = RTE_ETH_LINK_DOWN;
 	return 0;
 }
@@ -474,6 +459,7 @@  eth_dev_close(struct rte_eth_dev *dev)
 	struct pmd_internals *internals;
 	struct tpacket_req *req;
 	unsigned int q;
+	int sockfd;
 
 	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
 		return 0;
@@ -484,6 +470,20 @@  eth_dev_close(struct rte_eth_dev *dev)
 	internals = dev->data->dev_private;
 	req = &internals->req;
 	for (q = 0; q < internals->nb_queues; q++) {
+		sockfd = internals->rx_queue[q].sockfd;
+		if (sockfd != -1)
+			close(sockfd);
+
+		/* Prevent use after free in case tx fd == rx fd */
+		if (sockfd != internals->tx_queue[q].sockfd) {
+			sockfd = internals->tx_queue[q].sockfd;
+			if (sockfd != -1)
+				close(sockfd);
+		}
+
+		internals->rx_queue[q].sockfd = -1;
+		internals->tx_queue[q].sockfd = -1;
+
 		munmap(internals->rx_queue[q].map,
 			2 * req->tp_block_size * req->tp_block_nr);
 		rte_free(internals->rx_queue[q].rd);