public inbox for passt-dev@passt.top
 help / color / mirror / code / Atom feed
* [PATCH] pasta: fix tcp port forwarding in auto mode
@ 2023-03-20 18:10 Paul Holzinger
  2023-03-21  8:18 ` Stefano Brivio
  0 siblings, 1 reply; 6+ messages in thread
From: Paul Holzinger @ 2023-03-20 18:10 UTC (permalink / raw)
  To: passt-dev; +Cc: Paul Holzinger

The logic in tcp_timer() was inverted. fwd_out should expose the host
ports in the ns. Therfore it must read the ports on the host and then
bind them in the netns. The same for fwd_in which checks ports in the
ns and then exposes them on the host.

Note that this only fixes tcp ports, udp does not seems to work at all
right now with the auto mode.

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
---
 tcp.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tcp.c b/tcp.c
index 0214087..0d0ad13 100644
--- a/tcp.c
+++ b/tcp.c
@@ -89,7 +89,7 @@
  * No port translation is needed for connections initiated remotely or by the
  * local host: source port from socket is reused while establishing connections
  * to the guest.
- * 
+ *
  * For connections initiated by the guest, it's not possible to force the same
  * source port as connections are established by the host kernel: that's the
  * only port translation needed.
@@ -173,7 +173,7 @@
  *   new socket is created and mapped in connection tracking table, setting
  *   MSS and window clamping from header and option of the observed SYN segment
  *
- * 
+ *
  * Aging and timeout
  * -----------------
  *
@@ -560,7 +560,7 @@ static struct tcp6_l2_flags_buf_t {
 #endif
 	struct tap_hdr taph;	/* 14					   2 */
 	struct ipv6hdr ip6h;	/* 32					  20 */
-	struct tcphdr th	/* 72 */ __attribute__ ((aligned(4))); /* 60 */ 
+	struct tcphdr th	/* 72 */ __attribute__ ((aligned(4))); /* 60 */
 	char opts[OPT_MSS_LEN + OPT_WS_LEN + 1];
 #ifdef __AVX2__
 } __attribute__ ((packed, aligned(32)))
@@ -3308,14 +3308,14 @@ void tcp_timer(struct ctx *c, const struct timespec *ts)
 		struct tcp_port_detect_arg detect_arg = { c, 0 };
 		struct tcp_port_rebind_arg rebind_arg = { c, 0 };
 
-		if (c->tcp.fwd_in.mode == FWD_AUTO) {
+		if (c->tcp.fwd_out.mode == FWD_AUTO) {
 			detect_arg.detect_in_ns = 0;
 			tcp_port_detect(&detect_arg);
 			rebind_arg.bind_in_ns = 1;
 			NS_CALL(tcp_port_rebind, &rebind_arg);
 		}
 
-		if (c->tcp.fwd_out.mode == FWD_AUTO) {
+		if (c->tcp.fwd_in.mode == FWD_AUTO) {
 			detect_arg.detect_in_ns = 1;
 			NS_CALL(tcp_port_detect, &detect_arg);
 			rebind_arg.bind_in_ns = 0;
-- 
@@ -89,7 +89,7 @@
  * No port translation is needed for connections initiated remotely or by the
  * local host: source port from socket is reused while establishing connections
  * to the guest.
- * 
+ *
  * For connections initiated by the guest, it's not possible to force the same
  * source port as connections are established by the host kernel: that's the
  * only port translation needed.
@@ -173,7 +173,7 @@
  *   new socket is created and mapped in connection tracking table, setting
  *   MSS and window clamping from header and option of the observed SYN segment
  *
- * 
+ *
  * Aging and timeout
  * -----------------
  *
@@ -560,7 +560,7 @@ static struct tcp6_l2_flags_buf_t {
 #endif
 	struct tap_hdr taph;	/* 14					   2 */
 	struct ipv6hdr ip6h;	/* 32					  20 */
-	struct tcphdr th	/* 72 */ __attribute__ ((aligned(4))); /* 60 */ 
+	struct tcphdr th	/* 72 */ __attribute__ ((aligned(4))); /* 60 */
 	char opts[OPT_MSS_LEN + OPT_WS_LEN + 1];
 #ifdef __AVX2__
 } __attribute__ ((packed, aligned(32)))
@@ -3308,14 +3308,14 @@ void tcp_timer(struct ctx *c, const struct timespec *ts)
 		struct tcp_port_detect_arg detect_arg = { c, 0 };
 		struct tcp_port_rebind_arg rebind_arg = { c, 0 };
 
-		if (c->tcp.fwd_in.mode == FWD_AUTO) {
+		if (c->tcp.fwd_out.mode == FWD_AUTO) {
 			detect_arg.detect_in_ns = 0;
 			tcp_port_detect(&detect_arg);
 			rebind_arg.bind_in_ns = 1;
 			NS_CALL(tcp_port_rebind, &rebind_arg);
 		}
 
-		if (c->tcp.fwd_out.mode == FWD_AUTO) {
+		if (c->tcp.fwd_in.mode == FWD_AUTO) {
 			detect_arg.detect_in_ns = 1;
 			NS_CALL(tcp_port_detect, &detect_arg);
 			rebind_arg.bind_in_ns = 0;
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] pasta: fix tcp port forwarding in auto mode
  2023-03-20 18:10 [PATCH] pasta: fix tcp port forwarding in auto mode Paul Holzinger
@ 2023-03-21  8:18 ` Stefano Brivio
  2023-03-21 13:55   ` Paul Holzinger
  2023-03-21 15:25   ` Stefano Brivio
  0 siblings, 2 replies; 6+ messages in thread
