| Message ID | 20240209152117.957387-1-dceara@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v2] pinctrl: dns: Ignore additional records. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
Thank you for the updates, Dumitru. Acked-by: Mark Michelson <mmichels@redhat.com> On 2/9/24 10:21, Dumitru Ceara wrote: > EDNS is backwards compatible so it's safe to just ignore additional ARs. > > Reported-at: https://github.com/ovn-org/ovn/issues/228 > Reported-at: https://issues.redhat.com/browse/FDP-222 > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > V2: > - Addressed Mark's comments: > - fixed subject > - added test > --- > controller/pinctrl.c | 20 ++++++++--------- > tests/ovn.at | 51 ++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 61 insertions(+), 10 deletions(-) > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > index faa3f92260..98b29de9ff 100644 > --- a/controller/pinctrl.c > +++ b/controller/pinctrl.c > @@ -2887,6 +2887,7 @@ dns_build_ptr_answer( > } > > #define DNS_RCODE_SERVER_REFUSE 0x5 > +#define DNS_QUERY_TYPE_CLASS_LEN (2 * sizeof(ovs_be16)) > > /* Called with in the pinctrl_handler thread context. */ > static void > @@ -2950,18 +2951,13 @@ pinctrl_handle_dns_lookup( > goto exit; > } > > - /* Check if there is an additional record present, which is unsupported */ > - if (in_dns_header->arcount) { > - VLOG_DBG_RL(&rl, "Received DNS query with additional records, which" > - " is unsupported"); > - goto exit; > - } > - > struct udp_header *in_udp = dp_packet_l4(pkt_in); > size_t udp_len = ntohs(in_udp->udp_len); > size_t l4_len = dp_packet_l4_size(pkt_in); > + uint8_t *l4_start = (uint8_t *) in_udp; > uint8_t *end = (uint8_t *)in_udp + MIN(udp_len, l4_len); > uint8_t *in_dns_data = (uint8_t *)(in_dns_header + 1); > + uint8_t *in_dns_data_start = in_dns_data; > uint8_t *in_queryname = in_dns_data; > uint16_t idx = 0; > struct ds query_name; > @@ -2985,7 +2981,7 @@ pinctrl_handle_dns_lookup( > in_dns_data += idx; > > /* Query should have TYPE and CLASS fields */ > - if (in_dns_data + (2 * sizeof(ovs_be16)) > end) { > + if (in_dns_data + DNS_QUERY_TYPE_CLASS_LEN > end) { > ds_destroy(&query_name); > goto exit; > } > @@ -2999,6 +2995,10 @@ pinctrl_handle_dns_lookup( > goto exit; > } > > + uint8_t *rest = in_dns_data + DNS_QUERY_TYPE_CLASS_LEN; > + uint32_t query_size = rest - in_dns_data_start; > + uint32_t query_l4_size = rest - l4_start; > + > uint64_t dp_key = ntohll(pin->flow_metadata.flow.metadata); > const char *answer_data = NULL; > bool ovn_owned = false; > @@ -3081,7 +3081,7 @@ pinctrl_handle_dns_lookup( > goto exit; > } > > - uint16_t new_l4_size = ntohs(in_udp->udp_len) + dns_answer.size; > + uint16_t new_l4_size = query_l4_size + dns_answer.size; > size_t new_packet_size = pkt_in->l4_ofs + new_l4_size; > struct dp_packet pkt_out; > dp_packet_init(&pkt_out, new_packet_size); > @@ -3118,7 +3118,7 @@ pinctrl_handle_dns_lookup( > out_dns_header->arcount = 0; > > /* Copy the Query section. */ > - dp_packet_put(&pkt_out, dp_packet_data(pkt_in), dp_packet_size(pkt_in)); > + dp_packet_put(&pkt_out, dp_packet_data(pkt_in), query_size); > > /* Copy the answer sections. */ > dp_packet_put(&pkt_out, dns_answer.data, dns_answer.size); > diff --git a/tests/ovn.at b/tests/ovn.at > index 2cf335cf45..6b0b80bfc1 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -11455,6 +11455,57 @@ OVN_CLEANUP([hv1]) > AT_CLEANUP > ]) > > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([dns lookup : EDNS]) > +AT_SKIP_IF([test $HAVE_SCAPY = no]) > +ovn_start > + > +check ovn-nbctl ls-add ls \ > + -- lsp-add ls lsp \ > + -- lsp-set-addresses lsp "00:00:00:00:00:01 10.0.0.1" > + > +d=$(ovn-nbctl create dns records={}) > + > +check ovn-nbctl set dns $d records:foo.ovn.org="10.0.0.42" > +check ovn-nbctl set Logical_switch ls dns_records="$d" > + > +net_add n1 > +sim_add hv1 > + > +as hv1 > +ovs-vsctl add-br br-phys > +ovn_attach n1 br-phys 192.168.0.1 > +ovs-vsctl -- add-port br-int hv1-vif1 -- \ > + set interface hv1-vif1 external-ids:iface-id=lsp \ > + options:tx_pcap=hv1/vif1-tx.pcap \ > + options:rxq_pcap=hv1/vif1-rx.pcap > + > +OVN_POPULATE_ARP > +wait_for_ports_up > +check ovn-nbctl --wait=hv sync > + > +dns_req=$(fmt_pkt "Ether(dst='00:00:00:00:00:02', src='00:00:00:00:00:01') / \ > + IP(dst='10.0.0.254', src='10.0.0.1') / \ > + UDP(sport=42424, dport=53) / \ > + DNS(rd=1, qd=DNSQR(qname='foo.ovn.org'), arcount=1, ar=[ \ > + DNSRR(type='OPT', rclass=4096, \ > + rdata=EDNS0ClientSubnet(source_plen=24, \ > + address='10.0.0.1'))])") > +dns_reply=$(fmt_pkt "Ether(dst='00:00:00:00:00:01', src='00:00:00:00:00:02') / \ > + IP(dst='10.0.0.1', src='10.0.0.254') / \ > + UDP(sport=53, dport=42424,chksum=0) / \ > + DNS(qr=1, qd=DNSQR(qname='foo.ovn.org'), \ > + an=DNSRR(rrname='foo.ovn.org', type='A', ttl=3600, \ > + rdata='10.0.0.42'))") > +echo ${dns_reply} > expected > + > +as hv1 ovs-appctl netdev-dummy/receive hv1-vif1 ${dns_req} > +OVN_CHECK_PACKETS_REMOVE_BROADCAST([hv1/vif1-tx.pcap], [expected]) > + > +OVN_CLEANUP([hv1]) > +AT_CLEANUP > +]) > + > OVN_FOR_EACH_NORTHD([ > AT_SETUP([4 HV, 1 LS, 1 LR, packet test with HA distributed router gateway port]) > ovn_start
On 2/9/24 20:34, Mark Michelson wrote: > Thank you for the updates, Dumitru. > > Acked-by: Mark Michelson <mmichels@redhat.com> > Thanks, Mark, applied to main and backported to all stable branches down to 22.03. Regards, Dumitru
diff --git a/controller/pinctrl.c b/controller/pinctrl.c index faa3f92260..98b29de9ff 100644 --- a/controller/pinctrl.c +++ b/controller/pinctrl.c @@ -2887,6 +2887,7 @@ dns_build_ptr_answer( } #define DNS_RCODE_SERVER_REFUSE 0x5 +#define DNS_QUERY_TYPE_CLASS_LEN (2 * sizeof(ovs_be16)) /* Called with in the pinctrl_handler thread context. */ static void @@ -2950,18 +2951,13 @@ pinctrl_handle_dns_lookup( goto exit; } - /* Check if there is an additional record present, which is unsupported */ - if (in_dns_header->arcount) { - VLOG_DBG_RL(&rl, "Received DNS query with additional records, which" - " is unsupported"); - goto exit; - } - struct udp_header *in_udp = dp_packet_l4(pkt_in); size_t udp_len = ntohs(in_udp->udp_len); size_t l4_len = dp_packet_l4_size(pkt_in); + uint8_t *l4_start = (uint8_t *) in_udp; uint8_t *end = (uint8_t *)in_udp + MIN(udp_len, l4_len); uint8_t *in_dns_data = (uint8_t *)(in_dns_header + 1); + uint8_t *in_dns_data_start = in_dns_data; uint8_t *in_queryname = in_dns_data; uint16_t idx = 0; struct ds query_name; @@ -2985,7 +2981,7 @@ pinctrl_handle_dns_lookup( in_dns_data += idx; /* Query should have TYPE and CLASS fields */ - if (in_dns_data + (2 * sizeof(ovs_be16)) > end) { + if (in_dns_data + DNS_QUERY_TYPE_CLASS_LEN > end) { ds_destroy(&query_name); goto exit; } @@ -2999,6 +2995,10 @@ pinctrl_handle_dns_lookup( goto exit; } + uint8_t *rest = in_dns_data + DNS_QUERY_TYPE_CLASS_LEN; + uint32_t query_size = rest - in_dns_data_start; + uint32_t query_l4_size = rest - l4_start; + uint64_t dp_key = ntohll(pin->flow_metadata.flow.metadata); const char *answer_data = NULL; bool ovn_owned = false; @@ -3081,7 +3081,7 @@ pinctrl_handle_dns_lookup( goto exit; } - uint16_t new_l4_size = ntohs(in_udp->udp_len) + dns_answer.size; + uint16_t new_l4_size = query_l4_size + dns_answer.size; size_t new_packet_size = pkt_in->l4_ofs + new_l4_size; struct dp_packet pkt_out; dp_packet_init(&pkt_out, new_packet_size); @@ -3118,7 +3118,7 @@ pinctrl_handle_dns_lookup( out_dns_header->arcount = 0; /* Copy the Query section. */ - dp_packet_put(&pkt_out, dp_packet_data(pkt_in), dp_packet_size(pkt_in)); + dp_packet_put(&pkt_out, dp_packet_data(pkt_in), query_size); /* Copy the answer sections. */ dp_packet_put(&pkt_out, dns_answer.data, dns_answer.size); diff --git a/tests/ovn.at b/tests/ovn.at index 2cf335cf45..6b0b80bfc1 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -11455,6 +11455,57 @@ OVN_CLEANUP([hv1]) AT_CLEANUP ]) +OVN_FOR_EACH_NORTHD([ +AT_SETUP([dns lookup : EDNS]) +AT_SKIP_IF([test $HAVE_SCAPY = no]) +ovn_start + +check ovn-nbctl ls-add ls \ + -- lsp-add ls lsp \ + -- lsp-set-addresses lsp "00:00:00:00:00:01 10.0.0.1" + +d=$(ovn-nbctl create dns records={}) + +check ovn-nbctl set dns $d records:foo.ovn.org="10.0.0.42" +check ovn-nbctl set Logical_switch ls dns_records="$d" + +net_add n1 +sim_add hv1 + +as hv1 +ovs-vsctl add-br br-phys +ovn_attach n1 br-phys 192.168.0.1 +ovs-vsctl -- add-port br-int hv1-vif1 -- \ + set interface hv1-vif1 external-ids:iface-id=lsp \ + options:tx_pcap=hv1/vif1-tx.pcap \ + options:rxq_pcap=hv1/vif1-rx.pcap + +OVN_POPULATE_ARP +wait_for_ports_up +check ovn-nbctl --wait=hv sync + +dns_req=$(fmt_pkt "Ether(dst='00:00:00:00:00:02', src='00:00:00:00:00:01') / \ + IP(dst='10.0.0.254', src='10.0.0.1') / \ + UDP(sport=42424, dport=53) / \ + DNS(rd=1, qd=DNSQR(qname='foo.ovn.org'), arcount=1, ar=[ \ + DNSRR(type='OPT', rclass=4096, \ + rdata=EDNS0ClientSubnet(source_plen=24, \ + address='10.0.0.1'))])") +dns_reply=$(fmt_pkt "Ether(dst='00:00:00:00:00:01', src='00:00:00:00:00:02') / \ + IP(dst='10.0.0.1', src='10.0.0.254') / \ + UDP(sport=53, dport=42424,chksum=0) / \ + DNS(qr=1, qd=DNSQR(qname='foo.ovn.org'), \ + an=DNSRR(rrname='foo.ovn.org', type='A', ttl=3600, \ + rdata='10.0.0.42'))") +echo ${dns_reply} > expected + +as hv1 ovs-appctl netdev-dummy/receive hv1-vif1 ${dns_req} +OVN_CHECK_PACKETS_REMOVE_BROADCAST([hv1/vif1-tx.pcap], [expected]) + +OVN_CLEANUP([hv1]) +AT_CLEANUP +]) + OVN_FOR_EACH_NORTHD([ AT_SETUP([4 HV, 1 LS, 1 LR, packet test with HA distributed router gateway port]) ovn_start
EDNS is backwards compatible so it's safe to just ignore additional ARs. Reported-at: https://github.com/ovn-org/ovn/issues/228 Reported-at: https://issues.redhat.com/browse/FDP-222 Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- V2: - Addressed Mark's comments: - fixed subject - added test --- controller/pinctrl.c | 20 ++++++++--------- tests/ovn.at | 51 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 10 deletions(-)