Re: [PATCH 08/13] netfilter: ctnetlink: Resolve conntrack L3-protocol flush regression

[Felix Kaechele <felix@kaechele.ca>]

On 2019-06-25 7:52 a.m., Kristian Evensen wrote:

> I am sorry for the trouble that my change has caused.

No worries. I appreciate you taking the time helping me out.

>> this patch is giving me some trouble as it breaks deletion of conntrack
>> entries in software that doesn't set the version flag to anything else
>> but 0.
> 
> I might be a bit slow, but I have some trouble understanding this
> sentence. Is what you are saying that software that sets version to
> anything but 0 breaks?

Yeah, definitely not my best work of prose ;-)
What I was trying to say is: Any software that remains with the version 
set to 0 will not work. By association, since libnetfilter_conntrack 
explicitly sets the version to 0, anything that uses 
libnetfilter_conntrack will be unable to delete a specific entry in the 
conntrack table.

> According to the discussion triggered by the
> patch adding the feature that we fix here (see the thread [PATCH
> 07/31] netfilter: ctnetlink: Support L3 protocol-filter on flush),
> using the version field is the correct solution. Pablo wrote:
> 
>> The version field was meant to deal with this case.
>>
>> It has been not unused so far because we had no good reason.
> 
> So I guess Nicholas worry was correct, that there are applications
> that leave version uninitialized and they trigger the regression. I
> will let others decide if not setting version counts as a regression
> or incorrect API usage. If an uninitialized version counts as a
> regression, I am fine with reverting and will try to come up with
> another approach. However, I guess we now might have users that depend
> on the new behavior of flush as well.

I believe that's not the issue here.

So here's what my understanding is of what is happening:

Let's go back to that line of code:

u_int8_t u3 = nfmsg->version ? nfmsg->nfgen_family : AF_UNSPEC;

Just to make sure I understand this correctly: If the version is set to 
0 the address family (l3proto) will be set to AF_UNSPEC regardless of 
what the actual l3proto was set to by the user of the API.
It is only set to the value chosen by the if the version is set to a 
non-null value.
We assume that all clients that require the old behaviour set their 
version to 0, since that's the only valid value to set it to at this 
point anyway.

The problem that arises from this:
When creating a conntrack entry with libnetfilter_conntrack it will 
happly accept the address family you supply. Let's assume we create an 
entry where AF_INET is supplied.
The entry will be created with AF_INET as it's l3proto. Hence, the tuple 
and its hash are only going to be matched upon search if the l3proto 
matches as well.

Now, when you try to delete an entry using libnetfilter_conntrack it 
will explicitly set it's version to 0 in the process of creating the 
message to the API, as it usually does.
Therefor the kernel, under the new behaviour, will disregard the l3proto 
and set AF_UNSPEC instead. By doing this we have created a new tuple 
with a hash that is different from the hash of the tuple that was used 
when creating the conntrack entry, since it was initially created with 
AF_INET, not AF_UNSPEC.
Therefor the search will turn up with no results as the tuples we 
actually want to match are now different, after AF_INET was yanked and 
AF_UNSPEC was put in.
Because the search doesn't find the entry we're trying to delete it will 
return ENOENT and our entry will not be deleted.

I hope my description is somewhat comprehensible. I'm basically thinking 
out loud here.

I have to admit that I also don't know how to approach this in a way 
that will not break userspace but also satisfy the requirements of users 
that would like L3 proto filtering.
I believe the changes required to make it work would also need to 
permeate into how searches are performed. Specifically it would need to 
include tuples with all possible l3protos when AF_UNSPEC is given, so we 
actually gather all entries regardless of l3proto. I believe that this 
is the intended behaviour, right?

Regards,
   Felix