[syslinux] [PATCH 0/1] Network UEFI PXE DHCP/proxyDHCP fix
jeff_sloan at selinc.com
jeff_sloan at selinc.com
Tue Jun 16 09:40:50 PDT 2015
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 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
It 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 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?
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 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?
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-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
>
>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
More information about the Syslinux
mailing list