* Re: [PATCH v2] tcp: tsq: restore minimal amount of queueing
From: Holger Hoffstaette @ 2013-11-13 21:59 UTC (permalink / raw)
To: netdev
In-Reply-To: <8738n0ngq3.fsf@natisbad.org>
On Wed, 13 Nov 2013 22:18:12 +0100, Arnaud Ebalard wrote:
> Hi Eric,
>
> Eric Dumazet <eric.dumazet@gmail.com> writes:
>
>> From: Eric Dumazet <edumazet@google.com>
>>
>> After commit c9eeec26e32e ("tcp: TSQ can use a dynamic limit"), several
>> users reported throughput regressions, notably on mvneta and wifi
>> adapters.
>>
>> 802.11 AMPDU requires a fair amount of queueing to be effective.
>>
>> This patch partially reverts the change done in tcp_write_xmit() so that
>> the minimal amount is sysctl_tcp_limit_output_bytes.
>
> I just tested the fix on current linux tree with the same setup on which I
> observed the regression: it also fixes it on my side on my RN102. Thanks
> for the quick fix, Eric.
+1: fixes spastic NFS & Samba throughput since 3.12.0 with r8169 and e100e
for me as well. It's really not just broken/weird Wifi cards that are
affected by this.
Thanks!
-h
^ permalink raw reply
* Re: [PATCH] net: sctp: bug-fixing: retran_path not set properly after transports recovering (v2)
From: Vlad Yasevich @ 2013-11-13 22:13 UTC (permalink / raw)
To: Chang Xiangzhong; +Cc: nhorman, davem, linux-sctp, netdev
In-Reply-To: <1384379666-7541-1-git-send-email-changxiangzhong@gmail.com>
On 11/13/2013 04:54 PM, Chang Xiangzhong wrote:
> When a transport recovers due to the new coming sack, SCTP should
> iterate all of its transport_list to locate the __two__ most recently used
> transport and set to active_path and retran_path respectively. The exising
> code does not find the two properly - In case of the following list:
>
> [most-recent] -> [2nd-most-recent] -> ...
>
> Both active_path and retran_path would be set to the 1st element.
>
> The bug happens when:
> 1) multi-homing
> 2) failure/partial_failure transport recovers
> Both active_path and retran_path would be set to the same most-recent one, in
> other words, retran_path would not take its role - an end user might not even
> notice this issue.
>
> Signed-off-by: Chang Xiangzhong <changxiangzhong@gmail.com>
> ---
> net/sctp/associola.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> index ab67efc..a7fc856 100644
> --- a/net/sctp/associola.c
> +++ b/net/sctp/associola.c
> @@ -913,11 +913,12 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
> if (!first || t->last_time_heard > first->last_time_heard) {
> second = first;
> first = t;
> - }
> - if (!second || t->last_time_heard > second->last_time_heard)
> + } else if (!second ||
> + t->last_time_heard > second->last_time_heard)
Align with !second.
> second = t;
> }
>
> +
Spurious new line.
-vlad
> /* RFC 2960 6.4 Multi-Homed SCTP Endpoints
> *
> * By default, an endpoint should always transmit to the
> @@ -935,6 +936,8 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
> first = asoc->peer.primary_path;
> }
>
> + if (!second)
> + second = first;
> /* If we failed to find a usable transport, just camp on the
> * primary, even if it is inactive.
> */
>
^ permalink raw reply
* Re: oops in tcp_get_metrics, followed by lockup.
From: Eric Dumazet @ 2013-11-13 22:40 UTC (permalink / raw)
To: Dave Jones; +Cc: netdev
In-Reply-To: <20131113204543.GA26715@redhat.com>
On Wed, 2013-11-13 at 15:45 -0500, Dave Jones wrote:
> My fuzzer just hit this on v3.12-7033-g42a2d923cc34
>
> Oops: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC
> Modules linked in: fuse hidp tun snd_seq_dummy bnep nfnetlink rfcomm ipt_ULOG can_bcm nfc caif_socket caif af_802154 phonet af_rxrpc bluetooth rfkill can_raw can llc2 pppoe pppox ppp
> _generic slhc irda crc_ccitt rds scsi_transport_iscsi af_key rose x25 atm netrom appletalk ipx p8023 psnap p8022 llc ax25 xfs libcrc32c coretemp hwmon x86_pkg_temp_thermal kvm_intel kvm crct10dif_p
> clmul crc32c_intel ghash_clmulni_intel usb_debug snd_hda_codec_realtek snd_hda_codec_hdmi microcode pcspkr snd_hda_intel snd_hda_codec snd_hwdep snd_seq snd_seq_device snd_pcm snd_page_alloc snd_ti
> mer e1000e snd ptp shpchp soundcore pps_core serio_raw
> CPU: 1 PID: 16002 Comm: trinity-child1 Not tainted 3.12.0+ #2
> task: ffff88023cd75580 ti: ffff88009ee26000 task.ti: ffff88009ee26000
> RIP: 0010:[<ffffffff81658dd2>] [<ffffffff81658dd2>] tcp_get_metrics+0x62/0x420
> RSP: 0018:ffff880244a03d28 EFLAGS: 00010246
> RAX: 0000000000000002 RBX: ffff88009c77a4c0 RCX: 0000000000000001
> RDX: 0000000000000001 RSI: 0000000000000000 RDI: ffff88009c77a4c0
> RBP: ffff880244a03d78 R08: 0000000000000001 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
> R13: 0000000000000000 R14: 00000000000010ac R15: 0000000000000000
> FS: 0000000000000000(0000) GS:ffff880244a00000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000000018 CR3: 0000000001c0b000 CR4: 00000000001407e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Stack:
> 000000018165a6b5 ffffffff000010ac 0000000000000246 0000000044a00002
> ffffffff81c480a0 ffff88009c77a4c0 0000000000000000 0000000000000000
> 0000000000000001 0000000000000000 ffff880244a03db8 ffffffff8165a740
> Call Trace:
> <IRQ>
> [<ffffffff8165a740>] tcp_fastopen_cache_set+0x90/0x280
> [<ffffffff8165a6b5>] ? tcp_fastopen_cache_set+0x5/0x280
> [<ffffffff8164f3a7>] tcp_retransmit_timer+0x1d7/0x930
> [<ffffffff8164fcb0>] ? tcp_write_timer_handler+0x1b0/0x1b0
> [<ffffffff8164fba0>] tcp_write_timer_handler+0xa0/0x1b0
> [<ffffffff8164fd2c>] tcp_write_timer+0x7c/0x80
> [<ffffffff81063c1a>] call_timer_fn+0x8a/0x340
> [<ffffffff81063b95>] ? call_timer_fn+0x5/0x340
> [<ffffffff8164fcb0>] ? tcp_write_timer_handler+0x1b0/0x1b0
> [<ffffffff81064114>] run_timer_softirq+0x244/0x3a0
> [<ffffffff8105aa9c>] __do_softirq+0xfc/0x490
> [<ffffffff8105b28d>] irq_exit+0x13d/0x160
> [<ffffffff8172fe25>] smp_apic_timer_interrupt+0x45/0x60
> [<ffffffff8172eaaf>] apic_timer_interrupt+0x6f/0x80
> <EOI>
> [<ffffffff810d559d>] ? trace_hardirqs_on+0xd/0x10
> [<ffffffff811560af>] ? free_hot_cold_page+0xff/0x180
> [<ffffffff81156176>] free_hot_cold_page_list+0x46/0x160
> [<ffffffff8115c21e>] release_pages+0x8e/0x1f0
> [<ffffffff8118c135>] free_pages_and_swap_cache+0x95/0xb0
> [<ffffffff81175acc>] tlb_flush_mmu.part.73+0x4c/0x90
> [<ffffffff81176115>] tlb_finish_mmu+0x55/0x60
> [<ffffffff81180d84>] exit_mmap+0xf4/0x170
> [<ffffffff8105108b>] mmput+0x6b/0x100
> [<ffffffff810559e8>] do_exit+0x278/0xcb0
> [<ffffffff817250e1>] ? _raw_spin_unlock+0x31/0x50
> [<ffffffff810d53c6>] ? trace_hardirqs_on_caller+0x16/0x1e0
> [<ffffffff810d559d>] ? trace_hardirqs_on+0xd/0x10
> [<ffffffff810577ec>] do_group_exit+0x4c/0xc0
> [<ffffffff81057874>] SyS_exit_group+0x14/0x20
> [<ffffffff8172e064>] tracesys+0xdd/0xe2
> Code: 0a 0f 85 c2 01 00 00 48 8b 47 38 48 8b 57 40 48 89 44 24 08 48 8b 47 40 48 89 54 24 10 48 33 47 38 49 89 c6 49 c1 ee 20 41 31 c6 <49> 8b 45 18 b9 20 00 00 00 45 69 f6 01 00 37 9e 48 8b 80 d8 04
> RIP [<ffffffff81658dd2>] tcp_get_metrics+0x62/0x420
> RSP <ffff880244a03d28>
> CR2: 0000000000000018
> ---[ end trace c25bf4de9744120a ]---
>
>
> The disassembly looks like it happened here :-
>
>
> static inline u32 ipv6_addr_hash(const struct in6_addr *a)
> {
> #if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) && BITS_PER_LONG == 64
> const unsigned long *ul = (const unsigned long *)a;
> unsigned long x = ul[0] ^ ul[1];
> 10db: 48 8b 47 40 mov 0x40(%rdi),%rax
> 10df: 48 89 54 24 10 mov %rdx,0x10(%rsp)
> 10e4: 48 33 47 38 xor 0x38(%rdi),%rax
>
> return (u32)(x ^ (x >> 32));
> 10e8: 49 89 c6 mov %rax,%r14
> 10eb: 49 c1 ee 20 shr $0x20,%r14
> 10ef: 41 31 c6 xor %eax,%r14d
> 10f2: 49 8b 45 18 mov 0x18(%r13),%rax <<<< Faulting instruction.
> 10f6: b9 20 00 00 00 mov $0x20,%ecx
> }
I do not think this is the ipv6_addr_hash()
%r13 here seems to be dst pointer
Trap on accessing dst->dev as in :
net = dev_net(dst->dev);
So we crash because socket has a NULL dst entry.
^ permalink raw reply
* Re: [PATCH v2] tcp: tsq: restore minimal amount of queueing
From: Eric Dumazet @ 2013-11-13 22:41 UTC (permalink / raw)
To: Arnaud Ebalard
Cc: David Miller, Sujith Manoharan, Cong Wang, netdev, Felix Fietkau
In-Reply-To: <8738n0ngq3.fsf@natisbad.org>
On Wed, 2013-11-13 at 22:18 +0100, Arnaud Ebalard wrote:
> Hi Eric,
>
> Eric Dumazet <eric.dumazet@gmail.com> writes:
>
> > From: Eric Dumazet <edumazet@google.com>
> >
> > After commit c9eeec26e32e ("tcp: TSQ can use a dynamic limit"), several
> > users reported throughput regressions, notably on mvneta and wifi
> > adapters.
> >
> > 802.11 AMPDU requires a fair amount of queueing to be effective.
> >
> > This patch partially reverts the change done in tcp_write_xmit()
> > so that the minimal amount is sysctl_tcp_limit_output_bytes.
>
> I just tested the fix on current linux tree with the same setup
> on which I observed the regression: it also fixes it on my side
> on my RN102. Thanks for the quick fix, Eric.
>
It would be nice you find the 'right value' for the sysctl, to give a
hint to mvneta maintainers ;)
Thanks
^ permalink raw reply
* [PATCH] net-tcp: fix panic in tcp_fastopen_cache_set()
From: Eric Dumazet @ 2013-11-13 23:00 UTC (permalink / raw)
To: Dave Jones, David Miller; +Cc: netdev, Yuchung Cheng
In-Reply-To: <1384382413.28458.132.camel@edumazet-glaptop2.roam.corp.google.com>
From: Eric Dumazet <edumazet@google.com>
We had some reports of crashes using TCP fastopen, and Dave Jones
gave a nice stack trace pointing to the error.
Issue is that tcp_get_metrics() should not be called with a NULL dst
Fixes: 1fe4c481ba637 ("net-tcp: Fast Open client - cookie cache")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Dave Jones <davej@redhat.com>
Cc: Yuchung Cheng <ycheng@google.com>
---
net/ipv4/tcp_metrics.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/net/ipv4/tcp_metrics.c b/net/ipv4/tcp_metrics.c
index 2ab09cbae74d..d3ee2e0c28b6 100644
--- a/net/ipv4/tcp_metrics.c
+++ b/net/ipv4/tcp_metrics.c
@@ -663,10 +663,13 @@ void tcp_fastopen_cache_get(struct sock *sk, u16 *mss,
void tcp_fastopen_cache_set(struct sock *sk, u16 mss,
struct tcp_fastopen_cookie *cookie, bool syn_lost)
{
+ struct dst_entry *dst = __sk_dst_get(sk);
struct tcp_metrics_block *tm;
+ if (!dst)
+ return;
rcu_read_lock();
- tm = tcp_get_metrics(sk, __sk_dst_get(sk), true);
+ tm = tcp_get_metrics(sk, dst, true);
if (tm) {
struct tcp_fastopen_metrics *tfom = &tm->tcpm_fastopen;
^ permalink raw reply related
* Re: [PATCH] net-tcp: fix panic in tcp_fastopen_cache_set()
From: Yuchung Cheng @ 2013-11-13 23:08 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Dave Jones, David Miller, netdev
In-Reply-To: <1384383646.28458.138.camel@edumazet-glaptop2.roam.corp.google.com>
On Wed, Nov 13, 2013 at 3:00 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> We had some reports of crashes using TCP fastopen, and Dave Jones
> gave a nice stack trace pointing to the error.
>
> Issue is that tcp_get_metrics() should not be called with a NULL dst
>
> Fixes: 1fe4c481ba637 ("net-tcp: Fast Open client - cookie cache")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: Dave Jones <davej@redhat.com>
> Cc: Yuchung Cheng <ycheng@google.com>
Acked-by: Yuchung Cheng <ycheng@google.com>
> ---
> net/ipv4/tcp_metrics.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/net/ipv4/tcp_metrics.c b/net/ipv4/tcp_metrics.c
> index 2ab09cbae74d..d3ee2e0c28b6 100644
> --- a/net/ipv4/tcp_metrics.c
> +++ b/net/ipv4/tcp_metrics.c
> @@ -663,10 +663,13 @@ void tcp_fastopen_cache_get(struct sock *sk, u16 *mss,
> void tcp_fastopen_cache_set(struct sock *sk, u16 mss,
> struct tcp_fastopen_cookie *cookie, bool syn_lost)
> {
> + struct dst_entry *dst = __sk_dst_get(sk);
> struct tcp_metrics_block *tm;
>
> + if (!dst)
> + return;
> rcu_read_lock();
> - tm = tcp_get_metrics(sk, __sk_dst_get(sk), true);
> + tm = tcp_get_metrics(sk, dst, true);
> if (tm) {
> struct tcp_fastopen_metrics *tfom = &tm->tcpm_fastopen;
>
>
>
^ permalink raw reply
* Re: [PATCH] netfilter: fix connlimit Kconfig prompt string
From: Pablo Neira Ayuso @ 2013-11-13 23:12 UTC (permalink / raw)
To: Randy Dunlap
Cc: netdev@vger.kernel.org, David Miller, netfilter-devel,
lailavrazda1979
In-Reply-To: <527B29A6.6090802@infradead.org>
On Wed, Nov 06, 2013 at 09:48:22PM -0800, Randy Dunlap wrote:
> From: Randy Dunlap <rdunlap@infradead.org>
>
> Under Core Netfilter Configuration, connlimit match support has
> an extra double quote at the end of it.
>
> Fixes a portion of kernel bugzilla #52671:
> https://bugzilla.kernel.org/show_bug.cgi?id=52671
Applied, thanks Randy.
^ permalink raw reply
* Re: [PATCH v2] tcp: tsq: restore minimal amount of queueing
From: Eric Dumazet @ 2013-11-13 23:40 UTC (permalink / raw)
To: Holger Hoffstaette; +Cc: netdev
In-Reply-To: <pan.2013.11.13.21.59.50.578858@googlemail.com>
On Wed, 2013-11-13 at 22:59 +0100, Holger Hoffstaette wrote:
> +1: fixes spastic NFS & Samba throughput since 3.12.0 with r8169 and e100e
> for me as well. It's really not just broken/weird Wifi cards that are
> affected by this.
Same remark, it would be nice to pinpoint the problematic driver.
e1000e had a problem last year, fixed in commit
8edc0e624db3756783233e464879eb2e3b904c13
("e1000e: Change wthresh to 1 to avoid possible Tx stalls")
We can live with the current situation and continue to fill buffers
to work around bugs, or we can try to fix the bugs ;)
Thanks
^ permalink raw reply
* [PATCH] of: make of_get_phy_mode parse 'phy-connection-type'
From: Florian Fainelli @ 2013-11-13 23:42 UTC (permalink / raw)
To: grant.likely
Cc: rob.herring, netdev, devicetree, linux-kernel, Florian Fainelli
Per the ePAPR v1.1 specification, 'phy-connection-type' is the canonical
property name for describing an Ethernet to PHY connection type. Make
sure that of_get_phy_mode() also attempts to parse that property and
update the comments mentioninng 'phy-mode' to also include
'phy-connection-type'.
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/of/of_net.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c
index 8f9be2e..101a34c 100644
--- a/drivers/of/of_net.c
+++ b/drivers/of/of_net.c
@@ -13,8 +13,8 @@
/**
* It maps 'enum phy_interface_t' found in include/linux/phy.h
- * into the device tree binding of 'phy-mode', so that Ethernet
- * device driver can get phy interface from device tree.
+ * into the device tree binding of 'phy-mode' or 'phy-connection-type',
+ * so that Ethernet device driver can get phy interface from device tree.
*/
static const char *phy_modes[] = {
[PHY_INTERFACE_MODE_NA] = "",
@@ -36,8 +36,9 @@ static const char *phy_modes[] = {
* of_get_phy_mode - Get phy mode for given device_node
* @np: Pointer to the given device_node
*
- * The function gets phy interface string from property 'phy-mode',
- * and return its index in phy_modes table, or errno in error case.
+ * The function gets phy interface string from property 'phy-mode' or
+ * 'phy-connection-type', and return its index in phy_modes table, or errno in
+ * error case.
*/
int of_get_phy_mode(struct device_node *np)
{
@@ -45,8 +46,11 @@ int of_get_phy_mode(struct device_node *np)
int err, i;
err = of_property_read_string(np, "phy-mode", &pm);
- if (err < 0)
- return err;
+ if (err < 0) {
+ err = of_property_read_string(np, "phy-connection-type", &pm);
+ if (err < 0)
+ return err;
+ }
for (i = 0; i < ARRAY_SIZE(phy_modes); i++)
if (!strcasecmp(pm, phy_modes[i]))
--
1.8.3.2
^ permalink raw reply related
* Re: [PATCH v2] tcp: tsq: restore minimal amount of queueing
From: Holger Hoffstaette @ 2013-11-13 23:52 UTC (permalink / raw)
To: netdev
In-Reply-To: <1384386040.28458.143.camel@edumazet-glaptop2.roam.corp.google.com>
On Wed, 13 Nov 2013 15:40:40 -0800, Eric Dumazet wrote:
> On Wed, 2013-11-13 at 22:59 +0100, Holger Hoffstaette wrote:
>
>> +1: fixes spastic NFS & Samba throughput since 3.12.0 with r8169 and
>> e100e for me as well. It's really not just broken/weird Wifi cards that
>> are affected by this.
>
> Same remark, it would be nice to pinpoint the problematic driver.
Since I saw this with r8169->r8169 and e1000e->r8169 it's probably
everyone's favourite r8169 :)
Unfortunately I can't be more help but if you can suggest/whip up a fix
I'd be happy to help test.
-h
^ permalink raw reply
* [PATCH] net: sctp: bug-fixing: retran_path not set properly after transports recovering (v3)
From: Chang Xiangzhong @ 2013-11-13 23:58 UTC (permalink / raw)
To: vyasevich; +Cc: nhorman, davem, linux-sctp, netdev, Chang Xiangzhong
When a transport recovers due to the new coming sack, SCTP should
iterate all of its transport_list to locate the __two__ most recently used
transport and set to active_path and retran_path respectively. The exising
code does not find the two properly - In case of the following list:
[most-recent] -> [2nd-most-recent] -> ...
Both active_path and retran_path would be set to the 1st element.
The bug happens when:
1) multi-homing
2) failure/partial_failure transport recovers
Both active_path and retran_path would be set to the same most-recent one, in
other words, retran_path would not take its role - an end user might not even
notice this issue.
Signed-off-by: Chang Xiangzhong <changxiangzhong@gmail.com>
---
net/sctp/associola.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index ab67efc..8f26276 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -913,8 +913,8 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
if (!first || t->last_time_heard > first->last_time_heard) {
second = first;
first = t;
- }
- if (!second || t->last_time_heard > second->last_time_heard)
+ } else if (!second ||
+ t->last_time_heard > second->last_time_heard)
second = t;
}
@@ -935,6 +935,8 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
first = asoc->peer.primary_path;
}
+ if (!second)
+ second = first;
/* If we failed to find a usable transport, just camp on the
* primary, even if it is inactive.
*/
--
1.7.9.5
^ permalink raw reply related
* [PATCH net 0/6] net_tstamp: Validate hwtstamp_config completely before applying it
From: Ben Hutchings @ 2013-11-14 0:39 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Richard Cochran
This series fixes very similar bugs in 6 implementations of
SIOCSHWTSTAMP.
Ben.
Ben Hutchings (6):
tg3: Validate hwtstamp_config completely before applying it
e1000e: Validate hwtstamp_config completely before applying it
pch_gbe: Validate hwtstamp_config completely before applying it
stmmac: Validate hwtstamp_config completely before applying it
ti_cpsw: Validate hwtstamp_config completely before applying it
ixp4xx_eth: Validate hwtstamp_config completely before applying it
drivers/net/ethernet/broadcom/tg3.c | 16 +++++++---------
drivers/net/ethernet/intel/e1000e/netdev.c | 14 ++++++--------
drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 12 +++---------
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 12 +++---------
drivers/net/ethernet/ti/cpsw.c | 18 ++++++++----------
drivers/net/ethernet/xscale/ixp4xx_eth.c | 12 +++---------
6 files changed, 30 insertions(+), 54 deletions(-)
--
Ben Hutchings, Staff Engineer, Solarflare
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
* [PATCH net 1/6] tg3: Validate hwtstamp_config completely before applying it
From: Ben Hutchings @ 2013-11-14 0:40 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Richard Cochran, Nithin Nayak Sujir, Michael Chan
In-Reply-To: <1384389542.29151.5.camel@bwh-desktop.uk.level5networks.com>
tg3_hwtstamp_ioctl() should validate all fields of hwtstamp_config
before making any changes. Currently it sets the TX configuration
before validating the rx_filter field.
Compile-tested only.
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
drivers/net/ethernet/broadcom/tg3.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index 12d961c4ebca..d3a700f86f1a 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -13598,16 +13598,9 @@ static int tg3_hwtstamp_ioctl(struct net_device *dev,
if (stmpconf.flags)
return -EINVAL;
- switch (stmpconf.tx_type) {
- case HWTSTAMP_TX_ON:
- tg3_flag_set(tp, TX_TSTAMP_EN);
- break;
- case HWTSTAMP_TX_OFF:
- tg3_flag_clear(tp, TX_TSTAMP_EN);
- break;
- default:
+ if (stmpconf.tx_type != HWTSTAMP_TX_ON &&
+ stmpconf.tx_type != HWTSTAMP_TX_OFF)
return -ERANGE;
- }
switch (stmpconf.rx_filter) {
case HWTSTAMP_FILTER_NONE:
@@ -13669,6 +13662,11 @@ static int tg3_hwtstamp_ioctl(struct net_device *dev,
tw32(TG3_RX_PTP_CTL,
tp->rxptpctl | TG3_RX_PTP_CTL_HWTS_INTERLOCK);
+ if (stmpconf.tx_type == HWTSTAMP_TX_ON)
+ tg3_flag_set(tp, TX_TSTAMP_EN);
+ else
+ tg3_flag_clear(tp, TX_TSTAMP_EN);
+
return copy_to_user(ifr->ifr_data, &stmpconf, sizeof(stmpconf)) ?
-EFAULT : 0;
}
--
Ben Hutchings, Staff Engineer, Solarflare
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 related
* [PATCH net 2/6] e1000e: Validate hwtstamp_config completely before applying it
From: Ben Hutchings @ 2013-11-14 0:41 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Richard Cochran, e1000-devel
In-Reply-To: <1384389542.29151.5.camel@bwh-desktop.uk.level5networks.com>
e1000e_hwtstamp_ioctl() should validate all fields of hwtstamp_config
before making any changes. Currently it copies the configuration to
the e1000_adapter structure before validating it at all.
Change e1000e_config_hwtstamp() to take a pointer to the
hwstamp_config and to copy the config after validating it.
Compile-tested only.
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
drivers/net/ethernet/intel/e1000e/netdev.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 4ef786775acb..f02816575369 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -3482,10 +3482,10 @@ s32 e1000e_get_base_timinca(struct e1000_adapter *adapter, u32 *timinca)
* specified. Matching the kind of event packet is not supported, with the
* exception of "all V2 events regardless of level 2 or 4".
**/
-static int e1000e_config_hwtstamp(struct e1000_adapter *adapter)
+static int e1000e_config_hwtstamp(struct e1000_adapter *adapter,
+ struct hwtstamp_config *config)
{
struct e1000_hw *hw = &adapter->hw;
- struct hwtstamp_config *config = &adapter->hwtstamp_config;
u32 tsync_tx_ctl = E1000_TSYNCTXCTL_ENABLED;
u32 tsync_rx_ctl = E1000_TSYNCRXCTL_ENABLED;
u32 rxmtrl = 0;
@@ -3586,6 +3586,8 @@ static int e1000e_config_hwtstamp(struct e1000_adapter *adapter)
return -ERANGE;
}
+ adapter->hwtstamp_config = *config;
+
/* enable/disable Tx h/w time stamping */
regval = er32(TSYNCTXCTL);
regval &= ~E1000_TSYNCTXCTL_ENABLED;
@@ -3874,7 +3876,7 @@ void e1000e_reset(struct e1000_adapter *adapter)
e1000e_reset_adaptive(hw);
/* initialize systim and reset the ns time counter */
- e1000e_config_hwtstamp(adapter);
+ e1000e_config_hwtstamp(adapter, &adapter->hwtstamp_config);
/* Set EEE advertisement as appropriate */
if (adapter->flags2 & FLAG2_HAS_EEE) {
@@ -5797,14 +5799,10 @@ static int e1000e_hwtstamp_ioctl(struct net_device *netdev, struct ifreq *ifr)
if (copy_from_user(&config, ifr->ifr_data, sizeof(config)))
return -EFAULT;
- adapter->hwtstamp_config = config;
-
- ret_val = e1000e_config_hwtstamp(adapter);
+ ret_val = e1000e_config_hwtstamp(adapter, &config);
if (ret_val)
return ret_val;
- config = adapter->hwtstamp_config;
-
switch (config.rx_filter) {
case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
case HWTSTAMP_FILTER_PTP_V2_L2_SYNC:
--
Ben Hutchings, Staff Engineer, Solarflare
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 related
* [PATCH net 3/6] pch_gbe: Validate hwtstamp_config completely before applying it
From: Ben Hutchings @ 2013-11-14 0:42 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Richard Cochran
In-Reply-To: <1384389542.29151.5.camel@bwh-desktop.uk.level5networks.com>
hwtstamp_ioctl() should validate all fields of hwtstamp_config
before making any changes. Currently it sets the TX configuration
before validating the rx_filter field.
Compile-tested only.
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 12 +++---------
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
index 5a0f04c2c813..27ffe0ebf0a6 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
@@ -245,16 +245,8 @@ static int hwtstamp_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
/* Get ieee1588's dev information */
pdev = adapter->ptp_pdev;
- switch (cfg.tx_type) {
- case HWTSTAMP_TX_OFF:
- adapter->hwts_tx_en = 0;
- break;
- case HWTSTAMP_TX_ON:
- adapter->hwts_tx_en = 1;
- break;
- default:
+ if (cfg.tx_type != HWTSTAMP_TX_OFF && cfg.tx_type != HWTSTAMP_TX_ON)
return -ERANGE;
- }
switch (cfg.rx_filter) {
case HWTSTAMP_FILTER_NONE:
@@ -284,6 +276,8 @@ static int hwtstamp_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
return -ERANGE;
}
+ adapter->hwts_tx_en = cfg.tx_type == HWTSTAMP_TX_ON;
+
/* Clear out any old time stamps. */
pch_ch_event_write(pdev, TX_SNAPSHOT_LOCKED | RX_SNAPSHOT_LOCKED);
--
Ben Hutchings, Staff Engineer, Solarflare
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 related
* [PATCH net 4/6] stmmac: Validate hwtstamp_config completely before applying it
From: Ben Hutchings @ 2013-11-14 0:43 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Richard Cochran
In-Reply-To: <1384389542.29151.5.camel@bwh-desktop.uk.level5networks.com>
stmmac_hwtstamp_ioctl() should validate all fields of hwtstamp_config
before making any changes. Currently it sets the TX configuration
before validating the rx_filter field.
Compile-tested only.
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 12 +++---------
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 8d4ccd35a016..8a7a23a84ac5 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -435,16 +435,9 @@ static int stmmac_hwtstamp_ioctl(struct net_device *dev, struct ifreq *ifr)
if (config.flags)
return -EINVAL;
- switch (config.tx_type) {
- case HWTSTAMP_TX_OFF:
- priv->hwts_tx_en = 0;
- break;
- case HWTSTAMP_TX_ON:
- priv->hwts_tx_en = 1;
- break;
- default:
+ if (config.tx_type != HWTSTAMP_TX_OFF &&
+ config.tx_type != HWTSTAMP_TX_ON)
return -ERANGE;
- }
if (priv->adv_ts) {
switch (config.rx_filter) {
@@ -576,6 +569,7 @@ static int stmmac_hwtstamp_ioctl(struct net_device *dev, struct ifreq *ifr)
}
}
priv->hwts_rx_en = ((config.rx_filter == HWTSTAMP_FILTER_NONE) ? 0 : 1);
+ priv->hwts_tx_en = config.tx_type == HWTSTAMP_TX_ON;
if (!priv->hwts_tx_en && !priv->hwts_rx_en)
priv->hw->ptp->config_hw_tstamping(priv->ioaddr, 0);
--
Ben Hutchings, Staff Engineer, Solarflare
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 related
* [PATCH net 5/6] ti_cpsw: Validate hwtstamp_config completely before applying it
From: Ben Hutchings @ 2013-11-14 0:47 UTC (permalink / raw)
To: David Miller, Mugunthan V N, Felipe Balbi; +Cc: netdev, Richard Cochran
In-Reply-To: <1384389542.29151.5.camel@bwh-desktop.uk.level5networks.com>
cpsw_hwtstamp_ioctl() should validate all fields of hwtstamp_config,
and the hardware version, before making any changes. Currently it
sets the TX configuration before validating the rx_filter field
or that the hardware supports timestamping.
Also correct the error code for hardware versions that don't
support timestamping. ENOTSUPP is used by the NFS implementation
and is not part of userland API; we want EOPNOTSUPP (which glibc
also calls ENOTSUP, with one 'P').
Untested as I don't have a cross-compiler to hand.
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
drivers/net/ethernet/ti/cpsw.c | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)
diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index cc3ce557e4aa..8bda244df0fb 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -1321,6 +1321,10 @@ static int cpsw_hwtstamp_ioctl(struct net_device *dev, struct ifreq *ifr)
struct cpts *cpts = priv->cpts;
struct hwtstamp_config cfg;
+ if (priv->version != CPSW_VERSION_1 &&
+ priv->version != CPSW_VERSION_2)
+ return -EOPNOTSUPP;
+
if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg)))
return -EFAULT;
@@ -1328,16 +1332,8 @@ static int cpsw_hwtstamp_ioctl(struct net_device *dev, struct ifreq *ifr)
if (cfg.flags)
return -EINVAL;
- switch (cfg.tx_type) {
- case HWTSTAMP_TX_OFF:
- cpts->tx_enable = 0;
- break;
- case HWTSTAMP_TX_ON:
- cpts->tx_enable = 1;
- break;
- default:
+ if (cfg.tx_type != HWTSTAMP_TX_OFF && cfg.tx_type != HWTSTAMP_TX_ON)
return -ERANGE;
- }
switch (cfg.rx_filter) {
case HWTSTAMP_FILTER_NONE:
@@ -1364,6 +1360,8 @@ static int cpsw_hwtstamp_ioctl(struct net_device *dev, struct ifreq *ifr)
return -ERANGE;
}
+ cpts->tx_enable = cfg.tx_type == HWTSTAMP_TX_ON;
+
switch (priv->version) {
case CPSW_VERSION_1:
cpsw_hwtstamp_v1(priv);
@@ -1372,7 +1370,7 @@ static int cpsw_hwtstamp_ioctl(struct net_device *dev, struct ifreq *ifr)
cpsw_hwtstamp_v2(priv);
break;
default:
- return -ENOTSUPP;
+ WARN_ON(1);
}
return copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)) ? -EFAULT : 0;
--
Ben Hutchings, Staff Engineer, Solarflare
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 related
* [PATCH net 6/6] ixp4xx_eth: Validate hwtstamp_config completely before applying it
From: Ben Hutchings @ 2013-11-14 0:48 UTC (permalink / raw)
To: David Miller, Krzysztof Halasa; +Cc: netdev, Richard Cochran
In-Reply-To: <1384389542.29151.5.camel@bwh-desktop.uk.level5networks.com>
hwtstamp_ioctl() should validate all fields of hwtstamp_config
before making any changes. Currently it sets the TX configuration
before validating the rx_filter field.
Untested as I don't have a cross-compiler to hand.
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
drivers/net/ethernet/xscale/ixp4xx_eth.c | 12 +++---------
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/drivers/net/ethernet/xscale/ixp4xx_eth.c b/drivers/net/ethernet/xscale/ixp4xx_eth.c
index e78802e75ea6..bcc224a83734 100644
--- a/drivers/net/ethernet/xscale/ixp4xx_eth.c
+++ b/drivers/net/ethernet/xscale/ixp4xx_eth.c
@@ -389,16 +389,8 @@ static int hwtstamp_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
ch = PORT2CHANNEL(port);
regs = (struct ixp46x_ts_regs __iomem *) IXP4XX_TIMESYNC_BASE_VIRT;
- switch (cfg.tx_type) {
- case HWTSTAMP_TX_OFF:
- port->hwts_tx_en = 0;
- break;
- case HWTSTAMP_TX_ON:
- port->hwts_tx_en = 1;
- break;
- default:
+ if (cfg.tx_type != HWTSTAMP_TX_OFF && cfg.tx_type != HWTSTAMP_TX_ON)
return -ERANGE;
- }
switch (cfg.rx_filter) {
case HWTSTAMP_FILTER_NONE:
@@ -416,6 +408,8 @@ static int hwtstamp_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
return -ERANGE;
}
+ port->hwts_tx_en = cfg.tx_type == HWTSTAMP_TX_ON;
+
/* Clear out any old time stamps. */
__raw_writel(TX_SNAPSHOT_LOCKED | RX_SNAPSHOT_LOCKED,
®s->channel[ch].ch_event);
--
Ben Hutchings, Staff Engineer, Solarflare
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 related
* Re: [PATCH] rds: Error on offset mismatch if not loopback
From: Honggang LI @ 2013-11-14 0:55 UTC (permalink / raw)
To: Josh Hunt, Venkat Venkatsubra; +Cc: David Miller, jjolly, LKML, netdev
In-Reply-To: <CAKA=qzbK1YX5OrVQbwM=1xM6Si=GKv0UDofULkOC7RNo08zZPw@mail.gmail.com>
On 11/14/2013 01:40 AM, Josh Hunt wrote:
> On Wed, Nov 13, 2013 at 9:16 AM, Venkat Venkatsubra
> <venkat.x.venkatsubra@oracle.com> wrote:
>>
>> -----Original Message-----
>> From: Josh Hunt [mailto:joshhunt00@gmail.com]
>> Sent: Tuesday, November 12, 2013 10:25 PM
>> To: David Miller
>> Cc: jjolly@suse.com; LKML; Venkat Venkatsubra; netdev@vger.kernel.org
>> Subject: Re: [PATCH] rds: Error on offset mismatch if not loopback
>>
>> On Tue, Nov 12, 2013 at 10:22 PM, Josh Hunt <joshhunt00@gmail.com> wrote:
>>> On Sat, Sep 22, 2012 at 2:25 PM, David Miller <davem@davemloft.net> wrote:
>>>> From: John Jolly <jjolly@suse.com>
>>>> Date: Fri, 21 Sep 2012 15:32:40 -0600
>>>>
>>>>> Attempting an rds connection from the IP address of an IPoIB
>>>>> interface to itself causes a kernel panic due to a BUG_ON() being triggered.
>>>>> Making the test less strict allows rds-ping to work without
>>>>> crashing the machine.
>>>>>
>>>>> A local unprivileged user could use this flaw to crash the system.
>>>>>
>>>>> Signed-off-by: John Jolly <jjolly@suse.com>
>>>> Besides the questions being asked of you by Venkat Venkatsubra, this
>>>> patch has another issue.
>>>>
>>>> It has been completely corrupted by your email client, it has turned
>>>> all TAB characters into spaces, making the patch useless.
>>>>
>>>> Please learn how to send a patch unmolested in the body of your
>>>> email. Test it by emailing the patch to yourself, and verifying that
>>>> you can in fact apply the patch you receive in that email.
>>>> Then, and only then, should you consider making a new submission of
>>>> this patch.
>>>>
>>>> Use Documentation/email-clients.txt for guidance.
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe
>>>> linux-kernel" in the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>> Please read the FAQ at http://www.tux.org/lkml/
>>>
>>> I think this issue was lost in the shuffle. It appears that redhat,
>>> ubuntu, and oracle are maintaining local patches to resolve this:
>>>
>>> https://oss.oracle.com/git/?p=redpatch.git;a=commit;h=c7b6a0a1d8d63685
>>> 2be130fa15fa8be10d4704e8
>>> https://bugzilla.redhat.com/show_bug.cgi?id=822754
>>> http://ubuntu.5.x6.nabble.com/CVE-2012-2372-RDS-local-ping-DOS-td49853
>>> 88.html
>>>
>>> Given that Oracle has applied it I'll make the assumption that
>>> Venkat's question was answered at some point.
>>>
>>> David - I can resubmit the patch with the proper signed-off-by and
>>> formatting if you are willing to apply it unless John wants to try
>>> again. I think it's time this got upstream.
>>>
>>> --
>>> Josh
>> Ugh.. hopefully resending with all the html crap removed...
>>
>> --
>> Josh
>>
>> Hi Josh,
>>
>> No, I still didn't get an answer for how "off" could be non-zero in case of rds-ping to hit BUG_ON(off % RDS_FRAG_SIZE).
>> Because, rds-ping uses zero byte messages to ping.
>> If you have a test case that reproduces the kernel panic I can try it out and see how that can happen.
>> The Oracle's internal code I checked doesn't have that patch applied.
>>
>> Venkat
> No I don't have a test case. I came across this CVE while doing an
> audit and noticed it was patched in Ubuntu's kernel and other distros,
> but was not in the upstream kernel yet. Quick googling of lkml showed
> that there were at least two attempts to get this patch upstream, but
> both had issues due to not following the proper submission process:
>
> https://lkml.org/lkml/2012/10/22/433
> https://lkml.org/lkml/2012/9/21/505
>
> From my searching it appears the initial bug was found by someone at redhat:
> https://bugzilla.redhat.com/show_bug.cgi?id=822754
>
> I've added Li Honggang the reporter of this issue from Redhat to the
> mail. Hopefully he can share his testcase.
The test case is very simple:
Steps to Reproduce:
1. yum install -y rds-tools
2. [root@rdma3 ~]# ifconfig ib0 | grep 'inet addr'
inet addr:172.31.0.3 Bcast:172.31.0.255 Mask:255.255.255.0
3. [root@rdma3 ~]# /usr/bin/rds-ping 172.31.0.3 <<<< kernel panic (You
may need to wait for a few seconds before the kernel panic.)
>
> and possibly requires certain hardware as Jay writes in the first link above:
> "...some Infiniband HCAs(QLogic, possibly others) the machine will panic..."
This bug can be reproduced with Mellanox HCAs (mlx4_ib.ko and mthca.ko),
QLogic HCA (ib_qib.ko). I did not test the QLogic HCA running "ib_ipath.ko".
As I know the upstream code of RDS is broken. There are *many* RDS bugs.
Best regards.
Honggang
>
> I was referring to this oracle commit:
> https://oss.oracle.com/git/?p=redpatch.git;a=commit;h=c7b6a0a1d8d636852be130fa15fa8be10d4704e8
>
> I have no experience with this code. There were a few comments around
> the reset and xmit fns about making sure the caller did certain things
> if not they were racy, but I have no idea if that's coming into play
> here.
>
^ permalink raw reply
* Re: [PATCH net 1/6] tg3: Validate hwtstamp_config completely before applying it
From: Nithin Nayak Sujir @ 2013-11-14 0:55 UTC (permalink / raw)
To: Ben Hutchings, David Miller; +Cc: netdev, Richard Cochran, Michael Chan
In-Reply-To: <1384389656.29151.7.camel@bwh-desktop.uk.level5networks.com>
On 11/13/2013 04:40 PM, Ben Hutchings wrote:
> tg3_hwtstamp_ioctl() should validate all fields of hwtstamp_config
> before making any changes. Currently it sets the TX configuration
> before validating the rx_filter field.
>
> Compile-tested only.
>
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
> ---
> drivers/net/ethernet/broadcom/tg3.c | 16 +++++++---------
> 1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
> index 12d961c4ebca..d3a700f86f1a 100644
> --- a/drivers/net/ethernet/broadcom/tg3.c
> +++ b/drivers/net/ethernet/broadcom/tg3.c
> @@ -13598,16 +13598,9 @@ static int tg3_hwtstamp_ioctl(struct net_device *dev,
> if (stmpconf.flags)
> return -EINVAL;
>
> - switch (stmpconf.tx_type) {
> - case HWTSTAMP_TX_ON:
> - tg3_flag_set(tp, TX_TSTAMP_EN);
> - break;
> - case HWTSTAMP_TX_OFF:
> - tg3_flag_clear(tp, TX_TSTAMP_EN);
> - break;
> - default:
> + if (stmpconf.tx_type != HWTSTAMP_TX_ON &&
> + stmpconf.tx_type != HWTSTAMP_TX_OFF)
> return -ERANGE;
> - }
>
> switch (stmpconf.rx_filter) {
> case HWTSTAMP_FILTER_NONE:
> @@ -13669,6 +13662,11 @@ static int tg3_hwtstamp_ioctl(struct net_device *dev,
> tw32(TG3_RX_PTP_CTL,
> tp->rxptpctl | TG3_RX_PTP_CTL_HWTS_INTERLOCK);
>
> + if (stmpconf.tx_type == HWTSTAMP_TX_ON)
> + tg3_flag_set(tp, TX_TSTAMP_EN);
> + else
> + tg3_flag_clear(tp, TX_TSTAMP_EN);
> +
> return copy_to_user(ifr->ifr_data, &stmpconf, sizeof(stmpconf)) ?
> -EFAULT : 0;
> }
>
>
Thanks Ben.
Acked-by: Nithin Nayak Sujir <nsujir@broadcom.com>
^ permalink raw reply
* [PATCH] rds: fix local ping DoS
From: Josh Hunt @ 2013-11-14 1:15 UTC (permalink / raw)
To: netdev, venkat.x.venkatsubra
Cc: linux-kernel, jjolly, fenlason, honli, Josh Hunt
The rds_ib_xmit function in net/rds/ib_send.c in the Reliable Datagram Sockets
(RDS) protocol implementation allows local users to cause a denial of service
(BUG_ON and kernel panic) by establishing an RDS connection with the source
IP address equal to the IPoIB interface's own IP address, as demonstrated by
rds-ping.
A local unprivileged user could use this flaw to crash the system.
CVE-2012-2372
Reported-by: Honggang Li <honli@redhat.com>
Signed-off-by: Josh Hunt <johunt@akamai.com>
---
net/rds/ib_send.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/net/rds/ib_send.c b/net/rds/ib_send.c
index e590949..7920c85 100644
--- a/net/rds/ib_send.c
+++ b/net/rds/ib_send.c
@@ -544,7 +544,7 @@ int rds_ib_xmit(struct rds_connection *conn, struct rds_message *rm,
int flow_controlled = 0;
int nr_sig = 0;
- BUG_ON(off % RDS_FRAG_SIZE);
+ BUG_ON(!conn->c_loopback && off % RDS_FRAG_SIZE);
BUG_ON(hdr_off != 0 && hdr_off != sizeof(struct rds_header));
/* Do not send cong updates to IB loopback */
--
1.7.0.4
^ permalink raw reply related
* Re: [PATCH] rds: Error on offset mismatch if not loopback
From: Josh Hunt @ 2013-11-14 1:27 UTC (permalink / raw)
To: Honggang LI; +Cc: Venkat Venkatsubra, David Miller, jjolly, LKML, netdev
In-Reply-To: <52841F95.7040204@redhat.com>
On Wed, Nov 13, 2013 at 6:55 PM, Honggang LI <honli@redhat.com> wrote:
> On 11/14/2013 01:40 AM, Josh Hunt wrote:
>> On Wed, Nov 13, 2013 at 9:16 AM, Venkat Venkatsubra
>> <venkat.x.venkatsubra@oracle.com> wrote:
>>>
>>> -----Original Message-----
>>> From: Josh Hunt [mailto:joshhunt00@gmail.com]
>>> Sent: Tuesday, November 12, 2013 10:25 PM
>>> To: David Miller
>>> Cc: jjolly@suse.com; LKML; Venkat Venkatsubra; netdev@vger.kernel.org
>>> Subject: Re: [PATCH] rds: Error on offset mismatch if not loopback
>>>
>>> On Tue, Nov 12, 2013 at 10:22 PM, Josh Hunt <joshhunt00@gmail.com> wrote:
>>>> On Sat, Sep 22, 2012 at 2:25 PM, David Miller <davem@davemloft.net> wrote:
>>>>> From: John Jolly <jjolly@suse.com>
>>>>> Date: Fri, 21 Sep 2012 15:32:40 -0600
>>>>>
>>>>>> Attempting an rds connection from the IP address of an IPoIB
>>>>>> interface to itself causes a kernel panic due to a BUG_ON() being triggered.
>>>>>> Making the test less strict allows rds-ping to work without
>>>>>> crashing the machine.
>>>>>>
>>>>>> A local unprivileged user could use this flaw to crash the system.
>>>>>>
>>>>>> Signed-off-by: John Jolly <jjolly@suse.com>
>>>>> Besides the questions being asked of you by Venkat Venkatsubra, this
>>>>> patch has another issue.
>>>>>
>>>>> It has been completely corrupted by your email client, it has turned
>>>>> all TAB characters into spaces, making the patch useless.
>>>>>
>>>>> Please learn how to send a patch unmolested in the body of your
>>>>> email. Test it by emailing the patch to yourself, and verifying that
>>>>> you can in fact apply the patch you receive in that email.
>>>>> Then, and only then, should you consider making a new submission of
>>>>> this patch.
>>>>>
>>>>> Use Documentation/email-clients.txt for guidance.
>>>>> --
>>>>> To unsubscribe from this list: send the line "unsubscribe
>>>>> linux-kernel" in the body of a message to majordomo@vger.kernel.org
>>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>> Please read the FAQ at http://www.tux.org/lkml/
>>>>
>>>> I think this issue was lost in the shuffle. It appears that redhat,
>>>> ubuntu, and oracle are maintaining local patches to resolve this:
>>>>
>>>> https://oss.oracle.com/git/?p=redpatch.git;a=commit;h=c7b6a0a1d8d63685
>>>> 2be130fa15fa8be10d4704e8
>>>> https://bugzilla.redhat.com/show_bug.cgi?id=822754
>>>> http://ubuntu.5.x6.nabble.com/CVE-2012-2372-RDS-local-ping-DOS-td49853
>>>> 88.html
>>>>
>>>> Given that Oracle has applied it I'll make the assumption that
>>>> Venkat's question was answered at some point.
>>>>
>>>> David - I can resubmit the patch with the proper signed-off-by and
>>>> formatting if you are willing to apply it unless John wants to try
>>>> again. I think it's time this got upstream.
>>>>
>>>> --
>>>> Josh
>>> Ugh.. hopefully resending with all the html crap removed...
>>>
>>> --
>>> Josh
>>>
>>> Hi Josh,
>>>
>>> No, I still didn't get an answer for how "off" could be non-zero in case of rds-ping to hit BUG_ON(off % RDS_FRAG_SIZE).
>>> Because, rds-ping uses zero byte messages to ping.
>>> If you have a test case that reproduces the kernel panic I can try it out and see how that can happen.
>>> The Oracle's internal code I checked doesn't have that patch applied.
>>>
>>> Venkat
>> No I don't have a test case. I came across this CVE while doing an
>> audit and noticed it was patched in Ubuntu's kernel and other distros,
>> but was not in the upstream kernel yet. Quick googling of lkml showed
>> that there were at least two attempts to get this patch upstream, but
>> both had issues due to not following the proper submission process:
>>
>> https://lkml.org/lkml/2012/10/22/433
>> https://lkml.org/lkml/2012/9/21/505
>>
>> From my searching it appears the initial bug was found by someone at redhat:
>> https://bugzilla.redhat.com/show_bug.cgi?id=822754
>>
>> I've added Li Honggang the reporter of this issue from Redhat to the
>> mail. Hopefully he can share his testcase.
> The test case is very simple:
> Steps to Reproduce:
> 1. yum install -y rds-tools
>
> 2. [root@rdma3 ~]# ifconfig ib0 | grep 'inet addr'
> inet addr:172.31.0.3 Bcast:172.31.0.255 Mask:255.255.255.0
>
> 3. [root@rdma3 ~]# /usr/bin/rds-ping 172.31.0.3 <<<< kernel panic (You
> may need to wait for a few seconds before the kernel panic.)
>>
>> and possibly requires certain hardware as Jay writes in the first link above:
>> "...some Infiniband HCAs(QLogic, possibly others) the machine will panic..."
> This bug can be reproduced with Mellanox HCAs (mlx4_ib.ko and mthca.ko),
> QLogic HCA (ib_qib.ko). I did not test the QLogic HCA running "ib_ipath.ko".
>
> As I know the upstream code of RDS is broken. There are *many* RDS bugs.
>
> Best regards.
> Honggang
Thanks Honggang. I have resubmitted the patch for approval.
--
Josh
^ permalink raw reply
* Re: [PATCH] net: sctp: bug-fixing: retran_path not set properly after transports recovering (v3)
From: Vlad Yasevich @ 2013-11-14 1:42 UTC (permalink / raw)
To: Chang Xiangzhong; +Cc: nhorman, davem, linux-sctp, netdev
In-Reply-To: <1384387106-8105-1-git-send-email-changxiangzhong@gmail.com>
On 11/13/2013 06:58 PM, Chang Xiangzhong wrote:
> When a transport recovers due to the new coming sack, SCTP should
> iterate all of its transport_list to locate the __two__ most recently used
> transport and set to active_path and retran_path respectively. The exising
> code does not find the two properly - In case of the following list:
>
> [most-recent] -> [2nd-most-recent] -> ...
>
> Both active_path and retran_path would be set to the 1st element.
>
> The bug happens when:
> 1) multi-homing
> 2) failure/partial_failure transport recovers
> Both active_path and retran_path would be set to the same most-recent one, in
> other words, retran_path would not take its role - an end user might not even
> notice this issue.
>
> Signed-off-by: Chang Xiangzhong <changxiangzhong@gmail.com>
Acked-by: Vlad Yasevich <vyasevich@gmail.com>
-vlad
> ---
> net/sctp/associola.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> index ab67efc..8f26276 100644
> --- a/net/sctp/associola.c
> +++ b/net/sctp/associola.c
> @@ -913,8 +913,8 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
> if (!first || t->last_time_heard > first->last_time_heard) {
> second = first;
> first = t;
> - }
> - if (!second || t->last_time_heard > second->last_time_heard)
> + } else if (!second ||
> + t->last_time_heard > second->last_time_heard)
> second = t;
> }
>
> @@ -935,6 +935,8 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
> first = asoc->peer.primary_path;
> }
>
> + if (!second)
> + second = first;
> /* If we failed to find a usable transport, just camp on the
> * primary, even if it is inactive.
> */
>
^ permalink raw reply
* RE: [PATCH v2 1/1] net:fec: fix WARNING caused by lack of calls to dma_mapping_error()
From: Fugang Duan @ 2013-11-14 1:47 UTC (permalink / raw)
To: Ben Hutchings; +Cc: davem@davemloft.net, netdev@vger.kernel.org, Frank Li
In-Reply-To: <1384378020.29151.3.camel@bwh-desktop.uk.level5networks.com>
From: Ben Hutchings [mailto:bhutchings@solarflare.com]
Data: Thursday, November 14, 2013 5:27 AM
>To: Duan Fugang-B38611
>Cc: davem@davemloft.net; netdev@vger.kernel.org; Li Frank-B20596
>Subject: Re: [PATCH v2 1/1] net:fec: fix WARNING caused by lack of calls to
>dma_mapping_error()
>
>On Wed, 2013-11-13 at 11:04 +0800, Fugang Duan wrote:
>[...]
>> =============================================
>> V2: add net_ratelimit() to limit map err message.
>> use dma_sync_single_for_cpu() instead of dma_map_single().
>> fix the issue that pass DMA addresses to __va() to get virture address.
>> V1: initial send
>> =============================================
>>
>> Signed-off-by: Fugang Duan <B38611@freescale.com>
>> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
>[...]
>
>No that's not how it works. If I were to thoroughly review this (but I haven't,
>and I don't have time to) then I might say 'Reviewed-by: Ben Hutchings' which
>you could then add to the patch if re-posting it.
>Signed-off-by means something different and you must never add it on behalf of
>someone else.
>
>Ben.
You had supported many ideas for the patch V2, so add you to Signed-off.
Ok, I change it to "Reveiewed-by" and resend the patch.
Thanks,
Andy
>
>--
>Ben Hutchings, Staff Engineer, Solarflare 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: [PATCH net] bonding: fix two race conditions in bond_store_updelay/downdelay
From: Ding Tianhong @ 2013-11-14 1:50 UTC (permalink / raw)
To: Nikolay Aleksandrov, Eric Dumazet
Cc: netdev, davem, Jay Vosburgh, Andy Gospodarek, Veaceslav Falico
In-Reply-To: <5283B19A.9050302@redhat.com>
On 2013/11/14 1:06, Nikolay Aleksandrov wrote:
> On 11/13/2013 06:03 PM, Eric Dumazet wrote:
>> On Wed, 2013-11-13 at 17:07 +0100, Nikolay Aleksandrov wrote:
>>> This patch fixes two race conditions between bond_store_updelay/downdelay
>>> and bond_store_miimon which could lead to division by zero as miimon can
>>> be set to 0 while either updelay/downdelay are being set and thus miss the
>>> zero check in the beginning, the zero div happens because updelay/downdelay
>>> are stored as new_value / bond->params.miimon. Use rtnl to synchronize with
>>> miimon setting.
>>
>> It seems a bit heavy duty to take rtnl for this.
>>
>> Using ACCESS_ONCE() in bonding_store_updelay()/bonding_store_downdelay()
>> should be enough ?
>>
>> int miimon = ACCESS_ONCE(bond->params.miimon);
>>
>>
>>
>> --
>> 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
>>
> Hi Eric,
> I thought about this version too, but downdelay/updelay can be changed in other
> places (e.g., store_miimon) and the resulting downdelay/updelay value might not
> be the right one.
> Correct me if I'm wrong, but this is what I have in mind (miimon = 100, updelay
> = 200):
> set miimon to 300 and concurrently set updelay to 400, we might endup leaving
> updelay to 400 because the old value of miimon is used in the calculation in
> store_updelay even though when changing miimon updelay/downdelay get adjusted,
> they might get adjusted by store_updelay/downdelay to a wrong value afterwards.
>
> Nik
>
>
agree, set miimon and set updelay may conflict.
>
>
> --
> 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
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