On Thu, Aug 06, 2015 at 08:27:02PM -0700, Jesse Gross wrote:
> On Thu, Aug 6, 2015 at 7:43 PM, Simon Horman <simon.hor...@netronome.com>
> wrote:
> > On Thu, Aug 06, 2015 at 04:00:36PM -0700, Jesse Gross wrote:
> >> On Wed, Aug 5, 2015 at 10:20 PM, Simon Horman
> >> <simon.hor...@netronome.com> wrote:
> >> > There appears to be a miss-match between the handling of miss-sized
> >> > neighbour discovery options in implementation of
> >> > parse_icmpv6() in user-space and in the kernel datapath.
> >> >
> >> > This patch addresses that by modifying the user-space handling to
> >> > match that of the kernel datapath; processing is terminated without
> >> > rather than with an error.
> >> >
> >> > The result of this is that the ICMPv6 type and code are present in
> >> > the flow and thus may be matched on, which seems logical.
> >> >
> >> > I can across this while investigating why neighbour solicitation packets
> >> > with an option whose header has type and length both set to zero did
> >> > not hit a rule that matches on its type.
> >> >
> >> > Signed-off-by: Simon Horman <simon.hor...@netronome.com>
> >>
> >> I agree this is a bug and I think the unwanted side effects are likely
> >> pretty minimal.
> >>
> >> However, doesn't this logic to all of the invalid paths in this
> >> function? The kernel always populates the ICMPv6 type and code
> >> regardless of neighbor discovery processing. The only thing the
> >> invalid target does in that case is clear any half parsed ND
> >> information.
> >
> > Are you referring to the following in lib/flow.c:parse_icmpv6() ?
> >
> >
> > *nd_target = data_try_pull(datap, sizep, sizeof **nd_target);
> > if (OVS_UNLIKELY(!*nd_target)) {
> > return false;
> > }
> >
> > ...
> >
> >
> > if (OVS_UNLIKELY(!data_try_pull(datap, sizep, opt_len))) {
> > goto invalid;
> > }
> >
> > If so, yes, now I look at the code again it looks like they are
> > also inconsistent with the kernel implementation and could be
> > changed to return true.
>
> Yes - I think this function should never return false since we should
> always report type/code regardless of what happens with neighbor
> discovery. I think we should just clear any partial information in the
> event of an error, similar to what the kernel does.
Thanks, I agree.
I will see about making it so.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
