[PATCH 0/8] Clean up and fix bugs in port forwarding data structures

older
[PATCH 00/12] Assorted test fixes,...

David Gibson

23 Sep 2022 23 Sep '22

6:57 a.m.

The information we need to keep track of which ports are forwarded where is currently split across several different values and data structures in several different places. Worse, a number of those structures are incorrectly sized due to an off by one error, which could lead to buffer overruns. Fix the sizing, and re-organize the data structures in a way that should make it less likely to repeat that mistake. While we're there, correct a similar off-by-one mis-sizing of a number of other arrays. David Gibson (8): Improve types and names for port forwarding configuration Consolidate port forwarding configuration into a common structure udp: Delay initialization of UDP reversed port mapping table Don't use indirect remap functions for conf_ports() Pass entire port forwarding configuration substructure to conf_ports() Treat port numbers as unsigned Fix widespread off-by-one error dealing with port numbers icmp: Correct off by one errors dealing with number of echo request ids Makefile | 2 +- conf.c | 136 +++++++++++++++++++++++---------------------------- icmp.c | 5 +- passt.h | 1 + port_fwd.h | 34 +++++++++++++ tcp.c | 68 ++++++++------------------ tcp.h | 13 ++--- tcp_splice.c | 4 +- udp.c | 59 ++++++++++------------ udp.h | 27 +++++----- 10 files changed, 168 insertions(+), 181 deletions(-) create mode 100644 port_fwd.h -- 2.37.3

Show replies by date

David Gibson

23 Sep 23 Sep

6:57 a.m.

New subject: [PATCH 1/8] Improve types and names for port forwarding configuration

enum conf_port_type is local to conf.c and is used to track the port forwarding mode during configuration. We don't keep it around in the context structure, however the 'init_detect_ports' and 'ns_detect_ports' fields in the context are based solely on this. Rather than changing encoding, just include the forwarding mode into the context structure. Move the type definition to a new port_fwd.h, which is kind of trivial at the moment but will have more stuff later. While we're there, "conf_port_type" doesn't really convey that this enum is describing how port forwarding is configured. Rename it to port_fwd_mode. The variables (now fields) of this type also have mildly confusing names since it's not immediately obvious whether 'ns' and 'init' refer to the source or destination of the packets. Use "in" (host to guest / init to ns) and "out" (guest to host / ns to init) instead. This has the added bonus that we no longer have locals 'udp_init' and 'tcp_init' which shadow global functions. In addition, add a typedef 'port_fwd_map' for a bitmap of each port number, which is used in several places. Signed-off-by: David Gibson --- Makefile | 2 +- conf.c | 73 +++++++++++++++++++++++++++--------------------------- passt.h | 1 + port_fwd.h | 19 ++++++++++++++ tcp.c | 12 ++++----- tcp.h | 12 ++++----- udp.h | 12 ++++----- 7 files changed, 76 insertions(+), 55 deletions(-) create mode 100644 port_fwd.h diff --git a/Makefile b/Makefile index c47a5f6..432ee7a 100644 --- a/Makefile +++ b/Makefile @@ -41,7 +41,7 @@ MANPAGES = passt.1 pasta.1 qrap.1 PASST_HEADERS = arch.h arp.h checksum.h conf.h dhcp.h dhcpv6.h icmp.h \ isolation.h lineread.h ndp.h netlink.h packet.h passt.h pasta.h \ - pcap.h siphash.h tap.h tcp.h tcp_splice.h udp.h util.h + pcap.h port_fwd.h siphash.h tap.h tcp.h tcp_splice.h udp.h util.h HEADERS = $(PASST_HEADERS) seccomp.h # On gcc 11.2, with -O2 and -flto, tcp_hash() and siphash_20b(), if inlined, diff --git a/conf.c b/conf.c index 7ecfa1e..1ff43b7 100644 --- a/conf.c +++ b/conf.c @@ -64,14 +64,14 @@ void get_bound_ports(struct ctx *c, int ns, uint8_t proto) } if (proto == IPPROTO_UDP) { - memset(udp_map, 0, USHRT_MAX / 8); + memset(udp_map, 0, sizeof(port_fwd_map)); procfs_scan_listen(c, IPPROTO_UDP, V4, ns, udp_map, udp_excl); procfs_scan_listen(c, IPPROTO_UDP, V6, ns, udp_map, udp_excl); procfs_scan_listen(c, IPPROTO_TCP, V4, ns, udp_map, udp_excl); procfs_scan_listen(c, IPPROTO_TCP, V6, ns, udp_map, udp_excl); } else if (proto == IPPROTO_TCP) { - memset(tcp_map, 0, USHRT_MAX / 8); + memset(tcp_map, 0, sizeof(port_fwd_map)); procfs_scan_listen(c, IPPROTO_TCP, V4, ns, tcp_map, tcp_excl); procfs_scan_listen(c, IPPROTO_TCP, V6, ns, tcp_map, tcp_excl); } @@ -106,31 +106,25 @@ static int get_bound_ports_ns(void *arg) return 0; } -enum conf_port_type { - PORT_SPEC = 1, - PORT_NONE, - PORT_AUTO, - PORT_ALL, -}; - /** * conf_ports() - Parse port configuration options, initialise UDP/TCP sockets * @c: Execution context * @optname: Short option name, t, T, u, or U * @optarg: Option argument (port specification) - * @set: Pointer to @conf_port_type to be set (port binding type) + * @set: Pointer to @port_fwd_mode to be set (port forwarding mode) * * Return: -EINVAL on parsing error, 0 otherwise */ static int conf_ports(struct ctx *c, char optname, const char *optarg, - enum conf_port_type *set) + enum port_fwd_mode *set) { int start_src, end_src, start_dst, end_dst, exclude_only = 1, i, port; char addr_buf[sizeof(struct in6_addr)] = { 0 }, *addr = addr_buf; - uint8_t *map, exclude[DIV_ROUND_UP(USHRT_MAX, 8)] = { 0 }; void (*remap)(in_port_t port, in_port_t delta); char buf[BUFSIZ], *sep, *spec, *p; + port_fwd_map exclude = { 0 }; sa_family_t af = AF_UNSPEC; + uint8_t *map; if (optname == 't') { map = c->tcp.port_to_tap; @@ -151,14 +145,14 @@ static int conf_ports(struct ctx *c, char optname, const char *optarg, if (!strcmp(optarg, "none")) { if (*set) return -EINVAL; - *set = PORT_NONE; + *set = FWD_NONE; return 0; } if (!strcmp(optarg, "auto")) { if (*set || c->mode != MODE_PASTA) return -EINVAL; - *set = PORT_AUTO; + *set = FWD_AUTO; return 0; } @@ -167,7 +161,7 @@ static int conf_ports(struct ctx *c, char optname, const char *optarg, if (*set || c->mode != MODE_PASST) return -EINVAL; - *set = PORT_ALL; + *set = FWD_ALL; memset(map, 0xff, PORT_EPHEMERAL_MIN / 8); for (i = 0; i < PORT_EPHEMERAL_MIN; i++) { @@ -180,10 +174,10 @@ static int conf_ports(struct ctx *c, char optname, const char *optarg, return 0; } - if (*set > PORT_SPEC) + if (*set > FWD_SPEC) return -EINVAL; - *set = PORT_SPEC; + *set = FWD_SPEC; strncpy(buf, optarg, sizeof(buf) - 1); @@ -1088,8 +1082,6 @@ void conf(struct ctx *c, int argc, char **argv) }; struct get_bound_ports_ns_arg ns_ports_arg = { .c = c }; char userns[PATH_MAX] = { 0 }, netns[PATH_MAX] = { 0 }; - enum conf_port_type tcp_tap = 0, tcp_init = 0; - enum conf_port_type udp_tap = 0, udp_init = 0; bool v4_only = false, v6_only = false; struct in6_addr *dns6 = c->ip6.dns; struct fqdn *dnss = c->dns_search; @@ -1103,6 +1095,9 @@ void conf(struct ctx *c, int argc, char **argv) if (c->mode == MODE_PASTA) c->no_dhcp_dns = c->no_dhcp_dns_search = 1; + c->tcp.fwd_mode_in = c->tcp.fwd_mode_out = 0; + c->udp.fwd_mode_in = c->udp.fwd_mode_out = 0; + do { const char *optstring; @@ -1553,7 +1548,7 @@ void conf(struct ctx *c, int argc, char **argv) /* Now we can process port configuration options */ optind = 1; do { - enum conf_port_type *set = NULL; + enum port_fwd_mode *fwd = NULL; const char *optstring; if (c->mode == MODE_PASST) @@ -1568,15 +1563,15 @@ void conf(struct ctx *c, int argc, char **argv) case 'T': case 'U': if (name == 't') - set = &tcp_tap; + fwd = &c->tcp.fwd_mode_in; else if (name == 'T') - set = &tcp_init; + fwd = &c->tcp.fwd_mode_out; else if (name == 'u') - set = &udp_tap; + fwd = &c->udp.fwd_mode_in; else if (name == 'U') - set = &udp_init; + fwd = &c->udp.fwd_mode_out; - if (!optarg || conf_ports(c, name, optarg, set)) + if (!optarg || conf_ports(c, name, optarg, fwd)) usage(argv[0]); break; @@ -1605,33 +1600,39 @@ void conf(struct ctx *c, int argc, char **argv) if_indextoname(c->ifi6, c->pasta_ifn); } - c->tcp.ns_detect_ports = c->udp.ns_detect_ports = 0; - c->tcp.init_detect_ports = c->udp.init_detect_ports = 0; - if (c->mode == MODE_PASTA) { c->proc_net_tcp[V4][0] = c->proc_net_tcp[V4][1] = -1; c->proc_net_tcp[V6][0] = c->proc_net_tcp[V6][1] = -1; c->proc_net_udp[V4][0] = c->proc_net_udp[V4][1] = -1; c->proc_net_udp[V6][0] = c->proc_net_udp[V6][1] = -1; - if (!tcp_tap || tcp_tap == PORT_AUTO) { - c->tcp.ns_detect_ports = 1; + if (!c->tcp.fwd_mode_in || c->tcp.fwd_mode_in == FWD_AUTO) { + c->tcp.fwd_mode_in = FWD_AUTO; ns_ports_arg.proto = IPPROTO_TCP; NS_CALL(get_bound_ports_ns, &ns_ports_arg); } - if (!udp_tap || udp_tap == PORT_AUTO) { - c->udp.ns_detect_ports = 1; + if (!c->udp.fwd_mode_in || c->udp.fwd_mode_in == FWD_AUTO) { + c->udp.fwd_mode_in = FWD_AUTO; ns_ports_arg.proto = IPPROTO_UDP; NS_CALL(get_bound_ports_ns, &ns_ports_arg); } - if (!tcp_init || tcp_init == PORT_AUTO) { - c->tcp.init_detect_ports = 1; + if (!c->tcp.fwd_mode_out || c->tcp.fwd_mode_out == FWD_AUTO) { + c->tcp.fwd_mode_out = FWD_AUTO; get_bound_ports(c, 0, IPPROTO_TCP); } - if (!udp_init || udp_init == PORT_AUTO) { - c->udp.init_detect_ports = 1; + if (!c->udp.fwd_mode_out || c->udp.fwd_mode_out == FWD_AUTO) { + c->udp.fwd_mode_out = FWD_AUTO; get_bound_ports(c, 0, IPPROTO_UDP); } + } else { + if (!c->tcp.fwd_mode_in) + c->tcp.fwd_mode_in = FWD_NONE; + if (!c->tcp.fwd_mode_out) + c->tcp.fwd_mode_out= FWD_NONE; + if (!c->udp.fwd_mode_in) + c->udp.fwd_mode_in = FWD_NONE; + if (!c->udp.fwd_mode_out) + c->udp.fwd_mode_out = FWD_NONE; } if (!c->quiet) diff --git a/passt.h b/passt.h index 8c8d710..48e1096 100644 --- a/passt.h +++ b/passt.h @@ -33,6 +33,7 @@ union epoll_ref; #include "packet.h" #include "icmp.h" +#include "port_fwd.h" #include "tcp.h" #include "udp.h" diff --git a/port_fwd.h b/port_fwd.h new file mode 100644 index 0000000..5f6ad7e --- /dev/null +++ b/port_fwd.h @@ -0,0 +1,19 @@ +/* SPDX-License-Identifier: AGPL-3.0-or-later + * Copyright Red Hat + * Author: Stefano Brivio + * Author: David Gibson + */ + +#ifndef PORT_FWD_H +#define PORT_FWD_H + +enum port_fwd_mode { + FWD_SPEC = 1, + FWD_NONE, + FWD_AUTO, + FWD_ALL, +}; + +typedef uint8_t port_fwd_map[DIV_ROUND_UP(USHRT_MAX, 8)]; + +#endif /* PORT_FWD_H */ diff --git a/tcp.c b/tcp.c index ec8c32e..0bdef7f 100644 --- a/tcp.c +++ b/tcp.c @@ -3133,7 +3133,7 @@ void tcp_sock_init(const struct ctx *c, int ns, sa_family_t af, else s = -1; - if (c->tcp.init_detect_ports) + if (c->tcp.fwd_mode_in == FWD_AUTO) tcp_sock_init_ext[port][V4] = s; } @@ -3148,7 +3148,7 @@ void tcp_sock_init(const struct ctx *c, int ns, sa_family_t af, else s = -1; - if (c->tcp.ns_detect_ports) { + if (c->tcp.fwd_mode_out == FWD_AUTO) { if (ns) tcp_sock_ns[port][V4] = s; else @@ -3174,7 +3174,7 @@ void tcp_sock_init(const struct ctx *c, int ns, sa_family_t af, else s = -1; - if (c->tcp.init_detect_ports) + if (c->tcp.fwd_mode_in == FWD_AUTO) tcp_sock_init_ext[port][V6] = s; } @@ -3189,7 +3189,7 @@ void tcp_sock_init(const struct ctx *c, int ns, sa_family_t af, else s = -1; - if (c->tcp.ns_detect_ports) { + if (c->tcp.fwd_mode_out == FWD_AUTO) { if (ns) tcp_sock_ns[port][V6] = s; else @@ -3489,14 +3489,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.init_detect_ports) { + if (c->tcp.fwd_mode_in == 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.ns_detect_ports) { + if (c->tcp.fwd_mode_out == FWD_AUTO) { detect_arg.detect_in_ns = 1; NS_CALL(tcp_port_detect, &detect_arg); rebind_arg.bind_in_ns = 0; diff --git a/tcp.h b/tcp.h index 6431b75..58989db 100644 --- a/tcp.h +++ b/tcp.h @@ -58,9 +58,9 @@ union tcp_epoll_ref { * @conn_count: Count of connections (not spliced) in connection table * @splice_conn_count: Count of spliced connections in connection table * @port_to_tap: Ports bound host-side, packets to tap or spliced - * @init_detect_ports: If set, periodically detect ports bound in init + * @fwd_mode_in: Port forwarding mode for inbound packets * @port_to_init: Ports bound namespace-side, spliced to init - * @ns_detect_ports: If set, periodically detect ports bound in namespace + * @fwd_mode_out: Port forwarding mode for outbound packets * @timer_run: Timestamp of most recent timer run * @kernel_snd_wnd: Kernel reports sending window (with commit 8f7baad7f035) * @pipe_size: Size of pipes for spliced connections @@ -69,10 +69,10 @@ struct tcp_ctx { uint64_t hash_secret[2]; int conn_count; int splice_conn_count; - uint8_t port_to_tap [DIV_ROUND_UP(USHRT_MAX, 8)]; - int init_detect_ports; - uint8_t port_to_init [DIV_ROUND_UP(USHRT_MAX, 8)]; - int ns_detect_ports; + port_fwd_map port_to_tap; + enum port_fwd_mode fwd_mode_in; + port_fwd_map port_to_init; + enum port_fwd_mode fwd_mode_out; struct timespec timer_run; #ifdef HAS_SND_WND int kernel_snd_wnd; diff --git a/udp.h b/udp.h index 8f82842..0283ce6 100644 --- a/udp.h +++ b/udp.h @@ -47,16 +47,16 @@ union udp_epoll_ref { /** * struct udp_ctx - Execution context for UDP * @port_to_tap: Ports bound host-side, data to tap or ns L4 socket - * @init_detect_ports: If set, periodically detect ports bound in init (TODO) + * @fwd_mode_in: Port forwarding mode for inbound packets * @port_to_init: Ports bound namespace-side, data to init L4 socket - * @ns_detect_ports: If set, periodically detect ports bound in namespace + * @fwd_mode_out: Port forwarding mode for outbound packets * @timer_run: Timestamp of most recent timer run */ struct udp_ctx { - uint8_t port_to_tap [DIV_ROUND_UP(USHRT_MAX, 8)]; - int init_detect_ports; - uint8_t port_to_init [DIV_ROUND_UP(USHRT_MAX, 8)]; - int ns_detect_ports; + port_fwd_map port_to_tap; + enum port_fwd_mode fwd_mode_in; + port_fwd_map port_to_init; + enum port_fwd_mode fwd_mode_out; struct timespec timer_run; }; -- 2.37.3

