Re: [tproxy,regression] tproxy broken in 2.6.32

Re: [tproxy,regression] tproxy broken in 2.6.32

[Andreas Schultz]

Hi,

git bisect shows that TPROXY has been broken by commit
f7c6fd2465d8e6f4f89c5d1262da10b4a6d499d0, [PATCH] net: Fix RPF to work
with policy routing

I had a look at the patch, and it seems logical that this would break TPROXY.

Andreas

On Wed, Nov 25, 2009 at 9:23 AM, KOVACS Krisztian <hidden@balabit.hu> wrote:
> Hi,
>
> On Mon, 2009-11-23 at 13:43 +0100, Andreas Schultz wrote:
>> I was trying to replace a setup based on a 2.6.27.14 kernel with a
>> 2.6.32-rc8 kernel and found that TPROXY is no longer working.
>>
>> The 2.6.27.14 kernel had the last stable tproxy patch plus some
>> additional fixes (TIME_WAIT, inet_sk_flowi_flags).
>> Since 2.6.32 is supposed to have working tproxy support, i dropped all patches.
>>
>> Now, connections to the local tproxy port no longer arrive at that port.
>> From the kernel log:
>>
>> Nov 23 12:32:31 scg01-wiwob user.debug kernel: tproxy socket lookup:
>> proto 6 ac19c4df:49175 -> c0a80208:80, lookup type: 2, sock (null)
>> Nov 23 12:32:31 scg01-wiwob user.debug kernel: tproxy socket lookup:
>> proto 6 ac19c4df:49175 -> c0a80208:3128, lookup type: 1, sock debae040
>> Nov 23 12:32:31 scg01-wiwob user.debug kernel: redirecting: proto 6
>> c0a80208:80 -> 00000000:3128, mark: 880400a0
>>
>>
>> The redirecting message is the last indication of the packet. tcpdump
>> shows that no answer for the initial packet goes out and the listening
>> socket it not notified either.
>
> I'll have a look at this. In the meantime, could you please post your
> kernel config, along with a summary of the iptables & ip rules you're
> using?
>
> Cheers,
> Krisztian
>
>

Re: [tproxy,regression] tproxy broken in 2.6.32

[KOVACS Krisztian]

Hi,

On Thu, 2009-11-26 at 18:19 +0100, Andreas Schultz wrote:
> Hi,
> 
> git bisect shows that TPROXY has been broken by commit
> f7c6fd2465d8e6f4f89c5d1262da10b4a6d499d0, [PATCH] net: Fix RPF to work
> with policy routing
> 
> I had a look at the patch, and it seems logical that this would break TPROXY.

Indeed, that's a good catch. If this is indeed the problem you should be
able to work it around by disabling rpfilter on the ingress interface.
Does it work that way?

Cheers,
Krisztian

Re: [tproxy,regression] tproxy broken in 2.6.32

[Andreas Schultz]

Hi,

On Fri, Nov 27, 2009 at 9:26 AM, KOVACS Krisztian <hidden@balabit.hu> wrote:
> Hi,
>
> On Thu, 2009-11-26 at 18:19 +0100, Andreas Schultz wrote:
>> Hi,
>>
>> git bisect shows that TPROXY has been broken by commit
>> f7c6fd2465d8e6f4f89c5d1262da10b4a6d499d0, [PATCH] net: Fix RPF to work
>> with policy routing
>>
>> I had a look at the patch, and it seems logical that this would break TPROXY.
>
> Indeed, that's a good catch. If this is indeed the problem you should be
> able to work it around by disabling rpfilter on the ingress interface.
> Does it work that way?

That was my first guess also. I disabled rp_filter on all interfaces
and it had no impact. So far, only reverting that patch has solved the
problem.

Andreas

Re: [tproxy,regression] tproxy broken in 2.6.32

[jamal]

On Fri, 2009-11-27 at 09:26 +0100, KOVACS Krisztian wrote:
> Hi,
> 
> On Thu, 2009-11-26 at 18:19 +0100, Andreas Schultz wrote:
> > Hi,
> > 
> > git bisect shows that TPROXY has been broken by commit
> > f7c6fd2465d8e6f4f89c5d1262da10b4a6d499d0, [PATCH] net: Fix RPF to work
> > with policy routing
> > 
> > I had a look at the patch, and it seems logical that this would break TPROXY.
> 
> Indeed, that's a good catch. If this is indeed the problem you should be
> able to work it around by disabling rpfilter on the ingress interface.
> Does it work that way?

Not familiar with tproxy, but I suspect the system doesnt see the mark
before policy routing happens. So probably the wrong route cache gets
created. Easy to validate by dumping the route cache.
If thats so, you have to set the mark in pre-route hook if it uses
iptables.

cheers,
jamal

Re: [tproxy,regression] tproxy broken in 2.6.32

[KOVACS Krisztian]

Hi,

On p, nov 27, 2009 at 11:05:32 -0500, jamal wrote:
> On Fri, 2009-11-27 at 09:26 +0100, KOVACS Krisztian wrote:
> > Hi,
> > 
> > On Thu, 2009-11-26 at 18:19 +0100, Andreas Schultz wrote:
> > > Hi,
> > > 
> > > git bisect shows that TPROXY has been broken by commit
> > > f7c6fd2465d8e6f4f89c5d1262da10b4a6d499d0, [PATCH] net: Fix RPF to work
> > > with policy routing
> > > 
> > > I had a look at the patch, and it seems logical that this would break TPROXY.
> > 
> > Indeed, that's a good catch. If this is indeed the problem you should be
> > able to work it around by disabling rpfilter on the ingress interface.
> > Does it work that way?
> 
> Not familiar with tproxy, but I suspect the system doesnt see the mark
> before policy routing happens. So probably the wrong route cache gets
> created. Easy to validate by dumping the route cache.
> If thats so, you have to set the mark in pre-route hook if it uses
> iptables.

