netdev - Re: [PATCH v4 4/4] net: diag: Support destroying TCP sockets.

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] [day] [month] [year] [list]

Message-ID: <1448976352.25582.17.camel@edumazet-glaptop2.roam.corp.google.com>
Date:	Tue, 01 Dec 2015 05:25:52 -0800
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Lorenzo Colitti <lorenzo@...gle.com>
Cc:	netdev@...r.kernel.org, davem@...emloft.net,
	hannes@...essinduktion.org, ek@...gle.com, tom@...bertland.com,
	zenczykowski@...il.com
Subject: Re: [PATCH v4 4/4] net: diag: Support destroying TCP sockets.

On Tue, 2015-12-01 at 16:04 +0900, Lorenzo Colitti wrote:

> +	/* Don't race with BH socket closes such as inet_csk_listen_stop. */
> +	local_bh_disable();
> +	bh_lock_sock(sk);
> +
> +	if (!sock_flag(sk, SOCK_DEAD)) {
> +		smp_wmb();  /* Be consistent with tcp_reset */

This barrier is absolutely useless here, it must be moved _after_
sk->sk_err = ECONNABORTED.

Since you copied tcp_reset() code, please do not reorder barriers.

Also, comment in tcp_reset() is better IMO

/* This barrier is coupled with smp_rmb() in tcp_poll() */



> +		sk->sk_err = ECONNABORTED;
> +		sk->sk_error_report(sk);

Why not using tcp_need_reset() here, otherwise we might send spurious
RST for some tcp states.

Ideally, packetdrill tests would be nice.

> +		tcp_send_active_reset(sk, GFP_ATOMIC);
> +		tcp_done(sk);
> +	}

Please do not send a v5 before letting other people comment on v3 & v4,
no need to flood netdev with many versions sent the same day.

Thanks.


--
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