Stefano Brivio

24 Sep 24 Sep

12:52 a.m.

New subject: [PATCH 1/8] Improve types and names for port forwarding configuration

On Fri, 23 Sep 2022 14:57:59 +1000 David Gibson wrote:

...

[...]

--- /dev/null +++ b/port_fwd.h @@ -0,0 +1,19 @@ +/* SPDX-License-Identifier: AGPL-3.0-or-later + * Copyright Red Hat + * Author: Stefano Brivio + * Author: David Gibson + */ + +#ifndef PORT_FWD_H +#define PORT_FWD_H + +enum port_fwd_mode { + FWD_SPEC = 1, + FWD_NONE, + FWD_AUTO, + FWD_ALL, +}; + +typedef uint8_t port_fwd_map[DIV_ROUND_UP(USHRT_MAX, 8)];

Given that this gets conveniently embedded in a struct in 2/8, could we avoid the typedef (or perhaps drop it after 2/8)? It makes the actual type less obvious to figure out, and in general I agree with most points from this slide deck: http://www.kroah.com/linux/talks/ols_2002_kernel_codingstyle_talk/html/mgp00... :) ...well, unless there's some resulting complexity I'm missing. I reviewed the rest of the series, it all makes sense to me, thanks. -- Stefano

David Gibson

4:57 a.m.

New subject: [PATCH 1/8] Improve types and names for port forwarding configuration

On Sat, Sep 24, 2022 at 12:52:57AM +0200, Stefano Brivio wrote:

...

On Fri, 23 Sep 2022 14:57:59 +1000 David Gibson wrote:

...
[...]

