> -----Original Message-----
> From: Darrell Ball [mailto:db...@vmware.com]
> Sent: Thursday, September 21, 2017 7:56 PM
> To: Fischetti, Antonio <antonio.fische...@intel.com>; Darrell Ball
> <dlu...@gmail.com>; d...@openvswitch.org
> Subject: Re: [ovs-dev] [patch v2 4/5] conntrack: Add function
> print_conn_info().
> 
> 
> 
> 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);
> 
> =========================
> 
[Antonio]
yes, looks good.


> 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