[U-Boot] [PATCH] net: Fix incorrect DHCP/BOOTP packet layout on 64-bit systems
Joe Hershberger
joe.hershberger at gmail.com
Tue Mar 31 19:48:57 CEST 2015
Hi Sergey,
On Tue, Mar 31, 2015 at 12:17 PM, Sergey Temerkhanov <
s.temerkhanov at gmail.com> wrote:
>
>
> This commit fixes incorrect DHCP/BOOTP packet layout caused by
> 'ulong' type size difference on 64 and 32-bit architectures.
> It also converts protocol header structures to use explicitly
> sized fields and renames NetReadLong()/NetCopyLong() to
> NetReadU32/NetCopyU32() accordingly
>
> Signed-off-by: Radha Mohan Chintakuntla <rchintakuntla at cavium.com>
> Signed-off-by: Sergey Temerkhanov <s.temerkhanov at gmail.com>
>
> Cc: joe.hershberger at gmail.com
>
>
> ---
> include/net.h | 122
+++++++++++++++++++++++++++++-----------------------------
> net/bootp.c | 24 ++++++------
> net/bootp.h | 24 ++++++------
> 3 files changed, 85 insertions(+), 85 deletions(-)
>
> diff --git a/include/net.h b/include/net.h
> index 237c932..588ab7d 100644
> --- a/include/net.h
> +++ b/include/net.h
> @@ -183,9 +183,9 @@ u32 ether_crc(size_t len, unsigned char const *p);
> */
>
> struct ethernet_hdr {
> - uchar et_dest[6]; /* Destination node */
> - uchar et_src[6]; /* Source node */
> - ushort et_protlen; /* Protocol or length */
> + u8 et_dest[6]; /* Destination node */
> + u8 et_src[6]; /* Source node */
> + u16 et_protlen; /* Protocol or length */
> };
>
> /* Ethernet header size */
> @@ -194,16 +194,16 @@ struct ethernet_hdr {
> #define ETH_FCS_LEN 4 /* Octets in the FCS */
>
> struct e802_hdr {
> - uchar et_dest[6]; /* Destination node */
> - uchar et_src[6]; /* Source node */
> - ushort et_protlen; /* Protocol or length */
> - uchar et_dsap; /* 802 DSAP */
> - uchar et_ssap; /* 802 SSAP */
> - uchar et_ctl; /* 802 control */
> - uchar et_snap1; /* SNAP */
> - uchar et_snap2;
> - uchar et_snap3;
> - ushort et_prot; /* 802 protocol */
> + u8 et_dest[6]; /* Destination node */
> + u8 et_src[6]; /* Source node */
> + u16 et_protlen; /* Protocol or length */
> + u8 et_dsap; /* 802 DSAP */
> + u8 et_ssap; /* 802 SSAP */
> + u8 et_ctl; /* 802 control */
> + u8 et_snap1; /* SNAP */
> + u8 et_snap2;
> + u8 et_snap3;
> + u16 et_prot; /* 802 protocol */
> };
>
> /* 802 + SNAP + ethernet header size */
> @@ -213,11 +213,11 @@ struct e802_hdr {
> * Virtual LAN Ethernet header
> */
> struct vlan_ethernet_hdr {
> - uchar vet_dest[6]; /* Destination node */
> - uchar vet_src[6]; /* Source node */
> - ushort vet_vlan_type; /* PROT_VLAN */
> - ushort vet_tag; /* TAG of VLAN */
> - ushort vet_type; /* protocol type */
> + u8 vet_dest[6]; /* Destination node */
> + u8 vet_src[6]; /* Source node */
> + u16 vet_vlan_type; /* PROT_VLAN */
> + u16 vet_tag; /* TAG of VLAN */
> + u16 vet_type; /* protocol type */
> };
>
> /* VLAN Ethernet header size */
> @@ -235,14 +235,14 @@ struct vlan_ethernet_hdr {
> * Internet Protocol (IP) header.
> */
> struct ip_hdr {
> - uchar ip_hl_v; /* header length and version */
> - uchar ip_tos; /* type of service */
> - ushort ip_len; /* total length */
> - ushort ip_id; /* identification */
> - ushort ip_off; /* fragment offset field */
> - uchar ip_ttl; /* time to live */
> - uchar ip_p; /* protocol */
> - ushort ip_sum; /* checksum */
> + u8 ip_hl_v; /* header length and version */
> + u8 ip_tos; /* type of service */
> + u16 ip_len; /* total length */
> + u16 ip_id; /* identification */
> + u16 ip_off; /* fragment offset field */
> + u8 ip_ttl; /* time to live */
> + u8 ip_p; /* protocol */
> + u16 ip_sum; /* checksum */
> IPaddr_t ip_src; /* Source IP address */
> IPaddr_t ip_dst; /* Destination IP address */
> };
> @@ -259,20 +259,20 @@ struct ip_hdr {
> * Internet Protocol (IP) + UDP header.
> */
> struct ip_udp_hdr {
> - uchar ip_hl_v; /* header length and version */
> - uchar ip_tos; /* type of service */
> - ushort ip_len; /* total length */
> - ushort ip_id; /* identification */
> - ushort ip_off; /* fragment offset field */
> - uchar ip_ttl; /* time to live */
> - uchar ip_p; /* protocol */
> - ushort ip_sum; /* checksum */
> + u8 ip_hl_v; /* header length and version */
> + u8 ip_tos; /* type of service */
> + u16 ip_len; /* total length */
> + u16 ip_id; /* identification */
> + u16 ip_off; /* fragment offset field */
> + u8 ip_ttl; /* time to live */
> + u8 ip_p; /* protocol */
> + u16 ip_sum; /* checksum */
> IPaddr_t ip_src; /* Source IP address */
> IPaddr_t ip_dst; /* Destination IP address */
> - ushort udp_src; /* UDP source port */
> - ushort udp_dst; /* UDP destination port */
> - ushort udp_len; /* Length of UDP packet */
> - ushort udp_xsum; /* Checksum */
> + u16 udp_src; /* UDP source port */
> + u16 udp_dst; /* UDP destination port */
> + u16 udp_len; /* Length of UDP packet */
> + u16 udp_xsum; /* Checksum */
> };
>
> #define IP_UDP_HDR_SIZE (sizeof(struct ip_udp_hdr))
> @@ -282,14 +282,14 @@ struct ip_udp_hdr {
> * Address Resolution Protocol (ARP) header.
> */
> struct arp_hdr {
> - ushort ar_hrd; /* Format of hardware address */
> + u16 ar_hrd; /* Format of hardware address */
> # define ARP_ETHER 1 /* Ethernet hardware address */
> - ushort ar_pro; /* Format of protocol address */
> - uchar ar_hln; /* Length of hardware address */
> + u16 ar_pro; /* Format of protocol address */
> + u8 ar_hln; /* Length of hardware address */
> # define ARP_HLEN 6
> - uchar ar_pln; /* Length of protocol address */
> + u8 ar_pln; /* Length of protocol address */
> # define ARP_PLEN 4
> - ushort ar_op; /* Operation */
> + u16 ar_op; /* Operation */
> # define ARPOP_REQUEST 1 /* Request to resolve address */
> # define ARPOP_REPLY 2 /* Response to previous request */
>
> @@ -301,16 +301,16 @@ struct arp_hdr {
> * the sizes above, and are defined as appropriate for
> * specific hardware/protocol combinations.
> */
> - uchar ar_data[0];
> + u8 ar_data[0];
> #define ar_sha ar_data[0]
> #define ar_spa ar_data[ARP_HLEN]
> #define ar_tha ar_data[ARP_HLEN + ARP_PLEN]
> #define ar_tpa ar_data[ARP_HLEN + ARP_PLEN + ARP_HLEN]
> #if 0
> - uchar ar_sha[]; /* Sender hardware address */
> - uchar ar_spa[]; /* Sender protocol address */
> - uchar ar_tha[]; /* Target hardware address */
> - uchar ar_tpa[]; /* Target protocol address */
> + u8 ar_sha[]; /* Sender hardware address */
> + u8 ar_spa[]; /* Sender protocol address */
> + u8 ar_tha[]; /* Target hardware address */
> + u8 ar_tpa[]; /* Target protocol address */
> #endif /* 0 */
> };
>
> @@ -332,20 +332,20 @@ struct arp_hdr {
> #define ICMP_NOT_REACH_PORT 3 /* Port unreachable */
>
> struct icmp_hdr {
> - uchar type;
> - uchar code;
> - ushort checksum;
> + u8 type;
> + u8 code;
> + u16 checksum;
> union {
> struct {
> - ushort id;
> - ushort sequence;
> + u16 id;
> + u16 sequence;
> } echo;
> - ulong gateway;
> + u32 gateway;
> struct {
> - ushort unused;
> - ushort mtu;
> + u16 unused;
> + u16 mtu;
> } frag;
> - uchar data[0];
> + u8 data[0];
> } un;
> };
>
> @@ -608,9 +608,9 @@ static inline IPaddr_t NetReadIP(void *from)
> }
>
> /* return ulong *in network byteorder* */
> -static inline ulong NetReadLong(ulong *from)
> +static inline u32 NetReadU32(u32 *from)
> {
> - ulong l;
> + u32 l;
>
> memcpy((void *)&l, (void *)from, sizeof(l));
> return l;
> @@ -629,9 +629,9 @@ static inline void NetCopyIP(void *to, void *from)
> }
>
> /* copy ulong */
> -static inline void NetCopyLong(ulong *to, ulong *from)
> +static inline void NetCopyU32(u32 *to, u32 *from)
This surely causes a checkpatch.pl warning. While you are touching this,
you should name it "net_copy_u32()".
> {
> - memcpy((void *)to, (void *)from, sizeof(ulong));
> + memcpy((void *)to, (void *)from, sizeof(u32));
> }
>
> /**
> diff --git a/net/bootp.c b/net/bootp.c
> index 8106601..8ce0bdd 100644
> --- a/net/bootp.c
> +++ b/net/bootp.c
> @@ -51,7 +51,7 @@
> #define CONFIG_BOOTP_ID_CACHE_SIZE 4
> #endif
>
> -ulong bootp_ids[CONFIG_BOOTP_ID_CACHE_SIZE];
> +u32 bootp_ids[CONFIG_BOOTP_ID_CACHE_SIZE];
> unsigned int bootp_num_ids;
> int BootpTry;
> ulong bootp_start;
> @@ -59,7 +59,7 @@ ulong bootp_timeout;
>
> #if defined(CONFIG_CMD_DHCP)
> static dhcp_state_t dhcp_state = INIT;
> -static unsigned long dhcp_leasetime;
> +static u32 dhcp_leasetime;
> static IPaddr_t NetDHCPServerIP;
> static void DhcpHandler(uchar *pkt, unsigned dest, IPaddr_t sip,
unsigned src,
> unsigned len);
> @@ -125,7 +125,7 @@ static int BootpCheckPkt(uchar *pkt, unsigned dest,
unsigned src, unsigned len)
> retval = -4;
> else if (bp->bp_hlen != HWL_ETHER)
> retval = -5;
> - else if (!bootp_match_id(NetReadLong((ulong *)&bp->bp_id)))
> + else if (!bootp_match_id(NetReadU32(&bp->bp_id)))
> retval = -6;
>
> debug("Filtering pkt = %d\n", retval);
> @@ -350,7 +350,7 @@ BootpHandler(uchar *pkt, unsigned dest, IPaddr_t sip,
unsigned src,
> BootpCopyNetParams(bp); /* Store net parameters from
reply */
>
> /* Retrieve extended information (we must parse the vendor area)
*/
> - if (NetReadLong((ulong *)&bp->bp_vend[0]) ==
htonl(BOOTP_VENDOR_MAGIC))
> + if (NetReadU32((u32 *)&bp->bp_vend[0]) ==
htonl(BOOTP_VENDOR_MAGIC))
> BootpVendorProcess((uchar *)&bp->bp_vend[4], len);
>
> NetSetTimeout(0, (thand_f *)0);
> @@ -661,7 +661,7 @@ BootpRequest(void)
> #ifdef CONFIG_BOOTP_RANDOM_DELAY
> ulong rand_ms;
> #endif
> - ulong BootpID;
> + u32 BootpID;
Please rename this to "bootp_id".
>
> bootstage_mark_name(BOOTSTAGE_ID_BOOTP_START, "bootp_start");
> #if defined(CONFIG_CMD_DHCP)
> @@ -732,7 +732,7 @@ BootpRequest(void)
> BootpID += get_timer(0);
> BootpID = htonl(BootpID);
> bootp_add_id(BootpID);
> - NetCopyLong(&bp->bp_id, &BootpID);
> + NetCopyU32(&bp->bp_id, &BootpID);
>
> /*
> * Calculate proper packet lengths taking into account the
> @@ -770,7 +770,7 @@ static void DhcpOptionsProcess(uchar *popt, struct
Bootp_t *bp)
> #if defined(CONFIG_CMD_SNTP) && defined(CONFIG_BOOTP_TIMEOFFSET)
> case 2: /* Time offset */
> to_ptr = &NetTimeOffset;
> - NetCopyLong((ulong *)to_ptr, (ulong *)(popt + 2));
> + NetCopyU32((u32 *)to_ptr, (u32 *)(popt + 2));
> NetTimeOffset = ntohl(NetTimeOffset);
> break;
> #endif
> @@ -806,7 +806,7 @@ static void DhcpOptionsProcess(uchar *popt, struct
Bootp_t *bp)
> break;
> #endif
> case 51:
> - NetCopyLong(&dhcp_leasetime, (ulong *) (popt +
2));
> + NetCopyU32(&dhcp_leasetime, (u32 *) (popt + 2));
> break;
> case 53: /* Ignore Message Type Option */
> break;
> @@ -860,7 +860,7 @@ static void DhcpOptionsProcess(uchar *popt, struct
Bootp_t *bp)
>
> static int DhcpMessageType(unsigned char *popt)
> {
> - if (NetReadLong((ulong *)popt) != htonl(BOOTP_VENDOR_MAGIC))
> + if (NetReadU32((u32 *)popt) != htonl(BOOTP_VENDOR_MAGIC))
> return -1;
>
> popt += 4;
> @@ -911,7 +911,7 @@ static void DhcpSendRequestPkt(struct Bootp_t
*bp_offer)
> * ID is the id of the OFFER packet
> */
>
> - NetCopyLong(&bp->bp_id, &bp_offer->bp_id);
> + NetCopyU32(&bp->bp_id, &bp_offer->bp_id);
>
> /*
> * Copy options from OFFER packet if present
> @@ -970,7 +970,7 @@ DhcpHandler(uchar *pkt, unsigned dest, IPaddr_t sip,
unsigned src,
> debug("TRANSITIONING TO REQUESTING STATE\n");
> dhcp_state = REQUESTING;
>
> - if (NetReadLong((ulong *)&bp->bp_vend[0]) ==
> + if (NetReadU32((u32 *)&bp->bp_vend[0]) ==
> htonl(BOOTP_VENDOR_MAGIC))
> DhcpOptionsProcess((u8 *)&bp->bp_vend[4],
bp);
>
> @@ -986,7 +986,7 @@ DhcpHandler(uchar *pkt, unsigned dest, IPaddr_t sip,
unsigned src,
> debug("DHCP State: REQUESTING\n");
>
> if (DhcpMessageType((u8 *)bp->bp_vend) == DHCP_ACK) {
> - if (NetReadLong((ulong *)&bp->bp_vend[0]) ==
> + if (NetReadU32((u32 *)&bp->bp_vend[0]) ==
> htonl(BOOTP_VENDOR_MAGIC))
> DhcpOptionsProcess((u8 *)&bp->bp_vend[4],
bp);
> /* Store net params from reply */
> diff --git a/net/bootp.h b/net/bootp.h
> index 3b95a0a..a771655 100644
> --- a/net/bootp.h
> +++ b/net/bootp.h
> @@ -30,25 +30,25 @@ extern u8 *dhcp_vendorex_proc(u8 *e); /*rtn next e if
mine,else NULL */
> #endif
>
> struct Bootp_t {
> - uchar bp_op; /* Operation */
> + u8 bp_op; /* Operation */
> # define OP_BOOTREQUEST 1
> # define OP_BOOTREPLY 2
> - uchar bp_htype; /* Hardware type */
> + u8 bp_htype; /* Hardware type */
> # define HWT_ETHER 1
> - uchar bp_hlen; /* Hardware address length */
> + u8 bp_hlen; /* Hardware address length */
> # define HWL_ETHER 6
> - uchar bp_hops; /* Hop count (gateway thing) */
> - ulong bp_id; /* Transaction ID */
> - ushort bp_secs; /* Seconds since boot */
> - ushort bp_spare1; /* Alignment */
> + u8 bp_hops; /* Hop count (gateway thing) */
> + u32 bp_id; /* Transaction ID */
> + u16 bp_secs; /* Seconds since boot */
> + u16 bp_spare1; /* Alignment */
> IPaddr_t bp_ciaddr; /* Client IP address */
> IPaddr_t bp_yiaddr; /* Your (client) IP address */
> IPaddr_t bp_siaddr; /* Server IP address */
> IPaddr_t bp_giaddr; /* Gateway IP address */
> - uchar bp_chaddr[16]; /* Client hardware address */
> - char bp_sname[64]; /* Server host name */
> - char bp_file[128]; /* Boot file name */
> - char bp_vend[OPT_FIELD_SIZE]; /* Vendor information */
> + char bp_chaddr[16]; /* Client hardware address */
> + char bp_sname[64]; /* Server host name */
> + char bp_file[128]; /* Boot file name */
> + char bp_vend[OPT_FIELD_SIZE]; /* Vendor information */
> };
>
> #define BOOTP_HDR_SIZE sizeof(struct Bootp_t)
> @@ -59,7 +59,7 @@ struct Bootp_t {
> */
>
> /* bootp.c */
> -extern ulong BootpID; /* ID of cur BOOTP request */
> +extern u32 BootpID; /* ID of cur BOOTP request */
> extern char BootFile[128]; /* Boot file name */
> extern int BootpTry;
I like what you're doing here, but I'm doing a lot of refactoring in this
area (including most of this). I'm just going to pull this patch into the
series I'm working on right now and make the changes I suggested.
I'll be sending a refactoring series out in the next few weeks that will
include this.
Thanks,
-Joe
More information about the U-Boot
mailing list