Re: [edk2] [Patch] NetworkPkg/DnsDxe: Handle CNAME type responded from the name server

[Wu, Jiaxin]

Yes, take the minimum value of TTL is meaningful.

From: Fu, Siyuan
Sent: Tuesday, September 6, 2016 3:13 PM
To: Wu, Jiaxin ; edk2-devel@lists.01.org
Cc: Hegde Nagaraj P ; Ye, Ting 
Subject: RE: [Patch] NetworkPkg/DnsDxe: Handle CNAME type responded from the 
name server

Jiaxin,

For #2, it's ok, I didn't notice the change in your patch.

For #1, I think we shouldn't mix the CNAME RR and A RR, they are 2 different 
answers and may have different TTL.

Best Regards
Siyuan

From: Wu, Jiaxin
Sent: Tuesday, September 6, 2016 3:05 PM
To: Fu, Siyuan >; 
edk2-devel@lists.01.org
Cc: Hegde Nagaraj P >; 
Ye, Ting >
Subject: RE: [Patch] NetworkPkg/DnsDxe: Handle CNAME type responded from the 
name server

Hi Siyuan,

> I have 2 comments for the patch.
> 1. I think we can add the CNAME record to the DNS cache, just like the A and
>  records, so the driver won't need to repeat the query if user query the
> host again.

In my opinion , the CNAME is unnecessary to cache to avoid the repeat the 
query. Because if DnsCache is enabled, HostName and the corresponding IpAddress 
will be retrieved directly from  EFI_DNS4_CACHE_ENTRY.

> 2. We should hold the original query packet until the response is parsed
> successfully, otherwise the system will crash again if a retransmit is needed.

This patch already did the update to avoid the crash issue during retransmit by 
removing the below code after the parsing is complete.

  //
  // Parsing is complete, free the sending packet and signal Event here.
  //
  if (Item != NULL && Item->Value != NULL) {
NetbufFree ((NET_BUF *) (Item->Value));
  }

So, the original query packet is safe until the response is parsed.

>
> Best Regards
> Siyuan
>
>

Thanks,
Jiaxin

