On Thu, Nov 28, 2013 at 10:24 PM, Celelibi <celelibi at gmail.com> wrote:> 2013/11/29, Gene Cumm <gene.cumm at gmail.com>: >> On Thu, Nov 28, 2013 at 9:47 PM, Gene Cumm <gene.cumm at gmail.com> wrote: >>> On Thu, Nov 28, 2013 at 9:34 PM, Celelibi <celelibi at gmail.com> wrote: >>>> Without an assigned source port, Transmit function assign a random new >>>> source port to the packet being sent. It thus have to be set before >>>> calling Transmit if the source port have already been decided. >>>> Conversly, we have to save the assigned port to reuse it later if >>>> needed. >>>> >>>> Resolve bug #35. >>>> >>>> Signed-off-by: Celelibi <celelibi at gmail.com> >>>> --- >>>> efi/udp.c | 18 ++++++++++++++++++ >>>> 1 file changed, 18 insertions(+) >>>> >>>> diff --git a/efi/udp.c b/efi/udp.c >>>> index 59bb426..7271f1f 100644 >>>> --- a/efi/udp.c >>>> +++ b/efi/udp.c >>>> @@ -336,6 +336,9 @@ void core_udp_sendto(struct pxe_pvt_inode *socket, >>>> const void *data, >>>> >>>> memset(&udata, 0, sizeof(udata)); >>>> >>>> + /* Re-use the existing local port number if any */ >>>> + udata.StationPort = socket->net.efi.localport; >>>> + >>> >>> As HPA noted, this shouldn't be saved here. Commit 7ec052b on my >>> branch efi-fixes (will be renamed to efi-fixes-for-mfleming once Jason >>> gives more feedback). >> >> To clarify, this code-quote reuses (needed) while the removed one >> saves and should be in core_udp_open(). Perhaps a fallback-save in >> here if the local port is somehow still NULL. >> > > If I understand the behavior of UEFI UDP network stack, the local port > choice is made by the firmware only on a call to Transmit. Thus > core_udp_open can't chose the local port, or if it does, it's a > "manual" choice, not a choice from the firmware itself. > > As I proposed in the discussion with Jason, the only ways to solve > this bug is either to wait for a call to Transmit to chose a port, or > to chose a port "by hand" in core_udp_open. > This patch I propose implements the first solution.Celelibi, Jason: Please see also my request at the bottom. As far as our TFTP traffic from a programming perspective, we need to get a fixed StationPort, regardless of StationAddress or any routing rules, send a packet to 1 RemotePort, receive a packet from another RemotePort then begin a dialog with the second RemotePort and not see other traffic. Would you agree? Outside UEFI, an open() call would typically assign a port to the calling program. When no port is specified, it shall be a "randomly" assigned port (often just the next available local port from the "random" pool). The questions would be 1) What's the spec? and 2) How closely do the three or more implementations we've observed follow the spec? UEFI_2.4_0.pdf (page references are print-labeled page numbers not PDF page numbers; function context also provided) p1377 (EFI_UDP4_PROTOCOL.Configure()) "With different parameters in UdpConfigData, Configure() can be used to bind this instance to specified port." covers the manual case. p1376 (EFI_UDP4_PROTOCOL.GetModeData()) "StationPort" "The port number to which this EFI UDPv4 Protocol instance is bound. If a client of the EFI UDPv4 Protocol does not care about the port number, set StationPort to zero. The EFI UDPv4 Protocol driver will assign a random port number to transmitted UDP packets. Ignored if AcceptAnyPort is set to TRUE." p1385 (EFI_UDP4_PROTOCOL.Transmit()) "SourcePort" "Port from which this packet is sent. It is in host byte order. If this field is set to zero when sending packets, the port that is assigned in EFI_UDP4_PROTOCOL.Configure() is used. If this field is set to zero and unbound, a call to EFI_UDP4_PROTOCOL.Transmit() will fail." So, if AcceptAnyPort == TRUE, the call to Configure() from core_udp_open() won't request a port number from the EFI firmware. As long as StationPort remains zero, each call to Transit() may (and probably should) result in a new source port. We need to keep RemotePort == 0 until we know the second RemotePort then use core_udp_connect() to use that RemotePort exclusively. I'd say the observed behavior should be expected and that it's bug(s) in how Syslinux treats EFI. Celelibi, Jason: Is there any chance you two could try my branch efi-fixes (commit d04b0edf) as I believe this solves the issues of the changing StationPort and routing difficulties of both a single subnet and multiple subnets? -- -Gene
Op 2013-11-30 om 13:12 schreef Gene Cumm: <bigsnip/>> > Is there any chance you two could try my branch efi-fixes (commit > d04b0edf) as I believe this solves the issues of the changing > StationPort and routing difficulties of both a single subnet and > multiple subnets?What is the URL of the git repository? Groeten Geert Stappers who was triggered on "my branch"
On Nov 30, 2013 7:33 PM, "Geert Stappers" <stappers at stappers.nl> wrote:> > Op 2013-11-30 om 13:12 schreef Gene Cumm: > <bigsnip/> > > > > Is there any chance you two could try my branch efi-fixes (commit > > d04b0edf) as I believe this solves the issues of the changing > > StationPort and routing difficulties of both a single subnet and > > multiple subnets? > > What is the URL of the git repository? > > > Groeten > Geert Stappers > who was triggered on "my branch"Sorry about that. The same as usual and mentioned earlier this week. git://github.com/geneC/syslinux.git efi-fixes --Gene
2013/11/30, Gene Cumm <gene.cumm at gmail.com>:> On Thu, Nov 28, 2013 at 10:24 PM, Celelibi <celelibi at gmail.com> wrote: >> 2013/11/29, Gene Cumm <gene.cumm at gmail.com>: >>> On Thu, Nov 28, 2013 at 9:47 PM, Gene Cumm <gene.cumm at gmail.com> wrote: >>>> On Thu, Nov 28, 2013 at 9:34 PM, Celelibi <celelibi at gmail.com> wrote: >>>>> Without an assigned source port, Transmit function assign a random new >>>>> source port to the packet being sent. It thus have to be set before >>>>> calling Transmit if the source port have already been decided. >>>>> Conversly, we have to save the assigned port to reuse it later if >>>>> needed. >>>>> >>>>> Resolve bug #35. >>>>> >>>>> Signed-off-by: Celelibi <celelibi at gmail.com> >>>>> --- >>>>> efi/udp.c | 18 ++++++++++++++++++ >>>>> 1 file changed, 18 insertions(+) >>>>> >>>>> diff --git a/efi/udp.c b/efi/udp.c >>>>> index 59bb426..7271f1f 100644 >>>>> --- a/efi/udp.c >>>>> +++ b/efi/udp.c >>>>> @@ -336,6 +336,9 @@ void core_udp_sendto(struct pxe_pvt_inode *socket, >>>>> const void *data, >>>>> >>>>> memset(&udata, 0, sizeof(udata)); >>>>> >>>>> + /* Re-use the existing local port number if any */ >>>>> + udata.StationPort = socket->net.efi.localport; >>>>> + >>>> >>>> As HPA noted, this shouldn't be saved here. Commit 7ec052b on my >>>> branch efi-fixes (will be renamed to efi-fixes-for-mfleming once Jason >>>> gives more feedback). >>> >>> To clarify, this code-quote reuses (needed) while the removed one >>> saves and should be in core_udp_open(). Perhaps a fallback-save in >>> here if the local port is somehow still NULL. >>> >> >> If I understand the behavior of UEFI UDP network stack, the local port >> choice is made by the firmware only on a call to Transmit. Thus >> core_udp_open can't chose the local port, or if it does, it's a >> "manual" choice, not a choice from the firmware itself. >> >> As I proposed in the discussion with Jason, the only ways to solve >> this bug is either to wait for a call to Transmit to chose a port, or >> to chose a port "by hand" in core_udp_open. >> This patch I propose implements the first solution. > > Celelibi, Jason: Please see also my request at the bottom. > > As far as our TFTP traffic from a programming perspective, we need to > get a fixed StationPort, regardless of StationAddress or any routing > rules, send a packet to 1 RemotePort, receive a packet from another > RemotePort then begin a dialog with the second RemotePort and not see > other traffic. Would you agree?That's basically how TFTP works.> > Outside UEFI, an open() call would typically assign a port to the > calling program. When no port is specified, it shall be a "randomly" > assigned port (often just the next available local port from the > "random" pool). The questions would be 1) What's the spec? and 2) How > closely do the three or more implementations we've observed follow the > spec? > > UEFI_2.4_0.pdf (page references are print-labeled page numbers not PDF > page numbers; function context also provided) > > p1377 (EFI_UDP4_PROTOCOL.Configure()) "With different parameters in > UdpConfigData, Configure() can be used to bind this instance to > specified port." covers the manual case.Yes, but it doesn't say it will chose a local port if it is not specified. And that's the whole point. Actually edk2 does, and my real hardware does too. But I see no guaranty that this behavior would hold for every compliant implementation. The only guaranty I see is that a local port will be chosen at least when Transmit is called. And actually, if AcceptAnyPort is TRUE, only a call to Transmit will allocate a port in edk2.> > p1376 (EFI_UDP4_PROTOCOL.GetModeData()) "StationPort" "The port number > to which this EFI UDPv4 Protocol instance is bound. If a client of the > EFI UDPv4 Protocol does not care about the port number, set > StationPort to zero. The EFI UDPv4 Protocol driver will assign a > random port number to transmitted UDP packets. Ignored if > AcceptAnyPort is set to TRUE." > > p1385 (EFI_UDP4_PROTOCOL.Transmit()) "SourcePort" "Port from which > this packet is sent. It is in host byte order. If this field is set to > zero when sending packets, the port that is assigned in > EFI_UDP4_PROTOCOL.Configure() is used. If this field is set to zero > and unbound, a call to EFI_UDP4_PROTOCOL.Transmit() will fail." > > So, if AcceptAnyPort == TRUE, the call to Configure() from > core_udp_open() won't request a port number from the EFI firmware. As > long as StationPort remains zero, each call to Transit() may (and > probably should) result in a new source port. We need to keep > RemotePort == 0 until we know the second RemotePort then use > core_udp_connect() to use that RemotePort exclusively. I'd say the > observed behavior should be expected and that it's bug(s) in how > Syslinux treats EFI. > > > > Celelibi, Jason: > > Is there any chance you two could try my branch efi-fixes (commit > d04b0edf) as I believe this solves the issues of the changing > StationPort and routing difficulties of both a single subnet and > multiple subnets?I have only one subnet, but I confirm your branch resolves the local port increment issue. Celelibi