It's already on prerouting, so that's not the problem.

The problem is that for tproxy to work we've used to have a rule like
this:

# ip rule add fwmark 1 lookup 100

plus a few iptables rules setting mark values.

The issue is that previously fib_validate_source ignored the mark set on
the skb, and thus when fib_validate_source() did a FIB lookup, it all went
fine, because it found a result of type RTN_UNICAST. However, with your
change, and because of the ip rule above not being specific enough now
it's returning with type RTN_LOCAL, and that's considered invalid and thus
the skb is dropped.

The workaround is using more specific ip rules that include the ingress
interface name:

# ip rule add dev eth0 fwmark 1 lookup 100

(repeat the above for each interface except lo.)

Andreas, would you mind giving it a try on your system?

-- 
KOVACS Krisztian

Re: [tproxy,regression] tproxy broken in 2.6.32

[jamal]

On Sat, 2009-11-28 at 16:15 +0100, KOVACS Krisztian wrote:


> It's already on prerouting, so that's not the problem.

ok.

> The problem is that for tproxy to work we've used to have a rule like
> this:
> 
> # ip rule add fwmark 1 lookup 100
> 
> plus a few iptables rules setting mark values.
> 
> The issue is that previously fib_validate_source ignored the mark set on
> the skb, and thus when fib_validate_source() did a FIB lookup, it all went
> fine, because it found a result of type RTN_UNICAST.


Ok, that would be it ;-> 


>  However, with your
> change, and because of the ip rule above not being specific enough now
> it's returning with type RTN_LOCAL, and that's considered invalid and thus
> the skb is dropped.

Well, since we are validating a source address - only unicast routes
are legitimate imo. i.e it was wrong to allow local before.

> 
> The workaround is using more specific ip rules that include the ingress
> interface name:
> 
> # ip rule add dev eth0 fwmark 1 lookup 100
> 

Or adding routes into table 100 with type "unicast" would do it as
well.

cheers,
jamal

Re: [tproxy,regression] tproxy broken in 2.6.32

[KOVACS Krisztian]

Hi,

On szo, nov 28, 2009 at 10:45:57 -0500, jamal wrote:
> >  However, with your
> > change, and because of the ip rule above not being specific enough now
> > it's returning with type RTN_LOCAL, and that's considered invalid and thus
> > the skb is dropped.
> 
> Well, since we are validating a source address - only unicast routes
> are legitimate imo. i.e it was wrong to allow local before.
> 
> > 
> > The workaround is using more specific ip rules that include the ingress
> > interface name:
> > 
> > # ip rule add dev eth0 fwmark 1 lookup 100
> > 
> 
> Or adding routes into table 100 with type "unicast" would do it as
> well.

Well, the only route we're interested in is the following (see
Documentation/networking/tproxy.txt for the details):

ip route add local 0.0.0.0/0 dev lo table 100

Adding a unicast route is not really an option, so I'd say the only
workaround is modifying rules to include the ingress device names.

-- 
KOVACS Krisztian

Re: [tproxy,regression] tproxy broken in 2.6.32

[jamal]

On Sat, 2009-11-28 at 19:50 +0100, KOVACS Krisztian wrote:

> Well, the only route we're interested in is the following (see
> Documentation/networking/tproxy.txt for the details):
> 
> ip route add local 0.0.0.0/0 dev lo table 100
> 

ip route add unicast 0.0.0.0/0 dev lo table 100
would work.

> Adding a unicast route is not really an option, 

Can you explain this part?

cheers,
jamal

Re: [tproxy,regression] tproxy broken in 2.6.32

[Patrick McHardy]

KOVACS Krisztian wrote:
> Hi,
> 
> On p, nov 27, 2009 at 11:05:32 -0500, jamal wrote:
>> On Fri, 2009-11-27 at 09:26 +0100, KOVACS Krisztian wrote:
>>> Hi,
>>>
>>> On Thu, 2009-11-26 at 18:19 +0100, Andreas Schultz wrote:
>>>> Hi,
>>>>
>>>> git bisect shows that TPROXY has been broken by commit
>>>> f7c6fd2465d8e6f4f89c5d1262da10b4a6d499d0, [PATCH] net: Fix RPF to work
>>>> with policy routing
>>>>
>>>> I had a look at the patch, and it seems logical that this would break TPROXY.
>>> Indeed, that's a good catch. If this is indeed the problem you should be
>>> able to work it around by disabling rpfilter on the ingress interface.
>>> Does it work that way?
>> Not familiar with tproxy, but I suspect the system doesnt see the mark
>> before policy routing happens. So probably the wrong route cache gets
>> created. Easy to validate by dumping the route cache.
>> If thats so, you have to set the mark in pre-route hook if it uses
>> iptables.
> 
> It's already on prerouting, so that's not the problem.
> 
> The problem is that for tproxy to work we've used to have a rule like
> this:
> 
> # ip rule add fwmark 1 lookup 100
> 
> plus a few iptables rules setting mark values.
> 
> The issue is that previously fib_validate_source ignored the mark set on
> the skb, and thus when fib_validate_source() did a FIB lookup, it all went
> fine, because it found a result of type RTN_UNICAST. However, with your
> change, and because of the ip rule above not being specific enough now
> it's returning with type RTN_LOCAL, and that's considered invalid and thus
> the skb is dropped.
> 
> The workaround is using more specific ip rules that include the ingress
> interface name:
> 
> # ip rule add dev eth0 fwmark 1 lookup 100

The root cause seems to be an invalid assumption, marks are often not
used in a symetric fashion as required by RPF.

Since this patch has already proven to break existing setups, I think
it should be reverted or the behaviour made optional with a default to
off.

Re: [tproxy,regression] tproxy broken in 2.6.32

[jamal]

On Sat, 2009-11-28 at 16:46 +0100, Patrick McHardy wrote:

