Re: [PATCH v5] dhcp, dhcpv6: Add hostname and client fqdn ops

[Enrique Llorente Pastora <ellorent@redhat.com>]

On Thu, Jan 2, 2025 at 11:00 PM Stefano Brivio <sbrivio@redhat.com> wrote:
>
> 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.
>

Done

> > > 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.
>

Done

> > > > +                             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.
>

I will remove the len argument and calculate inside the function, also
for your last comment I am going to tally skip the option
if there is no size for it instead of truncating the string.

> > > > +     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.
>

Agree, since we are not using the proper mechanism to extend the
option let's just truncate at
options level.



> > > > + *
> > > > + * 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