[PATCH] tap: With pasta, don't reset on tap errors, handle write failures

[PATCH] tap: With pasta, don't reset on tap errors, handle write failures

[Stefano Brivio]

Since commit 0515adceaa8f ("passt, pasta: Namespace-based sandboxing,
defer seccomp policy application"), it makes no sense to close and
reopen the tap device on error: we don't have access to /dev/net/tun
after the initial setup phase.

If we hit ENOBUFS while writing (as reported: in one case because
the kernel actually ran out of memory, with another case under
investigation), or ENOSPC, we're supposed to drop whatever data we
were trying to send: there's no room for it.

Handle EINTR just like we handled EAGAIN/EWOULDBLOCK: there's no
particular reason why sending the same data should fail again.

Anything else I can think of would be an unrecoverable error: exit
with failure then.

While at it, drop a useless cast on the write() call: it takes a
const void * anyway.

Reported-by: Gianluca Stivan <me@yawnt.com>
Reported-by: Chris Kuhn <kuhnchris@kuhnchris.eu>
Fixes: 0515adceaa8f ("passt, pasta: Namespace-based sandboxing, defer seccomp policy application")
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 tap.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/tap.c b/tap.c
index c0b7f33..e323529 100644
--- a/tap.c
+++ b/tap.c
@@ -320,12 +320,23 @@ static size_t tap_send_frames_pasta(struct ctx *c,
 	size_t i;
 
 	for (i = 0; i < n; i++) {
-		if (write(c->fd_tap, (char *)iov[i].iov_base,
-			  iov[i].iov_len) < 0) {
+		if (write(c->fd_tap, iov[i].iov_base, iov[i].iov_len) < 0) {
 			debug("tap write: %s", strerror(errno));
-			if (errno != EAGAIN && errno != EWOULDBLOCK)
-				tap_handler(c, c->fd_tap, EPOLLERR, NULL);
-			i--;
+
+			switch (errno) {
+			case EAGAIN:
+#if EAGAIN != EWOULDBLOCK
+			case EWOULDBLOCK:
+#endif
+			case EINTR:
+				i--;
+				break;
+			case ENOBUFS:
+			case ENOSPC:
+				break;
+			default:
+				die("Write error on tap device, exiting");
+			}
 		}
 	}
 
@@ -1237,6 +1248,9 @@ void tap_handler(struct ctx *c, int fd, uint32_t events,
 			exit(EXIT_SUCCESS);
 		}
 
+		if (c->mode == MODE_PASTA)
+			die("Error on tap device, exiting");
+
 		tap_sock_init(c);
 	}
 }
-- 
@@ -320,12 +320,23 @@ static size_t tap_send_frames_pasta(struct ctx *c,
 	size_t i;
 
 	for (i = 0; i < n; i++) {
-		if (write(c->fd_tap, (char *)iov[i].iov_base,
-			  iov[i].iov_len) < 0) {
+		if (write(c->fd_tap, iov[i].iov_base, iov[i].iov_len) < 0) {
 			debug("tap write: %s", strerror(errno));
-			if (errno != EAGAIN && errno != EWOULDBLOCK)
-				tap_handler(c, c->fd_tap, EPOLLERR, NULL);
-			i--;
+
+			switch (errno) {
+			case EAGAIN:
+#if EAGAIN != EWOULDBLOCK
+			case EWOULDBLOCK:
+#endif
+			case EINTR:
+				i--;
+				break;
+			case ENOBUFS:
+			case ENOSPC:
+				break;
+			default:
+				die("Write error on tap device, exiting");
+			}
 		}
 	}
 
@@ -1237,6 +1248,9 @@ void tap_handler(struct ctx *c, int fd, uint32_t events,
 			exit(EXIT_SUCCESS);
 		}
 
+		if (c->mode == MODE_PASTA)
+			die("Error on tap device, exiting");
+
 		tap_sock_init(c);
 	}
 }
-- 
2.39.2

Re: [PATCH] tap: With pasta, don't reset on tap errors, handle write failures

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2610 bytes --]

On Sun, Jun 04, 2023 at 07:49:57AM +0200, Stefano Brivio wrote:
> Since commit 0515adceaa8f ("passt, pasta: Namespace-based sandboxing,
> defer seccomp policy application"), it makes no sense to close and
> reopen the tap device on error: we don't have access to /dev/net/tun
> after the initial setup phase.
> 
> If we hit ENOBUFS while writing (as reported: in one case because
> the kernel actually ran out of memory, with another case under
> investigation), or ENOSPC, we're supposed to drop whatever data we
> were trying to send: there's no room for it.
> 
> Handle EINTR just like we handled EAGAIN/EWOULDBLOCK: there's no
> particular reason why sending the same data should fail again.
> 
> Anything else I can think of would be an unrecoverable error: exit
> with failure then.
> 
> While at it, drop a useless cast on the write() call: it takes a
> const void * anyway.
> 
> Reported-by: Gianluca Stivan <me@yawnt.com>
> Reported-by: Chris Kuhn <kuhnchris@kuhnchris.eu>
> Fixes: 0515adceaa8f ("passt, pasta: Namespace-based sandboxing, defer seccomp policy application")
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  tap.c | 24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/tap.c b/tap.c
> index c0b7f33..e323529 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -320,12 +320,23 @@ static size_t tap_send_frames_pasta(struct ctx *c,
>  	size_t i;
>  
>  	for (i = 0; i < n; i++) {
> -		if (write(c->fd_tap, (char *)iov[i].iov_base,
> -			  iov[i].iov_len) < 0) {
> +		if (write(c->fd_tap, iov[i].iov_base, iov[i].iov_len) < 0) {
>  			debug("tap write: %s", strerror(errno));
> -			if (errno != EAGAIN && errno != EWOULDBLOCK)
> -				tap_handler(c, c->fd_tap, EPOLLERR, NULL);
> -			i--;
> +
> +			switch (errno) {
> +			case EAGAIN:
> +#if EAGAIN != EWOULDBLOCK
> +			case EWOULDBLOCK:
> +#endif
> +			case EINTR:
> +				i--;
> +				break;
> +			case ENOBUFS:
> +			case ENOSPC:
> +				break;
> +			default:
> +				die("Write error on tap device, exiting");
> +			}
>  		}
>  	}
>  
> @@ -1237,6 +1248,9 @@ void tap_handler(struct ctx *c, int fd, uint32_t events,
>  			exit(EXIT_SUCCESS);
>  		}
>  
> +		if (c->mode == MODE_PASTA)
> +			die("Error on tap device, exiting");
> +
>  		tap_sock_init(c);
>  	}
>  }

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]