From: Enrique Llorente Pastora <ellorent@redhat.com>
To: Stefano Brivio <sbrivio@redhat.com>
Cc: passt-dev@passt.top
Subject: Re: [PATCH v7] dhcp, dhcpv6: Add hostname and client fqdn ops
Date: Fri, 10 Jan 2025 09:39:09 +0100 [thread overview]
Message-ID: <CAHVoYmK6XMi7TSmmdjD=k9_OUzdm=s2CxjH8khQHm07N=Ow=Tg@mail.gmail.com> (raw)
In-Reply-To: <20250109143358.334ef58a@elisabeth>
On Thu, Jan 9, 2025 at 2:34 PM Stefano Brivio <sbrivio@redhat.com> wrote:
>
> On Thu, 9 Jan 2025 13:57:38 +0100
> Enrique Llorente Pastora <ellorent@redhat.com> wrote:
>
> > On Thu, Jan 9, 2025 at 9:16 AM Stefano Brivio <sbrivio@redhat.com> wrote:
> > >
> > > Just coding style comments, plus one functional issue, but it should be
> > > relatively contained:
> > >
> > > On Wed, 8 Jan 2025 15:58:11 +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 | 54 ++++++++++++++++++++++++++----
> > > > dhcpv6.c | 85 +++++++++++++++++++++++++++++++++++++++---------
> > > > passt.1 | 10 ++++++
> > > > passt.h | 5 +++
> > > > pasta.c | 17 +++++++---
> > > > test/lib/setup | 10 +++---
> > > > test/passt.mbuto | 6 ++--
> > > > test/passt/dhcp | 15 ++++++++-
> > > > util.c | 31 ++++++++++++++++++
> > > > util.h | 6 ++++
> > > > 11 files changed, 222 insertions(+), 37 deletions(-)
> > > >
> > > > diff --git a/conf.c b/conf.c
> > > > index df2b016..554e5c3 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))
> > >
> > > Why? This was fine in v6. I'm not sure what happened here. In case your
> > > editor/email client doesn't show this, now it's:
> > >
> > > tab tab tab tab tab tab tab tab tab tab tab tab space "%s", optarg))
> > >
> > > that is, twelve tabs and one space. See also:
> > >
> > > https://archives.passt.top/passt-dev/20250108145811.833308-1-ellorent@redhat.com/#Z31conf.c
> > >
> > > It should be:
> > >
> > > if (snprintf_check(c->hostname, PASST_MAXDNAME,
> > > "%s", optarg))
> > >
> > > tab tab tab tab tab space space space "%s", optarg))
> > >
> > > that is, five tabs and three spaces.
> > >
> >
> > Fixed, no clue how I de-formated it.
> >
> > > > + die("Invalid hostname: %s", optarg);
> > > > + break;
> > > > case '4':
> > > > v4_only = true;
> > > > v6_only = false;
> > > > diff --git a/dhcp.c b/dhcp.c
> > > > index d8515aa..50c220d 100644
> > > > --- a/dhcp.c
> > > > +++ b/dhcp.c
> > > > @@ -63,6 +63,11 @@ static struct opt opts[255];
> > > >
> > > > #define OPT_MIN 60 /* RFC 951 */
> > > >
> > > > +/* 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
> > > > +
> > > > /**
> > > > * dhcp_init() - Initialise DHCP options
> > > > */
> > > > @@ -122,7 +127,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 its length*/];
> > >
> > > Same as my comment to v6:
> > >
> > > "Spaces around /* ... */ wouldn't be bad for readability."
> > >
> >
> > Right, I understood the opposite, fixing.
> >
> > > > } __attribute__((__packed__));
> > > >
> > > > /**
> > > > @@ -130,15 +135,28 @@ 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 or -1 if option do not fit
> > >
> > > Same as my comment to v6:
> > >
> > > "doesn't fit"
> > >
> > > It's a bit weird, but harmless, that this function updates 'offset',
> > > and also returns it (unless there's an error). Maybe it saves a bit of
> > > typing in callers, though (I would have just returned offset or -1 at
> > > this point, without passing a pointer).
> >
> > Make sense to not pass a pointer to offset, I will change it.
> >
> > > > */
> > > > -static void fill_one(struct msg *m, int o, int *offset)
> > > > +static ssize_t fill_one(struct msg *m, int o, int *offset)
> > > > {
> > > > + size_t slen = opts[o].slen;
> > > > +
> > > > + /* If we don't have space to write the option, then just skip */
> > > > + if (*offset + 1 /* length of option */ + slen > OPT_MAX)
> > > > + return -1;
> > > > +
> > > > m->o[*offset] = o;
> > > > - m->o[*offset + 1] = opts[o].slen;
> > > > - memcpy(&m->o[*offset + 2], opts[o].s, opts[o].slen);
> > > > + m->o[*offset + 1] = slen;
> > > > +
> > > > + /* Move to option */
> > > > + *offset += 2;
> > > > +
> > > > + memcpy(&m->o[*offset], opts[o].s, slen);
> > > >
> > > > opts[o].sent = 1;
> > > > - *offset += 2 + opts[o].slen;
> > > > + *offset += slen;
> > > > + return *offset;
> > > > }
> > > >
> > > > /**
> > > > @@ -171,8 +189,11 @@ 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 (opts[o].slen != -1 && !opts[o].sent) {
> > > > + if (fill_one(m, o, &offset) == -1) {
> > >
> > > Curly brackets not needed.
> > >
> >
> > Done.
> >
> > > > + debug("DHCP: skipping option %i", o);
> > > > + }
> > > > + }
> > > > }
> > > >
> > > > m->o[offset++] = 255;
> > > > @@ -398,6 +419,25 @@ int dhcp(const struct ctx *c, const struct pool *p)
> > > > if (!opts[6].slen)
> > > > opts[6].slen = -1;
> > > >
> > > > + opt_len = strlen(c->hostname);
> > > > + if (opt_len > 0) {
> > > > + opts[12].slen = opt_len;
> > > > + memcpy(opts[12].s, &c->hostname, opt_len);
> > > > + }
> > > > +
> > > > + opt_len = strlen(c->fqdn);
> > > > + if (opt_len > 0) {
> > > > + size_t encoded_len;
> > > > + encoded_len = encode_domain_name((char *)opts[81].s + 3, sizeof(opts[81].s) - 3,
> > > > + c->fqdn, opt_len);
> > >
> > > Arguments are aligned properly, but it exceeds 80 columns for no
> > > compelling reason. It should be:
> > >
> > > encoded_len = encode_domain_name((char *)opts[81].s + 3,
> > > sizeof(opts[81].s) - 3,
> > > c->fqdn, opt_len);
> > >
> >
> > Done.
> >
> > > > + if (encoded_len > 0 ) {
> > >
> > > Excess whitespace between 0 and ).
> > >
> >
> > Done.
> >
> > > > + opts[81].s[0] = 0x4; /* flags (E) */
> > > > + opts[81].s[1] = 0xff; /* RCODE1 */
> > > > + opts[81].s[2] = 0xff; /* RCODE2 */
> > > > + 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..07ce768 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
> > > > + * @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;
> > > > size_t name_len = strlen(c->dns_search[i].n);
> > > >
> > > > /* We already append separators, don't duplicate if present */
> > > > @@ -388,29 +410,61 @@ 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(srch->list, NS_MAXDNAME,
> > > > + c->dns_search[i].n,
> > > > + name_len);
> > > > + srch->hdr.l += encoded_name_len;
> > >
> > > ...not if encoded_name_len is -1. In that case, you could also have an
> > > integer overflow, other than broken functionality.
> > >
> >
> > Right, I will check -1 and skip on that case.
> >
> > Also I have to move the list buffer or I will be storing it at the
> > first element.
> > encoded_name_len = encode_domain_name(srch->list + srch->hdr.l,
> > NS_MAXDNAME,
> > c->dns_search[i].n,
> > name_len);
>
> Oh, right. I didn't test this. Can't you just follow along where you
> are with 'offset'? I'm not sure, I didn't look into it.
I cannot use offset since it's initialize with + header
offset += sizeof(struct opt_hdr);
>
>
> > > > + 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,
> > > > - ".");
> > > > - }
> > > > - }
> > > > + if (srch)
> > > > 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 pool *p, const struct ctx *c,
> > > > + char *buf, int offset)
> > > > +
> > > > +{
> > > > + ssize_t opt_len;
> > > > + struct opt_client_fqdn *o;
> > > > + struct opt_client_fqdn const *req_opt;
> > >
> > > Same as my comment to v6:
> > >
> > > "These should go from longest to shortest."
> > >
> >
> > Done.
> >
> > > > +
> > > > + opt_len = MIN(PASST_MAXDNAME, OPT_MAX_SIZE - (offset + sizeof(struct opt_hdr) + 1/*flags*/));
> > >
> > > This should be PASST_MAXDNAME + 1, otherwise:
> > >
> > > ./passt -d -f --fqdn thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.and_get_to_252_with_this_one
> > >
> > > 19.4621: DHCPv6: received REQUEST/RENEW/CONFIRM, sending REPLY
> > > 19.4621: DHCPv6: client FQDN option does not fit, skipping
> > >
> >
> > Make sense I will change it
> >
> > > ...but that domain name is 252 characters, and it's accepted
> > > (longer domains are not accepted, which makes me think: should
> > > PASST_MAXDNAME be 254 instead, as it includes the terminator?).
> >
> > We are also using that const for validation (and on that case we don't
> > count the terminator) so
> > we cannot modify it
> >
> > if (snprintf_check(c->fqdn, PASST_MAXDNAME,
> > "%s", optarg))
> > die("Invalid FQDN: %s", optarg);
>
> Well, no, that was exactly my point. We do count the terminator there:
>
> $ ./passt -f --fqdn thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.then_get_to_253_with_this_one
> Invalid FQDN: thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.then_get_to_253_with_this_one
>
> because snprintf_check() is snprintf()-like, see snprintf(3):
>
> The functions snprintf() and vsnprintf() write at most size
> bytes (including the terminating null byte ('\0')) to str.
>
> Back to RFC 1035 (3.1):
>
> To simplify implementations, the total length of a domain name (i.e.,
> label octets and label length octets) is restricted to 255 octets or
> less.
>
> which means 253 characters before encoding, because we have two labels,
> at start and end. That's why we used 253.
>
> But that actually includes the terminator, so we should probably size
> everything to 254 bytes (either use PASST_MAXDNAME + 1 or define
> PASST_MAXDNAME as 254... the first one is more consistent with similar
> constants, the second one might be more practical, I'm not sure).
>
Ahh cool, I will go with defining PASST_MAXDNAME as 254 and explain
that it includes the
terminator at the const description, so we don't have to "+ 1" around
and explain every time.
> > > If it's one byte shorter, then it works (254 bytes option
> > > size, 253 bytes of encoded domain, 251 of original domain).
> > >
> > > Once this is fixed, this reveals an issue in encode_domain_name(), more
> > > on that below.
> > >
> > > By the way, this exceeds 80 columns for no particular reason, and
> > > "+ 1 /* flags */" is probably a bit more readable.
> > >
> > > > +
> > > > + o = (struct opt_client_fqdn *)(buf + offset);
> > > > + opt_len = encode_domain_name(o->domain_name, opt_len, c->fqdn, strlen(c->fqdn));
> > >
> > > This now exceeds 80 columns.
> > >
> >
> > Done.
> >
> > > > + if (opt_len == -1) {
> > > > + debug("DHCPv6: client FQDN option does not fit, skipping");
> > > > + return offset;
> > > > + }
> > > > +
> > > > + req_opt = (struct opt_client_fqdn *)dhcpv6_opt(p, &(size_t){ 0 },
> > > > + OPT_CLIENT_FQDN);
> > > > + if (req_opt && req_opt->flags & 0x01 /* S flag */)
> > > > + o->flags = 0x02 /* O flag */;
> > > > + else
> > > > + o->flags = 0x00;
> > > > +
> > > > + opt_len++;
> > > > +
> > > > + o->hdr.t = OPT_CLIENT_FQDN;
> > > > + o->hdr.l = htons(opt_len);
> > >
> > > It's much easier to follow now, I think.
> > >
> > > > +
> > > > + return offset + sizeof(struct opt_hdr) + opt_len;
> > > > +}
> > > > +
> > > > /**
> > > > * dhcpv6() - Check if this is a DHCPv6 message, reply as needed
> > > > * @c: Execution context
> > > > @@ -544,6 +598,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(p, c, (char *)&resp, n);
> > > >
> > > > resp.hdr.xid = mh->xid;
> > > >
> > > > diff --git a/passt.1 b/passt.1
> > > > index d9cd33e..7051fc4 100644
> > > > --- a/passt.1
> > > > +++ b/passt.1
> > > > @@ -401,6 +401,16 @@ 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 the client with.
> > > > +Send \fIname\fR as DHCP option 12 (hostname).
> > > > +
> > > > +.TP
> > > > +.BR \-\-fqdn " " \fIname
> > > > +FQDN to configure the client with.
> > > > +Send \fIname\fR as Client FQDN: DHCP option 81 and DHCPv6 option 39.
> > > > +
> > > > .SS \fBpasst\fR-only options
> > > >
> > > > .TP
> > > > diff --git a/passt.h b/passt.h
> > > > index 0dd4efa..f3151f0 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
> > > > @@ -269,6 +271,9 @@ struct ctx {
> > > >
> > > > 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..f3b4cae 100644
> > > > --- a/pasta.c
> > > > +++ b/pasta.c
> > > > @@ -169,10 +169,12 @@ void pasta_open_ns(struct ctx *c, const char *netns)
> > > > * struct pasta_spawn_cmd_arg - Argument for pasta_spawn_cmd()
> > > > * @exe: Executable to run
> > > > * @argv: Command and arguments to run
> > > > + * @ctx: Context to read config from
> > > > */
> > > > struct pasta_spawn_cmd_arg {
> > > > const char *exe;
> > > > char *const *argv;
> > > > + struct ctx *c;
> > > > };
> > > >
> > > > /**
> > > > @@ -186,6 +188,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 */
> > > > @@ -195,9 +198,15 @@ 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) {
> > >
> > > Same as my comment to v6, which was the same as my comment to v5...:
> > >
> > > "Same as for v5: this is an argument to gethostname(), so it should be
> > > aligned accordingly. ! is a unary operator, it doesn't take two operands.
> > >
> > > That is:
> > >
> > > } else if (!gethostname(hostname + sizeof(HOSTNAME_PREFIX) - 1,
> > > HOST_NAME_MAX + 1 - sizeof(HOSTNAME_PREFIX)) ||
> > >
> > > see also the version without your patch."
> > >
> > > > hostname[HOST_NAME_MAX] = '\0';
> > > > if (sethostname(hostname, strlen(hostname)))
> > > > warn("Unable to set pasta-prefixed hostname");
> >
> > Done.
> >
> > > > @@ -208,7 +217,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 +238,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..ba876ea 100644
> > > > --- a/util.c
> > > > +++ b/util.c
> > > > @@ -837,3 +837,34 @@ 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
> > > > + * @buf: Buffer to fill in with encoded domain name
> > > > + * @len Buffer length
> > >
> > > @len:
> > >
> >
> > Done.
> >
> > > > + * @domain_name: Input domain name
> > > > + * @domain_name_len Domain name length
> > >
> > > @domain_name_len:
> > >
> >
> > Done.
> >
> > > > + *
> > > > + * Return: encoded domain name length or -1 if it do not fit at buffer
> > >
> > > "doesn't"
> > >
> > > "fit in the buffer" (or simply "doesn't fit")
> > >
> >
> > Done.
> >
> > > > + */
> > > > +ssize_t encode_domain_name(char *buf, size_t len, const char *domain_name, size_t domain_name_len)
> > >
> > > This now exceeds 80 columns for no compelling reason.
> > >
> >
> > Done.
> >
> > > > +{
> > > > + char *p;
> > > > + size_t i;
> > >
> > > These should go from longest to shortest.
> > >
> >
> > Done.
> >
> > > > +
> > > > + if (domain_name_len + 2 > len)
> > > > + return -1;
> > > > +
> > > > + buf[0] = strcspn(domain_name, ".");
> > > > + p = buf + 1;
> > > > + for (i = 0; i < len; i++) {
> > > > + if (domain_name[i] == '.')
> > >
> > > This overflows the *input* buffer, if len > domain_name_len (which is
> > > the intended usage).
> > >
> > > I understand that if you limit len in the caller (<= PASST_MAXDNAME),
> > > then you avoid the overflow, but this looks extremely fragile (the
> > > restriction in the caller is buggy anyway), and it would leak host
> > > memory into the guest.
> > >
> > > I still think that v6 plus my suggestion (ensure adequate buffer size
> > > in the caller) would be a bit simpler.
> > >
> > > If you really want to add a check on the output buffer size here, I
> > > would suggest that you iterate over the *input* bytes anyway, and then
> > > at every byte check that you're not exceeding the output buffer.
> >
> > So your suggestion is at the caller limiting|check buffer size and not
> > pass domain_name len, so function will be:
> > size_t encode_domain_name(char* buf, const char *domain_name)
>
> Yes, just like you had.
>
> > That means that the caller has to know that the encoded len is len
> > (without terminator) + 2, is that fine ? The idea for v7
> > was encapsulating the size check at the function so we don't have to
> > repeat len + 2 check at all the callers, but maybe is
> > plain overengineering stuff.
>
> Well, the check is so simple that it's probably the same lines of code
> as checking if the return code is -1.
>
> Sure, it's nice in general to avoid duplication, but at the moment you
> have three bugs, which I don't think you introduced on purpose:
>
> - dhcp() uses encoded_len just as it is, even if it's -1 (I'm not sure
> if I reported this)
>
> - same for dhcpv6_dns_fill()
>
> - encode_domain_name() itself might overrun the *input*
>
> so perhaps it's actually less bug prone to have a simple check in the
> callers?
>
> Again, that's not the only way, you can still check the buffer size
> here, but it should be used as *check*, not to iterate on the input.
>
> Maybe implement both (it's a few lines) and see what looks simpler or
> more robust?
>
Let's go with doing the check at the caller, and if in the future we
see a lot of duplication there
we can refactor and do the check inside at the callee.
> > > > + p[i] = strcspn(domain_name + i + 1, ".");
> > > > + else
> > > > + p[i] = domain_name[i];
> > > > + }
> > > > +
> > > > + /* domain name is terminated by a length byte of zero */
> > > > + p[len + 1] = 0x00;
> > > > +
> > > > + return domain_name_len + 2;
> > > > +}
> > > > diff --git a/util.h b/util.h
> > > > index 3fa1d12..b7d5b91 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
> > >
> > > This would need tabs instead of spaces (even better: align it with the
> > > other values above, so that it becomes some kind of table).
> > >
> >
> > Done.
> >
> > > > +#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 */
> > > > +ssize_t encode_domain_name(char *buf, size_t len, const char *domain_name, size_t domain_name_len);
> > >
> > > This exceeds 80 columns and can conveniently be wrapped.
> > >
> > > > +
> > > > #endif /* UTIL_H */
>
> --
> Stefano
>
--
Quique Llorente
CNV networking Senior Software Engineer
Red Hat EMEA
ellorent@redhat.com
@RedHat Red Hat Red Hat
next prev parent reply other threads:[~2025-01-10 8:39 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-01-08 14:58 [PATCH v7] dhcp, dhcpv6: Add hostname and client fqdn ops Enrique Llorente
2025-01-09 8:16 ` Stefano Brivio
2025-01-09 12:57 ` Enrique Llorente Pastora
2025-01-09 13:33 ` Stefano Brivio
2025-01-10 8:39 ` Enrique Llorente Pastora [this message]
2025-01-10 8:59 ` Stefano Brivio
2025-01-10 9:44 ` Enrique Llorente Pastora
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAHVoYmK6XMi7TSmmdjD=k9_OUzdm=s2CxjH8khQHm07N=Ow=Tg@mail.gmail.com' \
--to=ellorent@redhat.com \
--cc=passt-dev@passt.top \
--cc=sbrivio@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://passt.top/passt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for IMAP folder(s).