diff mbox series

[ovs-dev] datapath-windows: Add trace level logs in conntrack for invalid ct state.

Message ID 20180202211854.4588-1-kumaranand@vmware.com
State Changes Requested
Delegated to: Alin Gabriel Serdean
Headers show
Series [ovs-dev] datapath-windows: Add trace level logs in conntrack for invalid ct state. | expand

Commit Message

Anand Kumar Feb. 2, 2018, 9:18 p.m. UTC 
Signed-off-by: Anand Kumar <kumaranand@vmware.com>
---
 datapath-windows/ovsext/Conntrack-icmp.c | 1 +
 datapath-windows/ovsext/Conntrack-tcp.c  | 4 ++++
 datapath-windows/ovsext/Conntrack.c      | 4 ++++
 3 files changed, 9 insertions(+)

Comments

Alin Serdean Feb. 2, 2018, 10:06 p.m. UTC | #1 
Looks good just a small nit:
s/syn/SYN/g
s/ack/ACK/g

Also I would prefer if you drop the text `Invalid!`. Either just remove it or expand it, i.e.:
"Invalid! ICMPhdr cannot be NULL" => "Invalid ICMP packet detected the header cannot be NULL"

Thanks,
Alin.

-----Mesaj original-----
De la: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-bounces@openvswitch.org] În numele Anand Kumar
Trimis: Friday, February 2, 2018 11:19 PM
Către: dev@openvswitch.org
Subiect: [ovs-dev] [PATCH] datapath-windows: Add trace level logs in conntrack for invalid ct state.

Signed-off-by: Anand Kumar <kumaranand@vmware.com>
---
 datapath-windows/ovsext/Conntrack-icmp.c | 1 +  datapath-windows/ovsext/Conntrack-tcp.c  | 4 ++++
 datapath-windows/ovsext/Conntrack.c      | 4 ++++
 3 files changed, 9 insertions(+)

