| Message ID | 1569282273-15175-1-git-send-email-dlu998@gmail.com |
|---|---|
| State | Accepted |
| Commit | a0b89c5139aba1e4ea1fa1bf79c69b4b20cc492f |
| Headers | show |
| Series | [ovs-dev,v1] conntrack: Fix 'check_orig_tuple()' Valgrind false positive. | expand |
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.
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. > >
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. >
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) {
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(-)