--- /dev/null +++ b/port_fwd.h @@ -0,0 +1,19 @@ +/* SPDX-License-Identifier: AGPL-3.0-or-later + * Copyright Red Hat + * Author: Stefano Brivio + * Author: David Gibson + */ + +#ifndef PORT_FWD_H +#define PORT_FWD_H + +enum port_fwd_mode { + FWD_SPEC = 1, + FWD_NONE, + FWD_AUTO, + FWD_ALL, +}; + +typedef uint8_t port_fwd_map[DIV_ROUND_UP(USHRT_MAX, 8)];

Given that this gets conveniently embedded in a struct in 2/8, could we avoid the typedef (or perhaps drop it after 2/8)? It makes the actual type less obvious to figure out, and in general I agree with most points from this slide deck:

http://www.kroah.com/linux/talks/ols_2002_kernel_codingstyle_talk/html/mgp00...

As do I, in fact especially for array types, due to their idiosyncratic handling in C.

...

:) ...well, unless there's some resulting complexity I'm missing.

Well, maybe. I added the typedef because of two things: 1) the fact that in one place we use a bitmap of this format separately from the rest for the 'exclude' map in conf_ports(). 2) the fact that we need the size of this map in get_bound_ports(). Having the typedef lets us handle both without duplicating the calculation of the size, which would mean more opportunities to get it wrong. I feel the advantages outweigh the disadvantages of a typedef in this case, but I won't be offended if you disagree. We could use a #define or const of the bitmap size instead.

...

I reviewed the rest of the series, it all makes sense to me, thanks.

-- 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

Stefano Brivio

8:28 a.m.

New subject: [PATCH 1/8] Improve types and names for port forwarding configuration

On Sat, 24 Sep 2022 12:57:25 +1000 David Gibson wrote:

...

On Sat, Sep 24, 2022 at 12:52:57AM +0200, Stefano Brivio wrote:

...
On Fri, 23 Sep 2022 14:57:59 +1000 David Gibson wrote:

...
[...]

--- /dev/null +++ b/port_fwd.h @@ -0,0 +1,19 @@ +/* SPDX-License-Identifier: AGPL-3.0-or-later + * Copyright Red Hat + * Author: Stefano Brivio + * Author: David Gibson + */ + +#ifndef PORT_FWD_H +#define PORT_FWD_H + +enum port_fwd_mode { + FWD_SPEC = 1, + FWD_NONE, + FWD_AUTO, + FWD_ALL, +}; + +typedef uint8_t port_fwd_map[DIV_ROUND_UP(USHRT_MAX, 8)];

Given that this gets conveniently embedded in a struct in 2/8, could we avoid the typedef (or perhaps drop it after 2/8)? It makes the actual type less obvious to figure out, and in general I agree with most points from this slide deck:

http://www.kroah.com/linux/talks/ols_2002_kernel_codingstyle_talk/html/mgp00...

As do I, in fact especially for array types, due to their idiosyncratic handling in C.

...
:) ...well, unless there's some resulting complexity I'm missing.

Well, maybe. I added the typedef because of two things: 1) the fact that in one place we use a bitmap of this format separately from the rest for the 'exclude' map in conf_ports(). 2) the fact that we need the size of this map in get_bound_ports(). Having the typedef lets us handle both without duplicating the calculation of the size, which would mean more opportunities to get it wrong.

I see, but at the same time: - forgetting to use the new type should be as easy as forgetting to use a define representing the size - the risk of doing stupid things (such as trying to pass this by value), despite the clear name of the typedef, looks to me slightly bigger than with "uint8_t ports[SIZE_PORTS]"

...

I feel the advantages outweigh the disadvantages of a typedef in this case, but I won't be offended if you disagree. We could use a #define or const of the bitmap size instead.

Well, I see now, and I don't have such a strong preference anymore... even though I would still prefer the define (perhaps SIZE_PORTS, based on NUM_PORTS from 7/8?). Also introducing the first typedef in the whole project for something we can implement almost equivalently, hmm. I don't know, if you still think it's clearly better than the alternative, let's go with it, I just have a slight preference against it at this point. -- Stefano

David Gibson

11:06 a.m.

New subject: [PATCH 1/8] Improve types and names for port forwarding configuration

On Sat, Sep 24, 2022 at 08:28:32AM +0200, Stefano Brivio wrote:

...

On Sat, 24 Sep 2022 12:57:25 +1000 David Gibson wrote:

...
On Sat, Sep 24, 2022 at 12:52:57AM +0200, Stefano Brivio wrote:

...
On Fri, 23 Sep 2022 14:57:59 +1000 David Gibson wrote:

...
[...]

--- /dev/null +++ b/port_fwd.h @@ -0,0 +1,19 @@ +/* SPDX-License-Identifier: AGPL-3.0-or-later + * Copyright Red Hat + * Author: Stefano Brivio + * Author: David Gibson + */ + +#ifndef PORT_FWD_H +#define PORT_FWD_H + +enum port_fwd_mode { + FWD_SPEC = 1, + FWD_NONE, + FWD_AUTO, + FWD_ALL, +}; + +typedef uint8_t port_fwd_map[DIV_ROUND_UP(USHRT_MAX, 8)];

Given that this gets conveniently embedded in a struct in 2/8, could we avoid the typedef (or perhaps drop it after 2/8)? It makes the actual type less obvious to figure out, and in general I agree with most points from this slide deck:

http://www.kroah.com/linux/talks/ols_2002_kernel_codingstyle_talk/html/mgp00...

As do I, in fact especially for array types, due to their idiosyncratic handling in C.

...
:) ...well, unless there's some resulting complexity I'm missing.

Well, maybe. I added the typedef because of two things: 1) the fact that in one place we use a bitmap of this format separately from the rest for the 'exclude' map in conf_ports(). 2) the fact that we need the size of this map in get_bound_ports(). Having the typedef lets us handle both without duplicating the calculation of the size, which would mean more opportunities to get it wrong.

I see, but at the same time:

- forgetting to use the new type should be as easy as forgetting to use a define representing the size

- the risk of doing stupid things (such as trying to pass this by value), despite the clear name of the typedef, looks to me slightly bigger than with "uint8_t ports[SIZE_PORTS]"

...
I feel the advantages outweigh the disadvantages of a typedef in this case, but I won't be offended if you disagree. We could use a #define or const of the bitmap size instead.

Well, I see now, and I don't have such a strong preference anymore... even though I would still prefer the define (perhaps SIZE_PORTS, based on NUM_PORTS from 7/8?).

Also introducing the first typedef in the whole project for something we can implement almost equivalently, hmm.

I don't know, if you still think it's clearly better than the alternative, let's go with it, I just have a slight preference against it at this point.

I think you convinced me. TBH, I only thought of the size define when I replied here, not when I originally wrote it. I'll respin with that instead of the typedef. -- 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

David Gibson

23 Sep 23 Sep

6:58 a.m.

New subject: [PATCH 2/8] Consolidate port forwarding configuration into a common structure

