On 2021-11-23 21:30 -07, Joel Knight <knight.j...@gmail.com> wrote:
> On Fri, Nov 19, 2021 at 1:01 PM Joel Knight <knight.j...@gmail.com> wrote:
>>
>> One thing that got missed in the refactor was that the requested-ip
>> option should not be set in a RENEWING or BINDING state (or in other
>> words, when ciaddr is set). This chunk on top of your diff also works
>> as expected (successful unicast renewal at T1).
Thanks, I had indeed missed this. I went through the RFC and found that
we MUST NOT send the server identifier in rebooting state. While here I
also made it explicit that we are not sending server identifier in
rebinding state. This was already the case since we only transition from
renewing to rebinding, but this is clearer.
>
> Hi Florian,
>
> Does the chunk below make sense?
No, this needs to be fixed in the engine, like this:
| $ git diff --relative
| diff --git engine.c engine.c
| index 3abae44afa4..ee999b27bcd 100644
| --- engine.c
| +++ engine.c
| @@ -1357,6 +1357,7 @@ state_transition(struct dhcpleased_iface *iface, enum
if_state new_state)
| else {
| /* make sure we send broadcast */
| iface->dhcp_server.s_addr = INADDR_ANY;
| + iface->server_identifier.s_addr = INADDR_ANY;
| iface->timo.tv_sec = START_EXP_BACKOFF;
| }
| request_dhcp_request(iface);
| @@ -1393,6 +1394,7 @@ state_transition(struct dhcpleased_iface *iface, enum
if_state new_state)
| case IF_REBINDING:
| if (old_state == IF_RENEWING) {
| iface->dhcp_server.s_addr = INADDR_ANY;
| + iface->server_identifier.s_addr = INADDR_ANY;
| iface->timo.tv_sec = (iface->lease_time -
| iface->rebinding_time) / 2; /* RFC 2131 4.4.5 */
| } else
| @@ -1492,7 +1494,11 @@ request_dhcp_request(struct dhcpleased_iface *iface)
| imsg_req_request.ciaddr.s_addr = 0;
| imsg_req_request.server_identifier.s_addr =
| iface->server_identifier.s_addr;
| - imsg_req_request.requested_ip.s_addr = iface->requested_ip.s_addr;
| + if (iface->state == IF_RENEWING || iface->state == IF_REBINDING)
| + imsg_req_request.requested_ip.s_addr = 0;
| + else
| + 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));
This is the complete diff.
diff --git dhcpleased.h dhcpleased.h
index b3b4938ad3c..d209d50a246 100644
--- dhcpleased.h
+++ dhcpleased.h
@@ -296,6 +296,7 @@ struct imsg_req_discover {
struct imsg_req_request {
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..ee999b27bcd 100644
--- engine.c
+++ engine.c
@@ -1357,6 +1357,7 @@ state_transition(struct dhcpleased_iface *iface, enum
if_state new_state)
else {
/* make sure we send broadcast */
iface->dhcp_server.s_addr = INADDR_ANY;
+ iface->server_identifier.s_addr = INADDR_ANY;
iface->timo.tv_sec = START_EXP_BACKOFF;
}
request_dhcp_request(iface);
@@ -1393,6 +1394,7 @@ state_transition(struct dhcpleased_iface *iface, enum
if_state new_state)
case IF_REBINDING:
if (old_state == IF_RENEWING) {
iface->dhcp_server.s_addr = INADDR_ANY;
+ iface->server_identifier.s_addr = INADDR_ANY;
iface->timo.tv_sec = (iface->lease_time -
iface->rebinding_time) / 2; /* RFC 2131 4.4.5 */
} else
@@ -1486,9 +1488,17 @@ request_dhcp_request(struct dhcpleased_iface *iface)
iface->xid = arc4random();
imsg_req_request.if_index = iface->if_index;
imsg_req_request.xid = iface->xid;
+ if (iface->state == IF_RENEWING || iface->state == IF_REBINDING)
+ imsg_req_request.ciaddr.s_addr = iface->requested_ip.s_addr;
+ else
+ imsg_req_request.ciaddr.s_addr = 0;
imsg_req_request.server_identifier.s_addr =
iface->server_identifier.s_addr;
- imsg_req_request.requested_ip.s_addr = iface->requested_ip.s_addr;
+ if (iface->state == IF_RENEWING || iface->state == IF_REBINDING)
+ imsg_req_request.requested_ip.s_addr = 0;
+ else
+ 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));
diff --git frontend.c frontend.c
index bd1e37fe952..3816a31689c 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;
@@ -91,7 +92,7 @@ struct iface *get_iface_by_id(uint32_t);
void remove_iface(uint32_t);
void set_bpfsock(int, uint32_t);
ssize_t build_packet(uint8_t, char *, uint32_t, struct
ether_addr *,
- struct in_addr *, struct in_addr *);
+ struct in_addr *, struct in_addr *, struct in_addr *);
void send_discover(struct iface *);
void send_request(struct iface *);
void bpf_send_packet(struct iface *, uint8_t *, ssize_t);
@@ -505,6 +506,8 @@ frontend_dispatch_engine(int fd, short event, void *bula)
iface = get_iface_by_id(imsg_req_request.if_index);
if (iface != NULL) {
iface->xid = imsg_req_request.xid;
+ iface->ciaddr.s_addr =
+ imsg_req_request.ciaddr.s_addr;
iface->requested_ip.s_addr =
imsg_req_request.requested_ip.s_addr;
iface->server_identifier.s_addr =
@@ -887,8 +890,8 @@ bpf_receive(int fd, short events, void *arg)
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 +929,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));
@@ -1005,12 +1009,14 @@ send_discover(struct iface *iface)
return;
}
iface->send_discover = 0;
+ iface->ciaddr.s_addr = 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);
+ &iface->ifinfo.hw_address, &iface->ciaddr, &iface->requested_ip,
+ NULL);
bpf_send_packet(iface, dhcp_packet, pkt_len);
}
@@ -1024,7 +1030,7 @@ send_request(struct iface *iface)
log_debug("DHCPREQUEST on %s", if_name == NULL ? "?" : if_name);
pkt_len = build_packet(DHCPREQUEST, if_name, iface->xid,
- &iface->ifinfo.hw_address, &iface->requested_ip,
+ &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)
--
I'm not entirely sure you are real.
