anyone else?
On 2021-12-04 17:26 -07, Joel Knight <knight.j...@gmail.com> wrote:
> We have a winner here. I tested from INIT through to REBINDING and the
> behavior matches what's in the RFC now. ok joel@
>
> One cosmetic thing I noticed this time around: log_dhcp_hdr() in
> engine.c should be printing dhcp_hdr->xid in host byte order.
>
>
>
> .joel
>
> On Fri, Dec 3, 2021 at 5:21 AM Florian Obser <flor...@openbsd.org> wrote:
>>
>> Last one, Joel spotted one more bug in the previous one, I was missing
>> an assignment to dhcp_server in the ACK case.
>>
>> This is a rewrite of the package construction logic to send the correct
>> information in the right states:
>>
>> RFC 2131 4.3.6, Table 4
>> ---------------------------------------------------------------------
>> | |REBOOTING |REQUESTING |RENEWING |REBINDING |
>> ---------------------------------------------------------------------
>> |broad/unicast |broadcast |broadcast |unicast |broadcast |
>> |server-ip |MUST NOT |MUST |MUST NOT |MUST NOT |
>> |requested-ip |MUST |MUST |MUST NOT |MUST NOT |
>> |ciaddr |zero |zero |IP address |IP address|
>> ---------------------------------------------------------------------
>>
>> Previous this logic was all over the place and difficult to check.
>> This diff moves it all into the engine.c functions
>> request_dhcp_discover() and request_dhcp_request().
>>
>> The frontend then leaves out dhcp options that are set to INADDR_ARPA,
>> but it does not need to know in which state the engine is.
>>
>> OK?
>>
>>
>> diff --git dhcpleased.h dhcpleased.h
>> index b3b4938ad3c..6df38be6f5b 100644
>> --- dhcpleased.h
>> +++ dhcpleased.h
>> @@ -287,15 +287,10 @@ struct imsg_dhcp {
>> uint8_t packet[1500];
>> };
>>
>> -
>> -struct imsg_req_discover {
>> - uint32_t if_index;
>> - uint32_t xid;
>> -};
>> -
>> -struct imsg_req_request {
>> +struct imsg_req_dhcp {
>> uint32_t if_index;
>> uint32_t xid;
>> + struct in_addr ciaddr;
>> struct in_addr requested_ip;
>> struct in_addr server_identifier;
>> struct in_addr dhcp_server;
>> diff --git engine.c engine.c
>> index 17e65fbe789..3169bd80d40 100644
>> --- engine.c
>> +++ engine.c
>> @@ -1170,6 +1170,7 @@ parse_dhcp(struct dhcpleased_iface *iface, struct
>> imsg_dhcp *dhcp)
>> return;
>> }
>> iface->server_identifier.s_addr = server_identifier.s_addr;
>> + iface->dhcp_server.s_addr = server_identifier.s_addr;
>> iface->requested_ip.s_addr = dhcp_hdr->yiaddr.s_addr;
>> state_transition(iface, IF_REQUESTING);
>> break;
>> @@ -1228,6 +1229,7 @@ parse_dhcp(struct dhcpleased_iface *iface, struct
>> imsg_dhcp *dhcp)
>>
>> clock_gettime(CLOCK_MONOTONIC, &iface->request_time);
>> iface->server_identifier.s_addr = server_identifier.s_addr;
>> + iface->dhcp_server.s_addr = server_identifier.s_addr;
>> iface->requested_ip.s_addr = dhcp_hdr->yiaddr.s_addr;
>> iface->mask.s_addr = subnet_mask.s_addr;
>> #ifndef SMALL
>> @@ -1354,11 +1356,8 @@ state_transition(struct dhcpleased_iface *iface, enum
>> if_state new_state)
>> case IF_REBOOTING:
>> if (old_state == IF_REBOOTING)
>> iface->timo.tv_sec *= 2;
>> - else {
>> - /* make sure we send broadcast */
>> - iface->dhcp_server.s_addr = INADDR_ANY;
>> + else
>> iface->timo.tv_sec = START_EXP_BACKOFF;
>> - }
>> request_dhcp_request(iface);
>> break;
>> case IF_REQUESTING:
>> @@ -1377,10 +1376,6 @@ state_transition(struct dhcpleased_iface *iface, enum
>> if_state new_state)
>> break;
>> case IF_RENEWING:
>> if (old_state == IF_BOUND) {
>> - iface->dhcp_server.s_addr =
>> - iface->server_identifier.s_addr;
>> - iface->server_identifier.s_addr = INADDR_ANY;
>> -
>> iface->timo.tv_sec = (iface->rebinding_time -
>> iface->renewal_time) / 2; /* RFC 2131 4.4.5 */
>> } else
>> @@ -1392,7 +1387,6 @@ state_transition(struct dhcpleased_iface *iface, enum
>> if_state new_state)
>> break;
>> case IF_REBINDING:
>> if (old_state == IF_RENEWING) {
>> - iface->dhcp_server.s_addr = INADDR_ANY;
>> iface->timo.tv_sec = (iface->lease_time -
>> iface->rebinding_time) / 2; /* RFC 2131 4.4.5 */
>> } else
>> @@ -1470,28 +1464,90 @@ iface_timeout(int fd, short events, void *arg)
>> void
>> request_dhcp_discover(struct dhcpleased_iface *iface)
>> {
>> - struct imsg_req_discover imsg_req_discover;
>> + struct imsg_req_dhcp imsg;
>>
>> - imsg_req_discover.if_index = iface->if_index;
>> - imsg_req_discover.xid = iface->xid = arc4random();
>> - engine_imsg_compose_frontend(IMSG_SEND_DISCOVER, 0,
>> &imsg_req_discover,
>> - sizeof(imsg_req_discover));
>> + memset(&imsg, 0, sizeof(imsg));
>> +
>> + iface->xid = arc4random();
>> + imsg.if_index = iface->if_index;
>> + imsg.xid = iface->xid;
>> +
>> + /*
>> + * similar to RFC 2131 4.3.6, Table 4 for DHCPDISCOVER
>> + * ------------------------------
>> + * | | INIT |
>> + * ------------------------------
>> + * |broad/unicast | broadcast |
>> + * |server-ip | MUST NOT |
>> + * |requested-ip | MAY |
>> + * |ciaddr | zero |
>> + * ------------------------------
>> + *
>> + * Leaving everything at 0 from the memset results in this table with
>> + * requested-ip not set.
>> + */
>> +
>> + engine_imsg_compose_frontend(IMSG_SEND_DISCOVER, 0, &imsg,
>> sizeof(imsg));
>> }
>>
>> void
>> request_dhcp_request(struct dhcpleased_iface *iface)
>> {
>> - struct imsg_req_request imsg_req_request;
>> + struct imsg_req_dhcp imsg;
>>
>> iface->xid = arc4random();
>> - imsg_req_request.if_index = iface->if_index;
>> - imsg_req_request.xid = iface->xid;
>> - imsg_req_request.server_identifier.s_addr =
>> - iface->server_identifier.s_addr;
>> - imsg_req_request.requested_ip.s_addr = iface->requested_ip.s_addr;
>> - imsg_req_request.dhcp_server.s_addr = iface->dhcp_server.s_addr;
>> - engine_imsg_compose_frontend(IMSG_SEND_REQUEST, 0, &imsg_req_request,
>> - sizeof(imsg_req_request));
>> + imsg.if_index = iface->if_index;
>> + imsg.xid = iface->xid;
>> +
>> + /*
>> + * RFC 2131 4.3.6, Table 4
>> + *
>> ---------------------------------------------------------------------
>> + * | |REBOOTING |REQUESTING |RENEWING
>> |REBINDING |
>> + *
>> ---------------------------------------------------------------------
>> + * |broad/unicast |broadcast |broadcast |unicast
>> |broadcast |
>> + * |server-ip |MUST NOT |MUST |MUST NOT |MUST
>> NOT |
>> + * |requested-ip |MUST |MUST |MUST NOT |MUST
>> NOT |
>> + * |ciaddr |zero |zero |IP address |IP
>> address|
>> + *
>> ---------------------------------------------------------------------
>> + */
>> + switch (iface->state) {
>> + case IF_DOWN:
>> + fatalx("invalid state IF_DOWN in %s", __func__);
>> + break;
>> + case IF_INIT:
>> + fatalx("invalid state IF_INIT in %s", __func__);
>> + break;
>> + case IF_BOUND:
>> + fatalx("invalid state IF_BOUND in %s", __func__);
>> + break;
>> + case IF_REBOOTING:
>> + imsg.dhcp_server.s_addr = INADDR_ANY; /* broadcast
>> */
>> + imsg.server_identifier.s_addr = INADDR_ANY; /* MUST NOT
>> */
>> + imsg.requested_ip = iface->requested_ip; /* MUST */
>> + imsg.ciaddr.s_addr = INADDR_ANY; /* zero */
>> + break;
>> + case IF_REQUESTING:
>> + imsg.dhcp_server.s_addr = INADDR_ANY; /* broadcast
>> */
>> + imsg.server_identifier =
>> + iface->server_identifier; /* MUST */
>> + imsg.requested_ip = iface->requested_ip; /* MUST */
>> + imsg.ciaddr.s_addr = INADDR_ANY; /* zero */
>> + break;
>> + case IF_RENEWING:
>> + imsg.dhcp_server = iface->dhcp_server; /* unicast */
>> + imsg.server_identifier.s_addr = INADDR_ANY; /* MUST NOT
>> */
>> + imsg.requested_ip.s_addr = INADDR_ANY; /* MUST NOT
>> */
>> + imsg.ciaddr = iface->requested_ip; /* IP
>> address */
>> + break;
>> + case IF_REBINDING:
>> + imsg.dhcp_server.s_addr = INADDR_ANY; /* broadcast
>> */
>> + imsg.server_identifier.s_addr = INADDR_ANY; /* MUST NOT
>> */
>> + imsg.requested_ip.s_addr = INADDR_ANY; /* MUST NOT
>> */
>> + imsg.ciaddr = iface->requested_ip; /* IP
>> address */
>> + break;
>> + }
>> +
>> + engine_imsg_compose_frontend(IMSG_SEND_REQUEST, 0, &imsg,
>> sizeof(imsg));
>> }
>>
>> void
>> diff --git frontend.c frontend.c
>> index bd1e37fe952..e2c626ce5d2 100644
>> --- frontend.c
>> +++ frontend.c
>> @@ -70,6 +70,7 @@ struct iface {
>> struct imsg_ifinfo ifinfo;
>> int send_discover;
>> uint32_t xid;
>> + struct in_addr ciaddr;
>> struct in_addr requested_ip;
>> struct in_addr server_identifier;
>> struct in_addr dhcp_server;
>> @@ -90,10 +91,10 @@ int get_xflags(char *);
>> struct iface *get_iface_by_id(uint32_t);
>> void remove_iface(uint32_t);
>> void set_bpfsock(int, uint32_t);
>> +void iface_data_from_imsg(struct iface*, struct imsg_req_dhcp *);
>> ssize_t build_packet(uint8_t, char *, uint32_t, struct
>> ether_addr *,
>> - struct in_addr *, struct in_addr *);
>> -void send_discover(struct iface *);
>> -void send_request(struct iface *);
>> + struct in_addr *, struct in_addr *, struct in_addr *);
>> +void send_packet(uint8_t, struct iface *);
>> void bpf_send_packet(struct iface *, uint8_t *, ssize_t);
>> int udp_send_packet(struct iface *, uint8_t *, ssize_t);
>> #ifndef SMALL
>> @@ -480,39 +481,39 @@ frontend_dispatch_engine(int fd, short event, void
>> *bula)
>> break;
>> #endif /* SMALL */
>> case IMSG_SEND_DISCOVER: {
>> - struct imsg_req_discover imsg_req_discover;
>> - if (IMSG_DATA_SIZE(imsg) !=
>> sizeof(imsg_req_discover))
>> + struct imsg_req_dhcp imsg_req_dhcp;
>> + if (IMSG_DATA_SIZE(imsg) != sizeof(imsg_req_dhcp))
>> fatalx("%s: IMSG_SEND_DISCOVER wrong "
>> "length: %lu", __func__,
>> IMSG_DATA_SIZE(imsg));
>> - memcpy(&imsg_req_discover, imsg.data,
>> - sizeof(imsg_req_discover));
>> - iface = get_iface_by_id(imsg_req_discover.if_index);
>> - if (iface != NULL) {
>> - iface->xid = imsg_req_discover.xid;
>> - send_discover(iface);
>> - }
>> + memcpy(&imsg_req_dhcp, imsg.data,
>> + sizeof(imsg_req_dhcp));
>> +
>> + iface = get_iface_by_id(imsg_req_dhcp.if_index);
>> +
>> + if (iface == NULL)
>> + break;
>> +
>> + iface_data_from_imsg(iface, &imsg_req_dhcp);
>> + send_packet(DHCPDISCOVER, iface);
>> break;
>> }
>> case IMSG_SEND_REQUEST: {
>> - struct imsg_req_request imsg_req_request;
>> - if (IMSG_DATA_SIZE(imsg) != sizeof(imsg_req_request))
>> + struct imsg_req_dhcp imsg_req_dhcp;
>> + if (IMSG_DATA_SIZE(imsg) != sizeof(imsg_req_dhcp))
>> fatalx("%s: IMSG_SEND_REQUEST wrong "
>> "length: %lu", __func__,
>> IMSG_DATA_SIZE(imsg));
>> - memcpy(&imsg_req_request, imsg.data,
>> - sizeof(imsg_req_request));
>> - iface = get_iface_by_id(imsg_req_request.if_index);
>> - if (iface != NULL) {
>> - iface->xid = imsg_req_request.xid;
>> - iface->requested_ip.s_addr =
>> - imsg_req_request.requested_ip.s_addr;
>> - iface->server_identifier.s_addr =
>> -
>> imsg_req_request.server_identifier.s_addr;
>> - iface->dhcp_server.s_addr =
>> - imsg_req_request.dhcp_server.s_addr;
>> - send_request(iface);
>> - }
>> + memcpy(&imsg_req_dhcp, imsg.data,
>> + sizeof(imsg_req_dhcp));
>> +
>> + iface = get_iface_by_id(imsg_req_dhcp.if_index);
>> +
>> + if (iface == NULL)
>> + break;
>> +
>> + iface_data_from_imsg(iface, &imsg_req_dhcp);
>> + send_packet(DHCPREQUEST, iface);
>> break;
>> }
>> default:
>> @@ -885,10 +886,20 @@ bpf_receive(int fd, short events, void *arg)
>> }
>> }
>>
>> +void
>> +iface_data_from_imsg(struct iface* iface, struct imsg_req_dhcp *imsg)
>> +{
>> + iface->xid = imsg->xid;
>> + iface->ciaddr.s_addr = imsg->ciaddr.s_addr;
>> + iface->requested_ip.s_addr = imsg->requested_ip.s_addr;
>> + iface->server_identifier.s_addr = imsg->server_identifier.s_addr;
>> + iface->dhcp_server.s_addr = imsg->dhcp_server.s_addr;
>> +}
>> +
>> ssize_t
>> build_packet(uint8_t message_type, char *if_name, uint32_t xid,
>> - struct ether_addr *hw_address, struct in_addr *requested_ip,
>> - struct in_addr *server_identifier)
>> + struct ether_addr *hw_address, struct in_addr *ciaddr, struct in_addr
>> + *requested_ip, struct in_addr *server_identifier)
>> {
>> static uint8_t dhcp_cookie[] = DHCP_COOKIE;
>> static uint8_t dhcp_message_type[] = {DHO_DHCP_MESSAGE_TYPE, 1,
>> @@ -926,6 +937,7 @@ build_packet(uint8_t message_type, char *if_name,
>> uint32_t xid,
>> hdr->hops = 0;
>> hdr->xid = xid;
>> hdr->secs = 0;
>> + hdr->ciaddr.s_addr = ciaddr->s_addr;
>> memcpy(hdr->chaddr, hw_address, sizeof(*hw_address));
>> p += sizeof(struct dhcp_hdr);
>> memcpy(p, dhcp_cookie, sizeof(dhcp_cookie));
>> @@ -966,20 +978,20 @@ build_packet(uint8_t message_type, char *if_name,
>> uint32_t xid,
>> memcpy(p, dhcp_req_list, sizeof(dhcp_req_list));
>> p += sizeof(dhcp_req_list);
>>
>> - if (message_type == DHCPREQUEST) {
>> + if (requested_ip->s_addr != INADDR_ANY) {
>> memcpy(dhcp_requested_address + 2, requested_ip,
>> sizeof(*requested_ip));
>> memcpy(p, dhcp_requested_address,
>> sizeof(dhcp_requested_address));
>> p += sizeof(dhcp_requested_address);
>> + }
>>
>> - if (server_identifier->s_addr != INADDR_ANY) {
>> - memcpy(dhcp_server_identifier + 2, server_identifier,
>> - sizeof(*server_identifier));
>> - memcpy(p, dhcp_server_identifier,
>> - sizeof(dhcp_server_identifier));
>> - p += sizeof(dhcp_server_identifier);
>> - }
>> + if (server_identifier->s_addr != INADDR_ANY) {
>> + memcpy(dhcp_server_identifier + 2, server_identifier,
>> + sizeof(*server_identifier));
>> + memcpy(p, dhcp_server_identifier,
>> + sizeof(dhcp_server_identifier));
>> + p += sizeof(dhcp_server_identifier);
>> }
>>
>> *p = DHO_END;
>> @@ -995,7 +1007,7 @@ build_packet(uint8_t message_type, char *if_name,
>> uint32_t xid,
>> }
>>
>> void
>> -send_discover(struct iface *iface)
>> +send_packet(uint8_t message_type, struct iface *iface)
>> {
>> ssize_t pkt_len;
>> char ifnamebuf[IF_NAMESIZE], *if_name;
>> @@ -1004,27 +1016,17 @@ send_discover(struct iface *iface)
>> iface->send_discover = 1;
>> return;
>> }
>> - iface->send_discover = 0;
>> -
>> - if_name = if_indextoname(iface->ifinfo.if_index, ifnamebuf);
>> - log_debug("DHCPDISCOVER on %s", if_name == NULL ? "?" : if_name);
>>
>> - pkt_len = build_packet(DHCPDISCOVER, if_name, iface->xid,
>> - &iface->ifinfo.hw_address, &iface->requested_ip, NULL);
>> - bpf_send_packet(iface, dhcp_packet, pkt_len);
>> -}
>> + iface->send_discover = 0;
>>
>> -void
>> -send_request(struct iface *iface)
>> -{
>> - ssize_t pkt_len;
>> - char ifnamebuf[IF_NAMESIZE], *if_name;
>> + if ((if_name = if_indextoname(iface->ifinfo.if_index, ifnamebuf)) ==
>> NULL)
>> + return; /* iface went away, nothing to do */
>>
>> - if_name = if_indextoname(iface->ifinfo.if_index, ifnamebuf);
>> - log_debug("DHCPREQUEST on %s", if_name == NULL ? "?" : if_name);
>> + log_debug("%s on %s", message_type == DHCPDISCOVER ? "DHCPDISCOVER" :
>> + "DHCPREQUEST", if_name);
>>
>> - pkt_len = build_packet(DHCPREQUEST, if_name, iface->xid,
>> - &iface->ifinfo.hw_address, &iface->requested_ip,
>> + pkt_len = build_packet(message_type, if_name, iface->xid,
>> + &iface->ifinfo.hw_address, &iface->ciaddr, &iface->requested_ip,
>> &iface->server_identifier);
>> if (iface->dhcp_server.s_addr != INADDR_ANY) {
>> if (udp_send_packet(iface, dhcp_packet, pkt_len) == -1)
>> @@ -1162,7 +1164,7 @@ set_bpfsock(int bpfsock, uint32_t if_index)
>> EV_PERSIST, bpf_receive, iface);
>> event_add(&iface->bpfev.ev, NULL);
>> if (iface->send_discover)
>> - send_discover(iface);
>> + send_packet(DHCPDISCOVER, iface);
>> }
>> }
>>
>>
>>
>> --
>> I'm not entirely sure you are real.
>>
>
--
I'm not entirely sure you are real.
