On Fri, Sep 25, 2015 at 6:40 PM, Celelibi via Syslinux <syslinux at zytor.com> wrote:> 2015-09-25 21:27 UTC+02:00, Geert Stappers via Syslinux <syslinux at zytor.com>: >> On Tue, Sep 15, 2015 at 05:22:40AM -0400, Gene Cumm via Syslinux wrote: >>> On Sep 10, 2015 1:32 AM, "celelibi--- via Syslinux" <syslinux at zytor.com> >>> wrote: >>> > >>> > From: Sylvain Gault <sylvain.gault at gmail.com> >>> > >>> > Despite having native network capabilities, UEFI 2.4 (the most >>> > widely deployed at the moment) has no native DNS resolver. I propose >>> > here an implementation more or less inspired by the one found in >>> > core/legacynet/dnsresolv.c. >>> > >>> > Since it's non-trivial, I'd like to ask for a deep review of this >>> > code. I tried to make it as strong as possible against malformed >>> > packets and should be devoided of remote code execution or infinite >>> > loops (contrary to the legacynet implementation), but I may have >>> > forgotten something. I would take any comment, >>> > even on the coding style. :) >>> > >>> > I made a few tests, it worked correctly with a normal dns >>> > server. Didn't test malformed packets. >>> > >>> > Here are the two main missing features of this patch. >>> > - No caching implemented. >>> > - No timeout management. The efi implementation of core_udp_recv >>> > wait 15ms before giving up. >>> > >>> > I'd also like to ask if it wouldn't be better to just write an >>> > efi driver for lwip and just inherit all the network protocols >>> > implemented there, instead of trying to use the buggy native UEFI >>> > implementations. Depending on this, I may or may not spend time on >>> > this DNS resolver. >>> > >>> > Sylvain Gault (1): >>> > efi: Implemented a DNS stub resolver for efi >>> > >>> > efi/dns.c | 529 >>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >>> > efi/pxe.c | 16 -- 2 files changed, 529 insertions(+), 16 >>> > deletions(-) create mode 100644 efi/dns.c >>> >>> Extending like this to reestablish equality between architectures >>> definitely is a good idea. >>> >> >> How to read 'a good idea' ? Does it mean "patch accepted"??? >> > > In any case, I may submit another version (or additional patches if > this one get merged) with a few improvements, like removing the > recursion limit and retry the same servers several times as the UDP > packets may get lost. And coding style improvement.As you noted, you don't accommodate timeouts which is probably the most critical operation this doesn't have. We can't stay in the query forever. jiffies() and ms_timer() are the two available in the core. core/legacynet/dnsresolv.c used jiffies() but ms_timer() should be dealing with millisecond values, not 55ms jiffies and about CLK_TCK jiffies per second. An alternative way to just stub it is to merge everything but the actual call that starts sending UDP packets or just the most basic functionality you've implemented first. -- -Gene
2015-09-28 12:50 UTC+02:00, Gene Cumm <gene.cumm at gmail.com>:> On Fri, Sep 25, 2015 at 6:40 PM, Celelibi via Syslinux > <syslinux at zytor.com> wrote: >> 2015-09-25 21:27 UTC+02:00, Geert Stappers via Syslinux >> <syslinux at zytor.com>: >>> On Tue, Sep 15, 2015 at 05:22:40AM -0400, Gene Cumm via Syslinux wrote: >>>> On Sep 10, 2015 1:32 AM, "celelibi--- via Syslinux" >>>> <syslinux at zytor.com> >>>> wrote: >>>> > >>>> > From: Sylvain Gault <sylvain.gault at gmail.com> >>>> > >>>> > Despite having native network capabilities, UEFI 2.4 (the most >>>> > widely deployed at the moment) has no native DNS resolver. I propose >>>> > here an implementation more or less inspired by the one found in >>>> > core/legacynet/dnsresolv.c. >>>> > >>>> > Since it's non-trivial, I'd like to ask for a deep review of this >>>> > code. I tried to make it as strong as possible against malformed >>>> > packets and should be devoided of remote code execution or infinite >>>> > loops (contrary to the legacynet implementation), but I may have >>>> > forgotten something. I would take any comment, >>>> > even on the coding style. :) >>>> > >>>> > I made a few tests, it worked correctly with a normal dns >>>> > server. Didn't test malformed packets. >>>> > >>>> > Here are the two main missing features of this patch. >>>> > - No caching implemented. >>>> > - No timeout management. The efi implementation of core_udp_recv >>>> > wait 15ms before giving up. >>>> > >>>> > I'd also like to ask if it wouldn't be better to just write an >>>> > efi driver for lwip and just inherit all the network protocols >>>> > implemented there, instead of trying to use the buggy native UEFI >>>> > implementations. Depending on this, I may or may not spend time on >>>> > this DNS resolver. >>>> > >>>> > Sylvain Gault (1): >>>> > efi: Implemented a DNS stub resolver for efi >>>> > >>>> > efi/dns.c | 529 >>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >>>> > efi/pxe.c | 16 -- 2 files changed, 529 insertions(+), 16 >>>> > deletions(-) create mode 100644 efi/dns.c >>>> >>>> Extending like this to reestablish equality between architectures >>>> definitely is a good idea. >>>> >>> >>> How to read 'a good idea' ? Does it mean "patch accepted"??? >>> >> >> In any case, I may submit another version (or additional patches if >> this one get merged) with a few improvements, like removing the >> recursion limit and retry the same servers several times as the UDP >> packets may get lost. And coding style improvement. > > As you noted, you don't accommodate timeouts which is probably the > most critical operation this doesn't have. We can't stay in the query > forever. jiffies() and ms_timer() are the two available in the core. > core/legacynet/dnsresolv.c used jiffies() but ms_timer() should be > dealing with millisecond values, not 55ms jiffies and about CLK_TCK > jiffies per second.Not sure I understand everything correctly. Currently, the code won't stay in the query forever. It tries every server once and exit. I think trying every server 3 times would be better as there may be a single DNS server and UDP packets may get lost. The only way we can stay in the query forever is if we get flooded with UDP packets not for us. I didn't implement a timeout because I didn't want to make a busy loop. But since EFI is mono-threaded, I think we don't care.> > An alternative way to just stub it is to merge everything but the > actual call that starts sending UDP packets or just the most basic > functionality you've implemented first.Not sure I see the point of merging something that wouldn't work. Celelibi
On Mon, Sep 28, 2015 at 06:36:24PM +0200, Celelibi via Syslinux wrote:> 2015-09-28 12:50 UTC+02:00, Gene Cumm > > On Fri, Sep 25, 2015 at 6:40 PM, Celelibi via Syslinux > >> 2015-09-25 21:27 UTC+02:00, Geert Stappers via Syslinux > >>> On Tue, Sep 15, 2015 at 05:22:40AM -0400, Gene Cumm via Syslinux wrote: > >>>> On Sep 10, 2015 1:32 AM, "celelibi--- via Syslinux" > >>>> > > >>>> > From: Sylvain Gault <sylvain.gault at gmail.com> > >>>> ><snip/>> >>>> > > >>>> > efi/dns.c | 529 > >>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > >>>> > efi/pxe.c | 16 -- 2 files changed, 529 insertions(+), 16 > >>>> > deletions(-) create mode 100644 efi/dns.c > >>>> > >>>> Extending like this to reestablish equality between architectures > >>>> definitely is a good idea. > >>>> > >>> > >>> How to read 'a good idea' ? Does it mean "patch accepted"??? > >>> > >> > >> In any case, I may submit another version (or additional patches if > >> this one get merged) with a few improvements, like removing the > >> recursion limit and retry the same servers several times as the UDP > >> packets may get lost. And coding style improvement. > > > > As you noted, you don't accommodate timeouts which is probably the > > most critical operation this doesn't have. We can't stay in the query > > forever. jiffies() and ms_timer() are the two available in the core. > > core/legacynet/dnsresolv.c used jiffies() but ms_timer() should be > > dealing with millisecond values, not 55ms jiffies and about CLK_TCK > > jiffies per second. > > Not sure I understand everything correctly. > Currently, the code won't stay in the query forever. It tries every > server once and exit. I think trying every server 3 times would be > better as there may be a single DNS server and UDP packets may get > lost. The only way we can stay in the query forever is if we get > flooded with UDP packets not for us. > > I didn't implement a timeout because I didn't want to make a busy > loop. But since EFI is mono-threaded, I think we don't care. > > > > > An alternative way to just stub it is to merge everything but the > > actual call that starts sending UDP packets or just the most basic > > functionality you've implemented first. > > Not sure I see the point of merging something that wouldn't work. >Point I see: Having work done in public git repo. Having it available for further work. Groeten Geert Stappers -- Leven en laten leven
On 09/28/2015 03:50 AM, Gene Cumm via Syslinux wrote:> > As you noted, you don't accommodate timeouts which is probably the > most critical operation this doesn't have. We can't stay in the query > forever. jiffies() and ms_timer() are the two available in the core. > core/legacynet/dnsresolv.c used jiffies() but ms_timer() should be > dealing with millisecond values, not 55ms jiffies and about CLK_TCK > jiffies per second. > > An alternative way to just stub it is to merge everything but the > actual call that starts sending UDP packets or just the most basic > functionality you've implemented first. >Yes, please; do share the code rather than having two copies. -hpa