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

Since a8afca032 (tcp: md5: protects md5sig_info with RCU) tcp_md5_do_lookup
doesn't require socket lock, rcu_read_lock is enough. Therefore socket lock is
no longer required for tcp_v{4,6}_inbound_md5_hash too, so we can move these
calls (wrapped with rcu_read_{,un}lock) outside of bh_{,un}lock_sock: 
from tcp_v{4,6}_do_rcv to tcp_v{4,6}_rcv.

Signed-off-by: Dmitry Popov <ixaphire@qrator.net>
---
 net/ipv4/tcp_ipv4.c | 36 +++++++++++++++++++++++++-----------
 net/ipv6/tcp_ipv6.c | 25 +++++++++++++++++++------
 2 files changed, 44 insertions(+), 17 deletions(-)
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Dmitry Popov <ixaphire@qrator.net>
Date: Tue, 5 Aug 2014 02:54:52 +0400

> Since a8afca032 (tcp: md5: protects md5sig_info with RCU) tcp_md5_do_lookup
> doesn't require socket lock, rcu_read_lock is enough. Therefore socket lock is
> no longer required for tcp_v{4,6}_inbound_md5_hash too, so we can move these
> calls (wrapped with rcu_read_{,un}lock) outside of bh_{,un}lock_sock: 
> from tcp_v{4,6}_do_rcv to tcp_v{4,6}_rcv.
> 
> Signed-off-by: Dmitry Popov <ixaphire@qrator.net>

But this change has a side effect outside of locking:

> @@ -1539,16 +1551,6 @@ static struct sock *tcp_v4_hnd_req(struct sock *sk, struct sk_buff *skb)
>  int tcp_v4_do_rcv(struct sock *sk, struct sk_buff *skb)
>  {
>  	struct sock *rsk;
> -#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;
> -#endif
>  
>  	if (sk->sk_state == TCP_ESTABLISHED) { /* Fast path */
>  		struct dst_entry *dst = sk->sk_rx_dst;
> @@ -1751,6 +1753,18 @@ process:
>  
>  	if (!xfrm4_policy_check(sk, XFRM_POLICY_IN, skb))
>  		goto discard_and_relse;
> +
> +#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
> +
>  	nf_reset(skb);
>  
>  	if (sk_filter(sk, skb))

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.

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

On Tue, 05 Aug 2014 16:27:03 -0700 (PDT)
David Miller <davem@davemloft.net> 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 netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Dmitry Popov <ixaphire@qrator.net>
Date: Wed, 6 Aug 2014 22:49:02 +0400

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

Ok, now I understand, thanks for explaining.

Please resubmit your patch, I'll apply it.

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