The configuration for how to forward ports in and out of the guest/ns is divided between several different variables. For each connect direction and protocol we have a mode in the udp/tcp context structure, a bitmap of which ports to forward also in the context structure and an array of deltas to apply if the outward facing and inward facing port numbers are different. This last is a separate global variable, rather than being in the context structure, for no particular reason. UDP also requires an additional array which has the reverse mapping used for return packets. Consolidate these into a re-used substructure in the context structure. Signed-off-by: David Gibson --- conf.c | 74 ++++++++++++++++++++++++++-------------------------- port_fwd.h | 12 +++++++++ tcp.c | 42 ++++++++++++++--------------- tcp.h | 15 +++++------ tcp_splice.c | 4 +-- udp.c | 30 +++++++++------------ udp.h | 27 ++++++++++++------- 7 files changed, 106 insertions(+), 98 deletions(-) diff --git a/conf.c b/conf.c index 1ff43b7..f74a32f 100644 --- a/conf.c +++ b/conf.c @@ -52,15 +52,15 @@ void get_bound_ports(struct ctx *c, int ns, uint8_t proto) uint8_t *udp_map, *udp_excl, *tcp_map, *tcp_excl; if (ns) { - udp_map = c->udp.port_to_tap; - udp_excl = c->udp.port_to_init; - tcp_map = c->tcp.port_to_tap; - tcp_excl = c->tcp.port_to_init; + udp_map = c->udp.fwd_in.f.map; + udp_excl = c->udp.fwd_out.f.map; + tcp_map = c->tcp.fwd_in.map; + tcp_excl = c->tcp.fwd_out.map; } else { - udp_map = c->udp.port_to_init; - udp_excl = c->udp.port_to_tap; - tcp_map = c->tcp.port_to_init; - tcp_excl = c->tcp.port_to_tap; + udp_map = c->udp.fwd_out.f.map; + udp_excl = c->udp.fwd_in.f.map; + tcp_map = c->tcp.fwd_out.map; + tcp_excl = c->tcp.fwd_in.map; } if (proto == IPPROTO_UDP) { @@ -120,23 +120,23 @@ static int conf_ports(struct ctx *c, char optname, const char *optarg, { int start_src, end_src, start_dst, end_dst, exclude_only = 1, i, port; char addr_buf[sizeof(struct in6_addr)] = { 0 }, *addr = addr_buf; - void (*remap)(in_port_t port, in_port_t delta); + void (*remap)(struct ctx *c, in_port_t port, in_port_t delta); char buf[BUFSIZ], *sep, *spec, *p; port_fwd_map exclude = { 0 }; sa_family_t af = AF_UNSPEC; uint8_t *map; if (optname == 't') { - map = c->tcp.port_to_tap; + map = c->tcp.fwd_in.map; remap = tcp_remap_to_tap; } else if (optname == 'T') { - map = c->tcp.port_to_init; + map = c->tcp.fwd_out.map; remap = tcp_remap_to_init; } else if (optname == 'u') { - map = c->udp.port_to_tap; + map = c->udp.fwd_in.f.map; remap = udp_remap_to_tap; } else if (optname == 'U') { - map = c->udp.port_to_init; + map = c->udp.fwd_out.f.map; remap = udp_remap_to_init; } else { /* For gcc -O3 */ return 0; @@ -365,8 +365,8 @@ static int conf_ports(struct ctx *c, char optname, const char *optarg, if (start_dst != -1) { /* 80:8080 or 22-80:8080:8080 */ - remap(i, (in_port_t)(start_dst - - start_src)); + remap(c, i, (in_port_t)(start_dst - + start_src)); } if (optname == 't') @@ -1095,8 +1095,8 @@ void conf(struct ctx *c, int argc, char **argv) if (c->mode == MODE_PASTA) c->no_dhcp_dns = c->no_dhcp_dns_search = 1; - c->tcp.fwd_mode_in = c->tcp.fwd_mode_out = 0; - c->udp.fwd_mode_in = c->udp.fwd_mode_out = 0; + c->tcp.fwd_in.mode = c->tcp.fwd_out.mode = 0; + c->udp.fwd_in.f.mode = c->udp.fwd_out.f.mode = 0; do { const char *optstring; @@ -1563,13 +1563,13 @@ void conf(struct ctx *c, int argc, char **argv) case 'T': case 'U': if (name == 't') - fwd = &c->tcp.fwd_mode_in; + fwd = &c->tcp.fwd_in.mode; else if (name == 'T') - fwd = &c->tcp.fwd_mode_out; + fwd = &c->tcp.fwd_out.mode; else if (name == 'u') - fwd = &c->udp.fwd_mode_in; + fwd = &c->udp.fwd_in.f.mode; else if (name == 'U') - fwd = &c->udp.fwd_mode_out; + fwd = &c->udp.fwd_out.f.mode; if (!optarg || conf_ports(c, name, optarg, fwd)) usage(argv[0]); @@ -1606,33 +1606,33 @@ void conf(struct ctx *c, int argc, char **argv) c->proc_net_udp[V4][0] = c->proc_net_udp[V4][1] = -1; c->proc_net_udp[V6][0] = c->proc_net_udp[V6][1] = -1; - if (!c->tcp.fwd_mode_in || c->tcp.fwd_mode_in == FWD_AUTO) { - c->tcp.fwd_mode_in = FWD_AUTO; + if (!c->tcp.fwd_in.mode || c->tcp.fwd_in.mode == FWD_AUTO) { + c->tcp.fwd_in.mode = FWD_AUTO; ns_ports_arg.proto = IPPROTO_TCP; NS_CALL(get_bound_ports_ns, &ns_ports_arg); } - if (!c->udp.fwd_mode_in || c->udp.fwd_mode_in == FWD_AUTO) { - c->udp.fwd_mode_in = FWD_AUTO; + if (!c->udp.fwd_in.f.mode || c->udp.fwd_in.f.mode == FWD_AUTO) { + c->udp.fwd_in.f.mode = FWD_AUTO; ns_ports_arg.proto = IPPROTO_UDP; NS_CALL(get_bound_ports_ns, &ns_ports_arg); } - if (!c->tcp.fwd_mode_out || c->tcp.fwd_mode_out == FWD_AUTO) { - c->tcp.fwd_mode_out = FWD_AUTO; + if (!c->tcp.fwd_out.mode || c->tcp.fwd_out.mode == FWD_AUTO) { + c->tcp.fwd_out.mode = FWD_AUTO; get_bound_ports(c, 0, IPPROTO_TCP); } - if (!c->udp.fwd_mode_out || c->udp.fwd_mode_out == FWD_AUTO) { - c->udp.fwd_mode_out = FWD_AUTO; + if (!c->udp.fwd_out.f.mode || c->udp.fwd_out.f.mode == FWD_AUTO) { + c->udp.fwd_out.f.mode = FWD_AUTO; get_bound_ports(c, 0, IPPROTO_UDP); } } else { - if (!c->tcp.fwd_mode_in) - c->tcp.fwd_mode_in = FWD_NONE; - if (!c->tcp.fwd_mode_out) - c->tcp.fwd_mode_out= FWD_NONE; - if (!c->udp.fwd_mode_in) - c->udp.fwd_mode_in = FWD_NONE; - if (!c->udp.fwd_mode_out) - c->udp.fwd_mode_out = FWD_NONE; + if (!c->tcp.fwd_in.mode) + c->tcp.fwd_in.mode = FWD_NONE; + if (!c->tcp.fwd_out.mode) + c->tcp.fwd_out.mode = FWD_NONE; + if (!c->udp.fwd_in.f.mode) + c->udp.fwd_in.f.mode = FWD_NONE; + if (!c->udp.fwd_out.f.mode) + c->udp.fwd_out.f.mode = FWD_NONE; } if (!c->quiet) diff --git a/port_fwd.h b/port_fwd.h index 5f6ad7e..5507bf2 100644 --- a/port_fwd.h +++ b/port_fwd.h @@ -16,4 +16,16 @@ enum port_fwd_mode { typedef uint8_t port_fwd_map[DIV_ROUND_UP(USHRT_MAX, 8)]; +/** + * port_fwd - Describes port forwarding for one protocol and direction + * @mode: Overall forwarding mode (all, none, auto, specific ports) + * @map: Bitmap describing which ports are forwarded + * @delta: Offset between the original destination and mapped port number + */ +struct port_fwd { + enum port_fwd_mode mode; + port_fwd_map map; + in_port_t delta[USHRT_MAX]; +}; + #endif /* PORT_FWD_H */ diff --git a/tcp.c b/tcp.c index 0bdef7f..e44177f 100644 --- a/tcp.c +++ b/tcp.c @@ -546,10 +546,6 @@ static const char *tcp_flag_str[] __attribute((__unused__)) = { "ACK_TO_TAP_DUE", "ACK_FROM_TAP_DUE", }; -/* Port re-mappings as delta, indexed by original destination port */ -static in_port_t tcp_port_delta_to_tap [USHRT_MAX]; -static in_port_t tcp_port_delta_to_init [USHRT_MAX]; - /* Listening sockets, used for automatic port forwarding in pasta mode only */ static int tcp_sock_init_lo [USHRT_MAX][IP_VERSIONS]; static int tcp_sock_init_ext [USHRT_MAX][IP_VERSIONS]; @@ -954,22 +950,24 @@ static void conn_event_do(const struct ctx *c, struct tcp_conn *conn, /** * tcp_remap_to_tap() - Set delta for port translation toward guest/tap + * @c: Execution context * @port: Original destination port, host order * @delta: Delta to be added to original destination port */ -void tcp_remap_to_tap(in_port_t port, in_port_t delta) +void tcp_remap_to_tap(struct ctx *c, in_port_t port, in_port_t delta) { - tcp_port_delta_to_tap[port] = delta; + c->tcp.fwd_in.delta[port] = delta; } /** * tcp_remap_to_tap() - Set delta for port translation toward init namespace + * @c: Execution context * @port: Original destination port, host order * @delta: Delta to be added to original destination port */ -void tcp_remap_to_init(in_port_t port, in_port_t delta) +void tcp_remap_to_init(struct ctx *c, in_port_t port, in_port_t delta) { - tcp_port_delta_to_init[port] = delta; + c->tcp.fwd_out.delta[port] = delta; } /** @@ -3109,11 +3107,9 @@ void tcp_sock_init(const struct ctx *c, int ns, sa_family_t af, int s; if (ns) { - tref.tcp.index = (in_port_t)(port + - tcp_port_delta_to_init[port]); + tref.tcp.index = (in_port_t)(port + c->tcp.fwd_out.delta[port]); } else { - tref.tcp.index = (in_port_t)(port + - tcp_port_delta_to_tap[port]); + tref.tcp.index = (in_port_t)(port + c->tcp.fwd_in.delta[port]); } if (af == AF_INET || af == AF_UNSPEC) { @@ -3133,7 +3129,7 @@ void tcp_sock_init(const struct ctx *c, int ns, sa_family_t af, else s = -1; - if (c->tcp.fwd_mode_in == FWD_AUTO) + if (c->tcp.fwd_in.mode == FWD_AUTO) tcp_sock_init_ext[port][V4] = s; } @@ -3148,7 +3144,7 @@ void tcp_sock_init(const struct ctx *c, int ns, sa_family_t af, else s = -1; - if (c->tcp.fwd_mode_out == FWD_AUTO) { + if (c->tcp.fwd_out.mode == FWD_AUTO) { if (ns) tcp_sock_ns[port][V4] = s; else @@ -3174,7 +3170,7 @@ void tcp_sock_init(const struct ctx *c, int ns, sa_family_t af, else s = -1; - if (c->tcp.fwd_mode_in == FWD_AUTO) + if (c->tcp.fwd_in.mode == FWD_AUTO) tcp_sock_init_ext[port][V6] = s; } @@ -3189,7 +3185,7 @@ void tcp_sock_init(const struct ctx *c, int ns, sa_family_t af, else s = -1; - if (c->tcp.fwd_mode_out == FWD_AUTO) { + if (c->tcp.fwd_out.mode == FWD_AUTO) { if (ns) tcp_sock_ns[port][V6] = s; else @@ -3213,7 +3209,7 @@ static int tcp_sock_init_ns(void *arg) ns_enter(c); for (port = 0; port < USHRT_MAX; port++) { - if (!bitmap_isset(c->tcp.port_to_init, port)) + if (!bitmap_isset(c->tcp.fwd_out.map, port)) continue; tcp_sock_init(c, 1, AF_UNSPEC, NULL, port); @@ -3413,7 +3409,7 @@ static int tcp_port_rebind(void *arg) ns_enter(a->c); for (port = 0; port < USHRT_MAX; port++) { - if (!bitmap_isset(a->c->tcp.port_to_init, port)) { + if (!bitmap_isset(a->c->tcp.fwd_out.map, port)) { if (tcp_sock_ns[port][V4] >= 0) { close(tcp_sock_ns[port][V4]); tcp_sock_ns[port][V4] = -1; @@ -3428,7 +3424,7 @@ static int tcp_port_rebind(void *arg) } /* Don't loop back our own ports */ - if (bitmap_isset(a->c->tcp.port_to_tap, port)) + if (bitmap_isset(a->c->tcp.fwd_in.map, port)) continue; if ((a->c->ifi4 && tcp_sock_ns[port][V4] == -1) || @@ -3437,7 +3433,7 @@ static int tcp_port_rebind(void *arg) } } else { for (port = 0; port < USHRT_MAX; port++) { - if (!bitmap_isset(a->c->tcp.port_to_tap, port)) { + if (!bitmap_isset(a->c->tcp.fwd_in.map, port)) { if (tcp_sock_init_ext[port][V4] >= 0) { close(tcp_sock_init_ext[port][V4]); tcp_sock_init_ext[port][V4] = -1; @@ -3461,7 +3457,7 @@ static int tcp_port_rebind(void *arg) } /* Don't loop back our own ports */ - if (bitmap_isset(a->c->tcp.port_to_init, port)) + if (bitmap_isset(a->c->tcp.fwd_out.map, port)) continue; if ((a->c->ifi4 && tcp_sock_init_ext[port][V4] == -1) || @@ -3489,14 +3485,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_mode_in == FWD_AUTO) { + if (c->tcp.fwd_in.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_mode_out == FWD_AUTO) { + if (c->tcp.fwd_out.mode == FWD_AUTO) { detect_arg.detect_in_ns = 1; NS_CALL(tcp_port_detect, &detect_arg); rebind_arg.bind_in_ns = 0; diff --git a/tcp.h b/tcp.h index 58989db..502b096 100644 --- a/tcp.h +++ b/tcp.h @@ -29,8 +29,8 @@ void tcp_defer_handler(struct ctx *c); void tcp_sock_set_bufsize(const struct ctx *c, int s); void tcp_update_l2_buf(const unsigned char *eth_d, const unsigned char *eth_s, const uint32_t *ip_da); -void tcp_remap_to_tap(in_port_t port, in_port_t delta); -void tcp_remap_to_init(in_port_t port, in_port_t delta); +void tcp_remap_to_tap(struct ctx *c, in_port_t port, in_port_t delta); +void tcp_remap_to_init(struct ctx *c, in_port_t port, in_port_t delta); /** * union tcp_epoll_ref - epoll reference portion for TCP connections @@ -58,9 +58,8 @@ union tcp_epoll_ref { * @conn_count: Count of connections (not spliced) in connection table * @splice_conn_count: Count of spliced connections in connection table * @port_to_tap: Ports bound host-side, packets to tap or spliced - * @fwd_mode_in: Port forwarding mode for inbound packets - * @port_to_init: Ports bound namespace-side, spliced to init - * @fwd_mode_out: Port forwarding mode for outbound packets + * @fwd_in: Port forwarding configuration for inbound packets + * @fwd_out: Port forwarding configuration for outbound packets * @timer_run: Timestamp of most recent timer run * @kernel_snd_wnd: Kernel reports sending window (with commit 8f7baad7f035) * @pipe_size: Size of pipes for spliced connections @@ -69,10 +68,8 @@ struct tcp_ctx { uint64_t hash_secret[2]; int conn_count; int splice_conn_count; - port_fwd_map port_to_tap; - enum port_fwd_mode fwd_mode_in; - port_fwd_map port_to_init; - enum port_fwd_mode fwd_mode_out; + struct port_fwd fwd_in; + struct port_fwd fwd_out; struct timespec timer_run; #ifdef HAS_SND_WND int kernel_snd_wnd; diff --git a/tcp_splice.c b/tcp_splice.c index 61e9b23..edbcfd4 100644 --- a/tcp_splice.c +++ b/tcp_splice.c @@ -514,7 +514,7 @@ static int tcp_splice_new(const struct ctx *c, struct tcp_splice_conn *conn, struct tcp_splice_connect_ns_arg ns_arg = { c, conn, port, 0 }; int *p, i, s = -1; - if (bitmap_isset(c->tcp.port_to_tap, port)) + if (bitmap_isset(c->tcp.fwd_in.map, port)) p = CONN_V6(conn) ? ns_sock_pool6 : ns_sock_pool4; else p = CONN_V6(conn) ? init_sock_pool6 : init_sock_pool4; @@ -525,7 +525,7 @@ static int tcp_splice_new(const struct ctx *c, struct tcp_splice_conn *conn, break; } - if (s < 0 && bitmap_isset(c->tcp.port_to_tap, port)) { + if (s < 0 && bitmap_isset(c->tcp.fwd_in.map, port)) { NS_CALL(tcp_splice_connect_ns, &ns_arg); return ns_arg.ret; } diff --git a/udp.c b/udp.c index 0b4e134..38bb8d8 100644 --- a/udp.c +++ b/udp.c @@ -166,12 +166,6 @@ struct udp_splice_port { static struct udp_tap_port udp_tap_map [IP_VERSIONS][USHRT_MAX]; static struct udp_splice_port udp_splice_map [IP_VERSIONS][USHRT_MAX]; -/* Port re-mappings as delta, indexed by original destination port */ -static in_port_t udp_port_delta_to_tap [USHRT_MAX]; -static in_port_t udp_port_delta_from_tap [USHRT_MAX]; -static in_port_t udp_port_delta_to_init [USHRT_MAX]; -static in_port_t udp_port_delta_from_init[USHRT_MAX]; - enum udp_act_type { UDP_ACT_TAP, UDP_ACT_NS_CONN, @@ -265,24 +259,26 @@ static struct mmsghdr udp_mmh_sendto [UDP_SPLICE_FRAMES]; /** * udp_remap_to_tap() - Set delta for port translation to/from guest/tap + * @c: Execution context * @port: Original destination port, host order * @delta: Delta to be added to original destination port */ -void udp_remap_to_tap(in_port_t port, in_port_t delta) +void udp_remap_to_tap(struct ctx *c, in_port_t port, in_port_t delta) { - udp_port_delta_to_tap[port] = delta; - udp_port_delta_from_tap[port + delta] = USHRT_MAX - delta; + c->udp.fwd_in.f.delta[port] = delta; + c->udp.fwd_in.rdelta[port + delta] = USHRT_MAX - delta; } /** * udp_remap_to_init() - Set delta for port translation to/from init namespace + * @c: Execution context * @port: Original destination port, host order * @delta: Delta to be added to original destination port */ -void udp_remap_to_init(in_port_t port, in_port_t delta) +void udp_remap_to_init(struct ctx *c, in_port_t port, in_port_t delta) { - udp_port_delta_to_init[port] = delta; - udp_port_delta_from_init[port + delta] = USHRT_MAX - delta; + c->udp.fwd_out.f.delta[port] = delta; + c->udp.fwd_out.rdelta[port + delta] = USHRT_MAX - delta; } /** @@ -583,7 +579,7 @@ static void udp_sock_handler_splice(const struct ctx *c, union epoll_ref ref, switch (ref.r.p.udp.udp.splice) { case UDP_TO_NS: - src += udp_port_delta_from_init[src]; + src += c->udp.fwd_out.rdelta[src]; if (!(s = udp_splice_map[v6][src].ns_conn_sock)) { struct udp_splice_connect_ns_arg arg = { @@ -603,7 +599,7 @@ static void udp_sock_handler_splice(const struct ctx *c, union epoll_ref ref, send_dst = udp_splice_map[v6][dst].init_dst_port; break; case UDP_TO_INIT: - src += udp_port_delta_from_tap[src]; + src += c->udp.fwd_in.rdelta[src]; if (!(s = udp_splice_map[v6][src].init_conn_sock)) { s = udp_splice_connect(c, v6, ref.r.s, src, dst, @@ -1121,10 +1117,10 @@ void udp_sock_init(const struct ctx *c, int ns, sa_family_t af, if (ns) { uref.udp.port = (in_port_t)(port + - udp_port_delta_to_init[port]); + c->udp.fwd_out.f.delta[port]); } else { uref.udp.port = (in_port_t)(port + - udp_port_delta_to_tap[port]); + c->udp.fwd_in.f.delta[port]); } if (af == AF_INET || af == AF_UNSPEC) { @@ -1209,7 +1205,7 @@ int udp_sock_init_ns(void *arg) return 0; for (dst = 0; dst < USHRT_MAX; dst++) { - if (!bitmap_isset(c->udp.port_to_init, dst)) + if (!bitmap_isset(c->udp.fwd_out.f.map, dst)) continue; udp_sock_init(c, 1, AF_UNSPEC, NULL, dst); diff --git a/udp.h b/udp.h index 0283ce6..cfd1a97 100644 --- a/udp.h +++ b/udp.h @@ -18,8 +18,8 @@ int udp_init(const struct ctx *c); void udp_timer(struct ctx *c, const struct timespec *ts); void udp_update_l2_buf(const unsigned char *eth_d, const unsigned char *eth_s, const uint32_t *ip_da); -void udp_remap_to_tap(in_port_t port, in_port_t delta); -void udp_remap_to_init(in_port_t port, in_port_t delta); +void udp_remap_to_tap(struct ctx *c, in_port_t port, in_port_t delta); +void udp_remap_to_init(struct ctx *c, in_port_t port, in_port_t delta); /** * union udp_epoll_ref - epoll reference portion for TCP connections @@ -44,19 +44,26 @@ union udp_epoll_ref { uint32_t u32; }; + +/** + * udp_port_fwd - UDP specific port forwarding configuration + * @f: Generic forwarding configuration + * @rdelta: Reversed delta map to translate source ports on return packets + */ +struct udp_port_fwd { + struct port_fwd f; + in_port_t rdelta[USHRT_MAX]; +}; + /** * struct udp_ctx - Execution context for UDP - * @port_to_tap: Ports bound host-side, data to tap or ns L4 socket - * @fwd_mode_in: Port forwarding mode for inbound packets - * @port_to_init: Ports bound namespace-side, data to init L4 socket - * @fwd_mode_out: Port forwarding mode for outbound packets + * @fwd_in: Port forwarding configuration for inbound packets + * @fwd_out: Port forwarding configuration for outbound packets * @timer_run: Timestamp of most recent timer run */ struct udp_ctx { - port_fwd_map port_to_tap; - enum port_fwd_mode fwd_mode_in; - port_fwd_map port_to_init; - enum port_fwd_mode fwd_mode_out; + struct udp_port_fwd fwd_in; + struct udp_port_fwd fwd_out; struct timespec timer_run; }; -- 2.37.3

David Gibson

6:58 a.m.

New subject: [PATCH 3/8] udp: Delay initialization of UDP reversed port mapping table

Because it's connectionless, when mapping UDP ports we need, in addition to the table of deltas for destination ports needed by TCP, we need an inverted table to translate the source ports on return packets. Currently we fill out the inverted table at the same time we construct the main table in udp_remap_to_tap() and udp_remap_to_init(). However, we don't use either table until after we've initialized UDP, so we can delay the construction of the reverse table to udp_init(). This makes the configuration more symmetric between TCP and UDP which will enable further cleanups. Signed-off-by: David Gibson --- udp.c | 25 ++++++++++++++++++++++--- udp.h | 2 +- 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/udp.c b/udp.c index 38bb8d8..eb32dda 100644 --- a/udp.c +++ b/udp.c @@ -109,6 +109,7 @@ #include #include #include +#include #include "checksum.h" #include "util.h" @@ -266,7 +267,6 @@ static struct mmsghdr udp_mmh_sendto [UDP_SPLICE_FRAMES]; void udp_remap_to_tap(struct ctx *c, in_port_t port, in_port_t delta) { c->udp.fwd_in.f.delta[port] = delta; - c->udp.fwd_in.rdelta[port + delta] = USHRT_MAX - delta; } /** @@ -278,7 +278,23 @@ void udp_remap_to_tap(struct ctx *c, in_port_t port, in_port_t delta) void udp_remap_to_init(struct ctx *c, in_port_t port, in_port_t delta) { c->udp.fwd_out.f.delta[port] = delta; - c->udp.fwd_out.rdelta[port + delta] = USHRT_MAX - delta; +} + +/** + * udp_invert_portmap() - Compute reverse port translations for return packets + * @fwd: Port forwarding configuration to compute reverse map for + */ +static void udp_invert_portmap(struct udp_port_fwd *fwd) +{ + int i; + + assert(ARRAY_SIZE(fwd->f.delta) == ARRAY_SIZE(fwd->rdelta)); + for (i = 0; i < ARRAY_SIZE(fwd->f.delta); i++) { + in_port_t delta = fwd->f.delta[i]; + + if (delta) + fwd->rdelta[(in_port_t)i + delta] = USHRT_MAX - delta; + } } /** @@ -1267,7 +1283,7 @@ static void udp_splice_iov_init(void) * * Return: 0 */ -int udp_init(const struct ctx *c) +int udp_init(struct ctx *c) { if (c->ifi4) udp_sock4_iov_init(); @@ -1275,6 +1291,9 @@ int udp_init(const struct ctx *c) if (c->ifi6) udp_sock6_iov_init(); + udp_invert_portmap(&c->udp.fwd_in); + udp_invert_portmap(&c->udp.fwd_out); + if (c->mode == MODE_PASTA) { udp_splice_iov_init(); NS_CALL(udp_sock_init_ns, c); diff --git a/udp.h b/udp.h index cfd1a97..25422b6 100644 --- a/udp.h +++ b/udp.h @@ -14,7 +14,7 @@ int udp_tap_handler(struct ctx *c, int af, const void *addr, const struct pool *p, const struct timespec *now); void udp_sock_init(const struct ctx *c, int ns, sa_family_t af, const void *addr, in_port_t port); -int udp_init(const struct ctx *c); +int udp_init(struct ctx *c); void udp_timer(struct ctx *c, const struct timespec *ts); void udp_update_l2_buf(const unsigned char *eth_d, const unsigned char *eth_s, const uint32_t *ip_da); -- 2.37.3

David Gibson

6:58 a.m.

New subject: [PATCH 4/8] Don't use indirect remap functions for conf_ports()

Now that we've delayed initialization of the UDP specific "reverse" map until udp_init(), the only difference between the various 'remap' functions used in conf_ports() is which array they target. So, simplify by open coding the logic into conf_ports() with a pointer to the correct mapping array. Signed-off-by: David Gibson --- conf.c | 14 +++++++------- tcp.c | 22 ---------------------- tcp.h | 2 -- udp.c | 22 ---------------------- udp.h | 2 -- 5 files changed, 7 insertions(+), 55 deletions(-) diff --git a/conf.c b/conf.c index f74a32f..c7c2cee 100644 --- a/conf.c +++ b/conf.c @@ -120,24 +120,24 @@ static int conf_ports(struct ctx *c, char optname, const char *optarg, { int start_src, end_src, start_dst, end_dst, exclude_only = 1, i, port; char addr_buf[sizeof(struct in6_addr)] = { 0 }, *addr = addr_buf; - void (*remap)(struct ctx *c, in_port_t port, in_port_t delta); char buf[BUFSIZ], *sep, *spec, *p; port_fwd_map exclude = { 0 }; sa_family_t af = AF_UNSPEC; + in_port_t *delta; uint8_t *map; if (optname == 't') { map = c->tcp.fwd_in.map; - remap = tcp_remap_to_tap; + delta = c->tcp.fwd_in.delta; } else if (optname == 'T') { map = c->tcp.fwd_out.map; - remap = tcp_remap_to_init; + delta = c->tcp.fwd_out.delta; } else if (optname == 'u') { map = c->udp.fwd_in.f.map; - remap = udp_remap_to_tap; + delta = c->udp.fwd_in.f.delta; } else if (optname == 'U') { map = c->udp.fwd_out.f.map; - remap = udp_remap_to_init; + delta = c->udp.fwd_out.f.delta; } else { /* For gcc -O3 */ return 0; } @@ -365,8 +365,8 @@ static int conf_ports(struct ctx *c, char optname, const char *optarg, if (start_dst != -1) { /* 80:8080 or 22-80:8080:8080 */ - remap(c, i, (in_port_t)(start_dst - - start_src)); + delta[i] = (in_port_t)(start_dst - + start_src); } if (optname == 't') diff --git a/tcp.c b/tcp.c index e44177f..509a0b3 100644 --- a/tcp.c +++ b/tcp.c @@ -948,28 +948,6 @@ static void conn_event_do(const struct ctx *c, struct tcp_conn *conn, conn_event_do(c, conn, event); \ } while (0) -/** - * tcp_remap_to_tap() - Set delta for port translation toward guest/tap - * @c: Execution context - * @port: Original destination port, host order - * @delta: Delta to be added to original destination port - */ -void tcp_remap_to_tap(struct ctx *c, in_port_t port, in_port_t delta) -{ - c->tcp.fwd_in.delta[port] = delta; -} - -/** - * tcp_remap_to_tap() - Set delta for port translation toward init namespace - * @c: Execution context - * @port: Original destination port, host order - * @delta: Delta to be added to original destination port - */ -void tcp_remap_to_init(struct ctx *c, in_port_t port, in_port_t delta) -{ - c->tcp.fwd_out.delta[port] = delta; -} - /** * tcp_rtt_dst_low() - Check if low RTT was seen for connection endpoint * @conn: Connection pointer diff --git a/tcp.h b/tcp.h index 502b096..2548d4d 100644 --- a/tcp.h +++ b/tcp.h @@ -29,8 +29,6 @@ void tcp_defer_handler(struct ctx *c); void tcp_sock_set_bufsize(const struct ctx *c, int s); void tcp_update_l2_buf(const unsigned char *eth_d, const unsigned char *eth_s, const uint32_t *ip_da); -void tcp_remap_to_tap(struct ctx *c, in_port_t port, in_port_t delta); -void tcp_remap_to_init(struct ctx *c, in_port_t port, in_port_t delta); /** * union tcp_epoll_ref - epoll reference portion for TCP connections diff --git a/udp.c b/udp.c index eb32dda..d17b3b4 100644 --- a/udp.c +++ b/udp.c @@ -258,28 +258,6 @@ static struct mmsghdr udp_mmh_send [UDP_SPLICE_FRAMES]; static struct iovec udp_iov_sendto [UDP_SPLICE_FRAMES]; static struct mmsghdr udp_mmh_sendto [UDP_SPLICE_FRAMES]; -/** - * udp_remap_to_tap() - Set delta for port translation to/from guest/tap - * @c: Execution context - * @port: Original destination port, host order - * @delta: Delta to be added to original destination port - */ -void udp_remap_to_tap(struct ctx *c, in_port_t port, in_port_t delta) -{ - c->udp.fwd_in.f.delta[port] = delta; -} - -/** - * udp_remap_to_init() - Set delta for port translation to/from init namespace - * @c: Execution context - * @port: Original destination port, host order - * @delta: Delta to be added to original destination port - */ -void udp_remap_to_init(struct ctx *c, in_port_t port, in_port_t delta) -{ - c->udp.fwd_out.f.delta[port] = delta; -} - /** * udp_invert_portmap() - Compute reverse port translations for return packets * @fwd: Port forwarding configuration to compute reverse map for diff --git a/udp.h b/udp.h index 25422b6..bc7b259 100644 --- a/udp.h +++ b/udp.h @@ -18,8 +18,6 @@ int udp_init(struct ctx *c); void udp_timer(struct ctx *c, const struct timespec *ts); void udp_update_l2_buf(const unsigned char *eth_d, const unsigned char *eth_s, const uint32_t *ip_da); -void udp_remap_to_tap(struct ctx *c, in_port_t port, in_port_t delta); -void udp_remap_to_init(struct ctx *c, in_port_t port, in_port_t delta); /** * union udp_epoll_ref - epoll reference portion for TCP connections -- 2.37.3

David Gibson

6:58 a.m.

New subject: [PATCH 5/8] Pass entire port forwarding configuration substructure to conf_ports()

conf_ports() switches on the optname argument to select the target array for several updates. Now that all these maps are in a common structure, we can simplify by just passing in a pointer to the whole struct port_fwd to update. Signed-off-by: David Gibson --- conf.c | 62 +++++++++++++++++++++------------------------------------- 1 file changed, 22 insertions(+), 40 deletions(-) diff --git a/conf.c b/conf.c index c7c2cee..2cbc519 100644 --- a/conf.c +++ b/conf.c @@ -111,58 +111,40 @@ static int get_bound_ports_ns(void *arg) * @c: Execution context * @optname: Short option name, t, T, u, or U * @optarg: Option argument (port specification) - * @set: Pointer to @port_fwd_mode to be set (port forwarding mode) + * @fwd: Pointer to @port_fwd to be updated * * Return: -EINVAL on parsing error, 0 otherwise */ -static int conf_ports(struct ctx *c, char optname, const char *optarg, - enum port_fwd_mode *set) +static int conf_ports(const struct ctx *c, char optname, const char *optarg, + struct port_fwd *fwd) { int start_src, end_src, start_dst, end_dst, exclude_only = 1, i, port; char addr_buf[sizeof(struct in6_addr)] = { 0 }, *addr = addr_buf; char buf[BUFSIZ], *sep, *spec, *p; port_fwd_map exclude = { 0 }; sa_family_t af = AF_UNSPEC; - in_port_t *delta; - uint8_t *map; - - if (optname == 't') { - map = c->tcp.fwd_in.map; - delta = c->tcp.fwd_in.delta; - } else if (optname == 'T') { - map = c->tcp.fwd_out.map; - delta = c->tcp.fwd_out.delta; - } else if (optname == 'u') { - map = c->udp.fwd_in.f.map; - delta = c->udp.fwd_in.f.delta; - } else if (optname == 'U') { - map = c->udp.fwd_out.f.map; - delta = c->udp.fwd_out.f.delta; - } else { /* For gcc -O3 */ - return 0; - } if (!strcmp(optarg, "none")) { - if (*set) + if (fwd->mode) return -EINVAL; - *set = FWD_NONE; + fwd->mode = FWD_NONE; return 0; } if (!strcmp(optarg, "auto")) { - if (*set || c->mode != MODE_PASTA) + if (fwd->mode || c->mode != MODE_PASTA) return -EINVAL; - *set = FWD_AUTO; + fwd->mode = FWD_AUTO; return 0; } if (!strcmp(optarg, "all")) { int i; - if (*set || c->mode != MODE_PASST) + if (fwd->mode || c->mode != MODE_PASST) return -EINVAL; - *set = FWD_ALL; - memset(map, 0xff, PORT_EPHEMERAL_MIN / 8); + fwd->mode = FWD_ALL; + memset(fwd->map, 0xff, PORT_EPHEMERAL_MIN / 8); for (i = 0; i < PORT_EPHEMERAL_MIN; i++) { if (optname == 't') @@ -174,10 +156,10 @@ static int conf_ports(struct ctx *c, char optname, const char *optarg, return 0; } - if (*set > FWD_SPEC) + if (fwd->mode > FWD_SPEC) return -EINVAL; - *set = FWD_SPEC; + fwd->mode = FWD_SPEC; strncpy(buf, optarg, sizeof(buf) - 1); @@ -265,7 +247,7 @@ static int conf_ports(struct ctx *c, char optname, const char *optarg, if (bitmap_isset(exclude, i)) continue; - bitmap_set(map, i); + bitmap_set(fwd->map, i); if (optname == 't') tcp_sock_init(c, 0, af, addr, i); @@ -355,18 +337,18 @@ static int conf_ports(struct ctx *c, char optname, const char *optarg, goto bad; /* 22-81:8022:8080 */ for (i = start_src; i <= end_src; i++) { - if (bitmap_isset(map, i)) + if (bitmap_isset(fwd->map, i)) goto overlap; if (bitmap_isset(exclude, i)) continue; - bitmap_set(map, i); + bitmap_set(fwd->map, i); if (start_dst != -1) { /* 80:8080 or 22-80:8080:8080 */ - delta[i] = (in_port_t)(start_dst - - start_src); + fwd->delta[i] = (in_port_t)(start_dst - + start_src); } if (optname == 't') @@ -1548,7 +1530,7 @@ void conf(struct ctx *c, int argc, char **argv) /* Now we can process port configuration options */ optind = 1; do { - enum port_fwd_mode *fwd = NULL; + struct port_fwd *fwd = NULL; const char *optstring; if (c->mode == MODE_PASST) @@ -1563,13 +1545,13 @@ void conf(struct ctx *c, int argc, char **argv) case 'T': case 'U': if (name == 't') - fwd = &c->tcp.fwd_in.mode; + fwd = &c->tcp.fwd_in; else if (name == 'T') - fwd = &c->tcp.fwd_out.mode; + fwd = &c->tcp.fwd_out; else if (name == 'u') - fwd = &c->udp.fwd_in.f.mode; + fwd = &c->udp.fwd_in.f; else if (name == 'U') - fwd = &c->udp.fwd_out.f.mode; + fwd = &c->udp.fwd_out.f; if (!optarg || conf_ports(c, name, optarg, fwd)) usage(argv[0]); -- 2.37.3

David Gibson

6:58 a.m.

New subject: [PATCH 6/8] Treat port numbers as unsigned

Port numbers are unsigned values, but we're storing them in (signed) int variables in some places. This isn't actually harmful, because int is large enough to hold the entire range of ports. However in places we don't want to use an in_port_t (usually to avoid overflow on the last iteration of a loop) it makes more conceptual sense to use an unsigned int. This will also avoid some problems with later cleanups. Signed-off-by: David Gibson --- conf.c | 11 ++++++----- tcp.c | 4 ++-- udp.c | 2 +- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/conf.c b/conf.c index 2cbc519..503d894 100644 --- a/conf.c +++ b/conf.c @@ -118,11 +118,12 @@ static int get_bound_ports_ns(void *arg) static int conf_ports(const struct ctx *c, char optname, const char *optarg, struct port_fwd *fwd) { - int start_src, end_src, start_dst, end_dst, exclude_only = 1, i, port; char addr_buf[sizeof(struct in6_addr)] = { 0 }, *addr = addr_buf; + int start_src, end_src, start_dst, end_dst, exclude_only = 1, i; char buf[BUFSIZ], *sep, *spec, *p; port_fwd_map exclude = { 0 }; sa_family_t af = AF_UNSPEC; + unsigned port; if (!strcmp(optarg, "none")) { if (fwd->mode) @@ -204,11 +205,11 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg, p++; errno = 0; - port = strtol(p, &sep, 10); + port = strtoul(p, &sep, 10); if (sep == p) break; - if (port < 0 || port > USHRT_MAX || errno) + if (port > USHRT_MAX || errno) goto bad; switch (*sep) { @@ -271,11 +272,11 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg, break; errno = 0; - port = strtol(p, &sep, 10); + port = strtoul(p, &sep, 10); if (sep == p) break; - if (port < 0 || port > USHRT_MAX || errno) + if (port > USHRT_MAX || errno) goto bad; /* -p 22 diff --git a/tcp.c b/tcp.c index 509a0b3..d96232c 100644 --- a/tcp.c +++ b/tcp.c @@ -3182,7 +3182,7 @@ void tcp_sock_init(const struct ctx *c, int ns, sa_family_t af, static int tcp_sock_init_ns(void *arg) { struct ctx *c = (struct ctx *)arg; - int port; + unsigned port; ns_enter(c); @@ -3381,7 +3381,7 @@ struct tcp_port_rebind_arg { static int tcp_port_rebind(void *arg) { struct tcp_port_rebind_arg *a = (struct tcp_port_rebind_arg *)arg; - int port; + unsigned port; if (a->bind_in_ns) { ns_enter(a->c); diff --git a/udp.c b/udp.c index d17b3b4..27c3aa3 100644 --- a/udp.c +++ b/udp.c @@ -1193,7 +1193,7 @@ void udp_sock_init(const struct ctx *c, int ns, sa_family_t af, int udp_sock_init_ns(void *arg) { struct ctx *c = (struct ctx *)arg; - int dst; + unsigned dst; if (ns_enter(c)) return 0; -- 2.37.3

David Gibson

6:58 a.m.

New subject: [PATCH 7/8] Fix widespread off-by-one error dealing with port numbers

Port numbers (for both TCP and UDP) are 16-bit, and so fit exactly into a 'short'. USHRT_MAX is therefore the maximum port number and this is widely used in the code. Unfortunately, a lot of those places don't actually want the maximum port number (USHRT_MAX == 65535), they want the total number of ports (65536). This leads to a number of potentially nasty consequences: * We have buffer overruns on the port_fwd::delta array if we try to use port 65535 * We have similar potential overruns for the tcp_sock_* arrays * Interestingly udp_act had the correct size, but we can calculate it in a more direct manner * We have a logical overrun of the ports bitmap as well, although it will just use an unused bit in the last byte so isnt harmful * Many loops don't consider port 65535 (which does mitigate some but not all of the buffer overruns above) * In udp_invert_portmap() we incorrectly compute the reverse port translation for return packets Correct all these by using a new NUM_PORTS defined explicitly for this purpose. Signed-off-by: David Gibson --- conf.c | 4 ++-- port_fwd.h | 7 +++++-- tcp.c | 12 ++++++------ udp.c | 10 +++++----- udp.h | 2 +- 5 files changed, 19 insertions(+), 16 deletions(-) diff --git a/conf.c b/conf.c index 503d894..5cdfe31 100644 --- a/conf.c +++ b/conf.c @@ -209,7 +209,7 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg, if (sep == p) break; - if (port > USHRT_MAX || errno) + if (port >= NUM_PORTS || errno) goto bad; switch (*sep) { @@ -276,7 +276,7 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg, if (sep == p) break; - if (port > USHRT_MAX || errno) + if (port >= NUM_PORTS || errno) goto bad; /* -p 22 diff --git a/port_fwd.h b/port_fwd.h index 5507bf2..976138f 100644 --- a/port_fwd.h +++ b/port_fwd.h @@ -7,6 +7,9 @@ #ifndef PORT_FWD_H #define PORT_FWD_H +/* Number of ports for both TCP and UDP */ +#define NUM_PORTS (1U << 16) + enum port_fwd_mode { FWD_SPEC = 1, FWD_NONE, @@ -14,7 +17,7 @@ enum port_fwd_mode { FWD_ALL, }; -typedef uint8_t port_fwd_map[DIV_ROUND_UP(USHRT_MAX, 8)]; +typedef uint8_t port_fwd_map[DIV_ROUND_UP(NUM_PORTS, 8)]; /** * port_fwd - Describes port forwarding for one protocol and direction @@ -25,7 +28,7 @@ typedef uint8_t port_fwd_map[DIV_ROUND_UP(USHRT_MAX, 8)]; struct port_fwd { enum port_fwd_mode mode; port_fwd_map map; - in_port_t delta[USHRT_MAX]; + in_port_t delta[NUM_PORTS]; }; #endif /* PORT_FWD_H */ diff --git a/tcp.c b/tcp.c index d96232c..e45dfda 100644 --- a/tcp.c +++ b/tcp.c @@ -547,9 +547,9 @@ static const char *tcp_flag_str[] __attribute((__unused__)) = { }; /* Listening sockets, used for automatic port forwarding in pasta mode only */ -static int tcp_sock_init_lo [USHRT_MAX][IP_VERSIONS]; -static int tcp_sock_init_ext [USHRT_MAX][IP_VERSIONS]; -static int tcp_sock_ns [USHRT_MAX][IP_VERSIONS]; +static int tcp_sock_init_lo [NUM_PORTS][IP_VERSIONS]; +static int tcp_sock_init_ext [NUM_PORTS][IP_VERSIONS]; +static int tcp_sock_ns [NUM_PORTS][IP_VERSIONS]; /* Table of destinations with very low RTT (assumed to be local), LRU */ static struct in6_addr low_rtt_dst[LOW_RTT_TABLE_SIZE]; @@ -3186,7 +3186,7 @@ static int tcp_sock_init_ns(void *arg) ns_enter(c); - for (port = 0; port < USHRT_MAX; port++) { + for (port = 0; port < NUM_PORTS; port++) { if (!bitmap_isset(c->tcp.fwd_out.map, port)) continue; @@ -3386,7 +3386,7 @@ static int tcp_port_rebind(void *arg) if (a->bind_in_ns) { ns_enter(a->c); - for (port = 0; port < USHRT_MAX; port++) { + for (port = 0; port < NUM_PORTS; port++) { if (!bitmap_isset(a->c->tcp.fwd_out.map, port)) { if (tcp_sock_ns[port][V4] >= 0) { close(tcp_sock_ns[port][V4]); @@ -3410,7 +3410,7 @@ static int tcp_port_rebind(void *arg) tcp_sock_init(a->c, 1, AF_UNSPEC, NULL, port); } } else { - for (port = 0; port < USHRT_MAX; port++) { + for (port = 0; port < NUM_PORTS; port++) { if (!bitmap_isset(a->c->tcp.fwd_in.map, port)) { if (tcp_sock_init_ext[port][V4] >= 0) { close(tcp_sock_init_ext[port][V4]); diff --git a/udp.c b/udp.c index 27c3aa3..bd3dc72 100644 --- a/udp.c +++ b/udp.c @@ -164,8 +164,8 @@ struct udp_splice_port { }; /* Port tracking, arrays indexed by packet source port (host order) */ -static struct udp_tap_port udp_tap_map [IP_VERSIONS][USHRT_MAX]; -static struct udp_splice_port udp_splice_map [IP_VERSIONS][USHRT_MAX]; +static struct udp_tap_port udp_tap_map [IP_VERSIONS][NUM_PORTS]; +static struct udp_splice_port udp_splice_map [IP_VERSIONS][NUM_PORTS]; enum udp_act_type { UDP_ACT_TAP, @@ -175,7 +175,7 @@ enum udp_act_type { }; /* Activity-based aging for bindings */ -static uint8_t udp_act[IP_VERSIONS][UDP_ACT_TYPE_MAX][(USHRT_MAX + 1) / 8]; +static uint8_t udp_act[IP_VERSIONS][UDP_ACT_TYPE_MAX][DIV_ROUND_UP(NUM_PORTS, 8)]; /* Static buffers */ @@ -271,7 +271,7 @@ static void udp_invert_portmap(struct udp_port_fwd *fwd) in_port_t delta = fwd->f.delta[i]; if (delta) - fwd->rdelta[(in_port_t)i + delta] = USHRT_MAX - delta; + fwd->rdelta[(in_port_t)i + delta] = NUM_PORTS - delta; } } @@ -1198,7 +1198,7 @@ int udp_sock_init_ns(void *arg) if (ns_enter(c)) return 0; - for (dst = 0; dst < USHRT_MAX; dst++) { + for (dst = 0; dst < NUM_PORTS; dst++) { if (!bitmap_isset(c->udp.fwd_out.f.map, dst)) continue; diff --git a/udp.h b/udp.h index bc7b259..d14df0a 100644 --- a/udp.h +++ b/udp.h @@ -50,7 +50,7 @@ union udp_epoll_ref { */ struct udp_port_fwd { struct port_fwd f; - in_port_t rdelta[USHRT_MAX]; + in_port_t rdelta[NUM_PORTS]; }; /** -- 2.37.3

David Gibson

6:58 a.m.

New subject: [PATCH 8/8] icmp: Correct off by one errors dealing with number of echo request ids

ICMP echo request and reply packets include a 16-bit 'id' value. We have some arrays indexed by this id value. Unfortunately we size those arrays with USHRT_MAX (65535) when they need to be sized by the total number of id values (65536). This could lead to buffer overruns. Resize the arrays correctly, using a new define for the purpose. Signed-off-by: David Gibson --- icmp.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/icmp.c b/icmp.c index 2da8b58..39a8694 100644 --- a/icmp.c +++ b/icmp.c @@ -39,6 +39,7 @@ #include "icmp.h" #define ICMP_ECHO_TIMEOUT 60 /* s, timeout for ICMP socket activity */ +#define ICMP_NUM_IDS (1U << 16) /** * struct icmp_id_sock - Tracking information for single ICMP echo identifier @@ -53,10 +54,10 @@ struct icmp_id_sock { }; /* Indexed by ICMP echo identifier */ -static struct icmp_id_sock icmp_id_map [IP_VERSIONS][USHRT_MAX]; +static struct icmp_id_sock icmp_id_map[IP_VERSIONS][ICMP_NUM_IDS]; /* Bitmaps, activity monitoring needed for identifier */ -static uint8_t icmp_act [IP_VERSIONS][USHRT_MAX / 8]; +static uint8_t icmp_act[IP_VERSIONS][DIV_ROUND_UP(ICMP_NUM_IDS, 8)]; /** * icmp_sock_handler() - Handle new data from socket -- 2.37.3

982

Age (days ago)

983

Last active (days ago)

List overview

Download

12 comments

2 participants

participants (2)

David Gibson
Stefano Brivio

[PATCH 0/8] Clean up and fix bugs in port forwarding data structures

tags

participants (2)