Netdev List
 help / color / mirror / Atom feed
* Re: [PATCH] netfilter: nf_conntrack: fix tcp_in_window for Fast Open
From: Jozsef Kadlecsik @ 2013-08-10 13:01 UTC (permalink / raw)
  To: Yuchung Cheng; +Cc: pablo, netfilter-devel, netdev, edumazet
In-Reply-To: <1376094087-17700-1-git-send-email-ycheng@google.com>

On Fri, 9 Aug 2013, Yuchung Cheng wrote:

> Currently the conntrack checks if the ending sequence of a packet
> falls within the observed receive window. However it does so even
> if it has not observe any packet from the remote yet and uses an
> uninitialized receive window (td_maxwin).
> 
> If a connection uses Fast Open to send a SYN-data packet which is
> dropped afterward in the network. The subsequent SYNs retransmits
> will all fail this check and be discarded, leading to a connection
> timeout. This is because the SYN retransmit does not contain data
> payload so
> 
> end == initial sequence number (isn) + 1
> sender->td_end == isn + syn_data_len
> receiver->td_maxwin == 0
> 
> The fix is to only apply this check after td_maxwin is initialized.
> 
> Reported-by: Michael Chan <mcfchan@stanford.edu>
> Signed-off-by: Yuchung Cheng <ycheng@google.com>

Acked-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>

