Re: [PATCH v2 05/10] flow: Introduce struct flowside, space for uniform tracking of addresses

[Stefano Brivio <sbrivio@redhat.com>]

On Mon, 28 Aug 2023 15:41:41 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> Handling of each protocol needs some degree of tracking of the addresses
> and ports at the end of each connection or flow.  Sometimes that's explicit
> (as in the guest visible addresses for TCP connections), sometimes implicit
> (the bound and connected addresses of sockets).
> 
> To allow more general abd robust handling, and more consistency across
> protocols we want to uniformly track the address and port at each end of
> the connection.  Furthermore, because we allow port remapping, and we
> sometimes need to apply NAT, the addresses and ports can be different as
> seen by the guest/namespace and as by the host.
> 
> Introduce 'struct flowside' to keep track of the address and ports of a
> flow from a single "side" (guest or host).  Store two of these in the
> common fields of a flow to track that information for both sides.
> 
> For now we just introduce the structure and fields themselves, along with
> some simple helpers.  Later patches will actually use these to store useful
> information.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  flow.c | 22 ++++++++++++++++++++++
>  flow.h | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 70 insertions(+)
> 
> diff --git a/flow.c b/flow.c
> index 12ca8db..a93cf8c 100644
> --- a/flow.c
> +++ b/flow.c
> @@ -7,6 +7,7 @@
>  
>  #include <unistd.h>
>  #include <string.h>
> +#include <arpa/inet.h>
>  
>  #include "util.h"
>  #include "passt.h"
> @@ -24,6 +25,27 @@ const char *flow_type_str[] = {
>  /* Global Flow Table */
>  union flow flowtab[FLOW_MAX];
>  
> +/** flowside_fmt - Format a flowside as a string
> + * @fs:		flowside to format
> + * @buf:	Buffer into which to store the formatted version
> + * @size:	Size of @buf
> + *
> + * Return: pointer to formatted string describing @fs, or NULL on error
> + */
> +/* cppcheck-suppress unusedFunction */
> +const char *flowside_fmt(const struct flowside *fs, char *buf, size_t size)
> +{
> +	char ebuf[INET6_ADDRSTRLEN], fbuf[INET6_ADDRSTRLEN];
> +
> +	if (!inet_ntop(AF_INET6, &fs->eaddr, ebuf, sizeof(ebuf))
> +	    || !inet_ntop(AF_INET6, &fs->faddr, fbuf, sizeof(fbuf)))

For consistency (also with flowside_complete() below):

	if (!inet_ntop(AF_INET6, &fs->eaddr, ebuf, sizeof(ebuf)) ||
	    !inet_ntop(AF_INET6, &fs->faddr, fbuf, sizeof(fbuf)))

> +		return NULL;
> +
> +	snprintf(buf, size, "[%s]:%hu <-> [%s]:%hu", fbuf, fs->fport,
> +		 ebuf, fs->eport);
> +	return (const char *)buf;
> +}
> +
>  /**
>   * flow_table_compact() - Perform compaction on flow table
>   * @c:		Execution context
> diff --git a/flow.h b/flow.h
> index e212796..9891fcb 100644
> --- a/flow.h
> +++ b/flow.h
> @@ -18,11 +18,59 @@ extern const char *flow_type_str[];
>  #define FLOW_TYPE(f)							\
>          ((f)->type <= FLOW_MAX ? flow_type_str[(f)->type] : "?")
>  
> +/**
> + * struct flowside - Describes a logical packet flow as seen from one "side"
> + * @eaddr:	Endpoint address (remote address from passt's PoV)
> + * @faddr:	Forwarding address (local address from passt's PoV)
> + * @eport:	Endpoint port
> + * @fport:	Forwarding port
> + */
> +struct flowside {
> +	union inany_addr faddr;
> +	union inany_addr eaddr;
> +	in_port_t fport, eport;

I guess always valid, but uncommon (compared to in_port_t x; in_port_t
y;)?

> +};
> +
> +/** flowside_from_af - Initialize a flowside from addresses
> + * @fs:		flowside to initialize
> + * @af:		Address family (AF_INET or AF_INET6)
> + * @faddr:	Forwarding address (pointer to in_addr or in6_addr)
> + * @fport:	Forwarding port
> + * @eaddr:	Endpoint address (pointer to in_addr or in6_addr)
> + * @eport:	Endpoint port
> + */
> +static inline void flowside_from_af(struct flowside *fs, int af,
> +				    const void *faddr, in_port_t fport,
> +				    const void *eaddr, in_port_t eport)
> +{
> +	inany_from_af(&fs->faddr, af, faddr);
> +	inany_from_af(&fs->eaddr, af, eaddr);
> +	fs->fport = fport;
> +	fs->eport = eport;
> +}
> +
> +/** flowside_complete - Check if flowside is fully initialized
> + * @fs:		flowside to check
> + */
> +static inline bool flowside_complete(const struct flowside *fs)
> +{
> +	return !IN6_IS_ADDR_UNSPECIFIED(&fs->faddr) &&
> +		!IN6_IS_ADDR_UNSPECIFIED(&fs->eaddr) &&
> +		fs->fport != 0 && fs->eport != 0;

Zero is reserved for TCP (which could be problematic anyway if we try
to match things), but for UDP a "zero" port can actually be used (in
the probably desuete sense of "no port"). Maybe we should use -1
instead?

> +}
> +
> +#define FLOWSIDE_STRLEN		(2*(INET6_ADDRSTRLEN+8) + 6)

For consistency: "(2 * INET6_ADDRSTRLEN + 8) + 6)".

Limited to the usage I've seen in 6/10 (maybe I'm ignoring something):
is it worth it to have flowside_fmt() as a function forming a string,
rather than something calling debug() with what we want...? At the
moment we have tap_packet_debug(), admittedly not very elegant but
perhaps more terse than this.

-- 
Stefano