On Wed, Mar 06, 2013 at 08:29:13AM -0800, Jesse Gross wrote:
> On Wed, Mar 6, 2013 at 8:21 AM, Ben Pfaff <b...@nicira.com> wrote:
> > On Wed, Mar 06, 2013 at 07:46:16AM -0800, Jesse Gross wrote:
> >> On Tue, Mar 5, 2013 at 11:08 PM, Simon Horman <ho...@verge.net.au> wrote:
> >> > [ Cc Pravin B Shelar ]
> >> >
> >> > On Tue, Mar 05, 2013 at 06:11:27PM -0800, Ben Pfaff wrote:
> >> >> On Wed, Mar 06, 2013 at 10:42:02AM +0900, Simon Horman wrote:
> >> >> > On Tue, Mar 05, 2013 at 10:14:44AM -0800, Ben Pfaff wrote:
> >> >> > > On Thu, Feb 28, 2013 at 06:15:07PM +0900, Simon Horman wrote:
> >> >> > > > This adds support for the OpenFlow 1.1+ dec_mpls_ttl action.
> >> >> > > > And also adds an NX dec_mpls_ttl action.
> >> >> > > >
> >> >> > > > The handling of the TTL modification is entirely handled in
> >> >> > > > userspace.
> >> >> > > >
> >> >> > > > Reviewed-by: Isaku Yamahata <yamah...@valinux.co.jp>
> >> >> > > > Signed-off-by: Simon Horman <ho...@verge.net.au>
> >> >> > >
> >> >> > > The only issue I see with this is that it seems uncertain about
> >> >> > > what is
> >> >> > > an invalid MPLS TTL. The code says that, pre-decrement, values 0
> >> >> > > and 1
> >> >> > > are invalid:
> >> >> > >
> >> >> > > + if (ttl > 1) {
> >> >> > > + ttl--;
> >> >> > > + set_mpls_lse_ttl(&ctx->flow.mpls_lse, ttl);
> >> >> > > + return false;
> >> >> > > + } else {
> >> >> > > + execute_controller_action(ctx, UINT16_MAX,
> >> >> > > OFPR_INVALID_TTL, 0);
> >> >> > >
> >> >> > > The documentation says that, pre-decrement, value 0 is invalid:
> >> >> > >
> >> >> > > +.IP \fBdec_mpls_ttl\fR
> >> >> > > +Decrement TTL of the outer MPLS label stack entry of a packet. If
> >> >> > > the TTL
> >> >> > > +is initially zero, no decrement occurs. Instead, a ``packet-in''
> >> >> > > message
> >> >> > >
> >> >> > > I don't know MPLS, so I don't know which definition is correct.
> >> >> >
> >> >> > I am not sure and to be honest I had just followed dec_ttl
> >> >> > implementation
> >> >> > when adding the code. Reading up I think that section 2.3 of RFC3443
> >> >> > implies this is the desired behaviour.
> >> >> >
> >> >> > I'm not sure why the documentation conflicts with the code, most
> >> >> > likely
> >> >> > it documents a previous incantation of the code. In any case I propose
> >> >> > updating it rather than the code.
> >> >>
> >> >> OK, that makes sense, thank you.
> >> >
> >> > It seems to me that dec_ttl has the same problem.
> >> >
> >> > But it also seems to me that it is the code rather than the documentation
> >> > that should be updated.
> >> >
> >> > The commit log for the addition of the TTL decrement action states:
> >> >
> >> > commit f0fd1a1772665ea57662281d9cccadb0f0146196
> >> > Author: Pravin B Shelar <pshe...@nicira.com>
> >> > Date: Fri Jan 13 17:54:04 2012 -0800
> >> >
> >> > ofproto: New action TTL decrement.
> >> >
> >> > Following patch implements dec_ttl as vendor action with similar
> >> > semantics as OpenFlow 1.2. If TTL reaches zero while procession
> >> > actions in current table, the remaining actions in previous tables
> >> > are processed. A configuration parameter is added to make TTL
> >> > decrement to zero generate packet in.
> >> >
> >> > Feature #8758
> >> > Signed-off-by: Pravin B Shelar <pshe...@nicira.com>
> >> >
> >> > But the code features "if (ttl > 1)".
> >> >
> >> > That now seems like a logic bug to me (though perhaps the afternoon
> >> > air is affecting my brain adversely).
> >> >
> >> >
> >> > I have revised the dec_mpls_ttl patch to use "(ttl > 0)".
> >> > Should I provide a patch to update dec_ttl?
> >>
> >> I think you were right the first time - if a decrement causes the TTL
> >> to hit zero then it should go to the exception case (in theory we
> >> should never receive a packet with TTL zero here, so it's always an
> >> exception). I believe this applies equally to IP and MPLS.
> >
> > My understanding is that, in IP, a host accepts packets with TTL=0, but
> > a router discards them. If that is correct, then decrementing a TTL to
> > 0 should not discard the packet, only decrementing a TTL that is
> > initially zero.
>
> From the IPv6 RFC (since the meaning has somewhat changed over time):
>
> Hop Limit 8-bit unsigned integer. Decremented by 1 by
> each node that forwards the packet. The packet
> is discarded if Hop Limit is decremented to
> zero.
The requirements are a little confusing. For example, RFC 1122 states:
A host MUST NOT discard a datagram just because it was
received with TTL less than 2.
However, RFC 1009 is very clear:
If the TTL is reduced to zero, the datagram must be discarded, and
the gateway may send an ICMP Time Exceeded message to the source.
A datagram should never be received with a TTL of zero.
so I agree with you.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
