Re: [PATCH nf-next v3 1/3] netfilter: nf_conntrack: push zone object into functions

[Daniel Borkmann <daniel@iogearbox.net>]

On 08/05/2015 12:51 PM, Pablo Neira Ayuso wrote:
> On Mon, Aug 03, 2015 at 06:00:35PM +0200, Daniel Borkmann wrote:
>> On 08/03/2015 05:59 PM, Pablo Neira Ayuso wrote:
>>> On Thu, Jul 30, 2015 at 06:34:48PM +0200, Daniel Borkmann wrote:
>>>> CTA_ZONE_DIR seems better, sure. I don't have any other extensions at
>>>> the moment, but it seems it makes sense to make this nested at this
>>>> point in time, so we have CTA_ZONE and CTA_ZONE_INFO as a container
>>>> for CTA_ZONE_DIR and whatever future might bring. I will look into it.
>>>
>>> thinking it well, this is part of the tuple, so I'd suggest you add
>>> CTA_TUPLE_ZONE to enum ctattr_tuple. We will probably need later to
>>> place each tuple in different zones.
>>
>> That's fine with me, will do after your tree rebase.
>
> Just pushed out a fresh copy with net-next into nf-next.

Cool, thanks, will move on with my rework/base of the set then!

> Daniel, a minor change that I came up with. With your patchset, the
> configurations that we accept look like:
>
>          zone original=Value     reply=0
>          zone original=0         reply=Value

Yes, the full picture is ...

   zone   original=Value     reply=Default
   zone   original=Default   reply=Value
   zone   original=Value     reply=Value
   zone   original=Default   reply=Default

... where Default (== 0) is to be understood "zone unspecified".

> But thinking on Thomas' requirements to limit the number of conntracks
> per zone, I think another valid configuration (and more generic) can
> be:
>
>          zone original=ValueX    reply=ValueY
>
> So we don't assume that the original or reply zone is always zero.
>
> The zone extension are that look like this:
>
> struct nf_conntrack_zone {
>          u16     id[IP_CT_DIR_MAX];
> };
>
> And we don't need to store the direction. To keep backward
> compatibility we can set the id in both directions to the same value.
>
> Can you see any problem with this approach? I think it should require
> just a little adjustment to you patchset. Thanks.

Don't yet see the bigger picture how this would relate to limiting the
number of conntrack entries per zone, but I also haven't thought deeply
on an implementation of that issue. A zone ValueX and ValueY where
ValueX != ValueY in different directions would imply fully overlapping
tuples in both directions, or only partially shared/common pools for
tuple allocations in the opposite directions.

Anyway, this would mean that instead of a simple direction flag as we
currently have, we'd need new revisions to add either two more zone ids
to uapi structs or a flag and a 2nd zone id field, if we want to reuse
the first zone field, and similarly also for the netlink interfaces.

On the mark to zone mapping, we would then need to fill the whole
id[IP_CT_DIR_MAX] on the template, which seems a little more problematic
to me.

This would probably mean we have to split the mark's bit space into
two 16 bit parts  (f.e. mark := (ValueX << 16) | ValueY)) to fill both
directions depending from which side we arrive.

The entity setting the mark would need to shift its own id encoding
into the correct direction in the mark and would leave the other one as
default zone (== 0) if unaware of it? Then, the problem is that the
/first/ one creating the conntrack entry does need to know ValueX /and/
ValueY, if later on we want to retrieve the entry again via zone ValueY.

This mark bit shifting seems rather inconvenient to use, imho. If ValueY
can be dymanic, perhaps then the one triggering the initial packet might
not even know which one to arrive in yet?

One could still add a direction parameter in the zone struct to know
where to apply the mark, if we don't want to encode both ids, and to
have the other one 0 to not expose this burden to the user. But that
means ValueY is static to 0 in this case, which seems undesired here.

So it looks actually a bit of a more complex change (/configuration),
doesn't seem to integrate as 'straight-forward' as the current approach,
but also targets a bit of a different use case (?) than this current set
does. Note that the current set doesn't ban us either from modifications
later on. Please let me know if I'm wrong somewhere, but given the above
issues, I would be happy to stick with the current approach instead,
with your feedback integrated from your other mails, of course.

Thanks,
Daniel