* Re: [PATCH net-next] neigh: speedup neigh_resolve_output()
From: Eric Dumazet @ 2010-10-11 19:46 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20101011.121609.246536311.davem@davemloft.net>
Le lundi 11 octobre 2010 à 12:16 -0700, David Miller a écrit :
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Thu, 07 Oct 2010 22:44:07 +0200
>
> > Le jeudi 07 octobre 2010 à 14:53 +0200, Eric Dumazet a écrit :
> >
> >> Further improvements would need to use a seqlock instead of an rwlock to
> >> protect neigh->ha[], to not dirty neigh too often and remove two atomic
> >> ops.
> >>
> >
> > I implemented this idea in following patch, on top on previous one.
> >
> > [PATCH net-next] neigh: speedup neigh_resolve_output()
>
> So Eric do you think this is more efficient than the idea I
> proposed, which is to "cmpxchg 'hh' and RCU"?
>
> If you think this seqlock thing is faster or more desirable
> for some reason, I'll add it.
>
> Thanks!
Hmm, we would need a neigh->ha pointer to some struct, with rcu
protection. It adds a dereference in hot path. I believe this seqlock
(only for pathological cases, where dst are used for few packets) should
be fine.
Note: After this patch, I have a "struct neighbour" small field reorg
pending, not yet submitted.
Thanks
^ permalink raw reply
* Re: [PATCH net-next] net: percpu net_device refcount
From: David Miller @ 2010-10-11 19:41 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev
In-Reply-To: <1286825929.3218.7.camel@edumazet-laptop>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 11 Oct 2010 21:38:49 +0200
> Le lundi 11 octobre 2010 à 12:13 -0700, David Miller a écrit :
>
>> Ok, I'm fine with this.
>>
>> But could you please get rid of that "#if 0" code block? The comment
>> is fine and should stay, but the commented out code shouldn't really
>> stay there.
>
> Sure, here is second version, against latest net-next-2.6
>
> Thanks !
>
> [PATCH net-next] net: percpu net_device refcount
Excellent, applied, thanks!
^ permalink raw reply
* Re: [PATCH net-next] net: percpu net_device refcount
From: Eric Dumazet @ 2010-10-11 19:38 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20101011.121344.260085789.davem@davemloft.net>
Le lundi 11 octobre 2010 à 12:13 -0700, David Miller a écrit :
> Ok, I'm fine with this.
>
> But could you please get rid of that "#if 0" code block? The comment
> is fine and should stay, but the commented out code shouldn't really
> stay there.
Sure, here is second version, against latest net-next-2.6
Thanks !
[PATCH net-next] net: percpu net_device refcount
We tried very hard to remove all possible dev_hold()/dev_put() pairs in
network stack, using RCU conversions.
There is still an unavoidable device refcount change for every dst we
create/destroy, and this can slow down some workloads (routers or some
app servers, mmap af_packet)
We can switch to a percpu refcount implementation, now dynamic per_cpu
infrastructure is mature. On a 64 cpus machine, this consumes 256 bytes
per device.
On x86, dev_hold(dev) code :
before
lock incl 0x280(%ebx)
after:
movl 0x260(%ebx),%eax
incl fs:(%eax)
Stress bench :
(Sending 160.000.000 UDP frames,
IP route cache disabled, dual E5540 @2.53GHz,
32bit kernel, FIB_TRIE)
Before:
real 1m1.662s
user 0m14.373s
sys 12m55.960s
After:
real 0m51.179s
user 0m15.329s
sys 10m15.942s
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
include/linux/netdevice.h | 6 ++---
net/core/dev.c | 39 +++++++++++++++++++++++++++++-------
2 files changed, 35 insertions(+), 10 deletions(-)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 4160db3..c0b5e05 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1026,7 +1026,7 @@ struct net_device {
struct timer_list watchdog_timer;
/* Number of references to this device */
- atomic_t refcnt ____cacheline_aligned_in_smp;
+ int __percpu *pcpu_refcnt;
/* delayed register/unregister */
struct list_head todo_list;
@@ -1798,7 +1798,7 @@ extern void netdev_run_todo(void);
*/
static inline void dev_put(struct net_device *dev)
{
- atomic_dec(&dev->refcnt);
+ irqsafe_cpu_dec(*dev->pcpu_refcnt);
}
/**
@@ -1809,7 +1809,7 @@ static inline void dev_put(struct net_device *dev)
*/
static inline void dev_hold(struct net_device *dev)
{
- atomic_inc(&dev->refcnt);
+ irqsafe_cpu_inc(*dev->pcpu_refcnt);
}
/* Carrier loss detection, dial on demand. The functions netif_carrier_on
diff --git a/net/core/dev.c b/net/core/dev.c
index 193eafa..4b2d820 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5192,9 +5192,6 @@ int init_dummy_netdev(struct net_device *dev)
*/
dev->reg_state = NETREG_DUMMY;
- /* initialize the ref count */
- atomic_set(&dev->refcnt, 1);
-
/* NAPI wants this */
INIT_LIST_HEAD(&dev->napi_list);
@@ -5202,6 +5199,11 @@ int init_dummy_netdev(struct net_device *dev)
set_bit(__LINK_STATE_PRESENT, &dev->state);
set_bit(__LINK_STATE_START, &dev->state);
+ /* Note : We dont allocate pcpu_refcnt for dummy devices,
+ * because users of this 'device' dont need to change
+ * its refcount.
+ */
+
return 0;
}
EXPORT_SYMBOL_GPL(init_dummy_netdev);
@@ -5243,6 +5245,15 @@ out:
}
EXPORT_SYMBOL(register_netdev);
+static int netdev_refcnt_read(const struct net_device *dev)
+{
+ int i, refcnt = 0;
+
+ for_each_possible_cpu(i)
+ refcnt += *per_cpu_ptr(dev->pcpu_refcnt, i);
+ return refcnt;
+}
+
/*
* netdev_wait_allrefs - wait until all references are gone.
*
@@ -5257,11 +5268,14 @@ EXPORT_SYMBOL(register_netdev);
static void netdev_wait_allrefs(struct net_device *dev)
{
unsigned long rebroadcast_time, warning_time;
+ int refcnt;
linkwatch_forget_dev(dev);
rebroadcast_time = warning_time = jiffies;
- while (atomic_read(&dev->refcnt) != 0) {
+ refcnt = netdev_refcnt_read(dev);
+
+ while (refcnt != 0) {
if (time_after(jiffies, rebroadcast_time + 1 * HZ)) {
rtnl_lock();
@@ -5288,11 +5302,13 @@ static void netdev_wait_allrefs(struct net_device *dev)
msleep(250);
+ refcnt = netdev_refcnt_read(dev);
+
if (time_after(jiffies, warning_time + 10 * HZ)) {
printk(KERN_EMERG "unregister_netdevice: "
"waiting for %s to become free. Usage "
"count = %d\n",
- dev->name, atomic_read(&dev->refcnt));
+ dev->name, refcnt);
warning_time = jiffies;
}
}
@@ -5350,7 +5366,7 @@ void netdev_run_todo(void)
netdev_wait_allrefs(dev);
/* paranoia */
- BUG_ON(atomic_read(&dev->refcnt));
+ BUG_ON(netdev_refcnt_read(dev));
WARN_ON(rcu_dereference_raw(dev->ip_ptr));
WARN_ON(dev->ip6_ptr);
WARN_ON(dev->dn_ptr);
@@ -5520,9 +5536,13 @@ struct net_device *alloc_netdev_mq(int sizeof_priv, const char *name,
dev = PTR_ALIGN(p, NETDEV_ALIGN);
dev->padded = (char *)dev - (char *)p;
- if (dev_addr_init(dev))
+ dev->pcpu_refcnt = alloc_percpu(int);
+ if (!dev->pcpu_refcnt)
goto free_tx;
+ if (dev_addr_init(dev))
+ goto free_pcpu;
+
dev_mc_init(dev);
dev_uc_init(dev);
@@ -5553,6 +5573,8 @@ struct net_device *alloc_netdev_mq(int sizeof_priv, const char *name,
free_tx:
kfree(tx);
+free_pcpu:
+ free_percpu(dev->pcpu_refcnt);
free_p:
kfree(p);
return NULL;
@@ -5586,6 +5608,9 @@ void free_netdev(struct net_device *dev)
list_for_each_entry_safe(p, n, &dev->napi_list, dev_list)
netif_napi_del(p);
+ free_percpu(dev->pcpu_refcnt);
+ dev->pcpu_refcnt = NULL;
+
/* Compatibility with error handling in drivers */
if (dev->reg_state == NETREG_UNINITIALIZED) {
kfree((char *)dev - dev->padded);
^ permalink raw reply related
* Re: [PATCH] net: clear heap allocations for privileged ethtool actions
From: David Miller @ 2010-10-11 19:24 UTC (permalink / raw)
To: bhutchings
Cc: kees.cook, linux-kernel, jgarzik, jeffrey.t.kirsher,
peter.p.waskiewicz.jr, netdev
In-Reply-To: <1286487284.2271.37.camel@achroite.uk.solarflarecom.com>
From: Ben Hutchings <bhutchings@solarflare.com>
Date: Thu, 07 Oct 2010 22:34:44 +0100
> On Thu, 2010-10-07 at 14:10 -0700, Kees Cook wrote:
>> Several other ethtool functions leave heap uncleared (potentially) by
>> drivers. Some interfaces appear safe (eeprom, etc), in that the sizes
>> are well controlled. In some situations (e.g. unchecked error conditions),
>> the heap will remain unchanged in areas before copying back to userspace.
>> Note that these are less of an issue since these all require CAP_NET_ADMIN.
>>
>> Cc: stable@kernel.org
>> Signed-off-by: Kees Cook <kees.cook@canonical.com>
...
> Acked-by: Ben Hutchings <bhutchings@solarflare.com>
So I've applied Kees's patch to net-2.6, and I'll merge net-2.6
into net-next-2.6 so I can resolve the vmalloc/kzalloc merge
conflict before Stephen Rothwell and others have to deal with it
in -next.
Thanks!
^ permalink raw reply
* Re: [PATCH net-next] neigh: speedup neigh_resolve_output()
From: David Miller @ 2010-10-11 19:16 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev
In-Reply-To: <1286484247.3745.91.camel@edumazet-laptop>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 07 Oct 2010 22:44:07 +0200
> Le jeudi 07 octobre 2010 à 14:53 +0200, Eric Dumazet a écrit :
>
>> Further improvements would need to use a seqlock instead of an rwlock to
>> protect neigh->ha[], to not dirty neigh too often and remove two atomic
>> ops.
>>
>
> I implemented this idea in following patch, on top on previous one.
>
> [PATCH net-next] neigh: speedup neigh_resolve_output()
So Eric do you think this is more efficient than the idea I
proposed, which is to "cmpxchg 'hh' and RCU"?
If you think this seqlock thing is faster or more desirable
for some reason, I'll add it.
Thanks!
^ permalink raw reply
* Re: [PATCH net-next] net: percpu net_device refcount
From: David Miller @ 2010-10-11 19:13 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev
In-Reply-To: <1286471555.2912.291.camel@edumazet-laptop>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 07 Oct 2010 19:12:35 +0200
> We tried very hard to remove all possible dev_hold()/dev_put() pairs in
> network stack, using RCU conversions.
>
> There is still an unavoidable device refcount change for every dst we
> create/destroy, and this can slow down some workloads (routers or some
> app servers)
>
> We can switch to a percpu refcount implementation, now dynamic per_cpu
> infrastructure is mature. On a 64 cpus machine, this consumes 256 bytes
> per device.
...
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Ok, I'm fine with this.
But could you please get rid of that "#if 0" code block? The comment
is fine and should stay, but the commented out code shouldn't really
stay there.
Thanks!
^ permalink raw reply
* Re: [PATCH] atl1c: Add support for Atheros AR8152 and AR8152
From: David Miller @ 2010-10-11 19:02 UTC (permalink / raw)
To: shemminger; +Cc: lrodriguez, ben, Luis.Rodriguez, netdev, Jie.Yang, linux-team
In-Reply-To: <20101011120128.2cd460c9@nehalam>
From: Stephen Hemminger <shemminger@vyatta.com>
Date: Mon, 11 Oct 2010 12:01:28 -0700
> On Mon, 11 Oct 2010 11:48:35 -0700
> "Luis R. Rodriguez" <lrodriguez@atheros.com> wrote:
>> @@ -132,7 +132,7 @@ static int atl1c_get_permanent_address(struct atl1c_hw *hw)
>> return -1;
>> }
>> /* Disable OTP_CLK */
>> - if ((hw->nic_type == athr_l1c || hw->nic_type == athr_l2c)) {
>> + if ((hw->nic_type == athr_l1c || hw->nic_type == athr_l2c_b)) {
>> otp_ctrl_data &= ~OTP_CTRL_CLK_EN;
>> AT_WRITE_REG(hw, REG_OTP_CTRL, otp_ctrl_data);
>> msleep(1);
>>
>
> Did you notice ((gratuitous extra parens))
Yeah let's kill that too while we're changing this.
^ permalink raw reply
* Re: [PATCH] atl1c: Add support for Atheros AR8152 and AR8152
From: Stephen Hemminger @ 2010-10-11 19:01 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: David Miller, ben@decadent.org.uk, Luis Rodriguez,
netdev@vger.kernel.org, Jie Yang, linux-team
In-Reply-To: <20101011184835.GA10049@tux>
On Mon, 11 Oct 2010 11:48:35 -0700
"Luis R. Rodriguez" <lrodriguez@atheros.com> wrote:
> On Sun, Oct 10, 2010 at 09:03:04PM -0700, David Miller wrote:
> > From: Ben Hutchings <ben@decadent.org.uk>
> > Date: Mon, 11 Oct 2010 02:18:50 +0100
> >
> > > Your commit 496c185c9495629ef1c65387cb2594578393cfe0 "atl1c: Add support
> > > for Atheros AR8152 and AR8152" included the following changes:
> > ...
> > >> + if (hw->nic_type == athr_l1c || hw->nic_type == athr_l2c_b) {
> > ...
> > >> + if ((hw->nic_type == athr_l1c || hw->nic_type == athr_l2c)) {
> > ...
> > > Shouldn't the first if-statement use the same condition as the second
> > > i.e. matching the previously-defined hardware types athr_l1c and
> > > athr_l2c?
> >
> > Yeah that definitely looks like a bug to me.
>
> Good catch, unfortunatley I don't have the source code I used to port
> this work the day I did this anymore locally, so adding
> Jie Yang who is actually our maintainer for this driver.
>
> Jie, can you please confirm if this patch is correct?
>
> diff --git a/drivers/net/atl1c/atl1c_hw.c b/drivers/net/atl1c/atl1c_hw.c
> index d8501f0..0a7b786 100644
> --- a/drivers/net/atl1c/atl1c_hw.c
> +++ b/drivers/net/atl1c/atl1c_hw.c
> @@ -132,7 +132,7 @@ static int atl1c_get_permanent_address(struct atl1c_hw *hw)
> return -1;
> }
> /* Disable OTP_CLK */
> - if ((hw->nic_type == athr_l1c || hw->nic_type == athr_l2c)) {
> + if ((hw->nic_type == athr_l1c || hw->nic_type == athr_l2c_b)) {
> otp_ctrl_data &= ~OTP_CTRL_CLK_EN;
> AT_WRITE_REG(hw, REG_OTP_CTRL, otp_ctrl_data);
> msleep(1);
>
Did you notice ((gratuitous extra parens))
--
^ permalink raw reply
* Re: [PATCH] atl1c: Add support for Atheros AR8152 and AR8152
From: Luis R. Rodriguez @ 2010-10-11 18:48 UTC (permalink / raw)
To: David Miller
Cc: ben@decadent.org.uk, Luis Rodriguez, netdev@vger.kernel.org,
Jie Yang, linux-team
In-Reply-To: <20101010.210304.71107535.davem@davemloft.net>
On Sun, Oct 10, 2010 at 09:03:04PM -0700, David Miller wrote:
> From: Ben Hutchings <ben@decadent.org.uk>
> Date: Mon, 11 Oct 2010 02:18:50 +0100
>
> > Your commit 496c185c9495629ef1c65387cb2594578393cfe0 "atl1c: Add support
> > for Atheros AR8152 and AR8152" included the following changes:
> ...
> >> + if (hw->nic_type == athr_l1c || hw->nic_type == athr_l2c_b) {
> ...
> >> + if ((hw->nic_type == athr_l1c || hw->nic_type == athr_l2c)) {
> ...
> > Shouldn't the first if-statement use the same condition as the second
> > i.e. matching the previously-defined hardware types athr_l1c and
> > athr_l2c?
>
> Yeah that definitely looks like a bug to me.
Good catch, unfortunatley I don't have the source code I used to port
this work the day I did this anymore locally, so adding
Jie Yang who is actually our maintainer for this driver.
Jie, can you please confirm if this patch is correct?
diff --git a/drivers/net/atl1c/atl1c_hw.c b/drivers/net/atl1c/atl1c_hw.c
index d8501f0..0a7b786 100644
--- a/drivers/net/atl1c/atl1c_hw.c
+++ b/drivers/net/atl1c/atl1c_hw.c
@@ -132,7 +132,7 @@ static int atl1c_get_permanent_address(struct atl1c_hw *hw)
return -1;
}
/* Disable OTP_CLK */
- if ((hw->nic_type == athr_l1c || hw->nic_type == athr_l2c)) {
+ if ((hw->nic_type == athr_l1c || hw->nic_type == athr_l2c_b)) {
otp_ctrl_data &= ~OTP_CTRL_CLK_EN;
AT_WRITE_REG(hw, REG_OTP_CTRL, otp_ctrl_data);
msleep(1);
Luis
^ permalink raw reply related
* Re: NFS & atl1c : "RPC: multiple fragments per record not supported"
From: J. Bruce Fields @ 2010-10-11 18:16 UTC (permalink / raw)
To: Phil Endecott; +Cc: Linux Kernel Mailing List, Linux NFS Mailing List, netdev
In-Reply-To: <1286722097803@dmwebmail.dmwebmail.chezphil.org>
On Sun, Oct 10, 2010 at 03:48:17PM +0100, Phil Endecott wrote:
> Dear Experts,
>
> I am seeing the error "RPC: multiple fragments per record not
> supported" on my NFS server when an NFS client with an atl1c network
> driver talks to it.
>
> The server is a QNAP TS119 ARM box running Debian's 2.6.33.2 kernel.
> It works reliably with other clients.
>
> The client is a new x86 system with an "Atheros Communications
> AR8131 Gigabit Ethernet (rev c0)" (1969:1063). The kernel is
> Debian's 2.6.32-5-686 and the driver seems to be atl1c.
To my knowledge the Linux client has never sent packets that would
trigger the prink above, so off hand it does sound like some sort of
corruption at the network level.
(Independently of that: we should fix the server to support multiple
fragments per record at some point. But if you hadn't hit that printk,
I'm guessing you would have had a failure soon enough anyway.)
> Typically NFS works for a few seconds and then stops, with that
> message repeated on the server. Other network activity seems
> reliable (e.g. HTTP, ssh, etc.)
>
> If I use a USB-ethernet adaptor instead of the built-in gigabit it
> works reliably. (The USB device is not gigabit, but I do still see
> the problems if I limit the port to 100 Mbit on the switch.)
>
> I see the problem with NFS v3 and v4. However, I only see it with
> proto=tcp. By changing the NFS protocol to UDP, the problem seems
> to go away [well, it has been working for about 20 minutes now
> without any issues].
>
> Google finds a previous report here:
> http://lkml.org/lkml/2010/1/20/198 ; the suggestion is to turn off
> tcp segmentation offload, but it seems that this is not possible
> with my system:
>
> # ethtool -K eth0 tso off
> Cannot set device tcp segmentation offload settings: Operation not supported
>
> I have looked at the changes to atl1c since 2.6.32 (http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=history;f=drivers/net/atl1c;h=53cd10d07d040b7bec957acb1c69bc7b44897e69;hb=HEAD)
> and they seem harmless.
>
> I wiresharked the network activity while this error was being shown,
> and it did include some packets with the high-contrast colour
> schemes that wireshark uses for "bad" packets. Unfortunately my
> laptop ran out of battery before I could decipher these packets
> further.
>
> So, is this a known issue? Do people agree that the atl1c driver is
> most likely the culprit? Can I offer any further debugging?
I haven't seen that before. Adding netdev to the cc:, as you seem to
have reasonable evidence that the problem is the network driver.
--b.
^ permalink raw reply
* Re: [PATCH 1/3] NET: pch, fix use after free
From: David Miller @ 2010-10-11 18:13 UTC (permalink / raw)
To: eric.dumazet; +Cc: jslaby, netdev, linux-kernel, jirislaby, masa-korg
In-Reply-To: <1286789918.2737.8.camel@edumazet-laptop>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 11 Oct 2010 11:38:38 +0200
> Le lundi 11 octobre 2010 à 11:26 +0200, Jiri Slaby a écrit :
>> Stanse found that pch_gbe_xmit_frame uses skb after it is freed. Fix
>> that.
>>
>> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
>> Cc: Masayuki Ohtake <masa-korg@dsn.okisemi.com>
>> ---
>> drivers/net/pch_gbe/pch_gbe_main.c | 2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> Applicable to net-next-2.6 only, this driver is not yet in Linus tree
>
> Acked-by: Eric Dumazet <eric.dumazet@gmail.com>
I'll apply this to net-next-2.6, thanks.
^ permalink raw reply
* Re: [PATCH 2/3] ATM: iphase, remove sleep-inside-atomic
From: David Miller @ 2010-10-11 18:13 UTC (permalink / raw)
To: jslaby; +Cc: netdev, linux-kernel, jirislaby, chas
In-Reply-To: <1286789218-13976-2-git-send-email-jslaby@suse.cz>
From: Jiri Slaby <jslaby@suse.cz>
Date: Mon, 11 Oct 2010 11:26:57 +0200
> Stanse found that ia_init_one locks a spinlock and inside of that it
> calls ia_start which calls:
> * request_irq
> * tx_init which does kmalloc(GFP_KERNEL)
>
> Both of them can thus sleep and result in a deadlock. I don't see a
> reason to have a per-device spinlock there which is used only there
> and inited right before the lock location. So remove it completely.
>
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Applied.
^ permalink raw reply
* Re: [PATCH 1/1] ATM: mpc, fix use after free
From: David Miller @ 2010-10-11 18:12 UTC (permalink / raw)
To: eric.dumazet; +Cc: jslaby, netdev, linux-atm-general, linux-kernel, jirislaby
In-Reply-To: <1286787580.2737.4.camel@edumazet-laptop>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 11 Oct 2010 10:59:40 +0200
> Le lundi 11 octobre 2010 à 10:46 +0200, Jiri Slaby a écrit :
>> Stanse found that mpc_push frees skb and then it dereferences it. It
>> is a typo, new_skb should be dereferenced there.
>>
>> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
>> Cc: Eric Dumazet <eric.dumazet@gmail.com>
...
> Acked-by: Eric Dumazet <eric.dumazet@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH 3/3] NET: wimax, fix use after free
From: David Miller @ 2010-10-11 18:12 UTC (permalink / raw)
To: inaky.perez-gonzalez; +Cc: jslaby, netdev, linux-kernel, jirislaby, linux-wimax
In-Reply-To: <1286815606.21592.11.camel@localhost.localdomain>
From: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>
Date: Mon, 11 Oct 2010 09:46:46 -0700
> On Mon, 2010-10-11 at 02:26 -0700, Jiri Slaby wrote:
>> Stanse found that i2400m_rx frees skb, but still uses skb->len even
>> though it has skb_len defined. So use skb_len properly in the code.
>>
>> And also define it unsinged int rather than size_t to solve
>> compilation warnings.
>>
>> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
>
> Ops, fail. Thanks for the catch. I assume you have compile tested it.
>
> Acked-by: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>
Applied.
^ permalink raw reply
* Re: [PATCH 1/1] ATM: solos-pci, remove use after free
From: David Miller @ 2010-10-11 18:12 UTC (permalink / raw)
To: eric.dumazet
Cc: jslaby, netdev, linux-atm-general, linux-kernel, jirislaby, chas
In-Reply-To: <1286785169.2737.3.camel@edumazet-laptop>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 11 Oct 2010 10:19:29 +0200
> Le lundi 11 octobre 2010 à 09:50 +0200, Jiri Slaby a écrit :
>> Stanse found we do in console_show:
>> kfree_skb(skb);
>> return skb->len;
>> which is not good. Fix that by remembering the len and use it in the
>> function instead.
>>
>> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
>> Cc: Chas Williams <chas@cmf.nrl.navy.mil>
>> ---
>
> Acked-by: Eric Dumazet <eric.dumazet@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH net-next 2/5] bnx2x: save cycles in setting gso_size
From: David Miller @ 2010-10-11 17:48 UTC (permalink / raw)
To: vladz; +Cc: bhutchings, dmitry, netdev, eilong
In-Reply-To: <8628FE4E7912BF47A96AE7DD7BAC0AADDDEE428246@SJEXCHCCR02.corp.ad.broadcom.com>
From: "Vladislav Zolotarov" <vladz@broadcom.com>
Date: Mon, 11 Oct 2010 10:06:19 -0700
> Dave, we will respin this patch series.
Ok, thanks guys.
^ permalink raw reply
* Re: [PATCH] af_packet: account for VLAN when checking packet size
From: Phil Sutter @ 2010-10-11 17:29 UTC (permalink / raw)
To: David Miller; +Cc: eric.dumazet, netdev, johann.baudy
In-Reply-To: <20101011.090153.226774563.davem@davemloft.net>
On Mon, Oct 11, 2010 at 09:01:53AM -0700, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Mon, 11 Oct 2010 16:03:02 +0200
>
> > If we dont test ETH_P_8021Q protocol here, we allow sending 1504 bytes
> > frames for MTU=1500
> >
> > Should we really care ?
> >
> > If not, just do
> >
> > reserve = dev->hard_header_len + VLAN_HLEN;
>
> Thats a good point, I think we need to validate the SKB protocol
> field.
Which is set to the value of the passed struct sockaddr_ll field
sll_protocol. At least in the two userspace code samples I have here,
the later field is set to htons(ETH_P_ALL). So unless one changes the
API, the only way to find out the packet type is to actually parse the
given ethernet header.
Since tpacket_rcv() just interprets the vlan_tci skb field, such
detailed packet inspection is otherwise not done in af_packet.c.
Greetings, Phil
^ permalink raw reply
* Re: [patch 1/2] vhost: potential integer overflows
From: Al Viro @ 2010-10-11 17:26 UTC (permalink / raw)
To: Dan Carpenter
Cc: Michael S. Tsirkin, Juan Quintela, David S. Miller, Rusty Russell,
kvm, virtualization, netdev, kernel-janitors
In-Reply-To: <20101011172256.GF5851@bicker>
On Mon, Oct 11, 2010 at 07:22:57PM +0200, Dan Carpenter wrote:
> I did an audit for potential integer overflows of values which get passed
> to access_ok() and here are the results.
FWIW, UINT_MAX is wrong here. What you want is maximal size_t value.
> Signed-off-by: Dan Carpenter <error27@gmail.com>
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index dd3d6f7..c2aa12c 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -429,6 +429,14 @@ static int vq_access_ok(unsigned int num,
> struct vring_avail __user *avail,
> struct vring_used __user *used)
> {
> +
> + if (num > UINT_MAX / sizeof *desc)
> + return 0;
> + if (num > UINT_MAX / sizeof *avail->ring - sizeof *avail)
> + return 0;
> + if (num > UINT_MAX / sizeof *used->ring - sizeof *used)
> + return 0;
> +
> return access_ok(VERIFY_READ, desc, num * sizeof *desc) &&
> access_ok(VERIFY_READ, avail,
> sizeof *avail + num * sizeof *avail->ring) &&
> @@ -447,6 +455,9 @@ int vhost_log_access_ok(struct vhost_dev *dev)
> /* Caller should have vq mutex and device mutex */
> static int vq_log_access_ok(struct vhost_virtqueue *vq, void __user *log_base)
> {
> + if (vq->num > UINT_MAX / sizeof *vq->used->ring - sizeof *vq->used)
> + return 0;
> +
> return vq_memory_access_ok(log_base, vq->dev->memory,
> vhost_has_feature(vq->dev, VHOST_F_LOG_ALL)) &&
> (!vq->log_used || log_access_ok(log_base, vq->log_addr,
> @@ -606,12 +617,17 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp)
> }
>
> /* Also validate log access for used ring if enabled. */
> - if ((a.flags & (0x1 << VHOST_VRING_F_LOG)) &&
> - !log_access_ok(vq->log_base, a.log_guest_addr,
> + if (a.flags & (0x1 << VHOST_VRING_F_LOG)) {
> + if (vq->num > UINT_MAX / sizeof *vq->used->ring - sizeof *vq->used) {
> + r = -EINVAL;
> + break;
> + }
> + if (!log_access_ok(vq->log_base, a.log_guest_addr,
> sizeof *vq->used +
> vq->num * sizeof *vq->used->ring)) {
> - r = -EINVAL;
> - break;
> + r = -EINVAL;
> + break;
> + }
> }
> }
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [patch 2/2] vhost: fix return code for log_access_ok()
From: Dan Carpenter @ 2010-10-11 17:24 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Juan Quintela, David S. Miller, Rusty Russell, kvm,
virtualization, netdev, kernel-janitors
access_ok() returns 1 if it's OK otherwise it should return 0.
Signed-off-by: Dan Carpenter <error27@gmail.com>
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index c2aa12c..f82fe57 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -371,7 +371,7 @@ static int log_access_ok(void __user *log_base, u64 addr, unsigned long sz)
/* Make sure 64 bit math will not overflow. */
if (a > ULONG_MAX - (unsigned long)log_base ||
a + (unsigned long)log_base > ULONG_MAX)
- return -EFAULT;
+ return 0;
return access_ok(VERIFY_WRITE, log_base + a,
(sz + VHOST_PAGE_SIZE * 8 - 1) / VHOST_PAGE_SIZE / 8);
^ permalink raw reply related
* [patch 1/2] vhost: potential integer overflows
From: Dan Carpenter @ 2010-10-11 17:22 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Juan Quintela, David S. Miller, Rusty Russell, kvm,
virtualization, netdev, kernel-janitors
I did an audit for potential integer overflows of values which get passed
to access_ok() and here are the results.
Signed-off-by: Dan Carpenter <error27@gmail.com>
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index dd3d6f7..c2aa12c 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -429,6 +429,14 @@ static int vq_access_ok(unsigned int num,
struct vring_avail __user *avail,
struct vring_used __user *used)
{
+
+ if (num > UINT_MAX / sizeof *desc)
+ return 0;
+ if (num > UINT_MAX / sizeof *avail->ring - sizeof *avail)
+ return 0;
+ if (num > UINT_MAX / sizeof *used->ring - sizeof *used)
+ return 0;
+
return access_ok(VERIFY_READ, desc, num * sizeof *desc) &&
access_ok(VERIFY_READ, avail,
sizeof *avail + num * sizeof *avail->ring) &&
@@ -447,6 +455,9 @@ int vhost_log_access_ok(struct vhost_dev *dev)
/* Caller should have vq mutex and device mutex */
static int vq_log_access_ok(struct vhost_virtqueue *vq, void __user *log_base)
{
+ if (vq->num > UINT_MAX / sizeof *vq->used->ring - sizeof *vq->used)
+ return 0;
+
return vq_memory_access_ok(log_base, vq->dev->memory,
vhost_has_feature(vq->dev, VHOST_F_LOG_ALL)) &&
(!vq->log_used || log_access_ok(log_base, vq->log_addr,
@@ -606,12 +617,17 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp)
}
/* Also validate log access for used ring if enabled. */
- if ((a.flags & (0x1 << VHOST_VRING_F_LOG)) &&
- !log_access_ok(vq->log_base, a.log_guest_addr,
+ if (a.flags & (0x1 << VHOST_VRING_F_LOG)) {
+ if (vq->num > UINT_MAX / sizeof *vq->used->ring - sizeof *vq->used) {
+ r = -EINVAL;
+ break;
+ }
+ if (!log_access_ok(vq->log_base, a.log_guest_addr,
sizeof *vq->used +
vq->num * sizeof *vq->used->ring)) {
- r = -EINVAL;
- break;
+ r = -EINVAL;
+ break;
+ }
}
}
^ permalink raw reply related
* RE: [PATCH net-next 2/5] bnx2x: save cycles in setting gso_size
From: Vladislav Zolotarov @ 2010-10-11 17:06 UTC (permalink / raw)
To: Ben Hutchings
Cc: David Miller, Dmitry Kravkov, netdev@vger.kernel.org,
Eilon Greenstein
In-Reply-To: <1286805633.2349.64.camel@achroite.uk.solarflarecom.com>
> -----Original Message-----
> From: Ben Hutchings [mailto:bhutchings@solarflare.com]
> Sent: Monday, October 11, 2010 4:01 PM
> To: Vladislav Zolotarov
> Cc: David Miller; Dmitry Kravkov; netdev@vger.kernel.org; Eilon
> Greenstein
> Subject: RE: [PATCH net-next 2/5] bnx2x: save cycles in setting
> gso_size
>
> On Mon, 2010-10-11 at 01:53 -0700, Vladislav Zolotarov wrote:
> [...]
> > Dave, it's a gSo_size, not a gRo_size and we are handling an LRO skb
> > here. It's quite confusing, I agree... ;) This is currently the way
> > to mark an LRO skb in order to properly handle the LRO skbs that
> > might still be forwarded to the stack short time after the LRO has
> > been disabled due to enabling the IP forwarding (or if there is a
> > bug in a driver).
> > See the skb_warn_if_lro() below:
> >
> > static inline bool skb_warn_if_lro(const struct sk_buff *skb)
> > {
> > /* LRO sets gso_size but not gso_type, whereas if GSO is really
> > * wanted then gso_type will be set. */
> > struct skb_shared_info *shinfo = skb_shinfo(skb);
> > if (skb_is_nonlinear(skb) && shinfo->gso_size != 0 &&
> > unlikely(shinfo->gso_type == 0)) {
> > __skb_warn_lro_forwarding(skb);
> > return true;
> > }
> > return false;
> > }
> >
> > So, the convention is to set the gSo_size to a none-zero value
> leaving
> > the gSo_type zero for an LRO skbs. It's quite hacky but this is what
> > we have for quite a while and it quite worked so far. ;) To make it
> > cleaner we might have done the following:
> [...]
>
> The requirement (or as you call it, "convention") is to set gso_size to
> the observed MSS of the packets that have been combined. If you don't
> do that then TCP will not know the true number of packets for purposes
> of delayed-ACK etc.
Thanks, Ben. I see that now.
Dave, we will respin this patch series.
Thanks,
vlad
>
> Ben.
>
> --
> Ben Hutchings, Senior Software Engineer, Solarflare Communications
> Not speaking for my employer; that's the marketing department's job.
> They asked us to note that Solarflare product names are trademarked.
>
^ permalink raw reply
* Re: [STABLE 2.6.32 PATCH] net: release dst entry while cache-hot for GSO case too
From: Michael Tokarev @ 2010-10-11 16:55 UTC (permalink / raw)
To: Eric Dumazet; +Cc: avagin, David Miller, avagin, stable, netdev, krkumar2
In-Reply-To: <1286814652.2737.41.camel@edumazet-laptop>
11.10.2010 20:30, Eric Dumazet wrote:
> Le lundi 11 octobre 2010 à 20:19 +0400, Andrew Vagin a écrit :
>> On 10/11/2010 07:59 PM, David Miller wrote:
>>> From: Eric Dumazet<eric.dumazet@gmail.com>
>>> Date: Mon, 11 Oct 2010 17:46:49 +0200
>>>
>>>> This patch was an optimization, not a bug fix.
>>> Right, this has no business going into 2.6.32-stable at all.
>> This is bug fix. Now nobody drops dst in case gso and veth, because the
>> commit 60df914e295a21a223e43a7ee01e0c73c64dd111 deletes skb_dst_drop
>> from the veth.c. We should commit my patch or revert commit 60df914e.
>>
>> We have two bug reports:
>>
>> http://www.spinics.net/lists/netdev/msg142104.html
This is korg#17251: https://bugzilla.kernel.org/show_bug.cgi?id=17251 ,
from me.
But I really wonder how that thing does not happen anymore
when I disabled netfilter hooks... I can't experiment till
weekend, but I'll try to get back to it and re-verify again,
with and without this fix, with and without netfilter hooks.
What I know for sure is 2 facts: I can't trigger the problem
now (with the hooks disabled), and I can't trigger it on a
subsequent kernel releases - e.g. 2.6.35 does not have the
issue, but 2.6.32 has.
>> http://bugzilla.openvz.org/show_bug.cgi?id=1634
Thanks!
/mjt
^ permalink raw reply
* Re: [PATCH 3/3] NET: wimax, fix use after free
From: Inaky Perez-Gonzalez @ 2010-10-11 16:46 UTC (permalink / raw)
To: Jiri Slaby
Cc: davem@davemloft.net, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, jirislaby@gmail.com, linux-wimax
In-Reply-To: <1286789218-13976-3-git-send-email-jslaby@suse.cz>
On Mon, 2010-10-11 at 02:26 -0700, Jiri Slaby wrote:
> Stanse found that i2400m_rx frees skb, but still uses skb->len even
> though it has skb_len defined. So use skb_len properly in the code.
>
> And also define it unsinged int rather than size_t to solve
> compilation warnings.
>
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Ops, fail. Thanks for the catch. I assume you have compile tested it.
Acked-by: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>
> Cc: linux-wimax@intel.com
> ---
> drivers/net/wimax/i2400m/rx.c | 26 +++++++++++++-------------
> 1 files changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/net/wimax/i2400m/rx.c b/drivers/net/wimax/i2400m/rx.c
> index c4876d0..844133b 100644
> --- a/drivers/net/wimax/i2400m/rx.c
> +++ b/drivers/net/wimax/i2400m/rx.c
> @@ -1244,16 +1244,16 @@ int i2400m_rx(struct i2400m *i2400m, struct sk_buff *skb)
> int i, result;
> struct device *dev = i2400m_dev(i2400m);
> const struct i2400m_msg_hdr *msg_hdr;
> - size_t pl_itr, pl_size, skb_len;
> + size_t pl_itr, pl_size;
> unsigned long flags;
> - unsigned num_pls, single_last;
> + unsigned num_pls, single_last, skb_len;
>
> skb_len = skb->len;
> - d_fnstart(4, dev, "(i2400m %p skb %p [size %zu])\n",
> + d_fnstart(4, dev, "(i2400m %p skb %p [size %u])\n",
> i2400m, skb, skb_len);
> result = -EIO;
> msg_hdr = (void *) skb->data;
> - result = i2400m_rx_msg_hdr_check(i2400m, msg_hdr, skb->len);
> + result = i2400m_rx_msg_hdr_check(i2400m, msg_hdr, skb_len);
> if (result < 0)
> goto error_msg_hdr_check;
> result = -EIO;
> @@ -1261,10 +1261,10 @@ int i2400m_rx(struct i2400m *i2400m, struct sk_buff *skb)
> pl_itr = sizeof(*msg_hdr) + /* Check payload descriptor(s) */
> num_pls * sizeof(msg_hdr->pld[0]);
> pl_itr = ALIGN(pl_itr, I2400M_PL_ALIGN);
> - if (pl_itr > skb->len) { /* got all the payload descriptors? */
> + if (pl_itr > skb_len) { /* got all the payload descriptors? */
> dev_err(dev, "RX: HW BUG? message too short (%u bytes) for "
> "%u payload descriptors (%zu each, total %zu)\n",
> - skb->len, num_pls, sizeof(msg_hdr->pld[0]), pl_itr);
> + skb_len, num_pls, sizeof(msg_hdr->pld[0]), pl_itr);
> goto error_pl_descr_short;
> }
> /* Walk each payload payload--check we really got it */
> @@ -1272,7 +1272,7 @@ int i2400m_rx(struct i2400m *i2400m, struct sk_buff *skb)
> /* work around old gcc warnings */
> pl_size = i2400m_pld_size(&msg_hdr->pld[i]);
> result = i2400m_rx_pl_descr_check(i2400m, &msg_hdr->pld[i],
> - pl_itr, skb->len);
> + pl_itr, skb_len);
> if (result < 0)
> goto error_pl_descr_check;
> single_last = num_pls == 1 || i == num_pls - 1;
> @@ -1290,16 +1290,16 @@ int i2400m_rx(struct i2400m *i2400m, struct sk_buff *skb)
> if (i < i2400m->rx_pl_min)
> i2400m->rx_pl_min = i;
> i2400m->rx_num++;
> - i2400m->rx_size_acc += skb->len;
> - if (skb->len < i2400m->rx_size_min)
> - i2400m->rx_size_min = skb->len;
> - if (skb->len > i2400m->rx_size_max)
> - i2400m->rx_size_max = skb->len;
> + i2400m->rx_size_acc += skb_len;
> + if (skb_len < i2400m->rx_size_min)
> + i2400m->rx_size_min = skb_len;
> + if (skb_len > i2400m->rx_size_max)
> + i2400m->rx_size_max = skb_len;
> spin_unlock_irqrestore(&i2400m->rx_lock, flags);
> error_pl_descr_check:
> error_pl_descr_short:
> error_msg_hdr_check:
> - d_fnend(4, dev, "(i2400m %p skb %p [size %zu]) = %d\n",
> + d_fnend(4, dev, "(i2400m %p skb %p [size %u]) = %d\n",
> i2400m, skb, skb_len, result);
> return result;
> }
^ permalink raw reply
* Re: [STABLE 2.6.32 PATCH] net: release dst entry while cache-hot for GSO case too
From: Eric Dumazet @ 2010-10-11 16:30 UTC (permalink / raw)
To: avagin; +Cc: David Miller, mjt, avagin, stable, netdev, krkumar2
In-Reply-To: <4CB33907.5060803@gmail.com>
Le lundi 11 octobre 2010 à 20:19 +0400, Andrew Vagin a écrit :
> On 10/11/2010 07:59 PM, David Miller wrote:
> > From: Eric Dumazet<eric.dumazet@gmail.com>
> > Date: Mon, 11 Oct 2010 17:46:49 +0200
> >
> >> This patch was an optimization, not a bug fix.
> > Right, this has no business going into 2.6.32-stable at all.
> This is bug fix. Now nobody drops dst in case gso and veth, because the
> commit 60df914e295a21a223e43a7ee01e0c73c64dd111 deletes skb_dst_drop
> from the veth.c. We should commit my patch or revert commit 60df914e.
>
> We have two bug reports:
>
> http://www.spinics.net/lists/netdev/msg142104.html
>
> http://bugzilla.openvz.org/show_bug.cgi?id=1634
>
> Taylan verified that the patch really fix his bug.
Now that makes sense ;)
This is always good to mention commit id of a bug origin.
Because reading your patch (changelog and content), it was not obvious
it fixed a bug.
Thanks !
^ permalink raw reply
* Re: [STABLE 2.6.32 PATCH] net: release dst entry while cache-hot for GSO case too
From: Andrew Vagin @ 2010-10-11 16:19 UTC (permalink / raw)
To: David Miller; +Cc: eric.dumazet, mjt, avagin, stable, netdev, krkumar2
In-Reply-To: <20101011.085952.193718228.davem@davemloft.net>
On 10/11/2010 07:59 PM, David Miller wrote:
> From: Eric Dumazet<eric.dumazet@gmail.com>
> Date: Mon, 11 Oct 2010 17:46:49 +0200
>
>> This patch was an optimization, not a bug fix.
> Right, this has no business going into 2.6.32-stable at all.
This is bug fix. Now nobody drops dst in case gso and veth, because the
commit 60df914e295a21a223e43a7ee01e0c73c64dd111 deletes skb_dst_drop
from the veth.c. We should commit my patch or revert commit 60df914e.
We have two bug reports:
http://www.spinics.net/lists/netdev/msg142104.html
http://bugzilla.openvz.org/show_bug.cgi?id=1634
Taylan verified that the patch really fix his bug.
^ 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