> ---
>  net/netfilter/nf_conntrack_proto_tcp.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
> index 4d4d8f1..e0f9a32 100644
> --- a/net/netfilter/nf_conntrack_proto_tcp.c
> +++ b/net/netfilter/nf_conntrack_proto_tcp.c
> @@ -526,7 +526,7 @@ static bool tcp_in_window(const struct nf_conn *ct,
>  	const struct nf_conntrack_tuple *tuple = &ct->tuplehash[dir].tuple;
>  	__u32 seq, ack, sack, end, win, swin;
>  	s16 receiver_offset;
> -	bool res;
> +	bool res, in_recv_win;
>  
>  	/*
>  	 * Get the required data from the packet.
> @@ -649,14 +649,18 @@ static bool tcp_in_window(const struct nf_conn *ct,
>  		 receiver->td_end, receiver->td_maxend, receiver->td_maxwin,
>  		 receiver->td_scale);
>  
> +	/* Is the ending sequence in the receive window (if available)? */
> +	in_recv_win = !receiver->td_maxwin ||
> +		      after(end, sender->td_end - receiver->td_maxwin - 1);
> +
>  	pr_debug("tcp_in_window: I=%i II=%i III=%i IV=%i\n",
>  		 before(seq, sender->td_maxend + 1),
> -		 after(end, sender->td_end - receiver->td_maxwin - 1),
> +		 (in_recv_win ? 1 : 0),
>  		 before(sack, receiver->td_end + 1),
>  		 after(sack, receiver->td_end - MAXACKWINDOW(sender) - 1));
>  
>  	if (before(seq, sender->td_maxend + 1) &&
> -	    after(end, sender->td_end - receiver->td_maxwin - 1) &&
> +	    in_recv_win &&
>  	    before(sack, receiver->td_end + 1) &&
>  	    after(sack, receiver->td_end - MAXACKWINDOW(sender) - 1)) {
>  		/*
> @@ -725,7 +729,7 @@ static bool tcp_in_window(const struct nf_conn *ct,
>  			nf_log_packet(net, pf, 0, skb, NULL, NULL, NULL,
>  			"nf_ct_tcp: %s ",
>  			before(seq, sender->td_maxend + 1) ?
> -			after(end, sender->td_end - receiver->td_maxwin - 1) ?
> +			in_recv_win ?
>  			before(sack, receiver->td_end + 1) ?
>  			after(sack, receiver->td_end - MAXACKWINDOW(sender) - 1) ? "BUG"
>  			: "ACK is under the lower bound (possible overly delayed ACK)"
> -- 
> 1.8.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
          H-1525 Budapest 114, POB. 49, Hungary

^ permalink raw reply

* Re: [PATCH net] ipv6: don't stop backtracking in fib6_lookup_1 if subtree does not match
From: Hannes Frederic Sowa @ 2013-08-10 12:15 UTC (permalink / raw)
  To: Teco Boot; +Cc: David Miller, netdev
In-Reply-To: <2A0809D4-EBF1-4646-9564-2B7F54956B41@inf-net.nl>

On Sat, Aug 10, 2013 at 01:39:12PM +0200, Teco Boot wrote:
> The hang problem in 3.8.0-28 has lower priority for me. Maybe I'll check current -net-next later. Don't hold your breath.

Ah, one more thing: Please don't test net-next but net
<https://git.kernel.org/cgit/linux/kernel/git/davem/net.git/>.

All relevant patches are in there. They will first show up in net-next when
David merges net back to net-next. That's because the other patches are also
queued up for 3.11 inclusion.

Greetings,

  Hannes

^ permalink raw reply

* Re: [PATCH net] ipv6: don't stop backtracking in fib6_lookup_1 if subtree does not match
From: Hannes Frederic Sowa @ 2013-08-10 12:03 UTC (permalink / raw)
  To: Teco Boot; +Cc: David Miller, netdev
In-Reply-To: <2A0809D4-EBF1-4646-9564-2B7F54956B41@inf-net.nl>

On Sat, Aug 10, 2013 at 01:39:12PM +0200, Teco Boot wrote:
> OK, I went back to kernel where I had the ipv6 sadr lookup in cache problem.
> There I patched the ip6_fib.c.
> Patch works !!
> 
> The hang problem in 3.8.0-28 has lower priority for me. Maybe I'll check current -net-next later. Don't hold your breath.
> 
> Now this patch should go mainline and older kernels, I think.
> What is the next step?

That already happend. Patch is queued up for inclusion in 3.11 and is sitting
in the stable queue:
<http://patchwork.ozlabs.org/bundle/davem/stable/?state=*>

Thanks for testing,

  Hannes

^ permalink raw reply

* Re: [PATCH net] skge: add dma_mapping check
From: poma @ 2013-08-10 11:51 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: David Miller, netdev
In-Reply-To: <20130804.183553.514470399074672614.davem@davemloft.net>

[-- Attachment #1: Type: text/plain, Size: 4529 bytes --]

On 05.08.2013 03:35, David Miller wrote:
> From: Stephen Hemminger <stephen@networkplumber.org>
> Date: Sun, 4 Aug 2013 17:22:34 -0700
> 
>> This old driver never checked for DMA mapping errors.
>> Causing splats with the new DMA mapping checks:
>> 	WARNING: at lib/dma-debug.c:937 check_unmap+0x47b/0x930()
>> 	skge 0000:01:09.0: DMA-API: device driver failed to check map
>>
>> Add checks and unwind code.
>>
>> Reported-by: poma <pomidorabelisima@gmail.com>
>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> 
> Applied, but:
> 
>> -static void skge_rx_setup(struct skge_port *skge, struct skge_element *e,
>> +static int skge_rx_setup(struct skge_port *skge, struct skge_element *e,
>>  			  struct sk_buff *skb, unsigned int bufsize)
> 
> I reflowed the argument indentation for this in the final commit.
> 
> Thanks.
> 

skge-add-dma_mapping-check.patch
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/patch/drivers/net/ethernet/marvell?id=136d8f377e1575463b47840bc5f1b22d94bf8f63

/usr/lib/modules/3.11.0-0.rc4.git2.1.fc20.x86_64/updates/skge.ko

skge: module verification failed: signature and/or required key missing
- tainting kernel
skge: 1.14 addr 0xfbffc000 irq 19 chip Yukon rev 1
skge 0000:01:09.0 eth0: addr nn:nn:nn:nn:nn:nn
skge 0000:01:09.0 enp1s9: enabling interface
skge 0000:01:09.0 enp1s9: Link is up at 1000 Mbps, full duplex, flow
control both
IPv6: ADDRCONF(NETDEV_CHANGE): enp1s9: link becomes ready

Starting Nmap 6.25

------------[ cut here ]------------
WARNING: CPU: 2 PID: 2443 at lib/dma-debug.c:986 check_sync+0x4bc/0x580()
skge 0000:01:09.0: DMA-API: device driver tries to sync DMA memory it
has not allocated [device address=0x00000000cf390040] [size=60 bytes]
Modules linked in: skge(OF) raid1 nouveau video mxm_wmi i2c_algo_bit
drm_kms_helper ttm drm i2c_core wmi
CPU: 2 PID: 2443 Comm: nmap Tainted: GF          O
3.11.0-0.rc4.git2.1.fc20.x86_64 #1
Hardware name: Gigabyte Technology Co., Ltd. M720-US3/M720-US3, BIOS F7n
09/07/2010
 0000000000000009 ffff88012aa03c48 ffffffff81723ac3 ffff88012aa03c90
 ffff88012aa03c80 ffffffff8107462d ffff88012837c2b0 000000000000003c
 ffff880128363470 00000000cf390040 ffff880126f79160 ffff88012aa03ce0
Call Trace:
 <IRQ>  [<ffffffff81723ac3>] dump_stack+0x54/0x74
 [<ffffffff8107462d>] warn_slowpath_common+0x7d/0xa0
 [<ffffffff8107469c>] warn_slowpath_fmt+0x4c/0x50
 [<ffffffff81392a5c>] check_sync+0x4bc/0x580
 [<ffffffff8138519e>] ? debug_check_no_obj_freed+0x14e/0x250
 [<ffffffff81392b6b>] debug_dma_sync_single_for_cpu+0x4b/0x50
 [<ffffffff810e6b6c>] ? trace_hardirqs_on_caller+0xac/0x1c0
 [<ffffffff815d9390>] ? build_skb+0x30/0x1d0
 [<ffffffff815db349>] ? __netdev_alloc_skb+0x89/0xf0
 [<ffffffffa01c7e21>] skge_poll+0x3a1/0x9f0 [skge]
 [<ffffffff815edb41>] ? net_rx_action+0xa1/0x380
 [<ffffffff815edc12>] net_rx_action+0x172/0x380
 [<ffffffff8107b4a7>] __do_softirq+0x107/0x410
 [<ffffffff8107b985>] irq_exit+0xc5/0xd0
 [<ffffffff81738c16>] do_IRQ+0x56/0xc0
 [<ffffffff8172d432>] common_interrupt+0x72/0x72
 <EOI>  [<ffffffff81721f92>] ? __slab_alloc+0x4c2/0x526
 [<ffffffff810e6bbd>] ? trace_hardirqs_on_caller+0xfd/0x1c0
 [<ffffffff815db08e>] ? __alloc_skb+0x7e/0x2b0
 [<ffffffff811d2d71>] __kmalloc_node_track_caller+0x1a1/0x410
 [<ffffffff815db08e>] ? __alloc_skb+0x7e/0x2b0
 [<ffffffff815da8a1>] __kmalloc_reserve.isra.25+0x31/0x90
 [<ffffffff815db08e>] __alloc_skb+0x7e/0x2b0
 [<ffffffff815d754e>] sock_alloc_send_pskb+0x27e/0x400
 [<ffffffff815d76e5>] sock_alloc_send_skb+0x15/0x20
 [<ffffffff816614af>] raw_sendmsg+0x74f/0xc50
 [<ffffffff81660e7d>] ? raw_sendmsg+0x11d/0xc50
 [<ffffffff8130246d>] ? avc_has_perm_flags+0x16d/0x350
 [<ffffffff81302329>] ? avc_has_perm_flags+0x29/0x350
 [<ffffffff810b797f>] ? local_clock+0x5f/0x70
 [<ffffffff810e3fcf>] ? lock_release_holdtime.part.28+0xf/0x1a0
 [<ffffffff816723a7>] inet_sendmsg+0x117/0x230
 [<ffffffff81672295>] ? inet_sendmsg+0x5/0x230
 [<ffffffff815d0939>] sock_sendmsg+0x99/0xd0
 [<ffffffff810e346d>] ? trace_hardirqs_off+0xd/0x10
 [<ffffffff810e9738>] ? lock_release_non_nested+0x308/0x350
 [<ffffffff811312e7>] ? rcu_irq_exit+0x77/0xc0
 [<ffffffff8172d4f3>] ? retint_restore_args+0x13/0x13
 [<ffffffff815d0e94>] SYSC_sendto+0x124/0x1d0
 [<ffffffff817369c5>] ? sysret_check+0x22/0x5d
 [<ffffffff810e6bbd>] ? trace_hardirqs_on_caller+0xfd/0x1c0
 [<ffffffff8137af2e>] ? trace_hardirqs_on_thunk+0x3a/0x3f
 [<ffffffff815d1ffe>] SyS_sendto+0xe/0x10
 [<ffffffff81736999>] system_call_fastpath+0x16/0x1b
---[ end trace ef4521ca4028fd28 ]---


poma




[-- Attachment #2: skge-add-dma_mapping-check-3.11.0-0.rc4.git2.1.fc20.x86_64.txt --]
[-- Type: text/plain, Size: 3680 bytes --]


skge-add-dma_mapping-check.patch
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/patch/drivers/net/ethernet/marvell?id=136d8f377e1575463b47840bc5f1b22d94bf8f63

/usr/lib/modules/3.11.0-0.rc4.git2.1.fc20.x86_64/updates/skge.ko

skge: module verification failed: signature and/or required key missing - tainting kernel
skge: 1.14 addr 0xfbffc000 irq 19 chip Yukon rev 1
skge 0000:01:09.0 eth0: addr nn:nn:nn:nn:nn:nn
skge 0000:01:09.0 enp1s9: enabling interface
skge 0000:01:09.0 enp1s9: Link is up at 1000 Mbps, full duplex, flow control both
IPv6: ADDRCONF(NETDEV_CHANGE): enp1s9: link becomes ready

Starting Nmap 6.25 

------------[ cut here ]------------
WARNING: CPU: 2 PID: 2443 at lib/dma-debug.c:986 check_sync+0x4bc/0x580()
skge 0000:01:09.0: DMA-API: device driver tries to sync DMA memory it has not allocated [device address=0x00000000cf390040] [size=60 bytes]
Modules linked in: skge(OF) raid1 nouveau video mxm_wmi i2c_algo_bit drm_kms_helper ttm drm i2c_core wmi
CPU: 2 PID: 2443 Comm: nmap Tainted: GF          O 3.11.0-0.rc4.git2.1.fc20.x86_64 #1
Hardware name: Gigabyte Technology Co., Ltd. M720-US3/M720-US3, BIOS F7n 09/07/2010
 0000000000000009 ffff88012aa03c48 ffffffff81723ac3 ffff88012aa03c90
 ffff88012aa03c80 ffffffff8107462d ffff88012837c2b0 000000000000003c
 ffff880128363470 00000000cf390040 ffff880126f79160 ffff88012aa03ce0
Call Trace:
 <IRQ>  [<ffffffff81723ac3>] dump_stack+0x54/0x74
 [<ffffffff8107462d>] warn_slowpath_common+0x7d/0xa0
 [<ffffffff8107469c>] warn_slowpath_fmt+0x4c/0x50
 [<ffffffff81392a5c>] check_sync+0x4bc/0x580
 [<ffffffff8138519e>] ? debug_check_no_obj_freed+0x14e/0x250
 [<ffffffff81392b6b>] debug_dma_sync_single_for_cpu+0x4b/0x50
 [<ffffffff810e6b6c>] ? trace_hardirqs_on_caller+0xac/0x1c0
 [<ffffffff815d9390>] ? build_skb+0x30/0x1d0
 [<ffffffff815db349>] ? __netdev_alloc_skb+0x89/0xf0
 [<ffffffffa01c7e21>] skge_poll+0x3a1/0x9f0 [skge]
 [<ffffffff815edb41>] ? net_rx_action+0xa1/0x380
 [<ffffffff815edc12>] net_rx_action+0x172/0x380
 [<ffffffff8107b4a7>] __do_softirq+0x107/0x410
 [<ffffffff8107b985>] irq_exit+0xc5/0xd0
 [<ffffffff81738c16>] do_IRQ+0x56/0xc0
 [<ffffffff8172d432>] common_interrupt+0x72/0x72
 <EOI>  [<ffffffff81721f92>] ? __slab_alloc+0x4c2/0x526
 [<ffffffff810e6bbd>] ? trace_hardirqs_on_caller+0xfd/0x1c0
 [<ffffffff815db08e>] ? __alloc_skb+0x7e/0x2b0
 [<ffffffff811d2d71>] __kmalloc_node_track_caller+0x1a1/0x410
 [<ffffffff815db08e>] ? __alloc_skb+0x7e/0x2b0
 [<ffffffff815da8a1>] __kmalloc_reserve.isra.25+0x31/0x90
 [<ffffffff815db08e>] __alloc_skb+0x7e/0x2b0
 [<ffffffff815d754e>] sock_alloc_send_pskb+0x27e/0x400
 [<ffffffff815d76e5>] sock_alloc_send_skb+0x15/0x20
 [<ffffffff816614af>] raw_sendmsg+0x74f/0xc50
 [<ffffffff81660e7d>] ? raw_sendmsg+0x11d/0xc50
 [<ffffffff8130246d>] ? avc_has_perm_flags+0x16d/0x350
 [<ffffffff81302329>] ? avc_has_perm_flags+0x29/0x350
 [<ffffffff810b797f>] ? local_clock+0x5f/0x70
 [<ffffffff810e3fcf>] ? lock_release_holdtime.part.28+0xf/0x1a0
 [<ffffffff816723a7>] inet_sendmsg+0x117/0x230
 [<ffffffff81672295>] ? inet_sendmsg+0x5/0x230
 [<ffffffff815d0939>] sock_sendmsg+0x99/0xd0
 [<ffffffff810e346d>] ? trace_hardirqs_off+0xd/0x10
 [<ffffffff810e9738>] ? lock_release_non_nested+0x308/0x350
 [<ffffffff811312e7>] ? rcu_irq_exit+0x77/0xc0
 [<ffffffff8172d4f3>] ? retint_restore_args+0x13/0x13
 [<ffffffff815d0e94>] SYSC_sendto+0x124/0x1d0
 [<ffffffff817369c5>] ? sysret_check+0x22/0x5d
 [<ffffffff810e6bbd>] ? trace_hardirqs_on_caller+0xfd/0x1c0
 [<ffffffff8137af2e>] ? trace_hardirqs_on_thunk+0x3a/0x3f
 [<ffffffff815d1ffe>] SyS_sendto+0xe/0x10
 [<ffffffff81736999>] system_call_fastpath+0x16/0x1b
---[ end trace ef4521ca4028fd28 ]---



^ permalink raw reply

* Re: [PATCH net] ipv6: don't stop backtracking in fib6_lookup_1 if subtree does not match
From: Teco Boot @ 2013-08-10 11:39 UTC (permalink / raw)
  To: Hannes Frederic Sowa, David Miller; +Cc: netdev
In-Reply-To: <20130809145743.GM14001@order.stressinduktion.org>

OK, I went back to kernel where I had the ipv6 sadr lookup in cache problem.
There I patched the ip6_fib.c.
Patch works !!

The hang problem in 3.8.0-28 has lower priority for me. Maybe I'll check current -net-next later. Don't hold your breath.

Now this patch should go mainline and older kernels, I think.
What is the next step?

Teco


Op 9 aug. 2013, om 16:57 heeft Hannes Frederic Sowa <hannes@stressinduktion.org> het volgende geschreven:

> On Fri, Aug 09, 2013 at 04:40:03PM +0200, Teco Boot wrote:
>> 
>> Op 9 aug. 2013, om 16:19 heeft Hannes Frederic Sowa <hannes@stressinduktion.org> het volgende geschreven:
>> 
>>> On Fri, Aug 09, 2013 at 04:12:20PM +0200, Teco Boot wrote:
>>>> Double checked, it works. Tested with Common Open Research Emulator (CORE, from NRL).
>>>> 
>>>> === But ===
>>>> 
>>>> I had some problems with CORE. Could be me, new kernel version, the patch, CORE using network namespaces, or combination of that. sudo hangs, reboot hangs, ip address show hangs.
>>>> With ps -ef, I see some suspected items:
>>>> root      4858  4161  0 15:57 ?        00:00:00 /sbin/ip -6 addr flush dev eth0
>>>> root      4859     2  0 15:57 ?        00:00:00 [kworker/0:3]
>>>> In the emulation I use, there was no IPv6.
>>>> 
>>>> I'll try to figure out if it is CORE & netns in combination with patch. Or just the patch. Or just the new kernel.
>>>> Just to speed up, any idea where to look for?
>>> 
>>> echo t > /proc/sysrq-trigger and check dmesg while the system hangs. Maybe you
>>> have to use a serial console and send the sysrq over keyboard/break.
>> 
>> System was still running.
>> Cleared dmesg, silence.
>> Then performed echo t > /proc/sysrq-trigger.
>> Quite large dump.
>> Afterwards, silence again.
> 
> I think you are experience high fib6_gc_lock contention (and while the lock is
> held you also hold rtnl_lock, which let's all the other processes hang).
> Please test if these patches make things more smooth for you:
> 
> https://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/commit/?id=2ac3ac8f86f2fe065d746d9a9abaca867adec577
> https://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/commit/?id=49a18d86f66d33a20144ecb5a34bba0d1856b260
> 
> Thanks,
> 
>  Hannes
> 

^ permalink raw reply

* Re: [PATCH net-next] rtnetlink: Fix inverted check in ndo_dflt_fdb_del()
From: David Miller @ 2013-08-10  8:24 UTC (permalink / raw)
  To: sri; +Cc: netdev
In-Reply-To: <1376000389.22896.34.camel@sridhar.usor.ibm.com>

From: Sridhar Samudrala <sri@us.ibm.com>
Date: Thu, 08 Aug 2013 15:19:48 -0700

> Fix inverted check when deleting an fdb entry.
> 
> Signed-off-by: Sridhar Samudrala <sri@us.ibm.com>

This fixes a bug that has been around for a release or two, so I've
applied it to 'net'.

Thanks.

^ permalink raw reply

* Re: [PATCH 2/2 net-next] net: attempt high order allocations in sock_alloc_send_pskb()
From: David Miller @ 2013-08-10  8:17 UTC (permalink / raw)
  To: eric.dumazet; +Cc: netdev, rientjes
In-Reply-To: <1375997927.4004.131.camel@edumazet-glaptop>

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 08 Aug 2013 14:38:47 -0700

> From: Eric Dumazet <edumazet@google.com>
> 
> Adding paged frags skbs to af_unix sockets introduced a performance
> regression on large sends because of additional page allocations, even
> if each skb could carry at least 100% more payload than before.
> 
> We can instruct sock_alloc_send_pskb() to attempt high order
> allocations.
> 
> Most of the time, it does a single page allocation instead of 8.
> 
> I added an additional parameter to sock_alloc_send_pskb() to
> let other users to opt-in for this new feature on followup patches.
> 
> Tested:
> 
> Before patch :
> 
> $ netperf -t STREAM_STREAM 
> STREAM STREAM TEST
> Recv   Send    Send                          
> Socket Socket  Message  Elapsed              
> Size   Size    Size     Time     Throughput  
> bytes  bytes   bytes    secs.    10^6bits/sec  
> 
>  2304  212992  212992    10.00    46861.15   
> 
> After patch :
> 
> $ netperf -t STREAM_STREAM 
> STREAM STREAM TEST
> Recv   Send    Send                          
> Socket Socket  Message  Elapsed              
> Size   Size    Size     Time     Throughput  
> bytes  bytes   bytes    secs.    10^6bits/sec  
> 
>  2304  212992  212992    10.00    57981.11   
> 
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Great work, applied, thanks Eric.

^ permalink raw reply

* Re: [PATCH 1/2 net-next] af_unix: improve STREAM behavior with fragmented memory
From: David Miller @ 2013-08-10  8:16 UTC (permalink / raw)
  To: eric.dumazet; +Cc: rientjes, netdev
In-Reply-To: <1375997852.4004.130.camel@edumazet-glaptop>

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 08 Aug 2013 14:37:32 -0700

> From: Eric Dumazet <edumazet@google.com>
> 
> unix_stream_sendmsg() currently uses order-2 allocations,
> and we had numerous reports this can fail.
> 
> The __GFP_REPEAT flag present in sock_alloc_send_pskb() is
> not helping.
> 
> This patch extends the work done in commit eb6a24816b247c
> ("af_unix: reduce high order page allocations) for
> datagram sockets.
> 
> This opens the possibility of zero copy IO (splice() and
> friends)
> 
> The trick is to not use skb_pull() anymore in recvmsg() path,
> and instead add a @consumed field in UNIXCB() to track amount
> of already read payload in the skb.
> 
> There is a performance regression for large sends
> because of extra page allocations that will be addressed
> in a follow-up patch, allowing sock_alloc_send_pskb()
> to attempt high order page allocations.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Applied.

^ permalink raw reply

* Re: [PATCH] tcp: add server ip to encrypt cookie in fast open
From: David Miller @ 2013-08-10  7:45 UTC (permalink / raw)
  To: ncardwell; +Cc: ycheng, edumazet, netdev
In-Reply-To: <CADVnQynQZDzefMWX868c46i0RkuRig=6By1B70a9=2GWTkcDfA@mail.gmail.com>

From: Neal Cardwell <ncardwell@google.com>
Date: Thu, 8 Aug 2013 20:48:28 -0400

> On Thu, Aug 8, 2013 at 5:06 PM, Yuchung Cheng <ycheng@google.com> wrote:
>> Encrypt the cookie with both server and client IPv4 addresses,
>> such that multi-homed server will grant different cookies
>> based on both the source and destination IPs. No client change
>> is needed since cookie is opaque to the client.
>>
>> Signed-off-by: Yuchung Cheng <ycheng@google.com>
>> ---
>>  include/net/tcp.h       |  3 ++-
>>  net/ipv4/tcp_fastopen.c | 13 ++++++-------
>>  net/ipv4/tcp_ipv4.c     | 10 +++++++---
>>  3 files changed, 15 insertions(+), 11 deletions(-)
> 
> Acked-by: Neal Cardwell <ncardwell@google.com>

Applied to net-next, thanks everyone.

^ permalink raw reply

* Re: [PATCH] net/usb: rtl8150: allocate URB transfer_buffer and setup_packet separately
From: Jussi Kivilinna @ 2013-08-10  7:38 UTC (permalink / raw)
  To: Petko Manolov; +Cc: netdev, Greg Kroah-Hartman, linux-usb, David S. Miller
In-Reply-To: <alpine.DEB.2.10.1308092149270.3114@fry.nucleusys.com>

On 09.08.2013 21:50, Petko Manolov wrote:
> On Thu, 8 Aug 2013, Jussi Kivilinna wrote:
> 
>> On 08.08.2013 18:14, Petko Manolov wrote:
>>> On Wed, 7 Aug 2013, Jussi Kivilinna wrote:
>>>
>>>> rtl8150 allocates URB transfer_buffer and setup_packet as part of same 
>>>> structure 'struct async_req'. This can cause same cacheline to be 
>>>> DMA-mapped twice with same URB. This can lead to memory corruption on 
>>>> some systems.
>>>
>>> I can see performance impact due to the double mapping.  However, memory 
>>> corruption seems a bit too much for sane cache and DMA controllers.  Out 
>>> of interest - which is the architecture that will potentially corrupt the 
>>> memory.
>>
>> rtlwifi driver had similar structure to allocate both setup_packet and 
>> transfer_buffer in single go (overlapping dma-mapping cachelines) and 
>> this caused problems on ARM/sunxi. Problems means: memory corruptions at 
>> random locations, device freezes and lock-ups.
> 
> Broken controllers?..
> 

At first I thought so too, but I discussed about this at linux-usb, and answer
was that driver which uses same (dma/-)cacheline for multiple buffers is buggy..
  http://marc.info/?l=linux-usb&m=137130407121137&w=2 (at end)
  http://marc.info/?l=linux-usb&m=137137080902265&w=2
  http://marc.info/?l=linux-usb&m=137242422120782&w=2

-Jussi

^ permalink raw reply

* Re: [PATCH 1/2] net/usb: catc: allocate URB setup_packet as separate buffer
From: Jussi Kivilinna @ 2013-08-10  7:31 UTC (permalink / raw)
  To: David Miller; +Cc: netdev, gregkh, linux-usb
In-Reply-To: <20130809.134222.2239756285423925844.davem@davemloft.net>

On 09.08.2013 23:42, David Miller wrote:
> From: Jussi Kivilinna <jussi.kivilinna@iki.fi>
> Date: Wed, 07 Aug 2013 16:26:29 +0300
> 
>> URB setup packet must not be allocated as part of larger structure
>> because DMA coherence issues.
>>
>> Patch changes catc to allocate ctrl_dr member as separate buffer.
>>
>> Patch is only compile tested.
>>
>> Cc: <stable@vger.kernel.org>
>> Signed-off-by: Jussi Kivilinna <jussi.kivilinna@iki.fi>
> 
> I absolutely do not agree with your adding the stable CC: tag for
> a patch which has only been compile tested.

Agreed, this was not brightest of ideas. I won't do it again.

-Jussi

> 
> This applies to all of your other patches you submitted in this
> cluster too.
> 

^ permalink raw reply

* [PATCH net-next] net: sctp: Add rudimentary infrastructure to account for control chunks
From: Vlad Yasevich @ 2013-08-10  2:05 UTC (permalink / raw)
  To: netdev; +Cc: davem, linux-sctp, dborkman, eric.dumazet, Vlad Yasevich
In-Reply-To: <52058BB2.4010606@gmail.com>

This patch adds a base infrastructure that allows SCTP to do
memory accounting for control chunks.  Real accounting code will
follow.

This patch alos fixes the following triggered bug ...

[  553.109742] kernel BUG at include/linux/skbuff.h:1813!
[  553.109766] invalid opcode: 0000 [#1] SMP
[  553.109789] Modules linked in: sctp libcrc32c rfcomm [...]
[  553.110259]  uinput i915 i2c_algo_bit drm_kms_helper e1000e drm ptp
pps_core i2c_core wmi video sunrpc
[  553.110320] CPU: 0 PID: 1636 Comm: lt-test_1_to_1_ Not tainted
3.11.0-rc3+ #2
[  553.110350] Hardware name: LENOVO 74597D6/74597D6, BIOS 6DET60WW
(3.10 ) 09/17/2009
[  553.110381] task: ffff88020a01dd40 ti: ffff880204ed0000 task.ti:
ffff880204ed0000
[  553.110411] RIP: 0010:[<ffffffffa0698017>]  [<ffffffffa0698017>]
skb_orphan.part.9+0x4/0x6 [sctp]
[  553.110459] RSP: 0018:ffff880204ed1bb8  EFLAGS: 00010286
[  553.110483] RAX: ffff8802086f5a40 RBX: ffff880204303300 RCX:
0000000000000000
[  553.110487] RDX: ffff880204303c28 RSI: ffff8802086f5a40 RDI:
ffff880202158000
[  553.110487] RBP: ffff880204ed1bb8 R08: 0000000000000000 R09:
0000000000000000
[  553.110487] R10: ffff88022f2d9a04 R11: ffff880233001600 R12:
0000000000000000
[  553.110487] R13: ffff880204303c00 R14: ffff8802293d0000 R15:
ffff880202158000
[  553.110487] FS:  00007f31b31fe740(0000) GS:ffff88023bc00000(0000)
knlGS:0000000000000000
[  553.110487] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[  553.110487] CR2: 000000379980e3e0 CR3: 000000020d225000 CR4:
00000000000407f0
[  553.110487] Stack:
[  553.110487]  ffff880204ed1ca8 ffffffffa068d7fc 0000000000000000
0000000000000000
[  553.110487]  0000000000000000 ffff8802293d0000 ffff880202158000
ffffffff81cb7900
[  553.110487]  0000000000000000 0000400000001c68 ffff8802086f5a40
000000000000000f
[  553.110487] Call Trace:
[  553.110487]  [<ffffffffa068d7fc>] sctp_sendmsg+0x6bc/0xc80 [sctp]
[  553.110487]  [<ffffffff8128f185>] ? sock_has_perm+0x75/0x90
[  553.110487]  [<ffffffff815a3593>] inet_sendmsg+0x63/0xb0
[  553.110487]  [<ffffffff8128f2b3>] ? selinux_socket_sendmsg+0x23/0x30
[  553.110487]  [<ffffffff8151c5d6>] sock_sendmsg+0xa6/0xd0
[  553.110487]  [<ffffffff81637b05>] ? _raw_spin_unlock_bh+0x15/0x20
[  553.110487]  [<ffffffff8151cd38>] SYSC_sendto+0x128/0x180
[  553.110487]  [<ffffffff8151ce6b>] ? SYSC_connect+0xdb/0x100
[  553.110487]  [<ffffffffa0690031>] ? sctp_inet_listen+0x71/0x1f0
[sctp]
[  553.110487]  [<ffffffff8151d35e>] SyS_sendto+0xe/0x10
[  553.110487]  [<ffffffff81640202>] system_call_fastpath+0x16/0x1b
[  553.110487] Code: e0 48 c7 c7 00 22 6a a0 e8 67 a3 f0 e0 48 c7 [...]
[  553.110487] RIP  [<ffffffffa0698017>] skb_orphan.part.9+0x4/0x6
[sctp]
[  553.110487]  RSP <ffff880204ed1bb8>
[  553.121578] ---[ end trace 46c20c5903ef5be2 ]---

The approach taken here is to split data and control chunks
creation a  bit.  Data chunks already have memory accounting
so noting needs to happen.  For control chunks, add stubs handlers.

Signed-off-by: Vlad Yasevich <vyasevich@gmail.com>
---
 net/sctp/sm_make_chunk.c | 99 +++++++++++++++++++++++++++++++++---------------
 1 file changed, 69 insertions(+), 30 deletions(-)

diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index 780a2d4..04a82a6 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -68,8 +68,12 @@
 #include <net/sctp/sctp.h>
 #include <net/sctp/sm.h>
 
-static struct sctp_chunk *sctp_make_chunk(const struct sctp_association *asoc,
-					  __u8 type, __u8 flags, int paylen);
+static struct sctp_chunk *sctp_make_control(const struct sctp_association *asoc,
+					    __u8 type, __u8 flags, int paylen);
+static struct sctp_chunk *sctp_make_data(const struct sctp_association *asoc,
+					 __u8 flags, int paylen);
+static struct sctp_chunk *_sctp_make_chunk(const struct sctp_association *asoc,
+					   __u8 type, __u8 flags, int paylen);
 static sctp_cookie_param_t *sctp_pack_cookie(const struct sctp_endpoint *ep,
 					const struct sctp_association *asoc,
 					const struct sctp_chunk *init_chunk,
@@ -82,6 +86,28 @@ static int sctp_process_param(struct sctp_association *asoc,
 static void *sctp_addto_param(struct sctp_chunk *chunk, int len,
 			      const void *data);
 
+/* Control chunk destructor */
+static void sctp_control_release_owner(struct sk_buff *skb)
+{
+	/*TODO: do memory release */
+}
+
+static void sctp_control_set_owner_w(struct sctp_chunk *chunk)
+{
+	struct sctp_association *asoc = chunk->asoc;
+	struct sk_buff *skb = chunk->skb;
+
+	/* TODO: properly account for control chunks. 
+	 * To do it right we'll need:
+	 *  1) endpoint if association isn't known.
+	 *  2) proper memory accounting.
+	 *
+	 *  For now don't do anything for now.
+	 */
+	skb->sk = asoc ? asoc->base.sk : NULL;
+	skb->destructor = sctp_control_release_owner;
+}
+
 /* What was the inbound interface for this chunk? */
 int sctp_chunk_iif(const struct sctp_chunk *chunk)
 {
@@ -296,7 +322,7 @@ struct sctp_chunk *sctp_make_init(const struct sctp_association *asoc,
 	 * PLEASE DO NOT FIXME [This version does not support Host Name.]
 	 */
 
-	retval = sctp_make_chunk(asoc, SCTP_CID_INIT, 0, chunksize);
+	retval = sctp_make_control(asoc, SCTP_CID_INIT, 0, chunksize);
 	if (!retval)
 		goto nodata;
 
@@ -443,7 +469,7 @@ struct sctp_chunk *sctp_make_init_ack(const struct sctp_association *asoc,
 					num_ext);
 
 	/* Now allocate and fill out the chunk.  */
-	retval = sctp_make_chunk(asoc, SCTP_CID_INIT_ACK, 0, chunksize);
+	retval = sctp_make_control(asoc, SCTP_CID_INIT_ACK, 0, chunksize);
 	if (!retval)
 		goto nomem_chunk;
 
@@ -548,7 +574,7 @@ struct sctp_chunk *sctp_make_cookie_echo(const struct sctp_association *asoc,
 	cookie_len = asoc->peer.cookie_len;
 
 	/* Build a cookie echo chunk.  */
-	retval = sctp_make_chunk(asoc, SCTP_CID_COOKIE_ECHO, 0, cookie_len);
+	retval = sctp_make_control(asoc, SCTP_CID_COOKIE_ECHO, 0, cookie_len);
 	if (!retval)
 		goto nodata;
 	retval->subh.cookie_hdr =
@@ -593,7 +619,7 @@ struct sctp_chunk *sctp_make_cookie_ack(const struct sctp_association *asoc,
 {
 	struct sctp_chunk *retval;
 
-	retval = sctp_make_chunk(asoc, SCTP_CID_COOKIE_ACK, 0, 0);
+	retval = sctp_make_control(asoc, SCTP_CID_COOKIE_ACK, 0, 0);
 
 	/* RFC 2960 6.4 Multi-homed SCTP Endpoints
 	 *
@@ -641,8 +667,8 @@ struct sctp_chunk *sctp_make_cwr(const struct sctp_association *asoc,
 	sctp_cwrhdr_t cwr;
 
 	cwr.lowest_tsn = htonl(lowest_tsn);
-	retval = sctp_make_chunk(asoc, SCTP_CID_ECN_CWR, 0,
-				 sizeof(sctp_cwrhdr_t));
+	retval = sctp_make_control(asoc, SCTP_CID_ECN_CWR, 0,
+				   sizeof(sctp_cwrhdr_t));
 
 	if (!retval)
 		goto nodata;
@@ -675,8 +701,8 @@ struct sctp_chunk *sctp_make_ecne(const struct sctp_association *asoc,
 	sctp_ecnehdr_t ecne;
 
 	ecne.lowest_tsn = htonl(lowest_tsn);
-	retval = sctp_make_chunk(asoc, SCTP_CID_ECN_ECNE, 0,
-				 sizeof(sctp_ecnehdr_t));
+	retval = sctp_make_control(asoc, SCTP_CID_ECN_ECNE, 0,
+				   sizeof(sctp_ecnehdr_t));
 	if (!retval)
 		goto nodata;
 	retval->subh.ecne_hdr =
@@ -712,7 +738,7 @@ struct sctp_chunk *sctp_make_datafrag_empty(struct sctp_association *asoc,
 		dp.ssn = htons(ssn);
 
 	chunk_len = sizeof(dp) + data_len;
-	retval = sctp_make_chunk(asoc, SCTP_CID_DATA, flags, chunk_len);
+	retval = sctp_make_data(asoc, flags, chunk_len);
 	if (!retval)
 		goto nodata;
 
@@ -759,7 +785,7 @@ struct sctp_chunk *sctp_make_sack(const struct sctp_association *asoc)
 		+ sizeof(__u32) * num_dup_tsns;
 
 	/* Create the chunk.  */
-	retval = sctp_make_chunk(asoc, SCTP_CID_SACK, 0, len);
+	retval = sctp_make_control(asoc, SCTP_CID_SACK, 0, len);
 	if (!retval)
 		goto nodata;
 
@@ -838,8 +864,8 @@ struct sctp_chunk *sctp_make_shutdown(const struct sctp_association *asoc,
 	ctsn = sctp_tsnmap_get_ctsn(&asoc->peer.tsn_map);
 	shut.cum_tsn_ack = htonl(ctsn);
 
-	retval = sctp_make_chunk(asoc, SCTP_CID_SHUTDOWN, 0,
-				 sizeof(sctp_shutdownhdr_t));
+	retval = sctp_make_control(asoc, SCTP_CID_SHUTDOWN, 0,
+				   sizeof(sctp_shutdownhdr_t));
 	if (!retval)
 		goto nodata;
 
@@ -857,7 +883,7 @@ struct sctp_chunk *sctp_make_shutdown_ack(const struct sctp_association *asoc,
 {
 	struct sctp_chunk *retval;
 
-	retval = sctp_make_chunk(asoc, SCTP_CID_SHUTDOWN_ACK, 0, 0);
+	retval = sctp_make_control(asoc, SCTP_CID_SHUTDOWN_ACK, 0, 0);
 
 	/* RFC 2960 6.4 Multi-homed SCTP Endpoints
 	 *
@@ -886,7 +912,7 @@ struct sctp_chunk *sctp_make_shutdown_complete(
 	 */
 	flags |= asoc ? 0 : SCTP_CHUNK_FLAG_T;
 
-	retval = sctp_make_chunk(asoc, SCTP_CID_SHUTDOWN_COMPLETE, flags, 0);
+	retval = sctp_make_control(asoc, SCTP_CID_SHUTDOWN_COMPLETE, flags, 0);
 
 	/* RFC 2960 6.4 Multi-homed SCTP Endpoints
 	 *
@@ -925,7 +951,7 @@ struct sctp_chunk *sctp_make_abort(const struct sctp_association *asoc,
 			flags = SCTP_CHUNK_FLAG_T;
 	}
 
-	retval = sctp_make_chunk(asoc, SCTP_CID_ABORT, flags, hint);
+	retval = sctp_make_control(asoc, SCTP_CID_ABORT, flags, hint);
 
 	/* RFC 2960 6.4 Multi-homed SCTP Endpoints
 	 *
@@ -1117,7 +1143,7 @@ struct sctp_chunk *sctp_make_heartbeat(const struct sctp_association *asoc,
 	struct sctp_chunk *retval;
 	sctp_sender_hb_info_t hbinfo;
 
-	retval = sctp_make_chunk(asoc, SCTP_CID_HEARTBEAT, 0, sizeof(hbinfo));
+	retval = sctp_make_control(asoc, SCTP_CID_HEARTBEAT, 0, sizeof(hbinfo));
 
 	if (!retval)
 		goto nodata;
@@ -1145,7 +1171,7 @@ struct sctp_chunk *sctp_make_heartbeat_ack(const struct sctp_association *asoc,
 {
 	struct sctp_chunk *retval;
 
-	retval  = sctp_make_chunk(asoc, SCTP_CID_HEARTBEAT_ACK, 0, paylen);
+	retval  = sctp_make_control(asoc, SCTP_CID_HEARTBEAT_ACK, 0, paylen);
 	if (!retval)
 		goto nodata;
 
@@ -1177,8 +1203,8 @@ static struct sctp_chunk *sctp_make_op_error_space(
 {
 	struct sctp_chunk *retval;
 
-	retval = sctp_make_chunk(asoc, SCTP_CID_ERROR, 0,
-				 sizeof(sctp_errhdr_t) + size);
+	retval = sctp_make_control(asoc, SCTP_CID_ERROR, 0,
+				   sizeof(sctp_errhdr_t) + size);
 	if (!retval)
 		goto nodata;
 
@@ -1248,7 +1274,7 @@ struct sctp_chunk *sctp_make_auth(const struct sctp_association *asoc)
 	if (unlikely(!hmac_desc))
 		return NULL;
 
-	retval = sctp_make_chunk(asoc, SCTP_CID_AUTH, 0,
+	retval = sctp_make_control(asoc, SCTP_CID_AUTH, 0,
 			hmac_desc->hmac_len + sizeof(sctp_authhdr_t));
 	if (!retval)
 		return NULL;
@@ -1351,8 +1377,8 @@ const union sctp_addr *sctp_source(const struct sctp_chunk *chunk)
 /* Create a new chunk, setting the type and flags headers from the
  * arguments, reserving enough space for a 'paylen' byte payload.
  */
-static struct sctp_chunk *sctp_make_chunk(const struct sctp_association *asoc,
-					  __u8 type, __u8 flags, int paylen)
+static struct sctp_chunk *_sctp_make_chunk(const struct sctp_association *asoc,
+					    __u8 type, __u8 flags, int paylen)
 {
 	struct sctp_chunk *retval;
 	sctp_chunkhdr_t *chunk_hdr;
@@ -1385,14 +1411,27 @@ static struct sctp_chunk *sctp_make_chunk(const struct sctp_association *asoc,
 	if (sctp_auth_send_cid(type, asoc))
 		retval->auth = 1;
 
-	/* Set the skb to the belonging sock for accounting.  */
-	skb->sk = sk;
-
 	return retval;
 nodata:
 	return NULL;
 }
 
+static struct sctp_chunk *sctp_make_data(const struct sctp_association *asoc,
+					 __u8 flags, int paylen)
+{
+	return _sctp_make_chunk(asoc, SCTP_CID_DATA, flags, paylen);
+}
+
+static struct sctp_chunk *sctp_make_control(const struct sctp_association *asoc,
+					    __u8 type, __u8 flags, int paylen)
+{
+	struct sctp_chunk *chunk = _sctp_make_chunk(asoc, type, flags, paylen);
+
+	if (chunk)
+		sctp_control_set_owner_w(chunk);
+
+	return chunk;
+}
 
 /* Release the memory occupied by a chunk.  */
 static void sctp_chunk_destroy(struct sctp_chunk *chunk)
@@ -2733,7 +2772,7 @@ static struct sctp_chunk *sctp_make_asconf(struct sctp_association *asoc,
 	length += addrlen;
 
 	/* Create the chunk.  */
-	retval = sctp_make_chunk(asoc, SCTP_CID_ASCONF, 0, length);
+	retval = sctp_make_control(asoc, SCTP_CID_ASCONF, 0, length);
 	if (!retval)
 		return NULL;
 
@@ -2917,7 +2956,7 @@ static struct sctp_chunk *sctp_make_asconf_ack(const struct sctp_association *as
 	int			length = sizeof(asconf) + vparam_len;
 
 	/* Create the chunk.  */
-	retval = sctp_make_chunk(asoc, SCTP_CID_ASCONF_ACK, 0, length);
+	retval = sctp_make_control(asoc, SCTP_CID_ASCONF_ACK, 0, length);
 	if (!retval)
 		return NULL;
 
@@ -3448,7 +3487,7 @@ struct sctp_chunk *sctp_make_fwdtsn(const struct sctp_association *asoc,
 
 	hint = (nstreams + 1) * sizeof(__u32);
 
-	retval = sctp_make_chunk(asoc, SCTP_CID_FWD_TSN, 0, hint);
+	retval = sctp_make_control(asoc, SCTP_CID_FWD_TSN, 0, hint);
 
 	if (!retval)
 		return NULL;
-- 
1.8.1.4

^ permalink raw reply related

* Re: [PATCH] net: sctp: fix panic during skb_orphan()
From: Vlad Yasevich @ 2013-08-10  1:34 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: netdev, davem, linux-sctp, dborkman
In-Reply-To: <1376097622.20509.19.camel@edumazet-glaptop>

On 08/09/2013 09:20 PM, Eric Dumazet wrote:
> On Fri, 2013-08-09 at 20:39 -0400, Vlad Yasevich wrote:
>> On 08/09/2013 07:20 PM, Eric Dumazet wrote:
>
>>> Why not using a dummy destructor ?
>>
>> It would just be useless once we do proper accounting for all chunks.
>> But, I'll send a different idea if this is just too ugly.
>
> Setting skb->sk is no accounting.

Right.  It is only really used right now when constructing a packet to 
transmit out of a set of chunks.

>
> It seems you would better use skb->cb[] so that you are 100% sure this
> skb->sk doesn't leak outside of SCTP code.

Hmm..  That might work too.  I am building a framework right now that
we can use to add proper buffer accounting.  That's probably a better
long term solution.

-vlad

^ permalink raw reply

* Re: [PATCH] net: sctp: fix panic during skb_orphan()
From: Eric Dumazet @ 2013-08-10  1:20 UTC (permalink / raw)
  To: Vlad Yasevich; +Cc: netdev, davem, linux-sctp, dborkman
In-Reply-To: <52058BB2.4010606@gmail.com>

On Fri, 2013-08-09 at 20:39 -0400, Vlad Yasevich wrote:
> On 08/09/2013 07:20 PM, Eric Dumazet wrote:

> > Why not using a dummy destructor ?
> 
> It would just be useless once we do proper accounting for all chunks.
> But, I'll send a different idea if this is just too ugly.

Setting skb->sk is no accounting.

It seems you would better use skb->cb[] so that you are 100% sure this
skb->sk doesn't leak outside of SCTP code.

^ permalink raw reply

* Good day
From: Leung limshek @ 2013-08-10  0:15 UTC (permalink / raw)




Good day
This is a personal E-mail directed to you and I request that it be treated as such. I am Leung limshek.Your attention is needed.kindly contact me for detailed information andbetter confidentiality


Email:   leunglimshek00@gmail.com
Sincerely,Leung limshek

^ permalink raw reply

* Re: SCTP performance?
From: David Miller @ 2013-08-10  0:54 UTC (permalink / raw)
  To: greearb; +Cc: eric.dumazet, netdev
In-Reply-To: <520583AB.9030509@candelatech.com>

From: Ben Greear <greearb@candelatech.com>
Date: Fri, 09 Aug 2013 17:04:59 -0700

> I got much better results when I explicitly set the
> tx/rx socket buffer sizes to 2MB, so maybe it just sucks
> at auto-tuning it's socket buffer sizes.

TCP is the only protocol in our stack which does socket buffer
size autotuning.

^ permalink raw reply

* Re: [PATCH 1/4 v2] drivers/net: enic: Adding support for Cisco Low Latency NIC
From: David Miller @ 2013-08-10  0:51 UTC (permalink / raw)
  To: neepatel; +Cc: netdev
In-Reply-To: <1376085623-21630-1-git-send-email-neepatel@cisco.com>

From: Neel Patel <neepatel@cisco.com>
Date: Fri,  9 Aug 2013 15:00:23 -0700

> This patch,
> 	- Adds new firmware commands for the new Cisco Low Latency NIC
>       (aka. USNIC).
>     - Enables descriptor prefetch on the hardware. This feature
>       reduces latency for small packet transmit.
> 
> Signed-off-by: Neel Patel <neepatel@cisco.com>
> Signed-off-by: Nishank Trivedi <nistrive@cisco.com>
> Signed-off-by: Christian Benvenuti <benve@cisco.com>

Please resend the whole patch series, not just the patches that
changed, when you have to change things.

^ permalink raw reply

* Re: [PATCH] net: sctp: fix panic during skb_orphan()
From: Vlad Yasevich @ 2013-08-10  0:39 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: netdev, davem, linux-sctp, dborkman
In-Reply-To: <1376090435.20509.13.camel@edumazet-glaptop>

On 08/09/2013 07:20 PM, Eric Dumazet wrote:
> On Fri, 2013-08-09 at 17:58 -0400, Vlad Yasevich wrote:
>> This patch fixes the following triggered bug ...
>>
>> [  553.109742] kernel BUG at include/linux/skbuff.h:1813!
>> [  553.109766] invalid opcode: 0000 [#1] SMP
>> [  553.109789] Modules linked in: sctp libcrc32c rfcomm [...]
>> [  553.110259]  uinput i915 i2c_algo_bit drm_kms_helper e1000e drm ptp
>> pps_core i2c_core wmi video sunrpc
>> [  553.110320] CPU: 0 PID: 1636 Comm: lt-test_1_to_1_ Not tainted
>> 3.11.0-rc3+ #2
>> [  553.110350] Hardware name: LENOVO 74597D6/74597D6, BIOS 6DET60WW
>> (3.10 ) 09/17/2009
>> [  553.110381] task: ffff88020a01dd40 ti: ffff880204ed0000 task.ti:
>> ffff880204ed0000
>> [  553.110411] RIP: 0010:[<ffffffffa0698017>]  [<ffffffffa0698017>]
>> skb_orphan.part.9+0x4/0x6 [sctp]
>> [  553.110459] RSP: 0018:ffff880204ed1bb8  EFLAGS: 00010286
>> [  553.110483] RAX: ffff8802086f5a40 RBX: ffff880204303300 RCX:
>> 0000000000000000
>> [  553.110487] RDX: ffff880204303c28 RSI: ffff8802086f5a40 RDI:
>> ffff880202158000
>> [  553.110487] RBP: ffff880204ed1bb8 R08: 0000000000000000 R09:
>> 0000000000000000
>> [  553.110487] R10: ffff88022f2d9a04 R11: ffff880233001600 R12:
>> 0000000000000000
>> [  553.110487] R13: ffff880204303c00 R14: ffff8802293d0000 R15:
>> ffff880202158000
>> [  553.110487] FS:  00007f31b31fe740(0000) GS:ffff88023bc00000(0000)
>> knlGS:0000000000000000
>> [  553.110487] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
>> [  553.110487] CR2: 000000379980e3e0 CR3: 000000020d225000 CR4:
>> 00000000000407f0
>> [  553.110487] Stack:
>> [  553.110487]  ffff880204ed1ca8 ffffffffa068d7fc 0000000000000000
>> 0000000000000000
>> [  553.110487]  0000000000000000 ffff8802293d0000 ffff880202158000
>> ffffffff81cb7900
>> [  553.110487]  0000000000000000 0000400000001c68 ffff8802086f5a40
>> 000000000000000f
>> [  553.110487] Call Trace:
>> [  553.110487]  [<ffffffffa068d7fc>] sctp_sendmsg+0x6bc/0xc80 [sctp]
>> [  553.110487]  [<ffffffff8128f185>] ? sock_has_perm+0x75/0x90
>> [  553.110487]  [<ffffffff815a3593>] inet_sendmsg+0x63/0xb0
>> [  553.110487]  [<ffffffff8128f2b3>] ? selinux_socket_sendmsg+0x23/0x30
>> [  553.110487]  [<ffffffff8151c5d6>] sock_sendmsg+0xa6/0xd0
>> [  553.110487]  [<ffffffff81637b05>] ? _raw_spin_unlock_bh+0x15/0x20
>> [  553.110487]  [<ffffffff8151cd38>] SYSC_sendto+0x128/0x180
>> [  553.110487]  [<ffffffff8151ce6b>] ? SYSC_connect+0xdb/0x100
>> [  553.110487]  [<ffffffffa0690031>] ? sctp_inet_listen+0x71/0x1f0
>> [sctp]
>> [  553.110487]  [<ffffffff8151d35e>] SyS_sendto+0xe/0x10
>> [  553.110487]  [<ffffffff81640202>] system_call_fastpath+0x16/0x1b
>> [  553.110487] Code: e0 48 c7 c7 00 22 6a a0 e8 67 a3 f0 e0 48 c7 [...]
>> [  553.110487] RIP  [<ffffffffa0698017>] skb_orphan.part.9+0x4/0x6
>> [sctp]
>> [  553.110487]  RSP <ffff880204ed1bb8>
>> [  553.121578] ---[ end trace 46c20c5903ef5be2 ]---
>>
>> When a SCTP data chunk is created, skb->sk is set by sctp_make_chunk().
>> At a later time, sctp_sendmg() calls sctp_set_owner_w() which will attempt
>> to orphan the skb and correctly set the skb->sk.  The skb_orphan call result
>> in the above crash since skb->sk was set the the destructor was not.
>>
>> The simple solution is to not set skb->sk for DATA chunks (since it
>> will be done later).  Continue to do it for control chunks since
>> sctp needs the sk at a later timer and control chunks are not currently
>> accouted agains socket memory.
>>
>> Signed-off-by: Vlad Yasevich <vyasevich@gmail.com>
>> ---
>>   net/sctp/sm_make_chunk.c | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
>> index 780a2d4..4ff7803 100644
>> --- a/net/sctp/sm_make_chunk.c
>> +++ b/net/sctp/sm_make_chunk.c
>> @@ -1385,8 +1385,12 @@ static struct sctp_chunk *sctp_make_chunk(const struct sctp_association *asoc,
>>   	if (sctp_auth_send_cid(type, asoc))
>>   		retval->auth = 1;
>>
>> -	/* Set the skb to the belonging sock for accounting.  */
>> -	skb->sk = sk;
>> +	/* Set the skb to the belonging sock for accounting.  Do not
>> +	 * do this for data chunks as that will be properly set
>> +	 * up later.
>> +	 */
>> +	if (type != SCTP_CID_DATA)
>> +		skb->sk = sk;
>>
>>   	return retval;
>>   nodata:
>
> This is ugly IMHO.

Yes, it is a bit ugly.  The "proper" way is to do accounting for all 
SCTP chunks, but that is a much larger piece of work.

>
> Why not using a dummy destructor ?

It would just be useless once we do proper accounting for all chunks.
But, I'll send a different idea if this is just too ugly.

>
> And I guess this patch is for net-next ?
>

Yes, sorry...

Anyway. I'll send an less ugly alternative...

-vlad

>
>

^ permalink raw reply

* Re: [PATCH] netfilter: nf_conntrack: fix tcp_in_window for Fast Open
From: Eric Dumazet @ 2013-08-10  0:25 UTC (permalink / raw)
  To: Yuchung Cheng; +Cc: pablo, netfilter-devel, netdev
In-Reply-To: <1376094087-17700-1-git-send-email-ycheng@google.com>

On Fri, 2013-08-09 at 17:21 -0700, Yuchung Cheng wrote:
> Currently the conntrack checks if the ending sequence of a packet
> falls within the observed receive window. However it does so even
> if it has not observe any packet from the remote yet and uses an
> uninitialized receive window (td_maxwin).
> 
> If a connection uses Fast Open to send a SYN-data packet which is
> dropped afterward in the network. The subsequent SYNs retransmits
> will all fail this check and be discarded, leading to a connection
> timeout. This is because the SYN retransmit does not contain data
> payload so
> 
> end == initial sequence number (isn) + 1
> sender->td_end == isn + syn_data_len
> receiver->td_maxwin == 0
> 
> The fix is to only apply this check after td_maxwin is initialized.
> 
> Reported-by: Michael Chan <mcfchan@stanford.edu>
> Signed-off-by: Yuchung Cheng <ycheng@google.com>
> ---

Acked-by: Eric Dumazet <edumazet@google.com>




^ permalink raw reply

* [PATCH] netfilter: nf_conntrack: fix tcp_in_window for Fast Open
From: Yuchung Cheng @ 2013-08-10  0:21 UTC (permalink / raw)
  To: pablo; +Cc: netfilter-devel, netdev, edumazet, Yuchung Cheng

Currently the conntrack checks if the ending sequence of a packet
falls within the observed receive window. However it does so even
if it has not observe any packet from the remote yet and uses an
uninitialized receive window (td_maxwin).

If a connection uses Fast Open to send a SYN-data packet which is
dropped afterward in the network. The subsequent SYNs retransmits
will all fail this check and be discarded, leading to a connection
timeout. This is because the SYN retransmit does not contain data
payload so

end == initial sequence number (isn) + 1
sender->td_end == isn + syn_data_len
receiver->td_maxwin == 0

The fix is to only apply this check after td_maxwin is initialized.

Reported-by: Michael Chan <mcfchan@stanford.edu>
Signed-off-by: Yuchung Cheng <ycheng@google.com>
---
 net/netfilter/nf_conntrack_proto_tcp.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
index 4d4d8f1..e0f9a32 100644
--- a/net/netfilter/nf_conntrack_proto_tcp.c
+++ b/net/netfilter/nf_conntrack_proto_tcp.c
@@ -526,7 +526,7 @@ static bool tcp_in_window(const struct nf_conn *ct,
 	const struct nf_conntrack_tuple *tuple = &ct->tuplehash[dir].tuple;
 	__u32 seq, ack, sack, end, win, swin;
 	s16 receiver_offset;
-	bool res;
+	bool res, in_recv_win;
 
 	/*
 	 * Get the required data from the packet.
@@ -649,14 +649,18 @@ static bool tcp_in_window(const struct nf_conn *ct,
 		 receiver->td_end, receiver->td_maxend, receiver->td_maxwin,
 		 receiver->td_scale);
 
+	/* Is the ending sequence in the receive window (if available)? */
+	in_recv_win = !receiver->td_maxwin ||
+		      after(end, sender->td_end - receiver->td_maxwin - 1);
+
 	pr_debug("tcp_in_window: I=%i II=%i III=%i IV=%i\n",
 		 before(seq, sender->td_maxend + 1),
-		 after(end, sender->td_end - receiver->td_maxwin - 1),
+		 (in_recv_win ? 1 : 0),
 		 before(sack, receiver->td_end + 1),
 		 after(sack, receiver->td_end - MAXACKWINDOW(sender) - 1));
 
 	if (before(seq, sender->td_maxend + 1) &&
-	    after(end, sender->td_end - receiver->td_maxwin - 1) &&
+	    in_recv_win &&
 	    before(sack, receiver->td_end + 1) &&
 	    after(sack, receiver->td_end - MAXACKWINDOW(sender) - 1)) {
 		/*
@@ -725,7 +729,7 @@ static bool tcp_in_window(const struct nf_conn *ct,
 			nf_log_packet(net, pf, 0, skb, NULL, NULL, NULL,
 			"nf_ct_tcp: %s ",
 			before(seq, sender->td_maxend + 1) ?
-			after(end, sender->td_end - receiver->td_maxwin - 1) ?
+			in_recv_win ?
 			before(sack, receiver->td_end + 1) ?
 			after(sack, receiver->td_end - MAXACKWINDOW(sender) - 1) ? "BUG"
 			: "ACK is under the lower bound (possible overly delayed ACK)"
-- 
1.8.3


^ permalink raw reply related

* Re: SCTP performance?
From: Ben Greear @ 2013-08-10  0:04 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: netdev
In-Reply-To: <1376090518.20509.14.camel@edumazet-glaptop>

On 08/09/2013 04:21 PM, Eric Dumazet wrote:
> On Fri, 2013-08-09 at 16:11 -0700, Ben Greear wrote:
>> I just added some support for SCTP to my traffic-generator tool.
>>
>> I tried to send 1Gbps of traffic, bi-directional, across two
>> 10G interfaces.  TCP does this without any trouble at all,
>> but SCTP was asymetric and never got above around 700Mbps,
>> and was usually stuck at < 500Mbps.
>>
>> perf top didn't show any obvious CPU bottlenecks.
>>
>> Has anyone done any recent performance testing with SCTP?
>>
>> I'd be interested to know the results if so...
>
> You mean there is no SCTP offload on the NIC and/or GRO, yes that's too
> bad.
>

I'm sure that is part of the problem.

I got much better results when I explicitly set the
tx/rx socket buffer sizes to 2MB, so maybe it just sucks
at auto-tuning it's socket buffer sizes.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

^ permalink raw reply

* Re: SCTP performance?
From: Eric Dumazet @ 2013-08-09 23:21 UTC (permalink / raw)
  To: Ben Greear; +Cc: netdev
In-Reply-To: <52057734.6060306@candelatech.com>

On Fri, 2013-08-09 at 16:11 -0700, Ben Greear wrote:
> I just added some support for SCTP to my traffic-generator tool.
> 
> I tried to send 1Gbps of traffic, bi-directional, across two
> 10G interfaces.  TCP does this without any trouble at all,
> but SCTP was asymetric and never got above around 700Mbps,
> and was usually stuck at < 500Mbps.
> 
> perf top didn't show any obvious CPU bottlenecks.
> 
> Has anyone done any recent performance testing with SCTP?
> 
> I'd be interested to know the results if so...

You mean there is no SCTP offload on the NIC and/or GRO, yes that's too
bad.

^ permalink raw reply

* Re: [PATCH] net: sctp: fix panic during skb_orphan()
From: Eric Dumazet @ 2013-08-09 23:20 UTC (permalink / raw)
  To: Vlad Yasevich; +Cc: netdev, davem, linux-sctp, dborkman
In-Reply-To: <1376085487-6466-1-git-send-email-vyasevich@gmail.com>

On Fri, 2013-08-09 at 17:58 -0400, Vlad Yasevich wrote:
> This patch fixes the following triggered bug ...
> 
> [  553.109742] kernel BUG at include/linux/skbuff.h:1813!
> [  553.109766] invalid opcode: 0000 [#1] SMP
> [  553.109789] Modules linked in: sctp libcrc32c rfcomm [...]
> [  553.110259]  uinput i915 i2c_algo_bit drm_kms_helper e1000e drm ptp
> pps_core i2c_core wmi video sunrpc
> [  553.110320] CPU: 0 PID: 1636 Comm: lt-test_1_to_1_ Not tainted
> 3.11.0-rc3+ #2
> [  553.110350] Hardware name: LENOVO 74597D6/74597D6, BIOS 6DET60WW
> (3.10 ) 09/17/2009
> [  553.110381] task: ffff88020a01dd40 ti: ffff880204ed0000 task.ti:
> ffff880204ed0000
> [  553.110411] RIP: 0010:[<ffffffffa0698017>]  [<ffffffffa0698017>]
> skb_orphan.part.9+0x4/0x6 [sctp]
> [  553.110459] RSP: 0018:ffff880204ed1bb8  EFLAGS: 00010286
> [  553.110483] RAX: ffff8802086f5a40 RBX: ffff880204303300 RCX:
> 0000000000000000
> [  553.110487] RDX: ffff880204303c28 RSI: ffff8802086f5a40 RDI:
> ffff880202158000
> [  553.110487] RBP: ffff880204ed1bb8 R08: 0000000000000000 R09:
> 0000000000000000
> [  553.110487] R10: ffff88022f2d9a04 R11: ffff880233001600 R12:
> 0000000000000000
> [  553.110487] R13: ffff880204303c00 R14: ffff8802293d0000 R15:
> ffff880202158000
> [  553.110487] FS:  00007f31b31fe740(0000) GS:ffff88023bc00000(0000)
> knlGS:0000000000000000
> [  553.110487] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
> [  553.110487] CR2: 000000379980e3e0 CR3: 000000020d225000 CR4:
> 00000000000407f0
> [  553.110487] Stack:
> [  553.110487]  ffff880204ed1ca8 ffffffffa068d7fc 0000000000000000
> 0000000000000000
> [  553.110487]  0000000000000000 ffff8802293d0000 ffff880202158000
> ffffffff81cb7900
> [  553.110487]  0000000000000000 0000400000001c68 ffff8802086f5a40
> 000000000000000f
> [  553.110487] Call Trace:
> [  553.110487]  [<ffffffffa068d7fc>] sctp_sendmsg+0x6bc/0xc80 [sctp]
> [  553.110487]  [<ffffffff8128f185>] ? sock_has_perm+0x75/0x90
> [  553.110487]  [<ffffffff815a3593>] inet_sendmsg+0x63/0xb0
> [  553.110487]  [<ffffffff8128f2b3>] ? selinux_socket_sendmsg+0x23/0x30
> [  553.110487]  [<ffffffff8151c5d6>] sock_sendmsg+0xa6/0xd0
> [  553.110487]  [<ffffffff81637b05>] ? _raw_spin_unlock_bh+0x15/0x20
> [  553.110487]  [<ffffffff8151cd38>] SYSC_sendto+0x128/0x180
> [  553.110487]  [<ffffffff8151ce6b>] ? SYSC_connect+0xdb/0x100
> [  553.110487]  [<ffffffffa0690031>] ? sctp_inet_listen+0x71/0x1f0
> [sctp]
> [  553.110487]  [<ffffffff8151d35e>] SyS_sendto+0xe/0x10
> [  553.110487]  [<ffffffff81640202>] system_call_fastpath+0x16/0x1b
> [  553.110487] Code: e0 48 c7 c7 00 22 6a a0 e8 67 a3 f0 e0 48 c7 [...]
> [  553.110487] RIP  [<ffffffffa0698017>] skb_orphan.part.9+0x4/0x6
> [sctp]
> [  553.110487]  RSP <ffff880204ed1bb8>
> [  553.121578] ---[ end trace 46c20c5903ef5be2 ]---
> 
> When a SCTP data chunk is created, skb->sk is set by sctp_make_chunk().
> At a later time, sctp_sendmg() calls sctp_set_owner_w() which will attempt
> to orphan the skb and correctly set the skb->sk.  The skb_orphan call result
> in the above crash since skb->sk was set the the destructor was not.
> 
> The simple solution is to not set skb->sk for DATA chunks (since it
> will be done later).  Continue to do it for control chunks since
> sctp needs the sk at a later timer and control chunks are not currently
> accouted agains socket memory.
> 
> Signed-off-by: Vlad Yasevich <vyasevich@gmail.com>
> ---
>  net/sctp/sm_make_chunk.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> index 780a2d4..4ff7803 100644
> --- a/net/sctp/sm_make_chunk.c
> +++ b/net/sctp/sm_make_chunk.c
> @@ -1385,8 +1385,12 @@ static struct sctp_chunk *sctp_make_chunk(const struct sctp_association *asoc,
>  	if (sctp_auth_send_cid(type, asoc))
>  		retval->auth = 1;
>  
> -	/* Set the skb to the belonging sock for accounting.  */
> -	skb->sk = sk;
> +	/* Set the skb to the belonging sock for accounting.  Do not
> +	 * do this for data chunks as that will be properly set
> +	 * up later.
> +	 */
> +	if (type != SCTP_CID_DATA)
> +		skb->sk = sk;
>  
>  	return retval;
>  nodata:

This is ugly IMHO.

Why not using a dummy destructor ?

And I guess this patch is for net-next ?

^ permalink raw reply

* SCTP performance?
From: Ben Greear @ 2013-08-09 23:11 UTC (permalink / raw)
  To: netdev

I just added some support for SCTP to my traffic-generator tool.

I tried to send 1Gbps of traffic, bi-directional, across two
10G interfaces.  TCP does this without any trouble at all,
but SCTP was asymetric and never got above around 700Mbps,
and was usually stuck at < 500Mbps.

perf top didn't show any obvious CPU bottlenecks.

Has anyone done any recent performance testing with SCTP?

I'd be interested to know the results if so...

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

^ permalink raw reply

* Re: [PATCH 3/4] drivers/net: enic: Make ASIC information available to USNIC
From: Stephen Hemminger @ 2013-08-09 22:21 UTC (permalink / raw)
  To: Neel Patel; +Cc: netdev, Nishank Trivedi, Christian Benvenuti
In-Reply-To: <1376071941-17001-4-git-send-email-neepatel@cisco.com>

On Fri,  9 Aug 2013 11:12:20 -0700
Neel Patel <neepatel@cisco.com> wrote:

> This patch provides asic information via ethtool.
> 
> Signed-off-by: Neel Patel <neepatel@cisco.com>
> Signed-off-by: Nishank Trivedi <nistrive@cisco.com>
> Signed-off-by: Christian Benvenuti <benve@cisco.com>
> ---
>  drivers/net/ethernet/cisco/enic/driver_utils.h | 49 ++++++++++++++++++++++++++
>  drivers/net/ethernet/cisco/enic/enic_ethtool.c |  4 +++
>  2 files changed, 53 insertions(+)
>  create mode 100644 drivers/net/ethernet/cisco/enic/driver_utils.h
> 
> diff --git a/drivers/net/ethernet/cisco/enic/driver_utils.h b/drivers/net/ethernet/cisco/enic/driver_utils.h
> new file mode 100644
> index 0000000..e654b4d
> --- /dev/null
> +++ b/drivers/net/ethernet/cisco/enic/driver_utils.h
> @@ -0,0 +1,49 @@
> +/**
> + * Copyright 2013 Cisco Systems, Inc.  All rights reserved.
> + *
> + * This program is free software; you may redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
> + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
> + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
> + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> + * SOFTWARE.
> + *
> + */
> +
> +#ifndef __DRIVER_UTILS_H__
> +#define __DRIVER_UTILS_H__
> +
> +#include <linux/string.h>
> +
> +static inline int driver_encode_asic_info(char *str, int strlen, u16 asic_type,
> +				u16 asic_rev)
> +{
> +	if (strlen < sizeof(asic_type) + sizeof(asic_rev))
> +		return -EINVAL;
> +
> +	memcpy(str, &asic_type, sizeof(asic_type));
> +	memcpy(str + sizeof(asic_type), &asic_rev, sizeof(asic_rev));
> +
> +	return 0;
> +}
> +
> +static inline int driver_decode_asic_info(char *str, int strlen, u16 *asic_type,
> +						u16 *asic_rev)
> +{
> +	if (strlen < sizeof(*asic_type) + sizeof(*asic_rev))
> +		return -EINVAL;
> +
> +	if (asic_type)
> +		memcpy(asic_type, str, sizeof(*asic_type));
> +	if (asic_rev)
> +		memcpy(asic_rev, str + sizeof(*asic_type), sizeof(*asic_rev));
> +	return 0;
> +}
> +
> +#endif /*!__DRIVER_UTILS_H__*/
> diff --git a/drivers/net/ethernet/cisco/enic/enic_ethtool.c b/drivers/net/ethernet/cisco/enic/enic_ethtool.c
> index 47e3562..c5d938a 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_ethtool.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_ethtool.c
> @@ -19,6 +19,7 @@
>  #include <linux/netdevice.h>
>  #include <linux/ethtool.h>
>  
> +#include "driver_utils.h"
>  #include "enic_res.h"
>  #include "enic.h"
>  #include "enic_dev.h"
> @@ -116,6 +117,9 @@ static void enic_get_drvinfo(struct net_device *netdev,
>  		sizeof(drvinfo->fw_version));
>  	strlcpy(drvinfo->bus_info, pci_name(enic->pdev),
>  		sizeof(drvinfo->bus_info));
> +	memset(drvinfo->reserved1, 0, sizeof(drvinfo->reserved1));
> +	driver_encode_asic_info(drvinfo->reserved1, sizeof(drvinfo->reserved1),
> +		fw_info->asic_type, fw_info->asic_rev);
>  }

If you want to use a reserved field, then make it a first class citizen.
Rename it to asic_info, make sure the result is okay for other drivers
and add send patch so Ben can make it part of normal ethtool support.

Otherwise, this code is likely to break when someone else actually unreserves
that field.

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox