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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
