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

["Wu, Jiaxin" <jiaxin.wu@intel.com>]

Yes, take the minimum value of TTL is meaningful.

From: Fu, Siyuan
Sent: Tuesday, September 6, 2016 3:13 PM
To: Wu, Jiaxin <jiaxin.wu@intel.com>; edk2-devel@lists.01.org
Cc: Hegde Nagaraj P <nagaraj-p.hegde@hpe.com>; Ye, Ting <ting.ye@intel.com>
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 <siyuan.fu@intel.com<mailto:siyuan.fu@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
Cc: Hegde Nagaraj P <nagaraj-p.hegde@hpe.com<mailto:nagaraj-p.hegde@hpe.com>>; Ye, Ting <ting.ye@intel.com<mailto:ting.ye@intel.com>>
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
> AAAA 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<mailto:edk2-devel@lists.01.org>
> > Cc: Hegde Nagaraj P <nagaraj-p.hegde@hpe.com<mailto:nagaraj-p.hegde@hpe.com>>; Fu, Siyuan
> > <siyuan.fu@intel.com<mailto:siyuan.fu@intel.com>>; Ye, Ting <ting.ye@intel.com<mailto:ting.ye@intel.com>>
> > 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 <nagaraj-p.hegde@hpe.com<mailto:nagaraj-p.hegde@hpe.com>>
> > Cc: Fu Siyuan <siyuan.fu@intel.com<mailto:siyuan.fu@intel.com>>
> > Cc: Ye Ting <ting.ye@intel.com<mailto:ting.ye@intel.com>>
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com<mailto:jiaxin.wu@intel.com>>
> > ---
> >  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_AAAA) {
> >          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 (Dns6RR[RRCount].QName, QueryName, AsciiStrLen
> (QueryName));
> >        Dns6RR[RRCount].QType = AnswerSection->Type;
> >        Dns6RR[RRCount].QClass = AnswerSection->Class;
> >        Dns6RR[RRCount].TTL = AnswerSection->Ttl;
> >        Dns6RR[RRCount].DataLength = AnswerSection->DataLength;
> >        Dns6RR[RRCount].RData = AllocateZeroPool
> > (Dns6RR[RRCount].DataLength);
> >        if (Dns6RR[RRCount].RData == NULL) {
> > -        Status = EFI_UNSUPPORTED;
> > +        Status = EFI_OUT_OF_RESOURCES;
> >          goto ON_EXIT;
> >        }
> >        CopyMem (Dns6RR[RRCount].RData, AnswerData,
> > Dns6RR[RRCount].DataLength);
> >
> >        RRCount ++;
> > +      Status = EFI_SUCCESS;
> >      } else {
> >        //
> >        // It's not the GeneralLookUp querying.
> >        // Check the Query type, parse the response packet.
> >        //
> > @@ -1425,30 +1422,31 @@ ParseDnsResponse (
> >          //
> >          // Allocate new CacheEntry pool.
> >          //
> >          Dns4CacheEntry = AllocateZeroPool (sizeof
> (EFI_DNS4_CACHE_ENTRY));
> >          if (Dns4CacheEntry == NULL) {
> > -          Status = EFI_UNSUPPORTED;
> > +          Status = EFI_OUT_OF_RESOURCES;
> >            goto ON_EXIT;
> >          }
> >          Dns4CacheEntry->HostName = AllocateZeroPool (2 *
> > (StrLen(Dns4TokenEntry->QueryHostName) + 1));
> >          if (Dns4CacheEntry->HostName == NULL) {
> > -          Status = EFI_UNSUPPORTED;
> > +          Status = EFI_OUT_OF_RESOURCES;
> >            goto ON_EXIT;
> >          }
> >          CopyMem (Dns4CacheEntry->HostName,
> > Dns4TokenEntry->QueryHostName,
> > 2 * (StrLen(Dns4TokenEntry->QueryHostName) + 1));
> >          Dns4CacheEntry->IpAddress = AllocateZeroPool (sizeof
> > (EFI_IPv4_ADDRESS));
> >          if (Dns4CacheEntry->IpAddress == NULL) {
> > -          Status = EFI_UNSUPPORTED;
> > +          Status = EFI_OUT_OF_RESOURCES;
> >            goto ON_EXIT;
> >          }
> >          CopyMem (Dns4CacheEntry->IpAddress, AnswerData, sizeof
> > (EFI_IPv4_ADDRESS));
> >          Dns4CacheEntry->Timeout = AnswerSection->Ttl;
> >
> >          UpdateDns4Cache (&mDriverData->Dns4CacheList, FALSE, TRUE,
> > *Dns4CacheEntry);
> >
> > -        IpCount ++;
> > +        IpCount ++;
> > +        Status = EFI_SUCCESS;
> >          break;
> >        case DNS_TYPE_AAAA:
> >          //
> >          // This is address entry, get Data.
> >          //
> > @@ -1476,30 +1474,38 @@ ParseDnsResponse (
> >          //
> >          // Allocate new CacheEntry pool.
> >          //
> >          Dns6CacheEntry = AllocateZeroPool (sizeof
> (EFI_DNS6_CACHE_ENTRY));
> >          if (Dns6CacheEntry == NULL) {
> > -          Status = EFI_UNSUPPORTED;
> > +          Status = EFI_OUT_OF_RESOURCES;
> >            goto ON_EXIT;
> >          }
> >          Dns6CacheEntry->HostName = AllocateZeroPool (2 *
> > (StrLen(Dns6TokenEntry->QueryHostName) + 1));
> >          if (Dns6CacheEntry->HostName == NULL) {
> > -          Status = EFI_UNSUPPORTED;
> > +          Status = EFI_OUT_OF_RESOURCES;
> >            goto ON_EXIT;
> >          }
> >          CopyMem (Dns6CacheEntry->HostName,
> > Dns6TokenEntry->QueryHostName,
> > 2 * (StrLen(Dns6TokenEntry->QueryHostName) + 1));
> >          Dns6CacheEntry->IpAddress = AllocateZeroPool (sizeof
> > (EFI_IPv6_ADDRESS));
> >          if (Dns6CacheEntry->IpAddress == NULL) {
> > -          Status = EFI_UNSUPPORTED;
> > +          Status = EFI_OUT_OF_RESOURCES;
> >            goto ON_EXIT;
> >          }
> >          CopyMem (Dns6CacheEntry->IpAddress, AnswerData, sizeof
> > (EFI_IPv6_ADDRESS));
> >          Dns6CacheEntry->Timeout = AnswerSection->Ttl;
> >
> >          UpdateDns6Cache (&mDriverData->Dns6CacheList, FALSE, TRUE,
> > *Dns6CacheEntry);
> >
> >          IpCount ++;
> > +        Status = EFI_SUCCESS;
> > +        break;
> > +      case DNS_TYPE_CNAME:
> > +        //
> > +        // 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. So, let's skip this CNAME record parsing and
> > continue the next one.
> > +        //
> >          break;
> >        default:
> >          Status = EFI_UNSUPPORTED;
> >          goto ON_EXIT;
> >        }
> > @@ -1539,24 +1545,28 @@ ParseDnsResponse (
> >        }
> >      }
> >    }
> >
> >    //
> > -  // Parsing is complete, SignalEvent here.
> > +  // Parsing is complete, free the sending packet and signal Event here.
> >    //
> > +  if (Item != NULL && Item->Value != NULL) {
> > +    NetbufFree ((NET_BUF *) (Item->Value));  }
> > +
> >    if (Instance->Service->IpVersion == IP_VERSION_4) {
> >      ASSERT (Dns4TokenEntry != NULL);
> >      Dns4RemoveTokenEntry (&Instance->Dns4TxTokens, Dns4TokenEntry);
> > -    Dns4TokenEntry->Token->Status = EFI_SUCCESS;
> > +    Dns4TokenEntry->Token->Status = Status;
> >      if (Dns4TokenEntry->Token->Event != NULL) {
> >        gBS->SignalEvent (Dns4TokenEntry->Token->Event);
> >        DispatchDpc ();
> >      }
> >    } else {
> >      ASSERT (Dns6TokenEntry != NULL);
> >      Dns6RemoveTokenEntry (&Instance->Dns6TxTokens, Dns6TokenEntry);
> > -    Dns6TokenEntry->Token->Status = EFI_SUCCESS;
> > +    Dns6TokenEntry->Token->Status = Status;
> >      if (Dns6TokenEntry->Token->Event != NULL) {
> >        gBS->SignalEvent (Dns6TokenEntry->Token->Event);
> >        DispatchDpc ();
> >      }
> >    }
> > --
> > 1.9.5.msysgit.1