Project

General

Profile

Actions

Bug #7240

closed

adding DHCP address through libipadm fails

Added by Daniel Kimmel almost 6 years ago. Updated over 5 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
networking
Start date:
2016-07-28
Due date:
% Done:

100%

Estimated time:
Difficulty:
Medium
Tags:
needs-triage
Gerrit CR:

Description

It looks like one can configure a 2nd DHCP address using the ipadm command, so the problem is specific to the way that we're creating a DHCP address using libipadm. When the failure occurs, dhcpagent prints the following to syslog:

Aug 26 01:57:15 guild.dc1 /sbin/dhcpagent116: [ID 854203 daemon.error] ipc_event: dhcp_ipc_recv_request failed: ipc protocol violation

This message is displayed as a result of dhcpagent's call to dhcp_ipc_recv_request() returning DHCP_IPC_E_PROTO. This error comes from the dhcp_ipc_recv_msg() function in libdhcpagent. This function is responsible for reading in a DHCP message from the "IPC" file descriptor (a loopback socket). It is passed in a "base_length" argument, which is the minimum size of the structure to be read in:

static int
dhcp_ipc_recv_msg(int fd, void **msg, uint32_t base_length, int msec) { ...

In our case, "base_length" was passed in by dhcp_ipc_recv_request() as DHCP_IPC_REQUEST_SIZE, which according to the output of the dtrace script I wrote to instrument this function, has the value 59 (this will be important to recall later). The function reads two pieces of data, formatted in the following way:

----- 4-byte-length ----- -----...-- <length>-byte-msg -----...---

So the function must read the 1st 4 bytes from the fd in order to then find out how much data to read from the subsequent message. This value is stored in the "length" variable:

int                     retval;
dhcp_ipc_reply_t *ipc_msg;
uint32_t length;
retval = dhcp_ipc_timed_read(fd, &length, sizeof (uint32_t), &msec);
if (retval != DHCP_IPC_SUCCESS)
return (retval);
if (length == 0)
return (DHCP_IPC_E_PROTO);

A length of 0 would obviously be bogus, and I verified using dtrace that we are not failing here. We continue on in the function by allocating enough memory into "*msg" to read "length" bytes, and then reading those bytes.

*msg = malloc(length);
if (*msg == NULL)
return (DHCP_IPC_E_MEMORY);
retval = dhcp_ipc_timed_read(fd, *msg, length, &msec);

So far so good, we then do a sanity check on the length (which we could have done before having read the message) to ensure that the length of the message read was in fact not less than the minimum size of the expected message (base_length):

if (length < base_length) {
free(*msg);
return (DHCP_IPC_E_PROTO);
}

I verified that we're not hitting this case either. In fact, in our case, the read "length" was 63 bytes, which is > base_length (recall this was 59). We then proceed to do another sanity check on the message that was read. The structure of the message is:

struct dhcp_ipc_request {
dhcp_ipc_type_t message_type; /* type of request /
dhcp_ipc_id_t ipc_id; /
per-socket unique request id /
dhcp_data_type_t data_type; /
type of payload /
uint32_t data_length; /
size of actual data in the buffer /
char ifname[LIFNAMSIZ];
int32_t timeout; /
timeout in seconds /
uchar_t buffer1; /
dynamically extended */
};

As such, the following bit of code is making sure that the "data_length" field indicating the size of the "buffer" field makes sense given everything else we know about the size of the message"

/*
 * the data_length field is in the same place in either ipc message.
*/
ipc_msg = (dhcp_ipc_reply_t *)(*msg);
if (ipc_msg->data_length + base_length != length) {
free(*msg);
return (DHCP_IPC_E_PROTO); <<<<======== BOOM
}

This is where we're returning DHCP_IPC_E_PROTO. Deconstructing this check, it is asserting that the amount of data contained in "buffer" plus the rest of the structure (which should logically be everything up to and including the timeout field) must be equal to the total size of the data we just read in (makes sense). That should indeed be the case given that DHCP_IPC_REQUEST_SIZE (the base_size passed in) is defined as (from dhcpagent_ipc.h):

/* * since ansi c won't let us define arrays with 0 elements, the * size of the ipc request/reply structures is off-by-1; use macros.
*/

#define DHCP_IPC_REPLY_SIZE (sizeof (dhcp_ipc_reply_t) - 1)
#define DHCP_IPC_REQUEST_SIZE (sizeof (dhcp_ipc_request_t) - 1)

What might data_length be in order for this check to fail? Given that we know that base_length was 59, and that length was 63, there must be 4 bytes in "buffer", and therefore data_length must be 4. In this case, however, data_length was 0 (based on a dtrace dump of the msg), and thus the check failed. This message is bogus, the check did its job. Now what?

Let's check how the API consumer (our mgmt stack via libipadm) constructs the message in order to understand where things went wrong. This code lives in the libdhcpagent (linked from libipadm) dhcp_ipc_send_msg() function:

/* * dhcp_ipc_send_msg(): transmits a message using the agent's ipc protocol * * input: int: the file descriptor to transmit on * void *: the message to send * uint32_t: the message length * output: int: 0 on success, DHCP_IPC_E_* otherwise
*/

static int
dhcp_ipc_send_msg(int fd, void *msg, uint32_t message_length) {
struct iovec iovec2;

iovec[0].iov_base = (caddr_t)&message_length;
iovec[0].iov_len = sizeof (uint32_t);
iovec[1].iov_base = msg;
iovec[1].iov_len = message_length;
if (writev(fd, iovec, sizeof (iovec) / sizeof (*iovec)) == -1)
return (DHCP_IPC_E_WRITEV);
return (0);
}

Here, the caller (dhcp_ipc_send_request()) passes in the message in question along with the total length of the message as "message_length". The function then writes the value of "message_length" as the 1st 4 bytes, followed by the message itself. This matches what was in turn read on the other end... What was message_length in this case? What was passed in by dhcp_ipc_send_request() was DHCP_IPC_REQUEST_SIZE + request->data_length, which should make the math add up, but we know that it does not. Using dtrace, we can tell that the value passed in was 63. We know that "request->data_length" is 0 (this is confirmed both from dtrace, and based on the code in libipadm that allocates the request in i_ipadm_op_dhcp().

All of this information leads us to only one possible conclusion, which is that the libdhcpagent linked into our program, and the libdhcpagent linked into dhcpagent itself don't agree on the value of DHCP_IPC_REQUEST_SIZE. Another clue to get us farther into this investigation is that our program is a 64-bit application, while dhcpagent is 32-bit. I wrote a little C program that simply prints the value of DHCP_IPC_REQUEST_SIZE, and compiled it in 32-bit and 64-bit modes. The output:

$ ./dhcp_ipc_32
59
$ ./dhcp_ipc_64
63

This is the problem. Our program is padding the end of the structure with 4 useless bytes which the 32-bit dhcpagent doesn't know what to do with. The cause of this is that when compiled in 32-bit mode, structure sizes may be a multiple of 4-bytes and be padded accordingly, while in 64-bit mode, structure sizes may be a multiple of 8-bytes and be end-padded accordingly.

Given that the IPC mechanism here constitutes protocol between applications with potentially different memory model, the IPC data structures must not be subject to padding or alignment restrictions that are specific to one memory model.

The fix here would be to ensure the same alignment for both memory models (e.g. 4-byte), and define DHCP_IPC_REQUEST_SIZE so that it does not use sizeof, but rather offsetof (buffer).

Actions #1

Updated by Electric Monk over 5 years ago

  • Status changed from New to Closed
  • % Done changed from 0 to 100

git commit 03aa4c8d900cbdc3667ab4b43388d480324be58e

commit  03aa4c8d900cbdc3667ab4b43388d480324be58e
Author: Sebastien Roy <seb@delphix.com>
Date:   2016-08-19T22:03:12.000Z

    7240 adding DHCP address through libipadm fails
    Reviewed by: Frank Salzmann <frank@delphix.com>
    Reviewed by: Basil Crow <basil.crow@delphix.com>
    Reviewed by: Robert Mustacchi <rm@joyent.com>
    Approved by: Dan McDonald <danmcd@omniti.com>

Actions

Also available in: Atom PDF