On Tue, 05 Aug 2014 16:27:03 -0700 (PDT)
David Miller <da...@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 linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
