[ovs-dev] conntrack: check the result of extract_l3_ipv4/6
diff mbox series

Message ID 1566194488-23810-1-git-send-email-lirongqing@baidu.com
State New
Headers show
Series
  • [ovs-dev] conntrack: check the result of extract_l3_ipv4/6
Related show

Commit Message

Li RongQing Aug. 19, 2019, 6:01 a.m. UTC
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(-)

Comments

Darrell Ball Aug. 19, 2019, 3:35 p.m. UTC | #1
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
>
Ben Pfaff Aug. 21, 2019, 10:13 p.m. UTC | #2
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.
Darrell Ball Aug. 22, 2019, 12:59 a.m. UTC | #3
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 ?
Li RongQing Aug. 22, 2019, 5:28 a.m. UTC | #4
发件人: 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

Patch
diff mbox series

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;