> 
> The root cause seems to be an invalid assumption, marks are often not
> used in a symetric fashion as required by RPF.

The only assumption is: if you set set up a mark on incoming, you are
asking the reverse validation that is to be used to consider that mark.

This has nothing to do with RPF really;-> RPF is off. There is a legit
bug in the old setup that has a table programmed with a route that is
not unicast. 

> Since this patch has already proven to break existing setups, I think
> it should be reverted or the behaviour made optional with a default to
> off.

I disagree. What other setup is broken? ;->

cheers,
jamal

Re: [tproxy,regression] tproxy broken in 2.6.32

[Patrick McHardy]

jamal wrote:
> On Sat, 2009-11-28 at 16:46 +0100, Patrick McHardy wrote:
> 
>> The root cause seems to be an invalid assumption, marks are often not
>> used in a symetric fashion as required by RPF.
> 
> The only assumption is: if you set set up a mark on incoming, you are
> asking the reverse validation that is to be used to consider that mark.
> 
> This has nothing to do with RPF really;-> RPF is off. There is a legit
> bug in the old setup that has a table programmed with a route that is
> not unicast. 

Right, its source validation. But the setup is valid, its asking for
specifically marked packets to be delivered locally for transparent
proxying. There's no requirement that rules using marks must resolve
to RTN_UNICAST.

>> Since this patch has already proven to break existing setups, I think
>> it should be reverted or the behaviour made optional with a default to
>> off.
> 
> I disagree. What other setup is broken? ;->

Isn't one setup usually considered enough? :)

Re: [tproxy,regression] tproxy broken in 2.6.32

[jamal]

On Sat, 2009-11-28 at 18:07 +0100, Patrick McHardy wrote:

> Right, its source validation. But the setup is valid, its asking for
> specifically marked packets to be delivered locally for transparent
> proxying. There's no requirement that rules using marks must resolve
> to RTN_UNICAST.

True, but that requirement is needed for source validation;->
i.e it is source address validation imposing the requirement
that we must have a RTN_UNICAST route. The tproxy iproute setup entered
a route that was not RTN_UNICAST. I think that the packet deserves to be
beaten with a club then dropped hard into an abyss (Feel free to come up
with  something more medievial to do to it Patrick;-> )
It doesnt make sense to have a source address that is not unicast
belonging to a host or pretending to belong to a host.
So i didnt introduce that logic thats causing this pain.
If it worked before it was hack or fluke imo ;-> If we think that
source address validation needs to check for something else
additionally, i think thats a separate topic (but doesnt
seem worth a change)

cheers,
jamal

Re: [tproxy,regression] tproxy broken in 2.6.32

[KOVACS Krisztian]

Hi,

On szo, nov 28, 2009 at 12:36:14 -0500, jamal wrote:
> On Sat, 2009-11-28 at 18:07 +0100, Patrick McHardy wrote:
> 
> > Right, its source validation. But the setup is valid, its asking for
> > specifically marked packets to be delivered locally for transparent
> > proxying. There's no requirement that rules using marks must resolve
> > to RTN_UNICAST.
> 
> True, but that requirement is needed for source validation;->
> i.e it is source address validation imposing the requirement
> that we must have a RTN_UNICAST route. The tproxy iproute setup entered
> a route that was not RTN_UNICAST. I think that the packet deserves to be
> beaten with a club then dropped hard into an abyss (Feel free to come up
> with  something more medievial to do to it Patrick;-> )
> 
> It doesnt make sense to have a source address that is not unicast
> belonging to a host or pretending to belong to a host.

The source address *is* unicast. The problem is that the routing setup is
asymmetrical, as Patrick has already mentioned: we're using a mark to
force certain packets (those that have matching sockets on the host) being
delivered locally.

In the other direction, reply packets won't be marked by the iptables
rules and thus will be routed on egress just fine. Your modification has
the assumption that routing is symmetrical, and that reply packets will
have the same mark. That assumption is not necessarily right, and I think
it's not entirely unreasonable to think that not only tproxy setups will
be broken by the change.

> So i didnt introduce that logic thats causing this pain.

Well, it depends whether or not you consider the initial setup valid.

> If it worked before it was hack or fluke imo ;-> If we think that
> source address validation needs to check for something else
> additionally, i think thats a separate topic (but doesnt
> seem worth a change)

My only concern is that this definitely breaks current setups, and while
we do have a workaround we don't have a way to let all users know what
needs to be done...

-- 
KOVACS Krisztian

Re: [tproxy,regression] tproxy broken in 2.6.32

[jamal]

On Sat, 2009-11-28 at 20:05 +0100, KOVACS Krisztian wrote:
> Hi,
> 

> The source address *is* unicast. 

Sorry - I meant the route type is unicast. The fact that an address is
unicast or not is already dealt with by the time you get to source
address validation (in ip_input())

> The problem is that the routing setup is
> asymmetrical, as Patrick has already mentioned: we're using a mark to
> force certain packets (those that have matching sockets on the host) being
> delivered locally.
>
> In the other direction, reply packets won't be marked by the iptables
> rules and thus will be routed on egress just fine. 

In that case i dont understand the reluctance to use unicast routes.
Maybe you can explain and put me at ease because i see youve put extra
effort to use local addresses. 

> Your modification has
> the assumption that routing is symmetrical, and that reply packets will
> have the same mark. That assumption is not necessarily right, and I think
> it's not entirely unreasonable to think that not only tproxy setups will
> be broken by the change.
> 
> > So i didnt introduce that logic thats causing this pain.
> 
> Well, it depends whether or not you consider the initial setup valid.
> 

Based on what i see - I frankly dont. If i looked up the source address
and found that it belonged to something other than unicast route - we
drop it. It doesnt matter whether you use policy routing, rpf or not.
It may be the solution is to allow local routes under certain conditions
although i dont understand why.

