* Re: MSI interrupts and disable_irq
From: Yinghai Lu @ 2007-10-14 19:55 UTC (permalink / raw)
To: Manfred Spraul
Cc: Jeff Garzik, Ayaz Abdulla, nedev, Linux Kernel Mailing List,
David Miller, Andrew Morton
In-Reply-To: <4711C217.2010605@colorfullife.com>
On 10/14/07, Manfred Spraul <manfred@colorfullife.com> wrote:
> Yinghai Lu wrote:
> > On 10/13/07, Manfred Spraul <manfred@colorfullife.com> wrote:
> >
> >> Someone around with a MSI capable board? The forcedeth driver does
> >> dev->irq = pci_dev->irq
> >> in nv_probe(), especially before pci_enable_msi().
> >> Does pci_enable_msi() change pci_dev->irq? Then we would disable the
> >> wrong interrupt....
> >>
> >
> > the request_irq==>setup_irq will make dev->irq = pci_dev->irq.
> >
> >
> Where is that?
in nv_request_irq
if ((ret = pci_enable_msi(np->pci_dev)) == 0) {
np->msi_flags |= NV_MSI_ENABLED;
if (request_irq(np->pci_dev->irq, handler,
IRQF_SHARED, dev->name, dev) != 0) {
in request_irq
int request_irq(unsigned int irq, irq_handler_t handler,
unsigned long irqflags, const char *devname, void *dev_id)
...
action->dev_id = dev_id;
...
retval = setup_irq(irq, action);
in setup_irq
int setup_irq(unsigned int irq, struct irqaction *new)
....
new->irq = irq;
it seems I missed that here new is irqaction instead of net_dev.
YH
^ permalink raw reply
* Re: kernel BUG at net/core/dev.c:1383 skb_checksum_help: BUG_ON(offset > (int)skb->len)
From: Guillaume Chazarain @ 2007-10-14 20:20 UTC (permalink / raw)
To: netdev
In-Reply-To: <20071014192640.35065e73@inria.fr>
Le Sun, 14 Oct 2007 19:26:40 +0200,
Guillaume Chazarain <guichaz@yahoo.fr> a écrit :
> #0 0xc02c9d5e in skb_checksum_help (skb=0xe218dcb0) at net/core/dev.c:1372
> 1372 BUG_ON(offset > (int)skb->len);
Just another data point, it seems that running the standard Bittorrent
client helps triggering the bug.
Thanks.
--
Guillaume
^ permalink raw reply
* Re: [PATCH v3 3/4] FEC mpc52xx: the driver
From: Grant Likely @ 2007-10-14 21:43 UTC (permalink / raw)
To: Domen Puncer; +Cc: galak, jgarzik, linuxppc-dev, tnt, netdev
In-Reply-To: <20071014075921.GF3000@nd47.coderock.org>
On 10/14/07, Domen Puncer <domen.puncer@telargo.com> wrote:
> Driver for ethernet on mpc5200/mpc5200b SoCs (FEC).
>
>
> Signed-off-by: Domen Puncer <domen.puncer@telargo.com>
Looks quite good to me. Comments below...
>
> ---
> drivers/net/Kconfig | 1
> drivers/net/Makefile | 1
> drivers/net/fec_mpc52xx/Kconfig | 15
> drivers/net/fec_mpc52xx/Makefile | 2
> drivers/net/fec_mpc52xx/fec.c | 1098 +++++++++++++++++++++++++++++++++++++++
> drivers/net/fec_mpc52xx/fec.h | 313 +++++++++++
> 6 files changed, 1430 insertions(+)
>
> Index: linux.git/drivers/net/Kconfig
> ===================================================================
> --- linux.git.orig/drivers/net/Kconfig
> +++ linux.git/drivers/net/Kconfig
> @@ -1894,6 +1894,7 @@ config NE_H8300
> controller on the Renesas H8/300 processor.
>
> source "drivers/net/fec_8xx/Kconfig"
> +source "drivers/net/fec_mpc52xx/Kconfig"
Personally, I'd just add the mpc52xx items to drivers/net/Kconfig; but
I won't make a fuss either way.
===================================================================
> --- linux.git.orig/drivers/net/Makefile
> +++ linux.git/drivers/net/Makefile
> @@ -210,6 +210,7 @@ obj-$(CONFIG_SMC911X) += smc911x.o
> obj-$(CONFIG_BFIN_MAC) += bfin_mac.o
> obj-$(CONFIG_DM9000) += dm9000.o
> obj-$(CONFIG_FEC_8XX) += fec_8xx/
> +obj-$(CONFIG_FEC_MPC52xx) += fec_mpc52xx/
Ditto here; Heck, we're only talking about 3 files here. I'd
probably put all three into drivers/net... or even merge the 2 .c and
one .h files into a single file. But, again, it's not a big deal.
> obj-$(CONFIG_PASEMI_MAC) += pasemi_mac.o
> obj-$(CONFIG_MLX4_CORE) += mlx4/
>
> Index: linux.git/drivers/net/fec_mpc52xx/Kconfig
> ===================================================================
> --- /dev/null
> +++ linux.git/drivers/net/fec_mpc52xx/Kconfig
> @@ -0,0 +1,15 @@
> +menu "MPC5200 Networking Options"
> + depends PPC_MPC52xx && NET_ETHERNET
Drop the menu
> +
> +config FEC_MPC52xx
> + tristate "FEC driver"
> + depends on NET_ETHERNET
Drop "NET_ETHERNET"; if you're here, then NET_ETHERNET *is* selected.
Add "PPC_MPC52xx"
> + select PPC_BESTCOMM
> + select PPC_BESTCOMM_FEC
> + select CRC32
> + select PHYLIB
> + ---help---
> + This option enables support for the MPC5200's on-chip
> + Fast Ethernet Controller
> +
> +endmenu
> Index: linux.git/drivers/net/fec_mpc52xx/Makefile
>
Cheers,
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195
^ permalink raw reply
* Re: MSI interrupts and disable_irq
From: Benjamin Herrenschmidt @ 2007-10-14 21:47 UTC (permalink / raw)
To: Manfred Spraul
Cc: Yinghai Lu, Jeff Garzik, Ayaz Abdulla, nedev,
Linux Kernel Mailing List, David Miller, Andrew Morton
In-Reply-To: <4711C217.2010605@colorfullife.com>
On Sun, 2007-10-14 at 09:15 +0200, Manfred Spraul wrote:
> Yinghai Lu wrote:
> > On 10/13/07, Manfred Spraul <manfred@colorfullife.com> wrote:
> >
> >> Someone around with a MSI capable board? The forcedeth driver does
> >> dev->irq = pci_dev->irq
> >> in nv_probe(), especially before pci_enable_msi().
> >> Does pci_enable_msi() change pci_dev->irq? Then we would disable the
> >> wrong interrupt....
> >>
> >
> > the request_irq==>setup_irq will make dev->irq = pci_dev->irq.
> >
> >
> Where is that?
> Otherwise I would propose the attached patch. My board is not
> MSI-capable, thus I can't test it myself.
Why not just copy pcidev->irq to dev->irq once ?
Ben.
^ permalink raw reply
* Re: [PATCH v3 4/4] FEC mpc52xx: phy part of the driver
From: Grant Likely @ 2007-10-14 22:05 UTC (permalink / raw)
To: Domen Puncer; +Cc: galak, jgarzik, linuxppc-dev, tnt, netdev
In-Reply-To: <20071014075958.GG3000@nd47.coderock.org>
On 10/14/07, Domen Puncer <domen.puncer@telargo.com> wrote:
> PHY part of the driver for mpc5200(b) ethernet.
Assuming I understand correctly, this comment is not correct and this
patch just adds an MDIO bus driver. PHY drivers are in phylib and
data transfer is setup via the core driver, correct?
It is conceivable that the PHY is connected to an alternate MDIO bus,
or the MDIO bus is used for a PHY connected to an external Ethernet
controller.
Speaking of which, is it possible to use this MDIO bus without the
core FEC being initialized?
> +static struct of_device_id fec_mdio_match[] = {
> + {
> + .type = "mdio",
> + .compatible = "mpc5200b-fec-phy",
This is not a phy; it's an MDIO bus. Also, shouldn't this be
"mpc5200-..." instead of "mpc5200b-..."?
> + },
> + {},
> +};
> +
> +struct of_platform_driver mpc52xx_fec_mdio_driver = {
> + .name = "mpc5200b-fec-phy",
> + .probe = fec_mdio_probe,
> + .remove = fec_mdio_remove,
> + .match_table = fec_mdio_match,
Inconsistent naming. Please use the same prefix on all global/static
symbols (ie. use "mpc52xx_mdio_" instead of the mix of
"mpc52xx_fec_mdio_", "fec_mdio_", etc.) I also thing that "fec_mdio_"
is too generic because there are a number of different incompatible
FEC devices.
> +};
> +
> +/* let fec driver call it, since this has to be registered before it */
> +EXPORT_SYMBOL_GPL(mpc52xx_fec_mdio_driver);
Why not have a module_init()/module_exit() in this file? I don't
think the FEC driver calls this driver's functions directly anymore,
and it's still dependent on the of_platform bus probe order anyway.
As an added bonus, it makes your Makefile much simpler.
> +
> +
> +MODULE_LICENSE("Dual BSD/GPL");
> Index: linux.git/drivers/net/fec_mpc52xx/Makefile
> ===================================================================
> --- linux.git.orig/drivers/net/fec_mpc52xx/Makefile
> +++ linux.git/drivers/net/fec_mpc52xx/Makefile
> @@ -1,2 +1,7 @@
> obj-$(CONFIG_FEC_MPC52xx) += fec_mpc52xx.o
> fec_mpc52xx-objs := fec.o
> +
> +ifeq ($(CONFIG_FEC_MPC52xx_MDIO),y)
> + obj-$(CONFIG_FEC_MPC52xx) += fec_mpc52xx_phy.o
> + fec_mpc52xx_phy-objs := fec_phy.o
> +endif
> Index: linux.git/drivers/net/fec_mpc52xx/Kconfig
> ===================================================================
> --- linux.git.orig/drivers/net/fec_mpc52xx/Kconfig
> +++ linux.git/drivers/net/fec_mpc52xx/Kconfig
> @@ -11,5 +11,18 @@ config FEC_MPC52xx
> ---help---
> This option enables support for the MPC5200's on-chip
> Fast Ethernet Controller
> + If compiled as module, it will be called 'fec_mpc52xx.ko'.
Drop this line and make the help text the same format as the other eth
drivers in drivers/net.
> +
> +config FEC_MPC52xx_MDIO
> + bool "FEC MII PHY driver"
> + depends on FEC_MPC52xx
> + default y
> + ---help---
> + The MPC5200's FEC can connect to the Ethernet either with
> + an external MII PHY chip or 10 Mbps 7-wire interface
> + (Motorola? industry standard).
> + If your board uses an external PHY, enable this.
Not strictly true. This enables talking to a PHY using the internal
MDIO controller. PHY register access could just as easily be accessed
via an alternate interface.
> + If not sure, enable.
> + If compiled as module, it will be called 'fec_mpc52xx_phy.ko'.
Drop the module name comment.
Cheers,
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195
^ permalink raw reply
* Re: Don't leak 'listeners' in netlink_kernel_create()
From: Eric W. Biederman @ 2007-10-14 22:30 UTC (permalink / raw)
To: Jesper Juhl
Cc: netdev, Linux Kernel Mailing List, Alan Cox, Alexey Kuznetsov,
David Miller
In-Reply-To: <200710142337.01059.jesper.juhl@gmail.com>
Jesper Juhl <jesper.juhl@gmail.com> writes:
> From: Jesper Juhl <jesper.juhl@gmail.com>
> Subject: Don't leak 'listeners' in netlink_kernel_create()
>
> The Coverity checker spotted that we'll leak the storage allocated
> to 'listeners' in netlink_kernel_create() when the
> if (!nl_table[unit].registered)
> check is false.
>
> This patch avoids the leak.
>
>
> Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
This patch appears trivially correct to me.
Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
>
> af_netlink.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> --- linux-2.6/net/netlink/af_netlink.c~ 2007-10-14 23:29:50.000000000 +0200
> +++ linux-2.6/net/netlink/af_netlink.c 2007-10-14 23:29:50.000000000 +0200
> @@ -1378,6 +1378,8 @@ netlink_kernel_create(struct net *net, i
> nl_table[unit].cb_mutex = cb_mutex;
> nl_table[unit].module = module;
> nl_table[unit].registered = 1;
> + } else {
> + kfree(listeners);
> }
> netlink_table_ungrab();
>
^ permalink raw reply
* Re: MSI interrupts and disable_irq
From: Yinghai Lu @ 2007-10-14 23:15 UTC (permalink / raw)
To: benh
Cc: Manfred Spraul, Jeff Garzik, Ayaz Abdulla, nedev,
Linux Kernel Mailing List, David Miller, Andrew Morton
In-Reply-To: <1192398473.6188.10.camel@pasglop>
On 10/14/07, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
>
> On Sun, 2007-10-14 at 09:15 +0200, Manfred Spraul wrote:
> > Yinghai Lu wrote:
> > > On 10/13/07, Manfred Spraul <manfred@colorfullife.com> wrote:
> > >
> > >> Someone around with a MSI capable board? The forcedeth driver does
> > >> dev->irq = pci_dev->irq
> > >> in nv_probe(), especially before pci_enable_msi().
> > >> Does pci_enable_msi() change pci_dev->irq? Then we would disable the
> > >> wrong interrupt....
> > >>
> > >
> > > the request_irq==>setup_irq will make dev->irq = pci_dev->irq.
> > >
> > >
> > Where is that?
> > Otherwise I would propose the attached patch. My board is not
> > MSI-capable, thus I can't test it myself.
>
> Why not just copy pcidev->irq to dev->irq once ?
it seems e1000 is using np->pci_dev->irq directly too.
YH
^ permalink raw reply
* Re: MSI interrupts and disable_irq
From: Benjamin Herrenschmidt @ 2007-10-14 23:36 UTC (permalink / raw)
To: Yinghai Lu
Cc: Manfred Spraul, Jeff Garzik, Ayaz Abdulla, nedev,
Linux Kernel Mailing List, David Miller, Andrew Morton
In-Reply-To: <86802c440710141615g60fb9775oba590deb1a9fabde@mail.gmail.com>
On Sun, 2007-10-14 at 16:15 -0700, Yinghai Lu wrote:
> On 10/14/07, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> >
> > On Sun, 2007-10-14 at 09:15 +0200, Manfred Spraul wrote:
> > > Yinghai Lu wrote:
> > > > On 10/13/07, Manfred Spraul <manfred@colorfullife.com> wrote:
> > > >
> > > >> Someone around with a MSI capable board? The forcedeth driver does
> > > >> dev->irq = pci_dev->irq
> > > >> in nv_probe(), especially before pci_enable_msi().
> > > >> Does pci_enable_msi() change pci_dev->irq? Then we would disable the
> > > >> wrong interrupt....
> > > >>
> > > >
> > > > the request_irq==>setup_irq will make dev->irq = pci_dev->irq.
> > > >
> > > >
> > > Where is that?
> > > Otherwise I would propose the attached patch. My board is not
> > > MSI-capable, thus I can't test it myself.
> >
> > Why not just copy pcidev->irq to dev->irq once ?
>
> it seems e1000 is using np->pci_dev->irq directly too.
Heh, allright, doesn't matter, I was just proposing to avoid one more
indirection :-)
Ben.
^ permalink raw reply
* [PATCH] netdev: convert non-obvious instances to use ARRAY_SIZE()
From: Alejandro Martinez Ruiz @ 2007-10-15 1:37 UTC (permalink / raw)
To: Jeff Garzik; +Cc: netdev
This will convert remaining non-obvious or naive calculations of array
sizes to use ARRAY_SIZE() macro.
Signed-off-by: Alejandro Martinez Ruiz <alex@flawedcode.org>
---
drivers/net/cassini.c | 2 +-
drivers/net/irda/donauboe.c | 2 +-
drivers/net/ne-h8300.c | 4 ++--
drivers/net/tg3.c | 2 +-
drivers/net/tulip/de4x5.c | 4 ++--
drivers/net/wan/sdla.c | 8 ++++----
6 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/net/cassini.c b/drivers/net/cassini.c
index 563bf5f..7df31b5 100644
--- a/drivers/net/cassini.c
+++ b/drivers/net/cassini.c
@@ -4443,7 +4443,7 @@ static struct {
{REG_MAC_COLL_EXCESS},
{REG_MAC_COLL_LATE}
};
-#define CAS_REG_LEN (sizeof(ethtool_register_table)/sizeof(int))
+#define CAS_REG_LEN ARRAY_SIZE(ethtool_register_table)
#define CAS_MAX_REGS (sizeof (u32)*CAS_REG_LEN)
static void cas_read_regs(struct cas *cp, u8 *ptr, int len)
diff --git a/drivers/net/irda/donauboe.c b/drivers/net/irda/donauboe.c
index 3e5eca1..a82d8f9 100644
--- a/drivers/net/irda/donauboe.c
+++ b/drivers/net/irda/donauboe.c
@@ -840,7 +840,7 @@ toshoboe_probe (struct toshoboe_cb *self)
/* test 1: SIR filter and back to back */
- for (j = 0; j < (sizeof (bauds) / sizeof (int)); ++j)
+ for (j = 0; j < ARRAY_SIZE(bauds); ++j)
{
int fir = (j > 1);
toshoboe_stopchip (self);
diff --git a/drivers/net/ne-h8300.c b/drivers/net/ne-h8300.c
index 368f256..fbc7531 100644
--- a/drivers/net/ne-h8300.c
+++ b/drivers/net/ne-h8300.c
@@ -93,7 +93,7 @@ static int __init init_reg_offset(struct net_device *dev,unsigned long base_addr
bus_width = *(volatile unsigned char *)ABWCR;
bus_width &= 1 << ((base_addr >> 21) & 7);
- for (i = 0; i < sizeof(reg_offset) / sizeof(u32); i++)
+ for (i = 0; i < ARRAY_SIZE(reg_offset); i++)
if (bus_width == 0)
reg_offset[i] = i * 2 + 1;
else
@@ -115,7 +115,7 @@ static int h8300_ne_irq[] = {EXT_IRQ5};
static inline int init_dev(struct net_device *dev)
{
- if (h8300_ne_count < (sizeof(h8300_ne_base) / sizeof(unsigned long))) {
+ if (h8300_ne_count < ARRAY_SIZE(h8300_ne_base)) {
dev->base_addr = h8300_ne_base[h8300_ne_count];
dev->irq = h8300_ne_irq[h8300_ne_count];
h8300_ne_count++;
diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
index 30b1cca..76efb3f 100644
--- a/drivers/net/tg3.c
+++ b/drivers/net/tg3.c
@@ -9034,7 +9034,7 @@ static int tg3_do_mem_test(struct tg3 *tp, u32 offset, u32 len)
int i;
u32 j;
- for (i = 0; i < sizeof(test_pattern)/sizeof(u32); i++) {
+ for (i = 0; i < ARRAY_SIZE(test_pattern); i++) {
for (j = 0; j < len; j += 4) {
u32 val;
diff --git a/drivers/net/tulip/de4x5.c b/drivers/net/tulip/de4x5.c
index 9b9cd83..41f34bb 100644
--- a/drivers/net/tulip/de4x5.c
+++ b/drivers/net/tulip/de4x5.c
@@ -1041,7 +1041,7 @@ static struct InfoLeaf infoleaf_array[] = {
{DC21142, dc21142_infoleaf},
{DC21143, dc21143_infoleaf}
};
-#define INFOLEAF_SIZE (sizeof(infoleaf_array)/(sizeof(int)+sizeof(int *)))
+#define INFOLEAF_SIZE ARRAY_SIZE(infoleaf_array)
/*
** List the SROM info block functions
@@ -1056,7 +1056,7 @@ static int (*dc_infoblock[])(struct net_device *dev, u_char, u_char *) = {
compact_infoblock
};
-#define COMPACT (sizeof(dc_infoblock)/sizeof(int *) - 1)
+#define COMPACT (ARRAY_SIZE(dc_infoblock) - 1)
/*
** Miscellaneous defines...
diff --git a/drivers/net/wan/sdla.c b/drivers/net/wan/sdla.c
index b39a541..05df0a3 100644
--- a/drivers/net/wan/sdla.c
+++ b/drivers/net/wan/sdla.c
@@ -1342,11 +1342,11 @@ static int sdla_set_config(struct net_device *dev, struct ifmap *map)
if (flp->initialized)
return(-EINVAL);
- for(i=0;i < sizeof(valid_port) / sizeof (int) ; i++)
+ for(i=0; i < ARRAY_SIZE(valid_port); i++)
if (valid_port[i] == map->base_addr)
break;
- if (i == sizeof(valid_port) / sizeof(int))
+ if (i == ARRAY_SIZE(valid_port))
return(-EINVAL);
if (!request_region(map->base_addr, SDLA_IO_EXTENTS, dev->name)){
@@ -1487,12 +1487,12 @@ got_type:
}
}
- for(i=0;i < sizeof(valid_mem) / sizeof (int) ; i++)
+ for(i=0; i < ARRAY_SIZE(valid_mem); i++)
if (valid_mem[i] == map->mem_start)
break;
err = -EINVAL;
- if (i == sizeof(valid_mem) / sizeof(int))
+ if (i == ARRAY_SIZE(valid_mem))
goto fail2;
if (flp->type == SDLA_S502A && (map->mem_start & 0xF000) >> 12 == 0x0E)
--
1.5.3.4
^ permalink raw reply related
* Re: [2.6 patch] __inet6_csk_dst_store(): fix check-after-use
From: Masahide NAKAMURA @ 2007-10-15 2:45 UTC (permalink / raw)
To: Adrian Bunk; +Cc: Noriaki TAKAMIYA, netdev, linux-kernel
In-Reply-To: <20071014175212.GP4211@stusta.de>
On Sun, 14 Oct 2007 19:52:12 +0200
Adrian Bunk <bunk@kernel.org> wrote:
> The Coverity checker spotted that we have already oops'ed if "dst"
> was NULL.
>
> Since "dst" being NULL doesn't seem to be possible at this point this
> patch removes the NULL check.
>
> Signed-off-by: Adrian Bunk <bunk@stusta.de>
Agreed.
Acked-by: Masahide NAKAMURA <nakam@linux-ipv6.org>
--
Masahide NAKAMURA
^ permalink raw reply
* Re: [2.6 patch] __inet6_csk_dst_store(): fix check-after-use
From: Noriaki TAKAMIYA @ 2007-10-15 3:37 UTC (permalink / raw)
To: nakam; +Cc: bunk, netdev, linux-kernel
In-Reply-To: <20071015113723.00F7.NAKAM@linux-ipv6.org>
Hi,
>> Mon, 15 Oct 2007 11:45:10 +0900
>> [Subject: Re: [2.6 patch] __inet6_csk_dst_store(): fix check-after-use]
>> Masahide NAKAMURA <nakam@linux-ipv6.org> wrote...
>
> On Sun, 14 Oct 2007 19:52:12 +0200
> Adrian Bunk <bunk@kernel.org> wrote:
>
> > The Coverity checker spotted that we have already oops'ed if "dst"
> > was NULL.
> >
> > Since "dst" being NULL doesn't seem to be possible at this point this
> > patch removes the NULL check.
> >
> > Signed-off-by: Adrian Bunk <bunk@stusta.de>
>
> Agreed.
>
> Acked-by: Masahide NAKAMURA <nakam@linux-ipv6.org>
I also agreed.
Acked-by: Noriaki TAKAMIYA <takamiya@po.ntts.co.jp>
--
Noriaki TAKAMIYA
^ permalink raw reply
* [NET]: Fix csum_start update in pskb_expand_head
From: Herbert Xu @ 2007-10-15 4:57 UTC (permalink / raw)
To: Guillaume Chazarain, David S. Miller; +Cc: netdev
In-Reply-To: <20071014222037.7fa28640@inria.fr>
On Sun, Oct 14, 2007 at 08:20:37PM +0000, Guillaume Chazarain wrote:
>
> Just another data point, it seems that running the standard Bittorrent
> client helps triggering the bug.
Doh! It was my patch a month ago which added code to update
csum_start if the headroom changed that broke this.
We'll need this patch for stable as well.
[NET]: Fix csum_start update in pskb_expand_head
I got confused by the dual nature of the off variable in the
function pskb_expand_head. The csum_start offset should use
nhead instead of off which can change depending on whether we
are using offsets or pointers.
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
However, the fact that you're triggering this code at all tells
us that there is another bug further up the chain that's causing
us to generate partial checksums even though the NIC doesn't
support it. Can you please give me the output of
ethtool -k <IFNAME>
where <IFNAME> is the interface that TCP connection is going
out of?
Thanks,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index c3aa68c..70d9b5d 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -682,7 +682,7 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
skb->transport_header += off;
skb->network_header += off;
skb->mac_header += off;
- skb->csum_start += off;
+ skb->csum_start += nhead;
skb->cloned = 0;
skb->hdr_len = 0;
skb->nohdr = 0;
^ permalink raw reply related
* [patch] do not rely on OPEN_MAX
From: Mike Frysinger @ 2007-10-15 5:50 UTC (permalink / raw)
To: yoshfuji; +Cc: netdev
[-- Attachment #1.1: Type: text/plain, Size: 172 bytes --]
the OPEN_MAX define was removed from the kernel's limits.h during the 2.6.23
development cycle, so switching to using the dynamic sysconf() func should be
done
-mike
[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 827 bytes --]
[-- Attachment #2: iputils-20070202-no-open-max.patch --]
[-- Type: text/x-diff, Size: 367 bytes --]
the OPEN_MAX define has been removed in newer kernel headers so use the
proper method of getting the value dynamically
http://bugs.gentoo.org/195861
--- a/rdisc.c
+++ b/rdisc.c
@@ -247,7 +247,7 @@ void do_fork(void)
if ((pid=fork()) != 0)
exit(0);
- for (t = 0; t < OPEN_MAX; t++)
+ for (t = 0; t < sysconf(_SC_OPEN_MAX); t++)
if (t != s)
close(t);
^ permalink raw reply
* [NET]: Avoid copying TCP packets unnecessarily
From: Herbert Xu @ 2007-10-15 5:54 UTC (permalink / raw)
To: Guillaume Chazarain, David S. Miller; +Cc: netdev
In-Reply-To: <20071015045723.GA28388@gondor.apana.org.au>
On Mon, Oct 15, 2007 at 12:57:23PM +0800, Herbert Xu wrote:
>
> However, the fact that you're triggering this code at all tells
> us that there is another bug further up the chain that's causing
> us to generate partial checksums even though the NIC doesn't
> support it. Can you please give me the output of
Nevermind, tcp_fragment can generate such packets out of
convenience.
Here's a little optimisation to avoid copying the packet in
this case.
[NET]: Avoid copying TCP packets unnecessarily
TCP packets all have writable heads, that is, even though it's cloned, it is
writable up to the end of the TCP header. This patch makes skb_checksum_help
aware of this fact by using skb_clone_writable and avoiding a copy for TCP.
I've also modified the BUG_ON tests to be unsigned. The only case where this
makes a difference is if csum_start points to a location before skb->data.
Since skb->data should always include the header where the checksum field
is (and all currently callers adhere to that), this change is safe and may
uncover bugs later.
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---
net/core/dev.c | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index ab15c11..af75bfd 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1355,22 +1355,21 @@ int skb_checksum_help(struct sk_buff *skb)
goto out_set_summed;
}
- if (skb_cloned(skb)) {
+ offset = skb->csum_start - skb_headroom(skb);
+ BUG_ON(offset >= skb_headlen(skb));
+ csum = skb_checksum(skb, offset, skb->len - offset, 0);
+
+ offset += skb->csum_offset;
+ BUG_ON(offset + sizeof(__sum16) > skb_headlen(skb));
+
+ if (skb_cloned(skb) &&
+ !skb_clone_writable(skb, offset + sizeof(__sum16))) {
ret = pskb_expand_head(skb, 0, 0, GFP_ATOMIC);
if (ret)
goto out;
}
- offset = skb->csum_start - skb_headroom(skb);
- BUG_ON(offset > (int)skb->len);
- csum = skb_checksum(skb, offset, skb->len-offset, 0);
-
- offset = skb_headlen(skb) - offset;
- BUG_ON(offset <= 0);
- BUG_ON(skb->csum_offset + 2 > offset);
-
- *(__sum16 *)(skb->head + skb->csum_start + skb->csum_offset) =
- csum_fold(csum);
+ *(__sum16 *)(skb->data + offset) = csum_fold(csum);
out_set_summed:
skb->ip_summed = CHECKSUM_NONE;
out:
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply related
* Re: [patch] ipv6.7: IPV6_ROUTER_ALERT sockopt correction
From: Pekka Savola @ 2007-10-15 5:51 UTC (permalink / raw)
To: Andrew McDonald; +Cc: netdev
In-Reply-To: <20071014111147.GE20850@mcdonald.org.uk>
Took off linux-man from cc:,
On Sun, 14 Oct 2007, Andrew McDonald wrote:
> +The tapped packets are not forwarded by the kernel, it is the
> +user's responsibility to send them out again.
This is probably incompliant (and from users' perspective,
unacceptible) behaviour that IMHO should be fixed.
--
Pekka Savola "You each name yourselves king, yet the
Netcore Oy kingdom bleeds."
Systems. Networks. Security. -- George R.R. Martin: A Clash of Kings
^ permalink raw reply
* Re: [0/10] Remove sk_buff ** from netfilter API
From: Patrick McHardy @ 2007-10-15 6:25 UTC (permalink / raw)
To: Herbert Xu; +Cc: David Miller, hadi, netdev, kuznet
In-Reply-To: <20071014042604.GA11431@gondor.apana.org.au>
Herbert Xu wrote:
> On Sun, Sep 30, 2007 at 05:38:28PM +0200, Patrick McHardy wrote:
>> >From a quick look it seems at least IPv4 and IPv6 don't need to
>> change the skb anywhere else. It seems to be necessary for
>> bridging though to unshare the skb. OTOH br_netfilter.c already
>> unshares the skb for IPv4 and IPv6, so we could simply do this
>> unconditionally before calling the hooks.
>
> Thanks. I got bored and finally did the plunge :)
>
> I've booted it here and it seems to work.
Thanks Herbert :) I'll have a closer look and give it some testing
myself.
^ permalink raw reply
* Re: [patch] ipv6.7: IPV6_ROUTER_ALERT sockopt correction
From: Michael Kerrisk @ 2007-10-15 6:53 UTC (permalink / raw)
To: linux-man, netdev, Andrew McDonald
In-Reply-To: <20071014111147.GE20850@mcdonald.org.uk>
Hello Andrew,
> I discovered that the current description of the IPV6_ROUTER_ALERT
> sockopt in ipv6.7 is significantly wrong. A patch to fix the
> description is below. I sent a version of this earlier in the year to
> mtk-manpages@gmx.net, but nothing happened with it at the time.
Hmmm -- somehow that message got dropped. I found it in my trash -- sorry
about that.
I've applied your patch for 2.68.
Thanks,
Michael
> The correction is based on reading the relevant parts of the kernel
> source code, and backed up by some test programs. The main bits of code
> in the kernel (in case someone wants to double-check my update) are
> net/ipv6/ipv6_sockglue.c:ip6_ra_control() and
> net/ipv6/ip6_output.c:ip6_call_ra_chain().
>
> The patch is against man-pages-2.66.
> --- man7/ipv6.7.orig 2007-10-14 11:59:46.000000000 +0100
> +++ man7/ipv6.7 2007-10-14 12:05:15.000000000 +0100
> @@ -233,10 +233,17 @@
> Argument is a pointer to boolean.
> .TP
> .B IPV6_ROUTER_ALERT
> -Pass all forwarded packets containing an router alert option to
> +Pass forwarded packets containing a router alert hop-by-hop option to
> this socket.
> -Only allowed for datagram sockets and for root.
> -Argument is a pointer to boolean.
> +Only allowed for SOCK_RAW sockets.
> +The tapped packets are not forwarded by the kernel, it is the
> +user's responsibility to send them out again.
> +Argument is a pointer to an integer.
> +A positive integer indicates a router alert option value to intercept.
> +Packets carrying a router alert option with a value field containing
> +this integer will be delivered to the socket.
> +A negative integer disables delivery of packets with router alert options
> +to this socket.
> .TP
> .B IPV6_UNICAST_HOPS
> Set the unicast hop limit for the socket.
> -
> To unsubscribe from this list: send the line "unsubscribe linux-man" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Michael Kerrisk
maintainer of Linux man pages Sections 2, 3, 4, 5, and 7
Want to help with man page maintenance? Grab the latest tarball at
http://www.kernel.org/pub/linux/docs/manpages/
read the HOWTOHELP file and grep the source files for 'FIXME'.
^ permalink raw reply
* Undocumented IPv6 options
From: Michael Kerrisk @ 2007-10-15 6:54 UTC (permalink / raw)
To: netdev; +Cc: linux-man, Andrew McDonald, linux-net
Hello netdev,
Andrew McDonald kindly fixed the description of IPV6_ROUTER_ALERT in the
ipv7.7 man page. As long as we're on the topic, I'll point out that the
following IPV6 options (and possibly others) are still not documented on
that page:
IPV6_CHECKSUM
IPV6_JOIN_ANYCAST
IPV6_LEAVE_ANYCAST
IPV6_V6ONLY
IPV6_RECVPKTINFO
IPV6_2292PKTINFO
Can anyone help with documenting any of these please?
Cheers,
Michael
--
Michael Kerrisk
maintainer of Linux man pages Sections 2, 3, 4, 5, and 7
Want to help with man page maintenance? Grab the latest tarball at
http://www.kernel.org/pub/linux/docs/manpages/
read the HOWTOHELP file and grep the source files for 'FIXME'.
^ permalink raw reply
* Re: [PATCH 6/10] [BRIDGE]: Unshare skb upon entry
From: Patrick McHardy @ 2007-10-15 7:20 UTC (permalink / raw)
To: Herbert Xu; +Cc: David Miller, hadi, netdev, kuznet
In-Reply-To: <E1Igv4q-00030k-00@gondolin.me.apana.org.au>
[-- Attachment #1: Type: text/plain, Size: 446 bytes --]
Herbert Xu wrote:
> [BRIDGE]: Unshare skb upon entry
>
> Due to the special location of the bridging hook, it should never see a
> shared packet anyway (certainly not with any in-kernel code). So it
> makes sense to unshare the skb there if necessary as that will greatly
> simplify the code below it (in particular, netfilter).
I guess we can remove the unsharing from br_netfilter.c now.
Signed-off-by: Patrick McHardy <kaber@trash.net>
[-- Attachment #2: x --]
[-- Type: text/plain, Size: 465 bytes --]
diff --git a/net/bridge/br_netfilter.c b/net/bridge/br_netfilter.c
index 246bf23..da22f90 100644
--- a/net/bridge/br_netfilter.c
+++ b/net/bridge/br_netfilter.c
@@ -511,9 +511,6 @@ static unsigned int br_nf_pre_routing(unsigned int hook, struct sk_buff *skb,
struct iphdr *iph;
__u32 len = nf_bridge_encap_header_len(skb);
- if ((skb = skb_share_check(skb, GFP_ATOMIC)) == NULL)
- return NF_STOLEN;
-
if (unlikely(!pskb_may_pull(skb, len)))
goto out;
^ permalink raw reply related
* Re: Undocumented IPv6 options
From: Matti Aarnio @ 2007-10-15 7:39 UTC (permalink / raw)
To: Michael Kerrisk; +Cc: netdev, linux-man, Andrew McDonald, linux-net
In-Reply-To: <47130EB9.3090405@gmail.com>
On Mon, Oct 15, 2007 at 08:54:49AM +0200, Michael Kerrisk wrote:
> Hello netdev,
>
> Andrew McDonald kindly fixed the description of IPV6_ROUTER_ALERT in the
> ipv7.7 man page. As long as we're on the topic, I'll point out that the
> following IPV6 options (and possibly others) are still not documented on
> that page:
>
> IPV6_CHECKSUM
> IPV6_JOIN_ANYCAST
> IPV6_LEAVE_ANYCAST
> IPV6_V6ONLY
> IPV6_RECVPKTINFO
> IPV6_2292PKTINFO
>
> Can anyone help with documenting any of these please?
RFC 2553 was obsoleted by RFC 3493, and it added IPV6_V6ONLY into Basic API.
RFC 2292 (IPV6 Advanced API) defines IPV6_CHECKSUM, and IPV6_PKTINFO
(latter of which is known above as IPV6_2292PKTINFO)
RFC 3542 obsoletes RFC 2292, and defines IPV6_RECVPKTINFO, and
modifies IPV6_PKTINFO.
The IPV6_JOIN_ANYCAST and IPV6_LEAVE_ANYCAST are not defined in any RFC.
Undocumented are also IPV6_JOIN_GROUP and IPV6_LEAVE_GROUP (RFC 2553 / 3493)
It really looks like time for major overhaul of that (and related) man-pages
is needed...
> Cheers,
>
> Michael
> --
> Michael Kerrisk
> maintainer of Linux man pages Sections 2, 3, 4, 5, and 7
/Matti Aarnio
^ permalink raw reply
* [IPV4]: Uninline netfilter okfns
From: Patrick McHardy @ 2007-10-15 7:57 UTC (permalink / raw)
To: Herbert Xu; +Cc: David Miller, hadi, netdev, kuznet
In-Reply-To: <20071014042604.GA11431@gondor.apana.org.au>
[-- Attachment #1: Type: text/plain, Size: 280 bytes --]
Your patches work fine so far, tested with fragments, packet
content mangling etc. Thanks again for doing this. And they
really do enable gcc to generate tail-calls for the NF_HOOK
okfns, so this patch changes net/ipv4 to take advantage of
this. IPv6 and bridge will follow ...
[-- Attachment #2: x --]
[-- Type: text/plain, Size: 2795 bytes --]
[IPV4]: Uninline netfilter okfns
Now that we don't pass double skb pointers to nf_hook_slow anymore, gcc
can generate tail calls for some of the netfilter hook okfn invocations,
so there is no need to inline the functions anymore. This caused huge
code bloat since we ended up with one inlined version and one out-of-line
version since we pass the address to nf_hook_slow.
Before:
text data bss dec hex filename
8997385 1016524 524652 10538561 a0ce41 vmlinux
After:
text data bss dec hex filename
8994009 1016524 524652 10535185 a0c111 vmlinux
-------------------------------------------------------
-3376
All cases have been verified to generate tail-calls with and without
netfilter. The okfns in ipmr and xfrm4_input still remain inline because
gcc can't generate tail-calls for them.
Signed-off-by: Patrick McHardy <kaber@trash.net>
---
commit 39533dafc9b0458fecebd2da36d682837c6ff9a2
tree af5f872142f9a0703c167843a1f51642f89c5662
parent 3e7e83795236b54106ef3ac57f140d97300aa5f5
author Patrick McHardy <kaber@trash.net> Mon, 15 Oct 2007 09:56:24 +0200
committer Patrick McHardy <kaber@trash.net> Mon, 15 Oct 2007 09:56:24 +0200
net/ipv4/ip_forward.c | 2 +-
net/ipv4/ip_input.c | 4 ++--
net/ipv4/ip_output.c | 2 +-
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/net/ipv4/ip_forward.c b/net/ipv4/ip_forward.c
index afbf938..877da3e 100644
--- a/net/ipv4/ip_forward.c
+++ b/net/ipv4/ip_forward.c
@@ -40,7 +40,7 @@
#include <net/route.h>
#include <net/xfrm.h>
-static inline int ip_forward_finish(struct sk_buff *skb)
+static int ip_forward_finish(struct sk_buff *skb)
{
struct ip_options * opt = &(IPCB(skb)->opt);
diff --git a/net/ipv4/ip_input.c b/net/ipv4/ip_input.c
index 8f75e43..168c871 100644
--- a/net/ipv4/ip_input.c
+++ b/net/ipv4/ip_input.c
@@ -195,7 +195,7 @@ int ip_call_ra_chain(struct sk_buff *skb)
return 0;
}
-static inline int ip_local_deliver_finish(struct sk_buff *skb)
+static int ip_local_deliver_finish(struct sk_buff *skb)
{
__skb_pull(skb, ip_hdrlen(skb));
@@ -324,7 +324,7 @@ drop:
return -1;
}
-static inline int ip_rcv_finish(struct sk_buff *skb)
+static int ip_rcv_finish(struct sk_buff *skb)
{
const struct iphdr *iph = ip_hdr(skb);
struct rtable *rt;
diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index 699f067..f508835 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -202,7 +202,7 @@ static inline int ip_skb_dst_mtu(struct sk_buff *skb)
skb->dst->dev->mtu : dst_mtu(skb->dst);
}
-static inline int ip_finish_output(struct sk_buff *skb)
+static int ip_finish_output(struct sk_buff *skb)
{
#if defined(CONFIG_NETFILTER) && defined(CONFIG_XFRM)
/* Policy lookup after SNAT yielded a new policy */
^ permalink raw reply related
* Re: [PATCH 6/10] [BRIDGE]: Unshare skb upon entry
From: Herbert Xu @ 2007-10-15 8:04 UTC (permalink / raw)
To: Patrick McHardy; +Cc: David Miller, hadi, netdev, kuznet
In-Reply-To: <471314A0.5080500@trash.net>
On Mon, Oct 15, 2007 at 09:20:00AM +0200, Patrick McHardy wrote:
>
> I guess we can remove the unsharing from br_netfilter.c now.
>
> Signed-off-by: Patrick McHardy <kaber@trash.net>
Well caught!
In fact this would have been a bug before my patch if it ever
triggered (fortunately it shouldn't with the stock kernel).
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [IPV4]: Uninline netfilter okfns
From: Herbert Xu @ 2007-10-15 8:05 UTC (permalink / raw)
To: Patrick McHardy; +Cc: David Miller, hadi, netdev, kuznet
In-Reply-To: <47131D64.3080008@trash.net>
On Mon, Oct 15, 2007 at 09:57:24AM +0200, Patrick McHardy wrote:
>
> Before:
> text data bss dec hex filename
> 8997385 1016524 524652 10538561 a0ce41 vmlinux
>
> After:
> text data bss dec hex filename
> 8994009 1016524 524652 10535185 a0c111 vmlinux
> -------------------------------------------------------
> -3376
Nice work Patrick :)
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH 10/10] [NETFILTER]: Replace sk_buff ** with sk_buff *
From: David Miller @ 2007-10-15 8:27 UTC (permalink / raw)
To: herbert; +Cc: kaber, hadi, netdev, kuznet
In-Reply-To: <20071014095541.GA13222@gondor.apana.org.au>
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Sun, 14 Oct 2007 17:55:41 +0800
> On Sun, Oct 14, 2007 at 12:27:49PM +0800, Herbert Xu wrote:
> > [NETFILTER]: Replace sk_buff ** with sk_buff *
> >
> > 74 files changed, 469 insertions(+), 503 deletions(-)
>
> Between me going to bed last night and waking up this morning,
> my editor wandered off causing half of the patch to disappear :)
>
> Here is the corrected version.
That looks a lot better, I've applied all 10 patches,
thanks Herbert.
^ permalink raw reply
* Re: [PATCH 1/3] [IPV6]: Make ipv6_frag_rcv return the same packet
From: David Miller @ 2007-10-15 8:28 UTC (permalink / raw)
To: herbert; +Cc: kaber, hadi, netdev, kuznet, yoshfuji, tgraf
In-Reply-To: <E1Ih4m7-0003rk-00@gondolin.me.apana.org.au>
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Sun, 14 Oct 2007 22:49:03 +0800
> [IPV6]: Make ipv6_frag_rcv return the same packet
>
> This patch implements the same change taht was done to ip_defrag. It
> makes ipv6_frag_rcv return the last packet received of a train of fragments
> rather than the head of that sequence.
>
> This allows us to get rid of the sk_buff ** argument later.
>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
Applied.
^ 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