Re: vmd: add minimal DHCP support

2017-11-06 Thread Mike Larkin
On Sun, Nov 05, 2017 at 09:34:53PM +0100, Reyk Floeter wrote:
> On Thu, Nov 02, 2017 at 10:26:55PM +0100, Reyk Floeter wrote:
> > Hi,
> > 
> > the problem got reported a few times and a similar diff was floating
> > around: vmd's "local interface" implements a simple auto-magic BOOTP
> > server, but udhcpc doesn't support BOOTP, which is the predecessor and
> > a valid subset of DHCP.  udhcpc is used by busybox and many embedded
> > Linux distributions, maybe even by Android.
> > 
> > The following diff adds minimal DHCP support which works with udhcpc.
> > 
> 
> Thanks Carlos and Mike, I just committed the change.
> 
> Here is a practical example of the actual difference:
> 
> 1. Before the change in simple stateless BOOTP mode:
> 
> ```
> # dhclient vio0
> DHCPDISCOVER on vio0 - interval 1
> BOOTREPLY from 100.64.1.2 (fe:e1:bb:d1:a9:65)
> bound to 100.64.1.3 -- renewal in 8000 seconds.
> 
> # cat /var/db/dhclient.leases.vio0
> lease {
>   bootp;
>   interface "vio0";
>   fixed-address 100.64.1.3;
>   next-server 100.64.1.2;
>   option subnet-mask 255.255.255.254;
>   option routers 100.64.1.2;
>   option domain-name-servers 100.64.1.2;
>   option dhcp-lease-time 12000;
>   option dhcp-renewal-time 8000;
>   option dhcp-rebinding-time 1;
>   renew 0 2017/11/05 22:03:14 UTC;
>   rebind 0 2017/11/05 22:36:34 UTC;
>   expire 0 2017/11/05 23:09:54 UTC;
> }
> ```
> 
> 2. After the change in minimal DHCP mode:
> 
> ```
> # dhclient vio0
> DHCPDISCOVER on vio0 - interval 1
> DHCPOFFER from 100.64.1.2 (fe:e1:bb:d1:09:1f)
> DHCPREQUEST on vio0 to 255.255.255.255
> DHCPACK from 100.64.1.2 (fe:e1:bb:d1:09:1f)
> bound to 100.64.1.3 -- renewal in 2147483647 seconds.
> 
> # cat /var/db/dhclient.leases.vio0
> lease {
>   interface "vio0";
>   fixed-address 100.64.1.3;
>   next-server 100.64.1.2;
>   option subnet-mask 255.255.255.254;
>   option routers 100.64.1.2;
>   option domain-name-servers 100.64.1.2,100.64.1.2;
>   option dhcp-lease-time 4294967295;
>   option dhcp-message-type 5;
>   option dhcp-server-identifier 100.64.1.2;
>   renew 5 2085/11/23 23:02:27 UTC;
>   rebind 6 2136/12/08 07:28:03 UTC;
>   expire 4 2153/12/13 02:16:35 UTC;
> }
> ```
> 
> As the lease never changes, I'm setting the lease time to 0x,
> as RFC 2132 section 9.2 defines it as "The time is in units of
> seconds, and is specified as a 32-bit unsigned integer".  This doesn't
> seem to cause any problems on dhclient or udhpc but it would be
> interesting to find out if there is any other DHCP implementation that
> handles it as "int" ...
> 
> Reyk
> 

Thanks reyk for taking the time here to fix this.

-ml



Re: vmd: add minimal DHCP support

2017-11-05 Thread Reyk Floeter
On Thu, Nov 02, 2017 at 10:26:55PM +0100, Reyk Floeter wrote:
> Hi,
> 
> the problem got reported a few times and a similar diff was floating
> around: vmd's "local interface" implements a simple auto-magic BOOTP
> server, but udhcpc doesn't support BOOTP, which is the predecessor and
> a valid subset of DHCP.  udhcpc is used by busybox and many embedded
> Linux distributions, maybe even by Android.
> 
> The following diff adds minimal DHCP support which works with udhcpc.
> 

