* Re: [PATCH 07/12] sfc: Remove another unused workaround macro
From: David Miller @ 2009-11-29 7:59 UTC (permalink / raw)
To: bhutchings; +Cc: netdev, linux-net-drivers
In-Reply-To: <1259422530.3709.8.camel@localhost>
From: Ben Hutchings <bhutchings@solarflare.com>
Date: Sat, 28 Nov 2009 15:35:30 +0000
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
Applied.
^ permalink raw reply
* Re: [PATCH 06/12] sfc: Remove EFX_WORKAROUND_9141 macro
From: David Miller @ 2009-11-29 7:59 UTC (permalink / raw)
To: bhutchings; +Cc: netdev, linux-net-drivers
In-Reply-To: <1259422524.3709.7.camel@localhost>
From: Ben Hutchings <bhutchings@solarflare.com>
Date: Sat, 28 Nov 2009 15:35:24 +0000
> From: Matthew Slattery <mslattery@solarflare.com>
>
> The "bug9141 workaround" of setting TX_FLUSH_MIN_LEN_EN should really
> be considered as a normal bit of configuration rather than a
> workaround.
>
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
Applied.
^ permalink raw reply
* Re: [PATCH 05/12] sfc: Limit some hardware workarounds to Falcon
From: David Miller @ 2009-11-29 7:59 UTC (permalink / raw)
To: bhutchings; +Cc: netdev, linux-net-drivers
In-Reply-To: <1259422509.3709.6.camel@localhost>
From: Ben Hutchings <bhutchings@solarflare.com>
Date: Sat, 28 Nov 2009 15:35:09 +0000
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
Applied.
^ permalink raw reply
* Re: [PATCH 04/12] sfc: Always start Falcon using the XMAC
From: David Miller @ 2009-11-29 7:59 UTC (permalink / raw)
To: bhutchings; +Cc: netdev, linux-net-drivers
In-Reply-To: <1259422500.3709.5.camel@localhost>
From: Ben Hutchings <bhutchings@solarflare.com>
Date: Sat, 28 Nov 2009 15:35:00 +0000
> From: Steve Hodgson <shodgson@solarflare.com>
>
> The strap bits are only important on Falcon A and all production
> boards using it have fixed-speed 10G PHYs.
>
> Replace dummy MAC operations with default MAC operations.
>
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
Applied.
^ permalink raw reply
* Re: [PATCH 03/12] sfc: Replace MDIO spinlock with mutex
From: David Miller @ 2009-11-29 7:59 UTC (permalink / raw)
To: bhutchings; +Cc: netdev, linux-net-drivers
In-Reply-To: <1259422484.3709.4.camel@localhost>
From: Ben Hutchings <bhutchings@solarflare.com>
Date: Sat, 28 Nov 2009 15:34:44 +0000
> From: Steve Hodgson <shodgson@solarflare.com>
>
> We never use MDIO in atomic context, so we don't need to spin.
>
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
Applied.
^ permalink raw reply
* Re: [PATCH 02/12] sfc: QT202x: Reset before reading PHY id
From: David Miller @ 2009-11-29 7:59 UTC (permalink / raw)
To: bhutchings; +Cc: netdev, linux-net-drivers
In-Reply-To: <1259422469.3709.3.camel@localhost>
From: Ben Hutchings <bhutchings@solarflare.com>
Date: Sat, 28 Nov 2009 15:34:29 +0000
> From: Steve Hodgson <shodgson@solarflare.com>
>
> Reading standard registers on the QT2025C before its firmware has
> booted may cause the boot process to fail. Therefore, follow the
> recommended reset sequence before reading its id registers. Either
> order works for the QT2022C2, so don't differentiate.
>
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
Applied.
^ permalink raw reply
* Re: [PATCH 01/12] sfc: Simplify PHY polling
From: David Miller @ 2009-11-29 7:59 UTC (permalink / raw)
To: bhutchings; +Cc: netdev, linux-net-drivers
In-Reply-To: <1259422445.3709.2.camel@localhost>
From: Ben Hutchings <bhutchings@solarflare.com>
Date: Sat, 28 Nov 2009 15:34:05 +0000
> From: Steve Hodgson <shodgson@solarflare.com>
>
> Falcon can generate events for LASI interrupts from the PHY, but in
> practice we have never implemented this in reference designs. Instead
> we have polled, inserted the appropriate events, and then handled the
> events later. This is a waste of time and code.
>
> Instead, make PHY poll functions update the link state synchronously
> and report whether it changed. We can still make use of the LASI
> registers as a shortcut on the SFT9001.
>
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
Applied.
^ permalink raw reply
* [PATCH] net: Coding style corrections on Sealevel Systems 4021 driver.
From: Rudy Matela @ 2009-11-29 2:49 UTC (permalink / raw)
To: trivial; +Cc: linux-kernel, netdev
Changed braces position on some statements.
Inserted a space between an "if" keyword and a parenthesis.
Signed-off-by: Rudy Matela <rudy.matela@gmail.com>
---
drivers/net/wan/sealevel.c | 11 ++++-------
1 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/drivers/net/wan/sealevel.c b/drivers/net/wan/sealevel.c
index 0c525e2..6b03704 100644
--- a/drivers/net/wan/sealevel.c
+++ b/drivers/net/wan/sealevel.c
@@ -84,8 +84,7 @@ static int sealevel_open(struct net_device *d)
* Link layer up.
*/
- switch (unit)
- {
+ switch (unit) {
case 0:
err = z8530_sync_dma_open(d, slvl->chan);
break;
@@ -133,8 +132,7 @@ static int sealevel_close(struct net_device *d)
hdlc_close(d);
netif_stop_queue(d);
- switch (unit)
- {
+ switch (unit) {
case 0:
z8530_sync_dma_close(d, slvl->chan);
break;
@@ -342,8 +340,7 @@ static void __exit slvl_shutdown(struct slvl_board *b)
z8530_shutdown(&b->board);
- for (u = 0; u < 2; u++)
- {
+ for (u = 0; u < 2; u++) {
struct net_device *d = b->dev[u].chan->netdevice;
unregister_hdlc_device(d);
free_netdev(d);
@@ -391,7 +388,7 @@ static int __init slvl_init_module(void)
static void __exit slvl_cleanup_module(void)
{
- if(slvl_unit)
+ if (slvl_unit)
slvl_shutdown(slvl_unit);
}
^ permalink raw reply related
* Re: tulip : kernel BUG in tulip_up/tulip_resume
From: Jarek Poplawski @ 2009-11-29 0:17 UTC (permalink / raw)
To: Philippe De Muyter; +Cc: grundler, kyle, netdev, linux-kernel
In-Reply-To: <20091108223305.GA21072@frolo.macqel>
Philippe De Muyter wrote, On 11/08/2009 11:33 PM:
> Hello,
>
> I have just installed 2.6.31 (from opensuse 11.2) one a tulip-equipped
> computer and I get the following error message from the kernel :
>
> [ 2495.526390] ------------[ cut here ]------------
> [ 2495.526390] kernel BUG at /usr/src/packages/BUILD/kernel-default-2.6.31.5/linux-2.6.31/include/linux/netdevice.h:439!
> [ 2495.526390] invalid opcode: 0000 [#1] SMP
...
> This comes from a BUG_ON in napi_enable in netdevice.h.
>
> napi_enable itself is called by tulip_up as such :
>
> #ifdef CONFIG_TULIP_NAPI
> napi_enable(&tp->napi);
> #endif
>
> At first reading, a matching napi_disable is called in tulip_down.
>
> Does someone know what could be wrong and have a fix or should I look myself ?
Don't know, guess only...
Jarek P.
---
drivers/net/tulip/tulip_core.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/tulip/tulip_core.c b/drivers/net/tulip/tulip_core.c
index 0df983b..6b1456c 100644
--- a/drivers/net/tulip/tulip_core.c
+++ b/drivers/net/tulip/tulip_core.c
@@ -1787,11 +1787,11 @@ static int tulip_resume(struct pci_dev *pdev)
return retval;
}
- netif_device_attach(dev);
-
if (netif_running(dev))
tulip_up(dev);
+ netif_device_attach(dev);
+
return 0;
}
^ permalink raw reply related
* [PATCH v2] ep93xx-eth: check for zero MAC address on probe, not on device open
From: Florian Fainelli @ 2009-11-28 23:57 UTC (permalink / raw)
To: netdev; +Cc: David Miller, buytenh
If we happen to have registered the driver without passing
a MAC address, we will print a zero MAC address and register
the interface with this invalid address, this is confusin. This
patch moves the checking of a valid ethernet address and the
generation of a random one down from the open function to
the probe function.
Signed-off-by: Florian Fainelli <florian@openwrt.org>
---
diff --git a/drivers/net/arm/ep93xx_eth.c b/drivers/net/arm/ep93xx_eth.c
index 2be49c8..87601ca 100644
--- a/drivers/net/arm/ep93xx_eth.c
+++ b/drivers/net/arm/ep93xx_eth.c
@@ -628,15 +628,6 @@ static int ep93xx_open(struct net_device *dev)
if (ep93xx_alloc_buffers(ep))
return -ENOMEM;
- if (is_zero_ether_addr(dev->dev_addr)) {
- random_ether_addr(dev->dev_addr);
- printk(KERN_INFO "%s: generated random MAC address "
- "%.2x:%.2x:%.2x:%.2x:%.2x:%.2x.\n", dev->name,
- dev->dev_addr[0], dev->dev_addr[1],
- dev->dev_addr[2], dev->dev_addr[3],
- dev->dev_addr[4], dev->dev_addr[5]);
- }
-
napi_enable(&ep->napi);
if (ep93xx_start_hw(dev)) {
@@ -876,6 +867,9 @@ static int ep93xx_eth_probe(struct platform_device *pdev)
ep->mii.mdio_read = ep93xx_mdio_read;
ep->mii.mdio_write = ep93xx_mdio_write;
ep->mdc_divisor = 40; /* Max HCLK 100 MHz, min MDIO clk 2.5 MHz. */
+
+ if (is_zero_ether_addr(dev->dev_addr))
+ random_ether_addr(dev->dev_addr);
err = register_netdev(dev);
if (err) {
^ permalink raw reply related
* [PATCH] ep93xx-eth: check for zero MAC address on probe, not on device open
From: Florian Fainelli @ 2009-11-28 23:43 UTC (permalink / raw)
To: netdev, buytenh; +Cc: David Miller
If we happen to have registered the driver without passing
a MAC address, we will print a zero MAC address and register
the interface with this invalid address, this is confusin. This
patch moves the checking of a valid ethernet address and the
generation of a random one down from the open function to
the probe function.
Signed-off-by: Florian Fainelli <florian@openwrt.org>
---
diff --git a/drivers/net/arm/ep93xx_eth.c b/drivers/net/arm/ep93xx_eth.c
index 2be49c8..47e0d9f 100644
--- a/drivers/net/arm/ep93xx_eth.c
+++ b/drivers/net/arm/ep93xx_eth.c
@@ -628,15 +628,6 @@ static int ep93xx_open(struct net_device *dev)
if (ep93xx_alloc_buffers(ep))
return -ENOMEM;
- if (is_zero_ether_addr(dev->dev_addr)) {
- random_ether_addr(dev->dev_addr);
- printk(KERN_INFO "%s: generated random MAC address "
- "%.2x:%.2x:%.2x:%.2x:%.2x:%.2x.\n", dev->name,
- dev->dev_addr[0], dev->dev_addr[1],
- dev->dev_addr[2], dev->dev_addr[3],
- dev->dev_addr[4], dev->dev_addr[5]);
- }
-
napi_enable(&ep->napi);
if (ep93xx_start_hw(dev)) {
@@ -876,13 +867,16 @@ static int ep93xx_eth_probe(struct platform_device *pdev)
ep->mii.mdio_read = ep93xx_mdio_read;
ep->mii.mdio_write = ep93xx_mdio_write;
ep->mdc_divisor = 40; /* Max HCLK 100 MHz, min MDIO clk 2.5 MHz. */
+
+ if (is_zero_ether_addr(dev->dev_addr))
+ random_ether_addr(dev->dev_addr);
err = register_netdev(dev);
if (err) {
dev_err(&pdev->dev, "Failed to register netdev\n");
goto err_out;
}
-
+
printk(KERN_INFO "%s: ep93xx on-chip ethernet, IRQ %d, "
"%.2x:%.2x:%.2x:%.2x:%.2x:%.2x.\n", dev->name,
ep->irq, data->dev_addr[0], data->dev_addr[1],
^ permalink raw reply related
* Re: [tproxy,regression] tproxy broken in 2.6.32
From: jamal @ 2009-11-28 22:20 UTC (permalink / raw)
To: David Miller; +Cc: hidden, kaber, hidden, aschultz, tproxy, netdev
In-Reply-To: <20091128.132158.34557470.davem@davemloft.net>
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
^ permalink raw reply
* Re: [ tc ] RTNETLINK answers: Invalid argument
From: Jarek Poplawski @ 2009-11-28 22:14 UTC (permalink / raw)
To: thomas yang; +Cc: netdev
In-Reply-To: <f4f837ab0911240312x329043b9y7e4694f046121237@mail.gmail.com>
thomas yang wrote, On 11/24/2009 12:12 PM:
> # tc filter add dev eth1 parent 1: protocol ip prio 10 u32 match ip
> src 192.168.1.0/24 flowid 1:16 action mirred egress mirror dev wlan0
> Action 3 device wlan0 ifindex 4
> RTNETLINK answers: Invalid argument
> We have an error talking to the kernel
>
>
> # uname -a
> Linux localhost.localdomain 2.6.30.8-64.fc11.i586 #1 SMP Fri Sep 25
> 04:30:19 EDT 2009 i686 i686 i386 GNU/Linux
>
> OS : Fedora 11
# tc filter add dev eth0 parent 1: protocol ip prio 10 u32 match ip
src 192.168.1.0/24 flowid 1:16 action mirred egress mirror dev wlan0
Action 3 device wlan0 ifindex 3
# uname -a
Linux ami 2.6.32-rc8g #72 PREEMPT Sat Nov 21 14:41:09 CET 2009 i686 GNU/Linux
I checked 2.6.31 too. So, generally it should work with current kernels,
at least x86_32.
Jarek P.
^ permalink raw reply
* Re: [tproxy,regression] tproxy broken in 2.6.32
From: David Miller @ 2009-11-28 21:22 UTC (permalink / raw)
To: kaber; +Cc: hidden, hadi, hidden, aschultz, tproxy, netdev
In-Reply-To: <4B1145F1.3090704@trash.net>
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.
^ permalink raw reply
* Re: [tproxy,regression] tproxy broken in 2.6.32
From: David Miller @ 2009-11-28 21:21 UTC (permalink / raw)
To: hadi; +Cc: hidden, kaber, hidden, aschultz, tproxy, netdev
In-Reply-To: <1259437442.3864.61.camel@bigi>
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.
^ permalink raw reply
* Re: [tproxy,regression] tproxy broken in 2.6.32
From: jamal @ 2009-11-28 19:44 UTC (permalink / raw)
To: KOVACS Krisztian
Cc: Patrick McHardy, KOVACS Krisztian, Andreas Schultz, tproxy,
netdev
In-Reply-To: <20091128190500.GB12264@sch.bme.hu>
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
^ permalink raw reply
* Re: [tproxy,regression] tproxy broken in 2.6.32
From: jamal @ 2009-11-28 19:26 UTC (permalink / raw)
To: KOVACS Krisztian; +Cc: KOVACS Krisztian, Andreas Schultz, tproxy, netdev
In-Reply-To: <20091128185019.GA12264@sch.bme.hu>
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
^ permalink raw reply
* Re: [tproxy,regression] tproxy broken in 2.6.32
From: KOVACS Krisztian @ 2009-11-28 19:05 UTC (permalink / raw)
To: jamal
Cc: Patrick McHardy, KOVACS Krisztian, KOVACS Krisztian,
Andreas Schultz, tproxy, netdev
In-Reply-To: <1259429774.3864.41.camel@bigi>
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
^ permalink raw reply
* Re: [tproxy,regression] tproxy broken in 2.6.32
From: KOVACS Krisztian @ 2009-11-28 18:50 UTC (permalink / raw)
To: jamal; +Cc: KOVACS Krisztian, KOVACS Krisztian, Andreas Schultz, tproxy,
netdev
In-Reply-To: <1259423157.3864.9.camel@bigi>
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
^ permalink raw reply
* Re: [tproxy,regression] tproxy broken in 2.6.32
From: jamal @ 2009-11-28 17:36 UTC (permalink / raw)
To: Patrick McHardy
Cc: KOVACS Krisztian, KOVACS Krisztian, Andreas Schultz, tproxy,
netdev
In-Reply-To: <4B1158CE.90803@trash.net>
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
^ permalink raw reply
* Re: [tproxy,regression] tproxy broken in 2.6.32
From: Patrick McHardy @ 2009-11-28 17:07 UTC (permalink / raw)
To: hadi; +Cc: KOVACS Krisztian, KOVACS Krisztian, Andreas Schultz, tproxy,
netdev
In-Reply-To: <1259424278.3864.16.camel@bigi>
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? :)
^ permalink raw reply
* Re: [tproxy,regression] tproxy broken in 2.6.32
From: jamal @ 2009-11-28 16:04 UTC (permalink / raw)
To: Patrick McHardy
Cc: KOVACS Krisztian, KOVACS Krisztian, Andreas Schultz, tproxy,
netdev
In-Reply-To: <4B1145F1.3090704@trash.net>
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
^ permalink raw reply
* Re: [tproxy,regression] tproxy broken in 2.6.32
From: Patrick McHardy @ 2009-11-28 15:46 UTC (permalink / raw)
To: KOVACS Krisztian; +Cc: jamal, KOVACS Krisztian, Andreas Schultz, tproxy, netdev
In-Reply-To: <20091128151515.GA20476@sch.bme.hu>
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.
^ permalink raw reply
* Re: [tproxy,regression] tproxy broken in 2.6.32
From: jamal @ 2009-11-28 15:45 UTC (permalink / raw)
To: KOVACS Krisztian; +Cc: KOVACS Krisztian, Andreas Schultz, tproxy, netdev
In-Reply-To: <20091128151515.GA20476@sch.bme.hu>
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
^ permalink raw reply
* Re: [tproxy,regression] tproxy broken in 2.6.32
From: KOVACS Krisztian @ 2009-11-28 15:15 UTC (permalink / raw)
To: jamal; +Cc: KOVACS Krisztian, Andreas Schultz, tproxy, netdev
In-Reply-To: <1259337932.3299.3.camel@bigi>
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
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox