Re: [PATCH v6 14/18] pesto: Read current ruleset from passt/pasta and optionally display it

[Laurent Vivier <lvivier@redhat.com>]

On 5/3/26 23:55, Stefano Brivio wrote:
> From: David Gibson <david@gibson.dropbear.id.au>
> 
> Implement serialisation of our current forwarding rules in conf.c,
> deserialising it to display in the pesto client.  Doing this requires
> adding ip.c, inany.c, bitmap.c, lineread.c and fwd_rule.c to the pesto
> build.  With previous preparations that now requires only a trivial change
> to lineread.c.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> [sbrivio: Use ntohs() for rule->to instead of htons() in
>   fwd_rule_read(), reported by Jon Maloy]
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

With the "pc->fwd.count <= MAX_FWD_RULES" check added below, add:

Reviewed-by: Laurent Vivier <lvivier@redhat.com>

More cosmetics nit below

> ---
>   Makefile   | 17 +++++++++++++----
>   conf.c     | 12 +++++++++++-
>   fwd_rule.c | 41 +++++++++++++++++++++++++++++++++++++++++
>   fwd_rule.h |  4 ++++
>   lineread.c |  2 +-
>   pesto.c    | 37 ++++++++++++++++++++++++++++++++++---
>   pesto.h    |  6 ++++++
>   7 files changed, 110 insertions(+), 9 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 6da76b4..057e4eb 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -47,7 +47,7 @@ PASST_SRCS = arch.c arp.c bitmap.c checksum.c conf.c dhcp.c dhcpv6.c \
>   	vhost_user.c virtio.c vu_common.c
>   QRAP_SRCS = qrap.c
>   PASST_REPAIR_SRCS = passt-repair.c
> -PESTO_SRCS = pesto.c serialise.c
> +PESTO_SRCS = pesto.c bitmap.c fwd_rule.c inany.c ip.c lineread.c serialise.c
>   SRCS = $(PASST_SRCS) $(QRAP_SRCS) $(PASST_REPAIR_SRCS) $(PESTO_SRCS)
>   
>   MANPAGES = passt.1 pasta.1 pesto.1 qrap.1 passt-repair.1
> @@ -62,6 +62,8 @@ PASST_HEADERS = arch.h arp.h bitmap.h checksum.h common.h conf.h dhcp.h \
>   QRAP_HEADERS = arp.h ip.h passt.h util.h
>   PASST_REPAIR_HEADERS = linux_dep.h
>   PESTO_HEADERS = common.h pesto.h log.h serialise.h

Duplicate PESTO_HEADERS ^ v

> +PESTO_HEADERS = common.h pesto.h bitmap.h fwd_rule.h inany.h ip.h lineread.h \
> +	log.h serialise.h
>   
>   C := \#include <sys/random.h>\nint main(){int a=getrandom(0, 0, 0);}
>   ifeq ($(shell printf "$(C)" | $(CC) -S -xc - -o - >/dev/null 2>&1; echo $$?),0)
> @@ -223,15 +225,22 @@ cppcheck: passt.cppcheck passt-repair.cppcheck pesto.cppcheck qrap.cppcheck
>   	$(CPPCHECK) $(CPPCHECK_FLAGS) $(BASE_CPPFLAGS) $^
>   
>   passt.cppcheck: BASE_CPPFLAGS += -UPESTO
> -passt.cppcheck: CPPCHECK_FLAGS += --suppress=unusedFunction:serialise.c
> +passt.cppcheck: CPPCHECK_FLAGS += \
> +	--suppress=unusedFunction:fwd_rule.c \
> +	--suppress=unusedFunction:serialise.c
>   passt.cppcheck: $(PASST_SRCS) $(PASST_HEADERS) seccomp.h
>   
>   passt-repair.cppcheck: $(PASST_REPAIR_SRCS) $(PASST_REPAIR_HEADERS) seccomp_repair.h
>   
>   pesto.cppcheck: BASE_CPPFLAGS += -DPESTO
>   pesto.cppcheck: CPPCHECK_FLAGS += \
> -	--suppress=unusedFunction:serialise.c \
> -	--suppress=staticFunction:serialise.c
> +	--suppress=unusedFunction:bitmap.c \
> +	--suppress=unusedFunction:inany.h \
> +	--suppress=unusedFunction:inany.c \
> +	--suppress=unusedFunction:ip.h \
> +	--suppress=unusedFunction:fwd_rule.c \
> +	--suppress=staticFunction:fwd_rule.c \
> +	--suppress=unusedFunction:serialise.c
>   pesto.cppcheck: $(PESTO_SRCS) $(PESTO_HEADERS) seccomp_pesto.h
>   
>   qrap.cppcheck: BASE_CPPFLAGS += -DARCH=\"$(TARGET_ARCH)\"
> diff --git a/conf.c b/conf.c
> index 3b2fe42..5e4e81e 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -1939,21 +1939,30 @@ static int conf_send_rules(const struct ctx *c, int fd)
>   	unsigned pif;
>   
>   	for (pif = 0; pif < PIF_NUM_TYPES; pif++) {
> +		struct fwd_table *fwd = c->fwd[pif];
>   		struct pesto_pif_info info;
> +		unsigned i;
>   		int rc;
>   
> -		if (!c->fwd[pif])
> +		if (!fwd)
>   			continue;
>   
>   		assert(pif != PIF_NONE);
>   
>   		rc = snprintf(info.name, sizeof(info.name), "%s", pif_name(pif));
>   		assert(rc >= 0 && (size_t)rc < sizeof(info.name));
> +		info.caps = htonl(fwd->caps);
> +		info.count = htonl(fwd->count);
>   
>   		if (write_u8(fd, pif) < 0)
>   			return -1;
>   		if (write_all_buf(fd, &info, sizeof(info)) < 0)
>   			return -1;
> +
> +		for (i = 0; i < fwd->count; i++) {
> +			if (fwd_rule_write(fd, &fwd->rules[i]))
> +				return -1;
> +		}
>   	}
>   
>   	if (write_u8(fd, PIF_NONE) < 0)
> @@ -2006,6 +2015,7 @@ static void conf_accept(struct ctx *c)
>   		.magic = PESTO_SERVER_MAGIC,
>   		.version = htonl(PESTO_PROTOCOL_VERSION),
>   		.pif_name_size = htonl(PIF_NAME_SIZE),
> +		.ifnamsiz = htonl(IFNAMSIZ),
>   	};
>   	union epoll_ref ref = { .type = EPOLL_TYPE_CONF };
>   	struct ucred uc = { 0 };
> diff --git a/fwd_rule.c b/fwd_rule.c
> index 7fd20dd..da9d893 100644
> --- a/fwd_rule.c
> +++ b/fwd_rule.c
> @@ -24,6 +24,7 @@
>   #include "fwd_rule.h"
>   #include "lineread.h"
>   #include "log.h"
> +#include "serialise.h"
>   
>   /* Ephemeral port range: values from RFC 6335 */
>   static in_port_t fwd_ephemeral_min = (1 << 15) + (1 << 14);
> @@ -645,3 +646,43 @@ void fwd_rule_parse(char optname, const char *optarg, struct fwd_table *fwd)
>   
>   	fwd_rule_parse_ports(fwd, proto, addr, ifname, spec);
>   }
> +
> +
> +/**
> + * fwd_rule_read() - Read serialised rule from an fd
> + * @fd:		fd to serialise to

should be "fd to deserialise from" (or something like that)

> + * @rule:	Buffer to store rule into
> + *
> + * Return: 0 on success, -1 on error (with errno set)
> + */
> +int fwd_rule_read(int fd, struct fwd_rule *rule)
> +{
> +	if (read_all_buf(fd, rule, sizeof(*rule)))
> +		return -1;
> +
> +	/* Byteswap for host */
> +	rule->first = ntohs(rule->first);
> +	rule->last = ntohs(rule->last);
> +	rule->to = ntohs(rule->to);
> +
> +	return 0;
> +}
> +
> +/**
> + * fwd_rule_write() - Serialise rule to an fd
> + * @fd:		fd to serialise to
> + * @rule:	Rule to send
> + *
> + * Return: 0 on success, -1 on error (with errno set)
> + */
> +int fwd_rule_write(int fd, const struct fwd_rule *rule)
> +{
> +	struct fwd_rule tmp = *rule;
> +
> +	/* Byteswap for transport */
> +	tmp.first = htons(tmp.first);
> +	tmp.last = htons(tmp.last);
> +	tmp.to = htons(tmp.to);
> +
> +	return write_all_buf(fd, &tmp, sizeof(tmp));
> +}
> diff --git a/fwd_rule.h b/fwd_rule.h
> index f51f1b4..330d49e 100644
> --- a/fwd_rule.h
> +++ b/fwd_rule.h
> @@ -29,6 +29,8 @@
>   #define FWD_CAP_UDP		BIT(3)
>   #define FWD_CAP_SCAN		BIT(4)
>   #define FWD_CAP_IFNAME		BIT(5)
> +#define FWD_CAP_ALL		(FWD_CAP_IPV4 | FWD_CAP_IPV6 | FWD_CAP_TCP | \
> +				 FWD_CAP_UDP | FWD_CAP_SCAN | FWD_CAP_IFNAME)
>   
>   /**
>    * struct fwd_rule - Forwarding rule governing a range of ports
> @@ -99,6 +101,8 @@ void fwd_probe_ephemeral(void);
>   const union inany_addr *fwd_rule_addr(const struct fwd_rule *rule);
>   const char *fwd_rule_fmt(const struct fwd_rule *rule, char *dst, size_t size);
>   void fwd_rule_parse(char optname, const char *optarg, struct fwd_table *fwd);
> +int fwd_rule_read(int fd, struct fwd_rule *rule);
> +int fwd_rule_write(int fd, const struct fwd_rule *rule);
>   
>   /**
>    * fwd_rules_dump() - Dump forwarding rules
> diff --git a/lineread.c b/lineread.c
> index b9ceae1..a4269a6 100644
> --- a/lineread.c
> +++ b/lineread.c
> @@ -19,8 +19,8 @@
>   #include <stdbool.h>
>   #include <unistd.h>
>   
> +#include "common.h"
>   #include "lineread.h"
> -#include "util.h"
>   
>   /**
>    * lineread_init() - Prepare for line by line file reading without allocation
> diff --git a/pesto.c b/pesto.c
> index 77244b3..4bf9bd8 100644
> --- a/pesto.c
> +++ b/pesto.c
> @@ -34,6 +34,7 @@
>   #include "common.h"
>   #include "seccomp_pesto.h"
>   #include "serialise.h"
> +#include "fwd_rule.h"
>   #include "pesto.h"
>   #include "log.h"
>   
> @@ -66,6 +67,7 @@ static void usage(const char *name, FILE *f, int status)
>   struct pif_configuration {
>   	uint8_t pif;
>   	char name[PIF_NAME_SIZE];
> +	struct fwd_table fwd;
>   };
>   
>   struct configuration {
> @@ -123,6 +125,7 @@ static bool read_pif_conf(int fd, struct configuration *conf)
>   	struct pif_configuration *pc;
>   	struct pesto_pif_info info;
>   	uint8_t pif;
> +	unsigned i;
>   
>   	if (read_u8(fd, &pif) < 0)
>   		die("Error reading from control socket");
> @@ -151,8 +154,17 @@ static bool read_pif_conf(int fd, struct configuration *conf)
>   	static_assert(sizeof(info.name) == sizeof(pc->name),
>   		      "Mismatching pif name lengths");
>   	memcpy(pc->name, info.name, sizeof(pc->name));
> -
> -	debug("PIF %"PRIu8": %s", pc->pif, pc->name);
> +	pc->fwd.caps = ntohl(info.caps);
> +	pc->fwd.count = ntohl(info.count);

We should check that pc->fwd.count <= MAX_FWD_RULES to avoid overflow while scanning the 
array.

> +
> +	debug("PIF %"PRIu8": %s, %"PRIu32" rules, capabilities 0x%"PRIx32
> +	      ":%s%s%s%s%s%s", pc->pif, pc->name, pc->fwd.count, pc->fwd.caps,
> +	      pc->fwd.caps & FWD_CAP_IPV4 ? " IPv4" : "",
> +	      pc->fwd.caps & FWD_CAP_IPV6 ? " IPv6" : "",
> +	      pc->fwd.caps & FWD_CAP_TCP ? " TCP" : "",
> +	      pc->fwd.caps & FWD_CAP_UDP ? " UDP" : "",
> +	      pc->fwd.caps & FWD_CAP_SCAN ? " scan" : "",
> +	      pc->fwd.caps & FWD_CAP_IFNAME ? " ifname" : "");
>   
>   	/* O(n^2), but n is bounded by MAX_PIFS */
>   	if (pif_conf_by_num(conf, pc->pif))
> @@ -162,6 +174,18 @@ static bool read_pif_conf(int fd, struct configuration *conf)
>   	if (pif_conf_by_name(conf, pc->name))
>   		die("Received duplicate interface name");
>   
> +	/* NOTE: We read the fwd rules directly into fwd.rules, rather than
> +	 * using fwd_rule_add().  This means we can read and display rules even
> +	 * if something has gone wrong (in pesto or passt) and we get rules that
> +	 * fwd_rule_add() would reject.  It does have the side effect that we
> +	 * never assign socket space for the fwd rules, but we don't need that
> +	 * within pesto.
> +	 */
> +	for (i = 0; i < pc->fwd.count; i++) {
> +		if (fwd_rule_read(fd, &pc->fwd.rules[i]) < 0)
> +			die("Error reading from control socket");
> +	}
> +
>   	conf->npifs++;
>   	return true;
>   }
> @@ -177,7 +201,8 @@ static void show_conf(const struct configuration *conf)
>   	for (i = 0; i < conf->npifs; i++) {
>   		const struct pif_configuration *pc = &conf->pif[i];
>   		printf("  %s\n", pc->name);
> -		printf("    TBD\n");
> +		fwd_rules_dump(printf, pc->fwd.rules, pc->fwd.count,
> +			       "    ", "\n");
>   	}
>   }
>   
> @@ -290,6 +315,12 @@ int main(int argc, char **argv)
>   		    ntohl(hello.pif_name_size), PIF_NAME_SIZE);
>   	}
>   
> +	if (ntohl(hello.ifnamsiz) != IFNAMSIZ) {
> +		die("Server has unexpected IFNAMSIZ (%"
> +		    PRIu32" not %"PRIu32"\n",

trailing '\n'

> +		    ntohl(hello.ifnamsiz), IFNAMSIZ);
> +	}
> +	

