[PATCH] [dhcp] Use random transaction ID to associate messages

[PATCH] [dhcp] Use random transaction ID to associate messages

[Amos Kong]

RFC2131.txt:
xid 4  Transaction ID, a random number chosen by the
       client, used by the client and server to associate
       messages and responses between a client and a
       server.
The 'xid' field is used by the client to match incoming DHCP messages
with pending requests.  A DHCP client MUST choose 'xid's in such a
way as to minimize the chance of using an 'xid' identical to one used
by another client. For example, a client may choose a different,
random initial 'xid' each time the client is rebooted, and
subsequently use sequential 'xid's until the next reboot.  Selecting
a new 'xid' for each retransmission is an implementation decision.  A
client may choose to reuse the same 'xid' or select a new 'xid' for
each retransmitted message.

This patch generates random id when start dhcp, and record it to
netdev struct.

Signed-off-by: Amos Kong <akong@redhat.com>
CC: Eduardo Habkost <ehabkost@redhat.com>
CC: Marty Connor <mdc@etherboot.org>
---
 src/include/gpxe/netdevice.h |    3 +++
 src/net/udp/dhcp.c           |   23 ++++-------------------
 2 files changed, 7 insertions(+), 19 deletions(-)

diff --git a/src/include/gpxe/netdevice.h b/src/include/gpxe/netdevice.h
index 97bf168..7272cf8 100644
--- a/src/include/gpxe/netdevice.h
+++ b/src/include/gpxe/netdevice.h
@@ -294,6 +294,9 @@ struct net_device {
 	/** Link-layer broadcast address */
 	const uint8_t *ll_broadcast;
 
+	/* DHCP Transaction ID */
+	uint32_t xid;
+
 	/** Current device state
 	 *
 	 * This is the bitwise-OR of zero or more NETDEV_XXX constants.
diff --git a/src/net/udp/dhcp.c b/src/net/udp/dhcp.c
index 4bfcb80..51b7150 100644
--- a/src/net/udp/dhcp.c
+++ b/src/net/udp/dhcp.c
@@ -136,23 +136,6 @@ static inline const char * dhcp_msgtype_name ( unsigned int msgtype ) {
 	}
 }
 
-/**
- * Calculate DHCP transaction ID for a network device
- *
- * @v netdev		Network device
- * @ret xid		DHCP XID
- *
- * Extract the least significant bits of the hardware address for use
- * as the transaction ID.
- */
-static uint32_t dhcp_xid ( struct net_device *netdev ) {
-	uint32_t xid;
-
-	memcpy ( &xid, ( netdev->ll_addr + netdev->ll_protocol->ll_addr_len
-			 - sizeof ( xid ) ), sizeof ( xid ) );
-	return xid;
-}
-
 /****************************************************************************
  *
  * DHCP session
@@ -1070,7 +1053,7 @@ int dhcp_create_packet ( struct dhcp_packet *dhcppkt,
 
 	/* Initialise DHCP packet content */
 	memset ( dhcphdr, 0, max_len );
-	dhcphdr->xid = dhcp_xid ( netdev );
+	dhcphdr->xid = netdev->xid;
 	dhcphdr->magic = htonl ( DHCP_MAGIC_COOKIE );
 	dhcphdr->htype = ntohs ( netdev->ll_protocol->ll_proto );
 	dhcphdr->op = dhcp_op[msgtype];
@@ -1313,7 +1296,8 @@ static int dhcp_deliver_iob ( struct xfer_interface *xfer,
 			&server_id, sizeof ( server_id ) );
 
 	/* Check for matching transaction ID */
-	if ( dhcphdr->xid != dhcp_xid ( dhcp->netdev ) ) {
+	if ( dhcphdr->xid != dhcp->netdev->xid ) {
+
 		DBGC ( dhcp, "DHCP %p %s from %s:%d has bad transaction "
 		       "ID\n", dhcp, dhcp_msgtype_name ( msgtype ),
 		       inet_ntoa ( peer->sin_addr ),
@@ -1442,6 +1426,7 @@ int start_dhcp ( struct job_interface *job, struct net_device *netdev ) {
 	dhcp = zalloc ( sizeof ( *dhcp ) );
 	if ( ! dhcp )
 		return -ENOMEM;
+	netdev->xid = random();
 	ref_init ( &dhcp->refcnt, dhcp_free );
 	job_init ( &dhcp->job, &dhcp_job_operations, &dhcp->refcnt );
 	xfer_init ( &dhcp->xfer, &dhcp_xfer_operations, &dhcp->refcnt );

Re: [PATCH] [dhcp] Use random transaction ID to associate messages

[Hagen Paul Pfeifer]

On Thu, 15 Sep 2011 21:00:38 +0800, Amos Kong <akong@redhat.com> wrote:



> +	netdev->xid = random();



This will not work for reboots. The decision that the hardware address is

choosen was not accidental. Not sure if some DHCP server will count on the

ID. (RFC 2131 "Retain DHCP client configuration across server reboots, and,

whenever possible, a DHCP client should be assigned the same configuration

parameters despite restarts of the DHCP mechanism"). If not so I am fine

with the patch.



Hagen

Re: [PATCH] [dhcp] Use random transaction ID to associate messages

[Yaniv Kaul]

----- Original Message -----
> On Thu, 15 Sep 2011 21:00:38 +0800, Amos Kong <akong@redhat.com>
> wrote:
> 
> > + netdev->xid = random();
> 
> This will not work for reboots. The decision that the hardware address
> is
> choosen was not accidental. Not sure if some DHCP server will count on
> the
> ID. (RFC 2131 "Retain DHCP client configuration across server reboots,
> and,
> whenever possible, a DHCP client should be assigned the same
> configuration
> parameters despite restarts of the DHCP mechanism"). If not so I am
> fine
> with the patch.
> 
> Hagen

But a DHCP client should be identified by its MAC, not the xid.
Y.

> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [dhcp] Use random transaction ID to associate messages

[Sasha Levin]

On Thu, 2011-09-15 at 10:13 -0400, Yaniv Kaul wrote:
> ----- Original Message -----
> > On Thu, 15 Sep 2011 21:00:38 +0800, Amos Kong <akong@redhat.com>
> > wrote:
> > 
> > > + netdev->xid = random();
> > 
> > This will not work for reboots. The decision that the hardware address
> > is
> > choosen was not accidental. Not sure if some DHCP server will count on
> > the
> > ID. (RFC 2131 "Retain DHCP client configuration across server reboots,
> > and,
> > whenever possible, a DHCP client should be assigned the same
> > configuration
> > parameters despite restarts of the DHCP mechanism"). If not so I am
> > fine
> > with the patch.
> > 
> > Hagen
> 
> But a DHCP client should be identified by its MAC, not the xid.
> Y.

DHCP server may not be aware of MAC address.

-- 

Sasha.

Re: [PATCH] [dhcp] Use random transaction ID to associate messages

[Amos Kong]

----- Original Message -----
> ----- Original Message -----
> > On Thu, 15 Sep 2011 21:00:38 +0800, Amos Kong <akong-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > wrote:
> >
> > > + netdev->xid = random();
> >
> > This will not work for reboots. The decision that the hardware
> > address
> > is
> > choosen was not accidental. Not sure if some DHCP server will count
> > on
> > the
> > ID. (RFC 2131 "Retain DHCP client configuration across server
> > reboots,
> > and,
> > whenever possible, a DHCP client should be assigned the same
> > configuration
> > parameters despite restarts of the DHCP mechanism"). If not so I am
> > fine
> > with the patch.

Hi Hagen,

rfc2131 clearly describes that we need a random xid,
I don't think xid is a port of "DHCP client configuration",
it only be used to associate messages and responses between client and server.

I would post a patch to ipxe maillist later if it's ok.

Thanks,
Amos

> But a DHCP client should be identified by its MAC, not the xid.
> Y.

Re: [PATCH] [dhcp] Use random transaction ID to associate messages

[Sasha Levin]

On Thu, 2011-09-15 at 10:43 -0400, Amos Kong wrote:
> ----- Original Message -----
> > ----- Original Message -----
> > > On Thu, 15 Sep 2011 21:00:38 +0800, Amos Kong <akong@redhat.com>
> > > wrote:
> > >
> > > > + netdev->xid = random();
> > >
> > > This will not work for reboots. The decision that the hardware
> > > address
> > > is
> > > choosen was not accidental. Not sure if some DHCP server will count
> > > on
> > > the
> > > ID. (RFC 2131 "Retain DHCP client configuration across server
> > > reboots,
> > > and,
> > > whenever possible, a DHCP client should be assigned the same
> > > configuration
> > > parameters despite restarts of the DHCP mechanism"). If not so I am
> > > fine
> > > with the patch.
> 
> Hi Hagen,
> 
> rfc2131 clearly describes that we need a random xid,
> I don't think xid is a port of "DHCP client configuration",
> it only be used to associate messages and responses between client and server.
> 
> I would post a patch to ipxe maillist later if it's ok.

rfc2131 only required that "A DHCP client MUST choose 'xid's in such a
way as to minimize the chance of using an 'xid' identical to one used
by another client.".

The 'random xid' suggestion is listed merely as an example.

The way I see it using a xid based on MAC instead of a random number is
safer since the odds for same MAC on the same network are pretty slim
since it would cause problems on other layers in the network.

Whats the reason behind this patch? Whats wrong with current selection
of xid?

-- 

Sasha.

Re: [PATCH] [dhcp] Use random transaction ID to associate messages

[Amos Kong]

Whole Archive: http://marc.info/?l=kvm&m=131609166918121&w=2

----- Original Message -----
> On Thu, 2011-09-15 at 10:43 -0400, Amos Kong wrote:
> > ----- Original Message -----
> > > ----- Original Message -----
> > > > On Thu, 15 Sep 2011 21:00:38 +0800, Amos Kong <akong@redhat.com>
> > > > wrote:
> > > >
> > > > > + netdev->xid = random();
> > > >
> > > > This will not work for reboots. The decision that the hardware
> > > > address
> > > > is
> > > > choosen was not accidental. Not sure if some DHCP server will
> > > > count
> > > > on
> > > > the
> > > > ID. (RFC 2131 "Retain DHCP client configuration across server
> > > > reboots,
> > > > and,
> > > > whenever possible, a DHCP client should be assigned the same
> > > > configuration
> > > > parameters despite restarts of the DHCP mechanism"). If not so I
> > > > am
> > > > fine
> > > > with the patch.
> >
> > Hi Hagen,
> >
> > rfc2131 clearly describes that we need a random xid,
> > I don't think xid is a port of "DHCP client configuration",
> > it only be used to associate messages and responses between client
> > and server.
> >
> > I would post a patch to ipxe maillist later if it's ok.
> 
> rfc2131 only required that "A DHCP client MUST choose 'xid's in such a
> way as to minimize the chance of using an 'xid' identical to one used
> by another client.".
> 
> The 'random xid' suggestion is listed merely as an example.
> 
> The way I see it using a xid based on MAC instead of a random number
> is
> safer since the odds for same MAC on the same network are pretty slim
> since it would cause problems on other layers in the network.

Users may boot up a QEMU guest without default mac address, it's easy to repeat.

Yaniv, what real problem do you touched? only not in accordance to the RFC?

Try to re-start host network, I can capture random dhcp idx, it's not fixed.

Amos

> Whats the reason behind this patch? Whats wrong with current selection
> of xid?

Re: [PATCH] [dhcp] Use random transaction ID to associate messages

[Eduardo Habkost]

On Thu, Sep 15, 2011 at 05:53:02PM +0300, Sasha Levin wrote:
[...]
> The 'random xid' suggestion is listed merely as an example.
> 
> The way I see it using a xid based on MAC instead of a random number is
> safer since the odds for same MAC on the same network are pretty slim
> since it would cause problems on other layers in the network.

I would agree with you if the current code didn't use just the last 4
bytes of the MAC address. So clients could have completely different MAC
addresses (as expected), have no problems communicating in the network,
but share the same final 4 bytes in the MAC address and end up
generating the same xid.

Probably a hash function that used all bytes of the MAC address as input
would work too, but using a random number seems to be good enough (and
simpler, IMO).


> 
> Whats the reason behind this patch? Whats wrong with current selection
> of xid?

I'm not sure what issue made Amos investigate the xid generation code,
but the current selection of xid is wrong as it uses just the last 4
bytes of the MAC address.

-- 
Eduardo

Re: [PATCH] [dhcp] Use random transaction ID to associate messages

[Amos Kong]

----- Original Message -----
> On Thu, 2011-09-15 at 10:43 -0400, Amos Kong wrote:
> > ----- Original Message -----
> > > ----- Original Message -----
> > > > On Thu, 15 Sep 2011 21:00:38 +0800, Amos Kong <akong@redhat.com>
> > > > wrote:
> > > >
> > > > > + netdev->xid = random();
> > > >
> > > > This will not work for reboots. The decision that the hardware
> > > > address
> > > > is
> > > > choosen was not accidental. Not sure if some DHCP server will
> > > > count
> > > > on
> > > > the
> > > > ID. (RFC 2131 "Retain DHCP client configuration across server
> > > > reboots,
> > > > and,
> > > > whenever possible, a DHCP client should be assigned the same
> > > > configuration
> > > > parameters despite restarts of the DHCP mechanism"). If not so I
> > > > am
> > > > fine
> > > > with the patch.
> >
> > Hi Hagen,
> >
> > rfc2131 clearly describes that we need a random xid,
> > I don't think xid is a port of "DHCP client configuration",
> > it only be used to associate messages and responses between client
> > and server.
> >
> > I would post a patch to ipxe maillist later if it's ok.
> 
> rfc2131 only required that "A DHCP client MUST choose 'xid's in such a
> way as to minimize the chance of using an 'xid' identical to one used
> by another client.".
> 
> The 'random xid' suggestion is listed merely as an example.

|   For example, a client may choose a different,
|   random initial 'xid' each time the client is rebooted, and
|   subsequently use sequential 'xid's until the next reboot. 

This example is not suggesting us use random xid, but give us an example
of how to use random xid.


|   xid           4  Transaction ID, a random number chosen by the
|                    client, used by the client and server to associate
|                    messages and responses between a client and a
|                    server.

This description doesn't mention random is an option.
So I think rfc suggests us to use random xid, it's necessary.

CC: droms@bucknell.edu

Amos

Re: [PATCH] [dhcp] Use random transaction ID to associate messages

[Amos Kong]

----- Original Message -----
> Whole Archive: http://marc.info/?l=kvm&m=131609166918121&w=2
> 
> ----- Original Message -----
> > On Thu, 2011-09-15 at 10:43 -0400, Amos Kong wrote:
> > > ----- Original Message -----
> > > > ----- Original Message -----
> > > > > On Thu, 15 Sep 2011 21:00:38 +0800, Amos Kong
> > > > > <akong-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > > > > wrote:
> > > > >
> > > > > > + netdev->xid = random();
> > > > >
> > > > > This will not work for reboots. The decision that the
> > > > > hardware
> > > > > address
> > > > > is
> > > > > choosen was not accidental. Not sure if some DHCP server will
> > > > > count
> > > > > on
> > > > > the
> > > > > ID. (RFC 2131 "Retain DHCP client configuration across server
> > > > > reboots,
> > > > > and,
> > > > > whenever possible, a DHCP client should be assigned the same
> > > > > configuration
> > > > > parameters despite restarts of the DHCP mechanism"). If not
> > > > > so I
> > > > > am
> > > > > fine
> > > > > with the patch.
> > >
> > > Hi Hagen,
> > >
> > > rfc2131 clearly describes that we need a random xid,
> > > I don't think xid is a port of "DHCP client configuration",
> > > it only be used to associate messages and responses between
> > > client
> > > and server.
> > >
> > > I would post a patch to ipxe maillist later if it's ok.
> > 
> > rfc2131 only required that "A DHCP client MUST choose 'xid's in
> > such a
> > way as to minimize the chance of using an 'xid' identical to one
> > used
> > by another client.".
> > 
> > The 'random xid' suggestion is listed merely as an example.
> > 
> > The way I see it using a xid based on MAC instead of a random
> > number
> > is
> > safer since the odds for same MAC on the same network are pretty
> > slim
> > since it would cause problems on other layers in the network.
> 
> Users may boot up a QEMU guest without default mac address, it's easy
> to repeat.
> 
> Yaniv, what real problem do you touched? only not in accordance to
> the RFC?

Answer from Yaniv:
Indeed. I saw some people complaining on inability to get a DHCP address, and
from the network capture saw that. We have no idea why the failed most of the
times to get an IP address with PXE boot.