Re: [PATCH v4 2/3] netfilter: nf_conntrack: add direction support for zones

[Daniel Borkmann <daniel@iogearbox.net>]

Hi Pablo,

thanks a lot for applying patch 1/3!

On 08/12/2015 07:48 PM, Pablo Neira Ayuso wrote:
...
> On Sat, Aug 08, 2015 at 09:40:02PM +0200, Daniel Borkmann wrote:
>> diff --git a/include/uapi/linux/netfilter/nfnetlink_conntrack.h b/include/uapi/linux/netfilter/nfnetlink_conntrack.h
>> index acad6c5..3bf4cb0 100644
>> --- a/include/uapi/linux/netfilter/nfnetlink_conntrack.h
>> +++ b/include/uapi/linux/netfilter/nfnetlink_conntrack.h
>> @@ -53,6 +53,7 @@ enum ctattr_type {
>>   	CTA_MARK_MASK,
>>   	CTA_LABELS,
>>   	CTA_LABELS_MASK,
>> +	CTA_TUPLE_ZONE,
>
> I remember to have suggested to place this in ctattr_tuple:
>
> http://www.spinics.net/lists/netfilter-devel/msg37593.html
>
> The zone is part of the tuple in an optional fashion, so it should
> appear there. The direction is already implicit based on
> CTA_TUPLE_ORIG or CTA_TUPLE_REPLY.

Sorry, seems like I totally misunderstood your email. :/

I thought to place a CTA_ZONE_DIR attribute into a new nested CTA_TUPLE_ZONE
container, where also possible future meta data can be placed there.

Thus, we'd have CTA_ZONE as the id itself and CTA_TUPLE_ZONE with additional
optional data related to the zone, but it seems this was your /initial/
suggestion (modulo the attribute name). I actually find this approach quite
reasonable, probably that's why my mind stuck to it too much. ;)

But you are basically saying to add the nested CTA_TUPLE_ZONE container here,
that is part of a nested CTA_TUPLE_ORIG and/or CTA_TUPLE_REPLY attribute ...

enum ctattr_tuple {
	CTA_TUPLE_UNSPEC,
	CTA_TUPLE_IP,
	CTA_TUPLE_PROTO,
	CTA_TUPLE_ZONE,  <---
	__CTA_TUPLE_MAX
};

... where CTA_TUPLE_ZONE would be a container for further attributes, say
CTA_TUPLE_ZONE_ID, which is then the actual NLA_U16 zone id, right?

So, we'd have a zone id spread in 3 possible places, and additional (future)
meta data spread around in 2 possible places, hmm ... Okay, lets say, we'd
add future attribute X and Y to zones. Now, if I want a zone only in ORIG
dir or only in REPLY dir, that works fine from ctnetlink perspective, even
with your idea that there could be two different non-default zones entirely.

But, lets say I just want to use a traditional zones config (as in: nowadays)
and have my tuple for /one/ particular zone id that is the same in /both/
directions. That would mean I have to duplicate my parameters X and Y across
CTA_TUPLE_ORIG and CTA_TUPLE_REPLY, right? Or, we'd add a third attribute
set (as in: CTA_ZONE_INFO) only for the single zone in both directions?

So far I find the current approach a bit cleaner to be honest (I can, of
course, still change the CTA_TUPLE_ZONE back into CTA_ZONE_INFO name) ...
but when the time comes where someone really should need two /non-default/
zones for a single tuple, don't we need a global setting as in this patch
here anyway (due to reasons above)? (I'm fine either way, I'm just asking on
how we want to handle this in an ideal/clean way.)

...
> I'd suggest the output shows the zone on the corresponding tuple, eg.
> in case it only applies to the original tuple:
>
> udp      17 29 src=192.168.2.195 dst=192.168.2.1 sport=40446 dport=53 zone=1 \
>                 src=192.168.2.1 dst=192.168.2.195 sport=53 dport=40446 [ASSURED] mark=0 use=1
>
> We have a more compact output IMO.

Okay, that's fine by me. It would mean we'd see zone=1 twice in case a
direction was not specified (thus, both directions apply), but I think
that should be totally okay for the stand-alone interface (and in future
conntrack -L).

> Please, don't forget that you also have to update
> libnetfilter_conntrack and conntrack to get this feature available
> from there. I'll take this patchset to the kernel so you have the time
> to update the userspace side later on without blocking this further.

Thanks, yes, after Plumbers I'll add proper support for both.

For testing that the netlink interface works, I had a local hack, but
will get properly ready after the kernel and iptables patches. Was planning
to do this anyway.

Thanks again,
Daniel