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
