[ovs-dev,v1] conntrack: Fix 'check_orig_tuple()' Valgrind false positive.
diff mbox series

Message ID 1569282273-15175-1-git-send-email-dlu998@gmail.com
State New
Headers show
Series
  • [ovs-dev,v1] conntrack: Fix 'check_orig_tuple()' Valgrind false positive.
Related show

Commit Message

Darrell Ball Sept. 23, 2019, 11:44 p.m. UTC
Valgrind reported that 'pkt->md.ct_orig_tuple.ipv4.ipv4_proto' is
uninitialized in 'check_orig_tuple()', if 'ct_state' is zero.  Although
this is true, the check is superceded, as even if it succeeds the check
for natted packets based on 'ct_state' is an ORed condition and is intended
to catch this case.
The check is '!(pkt->md.ct_state & (CS_SRC_NAT | CS_DST_NAT))' which
filters out all packets excepted natted ones.  Move this check up to
prevent the Valgrind complaint, which also helps performance and also remove
recenlty added redundant check adding extra cycles.

Fixes: f44733c527da ("conntrack: Validate accessing of conntrack data in pkt_metadata.")
CC: Yifeng Sun <pkusunyifeng@gmail.com>
Signed-off-by: Darrell Ball <dlu998@gmail.com>
---
 lib/conntrack.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Comments

Ben Pfaff Sept. 24, 2019, 7:57 p.m. UTC | #1
On Mon, Sep 23, 2019 at 04:44:33PM -0700, Darrell Ball wrote:
> Valgrind reported that 'pkt->md.ct_orig_tuple.ipv4.ipv4_proto' is
> uninitialized in 'check_orig_tuple()', if 'ct_state' is zero.  Although
> this is true, the check is superceded, as even if it succeeds the check
> for natted packets based on 'ct_state' is an ORed condition and is intended
> to catch this case.
> The check is '!(pkt->md.ct_state & (CS_SRC_NAT | CS_DST_NAT))' which
> filters out all packets excepted natted ones.  Move this check up to
> prevent the Valgrind complaint, which also helps performance and also remove
> recenlty added redundant check adding extra cycles.
> 
> Fixes: f44733c527da ("conntrack: Validate accessing of conntrack data in pkt_metadata.")
> CC: Yifeng Sun <pkusunyifeng@gmail.com>
> Signed-off-by: Darrell Ball <dlu998@gmail.com>

Thanks, applied to master.
Ben Pfaff Sept. 24, 2019, 8:31 p.m. UTC | #2
Done!

On Tue, Sep 24, 2019 at 02:56:40PM -0700, Darrell Ball wrote:
> Thanks Ben
> 
> Would you mind applying to 2.12 as well.
> 
> Darrell
> 
> On Tue, Sep 24, 2019 at 2:34 PM Ben Pfaff <blp@ovn.org> wrote:
> 
> > On Mon, Sep 23, 2019 at 04:44:33PM -0700, Darrell Ball wrote:
> > > Valgrind reported that 'pkt->md.ct_orig_tuple.ipv4.ipv4_proto' is
> > > uninitialized in 'check_orig_tuple()', if 'ct_state' is zero.  Although
> > > this is true, the check is superceded, as even if it succeeds the check
> > > for natted packets based on 'ct_state' is an ORed condition and is
> > intended
> > > to catch this case.
> > > The check is '!(pkt->md.ct_state & (CS_SRC_NAT | CS_DST_NAT))' which
> > > filters out all packets excepted natted ones.  Move this check up to
> > > prevent the Valgrind complaint, which also helps performance and also
> > remove
> > > recenlty added redundant check adding extra cycles.
> > >
> > > Fixes: f44733c527da ("conntrack: Validate accessing of conntrack data in
> > pkt_metadata.")
> > > CC: Yifeng Sun <pkusunyifeng@gmail.com>
> > > Signed-off-by: Darrell Ball <dlu998@gmail.com>
> >
> > Thanks, applied to master.
> >
Darrell Ball Sept. 24, 2019, 9:56 p.m. UTC | #3
Thanks Ben

Would you mind applying to 2.12 as well.

Darrell

On Tue, Sep 24, 2019 at 2:34 PM Ben Pfaff <blp@ovn.org> wrote:

> On Mon, Sep 23, 2019 at 04:44:33PM -0700, Darrell Ball wrote:
> > Valgrind reported that 'pkt->md.ct_orig_tuple.ipv4.ipv4_proto' is
> > uninitialized in 'check_orig_tuple()', if 'ct_state' is zero.  Although
> > this is true, the check is superceded, as even if it succeeds the check
> > for natted packets based on 'ct_state' is an ORed condition and is
> intended
> > to catch this case.
> > The check is '!(pkt->md.ct_state & (CS_SRC_NAT | CS_DST_NAT))' which
> > filters out all packets excepted natted ones.  Move this check up to
> > prevent the Valgrind complaint, which also helps performance and also
> remove
> > recenlty added redundant check adding extra cycles.
> >
> > Fixes: f44733c527da ("conntrack: Validate accessing of conntrack data in
> pkt_metadata.")
> > CC: Yifeng Sun <pkusunyifeng@gmail.com>
> > Signed-off-by: Darrell Ball <dlu998@gmail.com>
>
> Thanks, applied to master.
>

Patch
diff mbox series

diff --git a/lib/conntrack.c b/lib/conntrack.c
index fd71e6c..b56ef06 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -1005,11 +1005,11 @@  check_orig_tuple(struct conntrack *ct, struct dp_packet *pkt,
                  struct conn **conn,
                  const struct nat_action_info_t *nat_action_info)
 {
-    if ((ctx_in->key.dl_type == htons(ETH_TYPE_IP) &&
+    if (!(pkt->md.ct_state & (CS_SRC_NAT | CS_DST_NAT)) ||
+        (ctx_in->key.dl_type == htons(ETH_TYPE_IP) &&
          !pkt->md.ct_orig_tuple.ipv4.ipv4_proto) ||
         (ctx_in->key.dl_type == htons(ETH_TYPE_IPV6) &&
          !pkt->md.ct_orig_tuple.ipv6.ipv6_proto) ||
-        !(pkt->md.ct_state & (CS_SRC_NAT | CS_DST_NAT)) ||
         nat_action_info) {
         return false;
     }
@@ -1142,8 +1142,7 @@  process_one(struct conntrack *ct, struct dp_packet *pkt,
             handle_nat(pkt, conn, zone, ctx->reply, ctx->icmp_related);
         }
 
-    } else if (pkt->md.ct_state
-               && check_orig_tuple(ct, pkt, ctx, now, &conn, nat_action_info)) {
+    } else if (check_orig_tuple(ct, pkt, ctx, now, &conn, nat_action_info)) {
         create_new_conn = conn_update_state(ct, pkt, ctx, conn, now);
     } else {
         if (ctx->icmp_related) {