* [PATCH 1/3] NET: pch, fix use after free
From: Jiri Slaby @ 2010-10-11 9:26 UTC (permalink / raw)
To: davem; +Cc: netdev, linux-kernel, jirislaby, Masayuki Ohtake
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(-)
diff --git a/drivers/net/pch_gbe/pch_gbe_main.c b/drivers/net/pch_gbe/pch_gbe_main.c
index 53c56cf..e44644f 100644
--- a/drivers/net/pch_gbe/pch_gbe_main.c
+++ b/drivers/net/pch_gbe/pch_gbe_main.c
@@ -1847,9 +1847,9 @@ static int pch_gbe_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
unsigned long flags;
if (unlikely(skb->len > (adapter->hw.mac.max_frame_size - 4))) {
- dev_kfree_skb_any(skb);
pr_err("Transfer length Error: skb len: %d > max: %d\n",
skb->len, adapter->hw.mac.max_frame_size);
+ dev_kfree_skb_any(skb);
adapter->stats.tx_length_errors++;
return NETDEV_TX_OK;
}
--
1.7.3.1
^ permalink raw reply related
* Re: [PATCH 1/1] ATM: mpc, fix use after free
From: Eric Dumazet @ 2010-10-11 8:59 UTC (permalink / raw)
To: Jiri Slaby; +Cc: davem, netdev, linux-atm-general, linux-kernel, jirislaby
In-Reply-To: <1286786794-10410-1-git-send-email-jslaby@suse.cz>
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>
> ---
> net/atm/mpc.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/net/atm/mpc.c b/net/atm/mpc.c
> index 622b471..74bcc66 100644
> --- a/net/atm/mpc.c
> +++ b/net/atm/mpc.c
> @@ -778,7 +778,7 @@ static void mpc_push(struct atm_vcc *vcc, struct sk_buff *skb)
> eg->packets_rcvd++;
> mpc->eg_ops->put(eg);
>
> - memset(ATM_SKB(skb), 0, sizeof(struct atm_skb_data));
> + memset(ATM_SKB(new_skb), 0, sizeof(struct atm_skb_data));
> netif_rx(new_skb);
> }
>
Acked-by: Eric Dumazet <eric.dumazet@gmail.com>
^ permalink raw reply
* RE: [PATCH net-next 2/5] bnx2x: save cycles in setting gso_size
From: Vladislav Zolotarov @ 2010-10-11 8:53 UTC (permalink / raw)
To: David Miller, Dmitry Kravkov; +Cc: netdev@vger.kernel.org, Eilon Greenstein
In-Reply-To: <20101010.205254.193716921.davem@davemloft.net>
> -----Original Message-----
> From: David Miller [mailto:davem@davemloft.net]
> Sent: Monday, October 11, 2010 5:53 AM
> To: Dmitry Kravkov
> Cc: netdev@vger.kernel.org; Vladislav Zolotarov; Eilon Greenstein
> Subject: Re: [PATCH net-next 2/5] bnx2x: save cycles in setting
> gso_size
>
> From: "Dmitry Kravkov" <dmitry@broadcom.com>
> Date: Mon, 11 Oct 2010 00:27:55 +0200
>
> > diff --git a/drivers/net/bnx2x/bnx2x_cmn.c
> b/drivers/net/bnx2x/bnx2x_cmn.c
> > index cb2a3d6..ddf90e1 100644
> > --- a/drivers/net/bnx2x/bnx2x_cmn.c
> > +++ b/drivers/net/bnx2x/bnx2x_cmn.c
> > @@ -277,8 +277,7 @@ static int bnx2x_fill_frag_skb(struct bnx2x *bp,
> struct bnx2x_fastpath *fp,
> >
> > /* This is needed in order to enable forwarding support */
> > if (frag_size)
> > - skb_shinfo(skb)->gso_size = min((u32)SGE_PAGE_SIZE,
> > - max(frag_size, (u32)len_on_bd));
> > + skb_shinfo(skb)->gso_size = 1;
> >
>
> I wonder why you need to set this here.
>
> When you pass this packet into the stack, dev_gro_receive() is going
> to set ->gro_size() unconditionally if any GRO processing is going to
> occur at all.
>
> Why do you need to set it at all?
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:
1) added a new member to the SKB_GSO_XXX family enum since we are riding
on the gso_type anyway. In that case there should have been the following
line in the LRO flow:
if (frag_size)
skb_shinfo(skb)->gso_type = SKB_LSO_TYPE;
and skb_warn_if_lro() should have been reworked appropriately.
2) Change the name of the gso_type field to something like skb_type
and this would make the LRO flow crystal clear then but would require
changes in quite a few places.
So, the question is do we want to start all this? ;)
Regarding the current patch itself: we wanted to save the extra
(not needed at all) calculations in the LRO bnx2x flow considering that
current semantics requires the gso_size to be none zero for the LRO skb.
>
> And if we do need it, doesn't every driver that builds fragmented SKBs?
I guess the answer is "yes".
> And if it's correct, why is setting a don't care value like "1" ok and
> will not cause problems to whoever cares about this value?
I disagree with your definition of "don't care" - it's quite "care"... ;)
I guess I've answered this question above but the bottom line would
be: because it's the current semantics to mark the LRO skb and it's
done exactly for reason that there is somebody out there who cares... ;)
Thanks,
vlad
>
> I really want all of these questions answered, and at least lightly
> explained in the commit message before I apply this patch.
>
> Thanks.
^ permalink raw reply
* [PATCH 1/1] ATM: mpc, fix use after free
From: Jiri Slaby @ 2010-10-11 8:46 UTC (permalink / raw)
To: davem; +Cc: netdev, linux-atm-general, linux-kernel, jirislaby, Eric Dumazet
In-Reply-To: <1286785081.2737.2.camel@edumazet-laptop>
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>
---
net/atm/mpc.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/net/atm/mpc.c b/net/atm/mpc.c
index 622b471..74bcc66 100644
--- a/net/atm/mpc.c
+++ b/net/atm/mpc.c
@@ -778,7 +778,7 @@ static void mpc_push(struct atm_vcc *vcc, struct sk_buff *skb)
eg->packets_rcvd++;
mpc->eg_ops->put(eg);
- memset(ATM_SKB(skb), 0, sizeof(struct atm_skb_data));
+ memset(ATM_SKB(new_skb), 0, sizeof(struct atm_skb_data));
netif_rx(new_skb);
}
--
1.7.3.1
^ permalink raw reply related
* Re: [PATCH 4/4] Phonet: mark the pipe controller as EXPERIMENTAL
From: Rémi Denis-Courmont @ 2010-10-11 8:24 UTC (permalink / raw)
To: Kumar SANGHVI; +Cc: netdev
In-Reply-To: <20101011070928.GC31124@bnru01.bnr.st.com>
On Mon, 11 Oct 2010 12:39:29 +0530, Kumar SANGHVI
<kumar.sanghvi@stericsson.com> wrote:
> I have already started implementing 'connect()' socket call for Pipe
> controller logic, as per advise of Rémi Denis-Courmontt. I will upload
> this patch for review once I have validated GPRS/3G path (control/data)
> to be working fine.
>
> Hopefully, with all the reviews and suggestions shared on the mailing
> list, existing abuse can be removed and new abuses can be avoided.
>
> The only intention of submitting is to make Linux work smoothly with
> newer devices. If there is some improper code getting introduced
> unknowingly, then I will surely work on the review comments to fix it.
I am sure everybody wants to get things right eventually. That's not much
of a concern to me.
However, I am concerned that Linux 2.6.37 ships with an unfinished
implementation, or worse, an unfinished user-space interface.
--
Rémi Denis-Courmont
http://www.remlab.net
http://fi.linkedin.com/in/remidenis
^ permalink raw reply
* Re: [PATCH 1/1] ATM: solos-pci, remove use after free
From: Eric Dumazet @ 2010-10-11 8:19 UTC (permalink / raw)
To: Jiri Slaby
Cc: davem, netdev, linux-atm-general, linux-kernel, jirislaby,
Chas Williams
In-Reply-To: <1286783444-7719-1-git-send-email-jslaby@suse.cz>
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>
^ permalink raw reply
* Re: [HELP] ATM: mpc, use-after-free
From: Eric Dumazet @ 2010-10-11 8:18 UTC (permalink / raw)
To: Jiri Slaby; +Cc: David S. Miller, ML netdev, linux-atm-general, LKML, chas
In-Reply-To: <4CB2C33C.8080109@gmail.com>
Le lundi 11 octobre 2010 à 09:56 +0200, Jiri Slaby a écrit :
> Hi,
>
> Stanse found this use-after-free:
>
> static void mpc_push(struct atm_vcc *vcc, struct sk_buff *skb)
> {
> ...
> new_skb = skb_realloc_headroom(skb, eg->ctrl_info.DH_length);
>
> dev_kfree_skb_any(skb);
>
> FREE ^^^^^^^^^^^^^^^^^^^^^^^
>
> if (new_skb == NULL) {
> mpc->eg_ops->put(eg);
> return;
> }
> skb_push(new_skb, eg->ctrl_info.DH_length);
> skb_copy_to_linear_data(new_skb, eg->ctrl_info.DLL_header,
> eg->ctrl_info.DH_length);
> ...
> memset(ATM_SKB(skb), 0, sizeof(struct atm_skb_data));
>
> USE ^^^^^^^^^^^^
>
> netif_rx(new_skb);
>
> I guess it should be ATM_SKB(new_skb), right?
Yes
- memset(ATM_SKB(skb), 0, sizeof(struct atm_skb_data));
+ memset(ATM_SKB(new_skb), 0, sizeof(struct atm_skb_data));
>
> The two problems are:
> 1) obvious use-after-free
> 2) ?data leak, since we don't erase the right memory?
>
> thanks,
Indeed, please submit a formal patch ?
Thanks
^ permalink raw reply
* [HELP] ATM: mpc, use-after-free
From: Jiri Slaby @ 2010-10-11 7:56 UTC (permalink / raw)
To: David S. Miller; +Cc: ML netdev, linux-atm-general, LKML, chas
Hi,
Stanse found this use-after-free:
static void mpc_push(struct atm_vcc *vcc, struct sk_buff *skb)
{
...
new_skb = skb_realloc_headroom(skb, eg->ctrl_info.DH_length);
dev_kfree_skb_any(skb);
FREE ^^^^^^^^^^^^^^^^^^^^^^^
if (new_skb == NULL) {
mpc->eg_ops->put(eg);
return;
}
skb_push(new_skb, eg->ctrl_info.DH_length);
skb_copy_to_linear_data(new_skb, eg->ctrl_info.DLL_header,
eg->ctrl_info.DH_length);
...
memset(ATM_SKB(skb), 0, sizeof(struct atm_skb_data));
USE ^^^^^^^^^^^^
netif_rx(new_skb);
I guess it should be ATM_SKB(new_skb), right?
The two problems are:
1) obvious use-after-free
2) ?data leak, since we don't erase the right memory?
thanks,
--
js
^ permalink raw reply
* Re: [PATCH] net/fec: carrier off initially to avoid root mount failure
From: Oskar Schirmer @ 2010-10-11 7:54 UTC (permalink / raw)
To: David Miller; +Cc: oskar, netdev, dan, bigeasy, hjk, gerg, bhutchings
In-Reply-To: <20101010.211956.112597497.davem@davemloft.net>
On Sun, Oct 10, 2010 at 21:19:56 -0700, David Miller wrote:
> From: Oskar Schirmer <oskar@linutronix.de>
> Date: Thu, 7 Oct 2010 14:30:30 +0200
>
> > with hardware slow in negotiation, the system did freeze
> > while trying to mount root on nfs at boot time.
> >
> > the link state has not been initialised so network stack
> > tried to start transmission right away. this caused instant
> > retries, as the driver solely stated business upon link down,
> > rendering the system unusable.
> >
> > notify carrier off initially to prevent transmission until
> > phylib will report link up.
> >
> > Signed-off-by: Oskar Schirmer <oskar@linutronix.de>
>
> I did some more investigation into this situation, and for now I'm
> going to apply your patch. It is correct, and it also matches what
> the only other seemingly correct driver I could find using phylib does
> (gianfar) :-) Actually, although I didn't check, bi-modal drivers
> (those that only use phylib for some phy types) like tg3 probably do
> the right thing here too.
>
> Longer term I think the right thing to do might be:
>
> 1) Create some notion of "network device has managed carrier"
>
> This could simply be a flag bit in the netdev or netdev_ops,
> or some other kind of attribute.
>
> 2) Managed carrier devices start with netif_carrier_off(), otherwise
> the device starts with netif_carrier_on().
This last conditional (managed vs otherwise) would be implicit
with a null PHY driver as Ben Hutchings proposes it to Greg Ungerers
"allow FEC driver to not have attached PHY", 2010/10/07,
with the null PHY simply switching to netif_carrier_on right after
machine start.
Otherwise my patch would need another #ifdef to live in
peace with Gregs patch.
^ permalink raw reply
* [PATCH 1/1] ATM: solos-pci, remove use after free
From: Jiri Slaby @ 2010-10-11 7:50 UTC (permalink / raw)
To: davem; +Cc: netdev, linux-atm-general, linux-kernel, jirislaby, Chas Williams
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>
---
drivers/atm/solos-pci.c | 8 +++++---
1 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/atm/solos-pci.c b/drivers/atm/solos-pci.c
index f916ddf..f46138a 100644
--- a/drivers/atm/solos-pci.c
+++ b/drivers/atm/solos-pci.c
@@ -444,6 +444,7 @@ static ssize_t console_show(struct device *dev, struct device_attribute *attr,
struct atm_dev *atmdev = container_of(dev, struct atm_dev, class_dev);
struct solos_card *card = atmdev->dev_data;
struct sk_buff *skb;
+ unsigned int len;
spin_lock(&card->cli_queue_lock);
skb = skb_dequeue(&card->cli_queue[SOLOS_CHAN(atmdev)]);
@@ -451,11 +452,12 @@ static ssize_t console_show(struct device *dev, struct device_attribute *attr,
if(skb == NULL)
return sprintf(buf, "No data.\n");
- memcpy(buf, skb->data, skb->len);
- dev_dbg(&card->dev->dev, "len: %d\n", skb->len);
+ len = skb->len;
+ memcpy(buf, skb->data, len);
+ dev_dbg(&card->dev->dev, "len: %d\n", len);
kfree_skb(skb);
- return skb->len;
+ return len;
}
static int send_command(struct solos_card *card, int dev, const char *buf, size_t size)
--
1.7.3.1
^ permalink raw reply related
* Re: [v2 RFC PATCH 0/4] Implement multiqueue virtio-net
From: Krishna Kumar2 @ 2010-10-11 7:21 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: anthony, arnd, avi, davem, kvm, netdev, rusty
In-Reply-To: <cover.1286372004.git.mst@redhat.com>
"Michael S. Tsirkin" <mst@redhat.com> wrote on 10/06/2010 07:04:31 PM:
> On Fri, Sep 17, 2010 at 03:33:07PM +0530, Krishna Kumar wrote:
> > For 1 TCP netperf, I ran 7 iterations and summed it. Explanation
> > for degradation for 1 stream case:
>
> I thought about possible RX/TX contention reasons, and I realized that
> we get/put the mm counter all the time. So I write the following: I
> haven't seen any performance gain from this in a single queue case, but
> maybe this will help multiqueue?
Sorry for the delay, I was sick last couple of days. The results
with your patch are (%'s over original code):
Code BW% CPU% RemoteCPU
MQ (#txq=16) 31.4% 38.42% 6.41%
MQ+MST (#txq=16) 28.3% 18.9% -10.77%
The patch helps CPU utilization but didn't help single stream
drop.
Thanks,
- KK
^ permalink raw reply
* Re: [PATCH 4/4] Phonet: mark the pipe controller as EXPERIMENTAL
From: Kumar SANGHVI @ 2010-10-11 7:09 UTC (permalink / raw)
To: Rémi Denis-Courmont; +Cc: netdev@vger.kernel.org, Rémi Denis-Courmont
In-Reply-To: <1286546523-3340-4-git-send-email-remi@remlab.net>
On Fri, Oct 08, 2010 at 16:02:03 +0200, Rémi Denis-Courmont wrote:
> From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
>
> There are a bunch of issues that need to be fixed, including:
> - GFP_KERNEL allocations from atomic context
> (and GFP_ATOMIC in process context),
> - abuse of the setsockopt() call convention,
> - unprotected/unlocked static variables...
>
> IMHO, we will need to alter the userspace ABI when we fix it. So mark
> the configuration option as EXPERIMENTAL for the time being (or should
> it be BROKEN instead?).
>
> Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
Thank you for this patch.
I have already started implementing 'connect()' socket call for Pipe
controller logic, as per advise of Rémi Denis-Courmontt. I will upload
this patch for review once I have validated GPRS/3G path (control/data)
to be working fine.
Hopefully, with all the reviews and suggestions shared on the mailing
list, existing abuse can be removed and new abuses can be avoided.
The only intention of submitting is to make Linux work smoothly with
newer devices. If there is some improper code getting introduced
unknowingly, then I will surely work on the review comments to fix it.
Thank you all!
Best regards,
Kumar.
^ permalink raw reply
* RE: [PATCH v12 10/17] Add a hook to intercept external buffers from NIC driver.
From: Xin, Xiaohui @ 2010-10-11 7:06 UTC (permalink / raw)
To: Eric Dumazet
Cc: netdev@vger.kernel.org, kvm@vger.kernel.org,
linux-kernel@vger.kernel.org, mst@redhat.com, mingo@elte.hu,
davem@davemloft.net, herbert@gondor.apana.org.au,
jdike@linux.intel.com
In-Reply-To: <1285856533.2615.573.camel@edumazet-laptop>
>-----Original Message-----
>From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
>Sent: Thursday, September 30, 2010 10:22 PM
>To: Xin, Xiaohui
>Cc: netdev@vger.kernel.org; kvm@vger.kernel.org; linux-kernel@vger.kernel.org;
>mst@redhat.com; mingo@elte.hu; davem@davemloft.net; herbert@gondor.apana.org.au;
>jdike@linux.intel.com
>Subject: Re: [PATCH v12 10/17] Add a hook to intercept external buffers from NIC driver.
>
>Le jeudi 30 septembre 2010 à 22:04 +0800, xiaohui.xin@intel.com a
>écrit :
>> From: Xin Xiaohui <xiaohui.xin@intel.com>
>>
>> The hook is called in netif_receive_skb().
>> Signed-off-by: Xin Xiaohui <xiaohui.xin@intel.com>
>> Signed-off-by: Zhao Yu <yzhao81new@gmail.com>
>> Reviewed-by: Jeff Dike <jdike@linux.intel.com>
>> ---
>> net/core/dev.c | 35 +++++++++++++++++++++++++++++++++++
>> 1 files changed, 35 insertions(+), 0 deletions(-)
>>
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index c11e32c..83172b8 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -2517,6 +2517,37 @@ err:
>> EXPORT_SYMBOL(netdev_mp_port_prep);
>> #endif
>>
>> +#if defined(CONFIG_MEDIATE_PASSTHRU) ||
>defined(CONFIG_MEDIATE_PASSTHRU_MODULE)
>> +/* Add a hook to intercept mediate passthru(zero-copy) packets,
>> + * and insert it to the socket queue owned by mp_port specially.
>> + */
>> +static inline struct sk_buff *handle_mpassthru(struct sk_buff *skb,
>> + struct packet_type **pt_prev,
>> + int *ret,
>> + struct net_device *orig_dev)
>> +{
>> + struct mp_port *mp_port = NULL;
>> + struct sock *sk = NULL;
>> +
>> + if (!dev_is_mpassthru(skb->dev))
>> + return skb;
>> + mp_port = skb->dev->mp_port;
>> +
>> + if (*pt_prev) {
>> + *ret = deliver_skb(skb, *pt_prev, orig_dev);
>> + *pt_prev = NULL;
>> + }
>> +
>> + sk = mp_port->sock->sk;
>> + skb_queue_tail(&sk->sk_receive_queue, skb);
>> + sk->sk_state_change(sk);
>> +
>> + return NULL;
>> +}
>> +#else
>> +#define handle_mpassthru(skb, pt_prev, ret, orig_dev) (skb)
>> +#endif
>> +
>> /**
>> * netif_receive_skb - process receive buffer from network
>> * @skb: buffer to process
>> @@ -2598,6 +2629,10 @@ int netif_receive_skb(struct sk_buff *skb)
>> ncls:
>> #endif
>>
>> + /* To intercept mediate passthru(zero-copy) packets here */
>> + skb = handle_mpassthru(skb, &pt_prev, &ret, orig_dev);
>> + if (!skb)
>> + goto out;
>> skb = handle_bridge(skb, &pt_prev, &ret, orig_dev);
>> if (!skb)
>> goto out;
>
>This does not apply to current net-next-2.6
>
>We now have dev->rx_handler (currently for bridge or macvlan)
>
>
Ok. Thanks, will rebase to fix that.
Thanks
Xiaohui
>commit ab95bfe01f9872459c8678572ccadbf646badad0
>Author: Jiri Pirko <jpirko@redhat.com>
>Date: Tue Jun 1 21:52:08 2010 +0000
>
> net: replace hooks in __netif_receive_skb V5
>
> What this patch does is it removes two receive frame hooks (for bridge and for
> macvlan) from __netif_receive_skb. These are replaced them with a single
> hook for both. It only supports one hook per device because it makes no
> sense to do bridging and macvlan on the same device.
>
> Then a network driver (of virtual netdev like macvlan or bridge) can register
> an rx_handler for needed net device.
>
> Signed-off-by: Jiri Pirko <jpirko@redhat.com>
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> Signed-off-by: David S. Miller <davem@davemloft.net>
>
>
^ permalink raw reply
* RE: [PATCH v12 06/17] Use callback to deal with skb_release_data() specially.
From: Xin, Xiaohui @ 2010-10-11 7:06 UTC (permalink / raw)
To: David Miller
Cc: netdev@vger.kernel.org, kvm@vger.kernel.org,
linux-kernel@vger.kernel.org, mst@redhat.com, mingo@elte.hu,
herbert@gondor.apana.org.au, jdike@linux.intel.com
In-Reply-To: <20101001.001439.85411154.davem@davemloft.net>
>-----Original Message-----
>From: David Miller [mailto:davem@davemloft.net]
>Sent: Friday, October 01, 2010 3:15 PM
>To: Xin, Xiaohui
>Cc: netdev@vger.kernel.org; kvm@vger.kernel.org; linux-kernel@vger.kernel.org;
>mst@redhat.com; mingo@elte.hu; herbert@gondor.apana.org.au; jdike@linux.intel.com
>Subject: Re: [PATCH v12 06/17] Use callback to deal with skb_release_data() specially.
>
>From: xiaohui.xin@intel.com
>Date: Thu, 30 Sep 2010 22:04:23 +0800
>
>> @@ -197,10 +197,11 @@ struct skb_shared_info {
>> union skb_shared_tx tx_flags;
>> struct sk_buff *frag_list;
>> struct skb_shared_hwtstamps hwtstamps;
>> - skb_frag_t frags[MAX_SKB_FRAGS];
>> /* Intermediate layers must ensure that destructor_arg
>> * remains valid until skb destructor */
>> void * destructor_arg;
>> +
>> + skb_frag_t frags[MAX_SKB_FRAGS];
>> };
>>
>> /* The structure is for a skb which pages may point to
>
>Why are you moving frags[] to the end like this?
That's to avoid the new cache miss caused by using destructor_arg in data path
like skb_release_data().
That's based on the comment from Eric Dumazet on v7 patches.
Thanks
Xiaohui
^ permalink raw reply
* Re: [PATCH 3/4] Phonet: cleanup pipe enable socket option
From: Kumar SANGHVI @ 2010-10-11 6:50 UTC (permalink / raw)
To: Rémi Denis-Courmont; +Cc: netdev@vger.kernel.org, Rémi Denis-Courmont
In-Reply-To: <1286546523-3340-3-git-send-email-remi@remlab.net>
Hi,
On Fri, Oct 08, 2010 at 16:02:02 +0200, Rémi Denis-Courmont wrote:
> From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
>
> The current code works like this:
>
> int garbage, status;
> socklen_t len = sizeof(status);
>
> /* enable pipe */
> setsockopt(fd, SOL_PNPIPE, PNPIPE_ENABLE, &garbage, sizeof(garbage));
> /* disable pipe */
> setsockopt(fd, SOL_PNPIPE, PNPIPE_DISABLE, &garbage, sizeof(garbage));
> /* get status */
> getsockopt(fd, SOL_PNPIPE, PNPIPE_INQ, &status, &len);
>
> ...which does not follow the usual socket option pattern. This patch
> merges all three "options" into a single gettable&settable option,
> before Linux 2.6.37 gets out:
>
> int status;
> socklen_t len = sizeof(status);
>
> /* enable pipe */
> status = 1;
> setsockopt(fd, SOL_PNPIPE, PNPIPE_ENABLE, &status, sizeof(status));
> /* disable pipe */
> status = 0;
> setsockopt(fd, SOL_PNPIPE, PNPIPE_ENABLE, &status, sizeof(status));
> /* get status */
> getsockopt(fd, SOL_PNPIPE, PNPIPE_ENABLE, &status, &len);
>
> This also fixes the error code from EFAULT to ENOTCONN.
>
> Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
> Cc: Kumar Sanghvi <kumar.sanghvi@stericsson.com>
Thank you!
BR,
Kumar.
^ permalink raw reply
* Re: [PATCH 2/4] Phonet: advise against enabling the pipe controller
From: Kumar SANGHVI @ 2010-10-11 6:49 UTC (permalink / raw)
To: Rémi Denis-Courmont; +Cc: netdev@vger.kernel.org, Rémi Denis-Courmont
In-Reply-To: <1286546523-3340-2-git-send-email-remi@remlab.net>
Hi,
On Fri, Oct 08, 2010 at 16:02:01 +0200, Rémi Denis-Courmont wrote:
> From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
>
> As it currently is, the new code path is not compatible with existing
> Nokia modems. This would break existing userspace for Nokia modem, such
> as the existing oFono ISI driver.
>
> Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
Yes, the pipe controller logic is not intended for existing Nokia modems
that already implement pipe controller inside them.
Thank you for putting correct warning message.
^ permalink raw reply
* [PATCH net-2.6] tg3: restore rx_dropped accounting
From: Eric Dumazet @ 2010-10-11 5:55 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Matt Carlson, Michael Chan
commit 511d22247be7 (tg3: 64 bit stats on all arches), overlooked the
rx_dropped accounting.
We use a full "struct rtnl_link_stats64" to hold rx_dropped value, but
forgot to report it in tg3_get_stats64().
Use an "unsigned long" instead to shrink "struct tg3" by 176 bytes, and
report this value to stats readers.
Increment rx_dropped counter for oversized frames.
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
CC: Michael Chan <mchan@broadcom.com>
CC: Matt Carlson <mcarlson@broadcom.com>
---
drivers/net/tg3.c | 6 ++++--
drivers/net/tg3.h | 2 +-
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
index bc3af78..1ec4b9e 100644
--- a/drivers/net/tg3.c
+++ b/drivers/net/tg3.c
@@ -4666,7 +4666,7 @@ static int tg3_rx(struct tg3_napi *tnapi, int budget)
desc_idx, *post_ptr);
drop_it_no_recycle:
/* Other statistics kept track of by card. */
- tp->net_stats.rx_dropped++;
+ tp->rx_dropped++;
goto next_pkt;
}
@@ -4726,7 +4726,7 @@ static int tg3_rx(struct tg3_napi *tnapi, int budget)
if (len > (tp->dev->mtu + ETH_HLEN) &&
skb->protocol != htons(ETH_P_8021Q)) {
dev_kfree_skb(skb);
- goto next_pkt;
+ goto drop_it_no_recycle;
}
if (desc->type_flags & RXD_FLAG_VLAN &&
@@ -9240,6 +9240,8 @@ static struct rtnl_link_stats64 *tg3_get_stats64(struct net_device *dev,
stats->rx_missed_errors = old_stats->rx_missed_errors +
get_stat64(&hw_stats->rx_discards);
+ stats->rx_dropped = tp->rx_dropped;
+
return stats;
}
diff --git a/drivers/net/tg3.h b/drivers/net/tg3.h
index 4937bd1..be7ff13 100644
--- a/drivers/net/tg3.h
+++ b/drivers/net/tg3.h
@@ -2759,7 +2759,7 @@ struct tg3 {
/* begin "everything else" cacheline(s) section */
- struct rtnl_link_stats64 net_stats;
+ unsigned long rx_dropped;
struct rtnl_link_stats64 net_stats_prev;
struct tg3_ethtool_stats estats;
struct tg3_ethtool_stats estats_prev;
^ permalink raw reply related
* Re: [PATCH] net/fec: carrier off initially to avoid root mount failure
From: David Miller @ 2010-10-11 4:19 UTC (permalink / raw)
To: oskar; +Cc: netdev, dan, bigeasy, hjk
In-Reply-To: <1286454630-7396-1-git-send-email-oskar@linutronix.de>
From: Oskar Schirmer <oskar@linutronix.de>
Date: Thu, 7 Oct 2010 14:30:30 +0200
> with hardware slow in negotiation, the system did freeze
> while trying to mount root on nfs at boot time.
>
> the link state has not been initialised so network stack
> tried to start transmission right away. this caused instant
> retries, as the driver solely stated business upon link down,
> rendering the system unusable.
>
> notify carrier off initially to prevent transmission until
> phylib will report link up.
>
> Signed-off-by: Oskar Schirmer <oskar@linutronix.de>
I did some more investigation into this situation, and for now I'm
going to apply your patch. It is correct, and it also matches what
the only other seemingly correct driver I could find using phylib does
(gianfar) :-) Actually, although I didn't check, bi-modal drivers
(those that only use phylib for some phy types) like tg3 probably do
the right thing here too.
Longer term I think the right thing to do might be:
1) Create some notion of "network device has managed carrier"
This could simply be a flag bit in the netdev or netdev_ops,
or some other kind of attribute.
2) Managed carrier devices start with netif_carrier_off(), otherwise
the device starts with netif_carrier_on().
Then we gut all of the probe time netif_carrir_*() calls in all
of the drivers.
And hopefully it's less error prone than it is right now.
^ permalink raw reply
* Re: [PATCH] atl1c: Add support for Atheros AR8152 and AR8152
From: David Miller @ 2010-10-11 4:03 UTC (permalink / raw)
To: ben; +Cc: lrodriguez, netdev
In-Reply-To: <1286759930.2955.285.camel@localhost>
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.
^ permalink raw reply
* Re: [GIT PULL net-next-2.6] IrDA 2.6.37 updates
From: David Miller @ 2010-10-11 3:57 UTC (permalink / raw)
To: samuel; +Cc: netdev
In-Reply-To: <cover.1286756123.git.sameo@linux.intel.com>
From: Samuel Ortiz <samuel@sortiz.org>
Date: Mon, 11 Oct 2010 02:24:57 +0200
> I have 5 pending IrDA patches for the next merge window.
> The bulk of it is made of the BKL removal for irda, along with a couple of
> security fixes.
>
> The following changes since commit 7b738b55b2ec0e95a5030037c45b3c312e385789:
>
> sundance: get_stats proper locking (2010-10-09 09:22:54 -0700)
>
> are available in the git repository at:
> git://git.kernel.org/pub/scm/linux/kernel/git/sameo/irda-2.6.git for-davem
Pulled, thanks Sam.
^ permalink raw reply
* Re: [PATCH net-next 2/5] bnx2x: save cycles in setting gso_size
From: David Miller @ 2010-10-11 3:52 UTC (permalink / raw)
To: dmitry; +Cc: netdev, vladz, eilong
In-Reply-To: <1286749675.4669.24.camel@lb-tlvb-dmitry>
From: "Dmitry Kravkov" <dmitry@broadcom.com>
Date: Mon, 11 Oct 2010 00:27:55 +0200
> diff --git a/drivers/net/bnx2x/bnx2x_cmn.c b/drivers/net/bnx2x/bnx2x_cmn.c
> index cb2a3d6..ddf90e1 100644
> --- a/drivers/net/bnx2x/bnx2x_cmn.c
> +++ b/drivers/net/bnx2x/bnx2x_cmn.c
> @@ -277,8 +277,7 @@ static int bnx2x_fill_frag_skb(struct bnx2x *bp, struct bnx2x_fastpath *fp,
>
> /* This is needed in order to enable forwarding support */
> if (frag_size)
> - skb_shinfo(skb)->gso_size = min((u32)SGE_PAGE_SIZE,
> - max(frag_size, (u32)len_on_bd));
> + skb_shinfo(skb)->gso_size = 1;
>
I wonder why you need to set this here.
When you pass this packet into the stack, dev_gro_receive() is going
to set ->gro_size() unconditionally if any GRO processing is going to
occur at all.
Why do you need to set it at all?
And if we do need it, doesn't every driver that builds fragmented SKBs?
And if it's correct, why is setting a don't care value like "1" ok and
will not cause problems to whoever cares about this value?
I really want all of these questions answered, and at least lightly
explained in the commit message before I apply this patch.
Thanks.
^ permalink raw reply
* Re: [PATCH] atl1c: Add support for Atheros AR8152 and AR8152
From: Ben Hutchings @ 2010-10-11 1:18 UTC (permalink / raw)
To: Luis R. Rodriguez; +Cc: netdev
[-- Attachment #1: Type: text/plain, Size: 1669 bytes --]
Your commit 496c185c9495629ef1c65387cb2594578393cfe0 "atl1c: Add support
for Atheros AR8152 and AR8152" included the following changes:
> --- a/drivers/net/atl1c/atl1c_hw.c
> +++ b/drivers/net/atl1c/atl1c_hw.c
> @@ -70,17 +70,39 @@ static int atl1c_get_permanent_address(struct atl1c_hw *hw)
[...]
> - /* Enable OTP CLK */
> - if (!(otp_ctrl_data & OTP_CTRL_CLK_EN)) {
> - otp_ctrl_data |= OTP_CTRL_CLK_EN;
> - AT_WRITE_REG(hw, REG_OTP_CTRL, otp_ctrl_data);
> - AT_WRITE_FLUSH(hw);
> - msleep(1);
> + if (hw->nic_type == athr_l1c || hw->nic_type == athr_l2c_b) {
> + /* Enable OTP CLK */
> + if (!(otp_ctrl_data & OTP_CTRL_CLK_EN)) {
> + otp_ctrl_data |= OTP_CTRL_CLK_EN;
> + AT_WRITE_REG(hw, REG_OTP_CTRL, otp_ctrl_data);
> + AT_WRITE_FLUSH(hw);
> + msleep(1);
> + }
> + }
[...]
> @@ -96,11 +118,31 @@ static int atl1c_get_permanent_address(struct atl1c_hw *hw)
> return -1;
> }
> /* Disable OTP_CLK */
> - if (otp_ctrl_data & OTP_CTRL_CLK_EN) {
> - otp_ctrl_data &= ~OTP_CTRL_CLK_EN;
> - AT_WRITE_REG(hw, REG_OTP_CTRL, otp_ctrl_data);
> - AT_WRITE_FLUSH(hw);
> - msleep(1);
> + if ((hw->nic_type == athr_l1c || hw->nic_type == athr_l2c)) {
> + if (otp_ctrl_data & OTP_CTRL_CLK_EN) {
> + otp_ctrl_data &= ~OTP_CTRL_CLK_EN;
> + AT_WRITE_REG(hw, REG_OTP_CTRL, otp_ctrl_data);
> + AT_WRITE_FLUSH(hw);
> + msleep(1);
> + }
> + }
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?
Ben.
--
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply
* Re: [RFC PATCH net-next] drivers/net Documentation/networking: Create directory intel_wired_lan
From: Joe Perches @ 2010-10-11 0:41 UTC (permalink / raw)
To: Stephen Hemminger
Cc: e1000-devel, Bruce Allan, Jesse Brandeburg, linux-kernel,
Greg Rose, John Ronciak, Jeff Kirsher, netdev, PJ
In-Reply-To: <20101010144440.0d7a69f9@nehalam>
On Sun, 2010-10-10 at 14:44 -0700, Stephen Hemminger wrote:
> On Sun, 10 Oct 2010 13:42:32 -0700
> Joe Perches <joe@perches.com> wrote:
> > Perhaps it's better to move drivers from the very populated
> > drivers/net directory into vendor specific directories similar
> > to the Atheros approach used for drivers/net/wireless/ath/
> > Move intel drivers and Documentation to separate directories
> Seems like it would introduce more confusion and trouble.
It would also allow common code to be consolidated from the
various drivers.
------------------------------------------------------------------------------
Beautiful is writing same markup. Internet Explorer 9 supports
standards for HTML5, CSS3, SVG 1.1, ECMAScript5, and DOM L2 & L3.
Spend less time writing and rewriting code and more time creating great
experiences on the web. Be a part of the beta today.
http://p.sf.net/sfu/beautyoftheweb
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired
^ permalink raw reply
* [PATCH net-next 5/5] irda: Fix heap memory corruption in iriap.c
From: Samuel Ortiz @ 2010-10-11 0:25 UTC (permalink / raw)
To: David S. Miller; +Cc: netdev, Samuel Ortiz, stable
In-Reply-To: <cover.1286756123.git.sameo@linux.intel.com>
While parsing the GetValuebyClass command frame, we could potentially write
passed the skb->data pointer.
Cc: stable@kernel.org
Reported-by: Ilja Van Sprundel <ivansprundel@ioactive.com>
Signed-off-by: Samuel Ortiz <samuel@sortiz.org>
---
net/irda/iriap.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/net/irda/iriap.c b/net/irda/iriap.c
index fce364c..5b743bd 100644
--- a/net/irda/iriap.c
+++ b/net/irda/iriap.c
@@ -502,7 +502,8 @@ static void iriap_getvaluebyclass_confirm(struct iriap_cb *self,
IRDA_DEBUG(4, "%s(), strlen=%d\n", __func__, value_len);
/* Make sure the string is null-terminated */
- fp[n+value_len] = 0x00;
+ if (n + value_len < skb->len)
+ fp[n + value_len] = 0x00;
IRDA_DEBUG(4, "Got string %s\n", fp+n);
/* Will truncate to IAS_MAX_STRING bytes */
--
1.7.1
^ permalink raw reply related
* [PATCH net-next 4/5] irda: Fix parameter extraction stack overflow
From: Samuel Ortiz @ 2010-10-11 0:25 UTC (permalink / raw)
To: David S. Miller; +Cc: netdev, Samuel Ortiz, stable
In-Reply-To: <cover.1286756123.git.sameo@linux.intel.com>
Cc: stable@kernel.org
Reported-by: Ilja Van Sprundel <ivansprundel@ioactive.com>
Signed-off-by: Samuel Ortiz <samuel@sortiz.org>
---
net/irda/parameters.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/net/irda/parameters.c b/net/irda/parameters.c
index fc1a205..71cd38c 100644
--- a/net/irda/parameters.c
+++ b/net/irda/parameters.c
@@ -298,6 +298,8 @@ static int irda_extract_string(void *self, __u8 *buf, int len, __u8 pi,
p.pi = pi; /* In case handler needs to know */
p.pl = buf[1]; /* Extract length of value */
+ if (p.pl > 32)
+ p.pl = 32;
IRDA_DEBUG(2, "%s(), pi=%#x, pl=%d\n", __func__,
p.pi, p.pl);
@@ -318,7 +320,7 @@ static int irda_extract_string(void *self, __u8 *buf, int len, __u8 pi,
(__u8) str[0], (__u8) str[1]);
/* Null terminate string */
- str[p.pl+1] = '\0';
+ str[p.pl] = '\0';
p.pv.c = str; /* Handler will need to take a copy */
--
1.7.1
^ permalink raw reply related
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