Re: [PATCH net-next] tcp: md5: check md5 signature without socket lock

On Tue, 05 Aug 2014 16:27:03 -0700 (PDT)
David Miller [off-list ref] wrote:

quoted

+#ifdef CONFIG_TCP_MD5SIG
+	/*
+	 * We really want to reject the packet as early as possible
+	 * if:
+	 *  o We're expecting an MD5'd packet and this is no MD5 tcp option
+	 *  o There is an MD5 option and we're not expecting one
+	 */
+	if (tcp_v4_inbound_md5_hash(sk, skb))
+		goto discard_and_relse;
+#endif

The original ordering seemed very much intentional, as per the comment.

You need to either make your locking change without disturbing this
ordering, or proprosed first and separately that the early check
should be changed.

Also, you really shouldn't just move the early md5 check _after_ the
TCP_ESTABLISHED fast path, and keep the comment there as well.  The
comment makes no sense any longer if the MD5 check happens after the
TCP_ESTABLISHED fast path, right?

I'm not applying this, sorry.

It seems your arguments are based on the assumption that I moved md5 signature
check down the tcp processing chain. But I did exactly the opposite, md5 is
checked earlier in this patch.

I moved tcp_v{4,6}_inbound_md5_hash from tcp_v{4,6}_do_rcv to tcp_v{4,6}_rcv,
and tcp_v{4,6}_do_rcv is always called from tcp_v{4,6}_rcv (either directly or
via sk_backlog). 

For ipv4 it looks something like this (and ipv6 is almost the same):

tcp_v4_rcv(skb):
... /* various checks like pkt size, checksum and so on */
	sk = __inet_lookup_skb(&tcp_hashinfo, skb, th->source, th->dest);
... /* some more checks */
	if (!xfrm4_policy_check(sk, XFRM_POLICY_IN, skb))
...
+	if (tcp_v4_inbound_md5_hash(sk, skb))
+		goto discard_and_relse;
	nf_reset(skb);

	if (sk_filter(sk, skb))
		goto discard_and_relse;

	sk_mark_napi_id(sk, skb);
	skb->dev = NULL;

	bh_lock_sock_nested(sk);
	if ( /* NET_DMA, sk_backlog and tcp_prequeue checks */ )
        	ret = tcp_v4_do_rcv(sk, skb);
...
        bh_unlock_sock(sk);
	sock_put(sk);
        return ret;

tcp_v4_do_rcv(sk, skb):
-	if (tcp_v4_inbound_md5_hash(sk, skb))
-		goto discard_and_relse;
... /* do further processing */

So, speaking of ordering, only nf_reset, sk_filter, sk_mark_napi_id,
skb->dev=NULL, sk_backlogging, tcp_prequeue and NET_DMA things would be done
after tcp_v4_inbound_md5_hash (instead of before) if this patch is applied. 
I don't see anything bad about that, correct me if I'm wrong.

I could also move tcp_v4_inbound_md5_hash before xfrm4_policy_check or after
sk_filter: I don't have any strong arguments here, so I am ok to resubmit
with other ordering of these calls.