Thanks Carlos and Mike, I just committed the change.

Here is a practical example of the actual difference:

1. Before the change in simple stateless BOOTP mode:

```
# dhclient vio0
DHCPDISCOVER on vio0 - interval 1
BOOTREPLY from 100.64.1.2 (fe:e1:bb:d1:a9:65)
bound to 100.64.1.3 -- renewal in 8000 seconds.

# cat /var/db/dhclient.leases.vio0
lease {
  bootp;
  interface "vio0";
  fixed-address 100.64.1.3;
  next-server 100.64.1.2;
  option subnet-mask 255.255.255.254;
  option routers 100.64.1.2;
  option domain-name-servers 100.64.1.2;
  option dhcp-lease-time 12000;
  option dhcp-renewal-time 8000;
  option dhcp-rebinding-time 1;
  renew 0 2017/11/05 22:03:14 UTC;
  rebind 0 2017/11/05 22:36:34 UTC;
  expire 0 2017/11/05 23:09:54 UTC;
}
```

2. After the change in minimal DHCP mode:

```
# dhclient vio0
DHCPDISCOVER on vio0 - interval 1
DHCPOFFER from 100.64.1.2 (fe:e1:bb:d1:09:1f)
DHCPREQUEST on vio0 to 255.255.255.255
DHCPACK from 100.64.1.2 (fe:e1:bb:d1:09:1f)
bound to 100.64.1.3 -- renewal in 2147483647 seconds.

# cat /var/db/dhclient.leases.vio0
lease {
  interface "vio0";
  fixed-address 100.64.1.3;
  next-server 100.64.1.2;
  option subnet-mask 255.255.255.254;
  option routers 100.64.1.2;
  option domain-name-servers 100.64.1.2,100.64.1.2;
  option dhcp-lease-time 4294967295;
  option dhcp-message-type 5;
  option dhcp-server-identifier 100.64.1.2;
  renew 5 2085/11/23 23:02:27 UTC;
  rebind 6 2136/12/08 07:28:03 UTC;
  expire 4 2153/12/13 02:16:35 UTC;
}
```

As the lease never changes, I'm setting the lease time to 0x,
as RFC 2132 section 9.2 defines it as "The time is in units of
seconds, and is specified as a 32-bit unsigned integer".  This doesn't
seem to cause any problems on dhclient or udhpc but it would be
interesting to find out if there is any other DHCP implementation that
handles it as "int" ...

Reyk



Re: vmd: add minimal DHCP support

2017-11-04 Thread Mike Larkin
On Thu, Nov 02, 2017 at 10:26:55PM +0100, Reyk Floeter wrote:
> Hi,
> 
> the problem got reported a few times and a similar diff was floating
> around: vmd's "local interface" implements a simple auto-magic BOOTP
> server, but udhcpc doesn't support BOOTP, which is the predecessor and
> a valid subset of DHCP.  udhcpc is used by busybox and many embedded
> Linux distributions, maybe even by Android.
> 
> The following diff adds minimal DHCP support which works with udhcpc.
> 
> OK?
> 
> Reyk
> 

no objections here, thanks.

-ml