> > If it worked before it was hack or fluke imo ;-> If we think that
> > source address validation needs to check for something else
> > additionally, i think thats a separate topic (but doesnt
> > seem worth a change)
> 
> My only concern is that this definitely breaks current setups, and while
> we do have a workaround we don't have a way to let all users know what
> needs to be done...

This code just went in i think. And really this is a user space issue;
i am not unreasonable - please convince me i am just having a technical
challenge understanding your desire to use local instead of unicast.

cheers,
jamal

Re: [tproxy,regression] tproxy broken in 2.6.32

[David Miller]

From: jamal <hadi@cyberus.ca>
Date: Sat, 28 Nov 2009 14:44:02 -0500

> On Sat, 2009-11-28 at 20:05 +0100, KOVACS Krisztian wrote:
>> Well, it depends whether or not you consider the initial setup valid.
>> 
> 
> Based on what i see - I frankly dont. If i looked up the source address

What matters is that this worked for years and we broke it.

There is no other valid discussion about this.

The only thing to "pick" right now is whether we revert the
thing completely or add a sysctl and default it to off.

I prefer the former because nobody is going to turn the thing
on, especially not distributions, and that's %99.9999 of users.

Re: [tproxy,regression] tproxy broken in 2.6.32

[jamal]

On Sat, 2009-11-28 at 13:21 -0800, David Miller wrote:

> What matters is that this worked for years and we broke it.

But Tproxy just went in.

> There is no other valid discussion about this.

Surely we can have a valid technical discussion, no? 
I would like to hear from Krisztian his reasoning for using LOCAL
routes. There may be good reasons.

> The only thing to "pick" right now is whether we revert the
> thing completely or add a sysctl and default it to off.
>
> I prefer the former because nobody is going to turn the thing
> on, especially not distributions, and that's %99.9999 of users.

There is nothing to sysctl control. 
IMO, what is at stake here is the check:

-----
        if (res.type != RTN_UNICAST)
                goto e_inval_res;
----

There are several ways to resolve that:
a) either we say RTN_LOCAL is also legit if some
skb->transparent is set. IMO it is not worth it.
b) have the routing table (as programmed by the user) return 
RTN_UNICAST
c)do the approach Krisztian talked about - which is also
user space controlled.

cheers,
jamal

Re: [tproxy,regression] tproxy broken in 2.6.32

[KOVACS Krisztian]

Hi,

On szo, nov 28, 2009 at 02:44:02 -0500, jamal wrote:
> > The source address *is* unicast. 
> 
> Sorry - I meant the route type is unicast. The fact that an address is
> unicast or not is already dealt with by the time you get to source
> address validation (in ip_input())
> 
> > The problem is that the routing setup is
> > asymmetrical, as Patrick has already mentioned: we're using a mark to
> > force certain packets (those that have matching sockets on the host) being
> > delivered locally.
> >
> > In the other direction, reply packets won't be marked by the iptables
> > rules and thus will be routed on egress just fine. 
> 
> In that case i dont understand the reluctance to use unicast routes.
> Maybe you can explain and put me at ease because i see youve put extra
> effort to use local addresses. 

The short answer is: it doesn't work with unicast routes.

The story is that we really do want to deliver these packets locally, as
if the destination IP address was locally configured on the host. The only
way I know of to get the packet to ip_local_deliver() is by using a local
route.

(Now that you mentioned this I've actually gave it a try. Changing 'local'
in the route to 'unicast' doesn't work at all: incoming packets get
dropped because forwarding is not enabled on the ingress interface.)

-- 
KOVACS Krisztian

Re: [tproxy,regression] tproxy broken in 2.6.32

[jamal]

On Sun, 2009-11-29 at 21:35 +0100, KOVACS Krisztian wrote:

> The story is that we really do want to deliver these packets locally, as
> if the destination IP address was locally configured on the host. The only
> way I know of to get the packet to ip_local_deliver() is by using a local
> route.

Aha, now i understand where both you and Patrick are coming from. So
you literally have to hit the main(or default) table in the reverse
source validation. How does the workaround (that you suggested) work
then? i.e you are going to fail the RTN_UNICAST test no matter what.

Dave, give me some short time to mull this over. I am not sure i like
the sysctl approach - we may have to just revert the whole thing
instead.

cheers,
jamal

Re: [tproxy,regression] tproxy broken in 2.6.32

[KOVACS Krisztian]

Hi,

On Mon, 2009-11-30 at 07:15 -0500, jamal wrote:
> On Sun, 2009-11-29 at 21:35 +0100, KOVACS Krisztian wrote:
> 
> > The story is that we really do want to deliver these packets locally, as
> > if the destination IP address was locally configured on the host. The only
> > way I know of to get the packet to ip_local_deliver() is by using a local
> > route.
> 
> Aha, now i understand where both you and Patrick are coming from. So
> you literally have to hit the main(or default) table in the reverse
> source validation. How does the workaround (that you suggested) work
> then? i.e you are going to fail the RTN_UNICAST test no matter what.

No, because by narrowing the rule to specific ingress interfaces the
lookup done in fib_validate_source() won't match the rule(s) (because
the flow used will have iif set to the loopback device), and thus it
will look up the main table and select a unicast route.

> Dave, give me some short time to mull this over. I am not sure i like
> the sysctl approach - we may have to just revert the whole thing
> instead.

I don't think it would be unreasonable to add a sysctl but disable the
feature by default. It's up to you, of course.

Cheers,
Krisztian

Re: [tproxy,regression] tproxy broken in 2.6.32

[jamal]

[-- Attachment #1: Type: text/plain, Size: 943 bytes --]

On Mon, 2009-11-30 at 13:45 +0100, KOVACS Krisztian wrote:
> Hi,
> 

> No, because by narrowing the rule to specific ingress interfaces the
> lookup done in fib_validate_source() won't match the rule(s) (because
> the flow used will have iif set to the loopback device), and thus it
> will look up the main table and select a unicast route.