From: Stefano Brivio @ 2023-03-21  8:18 UTC (permalink / raw)
  To: Paul Holzinger; +Cc: passt-dev, David Gibson

Thanks for the patch!

On Mon, 20 Mar 2023 19:10:34 +0100
Paul Holzinger <pholzing@redhat.com> wrote:

> The logic in tcp_timer() was inverted. fwd_out should expose the host
> ports in the ns. Therfore it must read the ports on the host and then
> bind them in the netns. The same for fwd_in which checks ports in the
> ns and then exposes them on the host.
> 
> Note that this only fixes tcp ports, udp does not seems to work at all
> right now with the auto mode.

Note that for UDP there's no periodic scan, "auto" just checks bound
ports when pasta starts:

       -u, --udp-ports spec
              Configure UDP port forwarding to namespace. spec is as described
              for TCP above, and the list of ports is derived  from  listening
              sockets   reported  by  /proc/net/udp  and  /proc/net/udp6,  see
              proc(5), when pasta starts (not periodically).

> Signed-off-by: Paul Holzinger <pholzing@redhat.com>

Fixes: 1128fa03fe73 ("Improve types and names for port forwarding configuration")

(I'll add that tag).

The patch itself looks good to me. I'm now looking at other parts
(tcp_sock_init()) where we seem to have the same kind of swap.

Unfortunately this is only covered by the Podman demo as a test, which
has been disabled for a while now:

  https://bugs.passt.top/show_bug.cgi?id=29

and as David is meanwhile working to improve the test framework, we can
probably wait a bit to introduce a new test. Meanwhile I'll check this
part manually on related changes.

-- 
Stefano


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] pasta: fix tcp port forwarding in auto mode
  2023-03-21  8:18 ` Stefano Brivio
@ 2023-03-21 13:55   ` Paul Holzinger
  2023-03-21 14:02     ` Stefano Brivio
  2023-03-21 15:25   ` Stefano Brivio
  1 sibling, 1 reply; 6+ messages in thread
From: Paul Holzinger @ 2023-03-21 13:55 UTC (permalink / raw)
  To: Stefano Brivio; +Cc: passt-dev, David Gibson


On 21/03/2023 09:18, Stefano Brivio wrote:
> Thanks for the patch!
>
> On Mon, 20 Mar 2023 19:10:34 +0100
> Paul Holzinger <pholzing@redhat.com> wrote:
>
>> The logic in tcp_timer() was inverted. fwd_out should expose the host
>> ports in the ns. Therfore it must read the ports on the host and then
>> bind them in the netns. The same for fwd_in which checks ports in the
>> ns and then exposes them on the host.
>>
>> Note that this only fixes tcp ports, udp does not seems to work at all
>> right now with the auto mode.
> Note that for UDP there's no periodic scan, "auto" just checks bound
> ports when pasta starts:
>
>         -u, --udp-ports spec
>                Configure UDP port forwarding to namespace. spec is as described
>                for TCP above, and the list of ports is derived  from  listening
>                sockets   reported  by  /proc/net/udp  and  /proc/net/udp6,  see
>                proc(5), when pasta starts (not periodically).

Ok this makes sense then, is there a bug to track this? Because without 
it auto mode for UDP is useless for my Podman use case.

