* Re: xfrm: Fix initialize repl field of struct xfrm_state
From: David Miller @ 2011-03-21 5:55 UTC (permalink / raw)
To: yjwei; +Cc: netdev, steffen.klassert
In-Reply-To: <4D86E603.8080704@cn.fujitsu.com>
From: Wei Yongjun <yjwei@cn.fujitsu.com>
Date: Mon, 21 Mar 2011 13:45:39 +0800
> Commit 'xfrm: Move IPsec replay detection functions to a separate file'
> (9fdc4883d92d20842c5acea77a4a21bb1574b495)
> introduce repl field to struct xfrm_state, and only initialize it
> under SA's netlink create path, the other path, such as pf_key, the
> repl field remaining uninitialize. So if the SA is created by pf_key,
> any input packet with SA's encryption algorithm will cause panic.
Please, either add an xfrm_init_replay() call to the appropriate spot
in net/key/af_key.c or, if possible, only have the one call in
xfrm_init_state(). Don't leave two calls, one in xfrm_user.c and one
in xfrm_state.c
Anyways, I don't think just making one call from xfrm_init_state() is
possible, because the replay settings need to be assigned before we
can properly call xfrm_init_replay().
Therefore, please fix this by adding the necessary call to af_key.c
Thanks.
^ permalink raw reply
* Re: [PATCH v2 1/1] solos-pci: Fix regression introduced by newest firmware
From: Philip Prindeville @ 2011-03-21 5:56 UTC (permalink / raw)
To: David Miller; +Cc: bhutchings, netdev, linux-atm-general
In-Reply-To: <20110320.215704.193716335.davem@davemloft.net>
On 3/20/11 9:57 PM, David Miller wrote:
> From: Ben Hutchings<bhutchings@solarflare.com>
> Date: Mon, 21 Mar 2011 03:01:36 +0000
>
>> On Sun, 2011-03-20 at 18:52 -0700, Philip Prindeville wrote:
>>> The newest FPGA firmware on the Solos processors correctly signals
>>> carrier transitions, bitrate, etc.
>>>
>>> The driver previously ignored these messages, and the physical state
>>> was always ATM_PHY_SIG_UNKNOWN.
>>>
>>> Now that the board reports its state, we expose a bug whereby the
>>> transition from UNKNOWN to LOST causes us to release all VC's.
>>>
>>> We don't delete any VC's, but instead just send an indication of
>>> carrier change.
>>>
>>> Signed-off-by: Philip A Prindeville<philipp@redfish-solutions.com>
>>> ---
>>>
>>> --- a/drivers/atm/solos-pci.c 2011-03-20 15:27:40.000000000 -0600
>>> +++ b/drivers/atm/solos-pci.c 2011-03-20 16:32:11.000000000 -0600
>>> @@ -382,8 +382,10 @@ static int process_status(struct solos_c
>>>
>>> /* Anything but 'Showtime' is down */
>>> if (strcmp(state_str, "Showtime")) {
>>> atm_dev_signal_change(card->atmdev[port], ATM_PHY_SIG_LOST);
>>> +#if 0
>>> atm_dev_release_vccs(card->atmdev[port]);
>>> +#endif
>> Either remove it or don't. #if 0 is for people without version control.
> Also, this would seem to break those using the older firmware.
It's not clear that dropping all VCs abruptly when carrier flapped was ever the right thing to do.
-Philip
^ permalink raw reply
* Re: [PATCH v2 1/1] solos-pci: Fix regression introduced by newest firmware
From: David Miller @ 2011-03-21 6:04 UTC (permalink / raw)
To: philipp_subx; +Cc: bhutchings, netdev, linux-atm-general
In-Reply-To: <4D86E89B.1060905@redfish-solutions.com>
From: Philip Prindeville <philipp_subx@redfish-solutions.com>
Date: Sun, 20 Mar 2011 22:56:43 -0700
> It's not clear that dropping all VCs abruptly when carrier flapped was
> ever the right thing to do.
So you've tested your change with the older firmware present?
^ permalink raw reply
* Re: xfrm: Fix initialize repl field of struct xfrm_state
From: Wei Yongjun @ 2011-03-21 6:36 UTC (permalink / raw)
To: David Miller; +Cc: netdev, steffen.klassert
In-Reply-To: <20110320.225542.71119753.davem@davemloft.net>
> From: Wei Yongjun <yjwei@cn.fujitsu.com>
> Date: Mon, 21 Mar 2011 13:45:39 +0800
>
>> Commit 'xfrm: Move IPsec replay detection functions to a separate file'
>> (9fdc4883d92d20842c5acea77a4a21bb1574b495)
>> introduce repl field to struct xfrm_state, and only initialize it
>> under SA's netlink create path, the other path, such as pf_key, the
>> repl field remaining uninitialize. So if the SA is created by pf_key,
>> any input packet with SA's encryption algorithm will cause panic.
> Please, either add an xfrm_init_replay() call to the appropriate spot
> in net/key/af_key.c or, if possible, only have the one call in
> xfrm_init_state(). Don't leave two calls, one in xfrm_user.c and one
> in xfrm_state.c
>
> Anyways, I don't think just making one call from xfrm_init_state() is
> possible, because the replay settings need to be assigned before we
> can properly call xfrm_init_replay().
>
> Therefore, please fix this by adding the necessary call to af_key.c
Sorry for not said clearly, at the first time I want to do like this.
But when I grep 'xfrm_init_state', it be used in many place, not
any pf_key, but also XFRM MIGRATE, ipcomp, ipcomp6. So I did this ugly
patch by add this to xfrm_init_state() to avoid dup code.
Not sure whether the other case like ipcomp/ipcomp6 etc can cause panic, if
it panic, maybe we can fix by introduce new xfrm_init_replay() function
like to assign the default reply function.
int xfrm_init_replay(struct xfrm_state *x)
{
x->repl = &xfrm_replay_legacy;
return 0;
}
and change the orig xfrm_init_replay to xfrm_update_replay()?
Or dup those code to all used place?
If I was wrong, I will fix this by adding the necessary call to af_key.c.
Thanks.
> Thanks.
>
^ permalink raw reply
* Re: oops / kernel panic in bonding.
From: Jiri Pirko @ 2011-03-21 6:37 UTC (permalink / raw)
To: Nicolas de Pesloüan
Cc: Jay Vosburgh, Andy Gospodarek, netdev@vger.kernel.org
In-Reply-To: <4D866F59.5070703@gmail.com>
Sun, Mar 20, 2011 at 10:19:21PM CET, nicolas.2p.debian@gmail.com wrote:
>Le 20/03/2011 21:17, Nicolas de Pesloüan a écrit :
>>Hi Jiri,
>>
>>I suspect we have a race condition somewhere in the new
>>bond_handle_frame function:
>>
>>The following commands produce one of the following errors:
>>
>>modprobe bonding max_bonds=0
>>echo +bond0>/sys/class/net/bonding_masters
>>echo +bond1>/sys/class/net/bonding_masters
>>echo +eth1>/sys/class/net/bond1/bonding/slaves
>>
>>This is mostly reproducible, under VirtualBox.
>>
>>All tests done with 08351fc6a75731226e1112fc7254542bd3a2912e at the top
>>commit (current net-next-2.6).
>
>I suspect netdev_rx_handler_register is called too early in bond_enslave.
>
>I think it should be the last thing we do in bond_enslave, if we
>don't want to face the risk to have bond_handle_frame being called
>before everything is properly setup.
>
> Nicolas.
Hmm, Offset 0x280 is big, I suggest dev->master is not set. Will look at
this.
^ permalink raw reply
* Re: [PATCHv2 0/9] macb: add support for Cadence GEM
From: Nicolas Ferre @ 2011-03-21 6:38 UTC (permalink / raw)
To: David Miller, jamie, Russell King - ARM Linux
Cc: netdev, linux-arm-kernel, plagnioj, Andrew Victor,
Peter Korsgaard
In-Reply-To: <20110316.131734.242138246.davem@davemloft.net>
On 3/17/2011 4:17 AM, David Miller :
> From: Jamie Iles <jamie@jamieiles.com>
> Date: Tue, 15 Mar 2011 10:14:47 +0000
>
>> This patch series extends the Atmel MACB driver to support the Cadence
>> GEM (Gigabit Ethernet MAC) to support 10/100 operation. The GEM is
>> based on the MACB block but has a few moved registers and bitfields.
>> This patch series attempts to use the MACB accessors where block
>> functionallity is identical and only overrides to GEM specific
>> acccessors when needed.
>>
>> This has been runtested on a board with a Cadence GEM and compile tested
>> for all at91 configurations and a number of avr32 configurations.
>>
>> Changes since v1:
>> - AT91 now provides a fake "hclk" and "macb_clk" has been
>> renamed to "pclk" to be consistent with AVR32.
>> - Configurable GEM receive buffer size support has been added.
>> - pr_foo() and dev_foo() have been converted to netdev_foo()
>> where appropriate.
>> - New conditional accessors (macb_or_gem_{read,write}l) have
>> been introduced that do the conditional accesses dependent on
>> macb/gem type.
>> - GEM is now dynamically detected from the module ID rather than
>> platform device name.
>>
>> Jean-Christophe, I haven't based this on your conditional clock patch as
>> I wasn't sure what decision had been made on that and whether the
>> at91/avr32 detection is reliable.
>
> I'm happy to ACK this so you guys can merge this via one of the
> ARM trees:
>
> Acked-by: David S. Miller <davem@davemloft.net>
I add my:
Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
Now, tell me if we need to setup a git tree with this and make it handled by linux-next (we do not have for at91 yet)?
or Russell can take the series in a "devel" branch?
I have tested the series with the v2 patches. It is working good with some modification that were discussed (hclk, pclk clocks). I attach modifications I have made to make it work so that you have an idea of my testbed (not a patch as you should already have this and it is on a development platform):
+/* One additional fake clock for macb interfaces */
+static struct clk hclk = {
+ .name = "hclk",
+ .pmc_mask = 0,
+ .type = CLK_TYPE_PERIPHERAL,
+};
@@ -247,6 +254,7 @@ static struct clk *periph_clocks[] __initdata = {
// irq0
&ohci_clk,
&tcb1_clk,
+ &hclk,
};
[..]
/* Clock */
- at91_clock_associate("macb0_clk", &at91sam9x5_eth0_device.dev, "macb_clk");
+ at91_clock_associate("macb0_clk", &at91sam9x5_eth0_device.dev, "pclk");
eth0_data = *data;
platform_device_register(&at91sam9x5_eth0_device);
Thanks, best regards,
--
Nicolas Ferre
^ permalink raw reply
* Re: xfrm: Fix initialize repl field of struct xfrm_state
From: David Miller @ 2011-03-21 6:46 UTC (permalink / raw)
To: yjwei; +Cc: netdev, steffen.klassert
In-Reply-To: <4D86F1FD.3080009@cn.fujitsu.com>
From: Wei Yongjun <yjwei@cn.fujitsu.com>
Date: Mon, 21 Mar 2011 14:36:45 +0800
> Sorry for not said clearly, at the first time I want to do like this.
> But when I grep 'xfrm_init_state', it be used in many place, not
> any pf_key, but also XFRM MIGRATE, ipcomp, ipcomp6. So I did this ugly
> patch by add this to xfrm_init_state() to avoid dup code.
>
> Not sure whether the other case like ipcomp/ipcomp6 etc can cause panic, if
> it panic, maybe we can fix by introduce new xfrm_init_replay() function
> like to assign the default reply function.
> int xfrm_init_replay(struct xfrm_state *x)
> {
> x->repl = &xfrm_replay_legacy;
> return 0;
> }
> and change the orig xfrm_init_replay to xfrm_update_replay()?
> Or dup those code to all used place?
>
> If I was wrong, I will fix this by adding the necessary call to af_key.c.
Ok, thanks for the explanation.
I think there is a simple way out of this:
1) Rename current xfrm_init_state to __xfrm_init_state, add
"bool init_replay" argument. Add the xfrm_init_replay()
call, as in your patch, but conditionalized on this boolean.
2) Implement xfrm_init_state as inline, which calls
__xfrm_init_state(..., true)
3) Replace xfrm_init_state() call in xfrm_user.c with
__xfrm_init_state(..., false)
This seems to avoid all the problems. We don't need to touch every
caller, and we avoid initializing the replay state twice in xfrm_user
Ok?
^ permalink raw reply
* Re: xfrm: Fix initialize repl field of struct xfrm_state
From: Wei Yongjun @ 2011-03-21 6:49 UTC (permalink / raw)
To: David Miller; +Cc: netdev, steffen.klassert
In-Reply-To: <20110320.234606.183056322.davem@davemloft.net>
> From: Wei Yongjun <yjwei@cn.fujitsu.com>
> Date: Mon, 21 Mar 2011 14:36:45 +0800
>
>> Sorry for not said clearly, at the first time I want to do like this.
>> But when I grep 'xfrm_init_state', it be used in many place, not
>> any pf_key, but also XFRM MIGRATE, ipcomp, ipcomp6. So I did this ugly
>> patch by add this to xfrm_init_state() to avoid dup code.
>>
>> Not sure whether the other case like ipcomp/ipcomp6 etc can cause panic, if
>> it panic, maybe we can fix by introduce new xfrm_init_replay() function
>> like to assign the default reply function.
>> int xfrm_init_replay(struct xfrm_state *x)
>> {
>> x->repl = &xfrm_replay_legacy;
>> return 0;
>> }
>> and change the orig xfrm_init_replay to xfrm_update_replay()?
>> Or dup those code to all used place?
>>
>> If I was wrong, I will fix this by adding the necessary call to af_key.c.
> Ok, thanks for the explanation.
>
> I think there is a simple way out of this:
>
> 1) Rename current xfrm_init_state to __xfrm_init_state, add
> "bool init_replay" argument. Add the xfrm_init_replay()
> call, as in your patch, but conditionalized on this boolean.
>
> 2) Implement xfrm_init_state as inline, which calls
> __xfrm_init_state(..., true)
>
> 3) Replace xfrm_init_state() call in xfrm_user.c with
> __xfrm_init_state(..., false)
>
> This seems to avoid all the problems. We don't need to touch every
> caller, and we avoid initializing the replay state twice in xfrm_user
>
> Ok?
This OK, I will do this change, thanks.
>
>
^ permalink raw reply
* Re: [PATCH v2 1/1] solos-pci: Fix regression introduced by newest firmware
From: Philip Prindeville @ 2011-03-21 7:25 UTC (permalink / raw)
To: David Miller; +Cc: bhutchings, netdev, linux-atm-general
In-Reply-To: <20110320.230408.112594622.davem@davemloft.net>
On 3/20/11 11:04 PM, David Miller wrote:
> From: Philip Prindeville<philipp_subx@redfish-solutions.com>
> Date: Sun, 20 Mar 2011 22:56:43 -0700
>
>> It's not clear that dropping all VCs abruptly when carrier flapped was
>> ever the right thing to do.
> So you've tested your change with the older firmware present?
I haven't, no: back-revving firmware has been known to brick cards.
I'm waiting to hear back from Guy and Nathan, they have old cards on hand that they can test.
^ permalink raw reply
* Re: [PATCH 2/2] socket: add minimum listen queue length sysctl
From: Eric Dumazet @ 2011-03-21 7:36 UTC (permalink / raw)
To: Hagen Paul Pfeifer; +Cc: netdev
In-Reply-To: <1300662282-24741-2-git-send-email-hagen@jauu.net>
Le lundi 21 mars 2011 à 00:04 +0100, Hagen Paul Pfeifer a écrit :
> In the case that a server programmer misjudge network characteristic the
> backlog parameter for listen(2) may not adequate to utilize hosts
> capabilities and lead to unrequired SYN retransmission - a small backlog
> value can form an artificial limitation. From Erics server setup, a
> listen queue length of 8 is often a way to small):
>
> ss -a | head
> State Recv-Q Send-Q Local Address:Port Peer
> Address:Port
> LISTEN 0 8 *:imaps *:*
> LISTEN 0 8 *:pop3s *:*
> LISTEN 0 50 *:mysql *:*
> LISTEN 0 8 *:pop3 *:*
> LISTEN 0 8 *:imap2 *:*
> LISTEN 0 511 *:www *:*
>
> Until now it is not possible for the system (network) administrator to
> increase this value. A bug report must be filled, the backlog increased,
> a new version released or even worse: if using closed source software
> you cannot make anything.
>
> sysctl_min_syn_backlog provides the ability to increase the minimum
> queue length. The default is 8.
>
> Signed-off-by: Hagen Paul Pfeifer <hagen@jauu.net>
>
> ---
> I will spin a second documentation patch if Davem accept this patch.
> ---
> include/net/request_sock.h | 1 +
> net/core/request_sock.c | 5 ++++-
> net/ipv4/sysctl_net_ipv4.c | 7 +++++++
> 3 files changed, 12 insertions(+), 1 deletions(-)
>
> diff --git a/include/net/request_sock.h b/include/net/request_sock.h
> index 99e6e19..3e8865f 100644
> --- a/include/net/request_sock.h
> +++ b/include/net/request_sock.h
> @@ -89,6 +89,7 @@ static inline void reqsk_free(struct request_sock *req)
> }
>
> extern int sysctl_max_syn_backlog;
> +extern int sysctl_min_syn_backlog;
>
> /** struct listen_sock - listen state
> *
> diff --git a/net/core/request_sock.c b/net/core/request_sock.c
> index 182236b..e937e9c 100644
> --- a/net/core/request_sock.c
> +++ b/net/core/request_sock.c
> @@ -35,6 +35,9 @@
> int sysctl_max_syn_backlog = 256;
> EXPORT_SYMBOL(sysctl_max_syn_backlog);
>
> +int sysctl_min_syn_backlog = 8;
> +EXPORT_SYMBOL(sysctl_min_syn_backlog);
> +
> int reqsk_queue_alloc(struct request_sock_queue *queue,
> unsigned int nr_table_entries)
> {
> @@ -42,7 +45,7 @@ int reqsk_queue_alloc(struct request_sock_queue *queue,
> struct listen_sock *lopt;
>
> nr_table_entries = min_t(u32, nr_table_entries, sysctl_max_syn_backlog);
> - nr_table_entries = max_t(u32, nr_table_entries, 8);
> + nr_table_entries = max_t(u32, nr_table_entries, sysctl_min_syn_backlog);
> nr_table_entries = roundup_pow_of_two(nr_table_entries + 1);
> lopt_size += nr_table_entries * sizeof(struct request_sock *);
> if (lopt_size > PAGE_SIZE)
I believe you are mistaken.
The code you change is the code sizing the hash table, not
sk->sk_max_ack_backlog
This only matters if one application is able to change its listen
backlog during its lifetime.
Say, it begins with :
listen(fd, 1);
Then, a bit later :
listen(fd, 8192);
This certainly is very unlikely...
With current kernel, it does change the maximum SYN_RECV sockets in
flight, but hash table is not resized and stay with 8 slots, so
performance might be suboptimal, since chains are going to hold 1024
elements.
^ permalink raw reply
* [PATCH v2] xfrm: Fix initialize repl field of struct xfrm_state
From: Wei Yongjun @ 2011-03-21 7:44 UTC (permalink / raw)
To: David Miller; +Cc: netdev, steffen.klassert
In-Reply-To: <20110320.234606.183056322.davem@davemloft.net>
Commit 'xfrm: Move IPsec replay detection functions to a separate file'
(9fdc4883d92d20842c5acea77a4a21bb1574b495)
introduce repl field to struct xfrm_state, and only initialize it
under SA's netlink create path, the other path, such as pf_key,
ipcomp/ipcomp6 etc, the repl field remaining uninitialize. So if
the SA is created by pf_key, any input packet with SA's encryption
algorithm will cause panic.
int xfrm_input()
{
...
x->repl->advance(x, seq);
...
}
This patch fixed it by introduce new function __xfrm_init_state().
Pid: 0, comm: swapper Not tainted 2.6.38-next+ #14 Bochs Bochs
EIP: 0060:[<c078e5d5>] EFLAGS: 00010206 CPU: 0
EIP is at xfrm_input+0x31c/0x4cc
EAX: dd839c00 EBX: 00000084 ECX: 00000000 EDX: 01000000
ESI: dd839c00 EDI: de3a0780 EBP: dec1de88 ESP: dec1de64
DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
Process swapper (pid: 0, ti=dec1c000 task=c09c0f20 task.ti=c0992000)
Stack:
00000000 00000000 00000002 c0ba27c0 00100000 01000000 de3a0798 c0ba27c0
00000033 dec1de98 c0786848 00000000 de3a0780 dec1dea4 c0786868 00000000
dec1debc c074ee56 e1da6b8c de3a0780 c074ed44 de3a07a8 dec1decc c074ef32
Call Trace:
[<c0786848>] xfrm4_rcv_encap+0x22/0x27
[<c0786868>] xfrm4_rcv+0x1b/0x1d
[<c074ee56>] ip_local_deliver_finish+0x112/0x1b1
[<c074ed44>] ? ip_local_deliver_finish+0x0/0x1b1
[<c074ef32>] NF_HOOK.clone.1+0x3d/0x44
[<c074ef77>] ip_local_deliver+0x3e/0x44
[<c074ed44>] ? ip_local_deliver_finish+0x0/0x1b1
[<c074ec03>] ip_rcv_finish+0x30a/0x332
[<c074e8f9>] ? ip_rcv_finish+0x0/0x332
[<c074ef32>] NF_HOOK.clone.1+0x3d/0x44
[<c074f188>] ip_rcv+0x20b/0x247
[<c074e8f9>] ? ip_rcv_finish+0x0/0x332
[<c072797d>] __netif_receive_skb+0x373/0x399
[<c0727bc1>] netif_receive_skb+0x4b/0x51
[<e0817e2a>] cp_rx_poll+0x210/0x2c4 [8139cp]
[<c072818f>] net_rx_action+0x9a/0x17d
[<c0445b5c>] __do_softirq+0xa1/0x149
[<c0445abb>] ? __do_softirq+0x0/0x149
Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
---
include/net/xfrm.h | 1 +
net/xfrm/xfrm_state.c | 13 ++++++++++++-
net/xfrm/xfrm_user.c | 2 +-
3 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 42a8c32..cffa5dc 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -1430,6 +1430,7 @@ extern void xfrm_spd_getinfo(struct net *net, struct xfrmk_spdinfo *si);
extern u32 xfrm_replay_seqhi(struct xfrm_state *x, __be32 net_seq);
extern int xfrm_init_replay(struct xfrm_state *x);
extern int xfrm_state_mtu(struct xfrm_state *x, int mtu);
+extern int __xfrm_init_state(struct xfrm_state *x, bool init_replay);
extern int xfrm_init_state(struct xfrm_state *x);
extern int xfrm_prepare_input(struct xfrm_state *x, struct sk_buff *skb);
extern int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi,
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index d575f05..ff96b14 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -1907,7 +1907,7 @@ int xfrm_state_mtu(struct xfrm_state *x, int mtu)
return res;
}
-int xfrm_init_state(struct xfrm_state *x)
+int __xfrm_init_state(struct xfrm_state *x, bool init_replay)
{
struct xfrm_state_afinfo *afinfo;
struct xfrm_mode *inner_mode;
@@ -1980,12 +1980,23 @@ int xfrm_init_state(struct xfrm_state *x)
if (x->outer_mode == NULL)
goto error;
+ if (init_replay) {
+ err = xfrm_init_replay(x);
+ if (err)
+ goto error;
+ }
+
x->km.state = XFRM_STATE_VALID;
error:
return err;
}
+int xfrm_init_state(struct xfrm_state *x)
+{
+ return __xfrm_init_state(x, true);
+}
+
EXPORT_SYMBOL(xfrm_init_state);
int __net_init xfrm_state_init(struct net *net)
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 706385a..fc152d2 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -511,7 +511,7 @@ static struct xfrm_state *xfrm_state_construct(struct net *net,
xfrm_mark_get(attrs, &x->mark);
- err = xfrm_init_state(x);
+ err = __xfrm_init_state(x, false);
if (err)
goto error;
--
1.6.5.2
^ permalink raw reply related
* Re: xfrm: Fix initialize repl field of struct xfrm_state
From: Steffen Klassert @ 2011-03-21 7:54 UTC (permalink / raw)
To: Wei Yongjun; +Cc: David Miller, netdev
In-Reply-To: <4D86F4EA.6030909@cn.fujitsu.com>
Thanks for catching this!
On Mon, Mar 21, 2011 at 02:49:14PM +0800, Wei Yongjun wrote:
> > Ok, thanks for the explanation.
> >
> > I think there is a simple way out of this:
> >
> > 1) Rename current xfrm_init_state to __xfrm_init_state, add
> > "bool init_replay" argument. Add the xfrm_init_replay()
> > call, as in your patch, but conditionalized on this boolean.
> >
> > 2) Implement xfrm_init_state as inline, which calls
> > __xfrm_init_state(..., true)
> >
> > 3) Replace xfrm_init_state() call in xfrm_user.c with
> > __xfrm_init_state(..., false)
> >
> > This seems to avoid all the problems. We don't need to touch every
> > caller, and we avoid initializing the replay state twice in xfrm_user
> >
> > Ok?
>
> This OK, I will do this change, thanks.
>
This looks ok for me too. I just noticed that we also need to clone the
replay_esn/preplay_esn informations and to reinitialize the sequence
number counting on XFRM MIGRATE from the original state before we
call xfrm_init_replay.
I'll fix this up once your patch is applied.
Thanks,
Steffen
^ permalink raw reply
* Re: [PATCH v2] net: add Faraday FTGMAC100 Gigabit Ethernet driver
From: Po-Yu Chuang @ 2011-03-21 7:54 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, linux-kernel, ratbert
In-Reply-To: <1300361405.3133.175.camel@edumazet-laptop>
Hi Eric,
On Thu, Mar 17, 2011 at 7:30 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le jeudi 17 mars 2011 à 18:56 +0800, Po-Yu Chuang a écrit :
>> On Thu, Mar 17, 2011 at 6:24 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> > Le jeudi 17 mars 2011 à 17:47 +0800, Po-Yu Chuang a écrit :
>> >> From: Po-Yu Chuang <ratbert@faraday-tech.com>
>> >>
>> >> FTGMAC100 Ethernet Media Access Controller supports 10/100/1000 Mbps
>> >> and MII/GMII. This driver has been working on some ARM/NDS32 SoC's
>> >> including Faraday A369 and Andes AG102.
>> >>
>> >> Signed-off-by: Po-Yu Chuang <ratbert@faraday-tech.com>
>> >> ---
>> >
>> > Did you tried GRO support ?
>>
>> No, I don't understand how to do that yet. Let me do some study first.
>> I'd really appreciate it if you could provide me some pointers about GRO. :-)
>> Is there any document about it?
>
> Should be easy and might help performance too.
>
> Take a look at commit :
>
> 2edae08e5b75269855fef3c74fe4292c066e7c33 (r8169: add gro support)
It seems to work fine so far at least the driver is not broken, but I did not
see much difference in performance though. How do I know if GRO is
working indeed?
Best regards,
Po-Yu Chuang
^ permalink raw reply
* Re: [PATCH v2] xfrm: Fix initialize repl field of struct xfrm_state
From: David Miller @ 2011-03-21 8:00 UTC (permalink / raw)
To: yjwei; +Cc: netdev, steffen.klassert
In-Reply-To: <4D8701C3.5010304@cn.fujitsu.com>
From: Wei Yongjun <yjwei@cn.fujitsu.com>
Date: Mon, 21 Mar 2011 15:44:03 +0800
> -int xfrm_init_state(struct xfrm_state *x)
> +int __xfrm_init_state(struct xfrm_state *x, bool init_replay)
Since it is referenced by modules, this new function symbol must
be exported to them.
^ permalink raw reply
* Re: [PATCH v2] xfrm: Fix initialize repl field of struct xfrm_state
From: David Miller @ 2011-03-21 8:01 UTC (permalink / raw)
To: yjwei; +Cc: netdev, steffen.klassert
In-Reply-To: <20110321.010037.59680394.davem@davemloft.net>
From: David Miller <davem@davemloft.net>
Date: Mon, 21 Mar 2011 01:00:37 -0700 (PDT)
> From: Wei Yongjun <yjwei@cn.fujitsu.com>
> Date: Mon, 21 Mar 2011 15:44:03 +0800
>
>> -int xfrm_init_state(struct xfrm_state *x)
>> +int __xfrm_init_state(struct xfrm_state *x, bool init_replay)
>
> Since it is referenced by modules, this new function symbol must
> be exported to them.
BTW, this is one of the reasons I explicitly mentioned to make
this an inline function. :-)
^ permalink raw reply
* Re: [PATCH v2] net: add Faraday FTGMAC100 Gigabit Ethernet driver
From: Eric Dumazet @ 2011-03-21 8:19 UTC (permalink / raw)
To: Po-Yu Chuang; +Cc: netdev, linux-kernel, ratbert
In-Reply-To: <AANLkTinibrF_bJBbYwCSnGv1iD_9Dn7uBn2cqgcTrKHi@mail.gmail.com>
Le lundi 21 mars 2011 à 15:54 +0800, Po-Yu Chuang a écrit :
> It seems to work fine so far at least the driver is not broken, but I did not
> see much difference in performance though. How do I know if GRO is
> working indeed?
maybe you receive one interrupt per incoming frame, and your cpu is fast
enough to handle the load.
tcpdump should show big incoming TCP frames (multi segment)
(GRO only helps TCP receivers)
Example here :
09:16:32.372949 IP 192.168.20.110.55446 > 192.168.20.108.ssh: .
2372863:2374311(1448) ack 3136 win 73 <nop,nop,timestamp 317934201
257881345>
09:16:32.372975 IP 192.168.20.110.55446 > 192.168.20.108.ssh: .
2374311:2377207(2896) ack 3136 win 73 <nop,nop,timestamp 317934201
257881345>
Here, you can see GRO work, since 2nd "frame" is the result of two MTU
sized frames.
^ permalink raw reply
* Re: xfrm: Fix initialize repl field of struct xfrm_state
From: Steffen Klassert @ 2011-03-21 8:25 UTC (permalink / raw)
To: David Miller; +Cc: yjwei, netdev
In-Reply-To: <20110320.234606.183056322.davem@davemloft.net>
On Sun, Mar 20, 2011 at 11:46:06PM -0700, David Miller wrote:
>
> Ok, thanks for the explanation.
>
> I think there is a simple way out of this:
>
> 1) Rename current xfrm_init_state to __xfrm_init_state, add
> "bool init_replay" argument. Add the xfrm_init_replay()
> call, as in your patch, but conditionalized on this boolean.
>
> 2) Implement xfrm_init_state as inline, which calls
> __xfrm_init_state(..., true)
>
> 3) Replace xfrm_init_state() call in xfrm_user.c with
> __xfrm_init_state(..., false)
>
> This seems to avoid all the problems. We don't need to touch every
> caller, and we avoid initializing the replay state twice in xfrm_user
>
Btw, looking a bit closer to this. I think it would look a bit cleaner
if we would add the xfrm_init_replay() call to xfrm_init_state() and
to move the xfrm_init_state() call in xfrm_state_construct() behind
the assign of the replay settings.
^ permalink raw reply
* [PATCH v3] xfrm: Fix initialize repl field of struct xfrm_state
From: Wei Yongjun @ 2011-03-21 8:37 UTC (permalink / raw)
To: David Miller; +Cc: netdev, steffen.klassert
In-Reply-To: <20110321.010037.59680394.davem@davemloft.net>
Commit 'xfrm: Move IPsec replay detection functions to a separate file'
(9fdc4883d92d20842c5acea77a4a21bb1574b495)
introduce repl field to struct xfrm_state, and only initialize it
under SA's netlink create path, the other path, such as pf_key,
ipcomp/ipcomp6 etc, the repl field remaining uninitialize. So if
the SA is created by pf_key, any input packet with SA's encryption
algorithm will cause panic.
int xfrm_input()
{
...
x->repl->advance(x, seq);
...
}
This patch fixed it by introduce new function __xfrm_init_state().
Pid: 0, comm: swapper Not tainted 2.6.38-next+ #14 Bochs Bochs
EIP: 0060:[<c078e5d5>] EFLAGS: 00010206 CPU: 0
EIP is at xfrm_input+0x31c/0x4cc
EAX: dd839c00 EBX: 00000084 ECX: 00000000 EDX: 01000000
ESI: dd839c00 EDI: de3a0780 EBP: dec1de88 ESP: dec1de64
DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
Process swapper (pid: 0, ti=dec1c000 task=c09c0f20 task.ti=c0992000)
Stack:
00000000 00000000 00000002 c0ba27c0 00100000 01000000 de3a0798 c0ba27c0
00000033 dec1de98 c0786848 00000000 de3a0780 dec1dea4 c0786868 00000000
dec1debc c074ee56 e1da6b8c de3a0780 c074ed44 de3a07a8 dec1decc c074ef32
Call Trace:
[<c0786848>] xfrm4_rcv_encap+0x22/0x27
[<c0786868>] xfrm4_rcv+0x1b/0x1d
[<c074ee56>] ip_local_deliver_finish+0x112/0x1b1
[<c074ed44>] ? ip_local_deliver_finish+0x0/0x1b1
[<c074ef32>] NF_HOOK.clone.1+0x3d/0x44
[<c074ef77>] ip_local_deliver+0x3e/0x44
[<c074ed44>] ? ip_local_deliver_finish+0x0/0x1b1
[<c074ec03>] ip_rcv_finish+0x30a/0x332
[<c074e8f9>] ? ip_rcv_finish+0x0/0x332
[<c074ef32>] NF_HOOK.clone.1+0x3d/0x44
[<c074f188>] ip_rcv+0x20b/0x247
[<c074e8f9>] ? ip_rcv_finish+0x0/0x332
[<c072797d>] __netif_receive_skb+0x373/0x399
[<c0727bc1>] netif_receive_skb+0x4b/0x51
[<e0817e2a>] cp_rx_poll+0x210/0x2c4 [8139cp]
[<c072818f>] net_rx_action+0x9a/0x17d
[<c0445b5c>] __do_softirq+0xa1/0x149
[<c0445abb>] ? __do_softirq+0x0/0x149
Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
---
Changelog:
v2 -> v3 export __xfrm_init_state().
---
include/net/xfrm.h | 1 +
net/xfrm/xfrm_state.c | 15 ++++++++++++++-
net/xfrm/xfrm_user.c | 2 +-
3 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 42a8c32..cffa5dc 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -1430,6 +1430,7 @@ extern void xfrm_spd_getinfo(struct net *net, struct xfrmk_spdinfo *si);
extern u32 xfrm_replay_seqhi(struct xfrm_state *x, __be32 net_seq);
extern int xfrm_init_replay(struct xfrm_state *x);
extern int xfrm_state_mtu(struct xfrm_state *x, int mtu);
+extern int __xfrm_init_state(struct xfrm_state *x, bool init_replay);
extern int xfrm_init_state(struct xfrm_state *x);
extern int xfrm_prepare_input(struct xfrm_state *x, struct sk_buff *skb);
extern int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi,
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index d575f05..f83a3d1 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -1907,7 +1907,7 @@ int xfrm_state_mtu(struct xfrm_state *x, int mtu)
return res;
}
-int xfrm_init_state(struct xfrm_state *x)
+int __xfrm_init_state(struct xfrm_state *x, bool init_replay)
{
struct xfrm_state_afinfo *afinfo;
struct xfrm_mode *inner_mode;
@@ -1980,12 +1980,25 @@ int xfrm_init_state(struct xfrm_state *x)
if (x->outer_mode == NULL)
goto error;
+ if (init_replay) {
+ err = xfrm_init_replay(x);
+ if (err)
+ goto error;
+ }
+
x->km.state = XFRM_STATE_VALID;
error:
return err;
}
+EXPORT_SYMBOL(__xfrm_init_state);
+
+int xfrm_init_state(struct xfrm_state *x)
+{
+ return __xfrm_init_state(x, true);
+}
+
EXPORT_SYMBOL(xfrm_init_state);
int __net_init xfrm_state_init(struct net *net)
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 706385a..fc152d2 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -511,7 +511,7 @@ static struct xfrm_state *xfrm_state_construct(struct net *net,
xfrm_mark_get(attrs, &x->mark);
- err = xfrm_init_state(x);
+ err = __xfrm_init_state(x, false);
if (err)
goto error;
--
1.6.5.2
^ permalink raw reply related
* RE: [PATCH] Fix "possible circular locking dependency" in rfcomm
From: Ferraton, Jean RegisX @ 2011-03-21 8:43 UTC (permalink / raw)
To: Gustavo F. Padovan
Cc: marcel@holtmann.org, davem@davemloft.net, eric.dumazet@gmail.com,
xiaosuo@gmail.com, linux-bluetooth@vger.kernel.org,
linux-kernel@vger.kernel.org, netdev@vger.kernel.org
In-Reply-To: <6E42A1B4DD2F7B4D80A1F26BB498BF9F8C9F8C1539@irsmsx501.ger.corp.intel.com>
Any comment?
> -----Original Message-----
> From: linux-bluetooth-owner@vger.kernel.org [mailto:linux-bluetooth-
> owner@vger.kernel.org] On Behalf Of Ferraton, Jean RegisX
> Sent: Monday, March 14, 2011 3:56 PM
> To: Gustavo F. Padovan
> Cc: marcel@holtmann.org; davem@davemloft.net; eric.dumazet@gmail.com;
> xiaosuo@gmail.com; linux-bluetooth@vger.kernel.org; linux-
> kernel@vger.kernel.org; netdev@vger.kernel.org
> Subject: [PATCH] Fix "possible circular locking dependency" in rfcomm
>
> No response from original post, submit again, more details below:
>
> Changes in sock.c in function __rfcomm_sock_close, to release sock before
> calling rfcomm_dlc_close to avoid possible deadlock when calling
> rfcomm_lock (in rfcomm_dlc_close).
>
> Signed-off-by: Jean Regis Ferraton <jrferraton@gmail.com>
>
> ---
> net/bluetooth/rfcomm/sock.c | 3 +++
> 1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c
> index 66cc1f0..3b5ec09 100644
> --- a/net/bluetooth/rfcomm/sock.c
> +++ b/net/bluetooth/rfcomm/sock.c
> @@ -234,7 +234,10 @@ static void __rfcomm_sock_close(struct sock *sk)
> case BT_CONNECT2:
> case BT_CONFIG:
> case BT_CONNECTED:
> + /* release_sock to avoid deadlock when calling rfcomm_lock()
> */
> + release_sock(sk);
> rfcomm_dlc_close(d, 0);
> + lock_sock(sk);
>
> default:
> sock_set_flag(sk, SOCK_ZAPPED);
> --
> 1.6.2.5
>
>
>
> This patch solves a problem of "possible circular locking dependency"
> related to rfcomm. Problem seen on kernel 2.6.37. Maybe it's not a bug,
> just a warning, see the trace below.
>
> This problem occurs in 2 cases:
> 1- When initiating a connection from the remote device
> 2- When browsing my BT device from my PC
>
> My analysis is the following:
> 1- We call the function rfcomm_sock_shutdown() which locks a socket, an
> after calls __rfcomm_sock_close()
> lock_sock(sk);
> if (!sk->sk_shutdown) {
> sk->sk_shutdown = SHUTDOWN_MASK;
> __rfcomm_sock_close(sk);
> 2- In the function __rfcomm_sock_close(), we call rfcomm_dlc_close()
> case BT_CONNECTED:
> rfcomm_dlc_close(d, 0);
> 3- In the function rfcomm_dlc_close(), we call rfcomm_lock() which locks a
> mutex on rfcomm, and this second lock causes the problem.
> rfcomm_lock();
> r = __rfcomm_dlc_close(d, err);
>
> My message trace is the following (for remote connect):
> [ 113.995511] =======================================================
> [ 114.001897] [ INFO: possible circular locking dependency detected ]
> [ 114.008157] 2.6.37.3jrf_fix #1
> [ 114.011196] -------------------------------------------------------
> [ 114.017457] bluetoothd/511 is trying to acquire lock:
> [ 114.022499] (rfcomm_mutex){+.+.+.}, at: [<c14f281a>]
> rfcomm_dlc_close+0x15/0x30
> [ 114.029887]
> [ 114.029892] but task is already holding lock:
> [ 114.035713] (sk_lock-AF_BLUETOOTH){+.+.+.}, at: [<c14f4cfe>]
> rfcomm_sock_shutdown+0x17/0x5d
> [ 114.044147]
> [ 114.044152] which lock already depends on the new lock.
> [ 114.044158]
> [ 114.052319]
> [ 114.052324] the existing dependency chain (in reverse order) is:
> [ 114.059796]
> [ 114.059801] -> #1 (sk_lock-AF_BLUETOOTH){+.+.+.}:
> [ 114.065967] [<c105d660>] lock_acquire+0xf9/0x135
> [ 114.071184] [<c1445cf3>] lock_sock_nested+0x55/0x65
> [ 114.076662] [<c14ed17d>] l2cap_sock_sendmsg+0x3f/0x606
> [ 114.082400] [<c1443775>] sock_sendmsg+0xc9/0xe0
> [ 114.087530] [<c14437b4>] kernel_sendmsg+0x28/0x37
> [ 114.092833] [<c14f2278>] rfcomm_send_frame+0x2f/0x37
> [ 114.098398] [<c14f232c>] rfcomm_send_ua+0x57/0x59
> [ 114.103701] [<c14f3884>] rfcomm_run+0x431/0xb06
> [ 114.108830] [<c104bc8f>] kthread+0x5f/0x64
> [ 114.113525] [<c1002b7a>] kernel_thread_helper+0x6/0x1a
> [ 114.119265]
> [ 114.119270] -> #0 (rfcomm_mutex){+.+.+.}:
> [ 114.124739] [<c105cff1>] __lock_acquire+0xe03/0x1379
> [ 114.130304] [<c105d660>] lock_acquire+0xf9/0x135
> [ 114.135522] [<c1522bf3>] mutex_lock_nested+0x45/0x286
> [ 114.141171] [<c14f281a>] rfcomm_dlc_close+0x15/0x30
> [ 114.146650] [<c14f4cdb>] __rfcomm_sock_close+0x5e/0x6a
> [ 114.152388] [<c14f4d12>] rfcomm_sock_shutdown+0x2b/0x5d
> [ 114.158212] [<c14f4d5d>] rfcomm_sock_release+0x19/0x60
> [ 114.163951] [<c14445da>] sock_release+0x14/0x5b
> [ 114.169080] [<c144463d>] sock_close+0x1c/0x20
> [ 114.174037] [<c10bdd19>] fput+0xfa/0x19e
> [ 114.178557] [<c10bb3c5>] filp_close+0x51/0x5b
> [ 114.183512] [<c10bb488>] sys_close+0xb9/0xee
> [ 114.188381] [<c1002650>] sysenter_do_call+0x12/0x36
> [ 114.193858]
> [ 114.193863] other info that might help us debug this:
> [ 114.193870]
> [ 114.201860] 1 lock held by bluetoothd/511:
> [ 114.205945] #0: (sk_lock-AF_BLUETOOTH){+.+.+.}, at: [<c14f4cfe>]
> rfcomm_sock_shutdown+0x17/0x5d
> [ 114.214811]
> [ 114.214816] stack backtrace:
> [ 114.219167] Pid: 511, comm: bluetoothd Tainted: G WC
> 2.6.37.3jrf_fix #1
> [ 114.226463] Call Trace:
> [ 114.228913] [<c152108a>] ? printk+0xf/0x11
> [ 114.233086] [<c105bc3b>] print_circular_bug+0x90/0x9c
> [ 114.238211] [<c105cff1>] __lock_acquire+0xe03/0x1379
> [ 114.243260] [<c14f281a>] ? rfcomm_dlc_close+0x15/0x30
> [ 114.248382] [<c105d660>] lock_acquire+0xf9/0x135
> [ 114.253079] [<c14f281a>] ? rfcomm_dlc_close+0x15/0x30
> [ 114.258208] [<c1522bf3>] mutex_lock_nested+0x45/0x286
> [ 114.263338] [<c14f281a>] ? rfcomm_dlc_close+0x15/0x30
> [ 114.268469] [<c105b68d>] ? trace_hardirqs_on_caller+0x100/0x121
> [ 114.274467] [<c1445cf8>] ? lock_sock_nested+0x5a/0x65
> [ 114.279597] [<c105b6b9>] ? trace_hardirqs_on+0xb/0xd
> [ 114.284639] [<c14f281a>] rfcomm_dlc_close+0x15/0x30
> [ 114.289594] [<c14f4cdb>] __rfcomm_sock_close+0x5e/0x6a
> [ 114.294811] [<c14f4d12>] rfcomm_sock_shutdown+0x2b/0x5d
> [ 114.300115] [<c14f4d5d>] rfcomm_sock_release+0x19/0x60
> [ 114.305331] [<c14445da>] sock_release+0x14/0x5b
> [ 114.309937] [<c144463d>] sock_close+0x1c/0x20
> [ 114.314373] [<c10bdd19>] fput+0xfa/0x19e
> [ 114.318371] [<c10bb3c5>] filp_close+0x51/0x5b
> [ 114.322804] [<c10bb488>] sys_close+0xb9/0xee
> [ 114.327151] [<c1002650>] sysenter_do_call+0x12/0x36
> ---------------------------------------------------------------------
> Intel Corporation SAS (French simplified joint stock company)
> Registered headquarters: "Les Montalets"- 2, rue de Paris,
> 92196 Meudon Cedex, France
> Registration Number: 302 456 199 R.C.S. NANTERRE
> Capital: 4,572,000 Euros
>
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth"
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
---------------------------------------------------------------------
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris,
92196 Meudon Cedex, France
Registration Number: 302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
^ permalink raw reply
* Re: xfrm: Fix initialize repl field of struct xfrm_state
From: Wei Yongjun @ 2011-03-21 9:10 UTC (permalink / raw)
To: Steffen Klassert; +Cc: David Miller, netdev
In-Reply-To: <20110321082512.GB27581@secunet.com>
> On Sun, Mar 20, 2011 at 11:46:06PM -0700, David Miller wrote:
>> Ok, thanks for the explanation.
>>
>> I think there is a simple way out of this:
>>
>> 1) Rename current xfrm_init_state to __xfrm_init_state, add
>> "bool init_replay" argument. Add the xfrm_init_replay()
>> call, as in your patch, but conditionalized on this boolean.
>>
>> 2) Implement xfrm_init_state as inline, which calls
>> __xfrm_init_state(..., true)
>>
>> 3) Replace xfrm_init_state() call in xfrm_user.c with
>> __xfrm_init_state(..., false)
>>
>> This seems to avoid all the problems. We don't need to touch every
>> caller, and we avoid initializing the replay state twice in xfrm_user
>>
> Btw, looking a bit closer to this. I think it would look a bit cleaner
> if we would add the xfrm_init_replay() call to xfrm_init_state() and
> to move the xfrm_init_state() call in xfrm_state_construct() behind
> the assign of the replay settings.
The xfrm_init_replay() should be call after the call to
xfrm_update_ae_params(x, attrs);
since xfrm_update_ae_params() may update the replay_esn.
So we need move the xfrm_init_state() call just before return x.
The other issue:
static void xfrm_update_ae_params()
{
...
memcpy(x->replay_esn, replay_esn,
xfrm_replay_state_esn_len(replay_esn));
...
}
the memcpy() may cause memory overlap if we build a special
nl_data, we should free it and then do kmemdup()?
> --
> 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
* Re: xfrm: Fix initialize repl field of struct xfrm_state
From: Wei Yongjun @ 2011-03-21 9:18 UTC (permalink / raw)
To: Steffen Klassert; +Cc: David Miller, netdev
In-Reply-To: <4D871607.6090508@cn.fujitsu.com>
>> On Sun, Mar 20, 2011 at 11:46:06PM -0700, David Miller wrote:
>>> Ok, thanks for the explanation.
>>>
>>> I think there is a simple way out of this:
>>>
>>> 1) Rename current xfrm_init_state to __xfrm_init_state, add
>>> "bool init_replay" argument. Add the xfrm_init_replay()
>>> call, as in your patch, but conditionalized on this boolean.
>>>
>>> 2) Implement xfrm_init_state as inline, which calls
>>> __xfrm_init_state(..., true)
>>>
>>> 3) Replace xfrm_init_state() call in xfrm_user.c with
>>> __xfrm_init_state(..., false)
>>>
>>> This seems to avoid all the problems. We don't need to touch every
>>> caller, and we avoid initializing the replay state twice in xfrm_user
>>>
>> Btw, looking a bit closer to this. I think it would look a bit cleaner
>> if we would add the xfrm_init_replay() call to xfrm_init_state() and
>> to move the xfrm_init_state() call in xfrm_state_construct() behind
>> the assign of the replay settings.
> The xfrm_init_replay() should be call after the call to
> xfrm_update_ae_params(x, attrs);
> since xfrm_update_ae_params() may update the replay_esn.
>
> So we need move the xfrm_init_state() call just before return x.
Oh, sorry, the memcpy looks like dup code since we used
kmemdup. It is the same attr XFRMA_REPLAY_ESN_VAL.
> The other issue:
> static void xfrm_update_ae_params()
> {
> ...
> memcpy(x->replay_esn, replay_esn,
> xfrm_replay_state_esn_len(replay_esn));
> ...
> }
>
> the memcpy() may cause memory overlap if we build a special
> nl_data, we should free it and then do kmemdup()?
>
>> --
>> 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
>>
> --
> 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
* Re: LRO disable warnings on kernel 2.6.38
From: Jesper Dangaard Brouer @ 2011-03-21 9:21 UTC (permalink / raw)
To: Alexander Duyck; +Cc: netdev, Eric Dumazet, Neil Horman
In-Reply-To: <4D8385CE.1060205@intel.com>
On Fri, 2011-03-18 at 09:18 -0700, Alexander Duyck wrote:
> On 3/18/2011 4:12 AM, Jesper Dangaard Brouer wrote:
> > Hi
> >
> > I'm seeing the LRO disable warnings using kernel 2.6.38:
[...]
> >
> The error doesn't make any sense for igb to be triggering since it
> doesn't support setting the NETIF_F_LRO flag in the 2.6.38 kernel.
>
> By any chance are there any ixgbe or other interfaces in the system? I
> would suspect the error to come from a driver that at least contains the
> NETIF_F_LRO flag.
The servers actually also have a four port semi-build in NIC (its a PCIe
slot with an extra connector, most likely for controlling the LEDs). I
don't use that NIC as it (1) caused kernel panics on 2.6.35, (2) needs a
firmware blob, (3) reading through the driver code (at some point in
time) they didn't implement multiqueue support correctly.
So lets blame that driver ;-)
lspci-info:
Ethernet controller: NetXen Incorporated NX3031 Multifunction 1/10-Gigabit Server Adapter (rev 42)
driver info via ethtool:
# ethtool -i eth01
driver: netxen_nic
version: 4.0.75
firmware-version: 4.0.530
bus-info: 0000:06:00.3
The strange part is that my pre-prod server also have a netxen_nic, but
it does not result in a WARN being tricked...
The pre-prod server do have a different firmware version.
# ethtool -i eth01
driver: netxen_nic
version: 4.0.75
firmware-version: 4.0.406
bus-info: 0000:06:00.0
(I have pulled the NIC out of some of the prod server, due to the risk
of a kernel panic on the old kernel. As operations have taken over the
deployment process, these NICs are still in. Guess I'll ask them to
blacklist the driver, so they don't see the warn stacktrace, they get so
nervous when they see stuff like that ;-))
--
Med venlig hilsen / Best regards
Jesper Brouer
ComX Networks A/S
Linux Network Kernel Developer
Cand. Scient Datalog / MSc.CS
Author of http://adsl-optimizer.dk
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* [PATCH] l2tp: fix possible oops on l2tp_eth module unload
From: James Chapman @ 2011-03-21 9:26 UTC (permalink / raw)
To: netdev
A struct used in the l2tp_eth driver for registering network namespace
ops was incorrectly marked as __net_initdata, leading to oops when
module unloaded.
BUG: unable to handle kernel paging request at ffffffffa00ec098
IP: [<ffffffff8123dbd8>] ops_exit_list+0x7/0x4b
PGD 142d067 PUD 1431063 PMD 195da8067 PTE 0
Oops: 0000 [#1] SMP
last sysfs file: /sys/module/l2tp_eth/refcnt
Call Trace:
[<ffffffff8123dc94>] ? unregister_pernet_operations+0x32/0x93
[<ffffffff8123dd20>] ? unregister_pernet_device+0x2b/0x38
[<ffffffff81068b6e>] ? sys_delete_module+0x1b8/0x222
[<ffffffff810c7300>] ? do_munmap+0x254/0x318
[<ffffffff812c64e5>] ? page_fault+0x25/0x30
[<ffffffff812c6952>] ? system_call_fastpath+0x16/0x1b
Signed-off-by: James Chapman <jchapman@katalix.com>
---
Push to stable trees too?
diff --git a/net/l2tp/l2tp_eth.c b/net/l2tp/l2tp_eth.c
index 8d9ce0a..a8193f5 100644
--- a/net/l2tp/l2tp_eth.c
+++ b/net/l2tp/l2tp_eth.c
@@ -283,7 +283,7 @@ static __net_init int l2tp_eth_init_net(struct net *net)
return 0;
}
-static __net_initdata struct pernet_operations l2tp_eth_net_ops = {
+static struct pernet_operations l2tp_eth_net_ops = {
.init = l2tp_eth_init_net,
.id = &l2tp_eth_net_id,
.size = sizeof(struct l2tp_eth_net),
^ permalink raw reply related
* Re: LRO disable warnings on kernel 2.6.38
From: Eric Dumazet @ 2011-03-21 9:45 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: Alexander Duyck, netdev, Neil Horman, Stanislaw Gruszka
In-Reply-To: <1300699294.10934.63.camel@firesoul.comx.local>
Le lundi 21 mars 2011 à 10:21 +0100, Jesper Dangaard Brouer a écrit :
> On Fri, 2011-03-18 at 09:18 -0700, Alexander Duyck wrote:
> > On 3/18/2011 4:12 AM, Jesper Dangaard Brouer wrote:
> > > Hi
> > >
> > > I'm seeing the LRO disable warnings using kernel 2.6.38:
> [...]
> > >
> > The error doesn't make any sense for igb to be triggering since it
> > doesn't support setting the NETIF_F_LRO flag in the 2.6.38 kernel.
> >
> > By any chance are there any ixgbe or other interfaces in the system? I
> > would suspect the error to come from a driver that at least contains the
> > NETIF_F_LRO flag.
>
> The servers actually also have a four port semi-build in NIC (its a PCIe
> slot with an extra connector, most likely for controlling the LEDs). I
> don't use that NIC as it (1) caused kernel panics on 2.6.35, (2) needs a
> firmware blob, (3) reading through the driver code (at some point in
> time) they didn't implement multiqueue support correctly.
>
> So lets blame that driver ;-)
>
> lspci-info:
> Ethernet controller: NetXen Incorporated NX3031 Multifunction 1/10-Gigabit Server Adapter (rev 42)
>
> driver info via ethtool:
> # ethtool -i eth01
> driver: netxen_nic
> version: 4.0.75
> firmware-version: 4.0.530
> bus-info: 0000:06:00.3
>
> The strange part is that my pre-prod server also have a netxen_nic, but
> it does not result in a WARN being tricked...
>
> The pre-prod server do have a different firmware version.
> # ethtool -i eth01
> driver: netxen_nic
> version: 4.0.75
> firmware-version: 4.0.406
> bus-info: 0000:06:00.0
>
> (I have pulled the NIC out of some of the prod server, due to the risk
> of a kernel panic on the old kernel. As operations have taken over the
> deployment process, these NICs are still in. Guess I'll ask them to
> blacklist the driver, so they don't see the warn stacktrace, they get so
> nervous when they see stuff like that ;-))
>
Well, this warning can be ignored since these NIC dont receive packets
to be forwarded in your setup.
I would say netxen_nic_set_flags() is buggy : It rejects data if other
flag than ETH_FLAG_LRO is set.
if (data & ~ETH_FLAG_LRO)
return -EINVAL;
Brought by commit ef2519b1dd39940 (netxen: fail when try to setup
unsupported features) later corrected by commit 97d1935a61b7fe7a65f98f
(Make ethtool_ops::set_flags() return -EINVAL for unsupported flags)
Since this drivers asserts NETIF_F_HW_VLAN_TX (mirroring
ETH_FLAG_TXVLAN), ethtool_op_get_flags() can return more than
ETH_FLAG_LRO.
dev_disable_lro() then calls netxen_nic_set_flags() with ETH_FLAG_TXVLAN
-> -EINVAL, and ETH_FLAG_LRO stay ORed in dev->features -> WARNING
^ permalink raw reply
* Re: LRO disable warnings on kernel 2.6.38
From: Eric Dumazet @ 2011-03-21 9:53 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: Alexander Duyck, netdev, Neil Horman, Stanislaw Gruszka
In-Reply-To: <1300700708.2884.10.camel@edumazet-laptop>
Le lundi 21 mars 2011 à 10:45 +0100, Eric Dumazet a écrit :
> Le lundi 21 mars 2011 à 10:21 +0100, Jesper Dangaard Brouer a écrit :
> > On Fri, 2011-03-18 at 09:18 -0700, Alexander Duyck wrote:
> > > On 3/18/2011 4:12 AM, Jesper Dangaard Brouer wrote:
> > > > Hi
> > > >
> > > > I'm seeing the LRO disable warnings using kernel 2.6.38:
> > [...]
> > > >
> > > The error doesn't make any sense for igb to be triggering since it
> > > doesn't support setting the NETIF_F_LRO flag in the 2.6.38 kernel.
> > >
> > > By any chance are there any ixgbe or other interfaces in the system? I
> > > would suspect the error to come from a driver that at least contains the
> > > NETIF_F_LRO flag.
> >
> > The servers actually also have a four port semi-build in NIC (its a PCIe
> > slot with an extra connector, most likely for controlling the LEDs). I
> > don't use that NIC as it (1) caused kernel panics on 2.6.35, (2) needs a
> > firmware blob, (3) reading through the driver code (at some point in
> > time) they didn't implement multiqueue support correctly.
> >
> > So lets blame that driver ;-)
> >
> > lspci-info:
> > Ethernet controller: NetXen Incorporated NX3031 Multifunction 1/10-Gigabit Server Adapter (rev 42)
> >
> > driver info via ethtool:
> > # ethtool -i eth01
> > driver: netxen_nic
> > version: 4.0.75
> > firmware-version: 4.0.530
> > bus-info: 0000:06:00.3
> >
> > The strange part is that my pre-prod server also have a netxen_nic, but
> > it does not result in a WARN being tricked...
> >
> > The pre-prod server do have a different firmware version.
> > # ethtool -i eth01
> > driver: netxen_nic
> > version: 4.0.75
> > firmware-version: 4.0.406
> > bus-info: 0000:06:00.0
> >
> > (I have pulled the NIC out of some of the prod server, due to the risk
> > of a kernel panic on the old kernel. As operations have taken over the
> > deployment process, these NICs are still in. Guess I'll ask them to
> > blacklist the driver, so they don't see the warn stacktrace, they get so
> > nervous when they see stuff like that ;-))
> >
>
> Well, this warning can be ignored since these NIC dont receive packets
> to be forwarded in your setup.
>
> I would say netxen_nic_set_flags() is buggy : It rejects data if other
> flag than ETH_FLAG_LRO is set.
>
> if (data & ~ETH_FLAG_LRO)
> return -EINVAL;
>
> Brought by commit ef2519b1dd39940 (netxen: fail when try to setup
> unsupported features) later corrected by commit 97d1935a61b7fe7a65f98f
> (Make ethtool_ops::set_flags() return -EINVAL for unsupported flags)
>
> Since this drivers asserts NETIF_F_HW_VLAN_TX (mirroring
> ETH_FLAG_TXVLAN), ethtool_op_get_flags() can return more than
> ETH_FLAG_LRO.
>
> dev_disable_lro() then calls netxen_nic_set_flags() with ETH_FLAG_TXVLAN
> -> -EINVAL, and ETH_FLAG_LRO stay ORed in dev->features -> WARNING
>
>
>
>
Couly you please send us the result of "ethtool -k eth01" to make sure
its the problem ?
Make sure your ethtool version includes these bits :
# ethtool -k eth0
Offload parameters for eth0:
...
large-receive-offload: off
rx-vlan-offload: on
tx-vlan-offload: on
ntuple-filters: off
receive-hashing: off
Thanks
^ 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