On 9/21/17, 4:01 AM, "ovs-dev-boun...@openvswitch.org on behalf of Fischetti, 
Antonio" <ovs-dev-boun...@openvswitch.org on behalf of 
antonio.fische...@intel.com> wrote:

    Looks a good improvement to report on an intentional exploit, or 
    other issues.
    LGTM, just one comment. Now this new print fn is called just from 
    create_un_nat_conn but in the future it could potentially be called 
    from any other CT function. As the function call could affect the
    performance, especially for the memcpy when dl_type != ETH_TYPE_IP, 
    I was wondering if print_conn_info could be limited to work over a 
    certain info level. Could something like the following help on this?
     
    void
    print_conn_info(struct conn *c, char *log_msg)
    {
    +    if (!VLOG_IS_DBG_ENABLED()) {
    +        return;
    +    }
    

Good idea, thanks.

I think we go one step further and pass in the log level and do the checking
you mentioned against the requested log level.


BTW: I ended up improving the V6 address print handling to the following:

=========================

 char ip6_s0[INET6_ADDRSTRLEN];
 inet_ntop(AF_INET6, &c->key.src.addr.ipv6, ip6_s0, sizeof ip6_s0);
 char ip6_s1[INET6_ADDRSTRLEN];
 inet_ntop(AF_INET6, &c->key.dst.addr.ipv6, ip6_s1, sizeof ip6_s1);

  char ip6_s2[INET6_ADDRSTRLEN];
  inet_ntop(AF_INET6, &c->rev_key.src.addr.ipv6, ip6_s2, sizeof ip6_s2);
  char ip6_s3[INET6_ADDRSTRLEN];
  inet_ntop(AF_INET6, &c->rev_key.dst.addr.ipv6, ip6_s3, sizeof ip6_s3);

  VLOG_INFO("src addr %s; dst addr %s; rev src addr %s; rev dst addr %s",
                        ip6_s0, ip6_s1, ip6_s2, ip6_s3);

=========================

Thanks Darrell

        ..
    
    
    Acked-by: Antonio Fischetti <antonio.fische...@intel.com>
    
    
    > -----Original Message-----
    > From: ovs-dev-boun...@openvswitch.org 
[mailto:ovs-dev-boun...@openvswitch.org]
    > On Behalf Of Darrell Ball
    > Sent: Thursday, September 21, 2017 8:12 AM
    > To: dlu...@gmail.com; d...@openvswitch.org
    > Subject: [ovs-dev] [patch v2 4/5] conntrack: Add function 
print_conn_info().
    > 
    > A new debug function is added and used in a
    > subsequent patch.
    > 
    > Signed-off-by: Darrell Ball <dlu...@gmail.com>
    > ---
    >  lib/conntrack.c | 53 
+++++++++++++++++++++++++++++++++++++++++++++++++++++
    >  1 file changed, 53 insertions(+)
    > 
    > diff --git a/lib/conntrack.c b/lib/conntrack.c
    > index 2eca38d..8deeec9 100644
    > --- a/lib/conntrack.c
    > +++ b/lib/conntrack.c
    > @@ -67,6 +67,8 @@ enum ct_alg_mode {
    >      CT_TFTP_MODE,
    >  };
    > 
    > +void print_conn_info(struct conn *c, char *log_msg);
    > +
    >  static bool conn_key_extract(struct conntrack *, struct dp_packet *,
    >                               ovs_be16 dl_type, struct conn_lookup_ctx *,
    >                               uint16_t zone);
    > @@ -223,6 +225,57 @@ conn_key_cmp(const struct conn_key *key1, const 
struct
    > conn_key *key2)
    >      return 1;
    >  }
    > 
    > +void
    > +print_conn_info(struct conn *c, char *log_msg)
    > +{
    > +    VLOG_INFO("%s", log_msg);
    > +    if (c->key.dl_type == htons(ETH_TYPE_IP)) {
    > +        VLOG_INFO("src addr "IP_FMT " dst addr "IP_FMT
    > +                  " rev src addr "IP_FMT " rev dst addr "IP_FMT,
    > +                  IP_ARGS(c->key.src.addr.ipv4_aligned),
    > +                  IP_ARGS(c->key.dst.addr.ipv4_aligned),
    > +                  IP_ARGS(c->rev_key.src.addr.ipv4_aligned),
    > +                  IP_ARGS(c->rev_key.dst.addr.ipv4_aligned));
    > +    } else {
    > +        ovs_be32 a0, a1, a2, a3, a4, a5, a6, a7, a8, a9, a10, a11, a12, 
a13;
    > +        ovs_be32 a14, a15;
    > +        memcpy(&a0, &c->key.src.addr.ipv6_aligned.s6_addr[0], sizeof a0);
    > +        memcpy(&a1, &c->key.src.addr.ipv6_aligned.s6_addr[4], sizeof a1);
    > +        memcpy(&a2, &c->key.src.addr.ipv6_aligned.s6_addr[8], sizeof a2);
    > +        memcpy(&a3, &c->key.src.addr.ipv6_aligned.s6_addr[12], sizeof 
a3);
    > +        memcpy(&a4, &c->key.dst.addr.ipv6_aligned.s6_addr[0], sizeof a4);
    > +        memcpy(&a5, &c->key.dst.addr.ipv6_aligned.s6_addr[4], sizeof a5);
    > +        memcpy(&a6, &c->key.dst.addr.ipv6_aligned.s6_addr[8], sizeof a6);
    > +        memcpy(&a7, &c->key.dst.addr.ipv6_aligned.s6_addr[12], sizeof 
a7);
    > +        memcpy(&a8, &c->rev_key.src.addr.ipv6_aligned.s6_addr[0],
    > +               sizeof a8);
    > +        memcpy(&a9, &c->rev_key.src.addr.ipv6_aligned.s6_addr[4],
    > +               sizeof a9);
    > +        memcpy(&a10, &c->rev_key.src.addr.ipv6_aligned.s6_addr[8],
    > +               sizeof a10);
    > +        memcpy(&a11, &c->rev_key.src.addr.ipv6_aligned.s6_addr[12],
    > +               sizeof a11);
    > +        memcpy(&a12, &c->rev_key.dst.addr.ipv6_aligned.s6_addr[0],
    > +               sizeof a12);
    > +        memcpy(&a13, &c->rev_key.dst.addr.ipv6_aligned.s6_addr[4],
    > +               sizeof a13);
    > +        memcpy(&a14, &c->rev_key.dst.addr.ipv6_aligned.s6_addr[8],
    > +               sizeof a14);
    > +        memcpy(&a15, &c->rev_key.dst.addr.ipv6_aligned.s6_addr[12],
    > +               sizeof a15);
    > +
    > +        VLOG_INFO("src addr 0x%08x:%08x:%08x:%08x; "
    > +                  "dst addr 0x%08x:%08x:%08x:%08x; "
    > +                  "rev src addr 0x%08x:%08x:%08x:%08x; "
    > +                  "rev dst addr 0x%08x:%08x:%08x:%08x", ntohl(a0), 
ntohl(a1),
    > +                  ntohl(a2), ntohl(a3), ntohl(a4), ntohl(a5), ntohl(a6),
    > +                  ntohl(a7), ntohl(a8), ntohl(a9), ntohl(a10), 
ntohl(a11),
    > +                  ntohl(a12), ntohl(a13), ntohl(a14), ntohl(a15));
    > +    }
    > +    VLOG_INFO("src/dst ports %d/%d rev src/dst ports %d/%d", 
c->key.src.port,
    > +              c->key.dst.port, c->rev_key.src.port, c->rev_key.dst.port);
    > +}
    > +
    >  /* Initializes the connection tracker 'ct'.  The caller is responsible 
for
    >   * calling 'conntrack_destroy()', when the instance is not needed 
anymore */
    >  void
    > --
    > 1.9.1
    > 
    > _______________________________________________
    > dev mailing list
    > d...@openvswitch.org
    > 
https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=KJTV1pFisuVRJMYQggKXHW2nx5C-1FVotX7ZrszEYlM&s=QpT44FCD4Eg8jtTYFEn6ySJOOfO1uwKYKW7DEEczf_c&e=
 
    _______________________________________________
    dev mailing list
    d...@openvswitch.org
    
https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=KJTV1pFisuVRJMYQggKXHW2nx5C-1FVotX7ZrszEYlM&s=QpT44FCD4Eg8jtTYFEn6ySJOOfO1uwKYKW7DEEczf_c&e=
 
    

_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to