> Index: usr.sbin/vmd/dhcp.c
> ===
> RCS file: /cvs/src/usr.sbin/vmd/dhcp.c,v
> retrieving revision 1.3
> diff -u -p -u -p -r1.3 dhcp.c
> --- usr.sbin/vmd/dhcp.c   24 Apr 2017 07:14:27 -  1.3
> +++ usr.sbin/vmd/dhcp.c   2 Nov 2017 21:15:03 -
> @@ -22,6 +22,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -38,12 +39,13 @@ extern struct vmd *env;
>  ssize_t
>  dhcp_request(struct vionet_dev *dev, char *buf, size_t buflen, char **obuf)
>  {
> - unsigned char   *respbuf = NULL;
> + unsigned char   *respbuf = NULL, *op, *oe, dhcptype = 0;
>   ssize_t  offset, respbuflen = 0;
>   struct packet_ctxpc;
>   struct dhcp_packet   req, resp;
> - struct in_addr   in, mask;
> + struct in_addr   server_addr, mask, client_addr, requested_addr;
>   size_t   resplen, o;
> + uint32_t ltime;
>  
>   if (buflen < (ssize_t)(BOOTP_MIN_LEN + sizeof(struct ether_header)))
>   return (-1);
> @@ -76,24 +78,54 @@ dhcp_request(struct vionet_dev *dev, cha
>   if (req.ciaddr.s_addr != 0 || req.file[0] != '\0' || req.hops != 0)
>   return (-1);
>  
> + /* Get a few DHCP options (best effort as we fall back to BOOTP) */
> + if (memcmp(,
> + DHCP_OPTIONS_COOKIE, DHCP_OPTIONS_COOKIE_LEN) == 0) {
> + memset(_addr, 0, sizeof(requested_addr));
> + op = req.options + DHCP_OPTIONS_COOKIE_LEN;
> + oe = req.options + sizeof(req.options);
> + while (*op != DHO_END && op < oe) {
> + if (op[0] == DHO_PAD) {
> + op++;
> + continue;
> + }
> + if (op + 1 + op[1] >= oe)
> + break;
> + if (op[0] == DHO_DHCP_MESSAGE_TYPE &&
> + op[1] == 1)
> + dhcptype = op[2];
> + else if (op[0] == DHO_DHCP_REQUESTED_ADDRESS &&
> + op[1] == sizeof(requested_addr))
> + memcpy(_addr, [2],
> + sizeof(requested_addr));
> + op += 2 + op[1];
> + }
> + }
> +
>   memset(, 0, sizeof(resp));
>   resp.op = BOOTREPLY;
>   resp.htype = req.htype;
>   resp.hlen = req.hlen;
>   resp.xid = req.xid;
>  
> - if ((in.s_addr = vm_priv_addr(>vmd_cfg.cfg_localprefix,
> + if ((client_addr.s_addr =
> + vm_priv_addr(>vmd_cfg.cfg_localprefix,
>   dev->vm_vmid, dev->idx, 1)) == 0)
>   return (-1);
> - memcpy(, , sizeof(in));
> - memcpy((_dst)->sin_addr, , sizeof(in));
> + memcpy(, _addr,
> + sizeof(client_addr));
> + memcpy((_dst)->sin_addr, _addr,
> + sizeof(client_addr));
>   ss2sin(_dst)->sin_port = htons(CLIENT_PORT);
>  
> - if ((in.s_addr = vm_priv_addr(>vmd_cfg.cfg_localprefix,
> + if ((server_addr.s_addr =
> + vm_priv_addr(>vmd_cfg.cfg_localprefix,
>   dev->vm_vmid, dev->idx, 0)) == 0)
>   return (-1);
> - memcpy(, , sizeof(in));
> - memcpy((_src)->sin_addr, , sizeof(in));
> + memcpy(, _addr,
> + sizeof(server_addr));
> + memcpy((_src)->sin_addr, _addr,
> + sizeof(server_addr));
>   ss2sin(_src)->sin_port = htons(SERVER_PORT);
>  
>   /* Packet is already allocated */
> @@ -124,6 +156,44 @@ dhcp_request(struct vionet_dev *dev, cha
>   DHCP_OPTIONS_COOKIE, DHCP_OPTIONS_COOKIE_LEN);
>   o+= DHCP_OPTIONS_COOKIE_LEN;
>  
> + /* Did we receive a DHCP request or was it just BOOTP? */
> + if (dhcptype) {
> + /*
> +  * There is no need for a real state machine as we always
> +  * answer with the same client IP and options for the VM.
> +  */
> + if (dhcptype == DHCPDISCOVER)
> + dhcptype = DHCPOFFER;
> + else if (dhcptype == DHCPREQUEST &&
> + (requested_addr.s_addr == 0 ||
> + client_addr.s_addr == requested_addr.s_addr))
> + dhcptype = DHCPACK;
> + else
> + dhcptype = DHCPNAK;
> +
> + resp.options[o++] = 

Re: vmd: add minimal DHCP support

2017-11-02 Thread Carlos Cardenas

On 11/02/17 14:26, Reyk Floeter wrote:

Hi,

the problem got reported a few times and a similar diff was floating
around: vmd's "local interface" implements a simple auto-magic BOOTP
server, but udhcpc doesn't support BOOTP, which is the predecessor and
a valid subset of DHCP.  udhcpc is used by busybox and many embedded
Linux distributions, maybe even by Android.

The following diff adds minimal DHCP support which works with udhcpc.

OK?

Reyk



Nice patch. +1

+--+
Carlos


Index: usr.sbin/vmd/dhcp.c
===
RCS file: /cvs/src/usr.sbin/vmd/dhcp.c,v
retrieving revision 1.3
diff -u -p -u -p -r1.3 dhcp.c
--- usr.sbin/vmd/dhcp.c 24 Apr 2017 07:14:27 -  1.3
+++ usr.sbin/vmd/dhcp.c 2 Nov 2017 21:15:03 -
@@ -22,6 +22,7 @@
  #include 
  #include 
  #include 
+#include 
  
  #include 

  #include 
@@ -38,12 +39,13 @@ extern struct vmd *env;
  ssize_t
  dhcp_request(struct vionet_dev *dev, char *buf, size_t buflen, char **obuf)
  {
-   unsigned char   *respbuf = NULL;
+   unsigned char   *respbuf = NULL, *op, *oe, dhcptype = 0;
ssize_t  offset, respbuflen = 0;
struct packet_ctxpc;
struct dhcp_packet   req, resp;
-   struct in_addr   in, mask;
+   struct in_addr   server_addr, mask, client_addr, requested_addr;
size_t   resplen, o;
+   uint32_t ltime;
  
  	if (buflen < (ssize_t)(BOOTP_MIN_LEN + sizeof(struct ether_header)))

return (-1);
@@ -76,24 +78,54 @@ dhcp_request(struct vionet_dev *dev, cha
if (req.ciaddr.s_addr != 0 || req.file[0] != '\0' || req.hops != 0)
return (-1);
  
+	/* Get a few DHCP options (best effort as we fall back to BOOTP) */

+   if (memcmp(,
+   DHCP_OPTIONS_COOKIE, DHCP_OPTIONS_COOKIE_LEN) == 0) {
+   memset(_addr, 0, sizeof(requested_addr));
+   op = req.options + DHCP_OPTIONS_COOKIE_LEN;
+   oe = req.options + sizeof(req.options);
+   while (*op != DHO_END && op < oe) {
+   if (op[0] == DHO_PAD) {
+   op++;
+   continue;
+   }
+   if (op + 1 + op[1] >= oe)
+   break;
+   if (op[0] == DHO_DHCP_MESSAGE_TYPE &&
+   op[1] == 1)
+   dhcptype = op[2];
+   else if (op[0] == DHO_DHCP_REQUESTED_ADDRESS &&
+   op[1] == sizeof(requested_addr))
+   memcpy(_addr, [2],
+   sizeof(requested_addr));
+   op += 2 + op[1];
+   }
+   }
+
memset(, 0, sizeof(resp));
resp.op = BOOTREPLY;
resp.htype = req.htype;
resp.hlen = req.hlen;
resp.xid = req.xid;
  
-	if ((in.s_addr = vm_priv_addr(>vmd_cfg.cfg_localprefix,

+   if ((client_addr.s_addr =
+   vm_priv_addr(>vmd_cfg.cfg_localprefix,
dev->vm_vmid, dev->idx, 1)) == 0)
return (-1);
-   memcpy(, , sizeof(in));
-   memcpy((_dst)->sin_addr, , sizeof(in));
+   memcpy(, _addr,
+   sizeof(client_addr));
+   memcpy((_dst)->sin_addr, _addr,
+   sizeof(client_addr));
ss2sin(_dst)->sin_port = htons(CLIENT_PORT);
  
-	if ((in.s_addr = vm_priv_addr(>vmd_cfg.cfg_localprefix,

+   if ((server_addr.s_addr =
+   vm_priv_addr(>vmd_cfg.cfg_localprefix,
dev->vm_vmid, dev->idx, 0)) == 0)
return (-1);
-   memcpy(, , sizeof(in));
-   memcpy((_src)->sin_addr, , sizeof(in));
+   memcpy(, _addr,
+   sizeof(server_addr));
+   memcpy((_src)->sin_addr, _addr,
+   sizeof(server_addr));
ss2sin(_src)->sin_port = htons(SERVER_PORT);
  
  	/* Packet is already allocated */

@@ -124,6 +156,44 @@ dhcp_request(struct vionet_dev *dev, cha
DHCP_OPTIONS_COOKIE, DHCP_OPTIONS_COOKIE_LEN);
o+= DHCP_OPTIONS_COOKIE_LEN;
  
+	/* Did we receive a DHCP request or was it just BOOTP? */

+   if (dhcptype) {
+   /*
+* There is no need for a real state machine as we always
+* answer with the same client IP and options for the VM.
+*/
+   if (dhcptype == DHCPDISCOVER)
+   dhcptype = DHCPOFFER;
+   else if (dhcptype == DHCPREQUEST &&
+   (requested_addr.s_addr == 0 ||
+   client_addr.s_addr == requested_addr.s_addr))
+   dhcptype = DHCPACK;
+   else
+   dhcptype = DHCPNAK;
+
+   resp.options[o++] = DHO_DHCP_MESSAGE_TYPE;
+   resp.options[o++] = sizeof(dhcptype);
+   memcpy([o], , sizeof(dhcptype));
+

vmd: add minimal DHCP support

2017-11-02 Thread Reyk Floeter
Hi,

the problem got reported a few times and a similar diff was floating
around: vmd's "local interface" implements a simple auto-magic BOOTP
server, but udhcpc doesn't support BOOTP, which is the predecessor and
a valid subset of DHCP.  udhcpc is used by busybox and many embedded
Linux distributions, maybe even by Android.

The following diff adds minimal DHCP support which works with udhcpc.

OK?

Reyk

Index: usr.sbin/vmd/dhcp.c
===
RCS file: /cvs/src/usr.sbin/vmd/dhcp.c,v
retrieving revision 1.3
diff -u -p -u -p -r1.3 dhcp.c
--- usr.sbin/vmd/dhcp.c 24 Apr 2017 07:14:27 -  1.3
+++ usr.sbin/vmd/dhcp.c 2 Nov 2017 21:15:03 -
@@ -22,6 +22,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -38,12 +39,13 @@ extern struct vmd *env;
 ssize_t
 dhcp_request(struct vionet_dev *dev, char *buf, size_t buflen, char **obuf)
 {
-   unsigned char   *respbuf = NULL;
+   unsigned char   *respbuf = NULL, *op, *oe, dhcptype = 0;
ssize_t  offset, respbuflen = 0;
struct packet_ctxpc;
struct dhcp_packet   req, resp;
-   struct in_addr   in, mask;
+   struct in_addr   server_addr, mask, client_addr, requested_addr;
size_t   resplen, o;
+   uint32_t ltime;
 
if (buflen < (ssize_t)(BOOTP_MIN_LEN + sizeof(struct ether_header)))
return (-1);
@@ -76,24 +78,54 @@ dhcp_request(struct vionet_dev *dev, cha
if (req.ciaddr.s_addr != 0 || req.file[0] != '\0' || req.hops != 0)
return (-1);
 
+   /* Get a few DHCP options (best effort as we fall back to BOOTP) */
+   if (memcmp(,
+   DHCP_OPTIONS_COOKIE, DHCP_OPTIONS_COOKIE_LEN) == 0) {
+   memset(_addr, 0, sizeof(requested_addr));
+   op = req.options + DHCP_OPTIONS_COOKIE_LEN;
+   oe = req.options + sizeof(req.options);
+   while (*op != DHO_END && op < oe) {
+   if (op[0] == DHO_PAD) {
+   op++;
+   continue;
+   }
+   if (op + 1 + op[1] >= oe)
+   break;
+   if (op[0] == DHO_DHCP_MESSAGE_TYPE &&
+   op[1] == 1)
+   dhcptype = op[2];
+   else if (op[0] == DHO_DHCP_REQUESTED_ADDRESS &&
+   op[1] == sizeof(requested_addr))
+   memcpy(_addr, [2],
+   sizeof(requested_addr));
+   op += 2 + op[1];
+   }
+   }
+
memset(, 0, sizeof(resp));
resp.op = BOOTREPLY;
resp.htype = req.htype;
resp.hlen = req.hlen;
resp.xid = req.xid;
 
-   if ((in.s_addr = vm_priv_addr(>vmd_cfg.cfg_localprefix,
+   if ((client_addr.s_addr =
+   vm_priv_addr(>vmd_cfg.cfg_localprefix,
dev->vm_vmid, dev->idx, 1)) == 0)
return (-1);
-   memcpy(, , sizeof(in));
-   memcpy((_dst)->sin_addr, , sizeof(in));
+   memcpy(, _addr,
+   sizeof(client_addr));
+   memcpy((_dst)->sin_addr, _addr,
+   sizeof(client_addr));
ss2sin(_dst)->sin_port = htons(CLIENT_PORT);
 
-   if ((in.s_addr = vm_priv_addr(>vmd_cfg.cfg_localprefix,
+   if ((server_addr.s_addr =
+   vm_priv_addr(>vmd_cfg.cfg_localprefix,
dev->vm_vmid, dev->idx, 0)) == 0)
return (-1);
-   memcpy(, , sizeof(in));
-   memcpy((_src)->sin_addr, , sizeof(in));
+   memcpy(, _addr,
+   sizeof(server_addr));
+   memcpy((_src)->sin_addr, _addr,
+   sizeof(server_addr));
ss2sin(_src)->sin_port = htons(SERVER_PORT);
 
/* Packet is already allocated */
@@ -124,6 +156,44 @@ dhcp_request(struct vionet_dev *dev, cha
DHCP_OPTIONS_COOKIE, DHCP_OPTIONS_COOKIE_LEN);
o+= DHCP_OPTIONS_COOKIE_LEN;
 
+   /* Did we receive a DHCP request or was it just BOOTP? */
+   if (dhcptype) {
+   /*
+* There is no need for a real state machine as we always
+* answer with the same client IP and options for the VM.
+*/
+   if (dhcptype == DHCPDISCOVER)
+   dhcptype = DHCPOFFER;
+   else if (dhcptype == DHCPREQUEST &&
+   (requested_addr.s_addr == 0 ||
+   client_addr.s_addr == requested_addr.s_addr))
+   dhcptype = DHCPACK;
+   else
+   dhcptype = DHCPNAK;
+
+   resp.options[o++] = DHO_DHCP_MESSAGE_TYPE;
+   resp.options[o++] = sizeof(dhcptype);
+   memcpy([o], , sizeof(dhcptype));
+   o += sizeof(dhcptype);
+
+   /* Our lease