On Thu, Dec 19, 2024 at 1:19 AM Stefano Brivio <sbrivio(a)redhat.com> wrote:On Wed, 18 Dec 2024 12:57:24 +0100 Enrique Llorente Pastora <ellorent(a)redhat.com> wrote:Sure, will add the comment.On Wed, Dec 18, 2024 at 12:35 AM Stefano Brivio <sbrivio(a)redhat.com> wrote:Right, that's option 255 ("End") and its length (0). Can we have a comment here actually explaining that? Say: /* 576 (RFC 2131), minus offset * of options (268), minus end * option and its length (2) */The truncation logic looks good to me (as a first step). Some comments below: On Tue, 17 Dec 2024 07:50:28 +0100 Enrique Llorente <ellorent(a)redhat.com> wrote:Done.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(a)redhat.com> --- conf.c | 23 ++++++++++++++-- dhcp.c | 39 +++++++++++++++++++++++--- dhcpv6.c | 72 +++++++++++++++++++++++++++++++++++++++--------- passt.1 | 11 ++++++++ passt.h | 6 ++++ pasta.c | 15 ++++++++-- test/lib/setup | 10 +++---- test/passt.mbuto | 6 ++-- test/passt/dhcp | 15 +++++++++- util.c | 22 +++++++++++++++ util.h | 3 ++ 11 files changed, 191 insertions(+), 31 deletions(-) diff --git a/conf.c b/conf.c index df2b016..29e4bed 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,12 @@ 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)) {Nit: arguments should be visually aligned to the function you're calling, that is, here: if (snprintf_check(c->fqdn, PASST_MAXDNAME, "%s", optarg)) {Done.+ die("Invalid fqdn: %s", optarg);FQDNDone.+ } + break; case 'd': c->debug = 1; c->quiet = 0; @@ -1727,6 +1737,12 @@ 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; @@ -1741,6 +1757,7 @@ void conf(struct ctx *c, int argc, char **argv) c->one_off = true; break; +Spurious change.My bad it's 306, I found that we are adding a pair of extra bytes after the options so that's why I am using 306 instead of 308 for checkings: m->o[offset++] = 255; m->o[offset++] = 0;case 't': case 'u': case 'T': diff --git a/dhcp.c b/dhcp.c index d8515aa..a04462b 100644 --- a/dhcp.c +++ b/dhcp.c @@ -62,6 +62,7 @@ static struct opt opts[255]; #define DHCPFORCERENEW 9 #define OPT_MIN 60 /* RFC 951 */ +#define OPT_MAX 302 /* UDP packet limit - dhcp headers */The original value for o[] below, 308, is 576 (see RFC 2131, page 10) minus 268 (offset of options within an *IP* datagram). How did you calculate 302 (+2)? I can't find out.I will do a new patch after this one to cover that.It's a logically separated change, so it can/should be a different patch but I think that we really risk breaking things, so I would only push them together. It can be before or after this change. It should be kind of simple: instead of filling up (and sending) 'm', we should declare and initialise a separate struct msg, say: struct msg r = { 0 }; and use that. We'll need to copy a few fields from the request ('htype', 'hlen', 'xid', 'flags', 'giaddr', 'chaddr', see page 28 of RFC 2131), but that's about it. If you're lost I can also submit a patch on top of this one later, let me know.By the way, this reminds me that, as I mentioned previously, we should really stop recycling the memory from the original message and simply build a new one instead. It's not so much about security (a DHCP client can anyway wreck the networking of its guest at will), but about robustness. This becomes more relevant as we fill up replies more and more.Do we do this at this change or at a follow up one ?Make sense adding that too.Then we should make it clear: OPT_MAX + 2 /* End option and length */Yes, "fill(..)" function is adding a pair of extra ones at the end so we should limit the option to make space for them. m->o[offset++] = 255; m->o[offset++] = 0;/** * dhcp_init() - Initialise DHCP options @@ -122,7 +123,7 @@ struct msg { uint8_t sname[64]; uint8_t file[128]; uint32_t magic; - uint8_t o[308]; + uint8_t o[OPT_MAX+2];Spaces around operator for consistency (OPT_MAX + 2). Should it really be + 2, though?I am going to create a new OPT_MAX_SIZE with the computation and also a new IPV6_MIN_MTU to be part of it and use that later on with MIN.Well, that would be 528... so I think it's worth it. :) But in any case, this is not IPv4. From RFC 8415: The server must be aware of the recommendations on packet sizes and the use of fragmentation as discussed in Section 5 of [RFC8200]. ...so we should start from 1280 (IPV6_MIN_MTU) bytes here, subtract the size of an IPv6 header (40), of a UDP header (8), and of the message header (sizeof(struct msg_hdr), 4). I think we should really use defined constants and sizeof() otherwise it gets confusing quite fast.Done.} __attribute__((__packed__)); /** @@ -131,14 +132,28 @@ struct msg { * @o: Option number * @offset: Current offset within options field, updated on insertion */ -static void fill_one(struct msg *m, int o, int *offset) +static int fill_one(struct msg *m, int o, int *offset)You should update the function comment to say what it returns, now that it returns something (try: 'grep Return: *.c').Done{ + size_t idx, slen = 0; + + // We cannot write even enum + len + one byte then just skipFor consistency, no "C++ style" comments. I would make it clear it's a conditional: "If we can't ..., then ..."Done.+ if (*offset + 2 > OPT_MAX) { + return OPT_MAX; + }No need for curly brackets here.Done.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;Spaces around assignment operator (idx = *offset ...).Done.+ + // Truncate if it goes beyond OPT_MAXNo C++ style comments.Done.+ if (idx + slen > OPT_MAX) { + slen = OPT_MAX - idx; + }No need for curly brackets.Done.+ memcpy(&m->o[*offset + 2], opts[o].s, slen); opts[o].sent = 1; *offset += 2 + opts[o].slen; + return *offset; } /** @@ -172,7 +187,9 @@ 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) { + break; + }No need for curly brackets. We should log a debug message, here (a warning would be more appropriate but I'm a bit wary of a malicious DHCP client exploiting this to make log files blow up). Something like: debug("DHCP: truncating after option %i", o);Done.} m->o[offset++] = 255; @@ -398,6 +415,20 @@ int dhcp(const struct ctx *c, const struct pool *p) if (!opts[6].slen) opts[6].slen = -1; + size_t hostname_len = strlen(c->hostname);No mixed declarations and code (move the hostname_len declaration at the beginning of the function).Done.+ if (hostname_len > 0) { + opts[12].slen = hostname_len; + memcpy(opts[12].s, &c->hostname, hostname_len); + } + + size_t fqdn_len = strlen(c->fqdn);Same as above.Done.+ if (fqdn_len > 0) { + opts[81].s[0] = 0x4; // flags (E)No C++ style comments.Done.+ size_t encoded_fqdn_len = encode_domain_name(c->fqdn, fqdn_len, + (char *) opts[81].s + 3);Don't mix declarations and code. I think that 'encoded_len' would be clear enough here.Done.+ opts[81].slen = encoded_fqdn_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..b61a41d 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; @@ -163,6 +164,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 as stated at RFC 4704 (always zero for us)"described by RFC 4704" (see also comments to v3).Done.+ * @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 +206,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 +217,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 +243,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 +365,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) @@ -388,29 +406,56 @@ 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; + size_t encoded_name_len = encode_domain_name(c->dns_search[i].n, name_len, srch->list);No mixed declarations and code. Wrap at 80 columns if possible.That's the max UDP packet payload size, since "offset" is referring to the payload not the whole UDP maybe we can do we can do max_len = 576 - (sizeof(struct ipv6hdr) + sizeof(udphdr)), don't know if it's worth it.+ 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);Hah, thanks, that looks much better now.} 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 = 0; + + fqdn_len = strlen(c->fqdn); + opt_hdr_len = sizeof(struct opt_hdr); + + if (offset + opt_hdr_len + fqdn_len + 1 > 508) {I'm a bit lost: why 508? Would it be possible to have this as a define using sizeof() on the relevant structs (struct udphdr, struct ipv6hdr)?" * @dlen: UDP payload length (not including UDP header)"I guess we could have it as a define (OPT_MAX_SIZE?), no need to have it in any struct. By the way, note that we have MAX() defined in util.h, so you don't need a condition and a separate statement, you can just do:-- Quique Llorente CNV networking Senior Software Engineer Red Hat EMEA ellorent(a)redhat.com @RedHat Red Hat Red Hat> > + fqdn_len = 508 - (offset + opt_hdr_len + 1);fqdn_len = MAX(fqdn_len, offset + ...); -- Stefano