[PATCH] tcp: Add error checking for flow_epoll_set() in tcp_flow_migrate_target()

[PATCH] tcp: Add error checking for flow_epoll_set() in tcp_flow_migrate_target()

[Stefano Brivio]

Commit 90287c2a774e ("tcp: Register fds with epoll at flow creation")
added a call to flow_epoll_set() in tcp_flow_migrate_target() without
checking for the error code.

As Laurent explains, that's harmless, as tcp_flow_migrate_target_ext()
will clean up the flow anyway, and we'll just uselessly call
flow_hash_insert() before removing it again.

But Coverity Scan complains about the unchecked return code, so check
it, skip flow_hash_insert() in that case, and explain in comments why
we don't have to do anything else.

Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 tcp.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/tcp.c b/tcp.c
index 6a26826..0a6b7ff 100644
--- a/tcp.c
+++ b/tcp.c
@@ -3684,17 +3684,19 @@ int tcp_flow_migrate_target(struct ctx *c, int fd)
 
 	if ((rc = tcp_flow_repair_socket(c, conn))) {
 		flow_err(flow, "Can't set up socket: %s, drop", strerror_(-rc));
-		/* Can't leave the flow in an incomplete state */
-		FLOW_ACTIVATE(conn);
-		return 0;
+		goto out;
 	}
 
 	flow_epollid_set(&conn->f, EPOLLFD_ID_DEFAULT);
-	flow_epoll_set(&conn->f, EPOLL_CTL_ADD, 0, conn->sock, !TAPSIDE(conn));
+	if (flow_epoll_set(&conn->f, EPOLL_CTL_ADD, 0, conn->sock,
+			   !TAPSIDE(conn)))
+		goto out; /* tcp_flow_migrate_target_ext() will clean this up */
 
 	flow_hash_insert(c, TAP_SIDX(conn));
-	FLOW_ACTIVATE(conn);
 
+out:
+	/* Never leave the flow in an incomplete state */
+	FLOW_ACTIVATE(conn);
 	return 0;
 }
 
-- 
2.43.0

Re: [PATCH] tcp: Add error checking for flow_epoll_set() in tcp_flow_migrate_target()

[Laurent Vivier]

On 1/22/26 15:13, Stefano Brivio wrote:
> Commit 90287c2a774e ("tcp: Register fds with epoll at flow creation")
> added a call to flow_epoll_set() in tcp_flow_migrate_target() without
> checking for the error code.
> 
> As Laurent explains, that's harmless, as tcp_flow_migrate_target_ext()
> will clean up the flow anyway, and we'll just uselessly call
> flow_hash_insert() before removing it again.
> 
> But Coverity Scan complains about the unchecked return code, so check
> it, skip flow_hash_insert() in that case, and explain in comments why
> we don't have to do anything else.
> 
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
>   tcp.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/tcp.c b/tcp.c
> index 6a26826..0a6b7ff 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -3684,17 +3684,19 @@ int tcp_flow_migrate_target(struct ctx *c, int fd)
>   
>   	if ((rc = tcp_flow_repair_socket(c, conn))) {
>   		flow_err(flow, "Can't set up socket: %s, drop", strerror_(-rc));
> -		/* Can't leave the flow in an incomplete state */
> -		FLOW_ACTIVATE(conn);
> -		return 0;
> +		goto out;
>   	}
>   
>   	flow_epollid_set(&conn->f, EPOLLFD_ID_DEFAULT);
> -	flow_epoll_set(&conn->f, EPOLL_CTL_ADD, 0, conn->sock, !TAPSIDE(conn));
> +	if (flow_epoll_set(&conn->f, EPOLL_CTL_ADD, 0, conn->sock,
> +			   !TAPSIDE(conn)))
> +		goto out; /* tcp_flow_migrate_target_ext() will clean this up */
>   
>   	flow_hash_insert(c, TAP_SIDX(conn));
> -	FLOW_ACTIVATE(conn);
>   
> +out:
> +	/* Never leave the flow in an incomplete state */
> +	FLOW_ACTIVATE(conn);
>   	return 0;
>   }
>   

Reviewed-by: Laurent Vivier <lvivier@redhat.com>