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

[Dmitry Popov]

On Tue, 05 Aug 2014 16:27:03 -0700 (PDT)
David Miller <davem@xxxxxxxxxxxxx> wrote:

> > +#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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/