Trailing tab :)

>   	while (read_pif_conf(s, &conf))
>   		;
>   
> diff --git a/pesto.h b/pesto.h
> index 1879759..12b0b65 100644
> --- a/pesto.h
> +++ b/pesto.h
> @@ -26,11 +26,13 @@
>    * @magic:		PESTO_SERVER_MAGIC
>    * @version:		Version number
>    * @pif_name_size:	Server's value for PIF_NAME_SIZE
> + * @ifnamsiz:		Server's value for IFNAMSIZ
>    */
>   struct pesto_hello {
>   	char magic[8];
>   	uint32_t version;
>   	uint32_t pif_name_size;
> +	uint32_t ifnamsiz;
>   } __attribute__ ((__packed__));
>   
>   static_assert(sizeof(PESTO_SERVER_MAGIC)
> @@ -40,9 +42,13 @@ static_assert(sizeof(PESTO_SERVER_MAGIC)
>   /**
>    * struct pesto_pif_info - Message with basic metadata about a pif
>    * @name:	Name (\0 terminated)
> + * @caps:	Forwarding capabilities for this pif
> + * @count:	Number of forwarding rules for this pif
>    */
>   struct pesto_pif_info {
>   	char name[PIF_NAME_SIZE];
> +	uint32_t caps;
> +	uint32_t count;
>   } __attribute__ ((__packed__));
>   
>   #endif /* PESTO_H */