| Message ID | 1566194488-23810-1-git-send-email-lirongqing@baidu.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [ovs-dev] conntrack: check the result of extract_l3_ipv4/6 | expand |
Thanks for the patch On Sun, Aug 18, 2019 at 11:01 PM Li RongQing <lirongqing@baidu.com> wrote: > the result of extract_l3_ipv4/6 should be checked in reverse_nat_packet > when it is false, meaning this packet is wrong, should not do handle it > continually > > Signed-off-by: Li RongQing <lirongqing@baidu.com> > --- > lib/conntrack.c | 17 +++++++++++++++-- > 1 file changed, 15 insertions(+), 2 deletions(-) > > diff --git a/lib/conntrack.c b/lib/conntrack.c > index 5f60fea18..c26d5438c 100644 > --- a/lib/conntrack.c > +++ b/lib/conntrack.c > @@ -695,11 +695,18 @@ reverse_nat_packet(struct dp_packet *pkt, const > struct conn *conn) > uint16_t orig_l4_ofs = pkt->l4_ofs; > > if (conn->key.dl_type == htons(ETH_TYPE_IP)) { > + bool ok; > struct ip_header *nh = dp_packet_l3(pkt); > struct icmp_header *icmp = dp_packet_l4(pkt); > struct ip_header *inner_l3 = (struct ip_header *) (icmp + 1); > - extract_l3_ipv4(&inner_key, inner_l3, tail - ((char *)inner_l3) - > pad, > There is intentionally no checking for success/fail here bcoz the packet has already been parsed and found to be ok during conn_key_extract() code path. Reusing the same api here is just convenient. > + > + ok = extract_l3_ipv4(&inner_key, inner_l3, > + tail - ((char *)inner_l3) - pad, > &inner_l4, false); > + if (!ok) { > + return; > + } > + > pkt->l3_ofs += (char *) inner_l3 - (char *) nh; > pkt->l4_ofs += inner_l4 - (char *) icmp; > > @@ -715,13 +722,19 @@ reverse_nat_packet(struct dp_packet *pkt, const > struct conn *conn) > icmp->icmp_csum = 0; > icmp->icmp_csum = csum(icmp, tail - (char *) icmp - pad); > } else { > + bool ok; > struct ovs_16aligned_ip6_hdr *nh6 = dp_packet_l3(pkt); > struct icmp6_error_header *icmp6 = dp_packet_l4(pkt); > struct ovs_16aligned_ip6_hdr *inner_l3_6 = > (struct ovs_16aligned_ip6_hdr *) (icmp6 + 1); > - extract_l3_ipv6(&inner_key, inner_l3_6, > There is intentionally no checking for success/fail here bcoz the packet has already been parsed and found to be ok during conn_key_extract() code path. Reusing the same api here is just convenient. > + > + ok = extract_l3_ipv6(&inner_key, inner_l3_6, > tail - ((char *)inner_l3_6) - pad, > &inner_l4); > + > + if (!ok) { > + return; > + } > pkt->l3_ofs += (char *) inner_l3_6 - (char *) nh6; > pkt->l4_ofs += inner_l4 - (char *) icmp6; > > -- > 2.16.2 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Mon, Aug 19, 2019 at 08:35:11AM -0700, Darrell Ball wrote: > Thanks for the patch > > On Sun, Aug 18, 2019 at 11:01 PM Li RongQing <lirongqing@baidu.com> wrote: > > > the result of extract_l3_ipv4/6 should be checked in reverse_nat_packet > > when it is false, meaning this packet is wrong, should not do handle it > > continually > > > > Signed-off-by: Li RongQing <lirongqing@baidu.com> > > --- > > lib/conntrack.c | 17 +++++++++++++++-- > > 1 file changed, 15 insertions(+), 2 deletions(-) > > > > diff --git a/lib/conntrack.c b/lib/conntrack.c > > index 5f60fea18..c26d5438c 100644 > > --- a/lib/conntrack.c > > +++ b/lib/conntrack.c > > @@ -695,11 +695,18 @@ reverse_nat_packet(struct dp_packet *pkt, const > > struct conn *conn) > > uint16_t orig_l4_ofs = pkt->l4_ofs; > > > > if (conn->key.dl_type == htons(ETH_TYPE_IP)) { > > + bool ok; > > struct ip_header *nh = dp_packet_l3(pkt); > > struct icmp_header *icmp = dp_packet_l4(pkt); > > struct ip_header *inner_l3 = (struct ip_header *) (icmp + 1); > > - extract_l3_ipv4(&inner_key, inner_l3, tail - ((char *)inner_l3) - > > pad, > > > > There is intentionally no checking for success/fail here bcoz the packet > has already > been parsed and found to be ok during conn_key_extract() code path. Reusing > the > same api here is just convenient. Maybe a comment would be warranted to make that clear.
On Wed, Aug 21, 2019 at 3:13 PM Ben Pfaff <blp@ovn.org> wrote: > On Mon, Aug 19, 2019 at 08:35:11AM -0700, Darrell Ball wrote: > > Thanks for the patch > > > > On Sun, Aug 18, 2019 at 11:01 PM Li RongQing <lirongqing@baidu.com> > wrote: > > > > > the result of extract_l3_ipv4/6 should be checked in reverse_nat_packet > > > when it is false, meaning this packet is wrong, should not do handle it > > > continually > > > > > > Signed-off-by: Li RongQing <lirongqing@baidu.com> > > > --- > > > lib/conntrack.c | 17 +++++++++++++++-- > > > 1 file changed, 15 insertions(+), 2 deletions(-) > > > > > > diff --git a/lib/conntrack.c b/lib/conntrack.c > > > index 5f60fea18..c26d5438c 100644 > > > --- a/lib/conntrack.c > > > +++ b/lib/conntrack.c > > > @@ -695,11 +695,18 @@ reverse_nat_packet(struct dp_packet *pkt, const > > > struct conn *conn) > > > uint16_t orig_l4_ofs = pkt->l4_ofs; > > > > > > if (conn->key.dl_type == htons(ETH_TYPE_IP)) { > > > + bool ok; > > > struct ip_header *nh = dp_packet_l3(pkt); > > > struct icmp_header *icmp = dp_packet_l4(pkt); > > > struct ip_header *inner_l3 = (struct ip_header *) (icmp + 1); > > > - extract_l3_ipv4(&inner_key, inner_l3, tail - ((char > *)inner_l3) - > > > pad, > > > > > > > There is intentionally no checking for success/fail here bcoz the packet > > has already > > been parsed and found to be ok during conn_key_extract() code path. > Reusing > > the > > same api here is just convenient. > > Maybe a comment would be warranted to make that clear. > that's reasonable; maybe RongQing would like to submit a patch ?
发件人: Darrell Ball [mailto:dlu998@gmail.com] 发送时间: 2019年8月22日 9:00 收件人: Ben Pfaff <blp@ovn.org> 抄送: Li,Rongqing <lirongqing@baidu.com>; ovs-dev@openvswitch.org 主题: Re: [ovs-dev] [PATCH] conntrack: check the result of extract_l3_ipv4/6 On Wed, Aug 21, 2019 at 3:13 PM Ben Pfaff <blp@ovn.org<mailto:blp@ovn.org>> wrote: On Mon, Aug 19, 2019 at 08:35:11AM -0700, Darrell Ball wrote: > Thanks for the patch > > On Sun, Aug 18, 2019 at 11:01 PM Li RongQing <lirongqing@baidu.com<mailto:lirongqing@baidu.com>> wrote: > > > the result of extract_l3_ipv4/6 should be checked in reverse_nat_packet > > when it is false, meaning this packet is wrong, should not do handle it > > continually > > > > Signed-off-by: Li RongQing <lirongqing@baidu.com<mailto:lirongqing@baidu.com>> > > --- > > lib/conntrack.c | 17 +++++++++++++++-- > > 1 file changed, 15 insertions(+), 2 deletions(-) > > > > diff --git a/lib/conntrack.c b/lib/conntrack.c > > index 5f60fea18..c26d5438c 100644 > > --- a/lib/conntrack.c > > +++ b/lib/conntrack.c > > @@ -695,11 +695,18 @@ reverse_nat_packet(struct dp_packet *pkt, const > > struct conn *conn) > > uint16_t orig_l4_ofs = pkt->l4_ofs; > > > > if (conn->key.dl_type == htons(ETH_TYPE_IP)) { > > + bool ok; > > struct ip_header *nh = dp_packet_l3(pkt); > > struct icmp_header *icmp = dp_packet_l4(pkt); > > struct ip_header *inner_l3 = (struct ip_header *) (icmp + 1); > > - extract_l3_ipv4(&inner_key, inner_l3, tail - ((char *)inner_l3) - > > pad, > > > > There is intentionally no checking for success/fail here bcoz the packet > has already > been parsed and found to be ok during conn_key_extract() code path. Reusing > the > same api here is just convenient. Maybe a comment would be warranted to make that clear. that's reasonable; maybe RongQing would like to submit a patch ? Ball: I like you finish it, since you are more clear this codes. Thanks -RongQing
diff --git a/lib/conntrack.c b/lib/conntrack.c index 5f60fea18..c26d5438c 100644 --- a/lib/conntrack.c +++ b/lib/conntrack.c @@ -695,11 +695,18 @@ reverse_nat_packet(struct dp_packet *pkt, const struct conn *conn) uint16_t orig_l4_ofs = pkt->l4_ofs; if (conn->key.dl_type == htons(ETH_TYPE_IP)) { + bool ok; struct ip_header *nh = dp_packet_l3(pkt); struct icmp_header *icmp = dp_packet_l4(pkt); struct ip_header *inner_l3 = (struct ip_header *) (icmp + 1); - extract_l3_ipv4(&inner_key, inner_l3, tail - ((char *)inner_l3) - pad, + + ok = extract_l3_ipv4(&inner_key, inner_l3, + tail - ((char *)inner_l3) - pad, &inner_l4, false); + if (!ok) { + return; + } + pkt->l3_ofs += (char *) inner_l3 - (char *) nh; pkt->l4_ofs += inner_l4 - (char *) icmp; @@ -715,13 +722,19 @@ reverse_nat_packet(struct dp_packet *pkt, const struct conn *conn) icmp->icmp_csum = 0; icmp->icmp_csum = csum(icmp, tail - (char *) icmp - pad); } else { + bool ok; struct ovs_16aligned_ip6_hdr *nh6 = dp_packet_l3(pkt); struct icmp6_error_header *icmp6 = dp_packet_l4(pkt); struct ovs_16aligned_ip6_hdr *inner_l3_6 = (struct ovs_16aligned_ip6_hdr *) (icmp6 + 1); - extract_l3_ipv6(&inner_key, inner_l3_6, + + ok = extract_l3_ipv6(&inner_key, inner_l3_6, tail - ((char *)inner_l3_6) - pad, &inner_l4); + + if (!ok) { + return; + } pkt->l3_ofs += (char *) inner_l3_6 - (char *) nh6; pkt->l4_ofs += inner_l4 - (char *) icmp6;
the result of extract_l3_ipv4/6 should be checked in reverse_nat_packet when it is false, meaning this packet is wrong, should not do handle it continually Signed-off-by: Li RongQing <lirongqing@baidu.com> --- lib/conntrack.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)