> > -Original Message-
> > From: Wu, Jiaxin
> > Sent: Tuesday, September 6, 2016 11:39 AM
> > To: edk2-devel@lists.01.org
> > Cc: Hegde Nagaraj P 
> > >; Fu, Siyuan
> > >; Ye, Ting 
> > >
> > Subject: [Patch] NetworkPkg/DnsDxe: Handle CNAME type responded from
> > the name server
> >
> > According RFC 1034 - 3.6.2, if the query name is an alias, the name
> > server will include the CNAME record in the response and restart the
> > query at the domain name specified in the data field of the CNAME
> > record. RFC also provides one example server action when A query
> > received:
> >
> > Suppose a name server was processing a query with for USCISIC.ARPA,
> > asking for type A information, and had the following resource records:
> > USC-ISIC.ARPA IN CNAME C.ISI.EDU
> > C.ISI.EDU IN A 10.0.0.52
> > Both of these RRs would be returned in the response to the type A query.
> >
> > Currently, DnsDxe driver doesn't handle the CNAME type response, which
> > will cause any exception result. The driver need continue the packet
> > parsing while CNAME type record parsed. So, this patch is used to
> > handle it correctly.
> >
> > Cc: Hegde Nagaraj P 
> > >
> > Cc: Fu Siyuan >
> > Cc: Ye Ting >
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Jiaxin Wu >
> > ---
> >  NetworkPkg/DnsDxe/DnsImpl.c | 60
> > ++--
> > -
> >  1 file changed, 35 insertions(+), 25 deletions(-)
> >
> > diff --git a/NetworkPkg/DnsDxe/DnsImpl.c
> b/NetworkPkg/DnsDxe/DnsImpl.c
> > index 360f68e..c68ec88 100644
> > --- a/NetworkPkg/DnsDxe/DnsImpl.c
> > +++ b/NetworkPkg/DnsDxe/DnsImpl.c
> > @@ -1231,17 +1231,10 @@ ParseDnsResponse (
> >if (DnsHeader->Flags.Bits.RCode != DNS_FLAGS_RCODE_NO_ERROR ||
> > DnsHeader->AnswersNum < 1 || \
> >DnsHeader->Flags.Bits.QR != DNS_FLAGS_QR_RESPONSE) {
> >Status = EFI_ABORTED;
> >goto ON_EXIT;
> >}
> > -
> > -  //
> > -  // Free the sending packet.
> > -  //
> > -  if (Item->Value != NULL) {
> > -NetbufFree ((NET_BUF *) (Item->Value));
> > -  }
> >
> >//
> >// Do some buffer allocations.
> >//
> >if (Instance->Service->IpVersion == IP_VERSION_4) { @@ -1288,40
> > +1281,42 @@ ParseDnsResponse (
> >//
> >// It's the GeneralLookUp querying.
> >//
> >Dns6TokenEntry->Token->RspData.GLookupData = AllocatePool
> > (sizeof (DNS_RESOURCE_RECORD));
> >if (Dns6TokenEntry->Token->RspData.GLookupData == NULL) {
> > -Status = EFI_UNSUPPORTED;
> > +Status = EFI_OUT_OF_RESOURCES;
> >  goto ON_EXIT;
> >}
> >

Re: [edk2] [Patch] NetworkPkg/DnsDxe: Handle CNAME type responded from the name server

[Fu, Siyuan]

Jiaxin,

For #2, it's ok, I didn't notice the change in your patch.

For #1, I think we shouldn't mix the CNAME RR and A RR, they are 2 different 
answers and may have different TTL.

Best Regards
Siyuan

From: Wu, Jiaxin
Sent: Tuesday, September 6, 2016 3:05 PM
To: Fu, Siyuan ; edk2-devel@lists.01.org
Cc: Hegde Nagaraj P ; Ye, Ting 
Subject: RE: [Patch] NetworkPkg/DnsDxe: Handle CNAME type responded from the 
name server

Hi Siyuan,

> I have 2 comments for the patch.
> 1. I think we can add the CNAME record to the DNS cache, just like the A and
>  records, so the driver won't need to repeat the query if user query the
> host again.

In my opinion , the CNAME is unnecessary to cache to avoid the repeat the 
query. Because if DnsCache is enabled, HostName and the corresponding IpAddress 
will be retrieved directly from  EFI_DNS4_CACHE_ENTRY.

> 2. We should hold the original query packet until the response is parsed
> successfully, otherwise the system will crash again if a retransmit is needed.

This patch already did the update to avoid the crash issue during retransmit by 
removing the below code after the parsing is complete.

  //
  // Parsing is complete, free the sending packet and signal Event here.
  //
  if (Item != NULL && Item->Value != NULL) {
NetbufFree ((NET_BUF *) (Item->Value));
  }

So, the original query packet is safe until the response is parsed.

>
> Best Regards
> Siyuan
>
>

Thanks,
Jiaxin

> > -Original Message-
> > From: Wu, Jiaxin
> > Sent: Tuesday, September 6, 2016 11:39 AM
> > To: edk2-devel@lists.01.org
> > Cc: Hegde Nagaraj P 
> > >; Fu, Siyuan
> > >; Ye, Ting 
> > >
> > Subject: [Patch] NetworkPkg/DnsDxe: Handle CNAME type responded from
> > the name server
> >
> > According RFC 1034 - 3.6.2, if the query name is an alias, the name
> > server will include the CNAME record in the response and restart the
> > query at the domain name specified in the data field of the CNAME
> > record. RFC also provides one example server action when A query
> > received:
> >
> > Suppose a name server was processing a query with for USCISIC.ARPA,
> > asking for type A information, and had the following resource records:
> > USC-ISIC.ARPA IN CNAME C.ISI.EDU
> > C.ISI.EDU IN A 10.0.0.52
> > Both of these RRs would be returned in the response to the type A query.
> >
> > Currently, DnsDxe driver doesn't handle the CNAME type response, which
> > will cause any exception result. The driver need continue the packet
> > parsing while CNAME type record parsed. So, this patch is used to
> > handle it correctly.
> >
> > Cc: Hegde Nagaraj P 
> > >
> > Cc: Fu Siyuan >
> > Cc: Ye Ting >
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Jiaxin Wu >
> > ---
> >  NetworkPkg/DnsDxe/DnsImpl.c | 60
> > ++--
> > -
> >  1 file changed, 35 insertions(+), 25 deletions(-)
> >
> > diff --git a/NetworkPkg/DnsDxe/DnsImpl.c
> b/NetworkPkg/DnsDxe/DnsImpl.c
> > index 360f68e..c68ec88 100644
> > --- a/NetworkPkg/DnsDxe/DnsImpl.c
> > +++ b/NetworkPkg/DnsDxe/DnsImpl.c
> > @@ -1231,17 +1231,10 @@ ParseDnsResponse (
> >if (DnsHeader->Flags.Bits.RCode != DNS_FLAGS_RCODE_NO_ERROR ||
> > DnsHeader->AnswersNum < 1 || \
> >DnsHeader->Flags.Bits.QR != DNS_FLAGS_QR_RESPONSE) {
> >Status = EFI_ABORTED;
> >goto ON_EXIT;
> >}
> > -
> > -  //
> > -  // Free the sending packet.
> > -  //
> > -  if (Item->Value != NULL) {
> > -NetbufFree ((NET_BUF *) (Item->Value));
> > -  }
> >
> >//
> >// Do some buffer allocations.
> >//
> >if (Instance->Service->IpVersion == IP_VERSION_4) { @@ -1288,40
> > +1281,42 @@ ParseDnsResponse (
> >//
> >// It's the GeneralLookUp querying.
> >//
> >Dns6TokenEntry->Token->RspData.GLookupData = AllocatePool
> > (sizeof (DNS_RESOURCE_RECORD));
> >if (Dns6TokenEntry->Token->RspData.GLookupData == NULL) {
> > -Status = EFI_UNSUPPORTED;
> > +Status = EFI_OUT_OF_RESOURCES;
> >  goto ON_EXIT;
> >}
> >Dns6TokenEntry->Token->RspData.GLookupData->RRList =
> > AllocatePool (DnsHeader->AnswersNum * sizeof
> (DNS_RESOURCE_RECORD));
> >if (Dns6TokenEntry->Token->RspData.GLookupData->RRList == NULL) {
> > -Status = EFI_UNSUPPORTED;
> > +Status = EFI_OUT_OF_RESOURCES;
> >  goto ON_EXIT;
> >}
> >  } else {
> >//
> >// It's not the GeneralLookUp querying. Check the Query type.
> >//
> >if 

Re: [edk2] [Patch] NetworkPkg/DnsDxe: Handle CNAME type responded from the name server

[Wu, Jiaxin]

Hi Siyuan,

> I have 2 comments for the patch.
> 1. I think we can add the CNAME record to the DNS cache, just like the A and
>  records, so the driver won't need to repeat the query if user query the
> host again.

In my opinion , the CNAME is unnecessary to cache to avoid the repeat the 
query. Because if DnsCache is enabled, HostName and the corresponding IpAddress 
will be retrieved directly from  EFI_DNS4_CACHE_ENTRY.

> 2. We should hold the original query packet until the response is parsed
> successfully, otherwise the system will crash again if a retransmit is needed.

This patch already did the update to avoid the crash issue during retransmit by 
removing the below code after the parsing is complete.

  //
  // Parsing is complete, free the sending packet and signal Event here.
  //
  if (Item != NULL && Item->Value != NULL) {
NetbufFree ((NET_BUF *) (Item->Value));
  }

So, the original query packet is safe until the response is parsed.

> 
> Best Regards
> Siyuan
> 
> 

Thanks,
Jiaxin

> > -Original Message-
> > From: Wu, Jiaxin
> > Sent: Tuesday, September 6, 2016 11:39 AM
> > To: edk2-devel@lists.01.org
> > Cc: Hegde Nagaraj P ; Fu, Siyuan
> > ; Ye, Ting 
> > Subject: [Patch] NetworkPkg/DnsDxe: Handle CNAME type responded from
> > the name server
> >
> > According RFC 1034 - 3.6.2, if the query name is an alias, the name
> > server will include the CNAME record in the response and restart the
> > query at the domain name specified in the data field of the CNAME
> > record. RFC also provides one example server action when A query
> > received:
> >
> > Suppose a name server was processing a query with for USCISIC.ARPA,
> > asking for type A information, and had the following resource records:
> > USC-ISIC.ARPA IN CNAME C.ISI.EDU
> > C.ISI.EDU IN A 10.0.0.52
> > Both of these RRs would be returned in the response to the type A query.
> >
> > Currently, DnsDxe driver doesn't handle the CNAME type response, which
> > will cause any exception result. The driver need continue the packet
> > parsing while CNAME type record parsed. So, this patch is used to
> > handle it correctly.
> >
> > Cc: Hegde Nagaraj P 
> > Cc: Fu Siyuan 
> > Cc: Ye Ting 
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Jiaxin Wu 
> > ---
> >  NetworkPkg/DnsDxe/DnsImpl.c | 60
> > ++--
> > -
> >  1 file changed, 35 insertions(+), 25 deletions(-)
> >
> > diff --git a/NetworkPkg/DnsDxe/DnsImpl.c
> b/NetworkPkg/DnsDxe/DnsImpl.c
> > index 360f68e..c68ec88 100644
> > --- a/NetworkPkg/DnsDxe/DnsImpl.c
> > +++ b/NetworkPkg/DnsDxe/DnsImpl.c
> > @@ -1231,17 +1231,10 @@ ParseDnsResponse (
> >if (DnsHeader->Flags.Bits.RCode != DNS_FLAGS_RCODE_NO_ERROR ||
> > DnsHeader->AnswersNum < 1 || \
> >DnsHeader->Flags.Bits.QR != DNS_FLAGS_QR_RESPONSE) {
> >Status = EFI_ABORTED;
> >goto ON_EXIT;
> >}
> > -
> > -  //
> > -  // Free the sending packet.
> > -  //
> > -  if (Item->Value != NULL) {
> > -NetbufFree ((NET_BUF *) (Item->Value));
> > -  }
> >
> >//
> >// Do some buffer allocations.
> >//
> >if (Instance->Service->IpVersion == IP_VERSION_4) { @@ -1288,40
> > +1281,42 @@ ParseDnsResponse (
> >//
> >// It's the GeneralLookUp querying.
> >//
> >Dns6TokenEntry->Token->RspData.GLookupData = AllocatePool
> > (sizeof (DNS_RESOURCE_RECORD));
> >if (Dns6TokenEntry->Token->RspData.GLookupData == NULL) {
> > -Status = EFI_UNSUPPORTED;
> > +Status = EFI_OUT_OF_RESOURCES;
> >  goto ON_EXIT;
> >}
> >Dns6TokenEntry->Token->RspData.GLookupData->RRList =
> > AllocatePool (DnsHeader->AnswersNum * sizeof
> (DNS_RESOURCE_RECORD));
> >if (Dns6TokenEntry->Token->RspData.GLookupData->RRList == NULL) {
> > -Status = EFI_UNSUPPORTED;
> > +Status = EFI_OUT_OF_RESOURCES;
> >  goto ON_EXIT;
> >}
> >  } else {
> >//
> >// It's not the GeneralLookUp querying. Check the Query type.
> >//
> >if (QuerySection->Type == DNS_TYPE_) {
> >  Dns6TokenEntry->Token->RspData.H2AData = AllocatePool (sizeof
> > (DNS6_HOST_TO_ADDR_DATA));
> >  if (Dns6TokenEntry->Token->RspData.H2AData == NULL) {
> > -  Status = EFI_UNSUPPORTED;
> > +  Status = EFI_OUT_OF_RESOURCES;
> >goto ON_EXIT;
> >  }
> >  Dns6TokenEntry->Token->RspData.H2AData->IpList = AllocatePool
> > (DnsHeader->AnswersNum * sizeof (EFI_IPv6_ADDRESS));
> >  if (Dns6TokenEntry->Token->RspData.H2AData->IpList == NULL) {
> > -  Status = EFI_UNSUPPORTED;
> > +  Status = EFI_OUT_OF_RESOURCES;
> >goto ON_EXIT;
> >  }
> >} else {
> >  Status = EFI_UNSUPPORTED;
> > 

[edk2] [Patch] NetworkPkg/DnsDxe: Handle CNAME type responded from the name server

[Jiaxin Wu]

According RFC 1034 - 3.6.2, if the query name is an alias, the name server
will include the CNAME record in the response and restart the query at the
domain name specified in the data field of the CNAME record. RFC also provides
one example server action when A query received:

Suppose a name server was processing a query with for USCISIC.ARPA, asking for
type A information, and had the following resource records:
USC-ISIC.ARPA IN CNAME C.ISI.EDU
C.ISI.EDU IN A 10.0.0.52
Both of these RRs would be returned in the response to the type A query.

Currently, DnsDxe driver doesn't handle the CNAME type response, which will 
cause
any exception result. The driver need continue the packet parsing while CNAME 
type
record parsed. So, this patch is used to handle it correctly.

Cc: Hegde Nagaraj P 
Cc: Fu Siyuan 
Cc: Ye Ting 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jiaxin Wu 
---
 NetworkPkg/DnsDxe/DnsImpl.c | 60 ++---
 1 file changed, 35 insertions(+), 25 deletions(-)

diff --git a/NetworkPkg/DnsDxe/DnsImpl.c b/NetworkPkg/DnsDxe/DnsImpl.c
index 360f68e..c68ec88 100644
--- a/NetworkPkg/DnsDxe/DnsImpl.c
+++ b/NetworkPkg/DnsDxe/DnsImpl.c
@@ -1231,17 +1231,10 @@ ParseDnsResponse (
   if (DnsHeader->Flags.Bits.RCode != DNS_FLAGS_RCODE_NO_ERROR || 
DnsHeader->AnswersNum < 1 || \
   DnsHeader->Flags.Bits.QR != DNS_FLAGS_QR_RESPONSE) {
   Status = EFI_ABORTED;
   goto ON_EXIT;
   }
-
-  //
-  // Free the sending packet.
-  //
-  if (Item->Value != NULL) {
-NetbufFree ((NET_BUF *) (Item->Value));
-  }
   
   //
   // Do some buffer allocations.
   //
   if (Instance->Service->IpVersion == IP_VERSION_4) {
@@ -1288,40 +1281,42 @@ ParseDnsResponse (
   //
   // It's the GeneralLookUp querying.
   //
   Dns6TokenEntry->Token->RspData.GLookupData = AllocatePool (sizeof 
(DNS_RESOURCE_RECORD));
   if (Dns6TokenEntry->Token->RspData.GLookupData == NULL) {
-Status = EFI_UNSUPPORTED;
+Status = EFI_OUT_OF_RESOURCES;
 goto ON_EXIT;
   }
   Dns6TokenEntry->Token->RspData.GLookupData->RRList = AllocatePool 
(DnsHeader->AnswersNum * sizeof (DNS_RESOURCE_RECORD));
   if (Dns6TokenEntry->Token->RspData.GLookupData->RRList == NULL) {
-Status = EFI_UNSUPPORTED;
+Status = EFI_OUT_OF_RESOURCES;
 goto ON_EXIT;
   }
 } else {
   //
   // It's not the GeneralLookUp querying. Check the Query type.
   //
   if (QuerySection->Type == DNS_TYPE_) {
 Dns6TokenEntry->Token->RspData.H2AData = AllocatePool (sizeof 
(DNS6_HOST_TO_ADDR_DATA));
 if (Dns6TokenEntry->Token->RspData.H2AData == NULL) {
-  Status = EFI_UNSUPPORTED;
+  Status = EFI_OUT_OF_RESOURCES;
   goto ON_EXIT;
 }
 Dns6TokenEntry->Token->RspData.H2AData->IpList = AllocatePool 
(DnsHeader->AnswersNum * sizeof (EFI_IPv6_ADDRESS));
 if (Dns6TokenEntry->Token->RspData.H2AData->IpList == NULL) {
-  Status = EFI_UNSUPPORTED;
+  Status = EFI_OUT_OF_RESOURCES;
   goto ON_EXIT;
 }
   } else {
 Status = EFI_UNSUPPORTED;
 goto ON_EXIT;
   }
 }
   }
 
+  Status = EFI_NOT_FOUND;
+
   //
   // Processing AnswerSection.
   //
   while (AnswerSectionNum < DnsHeader->AnswersNum) {
 //
@@ -1348,51 +1343,53 @@ ParseDnsResponse (
   //
   // Fill the ResourceRecord.
   //
   Dns4RR[RRCount].QName = AllocateZeroPool (AsciiStrLen (QueryName) + 1);
   if (Dns4RR[RRCount].QName == NULL) {
-Status = EFI_UNSUPPORTED;
+Status = EFI_OUT_OF_RESOURCES;
 goto ON_EXIT;
   }
   CopyMem (Dns4RR[RRCount].QName, QueryName, AsciiStrLen (QueryName));
   Dns4RR[RRCount].QType = AnswerSection->Type;
   Dns4RR[RRCount].QClass = AnswerSection->Class;
   Dns4RR[RRCount].TTL = AnswerSection->Ttl;
   Dns4RR[RRCount].DataLength = AnswerSection->DataLength;
   Dns4RR[RRCount].RData = AllocateZeroPool (Dns4RR[RRCount].DataLength);
   if (Dns4RR[RRCount].RData == NULL) {
-Status = EFI_UNSUPPORTED;
+Status = EFI_OUT_OF_RESOURCES;
 goto ON_EXIT;
   }
   CopyMem (Dns4RR[RRCount].RData, AnswerData, Dns4RR[RRCount].DataLength);
   
   RRCount ++;
+  Status = EFI_SUCCESS;
 } else if (Instance->Service->IpVersion == IP_VERSION_6 && 
Dns6TokenEntry->GeneralLookUp) {
   Dns6RR = Dns6TokenEntry->Token->RspData.GLookupData->RRList;
   AnswerData = (UINT8 *) AnswerSection + sizeof (*AnswerSection);
 
   //
   // Fill the ResourceRecord.
   //
   Dns6RR[RRCount].QName = AllocateZeroPool (AsciiStrLen (QueryName) + 1);
   if (Dns6RR[RRCount].QName == NULL) {
-Status = EFI_UNSUPPORTED;
+Status = EFI_OUT_OF_RESOURCES;
 goto ON_EXIT;
   }
   CopyMem