netdev - Re: [PATCH net-next] sctp: support per-association stats via a new SCTP_GET_ASSOC

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20121026143704.GC25087@hmsreliant.think-freely.org>
Date:	Fri, 26 Oct 2012 10:37:04 -0400
From:	Neil Horman <nhorman@...driver.com>
To:	Michele Baldessari <michele@...syn.org>
Cc:	linux-sctp@...r.kernel.org, Vlad Yasevich <vyasevich@...il.com>,
	"David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
	Thomas Graf <tgraf@...g.ch>
Subject: Re: [PATCH net-next] sctp: support per-association stats via a new
 SCTP_GET_ASSOC_STATS call

On Fri, Oct 26, 2012 at 03:42:53PM +0200, Michele Baldessari wrote:
> The current SCTP stack is lacking an API to have per association
> statistics. This is a kernel implementation modeled after OpenSolaris'
> SCTP_GET_ASSOC_STATS.
> 
> Userspace part will follow on lksctp if/when there is a general ACK on this.
> 
> Signed-off-by: Michele Baldessari <michele@...syn.org>
> Acked-by: Thomas Graf <tgraf@...g.ch>
> ---
>  include/net/sctp/sctp.h    |  3 +++
>  include/net/sctp/structs.h | 39 +++++++++++++++++++++++++++
>  include/net/sctp/user.h    | 26 ++++++++++++++++++
>  net/sctp/associola.c       | 20 ++++++++++++++
>  net/sctp/endpointola.c     |  5 +++-
>  net/sctp/input.c           |  5 ++--
>  net/sctp/output.c          |  5 ++++
>  net/sctp/outqueue.c        | 12 +++++++++
>  net/sctp/sm_sideeffect.c   |  2 ++
>  net/sctp/sm_statefuns.c    | 12 +++++++--
>  net/sctp/socket.c          | 67 ++++++++++++++++++++++++++++++++++++++++++++++
>  net/sctp/transport.c       |  2 ++
>  12 files changed, 193 insertions(+), 5 deletions(-)
> 
We already have files in /proc/net/sctp to count snmp system-wide totals,
per-endpoint totals, and per association totals.  Why do these stats differently
instead of just adding them the per-association file?  I get that solaris does
this, but its not codified in any of the RFC's or other standards.  I would
really rather see something like this go into the interfaces we have, rather
than creating a new one.

I also am a bit confused regarding the stats themselves.  Most are fairly clear,
but some seem lacking (you count most things sent and received, but only count
received gap acks).  Others seems vague and or confusing (when counting
retransmitted chunks and packets, how do you count a packet that has both new
and retransmitted chunks)?  And the max observed rto stat is just odd.  Each
transport has an rto value, not each association, and you cal already see the
individual transport rto values in /proc/net/sctp/remaddr.


> +
> +	/* Gap Ack Blocks received */
> +	__u64 gapcnt;
> +
No gapcnt for sent gap ack blocks?

>  		unsigned long timeout;
> diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
> index 1b4a7f8..569ee3a 100644
> --- a/net/sctp/outqueue.c
> +++ b/net/sctp/outqueue.c
> @@ -667,6 +667,8 @@ redo:
>  				chunk->fast_retransmit = SCTP_DONT_FRTX;
>  
>  			q->empty = 0;
> +			if (q->asoc)
> +				q->asoc->rtxchunks++;
>  			break;
>  		}
>  
> @@ -678,6 +680,10 @@ redo:
>  			break;
>  	}
>  
> +	if (q->asoc)
> +		q->asoc->rtxpackets++;
> +
> +
This seems incorrect to me.  The packet being assembled here may have new chunks
in it (either control or data).  Counting a packet as being retransmitted just
because it has a retransmitted chunk in it seems wrong.  At the very least its a
misleading/vague statistic.
>  			break;
> diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
> index b6adef8..4f94432 100644
> --- a/net/sctp/sm_statefuns.c
> +++ b/net/sctp/sm_statefuns.c
> @@ -6127,9 +6127,13 @@ static int sctp_eat_data(const struct sctp_association *asoc,
>  		/* The TSN is too high--silently discard the chunk and
>  		 * count on it getting retransmitted later.
>  		 */
> +		if (chunk->asoc)
> +			chunk->asoc->outseqtsns++;
This just seems wrong.  The definition states that this is counting the last TSN
recevied (despite being name outseqtsns), yet this looks like you're:
1) just incrementing a counter, rather than recording the TSN value itself
(which may or may not be what you meant, but seems to contradict what the
comments at the definition)
2) Only incremanting it if the TSN is out of range, which makes very little
sense to me.

> 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html