* [PATCH v5] dhcp, dhcpv6: Add hostname and client fqdn ops
@ 2024-12-19 11:55 Enrique Llorente
2025-01-01 21:54 ` Stefano Brivio
0 siblings, 1 reply; 4+ messages in thread
From: Enrique Llorente @ 2024-12-19 11:55 UTC (permalink / raw)
To: passt-dev; +Cc: Enrique Llorente
Both DHCPv4 and DHCPv6 has the capability to pass the hostname to
clients, the DHCPv4 uses option 12 (hostname) while the DHCPv6 uses option 39
(client fqdn), for some virt deployments like kubevirt is expected to
have the VirtualMachine name as the guest hostname.
This change add the following arguments:
- -H --hostname NAME to configure the hostname DHCPv4 option(12)
- --fqdn NAME to configure client fqdn option for both DHCPv4(81) and
DHCPv6(39)
Signed-off-by: Enrique Llorente <ellorent@redhat.com>
---
conf.c | 20 +++++++++++--
dhcp.c | 50 ++++++++++++++++++++++++++++----
dhcpv6.c | 75 +++++++++++++++++++++++++++++++++++++++---------
passt.1 | 11 +++++++
passt.h | 5 ++++
pasta.c | 18 ++++++++----
test/lib/setup | 10 +++----
test/passt.mbuto | 6 ++--
test/passt/dhcp | 15 +++++++++-
util.c | 23 +++++++++++++++
util.h | 6 ++++
11 files changed, 204 insertions(+), 35 deletions(-)
diff --git a/conf.c b/conf.c
index df2b016..5f21193 100644
--- a/conf.c
+++ b/conf.c
@@ -854,7 +854,9 @@ static void usage(const char *name, FILE *f, int status)
FPRINTF(f, " default: use addresses from /etc/resolv.conf\n");
FPRINTF(f,
" -S, --search LIST Space-separated list, search domains\n"
- " a single, empty option disables the DNS search list\n");
+ " a single, empty option disables the DNS search list\n"
+ " -H, --hostname NAME Hostname to configure client with\n"
+ " --fqdn NAME FQDN to configure client with\n");
if (strstr(name, "pasta"))
FPRINTF(f, " default: don't use any search list\n");
else
@@ -1313,6 +1315,7 @@ void conf(struct ctx *c, int argc, char **argv)
{"outbound", required_argument, NULL, 'o' },
{"dns", required_argument, NULL, 'D' },
{"search", required_argument, NULL, 'S' },
+ {"hostname", required_argument, NULL, 'H' },
{"no-tcp", no_argument, &c->no_tcp, 1 },
{"no-udp", no_argument, &c->no_udp, 1 },
{"no-icmp", no_argument, &c->no_icmp, 1 },
@@ -1357,6 +1360,7 @@ void conf(struct ctx *c, int argc, char **argv)
/* vhost-user backend program convention */
{"print-capabilities", no_argument, NULL, 26 },
{"socket-path", required_argument, NULL, 's' },
+ {"fqdn", required_argument, NULL, 27 },
{ 0 },
};
const char *logname = (c->mode == MODE_PASTA) ? "pasta" : "passt";
@@ -1379,9 +1383,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;
fwd_default = FWD_AUTO;
- optstring = "+dqfel:hF:I:p:P:m:a:n:M:g:i:o:D:S:46t:u:T:U:";
+ optstring = "+dqfel:hF:I:p:P:m:a:n:M:g:i:o:D:S:H:46t:u:T:U:";
} else {
- optstring = "+dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:461t:u:";
+ optstring = "+dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:H:461t:u:";
}
c->tcp.fwd_in.mode = c->tcp.fwd_out.mode = FWD_UNSET;
@@ -1558,6 +1562,11 @@ void conf(struct ctx *c, int argc, char **argv)
case 26:
vu_print_capabilities();
break;
+ case 27:
+ if (snprintf_check(c->fqdn, PASST_MAXDNAME,
+ "%s", optarg))
+ die("Invalid FQDN: %s", optarg);
+ break;
case 'd':
c->debug = 1;
c->quiet = 0;
@@ -1727,6 +1736,11 @@ void conf(struct ctx *c, int argc, char **argv)
die("Cannot use DNS search domain %s", optarg);
break;
+ case 'H':
+ if (snprintf_check(c->hostname, PASST_MAXDNAME,
+ "%s", optarg))
+ die("Invalid hostname: %s", optarg);
+ break;
case '4':
v4_only = true;
v6_only = false;
diff --git a/dhcp.c b/dhcp.c
index d8515aa..b224bf8 100644
--- a/dhcp.c
+++ b/dhcp.c
@@ -63,6 +63,12 @@ static struct opt opts[255];
#define OPT_MIN 60 /* RFC 951 */
+/* 576 (RFC 2131), minus offset
+ * of options (268), minus end
+ * option and its length (2)
+ */
+#define OPT_MAX 306
+
/**
* dhcp_init() - Initialise DHCP options
*/
@@ -122,7 +128,7 @@ struct msg {
uint8_t sname[64];
uint8_t file[128];
uint32_t magic;
- uint8_t o[308];
+ uint8_t o[OPT_MAX + 2]; /* End option and length */
} __attribute__((__packed__));
/**
@@ -130,15 +136,31 @@ struct msg {
* @m: Message to fill
* @o: Option number
* @offset: Current offset within options field, updated on insertion
+ *
+ * Return: offset for the next option field
*/
-static void fill_one(struct msg *m, int o, int *offset)
+static int fill_one(struct msg *m, int o, int *offset)
{
+ size_t idx, slen = 0;
+
+ /* If it cannot write even enum + len + one byte, then just skip */
+ if (*offset + 2 > OPT_MAX)
+ return OPT_MAX;
+
m->o[*offset] = o;
m->o[*offset + 1] = opts[o].slen;
- memcpy(&m->o[*offset + 2], opts[o].s, opts[o].slen);
+ idx = *offset + 2;
+ slen = opts[o].slen;
+
+ /* Truncate if it goes beyond OPT_MAX */
+ if (idx + slen > OPT_MAX)
+ slen = OPT_MAX - idx;
+
+ memcpy(&m->o[*offset + 2], opts[o].s, slen);
opts[o].sent = 1;
*offset += 2 + opts[o].slen;
+ return *offset;
}
/**
@@ -172,7 +194,10 @@ static int fill(struct msg *m)
for (o = 0; o < 255; o++) {
if (opts[o].slen != -1 && !opts[o].sent)
- fill_one(m, o, &offset);
+ if (fill_one(m, o, &offset) == OPT_MAX) {
+ debug("DHCP: truncating after option %i", o);
+ break;
+ }
}
m->o[offset++] = 255;
@@ -285,7 +310,7 @@ static void opt_set_dns_search(const struct ctx *c, size_t max_len)
*/
int dhcp(const struct ctx *c, const struct pool *p)
{
- size_t mlen, dlen, offset = 0, opt_len, opt_off = 0;
+ size_t mlen, dlen, offset = 0, opt_len, opt_off = 0, hostname_len = 0, fqdn_len = 0;
char macstr[ETH_ADDRSTRLEN];
struct in_addr mask, dst;
const struct ethhdr *eh;
@@ -398,6 +423,21 @@ int dhcp(const struct ctx *c, const struct pool *p)
if (!opts[6].slen)
opts[6].slen = -1;
+ hostname_len = strlen(c->hostname);
+ if (hostname_len > 0) {
+ opts[12].slen = hostname_len;
+ memcpy(opts[12].s, &c->hostname, hostname_len);
+ }
+
+ fqdn_len = strlen(c->fqdn);
+ if (fqdn_len > 0) {
+ size_t encoded_len = 0;
+ opts[81].s[0] = 0x4; /* flags (E) */
+ encoded_len = encode_domain_name(c->fqdn, fqdn_len,
+ (char *) opts[81].s + 3);
+ opts[81].slen = encoded_len + 3;
+ }
+
if (!c->no_dhcp_dns_search)
opt_set_dns_search(c, sizeof(m->o));
diff --git a/dhcpv6.c b/dhcpv6.c
index 0523bba..ce3a1bd 100644
--- a/dhcpv6.c
+++ b/dhcpv6.c
@@ -48,6 +48,7 @@ struct opt_hdr {
# define STATUS_NOTONLINK htons_constant(4)
# define OPT_DNS_SERVERS htons_constant(23)
# define OPT_DNS_SEARCH htons_constant(24)
+# define OPT_CLIENT_FQDN htons_constant(39)
#define STR_NOTONLINK "Prefix not appropriate for link."
uint16_t l;
@@ -58,6 +59,9 @@ struct opt_hdr {
sizeof(struct opt_hdr))
#define OPT_VSIZE(x) (sizeof(struct opt_##x) - \
sizeof(struct opt_hdr))
+#define OPT_MAX_SIZE IPV6_MIN_MTU - (sizeof(struct ipv6hdr) + \
+ sizeof(struct udphdr) + \
+ sizeof(struct msg_hdr))
/**
* struct opt_client_id - DHCPv6 Client Identifier option
@@ -163,6 +167,18 @@ struct opt_dns_search {
char list[MAXDNSRCH * NS_MAXDNAME];
} __attribute__((packed));
+/**
+ * struct opt_client_fqdn - Client FQDN option (RFC 4704)
+ * @hdr: Option header
+ * @flags: Flags described by RFC 4704 (always zero for us)
+ * @domain_name: Client FQDN
+ */
+struct opt_client_fqdn{
+ struct opt_hdr hdr;
+ uint8_t flags;
+ char domain_name[PASST_MAXDNAME];
+} __attribute__((packed));
+
/**
* struct msg_hdr - DHCPv6 client/server message header
* @type: DHCP message type
@@ -193,6 +209,7 @@ struct msg_hdr {
* @client_id: Client Identifier, variable length
* @dns_servers: DNS Recursive Name Server, here just for storage size
* @dns_search: Domain Search List, here just for storage size
+ * @client_fqdn: Client FQDN, variable length
*/
static struct resp_t {
struct msg_hdr hdr;
@@ -203,6 +220,7 @@ static struct resp_t {
struct opt_client_id client_id;
struct opt_dns_servers dns_servers;
struct opt_dns_search dns_search;
+ struct opt_client_fqdn client_fqdn;
} __attribute__((__packed__)) resp = {
{ 0 },
SERVER_ID,
@@ -228,6 +246,10 @@ static struct resp_t {
{ { OPT_DNS_SEARCH, 0, },
{ 0 },
},
+
+ { { OPT_CLIENT_FQDN, 0, },
+ 0, { 0 },
+ },
};
static const struct opt_status_code sc_not_on_link = {
@@ -346,7 +368,6 @@ static size_t dhcpv6_dns_fill(const struct ctx *c, char *buf, int offset)
{
struct opt_dns_servers *srv = NULL;
struct opt_dns_search *srch = NULL;
- char *p = NULL;
int i;
if (c->no_dhcp_dns)
@@ -373,6 +394,7 @@ search:
return offset;
for (i = 0; *c->dns_search[i].n; i++) {
+ size_t encoded_name_len = 0;
size_t name_len = strlen(c->dns_search[i].n);
/* We already append separators, don't duplicate if present */
@@ -388,29 +410,53 @@ search:
offset += sizeof(struct opt_hdr);
srch->hdr.t = OPT_DNS_SEARCH;
srch->hdr.l = 0;
- p = srch->list;
}
-
- *p = '.';
- p = stpncpy(p + 1, c->dns_search[i].n, name_len);
- p++;
- srch->hdr.l += name_len + 2;
- offset += name_len + 2;
+
+ encoded_name_len = encode_domain_name(c->dns_search[i].n,
+ name_len, srch->list);
+ srch->hdr.l += encoded_name_len;
+ offset += encoded_name_len;
}
if (srch) {
- for (i = 0; i < srch->hdr.l; i++) {
- if (srch->list[i] == '.') {
- srch->list[i] = strcspn(srch->list + i + 1,
- ".");
- }
- }
srch->hdr.l = htons(srch->hdr.l);
}
return offset;
}
+/**
+ * dhcpv6_client_fqdn_fill() - Fill in client FQDN option
+ * @c: Execution context
+ * @buf: Response message buffer where options will be appended
+ * @offset: Offset in message buffer for new options
+ *
+ * Return: updated length of response message buffer.
+ */
+static size_t dhcpv6_client_fqdn_fill(const struct ctx *c, char *buf, int offset)
+{
+ size_t fqdn_len, opt_hdr_len, opt_len, encoded_fqdn_len;
+ struct opt_client_fqdn *o;
+
+ opt_hdr_len = sizeof(struct opt_hdr);
+
+ fqdn_len = MIN(strlen(c->fqdn), OPT_MAX_SIZE - (offset + opt_hdr_len + 1));
+
+ if (fqdn_len == 0)
+ return offset;
+
+ o = (struct opt_client_fqdn *)(buf + offset);
+ encoded_fqdn_len = encode_domain_name(c->fqdn, fqdn_len,
+ o->domain_name);
+ opt_len = encoded_fqdn_len + 1;
+
+ o->hdr.t = OPT_CLIENT_FQDN;
+ o->hdr.l = htons(opt_len);
+ o->flags = 0x00;
+
+ return offset + opt_hdr_len + opt_len;
+}
+
/**
* dhcpv6() - Check if this is a DHCPv6 message, reply as needed
* @c: Execution context
@@ -544,6 +590,7 @@ int dhcpv6(struct ctx *c, const struct pool *p,
n = offsetof(struct resp_t, client_id) +
sizeof(struct opt_hdr) + ntohs(client_id->l);
n = dhcpv6_dns_fill(c, (char *)&resp, n);
+ n = dhcpv6_client_fqdn_fill(c, (char *)&resp, n);
resp.hdr.xid = mh->xid;
diff --git a/passt.1 b/passt.1
index d9cd33e..8f6b194 100644
--- a/passt.1
+++ b/passt.1
@@ -401,6 +401,17 @@ Enable IPv6-only operation. IPv4 traffic will be ignored.
By default, IPv4 operation is enabled as long as at least an IPv4 route and an
interface address are configured on a given host interface.
+.TP
+.BR \-H ", " \-\-hostname " " \fIname
+Hostname to configure client with.
+Send \fIname as DHCP option 12 (hostname).
+
+.TP
+.BR \-\-fqdn " " \fIname
+FQDN to configure client with.
+Send \fIname as dhcp client fqdn option, for DHCP option 81 and for
+DHCPv6 option 39.
+
.SS \fBpasst\fR-only options
.TP
diff --git a/passt.h b/passt.h
index 0dd4efa..9909a10 100644
--- a/passt.h
+++ b/passt.h
@@ -209,6 +209,8 @@ struct ip6_ctx {
* @ifi4: Template interface for IPv4, -1: none, 0: IPv4 disabled
* @ip: IPv4 configuration
* @dns_search: DNS search list
+ * @hostname: Guest hostname
+ * @fqdn: Guest FQDN
* @ifi6: Template interface for IPv6, -1: none, 0: IPv6 disabled
* @ip6: IPv6 configuration
* @pasta_ifn: Name of namespace interface for pasta
@@ -268,6 +270,9 @@ struct ctx {
struct ip4_ctx ip4;
struct fqdn dns_search[MAXDNSRCH];
+
+ char hostname[PASST_MAXDNAME];
+ char fqdn[PASST_MAXDNAME];
int ifi6;
struct ip6_ctx ip6;
diff --git a/pasta.c b/pasta.c
index ff41c95..00678f3 100644
--- a/pasta.c
+++ b/pasta.c
@@ -173,6 +173,7 @@ void pasta_open_ns(struct ctx *c, const char *netns)
struct pasta_spawn_cmd_arg {
const char *exe;
char *const *argv;
+ struct ctx *c;
};
/**
@@ -186,6 +187,7 @@ static int pasta_spawn_cmd(void *arg)
{
char hostname[HOST_NAME_MAX + 1] = HOSTNAME_PREFIX;
const struct pasta_spawn_cmd_arg *a;
+ size_t conf_hostname_len;
sigset_t set;
/* We run in a detached PID and mount namespace: mount /proc over */
@@ -194,10 +196,16 @@ static int pasta_spawn_cmd(void *arg)
if (write_file("/proc/sys/net/ipv4/ping_group_range", "0 0"))
warn("Cannot set ping_group_range, ICMP requests might fail");
-
- if (!gethostname(hostname + sizeof(HOSTNAME_PREFIX) - 1,
- HOST_NAME_MAX + 1 - sizeof(HOSTNAME_PREFIX)) ||
- errno == ENAMETOOLONG) {
+
+ a = (const struct pasta_spawn_cmd_arg *)arg;
+
+ conf_hostname_len = strlen(a->c->hostname);
+ if (conf_hostname_len > 0) {
+ if (sethostname(a->c->hostname, conf_hostname_len))
+ warn("Unable to set configured hostname");
+ }else if (!gethostname(hostname + sizeof(HOSTNAME_PREFIX) - 1,
+ HOST_NAME_MAX + 1 - sizeof(HOSTNAME_PREFIX)) ||
+ errno == ENAMETOOLONG) {
hostname[HOST_NAME_MAX] = '\0';
if (sethostname(hostname, strlen(hostname)))
warn("Unable to set pasta-prefixed hostname");
@@ -208,7 +216,6 @@ static int pasta_spawn_cmd(void *arg)
sigaddset(&set, SIGUSR1);
sigwaitinfo(&set, NULL);
- a = (const struct pasta_spawn_cmd_arg *)arg;
execvp(a->exe, a->argv);
die_perror("Failed to start command or shell");
@@ -230,6 +237,7 @@ void pasta_start_ns(struct ctx *c, uid_t uid, gid_t gid,
struct pasta_spawn_cmd_arg arg = {
.exe = argv[0],
.argv = argv,
+ .c = c,
};
char uidmap[BUFSIZ], gidmap[BUFSIZ];
char *sh_argv[] = { NULL, NULL };
diff --git a/test/lib/setup b/test/lib/setup
index 580825f..ee67152 100755
--- a/test/lib/setup
+++ b/test/lib/setup
@@ -49,7 +49,7 @@ setup_passt() {
context_run passt "make clean"
context_run passt "make valgrind"
- context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt ${__opts} -s ${STATESETUP}/passt.socket -f -t 10001 -u 10001 -P ${STATESETUP}/passt.pid"
+ context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt ${__opts} -s ${STATESETUP}/passt.socket -f -t 10001 -u 10001 -H hostname1 --fqdn fqdn1.passt.test -P ${STATESETUP}/passt.pid"
# pidfile isn't created until passt is listening
wait_for [ -f "${STATESETUP}/passt.pid" ]
@@ -160,11 +160,11 @@ setup_passt_in_ns() {
if [ ${VALGRIND} -eq 1 ]; then
context_run passt "make clean"
context_run passt "make valgrind"
- context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt -f ${__opts} -s ${STATESETUP}/passt.socket -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}"
+ context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt -f ${__opts} -s ${STATESETUP}/passt.socket -H hostname1 --fqdn fqdn1.passt.test -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}"
else
context_run passt "make clean"
context_run passt "make"
- context_run_bg passt "./passt -f ${__opts} -s ${STATESETUP}/passt.socket -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}"
+ context_run_bg passt "./passt -f ${__opts} -s ${STATESETUP}/passt.socket -H hostname1 --fqdn fqdn1.passt.test -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}"
fi
wait_for [ -f "${STATESETUP}/passt.pid" ]
@@ -243,7 +243,7 @@ setup_two_guests() {
[ ${TRACE} -eq 1 ] && __opts="${__opts} --trace"
[ ${VHOST_USER} -eq 1 ] && __opts="${__opts} --vhost-user"
- context_run_bg passt_1 "./passt -s ${STATESETUP}/passt_1.socket -P ${STATESETUP}/passt_1.pid -f ${__opts} -t 10001 -u 10001"
+ context_run_bg passt_1 "./passt -s ${STATESETUP}/passt_1.socket -P ${STATESETUP}/passt_1.pid -f ${__opts} --fqdn fqdn1.passt.test -H hostname1 -t 10001 -u 10001"
wait_for [ -f "${STATESETUP}/passt_1.pid" ]
__opts=
@@ -252,7 +252,7 @@ setup_two_guests() {
[ ${TRACE} -eq 1 ] && __opts="${__opts} --trace"
[ ${VHOST_USER} -eq 1 ] && __opts="${__opts} --vhost-user"
- context_run_bg passt_2 "./passt -s ${STATESETUP}/passt_2.socket -P ${STATESETUP}/passt_2.pid -f ${__opts} -t 10004 -u 10004"
+ context_run_bg passt_2 "./passt -s ${STATESETUP}/passt_2.socket -P ${STATESETUP}/passt_2.pid -f ${__opts} --hostname hostname2 --fqdn fqdn2 -t 10004 -u 10004"
wait_for [ -f "${STATESETUP}/passt_2.pid" ]
__vmem="$((${MEM_KIB} / 1024 / 4))"
diff --git a/test/passt.mbuto b/test/passt.mbuto
index 138d365..1e07693 100755
--- a/test/passt.mbuto
+++ b/test/passt.mbuto
@@ -13,7 +13,7 @@
PROGS="${PROGS:-ash,dash,bash ip mount ls insmod mkdir ln cat chmod lsmod
modprobe find grep mknod mv rm umount jq iperf3 dhclient hostname
sed tr chown sipcalc cut socat dd strace ping tail killall sleep sysctl
- nproc tcp_rr tcp_crr udp_rr which tee seq bc sshd ssh-keygen cmp}"
+ nproc tcp_rr tcp_crr udp_rr which tee seq bc sshd ssh-keygen cmp env}"
# OpenSSH 9.8 introduced split binaries, with sshd being the daemon, and
# sshd-session the per-session program. We need the latter as well, and the path
@@ -41,6 +41,7 @@ FIXUP="${FIXUP}"'
#!/bin/sh
LOG=/var/log/dhclient-script.log
echo \${reason} \${interface} >> \$LOG
+env >> \$LOG
set >> \$LOG
[ -n "\${new_interface_mtu}" ] && ip link set dev \${interface} mtu \${new_interface_mtu}
@@ -54,7 +55,8 @@ set >> \$LOG
[ -n "\${new_ip6_address}" ] && ip addr add \${new_ip6_address}/\${new_ip6_prefixlen} dev \${interface}
[ -n "\${new_dhcp6_name_servers}" ] && for d in \${new_dhcp6_name_servers}; do echo "nameserver \${d}%\${interface}" >> /etc/resolv.conf; done
[ -n "\${new_dhcp6_domain_search}" ] && (printf "search"; for d in \${new_dhcp6_domain_search}; do printf " %s" "\${d}"; done; printf "\n") >> /etc/resolv.conf
-[ -n "\${new_host_name}" ] && hostname "\${new_host_name}"
+[ -n "\${new_host_name}" ] && echo "\${new_host_name}" > /tmp/new_host_name
+[ -n "\${new_fqdn_fqdn}" ] && echo "\${new_fqdn_fqdn}" > /tmp/new_fqdn_fqdn
exit 0
EOF
chmod 755 /sbin/dhclient-script
diff --git a/test/passt/dhcp b/test/passt/dhcp
index 9925ab9..145f1ba 100644
--- a/test/passt/dhcp
+++ b/test/passt/dhcp
@@ -11,7 +11,7 @@
# Copyright (c) 2021 Red Hat GmbH
# Author: Stefano Brivio <sbrivio@redhat.com>
-gtools ip jq dhclient sed tr
+gtools ip jq dhclient sed tr hostname
htools ip jq sed tr head
test Interface name
@@ -47,7 +47,16 @@ gout SEARCH sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^searc
hout HOST_SEARCH sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^search \(.*\)/\1/p' | tr ' \n' ',' | sed 's/,$//;s/$/\n/'
check [ "__SEARCH__" = "__HOST_SEARCH__" ]
+test DHCP: Hostname
+gout NEW_HOST_NAME cat /tmp/new_host_name
+check [ "__NEW_HOST_NAME__" = "hostname1" ]
+
+test DHCP: Client FQDN
+gout NEW_FQDN_FQDN cat /tmp/new_fqdn_fqdn
+check [ "__NEW_FQDN_FQDN__" = "fqdn1.passt.test" ]
+
test DHCPv6: address
+guest rm /tmp/new_fqdn_fqdn
guest /sbin/dhclient -6 __IFNAME__
# Wait for DAD to complete
guest while ip -j -6 addr show tentative | jq -e '.[].addr_info'; do sleep 0.1; done
@@ -70,3 +79,7 @@ test DHCPv6: search list
gout SEARCH6 sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^search \(.*\)/\1/p' | tr ' \n' ',' | sed 's/,$//;s/$/\n/'
hout HOST_SEARCH6 sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^search \(.*\)/\1/p' | tr ' \n' ',' | sed 's/,$//;s/$/\n/'
check [ "__SEARCH6__" = "__HOST_SEARCH6__" ]
+
+test DHCPv6: Hostname
+gout NEW_FQDN_FQDN cat /tmp/new_fqdn_fqdn
+check [ "__NEW_FQDN_FQDN__" = "fqdn1.passt.test" ]
diff --git a/util.c b/util.c
index 11973c4..7aeb5b4 100644
--- a/util.c
+++ b/util.c
@@ -837,3 +837,26 @@ void raw_random(void *buf, size_t buflen)
if (random_read < buflen)
die("Unexpected EOF on random data source");
}
+/**
+ * encode_domain_name() - Encode domain name according to RFC 1035, section 3.1
+ * @domain_name: Input domain name to encode
+ * @len: Domain name length
+ * @buf: Buffer to fill in with encoded domain name
+ *
+ * Return: encoded domain name length
+ */
+size_t encode_domain_name(const char *domain_name, size_t len, char *buf)
+{
+ char *p;
+ size_t i;
+
+ buf[0] = strcspn(domain_name, ".");
+ p = buf + 1;
+ for (i = 0; i < len; i++) {
+ if (domain_name[i] == '.')
+ p[i] = strcspn(domain_name + i + 1, ".");
+ else
+ p[i] = domain_name[i];
+ }
+ return len + 2;
+}
diff --git a/util.h b/util.h
index 3fa1d12..c55ef29 100644
--- a/util.h
+++ b/util.h
@@ -40,6 +40,9 @@
#ifndef IP_MAX_MTU
#define IP_MAX_MTU USHRT_MAX
#endif
+#ifndef IPV6_MIN_MTU
+#define IPV6_MIN_MTU 1280
+#endif
#ifndef MIN
#define MIN(x, y) (((x) < (y)) ? (x) : (y))
@@ -346,4 +349,7 @@ static inline int wrap_accept4(int sockfd, struct sockaddr *addr,
#define accept4(s, addr, addrlen, flags) \
wrap_accept4((s), (addr), (addrlen), (flags))
+#define PASST_MAXDNAME 253 /* RFC 1035 */
+size_t encode_domain_name(const char* domain_name, size_t len, char* buf);
+
#endif /* UTIL_H */
--
@@ -40,6 +40,9 @@
#ifndef IP_MAX_MTU
#define IP_MAX_MTU USHRT_MAX
#endif
+#ifndef IPV6_MIN_MTU
+#define IPV6_MIN_MTU 1280
+#endif
#ifndef MIN
#define MIN(x, y) (((x) < (y)) ? (x) : (y))
@@ -346,4 +349,7 @@ static inline int wrap_accept4(int sockfd, struct sockaddr *addr,
#define accept4(s, addr, addrlen, flags) \
wrap_accept4((s), (addr), (addrlen), (flags))
+#define PASST_MAXDNAME 253 /* RFC 1035 */
+size_t encode_domain_name(const char* domain_name, size_t len, char* buf);
+
#endif /* UTIL_H */
--
2.47.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v5] dhcp, dhcpv6: Add hostname and client fqdn ops
2024-12-19 11:55 [PATCH v5] dhcp, dhcpv6: Add hostname and client fqdn ops Enrique Llorente
@ 2025-01-01 21:54 ` Stefano Brivio
2025-01-02 16:09 ` Enrique Llorente Pastora
0 siblings, 1 reply; 4+ messages in thread
From: Stefano Brivio @ 2025-01-01 21:54 UTC (permalink / raw)
To: Enrique Llorente; +Cc: passt-dev
On Thu, 19 Dec 2024 12:55:57 +0100
Enrique Llorente <ellorent@redhat.com> wrote:
> Both DHCPv4 and DHCPv6 has the capability to pass the hostname to
> clients, the DHCPv4 uses option 12 (hostname) while the DHCPv6 uses option 39
> (client fqdn), for some virt deployments like kubevirt is expected to
> have the VirtualMachine name as the guest hostname.
>
> This change add the following arguments:
> - -H --hostname NAME to configure the hostname DHCPv4 option(12)
> - --fqdn NAME to configure client fqdn option for both DHCPv4(81) and
> DHCPv6(39)
>
> Signed-off-by: Enrique Llorente <ellorent@redhat.com>
> ---
> conf.c | 20 +++++++++++--
> dhcp.c | 50 ++++++++++++++++++++++++++++----
> dhcpv6.c | 75 +++++++++++++++++++++++++++++++++++++++---------
> passt.1 | 11 +++++++
> passt.h | 5 ++++
> pasta.c | 18 ++++++++----
> test/lib/setup | 10 +++----
> test/passt.mbuto | 6 ++--
> test/passt/dhcp | 15 +++++++++-
> util.c | 23 +++++++++++++++
> util.h | 6 ++++
> 11 files changed, 204 insertions(+), 35 deletions(-)
>
> diff --git a/conf.c b/conf.c
> index df2b016..5f21193 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -854,7 +854,9 @@ static void usage(const char *name, FILE *f, int status)
> FPRINTF(f, " default: use addresses from /etc/resolv.conf\n");
> FPRINTF(f,
> " -S, --search LIST Space-separated list, search domains\n"
> - " a single, empty option disables the DNS search list\n");
> + " a single, empty option disables the DNS search list\n"
> + " -H, --hostname NAME Hostname to configure client with\n"
> + " --fqdn NAME FQDN to configure client with\n");
> if (strstr(name, "pasta"))
> FPRINTF(f, " default: don't use any search list\n");
> else
> @@ -1313,6 +1315,7 @@ void conf(struct ctx *c, int argc, char **argv)
> {"outbound", required_argument, NULL, 'o' },
> {"dns", required_argument, NULL, 'D' },
> {"search", required_argument, NULL, 'S' },
> + {"hostname", required_argument, NULL, 'H' },
> {"no-tcp", no_argument, &c->no_tcp, 1 },
> {"no-udp", no_argument, &c->no_udp, 1 },
> {"no-icmp", no_argument, &c->no_icmp, 1 },
> @@ -1357,6 +1360,7 @@ void conf(struct ctx *c, int argc, char **argv)
> /* vhost-user backend program convention */
> {"print-capabilities", no_argument, NULL, 26 },
> {"socket-path", required_argument, NULL, 's' },
> + {"fqdn", required_argument, NULL, 27 },
> { 0 },
> };
> const char *logname = (c->mode == MODE_PASTA) ? "pasta" : "passt";
> @@ -1379,9 +1383,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;
> fwd_default = FWD_AUTO;
> - optstring = "+dqfel:hF:I:p:P:m:a:n:M:g:i:o:D:S:46t:u:T:U:";
> + optstring = "+dqfel:hF:I:p:P:m:a:n:M:g:i:o:D:S:H:46t:u:T:U:";
> } else {
> - optstring = "+dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:461t:u:";
> + optstring = "+dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:H:461t:u:";
> }
>
> c->tcp.fwd_in.mode = c->tcp.fwd_out.mode = FWD_UNSET;
> @@ -1558,6 +1562,11 @@ void conf(struct ctx *c, int argc, char **argv)
> case 26:
> vu_print_capabilities();
> break;
> + case 27:
> + if (snprintf_check(c->fqdn, PASST_MAXDNAME,
> + "%s", optarg))
Coding style: arguments are preferably aligned with parameter list they
belong to:
if (snprintf_check(c->fqdn, PASST_MAXDNAME,
"%s", optarg))
for another example, look just 20 lines below this (case 's').
> + die("Invalid FQDN: %s", optarg);
> + break;
> case 'd':
> c->debug = 1;
> c->quiet = 0;
> @@ -1727,6 +1736,11 @@ void conf(struct ctx *c, int argc, char **argv)
>
> die("Cannot use DNS search domain %s", optarg);
> break;
> + case 'H':
> + if (snprintf_check(c->hostname, PASST_MAXDNAME,
> + "%s", optarg))
Same as above.
> + die("Invalid hostname: %s", optarg);
> + break;
> case '4':
> v4_only = true;
> v6_only = false;
> diff --git a/dhcp.c b/dhcp.c
> index d8515aa..b224bf8 100644
> --- a/dhcp.c
> +++ b/dhcp.c
> @@ -63,6 +63,12 @@ static struct opt opts[255];
>
> #define OPT_MIN 60 /* RFC 951 */
>
> +/* 576 (RFC 2131), minus offset
> + * of options (268), minus end
> + * option and its length (2)
> + */
I formatted this multi-comment in my reply to v4 so that the first
line would fit with the define:
#define OPT_MAX 306 /* 576 (RFC 2131), minus offset ...
but if you don't want to have it on the same line (no preference from my
side), then there's no need for all that line wrapping (but then you
should mention what it refers to), say:
/* Total option size (excluding end option) is 576 (RFC 2131), minus
* offset of options (268), minus end option and its length (2).
*/
#define OPT_MAX 306
> +#define OPT_MAX 306
> +
> /**
> * dhcp_init() - Initialise DHCP options
> */
> @@ -122,7 +128,7 @@ struct msg {
> uint8_t sname[64];
> uint8_t file[128];
> uint32_t magic;
> - uint8_t o[308];
> + uint8_t o[OPT_MAX + 2]; /* End option and length */
..."plus end option and length", or move the comment so that it refers
to the "2":
uint8_t o[OPT_MAX + 2 /* End option and its length */ ];
Otherwise it looks like we use 310 bytes for the end option and its
length.
> } __attribute__((__packed__));
>
> /**
> @@ -130,15 +136,31 @@ struct msg {
> * @m: Message to fill
> * @o: Option number
> * @offset: Current offset within options field, updated on insertion
> + *
> + * Return: offset for the next option field
> */
> -static void fill_one(struct msg *m, int o, int *offset)
> +static int fill_one(struct msg *m, int o, int *offset)
> {
> + size_t idx, slen = 0;
> +
> + /* If it cannot write even enum + len + one byte, then just skip */
..."it" who? If we can't write, then skip.
It's not clear what "enum" refers to, I guess that should be "number"
or "code".
By the way, I just realised that this is not entirely correct because
there's at least one option (option 80, "Rapid Commit") that has length
zero (it's some sort of flag, with the presence of the option
indicating that the feature is enabled).
Maybe there's a more elegant solution to this, but a convenient
alternative might be to check *offset + 1 /* length */ + !!slen instead.
That's with slen assigned earlier, there's no point in initialising it
to zero and then unconditionally reassigning it later.
> + if (*offset + 2 > OPT_MAX)
> + return OPT_MAX;
> +
> m->o[*offset] = o;
> m->o[*offset + 1] = opts[o].slen;
> - memcpy(&m->o[*offset + 2], opts[o].s, opts[o].slen);
> + idx = *offset + 2;
I found it a bit difficult to understand what 'idx' is here (offset of
option value). Perhaps you could just recycle *offset to keep track of
the actual offset as we move in the buffer.
That is, we just set two bytes starting from *offset, and now we can do:
*offset += 2;
> + slen = opts[o].slen;
> +
> + /* Truncate if it goes beyond OPT_MAX */
Shouldn't we also report something via debug() in this case?
> + if (idx + slen > OPT_MAX)
> + slen = OPT_MAX - idx;
I didn't check what happens with a zero-length option, but it looks okay
to me. The (pending) change at:
https://github.com/AsahiLinux/muvm/pull/111
uses option 80 ("Rapid Commit"). I can also give it a quick try later.
> +
> + memcpy(&m->o[*offset + 2], opts[o].s, slen);
>
> opts[o].sent = 1;
> *offset += 2 + opts[o].slen;
> + return *offset;
> }
>
> /**
> @@ -172,7 +194,10 @@ static int fill(struct msg *m)
>
> for (o = 0; o < 255; o++) {
> if (opts[o].slen != -1 && !opts[o].sent)
> - fill_one(m, o, &offset);
> + if (fill_one(m, o, &offset) == OPT_MAX) {
> + debug("DHCP: truncating after option %i", o);
I think there's an off-by-one here: if the options exactly fit the
maximum size, we'll warn, but no truncation will actually happen.
Perhaps we should print messages directly in fill_one(), as we have a
better detail of what's going on, there?
> + break;
> + }
> }
>
> m->o[offset++] = 255;
> @@ -285,7 +310,7 @@ static void opt_set_dns_search(const struct ctx *c, size_t max_len)
> */
> int dhcp(const struct ctx *c, const struct pool *p)
> {
> - size_t mlen, dlen, offset = 0, opt_len, opt_off = 0;
> + size_t mlen, dlen, offset = 0, opt_len, opt_off = 0, hostname_len = 0, fqdn_len = 0;
You're assigning those unconditionally before using them, so there's no
need to initialise them here (otherwise one might mistake them as a
"counter" kind of thing).
Please wrap to 80 columns when feasible:
size_t mlen, dlen, offset = 0, opt_len, opt_off = 0;
size_t hostname_len, fqdn_len;
and by the way, you can probably recycle opt_len for those.
> char macstr[ETH_ADDRSTRLEN];
> struct in_addr mask, dst;
> const struct ethhdr *eh;
> @@ -398,6 +423,21 @@ int dhcp(const struct ctx *c, const struct pool *p)
> if (!opts[6].slen)
> opts[6].slen = -1;
>
> + hostname_len = strlen(c->hostname);
> + if (hostname_len > 0) {
> + opts[12].slen = hostname_len;
> + memcpy(opts[12].s, &c->hostname, hostname_len);
> + }
> +
> + fqdn_len = strlen(c->fqdn);
> + if (fqdn_len > 0) {
> + size_t encoded_len = 0;
Sorry, I missed a couple of observations on v4: encoded_len is
uselessly initialised here, and RFC 4702, 2.2, says, about the RCODE
fields:
A server SHOULD set these to 255 when sending the option
and MUST ignore them on receipt.
but you're leaving them as zeroes here.
> + opts[81].s[0] = 0x4; /* flags (E) */
> + encoded_len = encode_domain_name(c->fqdn, fqdn_len,
> + (char *) opts[81].s + 3);
Arguments should be visually aligned:
encoded_len = encode_domain_name(c->fqdn, fqdn_len,
(char *)opts[81].s + 3);
By the way, it's more common to have the destination buffer as first
argument (see memcpy(), snprintf()).
This usage would be more idiomatic and obvious (matching memcpy()) with
something like:
encode_domain_name(opts[81].s + 3, c->fqdn, fqdn_len);
> + opts[81].slen = encoded_len + 3;
> + }
> +
> if (!c->no_dhcp_dns_search)
> opt_set_dns_search(c, sizeof(m->o));
>
> diff --git a/dhcpv6.c b/dhcpv6.c
> index 0523bba..ce3a1bd 100644
> --- a/dhcpv6.c
> +++ b/dhcpv6.c
> @@ -48,6 +48,7 @@ struct opt_hdr {
> # define STATUS_NOTONLINK htons_constant(4)
> # define OPT_DNS_SERVERS htons_constant(23)
> # define OPT_DNS_SEARCH htons_constant(24)
> +# define OPT_CLIENT_FQDN htons_constant(39)
> #define STR_NOTONLINK "Prefix not appropriate for link."
>
> uint16_t l;
> @@ -58,6 +59,9 @@ struct opt_hdr {
> sizeof(struct opt_hdr))
> #define OPT_VSIZE(x) (sizeof(struct opt_##x) - \
> sizeof(struct opt_hdr))
> +#define OPT_MAX_SIZE IPV6_MIN_MTU - (sizeof(struct ipv6hdr) + \
> + sizeof(struct udphdr) + \
> + sizeof(struct msg_hdr))
You can just use tabs, no need to mix them with spaces here.
>
> /**
> * struct opt_client_id - DHCPv6 Client Identifier option
> @@ -163,6 +167,18 @@ struct opt_dns_search {
> char list[MAXDNSRCH * NS_MAXDNAME];
> } __attribute__((packed));
>
> +/**
> + * struct opt_client_fqdn - Client FQDN option (RFC 4704)
> + * @hdr: Option header
> + * @flags: Flags described by RFC 4704 (always zero for us)
> + * @domain_name: Client FQDN
> + */
> +struct opt_client_fqdn{
Missing space between identifier and {.
> + struct opt_hdr hdr;
> + uint8_t flags;
> + char domain_name[PASST_MAXDNAME];
> +} __attribute__((packed));
> +
> /**
> * struct msg_hdr - DHCPv6 client/server message header
> * @type: DHCP message type
> @@ -193,6 +209,7 @@ struct msg_hdr {
> * @client_id: Client Identifier, variable length
> * @dns_servers: DNS Recursive Name Server, here just for storage size
> * @dns_search: Domain Search List, here just for storage size
> + * @client_fqdn: Client FQDN, variable length
> */
> static struct resp_t {
> struct msg_hdr hdr;
> @@ -203,6 +220,7 @@ static struct resp_t {
> struct opt_client_id client_id;
> struct opt_dns_servers dns_servers;
> struct opt_dns_search dns_search;
> + struct opt_client_fqdn client_fqdn;
> } __attribute__((__packed__)) resp = {
> { 0 },
> SERVER_ID,
> @@ -228,6 +246,10 @@ static struct resp_t {
> { { OPT_DNS_SEARCH, 0, },
> { 0 },
> },
> +
> + { { OPT_CLIENT_FQDN, 0, },
> + 0, { 0 },
> + },
> };
>
> static const struct opt_status_code sc_not_on_link = {
> @@ -346,7 +368,6 @@ static size_t dhcpv6_dns_fill(const struct ctx *c, char *buf, int offset)
> {
> struct opt_dns_servers *srv = NULL;
> struct opt_dns_search *srch = NULL;
> - char *p = NULL;
> int i;
>
> if (c->no_dhcp_dns)
> @@ -373,6 +394,7 @@ search:
> return offset;
>
> for (i = 0; *c->dns_search[i].n; i++) {
> + size_t encoded_name_len = 0;
> size_t name_len = strlen(c->dns_search[i].n);
>
> /* We already append separators, don't duplicate if present */
> @@ -388,29 +410,53 @@ search:
> offset += sizeof(struct opt_hdr);
> srch->hdr.t = OPT_DNS_SEARCH;
> srch->hdr.l = 0;
> - p = srch->list;
> }
> -
> - *p = '.';
> - p = stpncpy(p + 1, c->dns_search[i].n, name_len);
> - p++;
> - srch->hdr.l += name_len + 2;
> - offset += name_len + 2;
> +
> + encoded_name_len = encode_domain_name(c->dns_search[i].n,
> + name_len, srch->list);
Please align these arguments:
encoded_name_len = encode_domain_name(c->dns_search[i].n,
name_len, srch->list);
> + srch->hdr.l += encoded_name_len;
> + offset += encoded_name_len;
> }
>
> if (srch) {
There's no need for those curly brackets anymore.
> - for (i = 0; i < srch->hdr.l; i++) {
> - if (srch->list[i] == '.') {
> - srch->list[i] = strcspn(srch->list + i + 1,
> - ".");
> - }
> - }
> srch->hdr.l = htons(srch->hdr.l);
> }
>
> return offset;
> }
>
> +/**
> + * dhcpv6_client_fqdn_fill() - Fill in client FQDN option
> + * @c: Execution context
> + * @buf: Response message buffer where options will be appended
> + * @offset: Offset in message buffer for new options
> + *
> + * Return: updated length of response message buffer.
> + */
> +static size_t dhcpv6_client_fqdn_fill(const struct ctx *c, char *buf, int offset)
Wrap to 80 columns:
static size_t dhcpv6_client_fqdn_fill(const struct ctx *c, char *buf,
int offset)
> +{
> + size_t fqdn_len, opt_hdr_len, opt_len, encoded_fqdn_len;
> + struct opt_client_fqdn *o;
> +
> + opt_hdr_len = sizeof(struct opt_hdr);
Does this really need its own variable?
> +
> + fqdn_len = MIN(strlen(c->fqdn), OPT_MAX_SIZE - (offset + opt_hdr_len + 1));
Wrap to 80 columns. This isn't really clear: here you seem to be using
it as maximum length for the option, but then encode_domain_name()
doesn't use it like that (it's the "Domain name length" according to
the comment there).
> +
Stray tab.
> + if (fqdn_len == 0)
What if it's negative? I guess you should use ssize_t for fqdn_len, and
adjust the check here.
> + return offset;
> +
> + o = (struct opt_client_fqdn *)(buf + offset);
> + encoded_fqdn_len = encode_domain_name(c->fqdn, fqdn_len,
> + o->domain_name);
Argument alignment:
encoded_fqdn_len = encode_domain_name(c->fqdn, fqdn_len,
o->domain_name);
I don't understand: if the option is too long, where is it truncated?
> + opt_len = encoded_fqdn_len + 1;
> +
> + o->hdr.t = OPT_CLIENT_FQDN;
> + o->hdr.l = htons(opt_len);
> + o->flags = 0x00;
I guess I mentioned this already: given that we'll set the "S" bit to
zero (which is correct), if the client's value for the "S" bit is not
zero, we'll need to set the "O" bit.
> +
> + return offset + opt_hdr_len + opt_len;
> +}
> +
> /**
> * dhcpv6() - Check if this is a DHCPv6 message, reply as needed
> * @c: Execution context
> @@ -544,6 +590,7 @@ int dhcpv6(struct ctx *c, const struct pool *p,
> n = offsetof(struct resp_t, client_id) +
> sizeof(struct opt_hdr) + ntohs(client_id->l);
> n = dhcpv6_dns_fill(c, (char *)&resp, n);
> + n = dhcpv6_client_fqdn_fill(c, (char *)&resp, n);
>
> resp.hdr.xid = mh->xid;
>
> diff --git a/passt.1 b/passt.1
> index d9cd33e..8f6b194 100644
> --- a/passt.1
> +++ b/passt.1
> @@ -401,6 +401,17 @@ Enable IPv6-only operation. IPv4 traffic will be ignored.
> By default, IPv4 operation is enabled as long as at least an IPv4 route and an
> interface address are configured on a given host interface.
>
> +.TP
> +.BR \-H ", " \-\-hostname " " \fIname
> +Hostname to configure client with.
For consistency: "the client".
> +Send \fIname as DHCP option 12 (hostname).
I think you want to have "name" in italic, not the whole rest of line
(\fIname\fR). Have a look at the result with 'man ./passt.1'.
> +
> +.TP
> +.BR \-\-fqdn " " \fIname
> +FQDN to configure client with.
> +Send \fIname as dhcp client fqdn option, for DHCP option 81 and for
Same as above.
It's "DHCP" and "FQDN".
> +DHCPv6 option 39.
> +
> .SS \fBpasst\fR-only options
>
> .TP
> diff --git a/passt.h b/passt.h
> index 0dd4efa..9909a10 100644
> --- a/passt.h
> +++ b/passt.h
> @@ -209,6 +209,8 @@ struct ip6_ctx {
> * @ifi4: Template interface for IPv4, -1: none, 0: IPv4 disabled
> * @ip: IPv4 configuration
> * @dns_search: DNS search list
> + * @hostname: Guest hostname
> + * @fqdn: Guest FQDN
> * @ifi6: Template interface for IPv6, -1: none, 0: IPv6 disabled
> * @ip6: IPv6 configuration
> * @pasta_ifn: Name of namespace interface for pasta
> @@ -268,6 +270,9 @@ struct ctx {
> struct ip4_ctx ip4;
>
> struct fqdn dns_search[MAXDNSRCH];
> +
> + char hostname[PASST_MAXDNAME];
> + char fqdn[PASST_MAXDNAME];
Indent with one tab, not spaces.
>
> int ifi6;
> struct ip6_ctx ip6;
> diff --git a/pasta.c b/pasta.c
> index ff41c95..00678f3 100644
> --- a/pasta.c
> +++ b/pasta.c
> @@ -173,6 +173,7 @@ void pasta_open_ns(struct ctx *c, const char *netns)
> struct pasta_spawn_cmd_arg {
> const char *exe;
> char *const *argv;
> + struct ctx *c;
The struct comment should be updated as a result.
> };
>
> /**
> @@ -186,6 +187,7 @@ static int pasta_spawn_cmd(void *arg)
> {
> char hostname[HOST_NAME_MAX + 1] = HOSTNAME_PREFIX;
> const struct pasta_spawn_cmd_arg *a;
> + size_t conf_hostname_len;
> sigset_t set;
>
> /* We run in a detached PID and mount namespace: mount /proc over */
> @@ -194,10 +196,16 @@ static int pasta_spawn_cmd(void *arg)
>
> if (write_file("/proc/sys/net/ipv4/ping_group_range", "0 0"))
> warn("Cannot set ping_group_range, ICMP requests might fail");
> -
> - if (!gethostname(hostname + sizeof(HOSTNAME_PREFIX) - 1,
> - HOST_NAME_MAX + 1 - sizeof(HOSTNAME_PREFIX)) ||
> - errno == ENAMETOOLONG) {
> +
Stray tab.
> + a = (const struct pasta_spawn_cmd_arg *)arg;
> +
> + conf_hostname_len = strlen(a->c->hostname);
> + if (conf_hostname_len > 0) {
> + if (sethostname(a->c->hostname, conf_hostname_len))
> + warn("Unable to set configured hostname");
> + }else if (!gethostname(hostname + sizeof(HOSTNAME_PREFIX) - 1,
Missing space between } and else.
> + HOST_NAME_MAX + 1 - sizeof(HOSTNAME_PREFIX)) ||
This is an argument to gethostname(), so it should be aligned
accordingly.
> + errno == ENAMETOOLONG) {
This is now aligned under "if" for some reason, instead of the original
(correct) alignment after "(".
> hostname[HOST_NAME_MAX] = '\0';
> if (sethostname(hostname, strlen(hostname)))
> warn("Unable to set pasta-prefixed hostname");
> @@ -208,7 +216,6 @@ static int pasta_spawn_cmd(void *arg)
> sigaddset(&set, SIGUSR1);
> sigwaitinfo(&set, NULL);
>
> - a = (const struct pasta_spawn_cmd_arg *)arg;
> execvp(a->exe, a->argv);
>
> die_perror("Failed to start command or shell");
> @@ -230,6 +237,7 @@ void pasta_start_ns(struct ctx *c, uid_t uid, gid_t gid,
> struct pasta_spawn_cmd_arg arg = {
> .exe = argv[0],
> .argv = argv,
> + .c = c,
> };
> char uidmap[BUFSIZ], gidmap[BUFSIZ];
> char *sh_argv[] = { NULL, NULL };
> diff --git a/test/lib/setup b/test/lib/setup
> index 580825f..ee67152 100755
> --- a/test/lib/setup
> +++ b/test/lib/setup
> @@ -49,7 +49,7 @@ setup_passt() {
>
> context_run passt "make clean"
> context_run passt "make valgrind"
> - context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt ${__opts} -s ${STATESETUP}/passt.socket -f -t 10001 -u 10001 -P ${STATESETUP}/passt.pid"
> + context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt ${__opts} -s ${STATESETUP}/passt.socket -f -t 10001 -u 10001 -H hostname1 --fqdn fqdn1.passt.test -P ${STATESETUP}/passt.pid"
>
> # pidfile isn't created until passt is listening
> wait_for [ -f "${STATESETUP}/passt.pid" ]
> @@ -160,11 +160,11 @@ setup_passt_in_ns() {
> if [ ${VALGRIND} -eq 1 ]; then
> context_run passt "make clean"
> context_run passt "make valgrind"
> - context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt -f ${__opts} -s ${STATESETUP}/passt.socket -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}"
> + context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt -f ${__opts} -s ${STATESETUP}/passt.socket -H hostname1 --fqdn fqdn1.passt.test -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}"
> else
> context_run passt "make clean"
> context_run passt "make"
> - context_run_bg passt "./passt -f ${__opts} -s ${STATESETUP}/passt.socket -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}"
> + context_run_bg passt "./passt -f ${__opts} -s ${STATESETUP}/passt.socket -H hostname1 --fqdn fqdn1.passt.test -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}"
> fi
> wait_for [ -f "${STATESETUP}/passt.pid" ]
>
> @@ -243,7 +243,7 @@ setup_two_guests() {
> [ ${TRACE} -eq 1 ] && __opts="${__opts} --trace"
> [ ${VHOST_USER} -eq 1 ] && __opts="${__opts} --vhost-user"
>
> - context_run_bg passt_1 "./passt -s ${STATESETUP}/passt_1.socket -P ${STATESETUP}/passt_1.pid -f ${__opts} -t 10001 -u 10001"
> + context_run_bg passt_1 "./passt -s ${STATESETUP}/passt_1.socket -P ${STATESETUP}/passt_1.pid -f ${__opts} --fqdn fqdn1.passt.test -H hostname1 -t 10001 -u 10001"
> wait_for [ -f "${STATESETUP}/passt_1.pid" ]
>
> __opts=
> @@ -252,7 +252,7 @@ setup_two_guests() {
> [ ${TRACE} -eq 1 ] && __opts="${__opts} --trace"
> [ ${VHOST_USER} -eq 1 ] && __opts="${__opts} --vhost-user"
>
> - context_run_bg passt_2 "./passt -s ${STATESETUP}/passt_2.socket -P ${STATESETUP}/passt_2.pid -f ${__opts} -t 10004 -u 10004"
> + context_run_bg passt_2 "./passt -s ${STATESETUP}/passt_2.socket -P ${STATESETUP}/passt_2.pid -f ${__opts} --hostname hostname2 --fqdn fqdn2 -t 10004 -u 10004"
> wait_for [ -f "${STATESETUP}/passt_2.pid" ]
>
> __vmem="$((${MEM_KIB} / 1024 / 4))"
> diff --git a/test/passt.mbuto b/test/passt.mbuto
> index 138d365..1e07693 100755
> --- a/test/passt.mbuto
> +++ b/test/passt.mbuto
> @@ -13,7 +13,7 @@
> PROGS="${PROGS:-ash,dash,bash ip mount ls insmod mkdir ln cat chmod lsmod
> modprobe find grep mknod mv rm umount jq iperf3 dhclient hostname
> sed tr chown sipcalc cut socat dd strace ping tail killall sleep sysctl
> - nproc tcp_rr tcp_crr udp_rr which tee seq bc sshd ssh-keygen cmp}"
> + nproc tcp_rr tcp_crr udp_rr which tee seq bc sshd ssh-keygen cmp env}"
>
> # OpenSSH 9.8 introduced split binaries, with sshd being the daemon, and
> # sshd-session the per-session program. We need the latter as well, and the path
> @@ -41,6 +41,7 @@ FIXUP="${FIXUP}"'
> #!/bin/sh
> LOG=/var/log/dhclient-script.log
> echo \${reason} \${interface} >> \$LOG
> +env >> \$LOG
> set >> \$LOG
>
> [ -n "\${new_interface_mtu}" ] && ip link set dev \${interface} mtu \${new_interface_mtu}
> @@ -54,7 +55,8 @@ set >> \$LOG
> [ -n "\${new_ip6_address}" ] && ip addr add \${new_ip6_address}/\${new_ip6_prefixlen} dev \${interface}
> [ -n "\${new_dhcp6_name_servers}" ] && for d in \${new_dhcp6_name_servers}; do echo "nameserver \${d}%\${interface}" >> /etc/resolv.conf; done
> [ -n "\${new_dhcp6_domain_search}" ] && (printf "search"; for d in \${new_dhcp6_domain_search}; do printf " %s" "\${d}"; done; printf "\n") >> /etc/resolv.conf
> -[ -n "\${new_host_name}" ] && hostname "\${new_host_name}"
> +[ -n "\${new_host_name}" ] && echo "\${new_host_name}" > /tmp/new_host_name
> +[ -n "\${new_fqdn_fqdn}" ] && echo "\${new_fqdn_fqdn}" > /tmp/new_fqdn_fqdn
> exit 0
> EOF
> chmod 755 /sbin/dhclient-script
> diff --git a/test/passt/dhcp b/test/passt/dhcp
> index 9925ab9..145f1ba 100644
> --- a/test/passt/dhcp
> +++ b/test/passt/dhcp
> @@ -11,7 +11,7 @@
> # Copyright (c) 2021 Red Hat GmbH
> # Author: Stefano Brivio <sbrivio@redhat.com>
>
> -gtools ip jq dhclient sed tr
> +gtools ip jq dhclient sed tr hostname
> htools ip jq sed tr head
>
> test Interface name
> @@ -47,7 +47,16 @@ gout SEARCH sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^searc
> hout HOST_SEARCH sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^search \(.*\)/\1/p' | tr ' \n' ',' | sed 's/,$//;s/$/\n/'
> check [ "__SEARCH__" = "__HOST_SEARCH__" ]
>
> +test DHCP: Hostname
> +gout NEW_HOST_NAME cat /tmp/new_host_name
> +check [ "__NEW_HOST_NAME__" = "hostname1" ]
> +
> +test DHCP: Client FQDN
> +gout NEW_FQDN_FQDN cat /tmp/new_fqdn_fqdn
> +check [ "__NEW_FQDN_FQDN__" = "fqdn1.passt.test" ]
> +
> test DHCPv6: address
> +guest rm /tmp/new_fqdn_fqdn
> guest /sbin/dhclient -6 __IFNAME__
> # Wait for DAD to complete
> guest while ip -j -6 addr show tentative | jq -e '.[].addr_info'; do sleep 0.1; done
> @@ -70,3 +79,7 @@ test DHCPv6: search list
> gout SEARCH6 sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^search \(.*\)/\1/p' | tr ' \n' ',' | sed 's/,$//;s/$/\n/'
> hout HOST_SEARCH6 sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^search \(.*\)/\1/p' | tr ' \n' ',' | sed 's/,$//;s/$/\n/'
> check [ "__SEARCH6__" = "__HOST_SEARCH6__" ]
> +
> +test DHCPv6: Hostname
> +gout NEW_FQDN_FQDN cat /tmp/new_fqdn_fqdn
> +check [ "__NEW_FQDN_FQDN__" = "fqdn1.passt.test" ]
> diff --git a/util.c b/util.c
> index 11973c4..7aeb5b4 100644
> --- a/util.c
> +++ b/util.c
> @@ -837,3 +837,26 @@ void raw_random(void *buf, size_t buflen)
> if (random_read < buflen)
> die("Unexpected EOF on random data source");
> }
> +/**
> + * encode_domain_name() - Encode domain name according to RFC 1035, section 3.1
> + * @domain_name: Input domain name to encode
> + * @len: Domain name length
> + * @buf: Buffer to fill in with encoded domain name
The descriptions should be aligned in the same way.
Shouldn't the maximum buffer length to write be also specified?
> + *
> + * Return: encoded domain name length
> + */
> +size_t encode_domain_name(const char *domain_name, size_t len, char *buf)
> +{
> + char *p;
> + size_t i;
Please order these from longest to shortest.
> +
> + buf[0] = strcspn(domain_name, ".");
> + p = buf + 1;
> + for (i = 0; i < len; i++) {
> + if (domain_name[i] == '.')
> + p[i] = strcspn(domain_name + i + 1, ".");
> + else
> + p[i] = domain_name[i];
> + }
> + return len + 2;
This is +2 if we append a terminator byte at the end, which we should
do as it's part of the encoding, but I don't see it being done.
> +}
> diff --git a/util.h b/util.h
> index 3fa1d12..c55ef29 100644
> --- a/util.h
> +++ b/util.h
> @@ -40,6 +40,9 @@
> #ifndef IP_MAX_MTU
> #define IP_MAX_MTU USHRT_MAX
> #endif
> +#ifndef IPV6_MIN_MTU
> +#define IPV6_MIN_MTU 1280
> +#endif
>
> #ifndef MIN
> #define MIN(x, y) (((x) < (y)) ? (x) : (y))
> @@ -346,4 +349,7 @@ static inline int wrap_accept4(int sockfd, struct sockaddr *addr,
> #define accept4(s, addr, addrlen, flags) \
> wrap_accept4((s), (addr), (addrlen), (flags))
>
> +#define PASST_MAXDNAME 253 /* RFC 1035 */
> +size_t encode_domain_name(const char* domain_name, size_t len, char* buf);
For consistency: char *domain_name, char *buf
> +
> #endif /* UTIL_H */
--
Stefano
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v5] dhcp, dhcpv6: Add hostname and client fqdn ops
2025-01-01 21:54 ` Stefano Brivio
@ 2025-01-02 16:09 ` Enrique Llorente Pastora
2025-01-02 22:00 ` Stefano Brivio
0 siblings, 1 reply; 4+ messages in thread
From: Enrique Llorente Pastora @ 2025-01-02 16:09 UTC (permalink / raw)
To: Stefano Brivio; +Cc: passt-dev
On Wed, Jan 1, 2025 at 10:54 PM Stefano Brivio <sbrivio@redhat.com> wrote:
>
> On Thu, 19 Dec 2024 12:55:57 +0100
> Enrique Llorente <ellorent@redhat.com> wrote:
>
> > Both DHCPv4 and DHCPv6 has the capability to pass the hostname to
> > clients, the DHCPv4 uses option 12 (hostname) while the DHCPv6 uses option 39
> > (client fqdn), for some virt deployments like kubevirt is expected to
> > have the VirtualMachine name as the guest hostname.
> >
> > This change add the following arguments:
> > - -H --hostname NAME to configure the hostname DHCPv4 option(12)
> > - --fqdn NAME to configure client fqdn option for both DHCPv4(81) and
> > DHCPv6(39)
> >
> > Signed-off-by: Enrique Llorente <ellorent@redhat.com>
> > ---
> > conf.c | 20 +++++++++++--
> > dhcp.c | 50 ++++++++++++++++++++++++++++----
> > dhcpv6.c | 75 +++++++++++++++++++++++++++++++++++++++---------
> > passt.1 | 11 +++++++
> > passt.h | 5 ++++
> > pasta.c | 18 ++++++++----
> > test/lib/setup | 10 +++----
> > test/passt.mbuto | 6 ++--
> > test/passt/dhcp | 15 +++++++++-
> > util.c | 23 +++++++++++++++
> > util.h | 6 ++++
> > 11 files changed, 204 insertions(+), 35 deletions(-)
> >
> > diff --git a/conf.c b/conf.c
> > index df2b016..5f21193 100644
> > --- a/conf.c
> > +++ b/conf.c
> > @@ -854,7 +854,9 @@ static void usage(const char *name, FILE *f, int status)
> > FPRINTF(f, " default: use addresses from /etc/resolv.conf\n");
> > FPRINTF(f,
> > " -S, --search LIST Space-separated list, search domains\n"
> > - " a single, empty option disables the DNS search list\n");
> > + " a single, empty option disables the DNS search list\n"
> > + " -H, --hostname NAME Hostname to configure client with\n"
> > + " --fqdn NAME FQDN to configure client with\n");
> > if (strstr(name, "pasta"))
> > FPRINTF(f, " default: don't use any search list\n");
> > else
> > @@ -1313,6 +1315,7 @@ void conf(struct ctx *c, int argc, char **argv)
> > {"outbound", required_argument, NULL, 'o' },
> > {"dns", required_argument, NULL, 'D' },
> > {"search", required_argument, NULL, 'S' },
> > + {"hostname", required_argument, NULL, 'H' },
> > {"no-tcp", no_argument, &c->no_tcp, 1 },
> > {"no-udp", no_argument, &c->no_udp, 1 },
> > {"no-icmp", no_argument, &c->no_icmp, 1 },
> > @@ -1357,6 +1360,7 @@ void conf(struct ctx *c, int argc, char **argv)
> > /* vhost-user backend program convention */
> > {"print-capabilities", no_argument, NULL, 26 },
> > {"socket-path", required_argument, NULL, 's' },
> > + {"fqdn", required_argument, NULL, 27 },
> > { 0 },
> > };
> > const char *logname = (c->mode == MODE_PASTA) ? "pasta" : "passt";
> > @@ -1379,9 +1383,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;
> > fwd_default = FWD_AUTO;
> > - optstring = "+dqfel:hF:I:p:P:m:a:n:M:g:i:o:D:S:46t:u:T:U:";
> > + optstring = "+dqfel:hF:I:p:P:m:a:n:M:g:i:o:D:S:H:46t:u:T:U:";
> > } else {
> > - optstring = "+dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:461t:u:";
> > + optstring = "+dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:H:461t:u:";
> > }
> >
> > c->tcp.fwd_in.mode = c->tcp.fwd_out.mode = FWD_UNSET;
> > @@ -1558,6 +1562,11 @@ void conf(struct ctx *c, int argc, char **argv)
> > case 26:
> > vu_print_capabilities();
> > break;
> > + case 27:
> > + if (snprintf_check(c->fqdn, PASST_MAXDNAME,
> > + "%s", optarg))
>
> Coding style: arguments are preferably aligned with parameter list they
> belong to:
>
> if (snprintf_check(c->fqdn, PASST_MAXDNAME,
> "%s", optarg))
>
> for another example, look just 20 lines below this (case 's').
>
Done
> > + die("Invalid FQDN: %s", optarg);
> > + break;
> > case 'd':
> > c->debug = 1;
> > c->quiet = 0;
> > @@ -1727,6 +1736,11 @@ void conf(struct ctx *c, int argc, char **argv)
> >
> > die("Cannot use DNS search domain %s", optarg);
> > break;
> > + case 'H':
> > + if (snprintf_check(c->hostname, PASST_MAXDNAME,
> > + "%s", optarg))
>
> Same as above.
>
Done
> > + die("Invalid hostname: %s", optarg);
> > + break;
> > case '4':
> > v4_only = true;
> > v6_only = false;
> > diff --git a/dhcp.c b/dhcp.c
> > index d8515aa..b224bf8 100644
> > --- a/dhcp.c
> > +++ b/dhcp.c
> > @@ -63,6 +63,12 @@ static struct opt opts[255];
> >
> > #define OPT_MIN 60 /* RFC 951 */
> >
> > +/* 576 (RFC 2131), minus offset
> > + * of options (268), minus end
> > + * option and its length (2)
> > + */
>
> I formatted this multi-comment in my reply to v4 so that the first
> line would fit with the define:
>
> #define OPT_MAX 306 /* 576 (RFC 2131), minus offset ...
>
> but if you don't want to have it on the same line (no preference from my
> side), then there's no need for all that line wrapping (but then you
> should mention what it refers to), say:
>
> /* Total option size (excluding end option) is 576 (RFC 2131), minus
> * offset of options (268), minus end option and its length (2).
> */
> #define OPT_MAX 306
>
Will go with your multiline comment since it's quite large and it reads better.
> > +#define OPT_MAX 306
> > +
> > /**
> > * dhcp_init() - Initialise DHCP options
> > */
> > @@ -122,7 +128,7 @@ struct msg {
> > uint8_t sname[64];
> > uint8_t file[128];
> > uint32_t magic;
> > - uint8_t o[308];
> > + uint8_t o[OPT_MAX + 2]; /* End option and length */
>
> ..."plus end option and length", or move the comment so that it refers
> to the "2":
>
> uint8_t o[OPT_MAX + 2 /* End option and its length */ ];
>
> Otherwise it looks like we use 310 bytes for the end option and its
> length.
>
Going with inlining it next to "2"
> > } __attribute__((__packed__));
> >
> > /**
> > @@ -130,15 +136,31 @@ struct msg {
> > * @m: Message to fill
> > * @o: Option number
> > * @offset: Current offset within options field, updated on insertion
> > + *
> > + * Return: offset for the next option field
> > */
> > -static void fill_one(struct msg *m, int o, int *offset)
> > +static int fill_one(struct msg *m, int o, int *offset)
> > {
> > + size_t idx, slen = 0;
> > +
> > + /* If it cannot write even enum + len + one byte, then just skip */
>
> ..."it" who? If we can't write, then skip.
>
> It's not clear what "enum" refers to, I guess that should be "number"
> or "code".
>
Done
> By the way, I just realised that this is not entirely correct because
> there's at least one option (option 80, "Rapid Commit") that has length
> zero (it's some sort of flag, with the presence of the option
> indicating that the feature is enabled).
>
> Maybe there's a more elegant solution to this, but a convenient
> alternative might be to check *offset + 1 /* length */ + !!slen instead.
Going with the alternative
>
> That's with slen assigned earlier, there's no point in initialising it
> to zero and then unconditionally reassigning it later.
Done
>
> > + if (*offset + 2 > OPT_MAX)
> > + return OPT_MAX;
> > +
> > m->o[*offset] = o;
> > m->o[*offset + 1] = opts[o].slen;
> > - memcpy(&m->o[*offset + 2], opts[o].s, opts[o].slen);
> > + idx = *offset + 2;
>
> I found it a bit difficult to understand what 'idx' is here (offset of
> option value). Perhaps you could just recycle *offset to keep track of
> the actual offset as we move in the buffer.
>
> That is, we just set two bytes starting from *offset, and now we can do:
>
> *offset += 2;
>
It Makes sense since we just use "idx" everywhere after that so we can
recycle it without problem.
> > + slen = opts[o].slen;
> > +
> > + /* Truncate if it goes beyond OPT_MAX */
>
> Shouldn't we also report something via debug() in this case?
>
Done.
> > + if (idx + slen > OPT_MAX)
> > + slen = OPT_MAX - idx;
>
> I didn't check what happens with a zero-length option, but it looks okay
> to me. The (pending) change at:
>
> https://github.com/AsahiLinux/muvm/pull/111
>
> uses option 80 ("Rapid Commit"). I can also give it a quick try later.
>
Thanks for checking, it would be awesome to have some kind of matrix
test for all these cases though, like starting passt per test with
specific arguments.
> > +
> > + memcpy(&m->o[*offset + 2], opts[o].s, slen);
> >
> > opts[o].sent = 1;
> > *offset += 2 + opts[o].slen;
> > + return *offset;
> > }
> >
> > /**
> > @@ -172,7 +194,10 @@ static int fill(struct msg *m)
> >
> > for (o = 0; o < 255; o++) {
> > if (opts[o].slen != -1 && !opts[o].sent)
> > - fill_one(m, o, &offset);
> > + if (fill_one(m, o, &offset) == OPT_MAX) {
> > + debug("DHCP: truncating after option %i", o);
>
> I think there's an off-by-one here: if the options exactly fit the
> maximum size, we'll warn, but no truncation will actually happen.
>
It will truncate since no more options will be included, this is not
the same as inter option truncate.
> Perhaps we should print messages directly in fill_one(), as we have a
> better detail of what's going on, there?
>
I prefer to log directly at the place where the action is happening
(skipping the rest of the options.
> > + break;
> > + }
> > }
> >
> > m->o[offset++] = 255;
> > @@ -285,7 +310,7 @@ static void opt_set_dns_search(const struct ctx *c, size_t max_len)
> > */
> > int dhcp(const struct ctx *c, const struct pool *p)
> > {
> > - size_t mlen, dlen, offset = 0, opt_len, opt_off = 0;
> > + size_t mlen, dlen, offset = 0, opt_len, opt_off = 0, hostname_len = 0, fqdn_len = 0;
>
> You're assigning those unconditionally before using them, so there's no
> need to initialise them here (otherwise one might mistake them as a
> "counter" kind of thing).
>
Done.
> Please wrap to 80 columns when feasible:
>
> size_t mlen, dlen, offset = 0, opt_len, opt_off = 0;
> size_t hostname_len, fqdn_len;
>
> and by the way, you can probably recycle opt_len for those.
>
Sure, recycling.
> > char macstr[ETH_ADDRSTRLEN];
> > struct in_addr mask, dst;
> > const struct ethhdr *eh;
> > @@ -398,6 +423,21 @@ int dhcp(const struct ctx *c, const struct pool *p)
> > if (!opts[6].slen)
> > opts[6].slen = -1;
> >
> > + hostname_len = strlen(c->hostname);
> > + if (hostname_len > 0) {
> > + opts[12].slen = hostname_len;
> > + memcpy(opts[12].s, &c->hostname, hostname_len);
> > + }
> > +
> > + fqdn_len = strlen(c->fqdn);
> > + if (fqdn_len > 0) {
> > + size_t encoded_len = 0;
>
> Sorry, I missed a couple of observations on v4: encoded_len is
> uselessly initialised here, and RFC 4702, 2.2, says, about the RCODE
> fields:
>
> A server SHOULD set these to 255 when sending the option
> and MUST ignore them on receipt.
>
> but you're leaving them as zeroes here.
Done.
>
> > + opts[81].s[0] = 0x4; /* flags (E) */
> > + encoded_len = encode_domain_name(c->fqdn, fqdn_len,
> > + (char *) opts[81].s + 3);
>
> Arguments should be visually aligned:
>
> encoded_len = encode_domain_name(c->fqdn, fqdn_len,
> (char *)opts[81].s + 3);
>
> By the way, it's more common to have the destination buffer as first
> argument (see memcpy(), snprintf()).
>
> This usage would be more idiomatic and obvious (matching memcpy()) with
> something like:
>
> encode_domain_name(opts[81].s + 3, c->fqdn, fqdn_len);
>
Done.
> > + opts[81].slen = encoded_len + 3;
> > + }
> > +
> > if (!c->no_dhcp_dns_search)
> > opt_set_dns_search(c, sizeof(m->o));
> >
> > diff --git a/dhcpv6.c b/dhcpv6.c
> > index 0523bba..ce3a1bd 100644
> > --- a/dhcpv6.c
> > +++ b/dhcpv6.c
> > @@ -48,6 +48,7 @@ struct opt_hdr {
> > # define STATUS_NOTONLINK htons_constant(4)
> > # define OPT_DNS_SERVERS htons_constant(23)
> > # define OPT_DNS_SEARCH htons_constant(24)
> > +# define OPT_CLIENT_FQDN htons_constant(39)
> > #define STR_NOTONLINK "Prefix not appropriate for link."
> >
> > uint16_t l;
> > @@ -58,6 +59,9 @@ struct opt_hdr {
> > sizeof(struct opt_hdr))
> > #define OPT_VSIZE(x) (sizeof(struct opt_##x) - \
> > sizeof(struct opt_hdr))
> > +#define OPT_MAX_SIZE IPV6_MIN_MTU - (sizeof(struct ipv6hdr) + \
> > + sizeof(struct udphdr) + \
> > + sizeof(struct msg_hdr))
>
> You can just use tabs, no need to mix them with spaces here.
>
Done
> >
> > /**
> > * struct opt_client_id - DHCPv6 Client Identifier option
> > @@ -163,6 +167,18 @@ struct opt_dns_search {
> > char list[MAXDNSRCH * NS_MAXDNAME];
> > } __attribute__((packed));
> >
> > +/**
> > + * struct opt_client_fqdn - Client FQDN option (RFC 4704)
> > + * @hdr: Option header
> > + * @flags: Flags described by RFC 4704 (always zero for us)
> > + * @domain_name: Client FQDN
> > + */
> > +struct opt_client_fqdn{
>
> Missing space between identifier and {.
>
Done
> > + struct opt_hdr hdr;
> > + uint8_t flags;
> > + char domain_name[PASST_MAXDNAME];
> > +} __attribute__((packed));
> > +
> > /**
> > * struct msg_hdr - DHCPv6 client/server message header
> > * @type: DHCP message type
> > @@ -193,6 +209,7 @@ struct msg_hdr {
> > * @client_id: Client Identifier, variable length
> > * @dns_servers: DNS Recursive Name Server, here just for storage size
> > * @dns_search: Domain Search List, here just for storage size
> > + * @client_fqdn: Client FQDN, variable length
> > */
> > static struct resp_t {
> > struct msg_hdr hdr;
> > @@ -203,6 +220,7 @@ static struct resp_t {
> > struct opt_client_id client_id;
> > struct opt_dns_servers dns_servers;
> > struct opt_dns_search dns_search;
> > + struct opt_client_fqdn client_fqdn;
> > } __attribute__((__packed__)) resp = {
> > { 0 },
> > SERVER_ID,
> > @@ -228,6 +246,10 @@ static struct resp_t {
> > { { OPT_DNS_SEARCH, 0, },
> > { 0 },
> > },
> > +
> > + { { OPT_CLIENT_FQDN, 0, },
> > + 0, { 0 },
> > + },
> > };
> >
> > static const struct opt_status_code sc_not_on_link = {
> > @@ -346,7 +368,6 @@ static size_t dhcpv6_dns_fill(const struct ctx *c, char *buf, int offset)
> > {
> > struct opt_dns_servers *srv = NULL;
> > struct opt_dns_search *srch = NULL;
> > - char *p = NULL;
> > int i;
> >
> > if (c->no_dhcp_dns)
> > @@ -373,6 +394,7 @@ search:
> > return offset;
> >
> > for (i = 0; *c->dns_search[i].n; i++) {
> > + size_t encoded_name_len = 0;
> > size_t name_len = strlen(c->dns_search[i].n);
> >
> > /* We already append separators, don't duplicate if present */
> > @@ -388,29 +410,53 @@ search:
> > offset += sizeof(struct opt_hdr);
> > srch->hdr.t = OPT_DNS_SEARCH;
> > srch->hdr.l = 0;
> > - p = srch->list;
> > }
> > -
> > - *p = '.';
> > - p = stpncpy(p + 1, c->dns_search[i].n, name_len);
> > - p++;
> > - srch->hdr.l += name_len + 2;
> > - offset += name_len + 2;
> > +
> > + encoded_name_len = encode_domain_name(c->dns_search[i].n,
> > + name_len, srch->list);
>
> Please align these arguments:
>
> encoded_name_len = encode_domain_name(c->dns_search[i].n,
> name_len, srch->list);
>
Done.
> > + srch->hdr.l += encoded_name_len;
> > + offset += encoded_name_len;
> > }
> >
> > if (srch) {
>
> There's no need for those curly brackets anymore.
>
Done.
> > - for (i = 0; i < srch->hdr.l; i++) {
> > - if (srch->list[i] == '.') {
> > - srch->list[i] = strcspn(srch->list + i + 1,
> > - ".");
> > - }
> > - }
> > srch->hdr.l = htons(srch->hdr.l);
> > }
> >
> > return offset;
> > }
> >
> > +/**
> > + * dhcpv6_client_fqdn_fill() - Fill in client FQDN option
> > + * @c: Execution context
> > + * @buf: Response message buffer where options will be appended
> > + * @offset: Offset in message buffer for new options
> > + *
> > + * Return: updated length of response message buffer.
> > + */
> > +static size_t dhcpv6_client_fqdn_fill(const struct ctx *c, char *buf, int offset)
>
> Wrap to 80 columns:
>
> static size_t dhcpv6_client_fqdn_fill(const struct ctx *c, char *buf,
> int offset)
>
Done.
> > +{
> > + size_t fqdn_len, opt_hdr_len, opt_len, encoded_fqdn_len;
> > + struct opt_client_fqdn *o;
> > +
> > + opt_hdr_len = sizeof(struct opt_hdr);
>
> Does this really need its own variable?
>
Not really, removing.
> > +
> > + fqdn_len = MIN(strlen(c->fqdn), OPT_MAX_SIZE - (offset + opt_hdr_len + 1));
>
> Wrap to 80 columns. This isn't really clear: here you seem to be using
> it as maximum length for the option, but then encode_domain_name()
> doesn't use it like that (it's the "Domain name length" according to
> the comment there).
>
I am going to re-use "opt_len" for all of this, no need for a new
variable, if this is the issue.
> > +
>
> Stray tab.
>
Done.
> > + if (fqdn_len == 0)
>
> What if it's negative? I guess you should use ssize_t for fqdn_len, and
> adjust the check here.
>
Done
> > + return offset;
> > +
> > + o = (struct opt_client_fqdn *)(buf + offset);
> > + encoded_fqdn_len = encode_domain_name(c->fqdn, fqdn_len,
> > + o->domain_name);
>
> Argument alignment:
>
> encoded_fqdn_len = encode_domain_name(c->fqdn, fqdn_len,
> o->domain_name);
>
> I don't understand: if the option is too long, where is it truncated?
>
The len after truncate it is already calculated so passing it to
"encode_domain_name" will truncate the c->fqdn value.
> > + opt_len = encoded_fqdn_len + 1;
> > +
> > + o->hdr.t = OPT_CLIENT_FQDN;
> > + o->hdr.l = htons(opt_len);
> > + o->flags = 0x00;
>
> I guess I mentioned this already: given that we'll set the "S" bit to
> zero (which is correct), if the client's value for the "S" bit is not
> zero, we'll need to set the "O" bit.
>
Totally forgotten, done.
> > +
> > + return offset + opt_hdr_len + opt_len;
> > +}
> > +
> > /**
> > * dhcpv6() - Check if this is a DHCPv6 message, reply as needed
> > * @c: Execution context
> > @@ -544,6 +590,7 @@ int dhcpv6(struct ctx *c, const struct pool *p,
> > n = offsetof(struct resp_t, client_id) +
> > sizeof(struct opt_hdr) + ntohs(client_id->l);
> > n = dhcpv6_dns_fill(c, (char *)&resp, n);
> > + n = dhcpv6_client_fqdn_fill(c, (char *)&resp, n);
> >
> > resp.hdr.xid = mh->xid;
> >
> > diff --git a/passt.1 b/passt.1
> > index d9cd33e..8f6b194 100644
> > --- a/passt.1
> > +++ b/passt.1
> > @@ -401,6 +401,17 @@ Enable IPv6-only operation. IPv4 traffic will be ignored.
> > By default, IPv4 operation is enabled as long as at least an IPv4 route and an
> > interface address are configured on a given host interface.
> >
> > +.TP
> > +.BR \-H ", " \-\-hostname " " \fIname
> > +Hostname to configure client with.
>
> For consistency: "the client".
>
Done.
> > +Send \fIname as DHCP option 12 (hostname).
>
> I think you want to have "name" in italic, not the whole rest of line
> (\fIname\fR). Have a look at the result with 'man ./passt.1'.
>
Done.
> > +
> > +.TP
> > +.BR \-\-fqdn " " \fIname
> > +FQDN to configure client with.
> > +Send \fIname as dhcp client fqdn option, for DHCP option 81 and for
>
> Same as above.
>
Done
> It's "DHCP" and "FQDN".
>
What do you mean ?
> > +DHCPv6 option 39.
> > +
> > .SS \fBpasst\fR-only options
> >
> > .TP
> > diff --git a/passt.h b/passt.h
> > index 0dd4efa..9909a10 100644
> > --- a/passt.h
> > +++ b/passt.h
> > @@ -209,6 +209,8 @@ struct ip6_ctx {
> > * @ifi4: Template interface for IPv4, -1: none, 0: IPv4 disabled
> > * @ip: IPv4 configuration
> > * @dns_search: DNS search list
> > + * @hostname: Guest hostname
> > + * @fqdn: Guest FQDN
> > * @ifi6: Template interface for IPv6, -1: none, 0: IPv6 disabled
> > * @ip6: IPv6 configuration
> > * @pasta_ifn: Name of namespace interface for pasta
> > @@ -268,6 +270,9 @@ struct ctx {
> > struct ip4_ctx ip4;
> >
> > struct fqdn dns_search[MAXDNSRCH];
> > +
> > + char hostname[PASST_MAXDNAME];
> > + char fqdn[PASST_MAXDNAME];
>
> Indent with one tab, not spaces.
>
Done.
> >
> > int ifi6;
> > struct ip6_ctx ip6;
> > diff --git a/pasta.c b/pasta.c
> > index ff41c95..00678f3 100644
> > --- a/pasta.c
> > +++ b/pasta.c
> > @@ -173,6 +173,7 @@ void pasta_open_ns(struct ctx *c, const char *netns)
> > struct pasta_spawn_cmd_arg {
> > const char *exe;
> > char *const *argv;
> > + struct ctx *c;
>
> The struct comment should be updated as a result.
>
Done.
> > };
> >
> > /**
> > @@ -186,6 +187,7 @@ static int pasta_spawn_cmd(void *arg)
> > {
> > char hostname[HOST_NAME_MAX + 1] = HOSTNAME_PREFIX;
> > const struct pasta_spawn_cmd_arg *a;
> > + size_t conf_hostname_len;
> > sigset_t set;
> >
> > /* We run in a detached PID and mount namespace: mount /proc over */
> > @@ -194,10 +196,16 @@ static int pasta_spawn_cmd(void *arg)
> >
> > if (write_file("/proc/sys/net/ipv4/ping_group_range", "0 0"))
> > warn("Cannot set ping_group_range, ICMP requests might fail");
> > -
> > - if (!gethostname(hostname + sizeof(HOSTNAME_PREFIX) - 1,
> > - HOST_NAME_MAX + 1 - sizeof(HOSTNAME_PREFIX)) ||
> > - errno == ENAMETOOLONG) {
> > +
>
> Stray tab.
>
> > + a = (const struct pasta_spawn_cmd_arg *)arg;
> > +
> > + conf_hostname_len = strlen(a->c->hostname);
> > + if (conf_hostname_len > 0) {
> > + if (sethostname(a->c->hostname, conf_hostname_len))
> > + warn("Unable to set configured hostname");
> > + }else if (!gethostname(hostname + sizeof(HOSTNAME_PREFIX) - 1,
>
> Missing space between } and else.
>
Done.
> > + HOST_NAME_MAX + 1 - sizeof(HOSTNAME_PREFIX)) ||
>
> This is an argument to gethostname(), so it should be aligned
> accordingly.
>
Done
> > + errno == ENAMETOOLONG) {
>
> This is now aligned under "if" for some reason, instead of the original
> (correct) alignment after "(".
>
Done.
> > hostname[HOST_NAME_MAX] = '\0';
> > if (sethostname(hostname, strlen(hostname)))
> > warn("Unable to set pasta-prefixed hostname");
> > @@ -208,7 +216,6 @@ static int pasta_spawn_cmd(void *arg)
> > sigaddset(&set, SIGUSR1);
> > sigwaitinfo(&set, NULL);
> >
> > - a = (const struct pasta_spawn_cmd_arg *)arg;
> > execvp(a->exe, a->argv);
> >
> > die_perror("Failed to start command or shell");
> > @@ -230,6 +237,7 @@ void pasta_start_ns(struct ctx *c, uid_t uid, gid_t gid,
> > struct pasta_spawn_cmd_arg arg = {
> > .exe = argv[0],
> > .argv = argv,
> > + .c = c,
> > };
> > char uidmap[BUFSIZ], gidmap[BUFSIZ];
> > char *sh_argv[] = { NULL, NULL };
> > diff --git a/test/lib/setup b/test/lib/setup
> > index 580825f..ee67152 100755
> > --- a/test/lib/setup
> > +++ b/test/lib/setup
> > @@ -49,7 +49,7 @@ setup_passt() {
> >
> > context_run passt "make clean"
> > context_run passt "make valgrind"
> > - context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt ${__opts} -s ${STATESETUP}/passt.socket -f -t 10001 -u 10001 -P ${STATESETUP}/passt.pid"
> > + context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt ${__opts} -s ${STATESETUP}/passt.socket -f -t 10001 -u 10001 -H hostname1 --fqdn fqdn1.passt.test -P ${STATESETUP}/passt.pid"
> >
> > # pidfile isn't created until passt is listening
> > wait_for [ -f "${STATESETUP}/passt.pid" ]
> > @@ -160,11 +160,11 @@ setup_passt_in_ns() {
> > if [ ${VALGRIND} -eq 1 ]; then
> > context_run passt "make clean"
> > context_run passt "make valgrind"
> > - context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt -f ${__opts} -s ${STATESETUP}/passt.socket -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}"
> > + context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt -f ${__opts} -s ${STATESETUP}/passt.socket -H hostname1 --fqdn fqdn1.passt.test -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}"
> > else
> > context_run passt "make clean"
> > context_run passt "make"
> > - context_run_bg passt "./passt -f ${__opts} -s ${STATESETUP}/passt.socket -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}"
> > + context_run_bg passt "./passt -f ${__opts} -s ${STATESETUP}/passt.socket -H hostname1 --fqdn fqdn1.passt.test -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}"
> > fi
> > wait_for [ -f "${STATESETUP}/passt.pid" ]
> >
> > @@ -243,7 +243,7 @@ setup_two_guests() {
> > [ ${TRACE} -eq 1 ] && __opts="${__opts} --trace"
> > [ ${VHOST_USER} -eq 1 ] && __opts="${__opts} --vhost-user"
> >
> > - context_run_bg passt_1 "./passt -s ${STATESETUP}/passt_1.socket -P ${STATESETUP}/passt_1.pid -f ${__opts} -t 10001 -u 10001"
> > + context_run_bg passt_1 "./passt -s ${STATESETUP}/passt_1.socket -P ${STATESETUP}/passt_1.pid -f ${__opts} --fqdn fqdn1.passt.test -H hostname1 -t 10001 -u 10001"
> > wait_for [ -f "${STATESETUP}/passt_1.pid" ]
> >
> > __opts=
> > @@ -252,7 +252,7 @@ setup_two_guests() {
> > [ ${TRACE} -eq 1 ] && __opts="${__opts} --trace"
> > [ ${VHOST_USER} -eq 1 ] && __opts="${__opts} --vhost-user"
> >
> > - context_run_bg passt_2 "./passt -s ${STATESETUP}/passt_2.socket -P ${STATESETUP}/passt_2.pid -f ${__opts} -t 10004 -u 10004"
> > + context_run_bg passt_2 "./passt -s ${STATESETUP}/passt_2.socket -P ${STATESETUP}/passt_2.pid -f ${__opts} --hostname hostname2 --fqdn fqdn2 -t 10004 -u 10004"
> > wait_for [ -f "${STATESETUP}/passt_2.pid" ]
> >
> > __vmem="$((${MEM_KIB} / 1024 / 4))"
> > diff --git a/test/passt.mbuto b/test/passt.mbuto
> > index 138d365..1e07693 100755
> > --- a/test/passt.mbuto
> > +++ b/test/passt.mbuto
> > @@ -13,7 +13,7 @@
> > PROGS="${PROGS:-ash,dash,bash ip mount ls insmod mkdir ln cat chmod lsmod
> > modprobe find grep mknod mv rm umount jq iperf3 dhclient hostname
> > sed tr chown sipcalc cut socat dd strace ping tail killall sleep sysctl
> > - nproc tcp_rr tcp_crr udp_rr which tee seq bc sshd ssh-keygen cmp}"
> > + nproc tcp_rr tcp_crr udp_rr which tee seq bc sshd ssh-keygen cmp env}"
> >
> > # OpenSSH 9.8 introduced split binaries, with sshd being the daemon, and
> > # sshd-session the per-session program. We need the latter as well, and the path
> > @@ -41,6 +41,7 @@ FIXUP="${FIXUP}"'
> > #!/bin/sh
> > LOG=/var/log/dhclient-script.log
> > echo \${reason} \${interface} >> \$LOG
> > +env >> \$LOG
> > set >> \$LOG
> >
> > [ -n "\${new_interface_mtu}" ] && ip link set dev \${interface} mtu \${new_interface_mtu}
> > @@ -54,7 +55,8 @@ set >> \$LOG
> > [ -n "\${new_ip6_address}" ] && ip addr add \${new_ip6_address}/\${new_ip6_prefixlen} dev \${interface}
> > [ -n "\${new_dhcp6_name_servers}" ] && for d in \${new_dhcp6_name_servers}; do echo "nameserver \${d}%\${interface}" >> /etc/resolv.conf; done
> > [ -n "\${new_dhcp6_domain_search}" ] && (printf "search"; for d in \${new_dhcp6_domain_search}; do printf " %s" "\${d}"; done; printf "\n") >> /etc/resolv.conf
> > -[ -n "\${new_host_name}" ] && hostname "\${new_host_name}"
> > +[ -n "\${new_host_name}" ] && echo "\${new_host_name}" > /tmp/new_host_name
> > +[ -n "\${new_fqdn_fqdn}" ] && echo "\${new_fqdn_fqdn}" > /tmp/new_fqdn_fqdn
> > exit 0
> > EOF
> > chmod 755 /sbin/dhclient-script
> > diff --git a/test/passt/dhcp b/test/passt/dhcp
> > index 9925ab9..145f1ba 100644
> > --- a/test/passt/dhcp
> > +++ b/test/passt/dhcp
> > @@ -11,7 +11,7 @@
> > # Copyright (c) 2021 Red Hat GmbH
> > # Author: Stefano Brivio <sbrivio@redhat.com>
> >
> > -gtools ip jq dhclient sed tr
> > +gtools ip jq dhclient sed tr hostname
> > htools ip jq sed tr head
> >
> > test Interface name
> > @@ -47,7 +47,16 @@ gout SEARCH sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^searc
> > hout HOST_SEARCH sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^search \(.*\)/\1/p' | tr ' \n' ',' | sed 's/,$//;s/$/\n/'
> > check [ "__SEARCH__" = "__HOST_SEARCH__" ]
> >
> > +test DHCP: Hostname
> > +gout NEW_HOST_NAME cat /tmp/new_host_name
> > +check [ "__NEW_HOST_NAME__" = "hostname1" ]
> > +
> > +test DHCP: Client FQDN
> > +gout NEW_FQDN_FQDN cat /tmp/new_fqdn_fqdn
> > +check [ "__NEW_FQDN_FQDN__" = "fqdn1.passt.test" ]
> > +
> > test DHCPv6: address
> > +guest rm /tmp/new_fqdn_fqdn
> > guest /sbin/dhclient -6 __IFNAME__
> > # Wait for DAD to complete
> > guest while ip -j -6 addr show tentative | jq -e '.[].addr_info'; do sleep 0.1; done
> > @@ -70,3 +79,7 @@ test DHCPv6: search list
> > gout SEARCH6 sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^search \(.*\)/\1/p' | tr ' \n' ',' | sed 's/,$//;s/$/\n/'
> > hout HOST_SEARCH6 sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^search \(.*\)/\1/p' | tr ' \n' ',' | sed 's/,$//;s/$/\n/'
> > check [ "__SEARCH6__" = "__HOST_SEARCH6__" ]
> > +
> > +test DHCPv6: Hostname
> > +gout NEW_FQDN_FQDN cat /tmp/new_fqdn_fqdn
> > +check [ "__NEW_FQDN_FQDN__" = "fqdn1.passt.test" ]
> > diff --git a/util.c b/util.c
> > index 11973c4..7aeb5b4 100644
> > --- a/util.c
> > +++ b/util.c
> > @@ -837,3 +837,26 @@ void raw_random(void *buf, size_t buflen)
> > if (random_read < buflen)
> > die("Unexpected EOF on random data source");
> > }
> > +/**
> > + * encode_domain_name() - Encode domain name according to RFC 1035, section 3.1
> > + * @domain_name: Input domain name to encode
> > + * @len: Domain name length
>
>
>
> > + * @buf: Buffer to fill in with encoded domain name
>
> The descriptions should be aligned in the same way.
Done.
>
> Shouldn't the maximum buffer length to write be also specified?
>
What do you mean ?
> > + *
> > + * Return: encoded domain name length
> > + */
> > +size_t encode_domain_name(const char *domain_name, size_t len, char *buf)
> > +{
> > + char *p;
> > + size_t i;
>
> Please order these from longest to shortest.
>
Done.
> > +
> > + buf[0] = strcspn(domain_name, ".");
> > + p = buf + 1;
> > + for (i = 0; i < len; i++) {
> > + if (domain_name[i] == '.')
> > + p[i] = strcspn(domain_name + i + 1, ".");
> > + else
> > + p[i] = domain_name[i];
> > + }
> > + return len + 2;
>
> This is +2 if we append a terminator byte at the end, which we should
> do as it's part of the encoding, but I don't see it being done.
>
Added it.
> > +}
> > diff --git a/util.h b/util.h
> > index 3fa1d12..c55ef29 100644
> > --- a/util.h
> > +++ b/util.h
> > @@ -40,6 +40,9 @@
> > #ifndef IP_MAX_MTU
> > #define IP_MAX_MTU USHRT_MAX
> > #endif
> > +#ifndef IPV6_MIN_MTU
> > +#define IPV6_MIN_MTU 1280
> > +#endif
> >
> > #ifndef MIN
> > #define MIN(x, y) (((x) < (y)) ? (x) : (y))
> > @@ -346,4 +349,7 @@ static inline int wrap_accept4(int sockfd, struct sockaddr *addr,
> > #define accept4(s, addr, addrlen, flags) \
> > wrap_accept4((s), (addr), (addrlen), (flags))
> >
> > +#define PASST_MAXDNAME 253 /* RFC 1035 */
> > +size_t encode_domain_name(const char* domain_name, size_t len, char* buf);
>
> For consistency: char *domain_name, char *buf
>
> > +
> > #endif /* UTIL_H */
>
> --
> Stefano
>
--
Quique Llorente
CNV networking Senior Software Engineer
Red Hat EMEA
ellorent@redhat.com
@RedHat Red Hat Red Hat
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v5] dhcp, dhcpv6: Add hostname and client fqdn ops
2025-01-02 16:09 ` Enrique Llorente Pastora
@ 2025-01-02 22:00 ` Stefano Brivio
0 siblings, 0 replies; 4+ messages in thread
From: Stefano Brivio @ 2025-01-02 22:00 UTC (permalink / raw)
To: Enrique Llorente Pastora; +Cc: passt-dev
On Thu, 2 Jan 2025 17:09:48 +0100
Enrique Llorente Pastora <ellorent@redhat.com> wrote:
> On Wed, Jan 1, 2025 at 10:54 PM Stefano Brivio <sbrivio@redhat.com> wrote:
> >
> > On Thu, 19 Dec 2024 12:55:57 +0100
> > Enrique Llorente <ellorent@redhat.com> wrote:
> >
> > > Both DHCPv4 and DHCPv6 has the capability to pass the hostname to
> > > clients, the DHCPv4 uses option 12 (hostname) while the DHCPv6 uses option 39
> > > (client fqdn), for some virt deployments like kubevirt is expected to
> > > have the VirtualMachine name as the guest hostname.
> > >
> > > This change add the following arguments:
> > > - -H --hostname NAME to configure the hostname DHCPv4 option(12)
> > > - --fqdn NAME to configure client fqdn option for both DHCPv4(81) and
> > > DHCPv6(39)
> > >
> > > Signed-off-by: Enrique Llorente <ellorent@redhat.com>
> > > ---
> > > conf.c | 20 +++++++++++--
> > > dhcp.c | 50 ++++++++++++++++++++++++++++----
> > > dhcpv6.c | 75 +++++++++++++++++++++++++++++++++++++++---------
> > > passt.1 | 11 +++++++
> > > passt.h | 5 ++++
> > > pasta.c | 18 ++++++++----
> > > test/lib/setup | 10 +++----
> > > test/passt.mbuto | 6 ++--
> > > test/passt/dhcp | 15 +++++++++-
> > > util.c | 23 +++++++++++++++
> > > util.h | 6 ++++
> > > 11 files changed, 204 insertions(+), 35 deletions(-)
> > >
> > > diff --git a/conf.c b/conf.c
> > > index df2b016..5f21193 100644
> > > --- a/conf.c
> > > +++ b/conf.c
> > > @@ -854,7 +854,9 @@ static void usage(const char *name, FILE *f, int status)
> > > FPRINTF(f, " default: use addresses from /etc/resolv.conf\n");
> > > FPRINTF(f,
> > > " -S, --search LIST Space-separated list, search domains\n"
> > > - " a single, empty option disables the DNS search list\n");
> > > + " a single, empty option disables the DNS search list\n"
> > > + " -H, --hostname NAME Hostname to configure client with\n"
> > > + " --fqdn NAME FQDN to configure client with\n");
> > > if (strstr(name, "pasta"))
> > > FPRINTF(f, " default: don't use any search list\n");
> > > else
> > > @@ -1313,6 +1315,7 @@ void conf(struct ctx *c, int argc, char **argv)
> > > {"outbound", required_argument, NULL, 'o' },
> > > {"dns", required_argument, NULL, 'D' },
> > > {"search", required_argument, NULL, 'S' },
> > > + {"hostname", required_argument, NULL, 'H' },
> > > {"no-tcp", no_argument, &c->no_tcp, 1 },
> > > {"no-udp", no_argument, &c->no_udp, 1 },
> > > {"no-icmp", no_argument, &c->no_icmp, 1 },
> > > @@ -1357,6 +1360,7 @@ void conf(struct ctx *c, int argc, char **argv)
> > > /* vhost-user backend program convention */
> > > {"print-capabilities", no_argument, NULL, 26 },
> > > {"socket-path", required_argument, NULL, 's' },
> > > + {"fqdn", required_argument, NULL, 27 },
> > > { 0 },
> > > };
> > > const char *logname = (c->mode == MODE_PASTA) ? "pasta" : "passt";
> > > @@ -1379,9 +1383,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;
> > > fwd_default = FWD_AUTO;
> > > - optstring = "+dqfel:hF:I:p:P:m:a:n:M:g:i:o:D:S:46t:u:T:U:";
> > > + optstring = "+dqfel:hF:I:p:P:m:a:n:M:g:i:o:D:S:H:46t:u:T:U:";
> > > } else {
> > > - optstring = "+dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:461t:u:";
> > > + optstring = "+dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:H:461t:u:";
> > > }
> > >
> > > c->tcp.fwd_in.mode = c->tcp.fwd_out.mode = FWD_UNSET;
> > > @@ -1558,6 +1562,11 @@ void conf(struct ctx *c, int argc, char **argv)
> > > case 26:
> > > vu_print_capabilities();
> > > break;
> > > + case 27:
> > > + if (snprintf_check(c->fqdn, PASST_MAXDNAME,
> > > + "%s", optarg))
> >
> > Coding style: arguments are preferably aligned with parameter list they
> > belong to:
> >
> > if (snprintf_check(c->fqdn, PASST_MAXDNAME,
> > "%s", optarg))
> >
> > for another example, look just 20 lines below this (case 's').
> >
>
> Done
>
> > > + die("Invalid FQDN: %s", optarg);
> > > + break;
> > > case 'd':
> > > c->debug = 1;
> > > c->quiet = 0;
> > > @@ -1727,6 +1736,11 @@ void conf(struct ctx *c, int argc, char **argv)
> > >
> > > die("Cannot use DNS search domain %s", optarg);
> > > break;
> > > + case 'H':
> > > + if (snprintf_check(c->hostname, PASST_MAXDNAME,
> > > + "%s", optarg))
> >
> > Same as above.
> >
>
> Done
>
>
>
> > > + die("Invalid hostname: %s", optarg);
> > > + break;
> > > case '4':
> > > v4_only = true;
> > > v6_only = false;
> > > diff --git a/dhcp.c b/dhcp.c
> > > index d8515aa..b224bf8 100644
> > > --- a/dhcp.c
> > > +++ b/dhcp.c
> > > @@ -63,6 +63,12 @@ static struct opt opts[255];
> > >
> > > #define OPT_MIN 60 /* RFC 951 */
> > >
> > > +/* 576 (RFC 2131), minus offset
> > > + * of options (268), minus end
> > > + * option and its length (2)
> > > + */
> >
> > I formatted this multi-comment in my reply to v4 so that the first
> > line would fit with the define:
> >
> > #define OPT_MAX 306 /* 576 (RFC 2131), minus offset ...
> >
> > but if you don't want to have it on the same line (no preference from my
> > side), then there's no need for all that line wrapping (but then you
> > should mention what it refers to), say:
> >
> > /* Total option size (excluding end option) is 576 (RFC 2131), minus
> > * offset of options (268), minus end option and its length (2).
> > */
> > #define OPT_MAX 306
> >
>
> Will go with your multiline comment since it's quite large and it reads better.
>
> > > +#define OPT_MAX 306
> > > +
> > > /**
> > > * dhcp_init() - Initialise DHCP options
> > > */
> > > @@ -122,7 +128,7 @@ struct msg {
> > > uint8_t sname[64];
> > > uint8_t file[128];
> > > uint32_t magic;
> > > - uint8_t o[308];
> > > + uint8_t o[OPT_MAX + 2]; /* End option and length */
> >
> > ..."plus end option and length", or move the comment so that it refers
> > to the "2":
> >
> > uint8_t o[OPT_MAX + 2 /* End option and its length */ ];
> >
> > Otherwise it looks like we use 310 bytes for the end option and its
> > length.
> >
>
> Going with inlining it next to "2"
>
> > > } __attribute__((__packed__));
> > >
> > > /**
> > > @@ -130,15 +136,31 @@ struct msg {
> > > * @m: Message to fill
> > > * @o: Option number
> > > * @offset: Current offset within options field, updated on insertion
> > > + *
> > > + * Return: offset for the next option field
> > > */
> > > -static void fill_one(struct msg *m, int o, int *offset)
> > > +static int fill_one(struct msg *m, int o, int *offset)
> > > {
> > > + size_t idx, slen = 0;
> > > +
> > > + /* If it cannot write even enum + len + one byte, then just skip */
> >
> > ..."it" who? If we can't write, then skip.
> >
> > It's not clear what "enum" refers to, I guess that should be "number"
> > or "code".
> >
>
> Done
>
> > By the way, I just realised that this is not entirely correct because
> > there's at least one option (option 80, "Rapid Commit") that has length
> > zero (it's some sort of flag, with the presence of the option
> > indicating that the feature is enabled).
> >
> > Maybe there's a more elegant solution to this, but a convenient
> > alternative might be to check *offset + 1 /* length */ + !!slen instead.
>
> Going with the alternative
>
> >
> > That's with slen assigned earlier, there's no point in initialising it
> > to zero and then unconditionally reassigning it later.
>
> Done
>
> >
> > > + if (*offset + 2 > OPT_MAX)
> > > + return OPT_MAX;
> > > +
> > > m->o[*offset] = o;
> > > m->o[*offset + 1] = opts[o].slen;
> > > - memcpy(&m->o[*offset + 2], opts[o].s, opts[o].slen);
> > > + idx = *offset + 2;
> >
> > I found it a bit difficult to understand what 'idx' is here (offset of
> > option value). Perhaps you could just recycle *offset to keep track of
> > the actual offset as we move in the buffer.
> >
> > That is, we just set two bytes starting from *offset, and now we can do:
> >
> > *offset += 2;
> >
>
> It Makes sense since we just use "idx" everywhere after that so we can
> recycle it without problem.
>
> > > + slen = opts[o].slen;
> > > +
> > > + /* Truncate if it goes beyond OPT_MAX */
> >
> > Shouldn't we also report something via debug() in this case?
> >
>
> Done.
>
> > > + if (idx + slen > OPT_MAX)
> > > + slen = OPT_MAX - idx;
> >
> > I didn't check what happens with a zero-length option, but it looks okay
> > to me. The (pending) change at:
> >
> > https://github.com/AsahiLinux/muvm/pull/111
> >
> > uses option 80 ("Rapid Commit"). I can also give it a quick try later.
>
> Thanks for checking, it would be awesome to have some kind of matrix
> test for all these cases though, like starting passt per test with
> specific arguments.
This is not so easy to check: muvm is not widely packaged, not
necessarily easy to build, and without it, we would probably need a
small stand-alone DHCP client.
Eventually we should add muvm tests, just like we run the ones specific
to pasta from Podman's test suite, but it's a bit too early (see
https://github.com/AsahiLinux/muvm/pull/117#issuecomment-2567922524 as
an example).
> > > +
> > > + memcpy(&m->o[*offset + 2], opts[o].s, slen);
> > >
> > > opts[o].sent = 1;
> > > *offset += 2 + opts[o].slen;
> > > + return *offset;
> > > }
> > >
> > > /**
> > > @@ -172,7 +194,10 @@ static int fill(struct msg *m)
> > >
> > > for (o = 0; o < 255; o++) {
> > > if (opts[o].slen != -1 && !opts[o].sent)
> > > - fill_one(m, o, &offset);
> > > + if (fill_one(m, o, &offset) == OPT_MAX) {
> > > + debug("DHCP: truncating after option %i", o);
> >
> > I think there's an off-by-one here: if the options exactly fit the
> > maximum size, we'll warn, but no truncation will actually happen.
>
> It will truncate since no more options will be included, this is not
> the same as inter option truncate.
I understand, but if you don't have further options (other than the end
option), then it's absolutely fine that no further options are included.
> > Perhaps we should print messages directly in fill_one(), as we have a
> > better detail of what's going on, there?
>
> I prefer to log directly at the place where the action is happening
> (skipping the rest of the options.
...I guess you can maintain that by refactoring the loop a bit, then.
> > > + break;
> > > + }
> > > }
> > >
> > > m->o[offset++] = 255;
> > > @@ -285,7 +310,7 @@ static void opt_set_dns_search(const struct ctx *c, size_t max_len)
> > > */
> > > int dhcp(const struct ctx *c, const struct pool *p)
> > > {
> > > - size_t mlen, dlen, offset = 0, opt_len, opt_off = 0;
> > > + size_t mlen, dlen, offset = 0, opt_len, opt_off = 0, hostname_len = 0, fqdn_len = 0;
> >
> > You're assigning those unconditionally before using them, so there's no
> > need to initialise them here (otherwise one might mistake them as a
> > "counter" kind of thing).
> >
>
> Done.
>
> > Please wrap to 80 columns when feasible:
> >
> > size_t mlen, dlen, offset = 0, opt_len, opt_off = 0;
> > size_t hostname_len, fqdn_len;
> >
> > and by the way, you can probably recycle opt_len for those.
> >
>
> Sure, recycling.
>
> > > char macstr[ETH_ADDRSTRLEN];
> > > struct in_addr mask, dst;
> > > const struct ethhdr *eh;
> > > @@ -398,6 +423,21 @@ int dhcp(const struct ctx *c, const struct pool *p)
> > > if (!opts[6].slen)
> > > opts[6].slen = -1;
> > >
> > > + hostname_len = strlen(c->hostname);
> > > + if (hostname_len > 0) {
> > > + opts[12].slen = hostname_len;
> > > + memcpy(opts[12].s, &c->hostname, hostname_len);
> > > + }
> > > +
> > > + fqdn_len = strlen(c->fqdn);
> > > + if (fqdn_len > 0) {
> > > + size_t encoded_len = 0;
> >
> > Sorry, I missed a couple of observations on v4: encoded_len is
> > uselessly initialised here, and RFC 4702, 2.2, says, about the RCODE
> > fields:
> >
> > A server SHOULD set these to 255 when sending the option
> > and MUST ignore them on receipt.
> >
> > but you're leaving them as zeroes here.
>
> Done.
>
> >
> > > + opts[81].s[0] = 0x4; /* flags (E) */
> > > + encoded_len = encode_domain_name(c->fqdn, fqdn_len,
> > > + (char *) opts[81].s + 3);
> >
> > Arguments should be visually aligned:
> >
> > encoded_len = encode_domain_name(c->fqdn, fqdn_len,
> > (char *)opts[81].s + 3);
> >
> > By the way, it's more common to have the destination buffer as first
> > argument (see memcpy(), snprintf()).
> >
> > This usage would be more idiomatic and obvious (matching memcpy()) with
> > something like:
> >
> > encode_domain_name(opts[81].s + 3, c->fqdn, fqdn_len);
> >
>
> Done.
>
> > > + opts[81].slen = encoded_len + 3;
> > > + }
> > > +
> > > if (!c->no_dhcp_dns_search)
> > > opt_set_dns_search(c, sizeof(m->o));
> > >
> > > diff --git a/dhcpv6.c b/dhcpv6.c
> > > index 0523bba..ce3a1bd 100644
> > > --- a/dhcpv6.c
> > > +++ b/dhcpv6.c
> > > @@ -48,6 +48,7 @@ struct opt_hdr {
> > > # define STATUS_NOTONLINK htons_constant(4)
> > > # define OPT_DNS_SERVERS htons_constant(23)
> > > # define OPT_DNS_SEARCH htons_constant(24)
> > > +# define OPT_CLIENT_FQDN htons_constant(39)
> > > #define STR_NOTONLINK "Prefix not appropriate for link."
> > >
> > > uint16_t l;
> > > @@ -58,6 +59,9 @@ struct opt_hdr {
> > > sizeof(struct opt_hdr))
> > > #define OPT_VSIZE(x) (sizeof(struct opt_##x) - \
> > > sizeof(struct opt_hdr))
> > > +#define OPT_MAX_SIZE IPV6_MIN_MTU - (sizeof(struct ipv6hdr) + \
> > > + sizeof(struct udphdr) + \
> > > + sizeof(struct msg_hdr))
> >
> > You can just use tabs, no need to mix them with spaces here.
> >
>
> Done
>
> > >
> > > /**
> > > * struct opt_client_id - DHCPv6 Client Identifier option
> > > @@ -163,6 +167,18 @@ struct opt_dns_search {
> > > char list[MAXDNSRCH * NS_MAXDNAME];
> > > } __attribute__((packed));
> > >
> > > +/**
> > > + * struct opt_client_fqdn - Client FQDN option (RFC 4704)
> > > + * @hdr: Option header
> > > + * @flags: Flags described by RFC 4704 (always zero for us)
> > > + * @domain_name: Client FQDN
> > > + */
> > > +struct opt_client_fqdn{
> >
> > Missing space between identifier and {.
> >
>
> Done
>
> > > + struct opt_hdr hdr;
> > > + uint8_t flags;
> > > + char domain_name[PASST_MAXDNAME];
> > > +} __attribute__((packed));
> > > +
> > > /**
> > > * struct msg_hdr - DHCPv6 client/server message header
> > > * @type: DHCP message type
> > > @@ -193,6 +209,7 @@ struct msg_hdr {
> > > * @client_id: Client Identifier, variable length
> > > * @dns_servers: DNS Recursive Name Server, here just for storage size
> > > * @dns_search: Domain Search List, here just for storage size
> > > + * @client_fqdn: Client FQDN, variable length
> > > */
> > > static struct resp_t {
> > > struct msg_hdr hdr;
> > > @@ -203,6 +220,7 @@ static struct resp_t {
> > > struct opt_client_id client_id;
> > > struct opt_dns_servers dns_servers;
> > > struct opt_dns_search dns_search;
> > > + struct opt_client_fqdn client_fqdn;
> > > } __attribute__((__packed__)) resp = {
> > > { 0 },
> > > SERVER_ID,
> > > @@ -228,6 +246,10 @@ static struct resp_t {
> > > { { OPT_DNS_SEARCH, 0, },
> > > { 0 },
> > > },
> > > +
> > > + { { OPT_CLIENT_FQDN, 0, },
> > > + 0, { 0 },
> > > + },
> > > };
> > >
> > > static const struct opt_status_code sc_not_on_link = {
> > > @@ -346,7 +368,6 @@ static size_t dhcpv6_dns_fill(const struct ctx *c, char *buf, int offset)
> > > {
> > > struct opt_dns_servers *srv = NULL;
> > > struct opt_dns_search *srch = NULL;
> > > - char *p = NULL;
> > > int i;
> > >
> > > if (c->no_dhcp_dns)
> > > @@ -373,6 +394,7 @@ search:
> > > return offset;
> > >
> > > for (i = 0; *c->dns_search[i].n; i++) {
> > > + size_t encoded_name_len = 0;
> > > size_t name_len = strlen(c->dns_search[i].n);
> > >
> > > /* We already append separators, don't duplicate if present */
> > > @@ -388,29 +410,53 @@ search:
> > > offset += sizeof(struct opt_hdr);
> > > srch->hdr.t = OPT_DNS_SEARCH;
> > > srch->hdr.l = 0;
> > > - p = srch->list;
> > > }
> > > -
> > > - *p = '.';
> > > - p = stpncpy(p + 1, c->dns_search[i].n, name_len);
> > > - p++;
> > > - srch->hdr.l += name_len + 2;
> > > - offset += name_len + 2;
> > > +
> > > + encoded_name_len = encode_domain_name(c->dns_search[i].n,
> > > + name_len, srch->list);
> >
> > Please align these arguments:
> >
> > encoded_name_len = encode_domain_name(c->dns_search[i].n,
> > name_len, srch->list);
> >
>
> Done.
>
> > > + srch->hdr.l += encoded_name_len;
> > > + offset += encoded_name_len;
> > > }
> > >
> > > if (srch) {
> >
> > There's no need for those curly brackets anymore.
> >
>
> Done.
>
> > > - for (i = 0; i < srch->hdr.l; i++) {
> > > - if (srch->list[i] == '.') {
> > > - srch->list[i] = strcspn(srch->list + i + 1,
> > > - ".");
> > > - }
> > > - }
> > > srch->hdr.l = htons(srch->hdr.l);
> > > }
> > >
> > > return offset;
> > > }
> > >
> > > +/**
> > > + * dhcpv6_client_fqdn_fill() - Fill in client FQDN option
> > > + * @c: Execution context
> > > + * @buf: Response message buffer where options will be appended
> > > + * @offset: Offset in message buffer for new options
> > > + *
> > > + * Return: updated length of response message buffer.
> > > + */
> > > +static size_t dhcpv6_client_fqdn_fill(const struct ctx *c, char *buf, int offset)
> >
> > Wrap to 80 columns:
> >
> > static size_t dhcpv6_client_fqdn_fill(const struct ctx *c, char *buf,
> > int offset)
> >
>
> Done.
>
> > > +{
> > > + size_t fqdn_len, opt_hdr_len, opt_len, encoded_fqdn_len;
> > > + struct opt_client_fqdn *o;
> > > +
> > > + opt_hdr_len = sizeof(struct opt_hdr);
> >
> > Does this really need its own variable?
> >
>
> Not really, removing.
>
>
> > > +
> > > + fqdn_len = MIN(strlen(c->fqdn), OPT_MAX_SIZE - (offset + opt_hdr_len + 1));
> >
> > Wrap to 80 columns. This isn't really clear: here you seem to be using
> > it as maximum length for the option, but then encode_domain_name()
> > doesn't use it like that (it's the "Domain name length" according to
> > the comment there).
> >
>
> I am going to re-use "opt_len" for all of this, no need for a new
> variable, if this is the issue.
>
> > > +
> >
> > Stray tab.
> >
>
> Done.
>
> > > + if (fqdn_len == 0)
> >
> > What if it's negative? I guess you should use ssize_t for fqdn_len, and
> > adjust the check here.
> >
>
> Done
>
> > > + return offset;
> > > +
> > > + o = (struct opt_client_fqdn *)(buf + offset);
> > > + encoded_fqdn_len = encode_domain_name(c->fqdn, fqdn_len,
> > > + o->domain_name);
> >
> > Argument alignment:
> >
> > encoded_fqdn_len = encode_domain_name(c->fqdn, fqdn_len,
> > o->domain_name);
> >
> > I don't understand: if the option is too long, where is it truncated?
>
> The len after truncate it is already calculated so passing it to
> "encode_domain_name" will truncate the c->fqdn value.
Yes, but the second argument of encode_domain_name() is an "input"
length, and fqdn_len is the output bound, so it doesn't quite seem to
match.
> > > + opt_len = encoded_fqdn_len + 1;
> > > +
> > > + o->hdr.t = OPT_CLIENT_FQDN;
> > > + o->hdr.l = htons(opt_len);
> > > + o->flags = 0x00;
> >
> > I guess I mentioned this already: given that we'll set the "S" bit to
> > zero (which is correct), if the client's value for the "S" bit is not
> > zero, we'll need to set the "O" bit.
> >
>
> Totally forgotten, done.
>
> > > +
> > > + return offset + opt_hdr_len + opt_len;
> > > +}
> > > +
> > > /**
> > > * dhcpv6() - Check if this is a DHCPv6 message, reply as needed
> > > * @c: Execution context
> > > @@ -544,6 +590,7 @@ int dhcpv6(struct ctx *c, const struct pool *p,
> > > n = offsetof(struct resp_t, client_id) +
> > > sizeof(struct opt_hdr) + ntohs(client_id->l);
> > > n = dhcpv6_dns_fill(c, (char *)&resp, n);
> > > + n = dhcpv6_client_fqdn_fill(c, (char *)&resp, n);
> > >
> > > resp.hdr.xid = mh->xid;
> > >
> > > diff --git a/passt.1 b/passt.1
> > > index d9cd33e..8f6b194 100644
> > > --- a/passt.1
> > > +++ b/passt.1
> > > @@ -401,6 +401,17 @@ Enable IPv6-only operation. IPv4 traffic will be ignored.
> > > By default, IPv4 operation is enabled as long as at least an IPv4 route and an
> > > interface address are configured on a given host interface.
> > >
> > > +.TP
> > > +.BR \-H ", " \-\-hostname " " \fIname
> > > +Hostname to configure client with.
> >
> > For consistency: "the client".
> >
>
> Done.
>
> > > +Send \fIname as DHCP option 12 (hostname).
> >
> > I think you want to have "name" in italic, not the whole rest of line
> > (\fIname\fR). Have a look at the result with 'man ./passt.1'.
> >
>
> Done.
>
> > > +
> > > +.TP
> > > +.BR \-\-fqdn " " \fIname
> > > +FQDN to configure client with.
> > > +Send \fIname as dhcp client fqdn option, for DHCP option 81 and for
> >
> > Same as above.
> >
>
> Done
>
> > It's "DHCP" and "FQDN".
> >
>
> What do you mean ?
That those are acronyms and, in case of DHCP, the standard name of a
protocol, so they are written uppercase. It's DHCP, not "dhcp", and
FQDN, not "fqdn".
> > > +DHCPv6 option 39.
> > > +
> > > .SS \fBpasst\fR-only options
> > >
> > > .TP
> > > diff --git a/passt.h b/passt.h
> > > index 0dd4efa..9909a10 100644
> > > --- a/passt.h
> > > +++ b/passt.h
> > > @@ -209,6 +209,8 @@ struct ip6_ctx {
> > > * @ifi4: Template interface for IPv4, -1: none, 0: IPv4 disabled
> > > * @ip: IPv4 configuration
> > > * @dns_search: DNS search list
> > > + * @hostname: Guest hostname
> > > + * @fqdn: Guest FQDN
> > > * @ifi6: Template interface for IPv6, -1: none, 0: IPv6 disabled
> > > * @ip6: IPv6 configuration
> > > * @pasta_ifn: Name of namespace interface for pasta
> > > @@ -268,6 +270,9 @@ struct ctx {
> > > struct ip4_ctx ip4;
> > >
> > > struct fqdn dns_search[MAXDNSRCH];
> > > +
> > > + char hostname[PASST_MAXDNAME];
> > > + char fqdn[PASST_MAXDNAME];
> >
> > Indent with one tab, not spaces.
> >
>
> Done.
>
> > >
> > > int ifi6;
> > > struct ip6_ctx ip6;
> > > diff --git a/pasta.c b/pasta.c
> > > index ff41c95..00678f3 100644
> > > --- a/pasta.c
> > > +++ b/pasta.c
> > > @@ -173,6 +173,7 @@ void pasta_open_ns(struct ctx *c, const char *netns)
> > > struct pasta_spawn_cmd_arg {
> > > const char *exe;
> > > char *const *argv;
> > > + struct ctx *c;
> >
> > The struct comment should be updated as a result.
> >
>
> Done.
>
> > > };
> > >
> > > /**
> > > @@ -186,6 +187,7 @@ static int pasta_spawn_cmd(void *arg)
> > > {
> > > char hostname[HOST_NAME_MAX + 1] = HOSTNAME_PREFIX;
> > > const struct pasta_spawn_cmd_arg *a;
> > > + size_t conf_hostname_len;
> > > sigset_t set;
> > >
> > > /* We run in a detached PID and mount namespace: mount /proc over */
> > > @@ -194,10 +196,16 @@ static int pasta_spawn_cmd(void *arg)
> > >
> > > if (write_file("/proc/sys/net/ipv4/ping_group_range", "0 0"))
> > > warn("Cannot set ping_group_range, ICMP requests might fail");
> > > -
> > > - if (!gethostname(hostname + sizeof(HOSTNAME_PREFIX) - 1,
> > > - HOST_NAME_MAX + 1 - sizeof(HOSTNAME_PREFIX)) ||
> > > - errno == ENAMETOOLONG) {
> > > +
> >
> > Stray tab.
> >
> > > + a = (const struct pasta_spawn_cmd_arg *)arg;
> > > +
> > > + conf_hostname_len = strlen(a->c->hostname);
> > > + if (conf_hostname_len > 0) {
> > > + if (sethostname(a->c->hostname, conf_hostname_len))
> > > + warn("Unable to set configured hostname");
> > > + }else if (!gethostname(hostname + sizeof(HOSTNAME_PREFIX) - 1,
> >
> > Missing space between } and else.
> >
>
> Done.
>
> > > + HOST_NAME_MAX + 1 - sizeof(HOSTNAME_PREFIX)) ||
> >
> > This is an argument to gethostname(), so it should be aligned
> > accordingly.
> >
>
> Done
>
> > > + errno == ENAMETOOLONG) {
> >
> > This is now aligned under "if" for some reason, instead of the original
> > (correct) alignment after "(".
> >
>
> Done.
>
> > > hostname[HOST_NAME_MAX] = '\0';
> > > if (sethostname(hostname, strlen(hostname)))
> > > warn("Unable to set pasta-prefixed hostname");
> > > @@ -208,7 +216,6 @@ static int pasta_spawn_cmd(void *arg)
> > > sigaddset(&set, SIGUSR1);
> > > sigwaitinfo(&set, NULL);
> > >
> > > - a = (const struct pasta_spawn_cmd_arg *)arg;
> > > execvp(a->exe, a->argv);
> > >
> > > die_perror("Failed to start command or shell");
> > > @@ -230,6 +237,7 @@ void pasta_start_ns(struct ctx *c, uid_t uid, gid_t gid,
> > > struct pasta_spawn_cmd_arg arg = {
> > > .exe = argv[0],
> > > .argv = argv,
> > > + .c = c,
> > > };
> > > char uidmap[BUFSIZ], gidmap[BUFSIZ];
> > > char *sh_argv[] = { NULL, NULL };
> > > diff --git a/test/lib/setup b/test/lib/setup
> > > index 580825f..ee67152 100755
> > > --- a/test/lib/setup
> > > +++ b/test/lib/setup
> > > @@ -49,7 +49,7 @@ setup_passt() {
> > >
> > > context_run passt "make clean"
> > > context_run passt "make valgrind"
> > > - context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt ${__opts} -s ${STATESETUP}/passt.socket -f -t 10001 -u 10001 -P ${STATESETUP}/passt.pid"
> > > + context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt ${__opts} -s ${STATESETUP}/passt.socket -f -t 10001 -u 10001 -H hostname1 --fqdn fqdn1.passt.test -P ${STATESETUP}/passt.pid"
> > >
> > > # pidfile isn't created until passt is listening
> > > wait_for [ -f "${STATESETUP}/passt.pid" ]
> > > @@ -160,11 +160,11 @@ setup_passt_in_ns() {
> > > if [ ${VALGRIND} -eq 1 ]; then
> > > context_run passt "make clean"
> > > context_run passt "make valgrind"
> > > - context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt -f ${__opts} -s ${STATESETUP}/passt.socket -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}"
> > > + context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt -f ${__opts} -s ${STATESETUP}/passt.socket -H hostname1 --fqdn fqdn1.passt.test -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}"
> > > else
> > > context_run passt "make clean"
> > > context_run passt "make"
> > > - context_run_bg passt "./passt -f ${__opts} -s ${STATESETUP}/passt.socket -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}"
> > > + context_run_bg passt "./passt -f ${__opts} -s ${STATESETUP}/passt.socket -H hostname1 --fqdn fqdn1.passt.test -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}"
> > > fi
> > > wait_for [ -f "${STATESETUP}/passt.pid" ]
> > >
> > > @@ -243,7 +243,7 @@ setup_two_guests() {
> > > [ ${TRACE} -eq 1 ] && __opts="${__opts} --trace"
> > > [ ${VHOST_USER} -eq 1 ] && __opts="${__opts} --vhost-user"
> > >
> > > - context_run_bg passt_1 "./passt -s ${STATESETUP}/passt_1.socket -P ${STATESETUP}/passt_1.pid -f ${__opts} -t 10001 -u 10001"
> > > + context_run_bg passt_1 "./passt -s ${STATESETUP}/passt_1.socket -P ${STATESETUP}/passt_1.pid -f ${__opts} --fqdn fqdn1.passt.test -H hostname1 -t 10001 -u 10001"
> > > wait_for [ -f "${STATESETUP}/passt_1.pid" ]
> > >
> > > __opts=
> > > @@ -252,7 +252,7 @@ setup_two_guests() {
> > > [ ${TRACE} -eq 1 ] && __opts="${__opts} --trace"
> > > [ ${VHOST_USER} -eq 1 ] && __opts="${__opts} --vhost-user"
> > >
> > > - context_run_bg passt_2 "./passt -s ${STATESETUP}/passt_2.socket -P ${STATESETUP}/passt_2.pid -f ${__opts} -t 10004 -u 10004"
> > > + context_run_bg passt_2 "./passt -s ${STATESETUP}/passt_2.socket -P ${STATESETUP}/passt_2.pid -f ${__opts} --hostname hostname2 --fqdn fqdn2 -t 10004 -u 10004"
> > > wait_for [ -f "${STATESETUP}/passt_2.pid" ]
> > >
> > > __vmem="$((${MEM_KIB} / 1024 / 4))"
> > > diff --git a/test/passt.mbuto b/test/passt.mbuto
> > > index 138d365..1e07693 100755
> > > --- a/test/passt.mbuto
> > > +++ b/test/passt.mbuto
> > > @@ -13,7 +13,7 @@
> > > PROGS="${PROGS:-ash,dash,bash ip mount ls insmod mkdir ln cat chmod lsmod
> > > modprobe find grep mknod mv rm umount jq iperf3 dhclient hostname
> > > sed tr chown sipcalc cut socat dd strace ping tail killall sleep sysctl
> > > - nproc tcp_rr tcp_crr udp_rr which tee seq bc sshd ssh-keygen cmp}"
> > > + nproc tcp_rr tcp_crr udp_rr which tee seq bc sshd ssh-keygen cmp env}"
> > >
> > > # OpenSSH 9.8 introduced split binaries, with sshd being the daemon, and
> > > # sshd-session the per-session program. We need the latter as well, and the path
> > > @@ -41,6 +41,7 @@ FIXUP="${FIXUP}"'
> > > #!/bin/sh
> > > LOG=/var/log/dhclient-script.log
> > > echo \${reason} \${interface} >> \$LOG
> > > +env >> \$LOG
> > > set >> \$LOG
> > >
> > > [ -n "\${new_interface_mtu}" ] && ip link set dev \${interface} mtu \${new_interface_mtu}
> > > @@ -54,7 +55,8 @@ set >> \$LOG
> > > [ -n "\${new_ip6_address}" ] && ip addr add \${new_ip6_address}/\${new_ip6_prefixlen} dev \${interface}
> > > [ -n "\${new_dhcp6_name_servers}" ] && for d in \${new_dhcp6_name_servers}; do echo "nameserver \${d}%\${interface}" >> /etc/resolv.conf; done
> > > [ -n "\${new_dhcp6_domain_search}" ] && (printf "search"; for d in \${new_dhcp6_domain_search}; do printf " %s" "\${d}"; done; printf "\n") >> /etc/resolv.conf
> > > -[ -n "\${new_host_name}" ] && hostname "\${new_host_name}"
> > > +[ -n "\${new_host_name}" ] && echo "\${new_host_name}" > /tmp/new_host_name
> > > +[ -n "\${new_fqdn_fqdn}" ] && echo "\${new_fqdn_fqdn}" > /tmp/new_fqdn_fqdn
> > > exit 0
> > > EOF
> > > chmod 755 /sbin/dhclient-script
> > > diff --git a/test/passt/dhcp b/test/passt/dhcp
> > > index 9925ab9..145f1ba 100644
> > > --- a/test/passt/dhcp
> > > +++ b/test/passt/dhcp
> > > @@ -11,7 +11,7 @@
> > > # Copyright (c) 2021 Red Hat GmbH
> > > # Author: Stefano Brivio <sbrivio@redhat.com>
> > >
> > > -gtools ip jq dhclient sed tr
> > > +gtools ip jq dhclient sed tr hostname
> > > htools ip jq sed tr head
> > >
> > > test Interface name
> > > @@ -47,7 +47,16 @@ gout SEARCH sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^searc
> > > hout HOST_SEARCH sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^search \(.*\)/\1/p' | tr ' \n' ',' | sed 's/,$//;s/$/\n/'
> > > check [ "__SEARCH__" = "__HOST_SEARCH__" ]
> > >
> > > +test DHCP: Hostname
> > > +gout NEW_HOST_NAME cat /tmp/new_host_name
> > > +check [ "__NEW_HOST_NAME__" = "hostname1" ]
> > > +
> > > +test DHCP: Client FQDN
> > > +gout NEW_FQDN_FQDN cat /tmp/new_fqdn_fqdn
> > > +check [ "__NEW_FQDN_FQDN__" = "fqdn1.passt.test" ]
> > > +
> > > test DHCPv6: address
> > > +guest rm /tmp/new_fqdn_fqdn
> > > guest /sbin/dhclient -6 __IFNAME__
> > > # Wait for DAD to complete
> > > guest while ip -j -6 addr show tentative | jq -e '.[].addr_info'; do sleep 0.1; done
> > > @@ -70,3 +79,7 @@ test DHCPv6: search list
> > > gout SEARCH6 sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^search \(.*\)/\1/p' | tr ' \n' ',' | sed 's/,$//;s/$/\n/'
> > > hout HOST_SEARCH6 sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^search \(.*\)/\1/p' | tr ' \n' ',' | sed 's/,$//;s/$/\n/'
> > > check [ "__SEARCH6__" = "__HOST_SEARCH6__" ]
> > > +
> > > +test DHCPv6: Hostname
> > > +gout NEW_FQDN_FQDN cat /tmp/new_fqdn_fqdn
> > > +check [ "__NEW_FQDN_FQDN__" = "fqdn1.passt.test" ]
> > > diff --git a/util.c b/util.c
> > > index 11973c4..7aeb5b4 100644
> > > --- a/util.c
> > > +++ b/util.c
> > > @@ -837,3 +837,26 @@ void raw_random(void *buf, size_t buflen)
> > > if (random_read < buflen)
> > > die("Unexpected EOF on random data source");
> > > }
> > > +/**
> > > + * encode_domain_name() - Encode domain name according to RFC 1035, section 3.1
> > > + * @domain_name: Input domain name to encode
> > > + * @len: Domain name length
> >
> >
> >
> > > + * @buf: Buffer to fill in with encoded domain name
> >
> > The descriptions should be aligned in the same way.
>
> Done.
>
> >
> > Shouldn't the maximum buffer length to write be also specified?
> >
>
> What do you mean ?
I mean that the current version of encode_domain_name() just takes
"len", which is the length of the *unencoded* domain (I suppose, the
comment doesn't really say it), but it doesn't take the length of the
encoded domain (output).
This isn't really safe, because usually formatting functions take the
size of the *output* buffer, and by writing two characters more, you
might trick users of this function to pass a buffer with 'len' size.
Tricked users include yourself as of this patch, I guess:
dhcpv6_dns_fill() passes the size of the *input*, while
dhcpv6_client_fqdn_fill() passes the size of the *output* (because
it's calculated from the available buffer size in the response), and
encode_domain_name() uses it as *input*.
Actually, you don't need the input length at all, because the input
string always has a terminator. But to truncate the domain names
properly, you need to know if you're exceeding the size of the
destination buffer.
If you're about to exceed that, by the way, you can't just stop
there and pretend that the domain is valid. Let's say you have 9
bytes left and you need to encode passt.top: ApasstBtopC, where A
is the decimal byte value 5, B is 3, and C is 0.
If you just use 8 bytes, that would be ApasstBt, which comes with
two issues:
- B doesn't represent the size of the next label anymore
- the terminator, C, is missing altogether
...now, the idea behind truncating these options was to keep this
patch simple for the moment, and somewhat easily allow extending
the feature for DHCP (with option overload) at a later time. But
perhaps it's adding more complexity than we thought... and perhaps
we risk crashing clients, too.
Should we just omit options if they don't fit? It's probably a
couple of lines of difference, compared to what you have now.
> > > + *
> > > + * Return: encoded domain name length
> > > + */
> > > +size_t encode_domain_name(const char *domain_name, size_t len, char *buf)
> > > +{
> > > + char *p;
> > > + size_t i;
> >
> > Please order these from longest to shortest.
> >
>
> Done.
>
> > > +
> > > + buf[0] = strcspn(domain_name, ".");
> > > + p = buf + 1;
> > > + for (i = 0; i < len; i++) {
> > > + if (domain_name[i] == '.')
> > > + p[i] = strcspn(domain_name + i + 1, ".");
> > > + else
> > > + p[i] = domain_name[i];
> > > + }
> > > + return len + 2;
> >
> > This is +2 if we append a terminator byte at the end, which we should
> > do as it's part of the encoding, but I don't see it being done.
> >
>
> Added it.
>
> > > +}
> > > diff --git a/util.h b/util.h
> > > index 3fa1d12..c55ef29 100644
> > > --- a/util.h
> > > +++ b/util.h
> > > @@ -40,6 +40,9 @@
> > > #ifndef IP_MAX_MTU
> > > #define IP_MAX_MTU USHRT_MAX
> > > #endif
> > > +#ifndef IPV6_MIN_MTU
> > > +#define IPV6_MIN_MTU 1280
> > > +#endif
> > >
> > > #ifndef MIN
> > > #define MIN(x, y) (((x) < (y)) ? (x) : (y))
> > > @@ -346,4 +349,7 @@ static inline int wrap_accept4(int sockfd, struct sockaddr *addr,
> > > #define accept4(s, addr, addrlen, flags) \
> > > wrap_accept4((s), (addr), (addrlen), (flags))
> > >
> > > +#define PASST_MAXDNAME 253 /* RFC 1035 */
> > > +size_t encode_domain_name(const char* domain_name, size_t len, char* buf);
> >
> > For consistency: char *domain_name, char *buf
> >
> > > +
> > > #endif /* UTIL_H */
--
Stefano
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-01-02 22:00 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-12-19 11:55 [PATCH v5] dhcp, dhcpv6: Add hostname and client fqdn ops Enrique Llorente
2025-01-01 21:54 ` Stefano Brivio
2025-01-02 16:09 ` Enrique Llorente Pastora
2025-01-02 22:00 ` 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).