ok, nice;->

> I don't think it would be unreasonable to add a sysctl but disable the
> feature by default. It's up to you, of course.

The problem is it is hard to decide where the proper exception is to be
made. I can make it only worry about the scenario you are faced with
something like attached (incomplete, uncompiled, untested) patch. Will
that do it or can you think of other scenarios where source validation
is done and this is needed (example forwarding path)? Patrick?
[I could move the check into fib_validate, but that would punish other
users with a few extra cycles]. 

cheers,
jamal

[-- Attachment #2: fib-val-sysctl --]
[-- Type: text/x-patch, Size: 587 bytes --]

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 5b1050a..f509f7b 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -2139,9 +2139,12 @@ static int ip_route_input_slow(struct sk_buff *skb, __be32 daddr, __be32 saddr,
 
 	if (res.type == RTN_LOCAL) {
 		int result;
+		int mark = 0;
+		if (sysctl_mark_validate_source)
+			mark = skb->mark;
 		result = fib_validate_source(saddr, daddr, tos,
 					     net->loopback_dev->ifindex,
-					     dev, &spec_dst, &itag, skb->mark);
+					     dev, &spec_dst, &itag, mark);
 		if (result < 0)
 			goto martian_source;
 		if (result)

Re: [tproxy,regression] tproxy broken in 2.6.32

[jamal]

[-- Attachment #1: Type: text/plain, Size: 247 bytes --]

On Mon, 2009-11-30 at 08:59 -0500, jamal wrote:

> [I could move the check into fib_validate, but that would punish other
> users with a few extra cycles]. 

As in the following patch (gleaned from Patrick's patch on send to self)

cheers,
jamal


[-- Attachment #2: fib-val-sysctl2 --]
[-- Type: text/x-patch, Size: 2734 bytes --]

diff --git a/include/linux/inetdevice.h b/include/linux/inetdevice.h
index ad27c7d..9cd0bcf 100644
--- a/include/linux/inetdevice.h
+++ b/include/linux/inetdevice.h
@@ -83,6 +83,7 @@ static inline void ipv4_devconf_setall(struct in_device *in_dev)
 #define IN_DEV_FORWARD(in_dev)		IN_DEV_CONF_GET((in_dev), FORWARDING)
 #define IN_DEV_MFORWARD(in_dev)		IN_DEV_ANDCONF((in_dev), MC_FORWARDING)
 #define IN_DEV_RPFILTER(in_dev)		IN_DEV_MAXCONF((in_dev), RP_FILTER)
+#define IN_DEV_SRC_VMARK(in_dev)    	IN_DEV_ORCONF((in_dev), SRC_VMARK)
 #define IN_DEV_SOURCE_ROUTE(in_dev)	IN_DEV_ANDCONF((in_dev), \
 						       ACCEPT_SOURCE_ROUTE)
 #define IN_DEV_BOOTP_RELAY(in_dev)	IN_DEV_ANDCONF((in_dev), BOOTP_RELAY)
diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index 1e4743e..843f71b 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -490,6 +490,7 @@ enum
 	NET_IPV4_CONF_PROMOTE_SECONDARIES=20,
 	NET_IPV4_CONF_ARP_ACCEPT=21,
 	NET_IPV4_CONF_ARP_NOTIFY=22,
+	NET_IPV4_CONF_SRC_VMARK=23,
 	__NET_IPV4_CONF_MAX
 };
 
diff --git a/kernel/sysctl_check.c b/kernel/sysctl_check.c
index b6e7aae..469193c 100644
--- a/kernel/sysctl_check.c
+++ b/kernel/sysctl_check.c
@@ -220,6 +220,7 @@ static const struct trans_ctl_table trans_net_ipv4_conf_vars_table[] = {
 	{ NET_IPV4_CONF_PROMOTE_SECONDARIES,	"promote_secondaries" },
 	{ NET_IPV4_CONF_ARP_ACCEPT,		"arp_accept" },
 	{ NET_IPV4_CONF_ARP_NOTIFY,		"arp_notify" },
+	{ NET_IPV4_CONF_SRC_VMARK,		"src_valid_mark" },
 	{}
 };
 
diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index 5df2f6a..0030e73 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -1450,6 +1450,7 @@ static struct devinet_sysctl_table {
 		DEVINET_SYSCTL_RW_ENTRY(SEND_REDIRECTS, "send_redirects"),
 		DEVINET_SYSCTL_RW_ENTRY(ACCEPT_SOURCE_ROUTE,
 					"accept_source_route"),
+		DEVINET_SYSCTL_RW_ENTRY(SRC_VMARK, "src_valid_mark"),
 		DEVINET_SYSCTL_RW_ENTRY(PROXY_ARP, "proxy_arp"),
 		DEVINET_SYSCTL_RW_ENTRY(MEDIUM_ID, "medium_id"),
 		DEVINET_SYSCTL_RW_ENTRY(BOOTP_RELAY, "bootp_relay"),
diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
index aa00398..b489135 100644
--- a/net/ipv4/fib_frontend.c
+++ b/net/ipv4/fib_frontend.c
@@ -241,16 +241,19 @@ int fib_validate_source(__be32 src, __be32 dst, u8 tos, int oif,
 			    .iif = oif };
 
 	struct fib_result res;
-	int no_addr, rpf;
+	int no_addr, rpf, validate_mark;
 	int ret;
 	struct net *net;
 
-	no_addr = rpf = 0;
+	no_addr = rpf = validate_mark = 0;
 	rcu_read_lock();
 	in_dev = __in_dev_get_rcu(dev);
 	if (in_dev) {
 		no_addr = in_dev->ifa_list == NULL;
 		rpf = IN_DEV_RPFILTER(in_dev);
+		validate_mark = IN_DEV_SRC_VMARK(in_dev);
+		if (!validate_mark)
+			mark = 0;
 	}
 	rcu_read_unlock();
 

Re: [tproxy,regression] tproxy broken in 2.6.32

[David Miller]

From: jamal <hadi@cyberus.ca>
Date: Tue, 01 Dec 2009 08:34:48 -0500

> On Mon, 2009-11-30 at 08:59 -0500, jamal wrote:
> 
>> [I could move the check into fib_validate, but that would punish other
>> users with a few extra cycles]. 
> 
> As in the following patch (gleaned from Patrick's patch on send to self)

Tproxy folks, please have a look at Jamal's patch, thanks.

Re: [tproxy,regression] tproxy broken in 2.6.32

[jamal]

BTW, it should be noted that the change from Patrick to fib_validate
which allows to accept local routes from will also solve this problem.
My suggestion below is to restore old expected behavior..

cheers,
jamal
 
On Wed, 2009-12-02 at 22:31 -0800, David Miller wrote:
> From: jamal <hadi@cyberus.ca>
> Date: Tue, 01 Dec 2009 08:34:48 -0500
> 
> > On Mon, 2009-11-30 at 08:59 -0500, jamal wrote:
> > 
> >> [I could move the check into fib_validate, but that would punish other
> >> users with a few extra cycles]. 
> > 
> > As in the following patch (gleaned from Patrick's patch on send to self)
> 
> Tproxy folks, please have a look at Jamal's patch, thanks.
> --

Re: [tproxy,regression] tproxy broken in 2.6.32

[Patrick McHardy]

jamal wrote:
> BTW, it should be noted that the change from Patrick to fib_validate
> which allows to accept local routes from will also solve this problem.
> My suggestion below is to restore old expected behavior..

Agreed, the accept_local sysctl should not be misused for this,
otherwise TPROXY setups wouldn't have source validation anymore.

Re: [tproxy,regression] tproxy broken in 2.6.32

[KOVACS Krisztian]

Hi,

On Thu, 2009-12-03 at 14:55 +0100, Patrick McHardy wrote:
> jamal wrote:
> > BTW, it should be noted that the change from Patrick to fib_validate
> > which allows to accept local routes from will also solve this problem.
> > My suggestion below is to restore old expected behavior..
> 
> Agreed, the accept_local sysctl should not be misused for this,
> otherwise TPROXY setups wouldn't have source validation anymore.

Absolutely agreed.

Cheers,
Krisztian

Re: [tproxy,regression] tproxy broken in 2.6.32

[jamal]

On Thu, 2009-12-03 at 15:07 +0100, KOVACS Krisztian wrote:
> Hi,
> 
> On Thu, 2009-12-03 at 14:55 +0100, Patrick McHardy wrote:

> > Agreed, the accept_local sysctl should not be misused for this,
> > otherwise TPROXY setups wouldn't have source validation anymore.
> 
> Absolutely agreed.
> 

Ok, thanks.
Dave - i can resubmit on top of Patricks changes once you swallow them.
Or if you consider this a bug fix then i could submit before Patrick's
(which is essentially that patch). 
Let me know.

cheers,
jamal

[PATCH] net: restore ip source validation WAS(Re: [tproxy,regression] tproxy broken in 2.6.32

[jamal]

[-- Attachment #1: Type: text/plain, Size: 343 bytes --]

On Thu, 2009-12-03 at 09:29 -0500, jamal wrote:

> Dave - i can resubmit on top of Patricks changes once you swallow them.
> Or if you consider this a bug fix then i could submit before Patrick's
> (which is essentially that patch). 
> Let me know.

Ok, I am gonna pull a Rick-Jones;-> and ASS-U-ME the attached patch is
fine.

cheers,
jamal


[-- Attachment #2: src-valid-m --]
[-- Type: text/plain, Size: 2818 bytes --]

commit 24a5f4ecb9f90be07f357b55a3c782088789a990
Author: Jamal Hadi Salim <hadi@cyberus.ca>
Date:   Sun Dec 13 11:25:52 2009 -0500

    [PATCH] net: restore ip source validation
    
    when using policy routing and the skb mark:
    there are cases where a back path validation requires us
    to use a different routing table for src ip validation than
    the one used for mapping ingress dst ip.
    One such a case is transparent proxying where we pretend to be
    the destination system and therefore the local table
    is used for incoming packets but possibly a main table would
    be used on outbound.
    Make the default behavior to allow the above and if users
    need to turn on the symmetry via sysctl src_valid_mark
    
    Signed-off-by: Jamal Hadi Salim <hadi@cyberus.ca>

diff --git a/include/linux/inetdevice.h b/include/linux/inetdevice.h
index 699e85c..b230492 100644
--- a/include/linux/inetdevice.h
+++ b/include/linux/inetdevice.h
@@ -81,6 +81,7 @@ static inline void ipv4_devconf_setall(struct in_device *in_dev)
 #define IN_DEV_FORWARD(in_dev)		IN_DEV_CONF_GET((in_dev), FORWARDING)
 #define IN_DEV_MFORWARD(in_dev)		IN_DEV_ANDCONF((in_dev), MC_FORWARDING)
 #define IN_DEV_RPFILTER(in_dev)		IN_DEV_MAXCONF((in_dev), RP_FILTER)
+#define IN_DEV_SRC_VMARK(in_dev)    	IN_DEV_ORCONF((in_dev), SRC_VMARK)
 #define IN_DEV_SOURCE_ROUTE(in_dev)	IN_DEV_ANDCONF((in_dev), \
 						       ACCEPT_SOURCE_ROUTE)
 #define IN_DEV_ACCEPT_LOCAL(in_dev)	IN_DEV_ORCONF((in_dev), ACCEPT_LOCAL)
diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index 877ba03..bd27fbc 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -482,6 +482,7 @@ enum
 	NET_IPV4_CONF_ARP_ACCEPT=21,
 	NET_IPV4_CONF_ARP_NOTIFY=22,
 	NET_IPV4_CONF_ACCEPT_LOCAL=23,
+	NET_IPV4_CONF_SRC_VMARK=24,
 	__NET_IPV4_CONF_MAX
 };
 
diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index 5cdbc10..040c4f0 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -1397,6 +1397,7 @@ static struct devinet_sysctl_table {
 		DEVINET_SYSCTL_RW_ENTRY(ACCEPT_SOURCE_ROUTE,
 					"accept_source_route"),
 		DEVINET_SYSCTL_RW_ENTRY(ACCEPT_LOCAL, "accept_local"),
+		DEVINET_SYSCTL_RW_ENTRY(SRC_VMARK, "src_valid_mark"),
 		DEVINET_SYSCTL_RW_ENTRY(PROXY_ARP, "proxy_arp"),
 		DEVINET_SYSCTL_RW_ENTRY(MEDIUM_ID, "medium_id"),
 		DEVINET_SYSCTL_RW_ENTRY(BOOTP_RELAY, "bootp_relay"),
diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
index 3323168..3ca6269 100644
--- a/net/ipv4/fib_frontend.c
+++ b/net/ipv4/fib_frontend.c
@@ -252,6 +252,8 @@ int fib_validate_source(__be32 src, __be32 dst, u8 tos, int oif,
 		no_addr = in_dev->ifa_list == NULL;
 		rpf = IN_DEV_RPFILTER(in_dev);
 		accept_local = IN_DEV_ACCEPT_LOCAL(in_dev);
+		if (mark && !IN_DEV_SRC_VMARK(in_dev))
+			mark = 0;
 	}
 	rcu_read_unlock();
 

Re: [PATCH] net: restore ip source validation WAS(Re: [tproxy,regression] tproxy broken in 2.6.32

[Julian Anastasov]

	Hello,

On Sun, 13 Dec 2009, jamal wrote:

> Ok, I am gonna pull a Rick-Jones;-> and ASS-U-ME the attached patch is
> fine.
> 
> cheers,
> jamal

> --- a/net/ipv4/fib_frontend.c
> +++ b/net/ipv4/fib_frontend.c
> @@ -252,6 +252,8 @@ int fib_validate_source(__be32 src, __be32 dst, u8 tos, int oif,
>                 no_addr = in_dev->ifa_list == NULL;
>                 rpf = IN_DEV_RPFILTER(in_dev);
>                 accept_local = IN_DEV_ACCEPT_LOCAL(in_dev);
> +               if (mark && !IN_DEV_SRC_VMARK(in_dev))

	May be "fl.mark = 0;" ?:

> +                       mark = 0;
>         }
>         rcu_read_unlock();

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [PATCH] net: restore ip source validation WAS(Re: [tproxy,regression] tproxy broken in 2.6.32

[jamal]

On Sun, 2009-12-13 at 20:12 +0200, Julian Anastasov wrote:
> 	Hello,

> 	May be "fl.mark = 0;" ?:
> 

Of course. I will resend ..

cheers,
jamal

Re: [PATCH] net: restore ip source validation WAS(Re: [tproxy,regression] tproxy broken in 2.6.32

[jamal]

[-- Attachment #1: Type: text/plain, Size: 125 bytes --]

On Sun, 2009-12-13 at 13:38 -0500, jamal wrote:

> Of course. I will resend ..

Updated. Much thanks Julian.

cheers,
jamal


[-- Attachment #2: src-valid-m2 --]
[-- Type: application/mbox, Size: 789 bytes --]

Re: [PATCH] net: restore ip source validation WAS(Re: [tproxy,regression] tproxy broken in 2.6.32

[jamal]

[-- Attachment #1: Type: text/plain, Size: 301 bytes --]

On Sun, 2009-12-13 at 14:11 -0500, jamal wrote:
> On Sun, 2009-12-13 at 13:38 -0500, jamal wrote:
> 
> > Of course. I will resend ..
> 
> Updated. Much thanks Julian.

Grrr. As a self-proclaimed master craftsman man i insist on blaming some
tool. It must be git! Proper patch attached.

cheers,
jamal

[-- Attachment #2: src-valid-m3 --]
[-- Type: application/mbox, Size: 3018 bytes --]

Re: [PATCH] net: restore ip source validation WAS(Re: [tproxy,regression] tproxy broken in 2.6.32

[David Miller]

From: jamal <hadi@cyberus.ca>
Date: Sun, 13 Dec 2009 14:15:16 -0500

> On Sun, 2009-12-13 at 14:11 -0500, jamal wrote:
>> On Sun, 2009-12-13 at 13:38 -0500, jamal wrote:
>> 
>> > Of course. I will resend ..
>> 
>> Updated. Much thanks Julian.
> 
> Grrr. As a self-proclaimed master craftsman man i insist on blaming some
> tool. It must be git! Proper patch attached.

Don't use binary attachments like that please.

It doesn't end up in patchwork properly, and I need that to
happen in order to track people's work effectively.

Re: [PATCH] net: restore ip source validation WAS(Re: [tproxy,regression] tproxy broken in 2.6.32

[jamal]

[-- Attachment #1: Type: text/plain, Size: 311 bytes --]

On Sun, 2009-12-13 at 19:10 -0800, David Miller wrote:

> Don't use binary attachments like that please.
> 
> It doesn't end up in patchwork properly, and I need that to
> happen in order to track people's work effectively.

That was me venturing into git-land. This is how i normally send them:

cheers,
jamal

[-- Attachment #2: src-valid-m3-1 --]
[-- Type: text/plain, Size: 2821 bytes --]

commit 2a69541430da1a888605e6092f5b35a76efd8475
Author: Jamal Hadi Salim <hadi@cyberus.ca>
Date:   Sun Dec 13 14:02:46 2009 -0500

    [PATCH] net: restore ip source validation
    
    when using policy routing and the skb mark:
    there are cases where a back path validation requires us
    to use a different routing table for src ip validation than
    the one used for mapping ingress dst ip.
    One such a case is transparent proxying where we pretend to be
    the destination system and therefore the local table
    is used for incoming packets but possibly a main table would
    be used on outbound.
    Make the default behavior to allow the above and if users
    need to turn on the symmetry via sysctl src_valid_mark
    
    Signed-off-by: Jamal Hadi Salim <hadi@cyberus.ca>

diff --git a/include/linux/inetdevice.h b/include/linux/inetdevice.h
index 699e85c..b230492 100644
--- a/include/linux/inetdevice.h
+++ b/include/linux/inetdevice.h
@@ -81,6 +81,7 @@ static inline void ipv4_devconf_setall(struct in_device *in_dev)
 #define IN_DEV_FORWARD(in_dev)		IN_DEV_CONF_GET((in_dev), FORWARDING)
 #define IN_DEV_MFORWARD(in_dev)		IN_DEV_ANDCONF((in_dev), MC_FORWARDING)
 #define IN_DEV_RPFILTER(in_dev)		IN_DEV_MAXCONF((in_dev), RP_FILTER)
+#define IN_DEV_SRC_VMARK(in_dev)    	IN_DEV_ORCONF((in_dev), SRC_VMARK)
 #define IN_DEV_SOURCE_ROUTE(in_dev)	IN_DEV_ANDCONF((in_dev), \
 						       ACCEPT_SOURCE_ROUTE)
 #define IN_DEV_ACCEPT_LOCAL(in_dev)	IN_DEV_ORCONF((in_dev), ACCEPT_LOCAL)
diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index 877ba03..bd27fbc 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -482,6 +482,7 @@ enum
 	NET_IPV4_CONF_ARP_ACCEPT=21,
 	NET_IPV4_CONF_ARP_NOTIFY=22,
 	NET_IPV4_CONF_ACCEPT_LOCAL=23,
+	NET_IPV4_CONF_SRC_VMARK=24,
 	__NET_IPV4_CONF_MAX
 };
 
diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index 5cdbc10..040c4f0 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -1397,6 +1397,7 @@ static struct devinet_sysctl_table {
 		DEVINET_SYSCTL_RW_ENTRY(ACCEPT_SOURCE_ROUTE,
 					"accept_source_route"),
 		DEVINET_SYSCTL_RW_ENTRY(ACCEPT_LOCAL, "accept_local"),
+		DEVINET_SYSCTL_RW_ENTRY(SRC_VMARK, "src_valid_mark"),
 		DEVINET_SYSCTL_RW_ENTRY(PROXY_ARP, "proxy_arp"),
 		DEVINET_SYSCTL_RW_ENTRY(MEDIUM_ID, "medium_id"),
 		DEVINET_SYSCTL_RW_ENTRY(BOOTP_RELAY, "bootp_relay"),
diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
index 3323168..82dbf71 100644
--- a/net/ipv4/fib_frontend.c
+++ b/net/ipv4/fib_frontend.c
@@ -252,6 +252,8 @@ int fib_validate_source(__be32 src, __be32 dst, u8 tos, int oif,
 		no_addr = in_dev->ifa_list == NULL;
 		rpf = IN_DEV_RPFILTER(in_dev);
 		accept_local = IN_DEV_ACCEPT_LOCAL(in_dev);
+		if (mark && !IN_DEV_SRC_VMARK(in_dev))
+			fl.mark = 0;
 	}
 	rcu_read_unlock();
 

Re: [PATCH] net: restore ip source validation WAS(Re: [tproxy,regression] tproxy broken in 2.6.32

[David Miller]

From: jamal <hadi@cyberus.ca>
Date: Mon, 14 Dec 2009 05:19:45 -0500

> On Sun, 2009-12-13 at 19:10 -0800, David Miller wrote:
> 
>> Don't use binary attachments like that please.
>> 
>> It doesn't end up in patchwork properly, and I need that to
>> happen in order to track people's work effectively.
> 
> That was me venturing into git-land. This is how i normally send them:

Applied, thanks Jamal.

I hope this puts the tproxy regression to rest :-)

Re: [PATCH] net: restore ip source validation WAS(Re: [tproxy,regression] tproxy broken in 2.6.32

[jamal]

On Fri, 2009-12-25 at 17:30 -0800, David Miller wrote:

> I hope this puts the tproxy regression to rest :-)

It should. The only open question is whether this is considered
a "bug fix" and needs to be propagated to -stable.

cheers,
jamal

Re: [PATCH] net: restore ip source validation WAS(Re: [tproxy,regression] tproxy broken in 2.6.32

[David Miller]

From: jamal <hadi@cyberus.ca>
Date: Sat, 26 Dec 2009 10:05:14 -0500

> On Fri, 2009-12-25 at 17:30 -0800, David Miller wrote:
> 
>> I hope this puts the tproxy regression to rest :-)
> 
> It should. The only open question is whether this is considered
> a "bug fix" and needs to be propagated to -stable.

I do intend to send it there, so I do consider it a bug fix.

Re: [tproxy,regression] tproxy broken in 2.6.32

[David Miller]

From: jamal <hadi@cyberus.ca>
Date: Mon, 30 Nov 2009 07:15:59 -0500

> Dave, give me some short time to mull this over. I am not sure i like
> the sysctl approach - we may have to just revert the whole thing
> instead.

Ok, please come to some conclusion by the end of this week.

Re: [tproxy,regression] tproxy broken in 2.6.32

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Sat, 28 Nov 2009 16:46:57 +0100

> Since this patch has already proven to break existing setups, I think
> it should be reverted or the behaviour made optional with a default to
> off.

Absolutely, and fully, agreed.  That is the only criteria for
deciding what to do here, it worked for an enormously long
time and it stopped doing so with the change in question.

"that configuration doesn't make sense" and other such talk
is completely irrelevant.