diff --git a/datapath-windows/ovsext/Conntrack-icmp.c b/datapath-windows/ovsext/Conntrack-icmp.c
index 4da0665..d86feed 100644
--- a/datapath-windows/ovsext/Conntrack-icmp.c
+++ b/datapath-windows/ovsext/Conntrack-icmp.c
@@ -61,6 +61,7 @@ BOOLEAN
 OvsConntrackValidateIcmpPacket(const ICMPHdr *icmp)  {
     if (!icmp) {
+        OVS_LOG_TRACE("Invalid! ICMPhdr cannot be NULL");
         return FALSE;
     }
 
diff --git a/datapath-windows/ovsext/Conntrack-tcp.c b/datapath-windows/ovsext/Conntrack-tcp.c
index f8e85a2..65eaac5 100644
--- a/datapath-windows/ovsext/Conntrack-tcp.c
+++ b/datapath-windows/ovsext/Conntrack-tcp.c
@@ -444,12 +444,14 @@ BOOLEAN
 OvsConntrackValidateTcpPacket(const TCPHdr *tcp)  {
     if (!tcp) {
+        OVS_LOG_TRACE("Invalid! TCPHdr cannot be NULL");
         return FALSE;
     }
 
     UINT16 tcp_flags = ntohs(tcp->flags);
 
     if (OvsCtInvalidTcpFlags(tcp_flags)) {
+        OVS_LOG_TRACE("Invalid! tcp_flags %hu", tcp_flags);
         return FALSE;
     }
 
@@ -457,6 +459,8 @@ OvsConntrackValidateTcpPacket(const TCPHdr *tcp)
      * totally new connections (syn) or already established, not partially
      * open (syn+ack). */
     if ((tcp_flags & TCP_SYN) && (tcp_flags & TCP_ACK)) {
+        OVS_LOG_TRACE("Invalid! syn+ack flags not allowed, tcp_flags %hu",
+                      tcp_flags);
         return FALSE;
     }
 
diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-windows/ovsext/Conntrack.c
index 43c9dd3..7e413c6 100644
--- a/datapath-windows/ovsext/Conntrack.c
+++ b/datapath-windows/ovsext/Conntrack.c
@@ -317,6 +317,9 @@ OvsCtEntryCreate(OvsForwardingContext *fwdCtx,
         const ICMPHdr *icmp;
         icmp = OvsGetIcmp(curNbl, l4Offset, &storage);
         if (!OvsConntrackValidateIcmpPacket(icmp)) {
+            if(icmp) {
+                OVS_LOG_TRACE("Invalid! icmp->type %u", icmp->type);
+            }
             state = OVS_CS_F_INVALID;
             break;
         }
@@ -334,6 +337,7 @@ OvsCtEntryCreate(OvsForwardingContext *fwdCtx,
         break;
     }
     default:
+        OVS_LOG_TRACE("Invalid! Not supported protocol, ipProto %u", 
+ ipProto);
         state = OVS_CS_F_INVALID;
         break;
     }
--
2.9.3.windows.1
Anand Kumar Feb. 2, 2018, 10:28 p.m. UTC | #2 
Hi Alin,

Thanks for the quick review. I will expand log messages to “Invalid XXX packet detected”.
I’m not sure if I follow your comment about “s/syn/SYN/g”. 

Do you want me to change it to Uppercase?

Thanks,
Anand Kumar

On 2/2/18, 2:06 PM, "Alin Serdean" <aserdean@cloudbasesolutions.com> wrote:

    Looks good just a small nit:
    s/syn/SYN/g
    s/ack/ACK/g
    
    Also I would prefer if you drop the text `Invalid!`. Either just remove it or expand it, i.e.:
    "Invalid! ICMPhdr cannot be NULL" => "Invalid ICMP packet detected the header cannot be NULL"
    
    Thanks,
    Alin.
    
    -----Mesaj original-----
    De la: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-bounces@openvswitch.org] În numele Anand Kumar
    Trimis: Friday, February 2, 2018 11:19 PM
    Către: dev@openvswitch.org
    Subiect: [ovs-dev] [PATCH] datapath-windows: Add trace level logs in conntrack for invalid ct state.
    
    Signed-off-by: Anand Kumar <kumaranand@vmware.com>

    ---
     datapath-windows/ovsext/Conntrack-icmp.c | 1 +  datapath-windows/ovsext/Conntrack-tcp.c  | 4 ++++
     datapath-windows/ovsext/Conntrack.c      | 4 ++++
     3 files changed, 9 insertions(+)
    
    diff --git a/datapath-windows/ovsext/Conntrack-icmp.c b/datapath-windows/ovsext/Conntrack-icmp.c
    index 4da0665..d86feed 100644
    --- a/datapath-windows/ovsext/Conntrack-icmp.c
    +++ b/datapath-windows/ovsext/Conntrack-icmp.c
    @@ -61,6 +61,7 @@ BOOLEAN
     OvsConntrackValidateIcmpPacket(const ICMPHdr *icmp)  {
         if (!icmp) {
    +        OVS_LOG_TRACE("Invalid! ICMPhdr cannot be NULL");
             return FALSE;
         }
     
    diff --git a/datapath-windows/ovsext/Conntrack-tcp.c b/datapath-windows/ovsext/Conntrack-tcp.c
    index f8e85a2..65eaac5 100644
    --- a/datapath-windows/ovsext/Conntrack-tcp.c
    +++ b/datapath-windows/ovsext/Conntrack-tcp.c
    @@ -444,12 +444,14 @@ BOOLEAN
     OvsConntrackValidateTcpPacket(const TCPHdr *tcp)  {
         if (!tcp) {
    +        OVS_LOG_TRACE("Invalid! TCPHdr cannot be NULL");
             return FALSE;
         }
     
         UINT16 tcp_flags = ntohs(tcp->flags);
     
         if (OvsCtInvalidTcpFlags(tcp_flags)) {
    +        OVS_LOG_TRACE("Invalid! tcp_flags %hu", tcp_flags);
             return FALSE;
         }
     
    @@ -457,6 +459,8 @@ OvsConntrackValidateTcpPacket(const TCPHdr *tcp)
          * totally new connections (syn) or already established, not partially
          * open (syn+ack). */
         if ((tcp_flags & TCP_SYN) && (tcp_flags & TCP_ACK)) {
    +        OVS_LOG_TRACE("Invalid! syn+ack flags not allowed, tcp_flags %hu",
    +                      tcp_flags);
             return FALSE;
         }
     
    diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-windows/ovsext/Conntrack.c
    index 43c9dd3..7e413c6 100644
    --- a/datapath-windows/ovsext/Conntrack.c
    +++ b/datapath-windows/ovsext/Conntrack.c
    @@ -317,6 +317,9 @@ OvsCtEntryCreate(OvsForwardingContext *fwdCtx,
             const ICMPHdr *icmp;
             icmp = OvsGetIcmp(curNbl, l4Offset, &storage);
             if (!OvsConntrackValidateIcmpPacket(icmp)) {
    +            if(icmp) {
    +                OVS_LOG_TRACE("Invalid! icmp->type %u", icmp->type);
    +            }
                 state = OVS_CS_F_INVALID;
                 break;
             }
    @@ -334,6 +337,7 @@ OvsCtEntryCreate(OvsForwardingContext *fwdCtx,
             break;
         }
         default:
    +        OVS_LOG_TRACE("Invalid! Not supported protocol, ipProto %u", 
    + ipProto);
             state = OVS_CS_F_INVALID;
             break;
         }
    --
    2.9.3.windows.1
    
    _______________________________________________
    dev mailing list
    dev@openvswitch.org
    https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwIFBA&c=uilaK90D4TOVoH58JNXRgQ&r=Q5z9tBe-nAOpE7LIHSPV8uy5-437agMXvkeHHMkR8Us&m=yuxpORhg-xij1o9VvWANs9QOyywwfr7YO_EDT-QvqQ4&s=ainXigtUgyesCYog0X4639gDSFv1mrY4OTsctKNaH0M&e=
Alin-Gabriel Serdean Feb. 2, 2018, 10:32 p.m. UTC | #3 
-----Mesaj original-----
De la: Anand Kumar [mailto:kumaranand@vmware.com] 
Trimis: Saturday, February 3, 2018 12:28 AM
Către: Alin Serdean <aserdean@cloudbasesolutions.com>; dev@openvswitch.org
Subiect: Re: [ovs-dev] [PATCH] datapath-windows: Add trace level logs in conntrack for invalid ct state.

Hi Alin,

Thanks for the quick review. I will expand log messages to “Invalid XXX packet detected”.
I’m not sure if I follow your comment about “s/syn/SYN/g”. 

Do you want me to change it to Uppercase?
[Alin Serdean] Yes please 😊.

Thanks,
Anand Kumar

On 2/2/18, 2:06 PM, "Alin Serdean" <aserdean@cloudbasesolutions.com> wrote:

    Looks good just a small nit:
    s/syn/SYN/g
    s/ack/ACK/g
    
    Also I would prefer if you drop the text `Invalid!`. Either just remove it or expand it, i.e.:
    "Invalid! ICMPhdr cannot be NULL" => "Invalid ICMP packet detected the header cannot be NULL"
    
    Thanks,
    Alin.
    
    -----Mesaj original-----
    De la: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-bounces@openvswitch.org] În numele Anand Kumar
    Trimis: Friday, February 2, 2018 11:19 PM
    Către: dev@openvswitch.org
    Subiect: [ovs-dev] [PATCH] datapath-windows: Add trace level logs in conntrack for invalid ct state.
    
    Signed-off-by: Anand Kumar <kumaranand@vmware.com>
    ---
     datapath-windows/ovsext/Conntrack-icmp.c | 1 +  datapath-windows/ovsext/Conntrack-tcp.c  | 4 ++++
     datapath-windows/ovsext/Conntrack.c      | 4 ++++
     3 files changed, 9 insertions(+)
    
    diff --git a/datapath-windows/ovsext/Conntrack-icmp.c b/datapath-windows/ovsext/Conntrack-icmp.c
    index 4da0665..d86feed 100644
    --- a/datapath-windows/ovsext/Conntrack-icmp.c
    +++ b/datapath-windows/ovsext/Conntrack-icmp.c
    @@ -61,6 +61,7 @@ BOOLEAN
     OvsConntrackValidateIcmpPacket(const ICMPHdr *icmp)  {
         if (!icmp) {
    +        OVS_LOG_TRACE("Invalid! ICMPhdr cannot be NULL");
             return FALSE;
         }
     
    diff --git a/datapath-windows/ovsext/Conntrack-tcp.c b/datapath-windows/ovsext/Conntrack-tcp.c
    index f8e85a2..65eaac5 100644
    --- a/datapath-windows/ovsext/Conntrack-tcp.c
    +++ b/datapath-windows/ovsext/Conntrack-tcp.c
    @@ -444,12 +444,14 @@ BOOLEAN
     OvsConntrackValidateTcpPacket(const TCPHdr *tcp)  {
         if (!tcp) {
    +        OVS_LOG_TRACE("Invalid! TCPHdr cannot be NULL");
             return FALSE;
         }
     
         UINT16 tcp_flags = ntohs(tcp->flags);
     
         if (OvsCtInvalidTcpFlags(tcp_flags)) {
    +        OVS_LOG_TRACE("Invalid! tcp_flags %hu", tcp_flags);
             return FALSE;
         }
     
    @@ -457,6 +459,8 @@ OvsConntrackValidateTcpPacket(const TCPHdr *tcp)
          * totally new connections (syn) or already established, not partially
          * open (syn+ack). */
         if ((tcp_flags & TCP_SYN) && (tcp_flags & TCP_ACK)) {
    +        OVS_LOG_TRACE("Invalid! syn+ack flags not allowed, tcp_flags %hu",
    +                      tcp_flags);
             return FALSE;
         }
     
    diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-windows/ovsext/Conntrack.c
    index 43c9dd3..7e413c6 100644
    --- a/datapath-windows/ovsext/Conntrack.c
    +++ b/datapath-windows/ovsext/Conntrack.c
    @@ -317,6 +317,9 @@ OvsCtEntryCreate(OvsForwardingContext *fwdCtx,
             const ICMPHdr *icmp;
             icmp = OvsGetIcmp(curNbl, l4Offset, &storage);
             if (!OvsConntrackValidateIcmpPacket(icmp)) {
    +            if(icmp) {
    +                OVS_LOG_TRACE("Invalid! icmp->type %u", icmp->type);
    +            }
                 state = OVS_CS_F_INVALID;
                 break;
             }
    @@ -334,6 +337,7 @@ OvsCtEntryCreate(OvsForwardingContext *fwdCtx,
             break;
         }
         default:
    +        OVS_LOG_TRACE("Invalid! Not supported protocol, ipProto %u", 
    + ipProto);
             state = OVS_CS_F_INVALID;
             break;
         }
    --
    2.9.3.windows.1
    
    _______________________________________________
    dev mailing list
    dev@openvswitch.org
    https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwIFBA&c=uilaK90D4TOVoH58JNXRgQ&r=Q5z9tBe-nAOpE7LIHSPV8uy5-437agMXvkeHHMkR8Us&m=yuxpORhg-xij1o9VvWANs9QOyywwfr7YO_EDT-QvqQ4&s=ainXigtUgyesCYog0X4639gDSFv1mrY4OTsctKNaH0M&e=
diff mbox series

Patch

diff --git a/datapath-windows/ovsext/Conntrack-icmp.c b/datapath-windows/ovsext/Conntrack-icmp.c
index 4da0665..d86feed 100644
--- a/datapath-windows/ovsext/Conntrack-icmp.c
+++ b/datapath-windows/ovsext/Conntrack-icmp.c
@@ -61,6 +61,7 @@  BOOLEAN
 OvsConntrackValidateIcmpPacket(const ICMPHdr *icmp)
 {
     if (!icmp) {
+        OVS_LOG_TRACE("Invalid! ICMPhdr cannot be NULL");
         return FALSE;
     }
 
diff --git a/datapath-windows/ovsext/Conntrack-tcp.c b/datapath-windows/ovsext/Conntrack-tcp.c
index f8e85a2..65eaac5 100644
--- a/datapath-windows/ovsext/Conntrack-tcp.c
+++ b/datapath-windows/ovsext/Conntrack-tcp.c
@@ -444,12 +444,14 @@  BOOLEAN
 OvsConntrackValidateTcpPacket(const TCPHdr *tcp)
 {
     if (!tcp) {
+        OVS_LOG_TRACE("Invalid! TCPHdr cannot be NULL");
         return FALSE;
     }
 
     UINT16 tcp_flags = ntohs(tcp->flags);
 
     if (OvsCtInvalidTcpFlags(tcp_flags)) {
+        OVS_LOG_TRACE("Invalid! tcp_flags %hu", tcp_flags);
         return FALSE;
     }
 
@@ -457,6 +459,8 @@  OvsConntrackValidateTcpPacket(const TCPHdr *tcp)
      * totally new connections (syn) or already established, not partially
      * open (syn+ack). */
     if ((tcp_flags & TCP_SYN) && (tcp_flags & TCP_ACK)) {
+        OVS_LOG_TRACE("Invalid! syn+ack flags not allowed, tcp_flags %hu",
+                      tcp_flags);
         return FALSE;
     }
 
diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-windows/ovsext/Conntrack.c
index 43c9dd3..7e413c6 100644
--- a/datapath-windows/ovsext/Conntrack.c
+++ b/datapath-windows/ovsext/Conntrack.c
@@ -317,6 +317,9 @@  OvsCtEntryCreate(OvsForwardingContext *fwdCtx,
         const ICMPHdr *icmp;
         icmp = OvsGetIcmp(curNbl, l4Offset, &storage);
         if (!OvsConntrackValidateIcmpPacket(icmp)) {
+            if(icmp) {
+                OVS_LOG_TRACE("Invalid! icmp->type %u", icmp->type);
+            }
             state = OVS_CS_F_INVALID;
             break;
         }
@@ -334,6 +337,7 @@  OvsCtEntryCreate(OvsForwardingContext *fwdCtx,
         break;
     }
     default:
+        OVS_LOG_TRACE("Invalid! Not supported protocol, ipProto %u", ipProto);
         state = OVS_CS_F_INVALID;
         break;
     }