[ovs-dev] [PATCH 2/2] conntrack: Return NEW for IPv6 ND packets without tracking.

[Daniele Di Proietto]

On 20/12/2016 17:14, "Joe Stringer" <joe at ovn.org> wrote:

>On 20 December 2016 at 14:46, Darrell Ball <dball at vmware.com> wrote:
>>
>>
>> On 12/20/16, 12:25 PM, "ovs-dev-bounces at openvswitch.org on behalf of Daniele Di Proietto" <ovs-dev-bounces at openvswitch.org on behalf of diproiettod at vmware.com> wrote:
>>     @@ -864,25 +896,32 @@ extract_l4_icmp6(struct conn_key *key, const void *data, size_t size,
>>                                    &key->dst.addr.ipv6_aligned)
>>                  || !ipv6_addr_equals(&inner_key.dst.addr.ipv6_aligned,
>>                                       &key->src.addr.ipv6_aligned)) {
>>     -            return false;
>>     +            return KEY_INVALID;
>>              }
>>
>>              key->src = inner_key.src;
>>              key->dst = inner_key.dst;
>>              key->nw_proto = inner_key.nw_proto;
>>
>>     -        ok = extract_l4(key, l4, tail - l4, NULL, l3);
>>     -        if (ok) {
>>     +        res = extract_l4(key, l4, tail - l4, NULL, l3);
>>     +        if (res != KEY_INVALID) {
>>                  conn_key_reverse(key);
>>                  *related = true;
>>              }
>>     -        return ok;
>>     +        return res;
>>          }
>>     +    case ND_NEIGHBOR_SOLICIT:
>>     +    case ND_NEIGHBOR_ADVERT:
>>     +    case ND_ROUTER_SOLICIT:
>>     +    case ND_ROUTER_ADVERT:
>>     +        if (icmp6->icmp6_code != 0) {
>>     +            return KEY_INVALID;
>>     +        }
>>     +        /* The packet is valid, but it's not going to be tracked. */
>>
>> Do we want to say “The packet is valid” ? based on a verification being half-baked/partial.
>> or simply we are not going to track something that looks like a ND packet ?
>>
>> Maybe, just check the type and say KEY_NO_TRACK, meaning “We certify nothing.”.
>
>I agree at this layer, I think it would be a bit clearer to return
>something like KEY_NO_TRACK.

The patch is already returning KEY_NO_TRACK (see below).
It returns KEY_INVALID above if the ICMP code is wrong.

Did I misinterpret your comment?

>
>When you get closer to the OpenFlow layer though, I think it's
>reasonable to end up converting that into something consistent with
>the Linux kernel implementation - that is, mark it as NEW and let the
>OpenFlow rule writer decide what to do with it. It's a subtle
>difference in semantics, but what we have in the OpenFlow API is an
>"INVALID" bit, which says that we've detected something that's pretty
>clearly invalid. We don't have a corresponding "valid" bit that
>certifies everything is guaranteed to be rosy. If someone is
>implementing a firewall with this feature, then they should consider
>whether this CT action is providing enough guarantees around the
>traffic identification for filtering, and therefore whether they
>should perform further validation on the packets. IMO that's a bit
>out of scope of CT action at this stage.

I agree with you

Thanks,

Daniele