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
> > Cc: Hegde Nagaraj P <nagaraj-p.he...@hpe.com>; Fu, Siyuan
> > <siyuan...@intel.com>; Ye, Ting <ting...@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.he...@hpe.com>
> > Cc: Fu Siyuan <siyuan...@intel.com>
> > Cc: Ye Ting <ting...@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Jiaxin Wu <jiaxin...@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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
