On Thu, Apr 05, 2012 at 06:47:23PM -0700, Jesse Gross wrote:
> On Thu, Apr 5, 2012 at 5:35 PM, Simon Horman <ho...@verge.net.au> wrote:
> > On Thu, Apr 05, 2012 at 05:00:12PM -0700, Jesse Gross wrote:
> >> On Wed, Apr 4, 2012 at 6:08 PM, Simon Horman <ho...@verge.net.au> wrote:
> >> > diff --git a/include/linux/tcp.h b/include/linux/tcp.h
> >> > index 3c7ffdb..36e794b 100644
> >> > --- a/include/linux/tcp.h
> >> > +++ b/include/linux/tcp.h
> >> > @@ -472,6 +472,11 @@ struct tcp_sock {
> >> > * contains related tcp_cookie_transactions fields.
> >> > */
> >> > struct tcp_cookie_values *cookie_values;
> >> > +
> >> > + /* For encapsulation sockets. */
> >> > + __u16 encap_type;
> >>
> >> Do we actually get any value out of encap_type? It seems equivalent
> >> to encap_rcv != NULL.
> >
> > I agree, but this is in keeping with the UDP implementation.
>
> I guess that I'm not convinced that exactly matching the UDP
> implementation is the right thing to do here. I think realistically
> there aren't going to be a lot of new TCP encapsulations and currently
> nothing sets this up from userspace, which is why UDP needs it. Given
> that, it seems best to simplify things.
Sure, that makes sense. I will remove encap_type.
> >> > diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> >> > index fd54c5f..ce56c6c 100644
> >> > --- a/net/ipv4/tcp_ipv4.c
> >> > +++ b/net/ipv4/tcp_ipv4.c
> >> > @@ -1714,6 +1715,32 @@ process:
> >> >
> >> > if (!xfrm4_policy_check(sk, XFRM_POLICY_IN, skb))
> >> > goto discard_and_relse;
> >> > +
> >> > + tp = tcp_sk(sk);
> >>
> >> I wonder if this is the right place for this hook. It seems odd to be
> >> after the TIME_WAIT check, for example.
> >
> > True. I pushed it down here because I felt that the xfrm4_policy_check and
> > ttl checks made sense and the TIME_WAIT would always fail. If not, perhaps
> > immediately after the socket lookup would be a better location.
>
> I think what we would ideally like to do is register an IP protocol
> handler. We can't do that because the normal TCP stack has already
> taken it but we'd like to get as close to that as possible, so I would
> tend to push it as close to the socket lookup as possible.
Ok sure, I'll move it up to just below the socket lookup.
> >> > + encap_rcv = ACCESS_ONCE(tp->encap_rcv);
> >> > + if (encap_rcv) {
> >> > + int ret = encap_rcv(sk, skb);
> >> > + if (ret <= 0)
> >> > + return -ret;
> >>
> >> It seems odd to resubmit as a different protocol here. With UDP it's
> >> common to have something that runs over either bare IP or encapsulated
> >> in UDP but I'm not sure that the same really applies here.
> >>
> >> > + }
> >
> > Ok, so perhaps it would be best to just have
> >
> > return encap_rcv(sk, skb);
> >
> > This was the behaviour of my original version of this patch but
> > I thought it may make sense to have behaviour closer to UDP's encap_rcv.
>
> I think there's probably still value in allowing the encap handler to
> reject the packet back to the TCP stack.
Ok, in that case I'm not sure that I understood your point about different
protocols.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
