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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <df99fb97-74ca-4b5c-9d5f-86466025a531@kernel.org>
Date: Mon, 3 Jun 2024 14:47:28 +0200
From: Matthieu Baerts <matttbe@...nel.org>
To: Jason Xing <kerneljasonxing@...il.com>, edumazet@...gle.com,
 kuba@...nel.org, pabeni@...hat.com, davem@...emloft.net, dsahern@...nel.org,
 martineau@...nel.org, geliang@...nel.org
Cc: netdev@...r.kernel.org, mptcp@...ts.linux.dev,
 Jason Xing <kernelxing@...cent.com>
Subject: Re: [PATCH net v4 2/2] mptcp: count CLOSE-WAIT sockets for
 MPTCP_MIB_CURRESTAB

Hi Jason,

On 31/05/2024 11:17, Jason Xing wrote:
> From: Jason Xing <kernelxing@...cent.com>
> 
> Like previous patch does in TCP, we need to adhere to RFC 1213:
> 
>   "tcpCurrEstab OBJECT-TYPE
>    ...
>    The number of TCP connections for which the current state
>    is either ESTABLISHED or CLOSE- WAIT."
> 
> So let's consider CLOSE-WAIT sockets.
> 
> The logic of counting
> When we increment the counter?
> a) Only if we change the state to ESTABLISHED.
> 
> When we decrement the counter?
> a) if the socket leaves ESTABLISHED and will never go into CLOSE-WAIT,
> say, on the client side, changing from ESTABLISHED to FIN-WAIT-1.
> b) if the socket leaves CLOSE-WAIT, say, on the server side, changing
> from CLOSE-WAIT to LAST-ACK.

Thank you for this modification, and for having updated the Fixes tag.

> Fixes: d9cd27b8cd19 ("mptcp: add CurrEstab MIB counter support")
> Signed-off-by: Jason Xing <kernelxing@...cent.com>
> ---
>  net/mptcp/protocol.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 7d44196ec5b6..6d59c1c4baba 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -2916,9 +2916,10 @@ void mptcp_set_state(struct sock *sk, int state)
>  		if (oldstate != TCP_ESTABLISHED)
>  			MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_CURRESTAB);
>  		break;
> -
> +	case TCP_CLOSE_WAIT:
> +		break;

The modification is correct: currently, and compared to TCP, the MPTCP
"accepted" socket will not go through the TCP_SYN_RECV state because it
will be created later on.

Still, I wonder if it would not be clearer to explicitly mention this
here, and (or) in the commit message, to be able to understand why the
logic is different here, compared to TCP. I don't think the SYN_RECV
state will be used in the future with MPTCP sockets, but just in case,
it might help to mention TCP_SYN_RECV state here. Could add a small
comment here above please?

>  	default:
> -		if (oldstate == TCP_ESTABLISHED)
> +		if (oldstate == TCP_ESTABLISHED || oldstate == TCP_CLOSE_WAIT)
>  			MPTCP_DEC_STATS(sock_net(sk), MPTCP_MIB_CURRESTAB);
>  	}
>  

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