>> Signed-off-by: Paul Holzinger <pholzing@redhat.com>
> Fixes: 1128fa03fe73 ("Improve types and names for port forwarding configuration")
>
> (I'll add that tag).
>
> The patch itself looks good to me. I'm now looking at other parts
> (tcp_sock_init()) where we seem to have the same kind of swap.
>
> Unfortunately this is only covered by the Podman demo as a test, which
> has been disabled for a while now:
>
>    https://bugs.passt.top/show_bug.cgi?id=29
>
> and as David is meanwhile working to improve the test framework, we can
> probably wait a bit to introduce a new test. Meanwhile I'll check this
> part manually on related changes.
>


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] pasta: fix tcp port forwarding in auto mode
  2023-03-21 13:55   ` Paul Holzinger
@ 2023-03-21 14:02     ` Stefano Brivio
  2023-03-21 23:30       ` Stefano Brivio
  0 siblings, 1 reply; 6+ messages in thread
From: Stefano Brivio @ 2023-03-21 14:02 UTC (permalink / raw)
  To: Paul Holzinger; +Cc: passt-dev, David Gibson

On Tue, 21 Mar 2023 14:55:17 +0100
Paul Holzinger <pholzing@redhat.com> wrote:

> On 21/03/2023 09:18, Stefano Brivio wrote:
> > Thanks for the patch!
> >
> > On Mon, 20 Mar 2023 19:10:34 +0100
> > Paul Holzinger <pholzing@redhat.com> wrote:
> >  
> >> The logic in tcp_timer() was inverted. fwd_out should expose the host
> >> ports in the ns. Therfore it must read the ports on the host and then
> >> bind them in the netns. The same for fwd_in which checks ports in the
> >> ns and then exposes them on the host.
> >>
> >> Note that this only fixes tcp ports, udp does not seems to work at all
> >> right now with the auto mode.  
> > Note that for UDP there's no periodic scan, "auto" just checks bound
> > ports when pasta starts:
> >
> >         -u, --udp-ports spec
> >                Configure UDP port forwarding to namespace. spec is as described
> >                for TCP above, and the list of ports is derived  from  listening
> >                sockets   reported  by  /proc/net/udp  and  /proc/net/udp6,  see
> >                proc(5), when pasta starts (not periodically).  
> 
> Ok this makes sense then, is there a bug to track this? Because without 
> it auto mode for UDP is useless for my Podman use case.

No, sorry, not yet, feel free to file one, or I can do that later
today.

Right now not even what the man page says works, I'll post a patch in a
bit.

-- 
Stefano


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] pasta: fix tcp port forwarding in auto mode
  2023-03-21  8:18 ` Stefano Brivio
  2023-03-21 13:55   ` Paul Holzinger
@ 2023-03-21 15:25   ` Stefano Brivio
  1 sibling, 0 replies; 6+ messages in thread
From: Stefano Brivio @ 2023-03-21 15:25 UTC (permalink / raw)
  To: Paul Holzinger; +Cc: passt-dev, David Gibson

On Tue, 21 Mar 2023 09:18:48 +0100
Stefano Brivio <sbrivio@redhat.com> wrote:

> The patch itself looks good to me. I'm now looking at other parts
> (tcp_sock_init()) where we seem to have the same kind of swap.

Not really, the rest was fine, I'll apply this one in a bit.

-- 
Stefano


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] pasta: fix tcp port forwarding in auto mode
  2023-03-21 14:02     ` Stefano Brivio
@ 2023-03-21 23:30       ` Stefano Brivio
  0 siblings, 0 replies; 6+ messages in thread
From: Stefano Brivio @ 2023-03-21 23:30 UTC (permalink / raw)
  To: Paul Holzinger; +Cc: passt-dev, David Gibson

On Tue, 21 Mar 2023 15:02:30 +0100
Stefano Brivio <sbrivio@redhat.com> wrote:

> On Tue, 21 Mar 2023 14:55:17 +0100
> Paul Holzinger <pholzing@redhat.com> wrote:
> 
> > On 21/03/2023 09:18, Stefano Brivio wrote:  
> > > Thanks for the patch!
> > >
> > > On Mon, 20 Mar 2023 19:10:34 +0100
> > > Paul Holzinger <pholzing@redhat.com> wrote:
> > >    
> > >> The logic in tcp_timer() was inverted. fwd_out should expose the host
> > >> ports in the ns. Therfore it must read the ports on the host and then
> > >> bind them in the netns. The same for fwd_in which checks ports in the
> > >> ns and then exposes them on the host.
> > >>
> > >> Note that this only fixes tcp ports, udp does not seems to work at all
> > >> right now with the auto mode.    
> > > Note that for UDP there's no periodic scan, "auto" just checks bound
> > > ports when pasta starts:
> > >
> > >         -u, --udp-ports spec
> > >                Configure UDP port forwarding to namespace. spec is as described
> > >                for TCP above, and the list of ports is derived  from  listening
> > >                sockets   reported  by  /proc/net/udp  and  /proc/net/udp6,  see
> > >                proc(5), when pasta starts (not periodically).    
> > 
> > Ok this makes sense then, is there a bug to track this? Because without 
> > it auto mode for UDP is useless for my Podman use case.  
> 
> No, sorry, not yet, feel free to file one, or I can do that later
> today.

There you go, https://bugs.passt.top/show_bug.cgi?id=45.

-- 
Stefano


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2023-03-21 23:30 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-20 18:10 [PATCH] pasta: fix tcp port forwarding in auto mode Paul Holzinger
2023-03-21  8:18 ` Stefano Brivio
2023-03-21 13:55   ` Paul Holzinger
2023-03-21 14:02     ` Stefano Brivio
2023-03-21 23:30       ` Stefano Brivio
2023-03-21 15:25   ` Stefano Brivio

Code repositories for project(s) associated with this public inbox

	https://passt.top/passt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for IMAP folder(s).