jeff_sloan at selinc.com
2015-Jun-11 20:02 UTC
[syslinux] [PATCH 0/1] Network UEFI PXE DHCP/proxyDHCP fix
from: Jeff Sloan <jeff_sloan at selinc.com>
Update UEFI PXE proxyDHCP handling.
This patch is based on commit ID 8a00e49
Modify two files to specify valid ip addresses. These files are efi/pxe.c
and efi/udp.c.
In efi/pxe.c: In net_parse_dhcp function.
If ProxyOffer has been received, start with DhcpAck packet since it is the
most complete. This requires a minimum of changes to the packet except
that it contains NULL server ip so populate server from ProxyOffer packet.
The information is loaded into pkt_v4 and then parsed.
In efi/udp.c: Both updates are in core_udp_connect and core_udp_sendto
functions.
1. Disable UseDefaultAddress, which is not complete.
2. Set StationAddress and SubnetMask to correct values from IPInfo struct
because both fields are not necessarily populated.
Signed-off-by: Jeff Sloan <jeff_sloan at selinc.com>
---
diffstat results:
pxe.c | 16 +++++++++++++++-
udp.c | 12 ++++++++++--
2 files changed, 25 insertions(+), 3 deletions(-)
Common subdirectories: orig_repo/syslinux-master/efi/i386 and
syslinux-master/efi/i386
diff -up orig_repo/syslinux-master/efi/pxe.c syslinux-master/efi/pxe.c
--- orig_repo/syslinux-master/efi/pxe.c 2015-06-09 02:50:15.000000000
-0700
+++ syslinux-master/efi/pxe.c 2015-06-10 13:45:29.000000000 -0700
@@ -150,7 +150,21 @@ void net_parse_dhcp(void)
if (mode->PxeReplyReceived)
pkt_v4 = &mode->PxeReply.Dhcpv4;
else if (mode->ProxyOfferReceived)
- pkt_v4 = &mode->ProxyOffer.Dhcpv4;
+ {
+ /*
+ * Starting with DhcpAck packet to get all ip addresses except
server ip
+ * which can be pulled from the ProxyOffer packet.
+ */
+ pkt_v4 = &mode->DhcpAck.Dhcpv4;
+
+ /*
+ * Grab the proxyserver ip from ProxyOffer packet
+ */
+ pkt_v4->BootpSiAddr[0] = mode->ProxyOffer.Dhcpv4.BootpSiAddr[0];
+ pkt_v4->BootpSiAddr[1] = mode->ProxyOffer.Dhcpv4.BootpSiAddr[1];
+ pkt_v4->BootpSiAddr[2] = mode->ProxyOffer.Dhcpv4.BootpSiAddr[2];
+ pkt_v4->BootpSiAddr[3] = mode->ProxyOffer.Dhcpv4.BootpSiAddr[3];
+ }
if (pkt_v4)
parse_dhcp(pkt_v4, pkt_len);
diff -up orig_repo/syslinux-master/efi/udp.c syslinux-master/efi/udp.c
--- orig_repo/syslinux-master/efi/udp.c 2015-06-09 02:50:15.000000000
-0700
+++ syslinux-master/efi/udp.c 2015-06-02 05:54:11.000000000 -0700
@@ -150,11 +150,15 @@ void core_udp_connect(struct pxe_pvt_ino
/* Re-use the existing local port number */
udata.StationPort = socket->net.efi.localport;
- udata.UseDefaultAddress = TRUE;
+ udata.UseDefaultAddress = FALSE;
memcpy(&udata.RemoteAddress, &ip, sizeof(ip));
udata.RemotePort = port;
udata.AcceptPromiscuous = TRUE;
udata.TimeToLive = 64;
+ ip = IPInfo.myip;
+ memcpy(&udata.StationAddress, &ip, sizeof(ip));
+ ip = IPInfo.netmask;
+ memcpy(&udata.SubnetMask, &ip, sizeof(ip));
status = core_udp_configure(udp, &udata,
L"core_udp_connect");
if (status != EFI_SUCCESS) {
@@ -372,11 +376,15 @@ void core_udp_sendto(struct pxe_pvt_inod
/* Re-use the existing local port number */
udata.StationPort = socket->net.efi.localport;
- udata.UseDefaultAddress = TRUE;
+ udata.UseDefaultAddress = FALSE;
memcpy(&udata.RemoteAddress, &ip, sizeof(ip));
udata.RemotePort = port;
udata.AcceptPromiscuous = TRUE;
udata.TimeToLive = 64;
+ ip = IPInfo.myip;
+ memcpy(&udata.StationAddress, &ip, sizeof(ip));
+ ip = IPInfo.netmask;
+ memcpy(&udata.SubnetMask, &ip, sizeof(ip));
status = core_udp_configure(udp, &udata, L"core_udp_sendto");
if (status != EFI_SUCCESS)
Common subdirectories: orig_repo/syslinux-master/efi/x86_64 and
syslinux-master/efi/x86_64
Gene Cumm
2015-Jun-12 02:17 UTC
[syslinux] [PATCH 0/1] Network UEFI PXE DHCP/proxyDHCP fix
On Thu, Jun 11, 2015 at 4:02 PM, <jeff_sloan at selinc.com> wrote:> from: Jeff Sloan <jeff_sloan at selinc.com> > > Update UEFI PXE proxyDHCP handling. > > This patch is based on commit ID 8a00e49 > > Modify two files to specify valid ip addresses. These files are efi/pxe.c > and efi/udp.c.1) In commit 2e266c35, I proposed using UseDefaultAddress. As I recall, there were clients that didn't utilize the default routing otherwise. This may require more code to distinguish subnet-local versus remote. Looks like the thread starting at http://www.syslinux.org/archives/2013-November/021039.html 2) Why not touch efi/tcp.c also which also uses UseDefaultAddress?> In efi/pxe.c: In net_parse_dhcp function. > > If ProxyOffer has been received, start with DhcpAck packet since it is the > most complete. This requires a minimum of changes to the packet except that > it contains NULL server ip so populate server from ProxyOffer packet. The > information is loaded into pkt_v4 and then parsed. > > In efi/udp.c: Both updates are in core_udp_connect and core_udp_sendto > functions. > > 1. Disable UseDefaultAddress, which is not complete.Were you able to do a packet capture? Why is it incomplete on your systems?> 2. Set StationAddress and SubnetMask to correct values from IPInfo struct > because both fields are not necessarily populated. > > Signed-off-by: Jeff Sloan <jeff_sloan at selinc.com> > --- > diffstat results: > pxe.c | 16 +++++++++++++++- > udp.c | 12 ++++++++++-- > 2 files changed, 25 insertions(+), 3 deletions(-) > > Common subdirectories: orig_repo/syslinux-master/efi/i386 and > syslinux-master/efi/i386 > diff -up orig_repo/syslinux-master/efi/pxe.c syslinux-master/efi/pxe.c > --- orig_repo/syslinux-master/efi/pxe.c 2015-06-09 02:50:15.000000000 > -0700 > +++ syslinux-master/efi/pxe.c 2015-06-10 13:45:29.000000000 -0700 > @@ -150,7 +150,21 @@ void net_parse_dhcp(void) > if (mode->PxeReplyReceived) > pkt_v4 = &mode->PxeReply.Dhcpv4; > else if (mode->ProxyOfferReceived) > - pkt_v4 = &mode->ProxyOffer.Dhcpv4; > + { > + /* > + * Starting with DhcpAck packet to get all ip addresses except > server ip > + * which can be pulled from the ProxyOffer packet. > + */ > + pkt_v4 = &mode->DhcpAck.Dhcpv4; > + > + /* > + * Grab the proxyserver ip from ProxyOffer packet > + */ > + pkt_v4->BootpSiAddr[0] = mode->ProxyOffer.Dhcpv4.BootpSiAddr[0]; > + pkt_v4->BootpSiAddr[1] = mode->ProxyOffer.Dhcpv4.BootpSiAddr[1]; > + pkt_v4->BootpSiAddr[2] = mode->ProxyOffer.Dhcpv4.BootpSiAddr[2]; > + pkt_v4->BootpSiAddr[3] = mode->ProxyOffer.Dhcpv4.BootpSiAddr[3]; > + } > > if (pkt_v4) > parse_dhcp(pkt_v4, pkt_len);1) Why modify the packet instead of parsing that data later? 2) We still need file/BootpBootFile in addition to siaddr/BootpSiAddr. 3) Perhaps we need to be more selective about parsing, perhaps adding a new parameter to parse_dhcp() specifying what packet it is, indicating what pieces are important.> diff -up orig_repo/syslinux-master/efi/udp.c syslinux-master/efi/udp.c > --- orig_repo/syslinux-master/efi/udp.c 2015-06-09 02:50:15.000000000 > -0700 > +++ syslinux-master/efi/udp.c 2015-06-02 05:54:11.000000000 -0700 > @@ -150,11 +150,15 @@ void core_udp_connect(struct pxe_pvt_ino > /* Re-use the existing local port number */ > udata.StationPort = socket->net.efi.localport; > > - udata.UseDefaultAddress = TRUE; > + udata.UseDefaultAddress = FALSE; > memcpy(&udata.RemoteAddress, &ip, sizeof(ip)); > udata.RemotePort = port; > udata.AcceptPromiscuous = TRUE; > udata.TimeToLive = 64; > + ip = IPInfo.myip; > + memcpy(&udata.StationAddress, &ip, sizeof(ip)); > + ip = IPInfo.netmask; > + memcpy(&udata.SubnetMask, &ip, sizeof(ip)); > > status = core_udp_configure(udp, &udata, L"core_udp_connect"); > if (status != EFI_SUCCESS) { > @@ -372,11 +376,15 @@ void core_udp_sendto(struct pxe_pvt_inod > /* Re-use the existing local port number */ > udata.StationPort = socket->net.efi.localport; > > - udata.UseDefaultAddress = TRUE; > + udata.UseDefaultAddress = FALSE; > memcpy(&udata.RemoteAddress, &ip, sizeof(ip)); > udata.RemotePort = port; > udata.AcceptPromiscuous = TRUE; > udata.TimeToLive = 64; > + ip = IPInfo.myip; > + memcpy(&udata.StationAddress, &ip, sizeof(ip)); > + ip = IPInfo.netmask; > + memcpy(&udata.SubnetMask, &ip, sizeof(ip)); > > status = core_udp_configure(udp, &udata, L"core_udp_sendto"); > if (status != EFI_SUCCESS) > Common subdirectories: orig_repo/syslinux-master/efi/x86_64 and > syslinux-master/efi/x86_641) This will need some more testing/review. Users that had issues prior to my commit should be involved. There will likely be more code needed. 2) Why not just reverse the previous commit? -- -Gene
jeff_sloan at selinc.com
2015-Jun-16 16:40 UTC
[syslinux] [PATCH 0/1] Network UEFI PXE DHCP/proxyDHCP fix
On Thu, Jun 11, 2015 at 9:17 PM, <gene.cumm at gmail.com> wrote:>On Thu, Jun 11, 2015 at 4:02 PM, <jeff_sloan at selinc.com> wrote: >> from: Jeff Sloan <jeff_sloan at selinc.com> >> >> Update UEFI PXE proxyDHCP handling. >> >> This patch is based on commit ID 8a00e49 >> >> Modify two files to specify valid ip addresses. These files areefi/pxe.c>> and efi/udp.c. > >1) In commit 2e266c35, I proposed using UseDefaultAddress. As I >recall, there were clients that didn't utilize the default routing >otherwise. This may require more code to distinguish subnet-local >versus remote. Looks like the thread starting at >http://www.syslinux.org/archives/2013-November/021039.htmlIt does seem that a conditional is the correct way to handle local (FALSE) v. remote (TRUE). I will add it and test as much as I can with the hardware and setup I have.>2) Why not touch efi/tcp.c also which also uses UseDefaultAddress?I was trying to keep my patch as simple as possible but once I get the above conditional tested out I will add it to tcp.c as well.>> In efi/pxe.c: In net_parse_dhcp function. >> >> If ProxyOffer has been received, start with DhcpAck packet since it isthe>> most complete. This requires a minimum of changes to the packet exceptthat>> it contains NULL server ip so populate server from ProxyOffer packet.The>> information is loaded into pkt_v4 and then parsed. >> >> In efi/udp.c: Both updates are in core_udp_connect and core_udp_sendto >> functions. >> >> 1. Disable UseDefaultAddress, which is not complete. > >Were you able to do a packet capture? Why is it incomplete on yoursystems? Strange thing is that the struct shows 0xc78cef71 for StationAddress and 0xc78cef75 for subnetmask when usedefaultaddress is set to true. Since my subnet is 10.39.31.xx, I am completely baffled as to how the default values were set but I will figure it out. I just need to do some additional digging.>> 2. Set StationAddress and SubnetMask to correct values from IPInfostruct>> because both fields are not necessarily populated. >> >> Signed-off-by: Jeff Sloan <jeff_sloan at selinc.com> >> --- >> diffstat results: >> pxe.c | 16 +++++++++++++++- >> udp.c | 12 ++++++++++-- >> 2 files changed, 25 insertions(+), 3 deletions(-) >> >> Common subdirectories: orig_repo/syslinux-master/efi/i386 and >> syslinux-master/efi/i386 >> diff -up orig_repo/syslinux-master/efi/pxe.c syslinux-master/efi/pxe.c >> --- orig_repo/syslinux-master/efi/pxe.c 2015-06-0902:50:15.000000000>> -0700 >> +++ syslinux-master/efi/pxe.c 2015-06-10 13:45:29.000000000-0700>> @@ -150,7 +150,21 @@ void net_parse_dhcp(void) >> if (mode->PxeReplyReceived) >> pkt_v4 = &mode->PxeReply.Dhcpv4; >> else if (mode->ProxyOfferReceived) >> - pkt_v4 = &mode->ProxyOffer.Dhcpv4; >> + { >> + /* >> + * Starting with DhcpAck packet to get all ip addresses except >> server ip >> + * which can be pulled from the ProxyOffer packet. >> + */ >> + pkt_v4 = &mode->DhcpAck.Dhcpv4; >> + >> + /* >> + * Grab the proxyserver ip from ProxyOffer packet >> + */ >> + pkt_v4->BootpSiAddr[0] =mode->ProxyOffer.Dhcpv4.BootpSiAddr[0];>> + pkt_v4->BootpSiAddr[1] =mode->ProxyOffer.Dhcpv4.BootpSiAddr[1];>> + pkt_v4->BootpSiAddr[2] =mode->ProxyOffer.Dhcpv4.BootpSiAddr[2];>> + pkt_v4->BootpSiAddr[3] =mode->ProxyOffer.Dhcpv4.BootpSiAddr[3];>> + } >> >> if (pkt_v4) >> parse_dhcp(pkt_v4, pkt_len); > >1) Why modify the packet instead of parsing that data later?Actually this would work well with the wrapper I mention in #3 just below but I can certainly parse it later/separately if preferred.>2) We still need file/BootpBootFile in addition to siaddr/BootpSiAddr.Noted. I will add this.>3) Perhaps we need to be more selective about parsing, perhaps adding >a new parameter to parse_dhcp() specifying what packet it is, >indicating what pieces are important.This makes a great deal of sense, however, I think an intermediate wrapper might be better so we don't break anything that uses these parse_dhcp. If I add a function that takes packet type, calls parse_dhcp as is and then strips and returns just germaine pieces we could leave the interface untouched. Or am I over complicating?>> diff -up orig_repo/syslinux-master/efi/udp.c syslinux-master/efi/udp.c >> --- orig_repo/syslinux-master/efi/udp.c 2015-06-0902:50:15.000000000>> -0700 >> +++ syslinux-master/efi/udp.c 2015-06-02 05:54:11.000000000-0700>> @@ -150,11 +150,15 @@ void core_udp_connect(struct pxe_pvt_ino >> /* Re-use the existing local port number */ >> udata.StationPort = socket->net.efi.localport; >> >> - udata.UseDefaultAddress = TRUE; >> + udata.UseDefaultAddress = FALSE; >> memcpy(&udata.RemoteAddress, &ip, sizeof(ip)); >> udata.RemotePort = port; >> udata.AcceptPromiscuous = TRUE; >> udata.TimeToLive = 64; >> + ip = IPInfo.myip; >> + memcpy(&udata.StationAddress, &ip, sizeof(ip)); >> + ip = IPInfo.netmask; >> + memcpy(&udata.SubnetMask, &ip, sizeof(ip)); >> >> status = core_udp_configure(udp, &udata, L"core_udp_connect"); >> if (status != EFI_SUCCESS) { >> @@ -372,11 +376,15 @@ void core_udp_sendto(struct pxe_pvt_inod >> /* Re-use the existing local port number */ >> udata.StationPort = socket->net.efi.localport; >> >> - udata.UseDefaultAddress = TRUE; >> + udata.UseDefaultAddress = FALSE; >> memcpy(&udata.RemoteAddress, &ip, sizeof(ip)); >> udata.RemotePort = port; >> udata.AcceptPromiscuous = TRUE; >> udata.TimeToLive = 64; >> + ip = IPInfo.myip; >> + memcpy(&udata.StationAddress, &ip, sizeof(ip)); >> + ip = IPInfo.netmask; >> + memcpy(&udata.SubnetMask, &ip, sizeof(ip)); >> >> status = core_udp_configure(udp, &udata, L"core_udp_sendto"); >> if (status != EFI_SUCCESS) >> Common subdirectories: orig_repo/syslinux-master/efi/x86_64 and >> syslinux-master/efi/x86_64 > >1) This will need some more testing/review. Users that had issues >prior to my commit should be involved. There will likely be more code >needed.I absolutely agree. I definitely do not want to break anything that currently works. That would make me very unpopular! While I test as thoroughly as possible in my environment, there are scenarios and network setups that I can't even imagine!>2) Why not just reverse the previous commit?After reviewing the entire thread you pointed me to, from November, 2013, there is a set of scenarios where it is necessary. The conditional setting should handle that. I will work on these integrating this feedback and repost as soon as possible. I welcome any additional feedback, comments, questions or concerns. I posted my network setup in the other UEFI PXE issues thread but as a reminder, I have two separate servers (a DHCP and a combined proxyDHCP/tftp), a client system and a system running Wireshark. I also have a debugger attached to the client and am using a managed eth switch for mirrored ports. Thanks! Jeff