From: David Gibson <david@gibson.dropbear.id.au>
To: Stefano Brivio <sbrivio@redhat.com>
Cc: passt-dev@passt.top, Yalan Zhang <yalzhang@redhat.com>
Subject: Re: [PATCH] conf, tcp, udp: Exit if we fail to bind sockets for all given ports
Date: Thu, 16 Feb 2023 15:42:54 +1100 [thread overview]
Message-ID: <Y+20TnH1tY5e9JkZ@yekko> (raw)
In-Reply-To: <20230216010720.2224138-1-sbrivio@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 8411 bytes --]
On Thu, Feb 16, 2023 at 02:07:20AM +0100, Stefano Brivio wrote:
> passt supports ranges of forwarded ports as well as 'all' for TCP and
> UDP, so it might be convenient to proceed if we fail to bind only
> some of the desired ports.
>
> But if we fail to bind even a single port for a given specification,
> we're clearly, unexpectedly, conflicting with another network
> service. In that case, report failure and exit.
>
> Reported-by: Yalan Zhang <yalzhang@redhat.com>
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> conf.c | 47 ++++++++++++++++++++++++++++++++++-------------
> tcp.c | 25 ++++++++++++++++++-------
> tcp.h | 4 ++--
> udp.c | 16 +++++++++++++---
> udp.h | 4 ++--
> 5 files changed, 69 insertions(+), 27 deletions(-)
>
> diff --git a/conf.c b/conf.c
> index f175405..675d961 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -179,9 +179,9 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
> {
> char addr_buf[sizeof(struct in6_addr)] = { 0 }, *addr = addr_buf;
> char buf[BUFSIZ], *spec, *ifname = NULL, *p;
> + bool exclude_only = true, bound_one = false;
> uint8_t exclude[PORT_BITMAP_SIZE] = { 0 };
> sa_family_t af = AF_UNSPEC;
> - bool exclude_only = true;
>
> if (!strcmp(optarg, "none")) {
> if (fwd->mode)
> @@ -215,12 +215,19 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
> memset(fwd->map, 0xff, PORT_EPHEMERAL_MIN / 8);
>
> for (i = 0; i < PORT_EPHEMERAL_MIN; i++) {
> - if (optname == 't')
> - tcp_sock_init(c, AF_UNSPEC, NULL, NULL, i);
> - else if (optname == 'u')
> - udp_sock_init(c, 0, AF_UNSPEC, NULL, NULL, i);
> + if (optname == 't') {
> + if (!tcp_sock_init(c, AF_UNSPEC, NULL, NULL, i))
> + bound_one = true;
> + } else if (optname == 'u') {
> + if (!udp_sock_init(c, 0, AF_UNSPEC, NULL, NULL,
> + i))
> + bound_one = true;
> + }
> }
>
> + if (!bound_one)
> + goto bind_fail;
> +
> return;
> }
>
> @@ -293,12 +300,18 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
>
> bitmap_set(fwd->map, i);
>
> - if (optname == 't')
> - tcp_sock_init(c, af, addr, ifname, i);
> - else if (optname == 'u')
> - udp_sock_init(c, 0, af, addr, ifname, i);
> + if (optname == 't') {
> + if (!tcp_sock_init(c, af, addr, ifname, i))
> + bound_one = true;
> + } else if (optname == 'u') {
> + if (!udp_sock_init(c, 0, af, addr, ifname, i))
> + bound_one = true;
> + }
> }
>
> + if (!bound_one)
> + goto bind_fail;
> +
> return;
> }
>
> @@ -339,13 +352,19 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
>
> fwd->delta[i] = mapped_range.first - orig_range.first;
>
> - if (optname == 't')
> - tcp_sock_init(c, af, addr, ifname, i);
> - else if (optname == 'u')
> - udp_sock_init(c, 0, af, addr, ifname, i);
> + if (optname == 't') {
> + if (!tcp_sock_init(c, af, addr, ifname, i))
> + bound_one = true;
> + } else if (optname == 'u') {
> + if (udp_sock_init(c, 0, af, addr, ifname, i))
> + bound_one = true;
> + }
> }
> } while ((p = next_chunk(p, ',')));
>
> + if (!bound_one)
> + goto bind_fail;
> +
> return;
> bad:
> die("Invalid port specifier %s", optarg);
> @@ -353,6 +372,8 @@ overlap:
> die("Overlapping port specifier %s", optarg);
> mode_conflict:
> die("Port forwarding mode '%s' conflicts with previous mode", optarg);
> +bind_fail:
> + die("Failed to bind any port for '-%c %s', exiting", optname, optarg);
> }
>
> /**
> diff --git a/tcp.c b/tcp.c
> index f028e01..32ce1e9 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -2916,20 +2916,31 @@ static int tcp_sock_init_af(const struct ctx *c, int af, in_port_t port,
> * @addr: Pointer to address for binding, NULL if not configured
> * @ifname: Name of interface to bind to, NULL if not configured
> * @port: Port, host order
> + *
> + * Return: 0 on (partial) success, -1 on (complete) failure
> */
> -void tcp_sock_init(const struct ctx *c, sa_family_t af, const void *addr,
> - const char *ifname, in_port_t port)
> +int tcp_sock_init(const struct ctx *c, sa_family_t af, const void *addr,
> + const char *ifname, in_port_t port)
> {
> + int ret = 0;
> +
> if (af == AF_UNSPEC && c->ifi4 && c->ifi6)
> /* Attempt to get a dual stack socket */
> if (tcp_sock_init_af(c, AF_UNSPEC, port, addr, ifname) >= 0)
> - return;
> + return 0;
>
> /* Otherwise create a socket per IP version */
> - if ((af == AF_INET || af == AF_UNSPEC) && c->ifi4)
> - tcp_sock_init_af(c, AF_INET, port, addr, ifname);
> - if ((af == AF_INET6 || af == AF_UNSPEC) && c->ifi6)
> - tcp_sock_init_af(c, AF_INET6, port, addr, ifname);
> + if ((af == AF_INET || af == AF_UNSPEC) && c->ifi4) {
> + if (tcp_sock_init_af(c, AF_INET, port, addr, ifname) < 0)
> + ret = -1;
> + }
> +
> + if ((af == AF_INET6 || af == AF_UNSPEC) && c->ifi6) {
> + if (tcp_sock_init_af(c, AF_INET6, port, addr, ifname) < 0)
> + ret = -1;
> + }
> +
> + return ret;
> }
>
> /**
> diff --git a/tcp.h b/tcp.h
> index 236038f..5527c5b 100644
> --- a/tcp.h
> +++ b/tcp.h
> @@ -17,8 +17,8 @@ void tcp_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events,
> const struct timespec *now);
> int tcp_tap_handler(struct ctx *c, int af, const void *addr,
> const struct pool *p, const struct timespec *now);
> -void tcp_sock_init(const struct ctx *c, sa_family_t af, const void *addr,
> - const char *ifname, in_port_t port);
> +int tcp_sock_init(const struct ctx *c, sa_family_t af, const void *addr,
> + const char *ifname, in_port_t port);
> int tcp_init(struct ctx *c);
> void tcp_timer(struct ctx *c, const struct timespec *ts);
> void tcp_defer_handler(struct ctx *c);
> diff --git a/udp.c b/udp.c
> index 2e9b7e6..c913d27 100644
> --- a/udp.c
> +++ b/udp.c
> @@ -955,12 +955,14 @@ int udp_tap_handler(struct ctx *c, int af, const void *addr,
> * @addr: Pointer to address for binding, NULL if not configured
> * @ifname: Name of interface to bind to, NULL if not configured
> * @port: Port, host order
> + *
> + * Return: 0 on (partial) success, -1 on (complete) failure
> */
> -void udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
> - const void *addr, const char *ifname, in_port_t port)
> +int udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
> + const void *addr, const char *ifname, in_port_t port)
> {
> union udp_epoll_ref uref = { .u32 = 0 };
> - int s;
> + int s, ret = 0;
>
> if (ns) {
> uref.udp.port = (in_port_t)(port +
> @@ -989,6 +991,9 @@ void udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
> ifname, port, uref.u32);
> udp_splice_ns[V4][port].sock = s;
> }
> +
> + if (s < 0)
> + ret = -1;
> }
>
> if ((af == AF_INET6 || af == AF_UNSPEC) && c->ifi6) {
> @@ -1009,7 +1014,12 @@ void udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
> ifname, port, uref.u32);
> udp_splice_ns[V6][port].sock = s;
> }
> +
> + if (s < 0)
> + ret = -1;
> }
> +
> + return ret;
> }
>
> /**
> diff --git a/udp.h b/udp.h
> index 68082ea..0e81be8 100644
> --- a/udp.h
> +++ b/udp.h
> @@ -12,8 +12,8 @@ void udp_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events,
> const struct timespec *now);
> 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, const char *ifname, in_port_t port);
> +int udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
> + const void *addr, const char *ifname, in_port_t port);
> 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,
--
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 --]
prev parent reply other threads:[~2023-02-16 5:43 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-02-16 1:07 [PATCH] conf, tcp, udp: Exit if we fail to bind sockets for all given ports Stefano Brivio
2023-02-16 4:42 ` David Gibson [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y+20TnH1tY5e9JkZ@yekko \
--to=david@gibson.dropbear.id.au \
--cc=passt-dev@passt.top \
--cc=sbrivio@redhat.com \
--cc=yalzhang@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).