Re: [PATCH net] netfilter: nf_conntrack: fix rt_gateway checks for h323

Hi Julian,

On Wed, Oct 10, 2012 at 02:00:47AM +0300, Julian Anastasov wrote:

	After the change "Adjust semantics of rt->rt_gateway"
(commit f8126f1d51) we should properly match the nexthop when
destinations are directly connected because rt_gateway can be 0.

Signed-off-by: Julian Anastasov <ja@ssi.bg>
---

	This patch needs a closer look from the Netfilter team.

	It restores the check as it was committed originally,
i.e. to compare nexthops. I'm not sure what is the desired logic,
it can depend on the following:

- two directly connected hosts (rt_gateway=0) can be from different
subnets or not

- one party A is the gateway (rt_gateway=0), another party uses
this gateway (rt_gateway=A)

	May be someone that knows this code better can comment
if the check should be different.

Your patch gets it working like before David's change in the
rt_gateway semantics.

I think the H.323 helper is doing "its best" to handle the following
call-forwarding scenario:

1) A calls B.
2) B replies to A that the alternate address is C.
3) A calls C.

Now assume that:

1) all traffic between A and B goes through the firewall.

and

2) all traffic between A and C don't go through the firewall.

If you want a picture, see section 5.2 of this site:

http://people.netfilter.org/zhaojingmin/h323_conntrack_nat_helper/#_Toc133598073

That code below is trying to detect if A and C don't go through the
firewall, just to skip the creation of one useless expectation (since
they can communicate without going through the firewall).

With the code below (function callforward_do_filter):

a) if A and C are on-link, then:
rt_nexthop(rt1, fl1.daddr) != rt_nexthop(rt2, fl2.daddr)

Bad luck, we create the expectation even if we don't need it.

b) if A and C are behind the same next hop:
rt_nexthop(rt1, fl1.daddr) == rt_nexthop(rt2, fl2.daddr)
We don't create the expectation.

c) if A is on-link and C is behind next hop:
rt_nexthop(rt1, fl1.daddr) != rt_nexthop(rt2, fl2.daddr)

Bad luck again, we create the expectation again.

This seems documented. We could make it better if we would have a way
to guess that A and C do not need to communicate through the firewall.

I'll take your patch, it leaves things just like it was before (which
was not really good).

Please, let me know if I'm missing anything. Thanks.