Yeah, well, I can't apply the patch, it's wrong and ... <grin/>
OK, here's the issues I have
1. You're adding the TH_PUSH flag, but isn't there at least one other flag
(TH_URG) that COULD be added to ACK? In fact, isn't almost ANY combination
of the six flags POSSIBLE?
# define TH_FIN 0x01
# define TH_SYN 0x02
# define TH_RST 0x04
# define TH_PUSH 0x08
# define TH_ACK 0x10
# define TH_URG 0x20
Since we don't track PUSH/URG, wouldn't we be better masking them off??
2. The code you're ripping out of handleSession() @ 1571
} else if((tp->th_flags == TH_ACK) && (theSession->sessionState ==
FLAG_FLAG_STATE_SYN_ACK)) {
...
allocateSecurityHostPkts(srcHost); allocateSecurityHostPkts(dstHost);
< incrementUsageCounter(&srcHost->secHostPkts->establishedTCPConnSent,
dstHost, actualDeviceId);
< incrementUsageCounter(&dstHost->secHostPkts->establishedTCPConnRcvd,
srcHost, actualDeviceId);
<
incrementTrafficCounter(&myGlobals.device[actualDeviceId].securityPkts.estab
lishedTCPConn, 1);
<
incrementTrafficCounter(&myGlobals.device[actualDeviceId].numEstablishedTCPC
onnections, 1);
} else if((addedNewEntry == 0)
&& ((theSession->sessionState == FLAG_STATE_SYN) ||
(theSession->sessionState == FLAG_FLAG_STATE_SYN_ACK))
&& (!(tp->th_flags & TH_RST))) {
/*
We might have lost a packet so:
- we cannot calculate latency
- we don't set the state to initialized
*/
...
incrementTrafficCounter(&myGlobals.device[actualDeviceId].numEstablishedTCPC
onnections, 1);
}
If we didn't have this:
theSession->sessionState = FLAG_STATE_ACTIVE;
we couldn't tell the two cases apart and then I'd still maintain it looks
like your code is the wrong choice - dropping the addition in the NORMAL
case (2nd ACK in SYN_ACK state) in favor of counting in the ABNORMAL case
(SYN in SYN_ACK state, meaning we missing the 2nd ACK) just seems wrong to
me...
As the code is, I think it's right since it should be impossible to walk
BOTH paths - once the FLAG_STATE_ACTIVE is set, the 2nd (lost packet) case
won't ever be entered. So if you enter that, you did lose the 1st ACK and
you do want to count a connection.
The stuff after this:
< /* This simulates a connection establishment */
Does just what it says in the abnormal case - and I think it's important,
too.
3. The code you're ripping out of the other part of that 2nd case, the
if(sport > dport) stuff is there for a reason. Since we're in the middle of
the conversation and didn't see all the ACKs, it's impossible to tell which
traffic type it is from just the two port #s. Suppose you have a connection
to a Squid proxy (3128). The sender could be on 12345 or 1234 - both are
legal. ntop's convention is to test the lower #ed port and if that's not
being monitored, then use the higher. It's not perfect, but it's as good as
any other choice short of total connection tracking. E.g. this in pbuf.c:
/* Handle UDP traffic like TCP, above -
That is: if we know about the lower# port, even if it's the
destination,
classify the traffic that way.
(BMS 12-2001)
*/
if(dport < sport) {
if(handleIP(dport, srcHost, dstHost, length, 0, 0, actualDeviceId)
== -1)
handleIP(sport, srcHost, dstHost, length, 0, 0, actualDeviceId);
} else {
if(handleIP(sport, srcHost, dstHost, length, 0, 0, actualDeviceId)
== -1)
handleIP(dport, srcHost, dstHost, length, 0, 0, actualDeviceId);
}
The difference in the pbuf.c stuff, is that we actually check whether it's a
recognized port #. So if any changes are made, it should probably be to
more inline with the pbuf.c stuff.
Now - regardless of where you DO count things - as to the counter increments
you're removing:
< incrementUsageCounter(&srcHost->secHostPkts->establishedTCPConnSent,
dstHost, actualDeviceId);
< incrementUsageCounter(&dstHost->secHostPkts->establishedTCPConnRcvd,
srcHost, actualDeviceId);
What those do is to count a connection established for both the src and dst.
If they're both local, then you will see the 'doubling'. But I'll argue
from the network perspective, this is correct. Each host HAS seen a
connection established (remember, a two-way tcp/ip connection is really two
one-way connections...)
I guess what I'm saying is that I *might* accept something that masked off
the PUSH/URG on the ACK/SYN path (although can't do the easy way, bit-wise
in the lines around 1500s, because we do track TH_FIN|TH_PUSH|TH_URG's).
Other than that, I don't think ntop is wrong. We could probably improve the
if (sport < dport) to check if which is the protocol we recognize to
determine sent/received, but that's not what you're looking at anyway.
-----Burton
-----Original Message-----
From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED] Behalf
Of Markus Rehbach
Sent: Saturday, September 06, 2003 12:49 PM
To: [EMAIL PROTECTED]
Subject: Re: [Ntop-dev] 'Packet Statistics' difficult to interpret (or
wrong?)
Well,
IANAP, but I will get my expected result using the patch below; Burton will
tell me that it will break all the good things ;-) Against sessions.c this
morning.
1516c1516,1519
< } else if((tp->th_flags == TH_ACK) && (theSession->sessionState ==
FLAG_FLAG_STATE_SYN_ACK)) {
---
> #ifdef DEBUG
> printf("DEBUG: Pushed sessionState to %d in
1\n",theSession->sessionState);
> #endif
> } else if(((tp->th_flags == TH_ACK)||(tp->th_flags == TH_ACK|TH_PUSH))
&& (theSession->sessionState == FLAG_FLAG_STATE_SYN_ACK)) {
1539d1541
< theSession->sessionState = FLAG_STATE_ACTIVE;
1569a1572,1573
> theSession->sessionState = FLAG_STATE_ACTIVE;
>
1571,1574c1575,1579
< incrementUsageCounter(&srcHost->secHostPkts->establishedTCPConnSent,
dstHost, actualDeviceId);
< incrementUsageCounter(&dstHost->secHostPkts->establishedTCPConnRcvd,
srcHost, actualDeviceId);
<
incrementTrafficCounter(&myGlobals.device[actualDeviceId].securityPkts.estab
lishedTCPConn, 1);
<
incrementTrafficCounter(&myGlobals.device[actualDeviceId].numEstablishedTCPC
onnections, 1);
---
>
> #ifdef DEBUG
> printf("DEBUG: Pushed sessionState to %d in
2\n",theSession->sessionState);
> #endif
>
1598,1614c1603
< if(sport > dport) {
< incrementUsageCounter(&srcHost->secHostPkts->establishedTCPConnSent,
dstHost, actualDeviceId);
< incrementUsageCounter(&dstHost->secHostPkts->establishedTCPConnRcvd,
srcHost, actualDeviceId);
< /* This simulates a connection establishment */
< incrementUsageCounter(&srcHost->secHostPkts->synPktsSent, dstHost,
actualDeviceId);
< incrementUsageCounter(&dstHost->secHostPkts->synPktsRcvd, srcHost,
actualDeviceId);
<
incrementTrafficCounter(&myGlobals.device[actualDeviceId].securityPkts.synPk
ts, 1);
< } else {
< incrementUsageCounter(&srcHost->secHostPkts->establishedTCPConnRcvd,
dstHost, actualDeviceId);
< incrementUsageCounter(&dstHost->secHostPkts->establishedTCPConnSent,
srcHost, actualDeviceId);
< /* This simulates a connection establishment */
< incrementUsageCounter(&dstHost->secHostPkts->synPktsSent, srcHost,
actualDeviceId);
< incrementUsageCounter(&srcHost->secHostPkts->synPktsRcvd, dstHost,
actualDeviceId);
<
<
incrementTrafficCounter(&myGlobals.device[actualDeviceId].securityPkts.estab
lishedTCPConn, 1);
<
incrementTrafficCounter(&myGlobals.device[actualDeviceId].securityPkts.synPk
ts, 1);
< }
---
> incrementUsageCounter(&srcHost->secHostPkts->establishedTCPConnSent,
dstHost, actualDeviceId);
1616c1605,1607
<
incrementTrafficCounter(&myGlobals.device[actualDeviceId].numEstablishedTCPC
onnections, 1);
---
> #ifdef DEBUG
> printf("DEBUG: Pushed sessionState to %d in
3\n",theSession->sessionState);
> #endif
_______________________________________
On Saturday 06 September 2003 09:57, Markus Rehbach wrote:
> Hi all,
>
> on my PC I started ntop, opened a news reader and fetched some
> articles. With ethereal I controlled the sessions, there were:
> 1. one nntp-Session openend to 10.20.30.100 and not yet terminated
> 2. another IDENT-try from 10.20.30.100 answered with an RST on
> the machine which is running ntop.
>
> Packet Statistics:
>
> TCP Connections Directed to Rcvd From
> Attempted 3 10.20.30.100 1 10.20.30.100
> Established 2 [67 %] 10.20.30.100 0 [0 %]
>
> TCP Flags Pkts Sent Pkts Rcvd
> SYN 3 10.20.30.100 1 10.20.30.100
> RST|ACK 1 10.20.30.100 0
>
> ............
_______________________________________________
Ntop-dev mailing list
[EMAIL PROTECTED]
http://listgateway.unipi.it/mailman/listinfo/ntop-dev
_______________________________________________
Ntop-dev mailing list
[EMAIL PROTECTED]
http://listgateway.unipi.it/mailman/listinfo/ntop-dev
