Netdev List
 help / color / mirror / Atom feed
* [PATCH] ip6_gre: reload ipv6h in prepare_ip6gre_xmit_ipv6
From: Haishuang Yan @ 2019-07-24 12:00 UTC (permalink / raw)
  To: David S. Miller, Alexey Kuznetsov
  Cc: netdev, linux-kernel, Haishuang Yan, William Tu

Since ip6_tnl_parse_tlv_enc_lim() can call pskb_may_pull()
which may change skb->data, so we need to re-load ipv6h at
the right place.

Fixes: 898b29798e36 ("ip6_gre: Refactor ip6gre xmit codes")
Cc: William Tu <u9012063@gmail.com>
Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com>
---
 net/ipv6/ip6_gre.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index c2049c7..dd2d0b96 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -660,12 +660,13 @@ static int prepare_ip6gre_xmit_ipv6(struct sk_buff *skb,
 				    struct flowi6 *fl6, __u8 *dsfield,
 				    int *encap_limit)
 {
-	struct ipv6hdr *ipv6h = ipv6_hdr(skb);
+	struct ipv6hdr *ipv6h;
 	struct ip6_tnl *t = netdev_priv(dev);
 	__u16 offset;
 
 	offset = ip6_tnl_parse_tlv_enc_lim(skb, skb_network_header(skb));
 	/* ip6_tnl_parse_tlv_enc_lim() might have reallocated skb->head */
+	ipv6h = ipv6_hdr(skb);
 
 	if (offset > 0) {
 		struct ipv6_tlv_tnl_enc_lim *tel;
-- 
1.8.3.1




^ permalink raw reply related

* Re: [PATCH 4.4 stable net] net: tcp: Fix use-after-free in tcp_write_xmit
From: maowenan @ 2019-07-24 12:13 UTC (permalink / raw)
  To: Greg KH; +Cc: stable, davem, netdev, linux-kernel
In-Reply-To: <20190724110524.GA4472@kroah.com>



On 2019/7/24 19:05, Greg KH wrote:
> On Wed, Jul 24, 2019 at 05:17:15PM +0800, Mao Wenan wrote:
>> There is one report about tcp_write_xmit use-after-free with version 4.4.136:
>>
>> BUG: KASAN: use-after-free in tcp_skb_pcount include/net/tcp.h:796 [inline]
>> BUG: KASAN: use-after-free in tcp_init_tso_segs net/ipv4/tcp_output.c:1619 [inline]
>> BUG: KASAN: use-after-free in tcp_write_xmit+0x3fc2/0x4cb0 net/ipv4/tcp_output.c:2056
>> Read of size 2 at addr ffff8801d6fc87b0 by task syz-executor408/4195
>>
>> CPU: 0 PID: 4195 Comm: syz-executor408 Not tainted 4.4.136-gfb7e319 #59
>> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
>>  0000000000000000 7d8f38ecc03be946 ffff8801d73b7710 ffffffff81e0edad
>>  ffffea00075bf200 ffff8801d6fc87b0 0000000000000000 ffff8801d6fc87b0
>>  dffffc0000000000 ffff8801d73b7748 ffffffff815159b6 ffff8801d6fc87b0
>> Call Trace:
>>  [<ffffffff81e0edad>] __dump_stack lib/dump_stack.c:15 [inline]
>>  [<ffffffff81e0edad>] dump_stack+0xc1/0x124 lib/dump_stack.c:51
>>  [<ffffffff815159b6>] print_address_description+0x6c/0x216 mm/kasan/report.c:252
>>  [<ffffffff81515cd5>] kasan_report_error mm/kasan/report.c:351 [inline]
>>  [<ffffffff81515cd5>] kasan_report.cold.7+0x175/0x2f7 mm/kasan/report.c:408
>>  [<ffffffff814f9784>] __asan_report_load2_noabort+0x14/0x20 mm/kasan/report.c:427
>>  [<ffffffff83286582>] tcp_skb_pcount include/net/tcp.h:796 [inline]
>>  [<ffffffff83286582>] tcp_init_tso_segs net/ipv4/tcp_output.c:1619 [inline]
>>  [<ffffffff83286582>] tcp_write_xmit+0x3fc2/0x4cb0 net/ipv4/tcp_output.c:2056
>>  [<ffffffff83287a40>] __tcp_push_pending_frames+0xa0/0x290 net/ipv4/tcp_output.c:2307
>>  [<ffffffff8328e966>] tcp_send_fin+0x176/0xab0 net/ipv4/tcp_output.c:2883
>>  [<ffffffff8324c0d0>] tcp_close+0xca0/0xf70 net/ipv4/tcp.c:2112
>>  [<ffffffff832f8d0f>] inet_release+0xff/0x1d0 net/ipv4/af_inet.c:435
>>  [<ffffffff82f1a156>] sock_release+0x96/0x1c0 net/socket.c:586
>>  [<ffffffff82f1a296>] sock_close+0x16/0x20 net/socket.c:1037
>>  [<ffffffff81522da5>] __fput+0x235/0x6f0 fs/file_table.c:208
>>  [<ffffffff815232e5>] ____fput+0x15/0x20 fs/file_table.c:244
>>  [<ffffffff8118bd7f>] task_work_run+0x10f/0x190 kernel/task_work.c:115
>>  [<ffffffff81135285>] exit_task_work include/linux/task_work.h:21 [inline]
>>  [<ffffffff81135285>] do_exit+0x9e5/0x26b0 kernel/exit.c:759
>>  [<ffffffff8113b1d1>] do_group_exit+0x111/0x330 kernel/exit.c:889
>>  [<ffffffff8115e5cc>] get_signal+0x4ec/0x14b0 kernel/signal.c:2321
>>  [<ffffffff8100e02b>] do_signal+0x8b/0x1d30 arch/x86/kernel/signal.c:712
>>  [<ffffffff8100360a>] exit_to_usermode_loop+0x11a/0x160 arch/x86/entry/common.c:248
>>  [<ffffffff81006535>] prepare_exit_to_usermode arch/x86/entry/common.c:283 [inline]
>>  [<ffffffff81006535>] syscall_return_slowpath+0x1b5/0x1f0 arch/x86/entry/common.c:348
>>  [<ffffffff838c29b5>] int_ret_from_sys_call+0x25/0xa3
>>
>> Allocated by task 4194:
>>  [<ffffffff810341d6>] save_stack_trace+0x26/0x50 arch/x86/kernel/stacktrace.c:63
>>  [<ffffffff814f8873>] save_stack+0x43/0xd0 mm/kasan/kasan.c:512
>>  [<ffffffff814f8b57>] set_track mm/kasan/kasan.c:524 [inline]
>>  [<ffffffff814f8b57>] kasan_kmalloc+0xc7/0xe0 mm/kasan/kasan.c:616
>>  [<ffffffff814f9122>] kasan_slab_alloc+0x12/0x20 mm/kasan/kasan.c:554
>>  [<ffffffff814f4c1e>] slab_post_alloc_hook mm/slub.c:1349 [inline]
>>  [<ffffffff814f4c1e>] slab_alloc_node mm/slub.c:2615 [inline]
>>  [<ffffffff814f4c1e>] slab_alloc mm/slub.c:2623 [inline]
>>  [<ffffffff814f4c1e>] kmem_cache_alloc+0xbe/0x2a0 mm/slub.c:2628
>>  [<ffffffff82f380a6>] kmem_cache_alloc_node include/linux/slab.h:350 [inline]
>>  [<ffffffff82f380a6>] __alloc_skb+0xe6/0x600 net/core/skbuff.c:218
>>  [<ffffffff832466c3>] alloc_skb_fclone include/linux/skbuff.h:856 [inline]
>>  [<ffffffff832466c3>] sk_stream_alloc_skb+0xa3/0x5d0 net/ipv4/tcp.c:833
>>  [<ffffffff83249164>] tcp_sendmsg+0xd34/0x2b00 net/ipv4/tcp.c:1178
>>  [<ffffffff83300ef3>] inet_sendmsg+0x203/0x4d0 net/ipv4/af_inet.c:755
>>  [<ffffffff82f1e1fc>] sock_sendmsg_nosec net/socket.c:625 [inline]
>>  [<ffffffff82f1e1fc>] sock_sendmsg+0xcc/0x110 net/socket.c:635
>>  [<ffffffff82f1eedc>] SYSC_sendto+0x21c/0x370 net/socket.c:1665
>>  [<ffffffff82f21560>] SyS_sendto+0x40/0x50 net/socket.c:1633
>>  [<ffffffff838c2825>] entry_SYSCALL_64_fastpath+0x22/0x9e
>>
>> Freed by task 4194:
>>  [<ffffffff810341d6>] save_stack_trace+0x26/0x50 arch/x86/kernel/stacktrace.c:63
>>  [<ffffffff814f8873>] save_stack+0x43/0xd0 mm/kasan/kasan.c:512
>>  [<ffffffff814f91a2>] set_track mm/kasan/kasan.c:524 [inline]
>>  [<ffffffff814f91a2>] kasan_slab_free+0x72/0xc0 mm/kasan/kasan.c:589
>>  [<ffffffff814f632e>] slab_free_hook mm/slub.c:1383 [inline]
>>  [<ffffffff814f632e>] slab_free_freelist_hook mm/slub.c:1405 [inline]
>>  [<ffffffff814f632e>] slab_free mm/slub.c:2859 [inline]
>>  [<ffffffff814f632e>] kmem_cache_free+0xbe/0x340 mm/slub.c:2881
>>  [<ffffffff82f3527f>] kfree_skbmem+0xcf/0x100 net/core/skbuff.c:635
>>  [<ffffffff82f372fd>] __kfree_skb+0x1d/0x20 net/core/skbuff.c:676
>>  [<ffffffff83288834>] sk_wmem_free_skb include/net/sock.h:1447 [inline]
>>  [<ffffffff83288834>] tcp_write_queue_purge include/net/tcp.h:1460 [inline]
>>  [<ffffffff83288834>] tcp_connect_init net/ipv4/tcp_output.c:3122 [inline]
>>  [<ffffffff83288834>] tcp_connect+0xb24/0x30c0 net/ipv4/tcp_output.c:3261
>>  [<ffffffff8329b991>] tcp_v4_connect+0xf31/0x1890 net/ipv4/tcp_ipv4.c:246
>>  [<ffffffff832f9ca9>] __inet_stream_connect+0x2a9/0xc30 net/ipv4/af_inet.c:615
>>  [<ffffffff832fa685>] inet_stream_connect+0x55/0xa0 net/ipv4/af_inet.c:676
>>  [<ffffffff82f1eb78>] SYSC_connect+0x1b8/0x300 net/socket.c:1557
>>  [<ffffffff82f214b4>] SyS_connect+0x24/0x30 net/socket.c:1538
>>  [<ffffffff838c2825>] entry_SYSCALL_64_fastpath+0x22/0x9e
>>
>> Syzkaller reproducer():
>> r0 = socket$packet(0x11, 0x3, 0x300)
>> r1 = socket$inet_tcp(0x2, 0x1, 0x0)
>> bind$inet(r1, &(0x7f0000000300)={0x2, 0x4e21, @multicast1}, 0x10)
>> connect$inet(r1, &(0x7f0000000140)={0x2, 0x1000004e21, @loopback}, 0x10)
>> recvmmsg(r1, &(0x7f0000001e40)=[{{0x0, 0x0, &(0x7f0000000100)=[{&(0x7f00000005c0)=""/88, 0x58}], 0x1}}], 0x1, 0x40000000, 0x0)
>> sendto$inet(r1, &(0x7f0000000000)="e2f7ad5b661c761edf", 0x9, 0x8080, 0x0, 0x0)
>> r2 = fcntl$dupfd(r1, 0x0, r0)
>> connect$unix(r2, &(0x7f00000001c0)=@file={0x0, './file0\x00'}, 0x6e)
>>
>> C repro link: https://syzkaller.appspot.com/text?tag=ReproC&x=14db474f800000
>>
>> This is because when tcp_connect_init call tcp_write_queue_purge, it will
>> kfree all the skb in the write_queue, but the sk->sk_send_head forget to set NULL,
>> then tcp_write_xmit try to send skb, which has freed in tcp_write_queue_purge, UAF happens.
>>
>> Signed-off-by: Mao Wenan <maowenan@huawei.com>
>> ---
>>  include/net/tcp.h | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/include/net/tcp.h b/include/net/tcp.h
>> index bf8a0dae977a..8f8aace28cf8 100644
>> --- a/include/net/tcp.h
>> +++ b/include/net/tcp.h
>> @@ -1457,6 +1457,7 @@ static inline void tcp_write_queue_purge(struct sock *sk)
>>  
>>  	while ((skb = __skb_dequeue(&sk->sk_write_queue)) != NULL)
>>  		sk_wmem_free_skb(sk, skb);
>> +	sk->sk_send_head = NULL;
>>  	sk_mem_reclaim(sk);
>>  	tcp_clear_all_retrans_hints(tcp_sk(sk));
>>  	inet_csk(sk)->icsk_backoff = 0;
> 
> Does this corrispond with a specific commit that is already in Linus's
> tree?  If not, why, did we change/mess something up when doing
> backports, or is the code just that different?
> 
> Also, is this needed in 4.9.y, 4.14.y, 4.19.y, and/or 5.2.y?  Why just
> 4.4.y?

Is it the commit 75c119afe14f? It does not use sk_send_head to indicate whether it has skb to be sent.

commit 75c119afe14f74b4dd967d75ed9f57ab6c0ef045
Author: Eric Dumazet <edumazet@google.com>
Date:   Thu Oct 5 22:21:27 2017 -0700

    tcp: implement rb-tree based retransmit queue


 static inline struct sk_buff *tcp_send_head(const struct sock *sk)
 {
-       return sk->sk_send_head;
+       return skb_peek(&sk->sk_write_queue);
 }



> 
> thanks,
> 
> greg k-h
> 
> .
> 


^ permalink raw reply

* Re: [RFC PATCH net-next 00/12] drop_monitor: Capture dropped packets and metadata
From: Toke Høiland-Jørgensen @ 2019-07-24  9:51 UTC (permalink / raw)
  To: Ido Schimmel
  Cc: netdev, davem, nhorman, dsahern, roopa, nikolay, jakub.kicinski,
	andy, f.fainelli, andrew, vivien.didelot, mlxsw, Ido Schimmel
In-Reply-To: <20190724081042.GB15878@splinter>

Ido Schimmel <idosch@idosch.org> writes:

> On Tue, Jul 23, 2019 at 06:08:21PM +0200, Toke Høiland-Jørgensen wrote:
>> Also, presumably the queue will have to change from a struct
>> sk_buff_head to something that can hold XDP frames and whatever devlink
>> puts there as well, right?
>
> Good point!
>
> For HW drops we get an SKB and relevant metadata from devlink about why
> the packet was dropped etc. I plan to store a pointer to this metadata
> in the SKB control block.
>
> Let me see how the implementation goes. Even if use sk_buff_head for
> now, I will make sure that converting to a more generalized data
> structure is straightforward.

Yup, that's fine, just wanted to make sure you were aware of the
eventual need to change it :)

-Toke

^ permalink raw reply

* Re: [PATCH 4.4 stable net] net: tcp: Fix use-after-free in tcp_write_xmit
From: maowenan @ 2019-07-24 12:29 UTC (permalink / raw)
  To: Eric Dumazet, davem, gregkh, netdev, linux-kernel
In-Reply-To: <c7eb64e0-702d-0792-9aea-c303454e7a33@huawei.com>



On 2019/7/24 18:38, maowenan wrote:
> 
> 
> On 2019/7/24 18:13, Eric Dumazet wrote:
>>
>>
>> On 7/24/19 12:01 PM, Eric Dumazet wrote:
>>>
>>>
>>> On 7/24/19 11:17 AM, Mao Wenan wrote:
>>>> There is one report about tcp_write_xmit use-after-free with version 4.4.136:
>>>
>>> Current stable 4.4 is 4.4.186
>>>
>>> Can you check the bug is still there ?
>>>
>>
>> BTW, I tried the C repro and another bug showed up.
>>
>> It looks like 4.4.186 misses other fixes :/
> 
> I will try 4.4.186.

Hi Eric, bug exist in latest commit for 4.4 stable.
a3e421f Linux 4.4.186

> 
>>
>> [  180.811610] skbuff: skb_under_panic: text:ffffffff825ec6ea len:156 put:84 head:ffff8837dd1f0990 data:ffff8837dd1f098c tail:0x98 end:0xc0 dev:ip6gre0
>> [  180.825037] ------------[ cut here ]------------
>> [  180.829688] kernel BUG at net/core/skbuff.c:104!
>> [  180.834316] invalid opcode: 0000 [#1] SMP KASAN
>> [  180.839305] gsmi: Log Shutdown Reason 0x03
>> [  180.843426] Modules linked in: ipip bonding bridge stp llc tun veth w1_therm wire i2c_mux_pca954x i2c_mux cdc_acm ehci_pci ehci_hcd ip_gre mlx4_en ib_uverbs mlx4_ib ib_sa ib_mad ib_core ib_addr mlx4_core
>> [  180.862052] CPU: 22 PID: 1619 Comm: kworker/22:1 Not tainted 4.4.186-smp-DEV #41
>> [  180.869475] Hardware name: Intel BIOS 2.56.0 10/19/2018
>> [  180.876463] Workqueue: ipv6_addrconf addrconf_dad_work
>> [  180.881658] task: ffff8837f1f59d80 ti: ffff8837eeeb8000 task.ti: ffff8837eeeb8000
>> [  180.889171] RIP: 0010:[<ffffffff821ef26f>]  [<ffffffff821ef26f>] skb_panic+0x14f/0x210
>> [  180.897162] RSP: 0018:ffff8837eeebf4b8  EFLAGS: 00010282
>> [  180.902504] RAX: 0000000000000088 RBX: ffff8837eeeeb600 RCX: 0000000000000000
>> [  180.909645] RDX: 0000000000000000 RSI: 0000000000000246 RDI: ffffffff83508c00
>> [  180.916854] RBP: ffff8837eeebf520 R08: 0000000000000016 R09: 0000000000000000
>> [  180.924029] R10: ffff881fc8abf038 R11: 0000000000000007 R12: ffff881fc8abe720
>> [  180.931213] R13: ffffffff82aa9e80 R14: 00000000000000c0 R15: 0000000000000098
>> [  180.938390] FS:  0000000000000000(0000) GS:ffff8837ff280000(0000) knlGS:0000000000000000
>> [  180.946519] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> [  180.952290] CR2: 00007f519426f530 CR3: 00000037d37f2000 CR4: 0000000000160670
>> [  180.959447] Stack:
>> [  180.961458]  ffff8837dd1f098c 0000000000000098 00000000000000c0 ffff881fc8abe720
>> [  180.968909]  ffffea00df747c00 ffff881fff404b40 ffff8837ff2a1a20 ffff8837eeebf5b8
>> [  180.976371]  ffff8837eeeeb600 ffffffff825ec6ea 1ffff106fddd7eb6 ffff8837eeeeb600
>> [  180.983848] Call Trace:
>> [  180.986297]  [<ffffffff825ec6ea>] ? ip6gre_header+0xba/0xd50
>> [  180.991962]  [<ffffffff821f0e01>] skb_push+0xc1/0x100
>> [  180.997023]  [<ffffffff825ec6ea>] ip6gre_header+0xba/0xd50
>> [  181.002519]  [<ffffffff8158dc16>] ? memcpy+0x36/0x40
>> [  181.007509]  [<ffffffff825ec630>] ? ip6gre_changelink+0x6d0/0x6d0
>> [  181.013629]  [<ffffffff82550741>] ? ndisc_constructor+0x5b1/0x770
>> [  181.019728]  [<ffffffff82666861>] ? _raw_write_unlock_bh+0x41/0x50
>> [  181.025924]  [<ffffffff8226540b>] ? __neigh_create+0xe6b/0x1670
>> [  181.031851]  [<ffffffff8225817f>] neigh_connected_output+0x23f/0x480
>> [  181.038219]  [<ffffffff824f61ec>] ip6_finish_output2+0x74c/0x1a90
>> [  181.044324]  [<ffffffff810f1d33>] ? print_context_stack+0x73/0xf0
>> [  181.050429]  [<ffffffff824f5aa0>] ? ip6_xmit+0x1700/0x1700
>> [  181.055933]  [<ffffffff82304a28>] ? nf_hook_slow+0x118/0x1b0
>> [  181.061617]  [<ffffffff82502d7a>] ip6_finish_output+0x2ba/0x580
>> [  181.067546]  [<ffffffff82503179>] ip6_output+0x139/0x380
>> [  181.072884]  [<ffffffff82503040>] ? ip6_finish_output+0x580/0x580
>> [  181.079004]  [<ffffffff82502ac0>] ? ip6_fragment+0x31b0/0x31b0
>> [  181.084852]  [<ffffffff82251b51>] ? dst_init+0x4b1/0x820
>> [  181.090172]  [<ffffffff8158da45>] ? kasan_unpoison_shadow+0x35/0x50
>> [  181.096437]  [<ffffffff8158da45>] ? kasan_unpoison_shadow+0x35/0x50
>> [  181.102712]  [<ffffffff8254f3ca>] NF_HOOK_THRESH.constprop.22+0xca/0x180
>> [  181.109421]  [<ffffffff8254f300>] ? ndisc_alloc_skb+0x340/0x340
>> [  181.115338]  [<ffffffff8254d820>] ? compat_ipv6_setsockopt+0x180/0x180
>> [  181.121874]  [<ffffffff8254fbc2>] ndisc_send_skb+0x742/0xd10
>> [  181.127550]  [<ffffffff8254f480>] ? NF_HOOK_THRESH.constprop.22+0x180/0x180
>> [  181.134516]  [<ffffffff821f2440>] ? skb_complete_tx_timestamp+0x280/0x280
>> [  181.141311]  [<ffffffff8254e2b3>] ? ndisc_fill_addr_option+0x193/0x260
>> [  181.147844]  [<ffffffff82553bd9>] ndisc_send_rs+0x179/0x2d0
>> [  181.153426]  [<ffffffff8251e7df>] addrconf_dad_completed+0x41f/0x7c0
>> [  181.159795]  [<ffffffff81297f78>] ? pick_next_entity+0x198/0x470
>> [  181.165807]  [<ffffffff8251e3c0>] ? addrconf_rs_timer+0x4a0/0x4a0
>> [  181.171918]  [<ffffffff81aab928>] ? find_next_bit+0x18/0x20
>> [  181.177504]  [<ffffffff81a99ec9>] ? prandom_seed+0xd9/0x160
>> [  181.183095]  [<ffffffff8251eef5>] addrconf_dad_work+0x375/0x9e0
>> [  181.189024]  [<ffffffff8251eb80>] ? addrconf_dad_completed+0x7c0/0x7c0
>> [  181.195576]  [<ffffffff81249d8f>] process_one_work+0x52f/0xf60
>> [  181.201468]  [<ffffffff8124a89d>] worker_thread+0xdd/0xe80
>> [  181.206977]  [<ffffffff8265cf0a>] ? __schedule+0x73a/0x16d0
>> [  181.212550]  [<ffffffff8124a7c0>] ? process_one_work+0xf60/0xf60
>> [  181.218572]  [<ffffffff8125a115>] kthread+0x205/0x2b0
>> [  181.223633]  [<ffffffff81259f10>] ? kthread_worker_fn+0x4e0/0x4e0
>> [  181.229743]  [<ffffffff81259f10>] ? kthread_worker_fn+0x4e0/0x4e0
>> [  181.235834]  [<ffffffff8266726f>] ret_from_fork+0x3f/0x70
>> [  181.241232]  [<ffffffff81259f10>] ? kthread_worker_fn+0x4e0/0x4e0
>>
>>
>> .
>>
> 
> 
> .
> 


^ permalink raw reply

* [PATCH] r8169: fix a typo in a comment
From: Corentin Musard @ 2019-07-24 12:34 UTC (permalink / raw)
  To: David S. Miller, netdev, linux-kernel; +Cc: trivial

Replace "additonal" by "additional" in a comment.
Typo found by checkpatch.pl.

Signed-off-by: Corentin Musard <corentinmusard@gmail.com>
---
 drivers/net/ethernet/realtek/r8169_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index 0637c6752a78..7231ab3573ff 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -6334,7 +6334,7 @@ rtl8169_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats)
 	stats->multicast	= dev->stats.multicast;
 
 	/*
-	 * Fetch additonal counter values missing in stats collected by driver
+	 * Fetch additional counter values missing in stats collected by driver
 	 * from tally counters.
 	 */
 	if (pm_runtime_active(&pdev->dev))
-- 
2.22.0


^ permalink raw reply related

* [PATCH 1/2] net: mac80211: Fix possible null-pointer dereferences in ieee80211_setup_sdata()
From: Jia-Ju Bai @ 2019-07-24 12:36 UTC (permalink / raw)
  To: johannes, davem; +Cc: linux-wireless, netdev, linux-kernel, Jia-Ju Bai

In ieee80211_setup_sdata(), there is an if statement on line 1410 to
check whether sdata->dev is NULL:
    if (sdata->dev)

When sdata->dev is NULL, it is used on lines 1458 and 1459:
    sdata->dev->type = ARPHRD_IEEE80211_RADIOTAP;
    sdata->dev->netdev_ops = &ieee80211_monitorif_ops;

Thus, possible null-pointer dereferences may occur.

To fix these bugs, sdata->dev is checked before being used.

These bugs are found by a static analysis tool STCheck written by us.

Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
---
 net/mac80211/iface.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 06aac0aaae64..e49264981a7b 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -1455,8 +1455,10 @@ static void ieee80211_setup_sdata(struct ieee80211_sub_if_data *sdata,
 			ieee80211_mesh_init_sdata(sdata);
 		break;
 	case NL80211_IFTYPE_MONITOR:
-		sdata->dev->type = ARPHRD_IEEE80211_RADIOTAP;
-		sdata->dev->netdev_ops = &ieee80211_monitorif_ops;
+		if (sdata->dev) {
+			sdata->dev->type = ARPHRD_IEEE80211_RADIOTAP;
+			sdata->dev->netdev_ops = &ieee80211_monitorif_ops;
+		}
 		sdata->u.mntr.flags = MONITOR_FLAG_CONTROL |
 				      MONITOR_FLAG_OTHER_BSS;
 		break;
-- 
2.17.0


^ permalink raw reply related

* [PATCH 2/2] net: mac80211: Fix possible null-pointer dereferences in ieee80211_xmit_fast_finish()
From: Jia-Ju Bai @ 2019-07-24 12:36 UTC (permalink / raw)
  To: johannes, davem; +Cc: linux-wireless, netdev, linux-kernel, Jia-Ju Bai

In ieee80211_xmit_fast_finish(), there is an if statement on line 3356
to check whether key is NULL:
    if (key)

When key is NULL, it is used on line 3388:
    switch (key->conf.cipher)
and line 3393:
    pn = atomic64_inc_return(&key->conf.tx_pn);
and line 3396:
    crypto_hdr[3] = 0x20 | (key->conf.keyidx << 6);

Thus, possible null-pointer dereferences may occur.

To fix these bugs, key is checked on line 3384.

These bugs are found by a static analysis tool STCheck written by us.

Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
---
 net/mac80211/tx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index f13eb2f61ccf..2cc261165b91 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -3381,7 +3381,7 @@ static void ieee80211_xmit_fast_finish(struct ieee80211_sub_if_data *sdata,
 	sta->tx_stats.bytes[skb_get_queue_mapping(skb)] += skb->len;
 	sta->tx_stats.packets[skb_get_queue_mapping(skb)]++;
 
-	if (pn_offs) {
+	if (key && pn_offs) {
 		u64 pn;
 		u8 *crypto_hdr = skb->data + pn_offs;
 
-- 
2.17.0


^ permalink raw reply related

* Re: [PATCH net-next 1/4] sctp: check addr_size with sa_family_t size in __sctp_setsockopt_connectx
From: Marcelo Ricardo Leitner @ 2019-07-24 12:36 UTC (permalink / raw)
  To: Neil Horman; +Cc: Xin Long, network dev, linux-sctp, davem
In-Reply-To: <20190724112235.GA7212@hmswarspite.think-freely.org>

On Wed, Jul 24, 2019 at 07:22:35AM -0400, Neil Horman wrote:
> On Wed, Jul 24, 2019 at 03:21:12PM +0800, Xin Long wrote:
> > On Tue, Jul 23, 2019 at 11:25 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> > >
> > > On Tue, Jul 23, 2019 at 01:37:57AM +0800, Xin Long wrote:
> > > > Now __sctp_connect() is called by __sctp_setsockopt_connectx() and
> > > > sctp_inet_connect(), the latter has done addr_size check with size
> > > > of sa_family_t.
> > > >
> > > > In the next patch to clean up __sctp_connect(), we will remove
> > > > addr_size check with size of sa_family_t from __sctp_connect()
> > > > for the 1st address.
> > > >
> > > > So before doing that, __sctp_setsockopt_connectx() should do
> > > > this check first, as sctp_inet_connect() does.
> > > >
> > > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > > ---
> > > >  net/sctp/socket.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > > > index aa80cda..5f92e4a 100644
> > > > --- a/net/sctp/socket.c
> > > > +++ b/net/sctp/socket.c
> > > > @@ -1311,7 +1311,7 @@ static int __sctp_setsockopt_connectx(struct sock *sk,
> > > >       pr_debug("%s: sk:%p addrs:%p addrs_size:%d\n",
> > > >                __func__, sk, addrs, addrs_size);
> > > >
> > > > -     if (unlikely(addrs_size <= 0))
> > > > +     if (unlikely(addrs_size < sizeof(sa_family_t)))
> > > I don't think this is what you want to check for here.  sa_family_t is
> > > an unsigned short, and addrs_size is the number of bytes in the addrs
> > > array.  The addrs array should be at least the size of one struct
> > > sockaddr (16 bytes iirc), and, if larger, should be a multiple of
> > > sizeof(struct sockaddr)
> > sizeof(struct sockaddr) is not the right value to check either.
> > 
> > The proper check will be done later in __sctp_connect():
> > 
> >         af = sctp_get_af_specific(daddr->sa.sa_family);
> >         if (!af || af->sockaddr_len > addrs_size)
> >                 return -EINVAL;
> > 
> > So the check 'addrs_size < sizeof(sa_family_t)' in this patch is
> > just to make sure daddr->sa.sa_family is accessible. the same
> > check is also done in sctp_inet_connect().
> > 
> That doesn't make much sense, if the proper check is done in __sctp_connect with
> the size of the families sockaddr_len, then we don't need this check at all, we
> can just let memdup_user take the fault on copy_to_user and return -EFAULT.  If
> we get that from memdup_user, we know its not accessible, and can bail out.
> 
> About the only thing we need to check for here is that addr_len isn't some
> absurdly high value (i.e. a negative value), so that we avoid trying to kmalloc
> upwards of 2G in memdup_user.  Your change does that just fine, but its no
> better or worse than checking for <=0

One can argue that such check against absurdly high values is random
and not effective, as 2G can be somewhat reasonable on 8GB systems but
certainly isn't on 512MB ones. On that, kmemdup_user() will also fail
gracefully as it uses GFP_USER and __GFP_NOWARN.

The original check is more for protecting for sane usage of the
variable, which is an int, and a negative value is questionable. We
could cast, yes, but.. was that really the intent of the application?
Probably not.

> 
> Neil
> 
> > >
> > > Neil
> > >
> > > >               return -EINVAL;
> > > >
> > > >       kaddrs = memdup_user(addrs, addrs_size);
> > > > --
> > > > 2.1.0
> > > >
> > > >
> > 

^ permalink raw reply

* Re: [PATCH 2/2] net: mac80211: Fix possible null-pointer dereferences in ieee80211_xmit_fast_finish()
From: Johannes Berg @ 2019-07-24 12:39 UTC (permalink / raw)
  To: Jia-Ju Bai, davem; +Cc: linux-wireless, netdev, linux-kernel
In-Reply-To: <20190724123633.10145-1-baijiaju1990@gmail.com>

On Wed, 2019-07-24 at 20:36 +0800, Jia-Ju Bai wrote:
> In ieee80211_xmit_fast_finish(), there is an if statement on line 3356
> to check whether key is NULL:
>     if (key)
> 
> When key is NULL, it is used on line 3388:
>     switch (key->conf.cipher)
> and line 3393:
>     pn = atomic64_inc_return(&key->conf.tx_pn);
> and line 3396:
>     crypto_hdr[3] = 0x20 | (key->conf.keyidx << 6);
> 
> Thus, possible null-pointer dereferences may occur.

No, this cannot happen, because pn_offs can only be non-zero when there
is a key. Maybe we should pass the fast_tx pointer instead of the
pn_offs/key from it, but the two are coupled.

johannes


^ permalink raw reply

* Re: [PATCH 1/2] net: mac80211: Fix possible null-pointer dereferences in ieee80211_setup_sdata()
From: Johannes Berg @ 2019-07-24 12:40 UTC (permalink / raw)
  To: Jia-Ju Bai, davem; +Cc: linux-wireless, netdev, linux-kernel
In-Reply-To: <20190724123623.10093-1-baijiaju1990@gmail.com>

On Wed, 2019-07-24 at 20:36 +0800, Jia-Ju Bai wrote:
> In ieee80211_setup_sdata(), there is an if statement on line 1410 to
> check whether sdata->dev is NULL:
>     if (sdata->dev)
> 
> When sdata->dev is NULL, it is used on lines 1458 and 1459:
>     sdata->dev->type = ARPHRD_IEEE80211_RADIOTAP;
>     sdata->dev->netdev_ops = &ieee80211_monitorif_ops;
> 
> Thus, possible null-pointer dereferences may occur.

No, this cannot happen, monitor interfaces (NL80211_IFTYPE_MONITOR) must
have a valid ->dev, only P2P device and NAN might not.

johannes


^ permalink raw reply

* Re: [PATCH net-next 1/4] sctp: check addr_size with sa_family_t size in __sctp_setsockopt_connectx
From: Marcelo Ricardo Leitner @ 2019-07-24 12:49 UTC (permalink / raw)
  To: Neil Horman; +Cc: Xin Long, network dev, linux-sctp, davem
In-Reply-To: <20190724123650.GD6204@localhost.localdomain>

On Wed, Jul 24, 2019 at 09:36:50AM -0300, Marcelo Ricardo Leitner wrote:
> On Wed, Jul 24, 2019 at 07:22:35AM -0400, Neil Horman wrote:
> > On Wed, Jul 24, 2019 at 03:21:12PM +0800, Xin Long wrote:
> > > On Tue, Jul 23, 2019 at 11:25 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> > > >
> > > > On Tue, Jul 23, 2019 at 01:37:57AM +0800, Xin Long wrote:
> > > > > Now __sctp_connect() is called by __sctp_setsockopt_connectx() and
> > > > > sctp_inet_connect(), the latter has done addr_size check with size
> > > > > of sa_family_t.
> > > > >
> > > > > In the next patch to clean up __sctp_connect(), we will remove
> > > > > addr_size check with size of sa_family_t from __sctp_connect()
> > > > > for the 1st address.
> > > > >
> > > > > So before doing that, __sctp_setsockopt_connectx() should do
> > > > > this check first, as sctp_inet_connect() does.
> > > > >
> > > > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > > > ---
> > > > >  net/sctp/socket.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > > > > index aa80cda..5f92e4a 100644
> > > > > --- a/net/sctp/socket.c
> > > > > +++ b/net/sctp/socket.c
> > > > > @@ -1311,7 +1311,7 @@ static int __sctp_setsockopt_connectx(struct sock *sk,
> > > > >       pr_debug("%s: sk:%p addrs:%p addrs_size:%d\n",
> > > > >                __func__, sk, addrs, addrs_size);
> > > > >
> > > > > -     if (unlikely(addrs_size <= 0))
> > > > > +     if (unlikely(addrs_size < sizeof(sa_family_t)))
> > > > I don't think this is what you want to check for here.  sa_family_t is
> > > > an unsigned short, and addrs_size is the number of bytes in the addrs
> > > > array.  The addrs array should be at least the size of one struct
> > > > sockaddr (16 bytes iirc), and, if larger, should be a multiple of
> > > > sizeof(struct sockaddr)
> > > sizeof(struct sockaddr) is not the right value to check either.
> > > 
> > > The proper check will be done later in __sctp_connect():
> > > 
> > >         af = sctp_get_af_specific(daddr->sa.sa_family);
> > >         if (!af || af->sockaddr_len > addrs_size)
> > >                 return -EINVAL;
> > > 
> > > So the check 'addrs_size < sizeof(sa_family_t)' in this patch is
> > > just to make sure daddr->sa.sa_family is accessible. the same
> > > check is also done in sctp_inet_connect().
> > > 
> > That doesn't make much sense, if the proper check is done in __sctp_connect with
> > the size of the families sockaddr_len, then we don't need this check at all, we
> > can just let memdup_user take the fault on copy_to_user and return -EFAULT.  If
> > we get that from memdup_user, we know its not accessible, and can bail out.
> > 
> > About the only thing we need to check for here is that addr_len isn't some
> > absurdly high value (i.e. a negative value), so that we avoid trying to kmalloc
> > upwards of 2G in memdup_user.  Your change does that just fine, but its no
> > better or worse than checking for <=0
> 
> One can argue that such check against absurdly high values is random
> and not effective, as 2G can be somewhat reasonable on 8GB systems but
> certainly isn't on 512MB ones. On that, kmemdup_user() will also fail
> gracefully as it uses GFP_USER and __GFP_NOWARN.
> 
> The original check is more for protecting for sane usage of the
> variable, which is an int, and a negative value is questionable. We
> could cast, yes, but.. was that really the intent of the application?
> Probably not.

Though that said, I'm okay with the new check here: a quick sanity
check that can avoid expensive calls to kmalloc(), while more refined
check is done later on.

> 
> > 
> > Neil
> > 
> > > >
> > > > Neil
> > > >
> > > > >               return -EINVAL;
> > > > >
> > > > >       kaddrs = memdup_user(addrs, addrs_size);
> > > > > --
> > > > > 2.1.0
> > > > >
> > > > >
> > > 

^ permalink raw reply

* Re: [RFC PATCH net-next 10/12] drop_monitor: Add packet alert mode
From: Jiri Pirko @ 2019-07-24 12:53 UTC (permalink / raw)
  To: Ido Schimmel
  Cc: netdev, davem, nhorman, dsahern, roopa, nikolay, jakub.kicinski,
	toke, andy, f.fainelli, andrew, vivien.didelot, mlxsw,
	Ido Schimmel
In-Reply-To: <20190722183134.14516-11-idosch@idosch.org>

Mon, Jul 22, 2019 at 08:31:32PM CEST, idosch@idosch.org wrote:
>From: Ido Schimmel <idosch@mellanox.com>
>
>So far drop monitor supported only one alert mode in which a summary of
>locations in which packets were recently dropped was sent to user space.
>
>This alert mode is sufficient in order to understand that packets were
>dropped, but lacks information to perform a more detailed analysis.
>
>Add a new alert mode in which the dropped packet itself is passed to
>user space along with metadata: The drop location (as program counter
>and resolved symbol), ingress / egress netdevice and arrival / departure
>timestamp. More metadata can be added in the future.
>
>To avoid performing expensive operations in the context in which
>kfree_skb() is invoked (can be hard IRQ), the dropped skb is cloned and
>queued on per-CPU skb drop list. Then, in process context the netlink
>message is allocated, prepared and finally sent to user space.
>
>Signed-off-by: Ido Schimmel <idosch@mellanox.com>
>---
> include/uapi/linux/net_dropmon.h |  29 ++++
> net/core/drop_monitor.c          | 287 ++++++++++++++++++++++++++++++-
> 2 files changed, 308 insertions(+), 8 deletions(-)
>
>diff --git a/include/uapi/linux/net_dropmon.h b/include/uapi/linux/net_dropmon.h
>index 5edbd0a675fd..7708c8a440a1 100644
>--- a/include/uapi/linux/net_dropmon.h
>+++ b/include/uapi/linux/net_dropmon.h
>@@ -53,6 +53,7 @@ enum {
> 	NET_DM_CMD_CONFIG,
> 	NET_DM_CMD_START,
> 	NET_DM_CMD_STOP,
>+	NET_DM_CMD_PACKET_ALERT,
> 	_NET_DM_CMD_MAX,
> };
> 
>@@ -62,4 +63,32 @@ enum {
>  * Our group identifiers
>  */
> #define NET_DM_GRP_ALERT 1
>+
>+enum net_dm_attr {
>+	NET_DM_ATTR_UNSPEC,
>+
>+	NET_DM_ATTR_ALERT_MODE,			/* u8 */
>+	NET_DM_ATTR_PC,				/* u64 */
>+	NET_DM_ATTR_SYMBOL,			/* string */
>+	NET_DM_ATTR_NETDEV_IFINDEX,		/* u32 */
>+	NET_DM_ATTR_NETDEV_NAME,		/* string */
>+	NET_DM_ATTR_TIMESTAMP,			/* struct timespec */
>+	NET_DM_ATTR_PAYLOAD,			/* binary */
>+	NET_DM_ATTR_PAD,
>+
>+	__NET_DM_ATTR_MAX,
>+	NET_DM_ATTR_MAX = __NET_DM_ATTR_MAX - 1
>+};
>+
>+/**
>+ * enum net_dm_alert_mode - Alert mode.
>+ * @NET_DM_ALERT_MODE_SUMMARY: A summary of recent drops is sent to user space.
>+ * @NET_DM_ALERT_MODE_PACKET: Each dropped packet is sent to user space along
>+ *                            with metadata.
>+ */
>+enum net_dm_alert_mode {
>+	NET_DM_ALERT_MODE_SUMMARY,
>+	NET_DM_ALERT_MODE_PACKET,
>+};
>+
> #endif
>diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c
>index f441fb653567..512935fc235b 100644
>--- a/net/core/drop_monitor.c
>+++ b/net/core/drop_monitor.c
>@@ -54,6 +54,7 @@ static DEFINE_MUTEX(net_dm_mutex);
> struct per_cpu_dm_data {
> 	spinlock_t		lock;	/* Protects 'skb' and 'send_timer' */
> 	struct sk_buff		*skb;
>+	struct sk_buff_head	drop_queue;
> 	struct work_struct	dm_alert_work;
> 	struct timer_list	send_timer;
> };
>@@ -75,6 +76,22 @@ static int dm_delay = 1;
> static unsigned long dm_hw_check_delta = 2*HZ;
> static LIST_HEAD(hw_stats_list);
> 
>+static enum net_dm_alert_mode net_dm_alert_mode = NET_DM_ALERT_MODE_SUMMARY;
>+
>+struct net_dm_skb_cb {
>+	void *pc;
>+};
>+
>+#define NET_DM_SKB_CB(__skb) ((struct net_dm_skb_cb *)&((__skb)->cb[0]))
>+
>+struct net_dm_alert_ops {
>+	void (*kfree_skb_probe)(void *ignore, struct sk_buff *skb,
>+				void *location);
>+	void (*napi_poll_probe)(void *ignore, struct napi_struct *napi,
>+				int work, int budget);
>+	void (*work_item_func)(struct work_struct *work);
>+};
>+
> static int net_dm_nl_pre_doit(const struct genl_ops *ops,
> 			      struct sk_buff *skb, struct genl_info *info)
> {
>@@ -255,10 +272,194 @@ static void trace_napi_poll_hit(void *ignore, struct napi_struct *napi,
> 	rcu_read_unlock();
> }
> 
>+static const struct net_dm_alert_ops net_dm_alert_summary_ops = {
>+	.kfree_skb_probe	= trace_kfree_skb_hit,
>+	.napi_poll_probe	= trace_napi_poll_hit,
>+	.work_item_func		= send_dm_alert,
>+};
>+
>+static void net_dm_packet_trace_kfree_skb_hit(void *ignore,
>+					      struct sk_buff *skb,
>+					      void *location)
>+{
>+	struct per_cpu_dm_data *data;
>+	struct sk_buff *nskb;
>+	unsigned long flags;
>+
>+	nskb = skb_clone(skb, GFP_ATOMIC);
>+	if (!nskb)
>+		return;
>+
>+	NET_DM_SKB_CB(nskb)->pc = location;
>+	if (nskb->dev)
>+		dev_hold(nskb->dev);
>+
>+	data = this_cpu_ptr(&dm_cpu_data);
>+	spin_lock_irqsave(&data->drop_queue.lock, flags);
>+
>+	__skb_queue_tail(&data->drop_queue, nskb);
>+
>+	if (!timer_pending(&data->send_timer)) {
>+		data->send_timer.expires = jiffies + dm_delay * HZ;
>+		add_timer(&data->send_timer);
>+	}
>+
>+	spin_unlock_irqrestore(&data->drop_queue.lock, flags);
>+}
>+
>+static void net_dm_packet_trace_napi_poll_hit(void *ignore,
>+					      struct napi_struct *napi,
>+					      int work, int budget)
>+{
>+}
>+
>+#define NET_DM_MAX_SYMBOL_LEN 40
>+
>+static size_t net_dm_packet_report_size(size_t payload_len)
>+{
>+	size_t size;
>+
>+	size = nlmsg_msg_size(GENL_HDRLEN + net_drop_monitor_family.hdrsize);
>+
>+	return NLMSG_ALIGN(size) +
>+	       /* NET_DM_ATTR_PC */
>+	       nla_total_size(sizeof(u64)) +
>+	       /* NET_DM_ATTR_SYMBOL */
>+	       nla_total_size(NET_DM_MAX_SYMBOL_LEN + 1) +
>+	       /* NET_DM_ATTR_NETDEV_IFINDEX */
>+	       nla_total_size(sizeof(u32)) +
>+	       /* NET_DM_ATTR_NETDEV_NAME */
>+	       nla_total_size(IFNAMSIZ + 1) +
>+	       /* NET_DM_ATTR_TIMESTAMP */
>+	       nla_total_size(sizeof(struct timespec)) +
>+	       /* NET_DM_ATTR_PAYLOAD */
>+	       nla_total_size(payload_len);
>+}
>+
>+static int net_dm_packet_report_fill(struct sk_buff *msg, struct sk_buff *skb,
>+				     size_t payload_len)
>+{
>+	u64 pc = (u64)(uintptr_t) NET_DM_SKB_CB(skb)->pc;
>+	char buf[NET_DM_MAX_SYMBOL_LEN];
>+	struct nlattr *attr;
>+	struct timespec ts;
>+	void *hdr;
>+
>+	hdr = genlmsg_put(msg, 0, 0, &net_drop_monitor_family, 0,
>+			  NET_DM_CMD_PACKET_ALERT);
>+	if (!hdr)
>+		return -EMSGSIZE;
>+
>+	if (nla_put_u64_64bit(msg, NET_DM_ATTR_PC, pc, NET_DM_ATTR_PAD))
>+		goto nla_put_failure;
>+
>+	snprintf(buf, sizeof(buf), "%pS", NET_DM_SKB_CB(skb)->pc);
>+	if (nla_put_string(msg, NET_DM_ATTR_SYMBOL, buf))
>+		goto nla_put_failure;
>+
>+	if (skb->dev &&
>+	    nla_put_u32(msg, NET_DM_ATTR_NETDEV_IFINDEX, skb->dev->ifindex))
>+		goto nla_put_failure;
>+
>+	if (skb->dev &&
>+	    nla_put_string(msg, NET_DM_ATTR_NETDEV_NAME, skb->dev->name))
>+		goto nla_put_failure;
>+
>+	if (ktime_to_timespec_cond(skb->tstamp, &ts) &&
>+	    nla_put(msg, NET_DM_ATTR_TIMESTAMP, sizeof(ts), &ts))
>+		goto nla_put_failure;
>+
>+	if (!payload_len)
>+		goto out;
>+
>+	attr = skb_put(msg, nla_total_size(payload_len));
>+	attr->nla_type = NET_DM_ATTR_PAYLOAD;
>+	attr->nla_len = nla_attr_size(payload_len);
>+	if (skb_copy_bits(skb, 0, nla_data(attr), payload_len))
>+		goto nla_put_failure;
>+
>+out:
>+	genlmsg_end(msg, hdr);
>+
>+	return 0;
>+
>+nla_put_failure:
>+	genlmsg_cancel(msg, hdr);
>+	return -EMSGSIZE;
>+}
>+
>+#define NET_DM_MAX_PACKET_SIZE (0xffff - NLA_HDRLEN - NLA_ALIGNTO)
>+
>+static void net_dm_packet_report(struct sk_buff *skb)
>+{
>+	struct sk_buff *msg;
>+	size_t payload_len;
>+	int rc;
>+
>+	/* Make sure we start copying the packet from the MAC header */
>+	if (skb->data > skb_mac_header(skb))
>+		skb_push(skb, skb->data - skb_mac_header(skb));
>+	else
>+		skb_pull(skb, skb_mac_header(skb) - skb->data);
>+
>+	/* Ensure packet fits inside a single netlink attribute */
>+	payload_len = min_t(size_t, skb->len, NET_DM_MAX_PACKET_SIZE);
>+
>+	msg = nlmsg_new(net_dm_packet_report_size(payload_len), GFP_KERNEL);
>+	if (!msg)
>+		goto out;
>+
>+	rc = net_dm_packet_report_fill(msg, skb, payload_len);
>+	if (rc) {
>+		nlmsg_free(msg);
>+		goto out;
>+	}
>+
>+	genlmsg_multicast(&net_drop_monitor_family, msg, 0, 0, GFP_KERNEL);
>+
>+out:
>+	if (skb->dev)
>+		dev_put(skb->dev);
>+	consume_skb(skb);
>+}
>+
>+static void net_dm_packet_work(struct work_struct *work)
>+{
>+	struct per_cpu_dm_data *data;
>+	struct sk_buff_head list;
>+	struct sk_buff *skb;
>+	unsigned long flags;
>+
>+	data = container_of(work, struct per_cpu_dm_data, dm_alert_work);
>+
>+	__skb_queue_head_init(&list);
>+
>+	spin_lock_irqsave(&data->drop_queue.lock, flags);
>+	skb_queue_splice_tail_init(&data->drop_queue, &list);
>+	spin_unlock_irqrestore(&data->drop_queue.lock, flags);
>+
>+	while ((skb = __skb_dequeue(&list)))
>+		net_dm_packet_report(skb);
>+}
>+
>+static const struct net_dm_alert_ops net_dm_alert_packet_ops = {
>+	.kfree_skb_probe	= net_dm_packet_trace_kfree_skb_hit,
>+	.napi_poll_probe	= net_dm_packet_trace_napi_poll_hit,
>+	.work_item_func		= net_dm_packet_work,
>+};
>+
>+static const struct net_dm_alert_ops *net_dm_alert_ops_arr[] = {
>+	[NET_DM_ALERT_MODE_SUMMARY]	= &net_dm_alert_summary_ops,
>+	[NET_DM_ALERT_MODE_PACKET]	= &net_dm_alert_packet_ops,
>+};

Please split this patch into 2:
1) introducing the ops and modes (only summary)
2) introducing the packet mode


>+
> static int net_dm_trace_on_set(struct netlink_ext_ack *extack)
> {
>+	const struct net_dm_alert_ops *ops;
> 	int cpu, rc;
> 
>+	ops = net_dm_alert_ops_arr[net_dm_alert_mode];
>+
> 	if (!try_module_get(THIS_MODULE)) {
> 		NL_SET_ERR_MSG_MOD(extack, "Failed to take reference on module");
> 		return -ENODEV;
>@@ -267,17 +468,17 @@ static int net_dm_trace_on_set(struct netlink_ext_ack *extack)
> 	for_each_possible_cpu(cpu) {
> 		struct per_cpu_dm_data *data = &per_cpu(dm_cpu_data, cpu);
> 
>-		INIT_WORK(&data->dm_alert_work, send_dm_alert);
>+		INIT_WORK(&data->dm_alert_work, ops->work_item_func);
> 		timer_setup(&data->send_timer, sched_send_work, 0);
> 	}
> 
>-	rc = register_trace_kfree_skb(trace_kfree_skb_hit, NULL);
>+	rc = register_trace_kfree_skb(ops->kfree_skb_probe, NULL);
> 	if (rc) {
> 		NL_SET_ERR_MSG_MOD(extack, "Failed to connect probe to kfree_skb() tracepoint");
> 		goto err_module_put;
> 	}
> 
>-	rc = register_trace_napi_poll(trace_napi_poll_hit, NULL);
>+	rc = register_trace_napi_poll(ops->napi_poll_probe, NULL);
> 	if (rc) {
> 		NL_SET_ERR_MSG_MOD(extack, "Failed to connect probe to napi_poll() tracepoint");
> 		goto err_unregister_trace;
>@@ -286,7 +487,7 @@ static int net_dm_trace_on_set(struct netlink_ext_ack *extack)
> 	return 0;
> 
> err_unregister_trace:
>-	unregister_trace_kfree_skb(trace_kfree_skb_hit, NULL);
>+	unregister_trace_kfree_skb(ops->kfree_skb_probe, NULL);
> err_module_put:
> 	module_put(THIS_MODULE);
> 	return rc;
>@@ -295,10 +496,13 @@ static int net_dm_trace_on_set(struct netlink_ext_ack *extack)
> static void net_dm_trace_off_set(void)
> {
> 	struct dm_hw_stat_delta *new_stat, *temp;
>+	const struct net_dm_alert_ops *ops;
> 	int cpu;
> 
>-	unregister_trace_napi_poll(trace_napi_poll_hit, NULL);
>-	unregister_trace_kfree_skb(trace_kfree_skb_hit, NULL);
>+	ops = net_dm_alert_ops_arr[net_dm_alert_mode];
>+
>+	unregister_trace_napi_poll(ops->napi_poll_probe, NULL);
>+	unregister_trace_kfree_skb(ops->kfree_skb_probe, NULL);
> 
> 	tracepoint_synchronize_unregister();
> 
>@@ -307,9 +511,18 @@ static void net_dm_trace_off_set(void)
> 	 */
> 	for_each_possible_cpu(cpu) {
> 		struct per_cpu_dm_data *data = &per_cpu(dm_cpu_data, cpu);
>+		struct sk_buff *skb;
> 
> 		del_timer_sync(&data->send_timer);
> 		cancel_work_sync(&data->dm_alert_work);
>+		/* If we deactivated a pending timer, some packets are still
>+		 * queued and we need to free them.
>+		 */
>+		while ((skb = __skb_dequeue(&data->drop_queue))) {
>+			if (skb->dev)
>+				dev_put(skb->dev);
>+			consume_skb(skb);
>+		}
> 	}
> 
> 	list_for_each_entry_safe(new_stat, temp, &hw_stats_list, list) {
>@@ -351,12 +564,61 @@ static int set_all_monitor_traces(int state, struct netlink_ext_ack *extack)
> 	return rc;
> }
> 
>+static int net_dm_alert_mode_get_from_info(struct genl_info *info,
>+					   enum net_dm_alert_mode *p_alert_mode)
>+{
>+	u8 val;
>+
>+	val = nla_get_u8(info->attrs[NET_DM_ATTR_ALERT_MODE]);
>+
>+	switch (val) {
>+	case NET_DM_ALERT_MODE_SUMMARY: /* fall-through */
>+	case NET_DM_ALERT_MODE_PACKET:
>+		*p_alert_mode = val;
>+		break;
>+	default:
>+		return -EINVAL;
>+	}
>+
>+	return 0;
>+}
>+
>+static int net_dm_alert_mode_set(struct genl_info *info)
>+{
>+	struct netlink_ext_ack *extack = info->extack;
>+	enum net_dm_alert_mode alert_mode;
>+	int rc;
>+
>+	if (!info->attrs[NET_DM_ATTR_ALERT_MODE])
>+		return 0;
>+
>+	rc = net_dm_alert_mode_get_from_info(info, &alert_mode);
>+	if (rc) {
>+		NL_SET_ERR_MSG_MOD(extack, "Invalid alert mode");
>+		return -EINVAL;
>+	}
>+
>+	net_dm_alert_mode = alert_mode;

2 things:
1) Shouldn't you check if the tracing is on and return -EBUSY in case it is?
2) You setup the mode globally. I guess it is fine and it does not make
   sense to do it otherwise, right? Like per-net or something.


>+
>+	return 0;
>+}
>+
> static int net_dm_cmd_config(struct sk_buff *skb,
> 			struct genl_info *info)
> {
>-	NL_SET_ERR_MSG_MOD(info->extack, "Command not supported");
>+	struct netlink_ext_ack *extack = info->extack;
>+	int rc;
> 
>-	return -EOPNOTSUPP;
>+	if (trace_state == TRACE_ON) {
>+		NL_SET_ERR_MSG_MOD(extack, "Cannot configure drop monitor while tracing is on");
>+		return -EOPNOTSUPP;
>+	}
>+
>+	rc = net_dm_alert_mode_set(info);
>+	if (rc)
>+		return rc;
>+
>+	return 0;
> }
> 
> static int net_dm_cmd_trace(struct sk_buff *skb,
>@@ -411,6 +673,11 @@ static int dropmon_net_event(struct notifier_block *ev_block,
> 	return NOTIFY_DONE;
> }
> 
>+static const struct nla_policy net_dm_nl_policy[NET_DM_ATTR_MAX + 1] = {
>+	[NET_DM_ATTR_UNSPEC] = { .strict_start_type = NET_DM_ATTR_UNSPEC + 1 },
>+	[NET_DM_ATTR_ALERT_MODE] = { .type = NLA_U8 },
>+};
>+
> static const struct genl_ops dropmon_ops[] = {
> 	{
> 		.cmd = NET_DM_CMD_CONFIG,
>@@ -434,6 +701,8 @@ static struct genl_family net_drop_monitor_family __ro_after_init = {
> 	.hdrsize        = 0,
> 	.name           = "NET_DM",
> 	.version        = 2,
>+	.maxattr	= NET_DM_ATTR_MAX,
>+	.policy		= net_dm_nl_policy,
> 	.pre_doit	= net_dm_nl_pre_doit,
> 	.post_doit	= net_dm_nl_post_doit,
> 	.module		= THIS_MODULE,
>@@ -479,6 +748,7 @@ static int __init init_net_drop_monitor(void)
> 		INIT_WORK(&data->dm_alert_work, send_dm_alert);
> 		timer_setup(&data->send_timer, sched_send_work, 0);
> 		spin_lock_init(&data->lock);
>+		skb_queue_head_init(&data->drop_queue);
> 		reset_per_cpu_data(data);
> 	}
> 
>@@ -509,6 +779,7 @@ static void exit_net_drop_monitor(void)
> 		 * to this struct and can free the skb inside it
> 		 */
> 		kfree_skb(data->skb);
>+		WARN_ON(!skb_queue_empty(&data->drop_queue));
> 	}
> 
> 	BUG_ON(genl_unregister_family(&net_drop_monitor_family));
>-- 
>2.21.0
>

^ permalink raw reply

* Re: [PATCH] net: 9p: Fix possible null-pointer dereferences in p9_cm_event_handler()
From: Dominique Martinet @ 2019-07-24 12:55 UTC (permalink / raw)
  To: Jia-Ju Bai; +Cc: ericvh, lucho, davem, v9fs-developer, netdev, linux-kernel
In-Reply-To: <20190724103948.5834-1-baijiaju1990@gmail.com>

Jia-Ju Bai wrote on Wed, Jul 24, 2019:
> In p9_cm_event_handler(), there is an if statement on 260 to check
> whether rdma is NULL, which indicates that rdma can be NULL.
> If so, using rdma->xxx may cause a possible null-pointer dereference.

The final dereference (complete(&rdma->cm_done) line 285) has been here
from the start, so we would have seen crashes by now if rdma could be
null at this point.

Let's do it the other way around and remove the useless "if (rdma)" that
has been here from day 1 instead ; I basically did the same with
c->status a few months ago (from a coverity report)...


That said, please note that 'rdma checked for null in this event->event
== RDMA_CM_EVENT_DISCONNECTED branch' does not mean rdma can be null in
other branches.
static analysis does not say anything more than the final complete()
should also have a check if the previous one has, but nothing about the
other cases in the switch.


Thanks,
-- 
Dominique

^ permalink raw reply

* RE: [PATCH net-next 1/3] enetc: Add mdio bus driver for the PCIe MDIO endpoint
From: Claudiu Manoil @ 2019-07-24 12:57 UTC (permalink / raw)
  To: Claudiu Manoil, Andrew Lunn
  Cc: David S . Miller, devicetree@vger.kernel.org,
	netdev@vger.kernel.org, Alexandru Marginean,
	linux-kernel@vger.kernel.org, Leo Li, Rob Herring,
	linux-arm-kernel@lists.infradead.org
In-Reply-To: <VI1PR04MB4880DAE881769F6DC845CEEF96C60@VI1PR04MB4880.eurprd04.prod.outlook.com>



>-----Original Message-----
>From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org> On
>Behalf Of Claudiu Manoil
>Sent: Wednesday, July 24, 2019 12:53 PM
>To: Andrew Lunn <andrew@lunn.ch>
>Cc: David S . Miller <davem@davemloft.net>; devicetree@vger.kernel.org;
>netdev@vger.kernel.org; Alexandru Marginean
><alexandru.marginean@nxp.com>; linux-kernel@vger.kernel.org; Leo Li
><leoyang.li@nxp.com>; Rob Herring <robh+dt@kernel.org>; linux-arm-
>kernel@lists.infradead.org
>Subject: RE: [PATCH net-next 1/3] enetc: Add mdio bus driver for the PCIe MDIO
>endpoint
>
>>-----Original Message-----
>>From: Andrew Lunn <andrew@lunn.ch>
>>Sent: Wednesday, July 24, 2019 1:25 AM
>>To: Claudiu Manoil <claudiu.manoil@nxp.com>
>>Cc: David S . Miller <davem@davemloft.net>; devicetree@vger.kernel.org;
>>netdev@vger.kernel.org; Alexandru Marginean
>><alexandru.marginean@nxp.com>; linux-kernel@vger.kernel.org; Leo Li
>><leoyang.li@nxp.com>; Rob Herring <robh+dt@kernel.org>; linux-arm-
>>kernel@lists.infradead.org
>>Subject: Re: [PATCH net-next 1/3] enetc: Add mdio bus driver for the
>>PCIe MDIO endpoint
>>
>>> +	bus = mdiobus_alloc_size(sizeof(u32 *));
>>> +	if (!bus)
>>> +		return -ENOMEM;
>>> +
>>
>>> +	bus->priv = pci_iomap_range(pdev, 0, ENETC_MDIO_REG_OFFSET, 0);
>>
>>This got me confused for a while. You allocate space for a u32 pointer.
>>bus->priv will point to this space. However, you are not using this
>>space, you {ab}use the pointer to directly hold the return from
>>pci_iomap_range(). This works, but sparse is probably unhappy, and you
>>are wasting the space the u32 pointer takes.
>>
>
>Thanks Andrew,
>This is not what I wanted to do, don't ask me how I got to this, it's confusing
>indeed.
>What's needed here is mdiobus_alloc() or better, devm_mdiobus_alloc().
>I've got to do some cleanup in the local mdio bus probing too.
>Will send v2.
>

This is tricky actually, mdiobus_alloc won't do it, I still need to allocate space
for the pointer.
So it's not ok to store the register space pointer directly into priv
(even if iomap returns void *), it's unusual.
Looks like I will have to use double pointers!

^ permalink raw reply

* Re: [RFC PATCH net-next 00/12] drop_monitor: Capture dropped packets and metadata
From: Jiri Pirko @ 2019-07-24 12:58 UTC (permalink / raw)
  To: Ido Schimmel
  Cc: Toke Høiland-Jørgensen, netdev, davem, nhorman, dsahern,
	roopa, nikolay, jakub.kicinski, andy, f.fainelli, andrew,
	vivien.didelot, mlxsw, Ido Schimmel
In-Reply-To: <20190723151423.GA10342@splinter>

Tue, Jul 23, 2019 at 05:14:23PM CEST, idosch@idosch.org wrote:
>On Tue, Jul 23, 2019 at 02:17:49PM +0200, Toke Høiland-Jørgensen wrote:
>> Ido Schimmel <idosch@idosch.org> writes:
>> 
>> > On Mon, Jul 22, 2019 at 09:43:15PM +0200, Toke Høiland-Jørgensen wrote:
>> >> Is there a mechanism for the user to filter the packets before they are
>> >> sent to userspace? A bpf filter would be the obvious choice I guess...
>> >
>> > Hi Toke,
>> >
>> > Yes, it's on my TODO list to write an eBPF program that only lets
>> > "unique" packets to be enqueued on the netlink socket. Where "unique" is
>> > defined as {5-tuple, PC}. The rest of the copies will be counted in an
>> > eBPF map, which is just a hash table keyed by {5-tuple, PC}.
>> 
>> Yeah, that's a good idea. Or even something simpler like tcpdump-style
>> filters for the packets returned by drop monitor (say if I'm just trying
>> to figure out what happens to my HTTP requests).
>
>Yep, that's a good idea. I guess different users will use different
>programs. Will look into both options.
>
>> > I think it would be good to have the program as part of the bcc
>> > repository [1]. What do you think?
>> 
>> Sure. We could also add it to the XDP tutorial[2]; it could go into a
>> section on introspection and debugging (just added a TODO about that[3]).
>
>Great!
>
>> >> For integrating with XDP the trick would be to find a way to do it that
>> >> doesn't incur any overhead when it's not enabled. Are you envisioning
>> >> that this would be enabled separately for the different "modes" (kernel,
>> >> hardware, XDP, etc)?
>> >
>> > Yes. Drop monitor have commands to enable and disable tracing, but they
>> > don't carry any attributes at the moment. My plan is to add an attribute
>> > (e.g., 'NET_DM_ATTR_DROP_TYPE') that will specify the type of drops
>> > you're interested in - SW/HW/XDP. If the attribute is not specified,
>> > then current behavior is maintained and all the drop types are traced.
>> > But if you're only interested in SW drops, then overhead for the rest
>> > should be zero.
>> 
>> Makes sense (although "should be" is the key here ;)).
>> 
>> I'm also worried about the drop monitor getting overwhelmed; if you turn
>> it on for XDP and you're running a filtering program there, you'll
>> suddenly get *a lot* of drops.
>> 
>> As I read your patch, the current code can basically queue up an
>> unbounded number of packets waiting to go out over netlink, can't it?
>
>That's a very good point. Each CPU holds a drop list. It probably makes
>sense to limit it by default (to 1000?) and allow user to change it

Shouldn't the queue len be configurable?


>later, if needed. I can expose a counter that shows how many packets
>were dropped because of this limit. It can be used as an indication to
>adjust the queue length (or flip to 'summary' mode).

^ permalink raw reply

* [PATCH] qlge: Fix build error without CONFIG_ETHERNET
From: YueHaibing @ 2019-07-24 13:01 UTC (permalink / raw)
  To: manishc, GR-Linux-NIC-Dev, gregkh, bpoirier
  Cc: linux-kernel, devel, netdev, YueHaibing

Now if CONFIG_ETHERNET is not set, QLGE driver
building fails:

drivers/staging/qlge/qlge_main.o: In function `qlge_remove':
drivers/staging/qlge/qlge_main.c:4831: undefined reference to `unregister_netdev'

Reported-by: Hulk Robot <hulkci@huawei.com>
Fixes: 955315b0dc8c ("qlge: Move drivers/net/ethernet/qlogic/qlge/ to drivers/staging/qlge/")
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
 drivers/staging/qlge/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/qlge/Kconfig b/drivers/staging/qlge/Kconfig
index ae9ed2c..a3cb25a3 100644
--- a/drivers/staging/qlge/Kconfig
+++ b/drivers/staging/qlge/Kconfig
@@ -2,7 +2,7 @@
 
 config QLGE
 	tristate "QLogic QLGE 10Gb Ethernet Driver Support"
-	depends on PCI
+	depends on ETHERNET && PCI
 	help
 	This driver supports QLogic ISP8XXX 10Gb Ethernet cards.
 
-- 
2.7.4



^ permalink raw reply related

* pull-request: can 2019-07-24
From: Marc Kleine-Budde @ 2019-07-24 13:03 UTC (permalink / raw)
  To: netdev; +Cc: davem, linux-can, kernel

Hello David,

this is a pull reqeust of 7 patches for net/master.

The first patch is by Rasmus Villemoes add a missing netif_carrier_off() to
register_candev() so that generic netdev trigger based LEDs are initially off.

Nikita Yushchenko's patch for the rcar_canfd driver fixes a possible IRQ storm
on high load.

The patch by Weitao Hou for the mcp251x driver add missing error checking to
the work queue allocation.

Both Wen Yang's and Joakim Zhang's patch for the flexcan driver fix a problem
with the stop-mode.

Stephane Grosjean contributes a patch for the peak_usb driver to fix a
potential double kfree_skb().

The last patch is by YueHaibing and fixes the error path in can-gw's
cgw_module_init() function.

regards,
Marc

---

The following changes since commit d86afb89305de205b0d2f20c2160adf039e9508d:

  net: thunderx: Use fwnode_get_mac_address() (2019-07-23 14:09:21 -0700)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can.git tags/linux-can-fixes-for-5.3-20190724

for you to fetch changes up to b7a14297f102b6e2ce6f16feffebbb9bde1e9b55:

  can: gw: Fix error path of cgw_module_init (2019-07-24 11:19:03 +0200)

----------------------------------------------------------------
linux-can-fixes-for-5.3-20190724

----------------------------------------------------------------
Joakim Zhang (1):
      can: flexcan: fix stop mode acknowledgment

Nikita Yushchenko (1):
      can: rcar_canfd: fix possible IRQ storm on high load

Rasmus Villemoes (1):
      can: dev: call netif_carrier_off() in register_candev()

Stephane Grosjean (1):
      can: peak_usb: fix potential double kfree_skb()

Weitao Hou (1):
      can: mcp251x: add error check when wq alloc failed

Wen Yang (1):
      can: flexcan: fix an use-after-free in flexcan_setup_stop_mode()

YueHaibing (1):
      can: gw: Fix error path of cgw_module_init

 drivers/net/can/dev.c                        |  2 ++
 drivers/net/can/flexcan.c                    | 39 ++++++++++++++++++----
 drivers/net/can/rcar/rcar_canfd.c            |  9 ++---
 drivers/net/can/spi/mcp251x.c                | 49 +++++++++++++---------------
 drivers/net/can/usb/peak_usb/pcan_usb_core.c |  8 ++---
 net/can/gw.c                                 | 48 ++++++++++++++++++---------
 6 files changed, 98 insertions(+), 57 deletions(-)



^ permalink raw reply

* [PATCH 1/7] can: dev: call netif_carrier_off() in register_candev()
From: Marc Kleine-Budde @ 2019-07-24 13:03 UTC (permalink / raw)
  To: netdev
  Cc: davem, linux-can, kernel, Rasmus Villemoes, Willem de Bruijn,
	Marc Kleine-Budde
In-Reply-To: <20190724130322.31702-1-mkl@pengutronix.de>

From: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

CONFIG_CAN_LEDS is deprecated. When trying to use the generic netdev
trigger as suggested, there's a small inconsistency with the link
property: The LED is on initially, stays on when the device is brought
up, and then turns off (as expected) when the device is brought down.

Make sure the LED always reflects the state of the CAN device.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
Acked-by: Willem de Bruijn <willemb@google.com>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/dev.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index b6b93a2d93a5..483d270664cc 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -1249,6 +1249,8 @@ int register_candev(struct net_device *dev)
 		return -EINVAL;
 
 	dev->rtnl_link_ops = &can_link_ops;
+	netif_carrier_off(dev);
+
 	return register_netdev(dev);
 }
 EXPORT_SYMBOL_GPL(register_candev);
-- 
2.20.1


^ permalink raw reply related

* [PATCH 3/7] can: mcp251x: add error check when wq alloc failed
From: Marc Kleine-Budde @ 2019-07-24 13:03 UTC (permalink / raw)
  To: netdev
  Cc: davem, linux-can, kernel, Weitao Hou, Willem de Bruijn,
	Sean Nyekjaer, Marc Kleine-Budde
In-Reply-To: <20190724130322.31702-1-mkl@pengutronix.de>

From: Weitao Hou <houweitaoo@gmail.com>

add error check when workqueue alloc failed, and remove redundant code
to make it clear.

Fixes: e0000163e30e ("can: Driver for the Microchip MCP251x SPI CAN controllers")
Signed-off-by: Weitao Hou <houweitaoo@gmail.com>
Acked-by: Willem de Bruijn <willemb@google.com>
Tested-by: Sean Nyekjaer <sean@geanix.com>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/spi/mcp251x.c | 49 ++++++++++++++++-------------------
 1 file changed, 22 insertions(+), 27 deletions(-)

diff --git a/drivers/net/can/spi/mcp251x.c b/drivers/net/can/spi/mcp251x.c
index 44e99e3d7134..2aec934fab0c 100644
--- a/drivers/net/can/spi/mcp251x.c
+++ b/drivers/net/can/spi/mcp251x.c
@@ -664,17 +664,6 @@ static int mcp251x_power_enable(struct regulator *reg, int enable)
 		return regulator_disable(reg);
 }
 
-static void mcp251x_open_clean(struct net_device *net)
-{
-	struct mcp251x_priv *priv = netdev_priv(net);
-	struct spi_device *spi = priv->spi;
-
-	free_irq(spi->irq, priv);
-	mcp251x_hw_sleep(spi);
-	mcp251x_power_enable(priv->transceiver, 0);
-	close_candev(net);
-}
-
 static int mcp251x_stop(struct net_device *net)
 {
 	struct mcp251x_priv *priv = netdev_priv(net);
@@ -940,37 +929,43 @@ static int mcp251x_open(struct net_device *net)
 				   flags | IRQF_ONESHOT, DEVICE_NAME, priv);
 	if (ret) {
 		dev_err(&spi->dev, "failed to acquire irq %d\n", spi->irq);
-		mcp251x_power_enable(priv->transceiver, 0);
-		close_candev(net);
-		goto open_unlock;
+		goto out_close;
 	}
 
 	priv->wq = alloc_workqueue("mcp251x_wq", WQ_FREEZABLE | WQ_MEM_RECLAIM,
 				   0);
+	if (!priv->wq) {
+		ret = -ENOMEM;
+		goto out_clean;
+	}
 	INIT_WORK(&priv->tx_work, mcp251x_tx_work_handler);
 	INIT_WORK(&priv->restart_work, mcp251x_restart_work_handler);
 
 	ret = mcp251x_hw_reset(spi);
-	if (ret) {
-		mcp251x_open_clean(net);
-		goto open_unlock;
-	}
+	if (ret)
+		goto out_free_wq;
 	ret = mcp251x_setup(net, spi);
-	if (ret) {
-		mcp251x_open_clean(net);
-		goto open_unlock;
-	}
+	if (ret)
+		goto out_free_wq;
 	ret = mcp251x_set_normal_mode(spi);
-	if (ret) {
-		mcp251x_open_clean(net);
-		goto open_unlock;
-	}
+	if (ret)
+		goto out_free_wq;
 
 	can_led_event(net, CAN_LED_EVENT_OPEN);
 
 	netif_wake_queue(net);
+	mutex_unlock(&priv->mcp_lock);
 
-open_unlock:
+	return 0;
+
+out_free_wq:
+	destroy_workqueue(priv->wq);
+out_clean:
+	free_irq(spi->irq, priv);
+	mcp251x_hw_sleep(spi);
+out_close:
+	mcp251x_power_enable(priv->transceiver, 0);
+	close_candev(net);
 	mutex_unlock(&priv->mcp_lock);
 	return ret;
 }
-- 
2.20.1


^ permalink raw reply related

* [PATCH 2/7] can: rcar_canfd: fix possible IRQ storm on high load
From: Marc Kleine-Budde @ 2019-07-24 13:03 UTC (permalink / raw)
  To: netdev
  Cc: davem, linux-can, kernel, Nikita Yushchenko, linux-stable,
	Marc Kleine-Budde
In-Reply-To: <20190724130322.31702-1-mkl@pengutronix.de>

From: Nikita Yushchenko <nikita.yoush@cogentembedded.com>

We have observed rcar_canfd driver entering IRQ storm under high load,
with following scenario:
- rcar_canfd_global_interrupt() in entered due to Rx available,
- napi_schedule_prep() is called, and sets NAPIF_STATE_SCHED in state
- Rx fifo interrupts are masked,
- rcar_canfd_global_interrupt() is entered again, this time due to
  error interrupt (e.g. due to overflow),
- since scheduled napi poller has not yet executed, condition for calling
  napi_schedule_prep() from rcar_canfd_global_interrupt() remains true,
  thus napi_schedule_prep() gets called and sets NAPIF_STATE_MISSED flag
  in state,
- later, napi poller function rcar_canfd_rx_poll() gets executed, and
  calls napi_complete_done(),
- due to NAPIF_STATE_MISSED flag in state, this call does not clear
  NAPIF_STATE_SCHED flag from state,
- on return from napi_complete_done(), rcar_canfd_rx_poll() unmasks Rx
  interrutps,
- Rx interrupt happens, rcar_canfd_global_interrupt() gets called
  and calls napi_schedule_prep(),
- since NAPIF_STATE_SCHED is set in state at this time, this call
  returns false,
- due to that false return, rcar_canfd_global_interrupt() returns
  without masking Rx interrupt
- and this results into IRQ storm: unmasked Rx interrupt happens again
  and again is misprocessed in the same way.

This patch fixes that scenario by unmasking Rx interrupts only when
napi_complete_done() returns true, which means it has cleared
NAPIF_STATE_SCHED in state.

Fixes: dd3bd23eb438 ("can: rcar_canfd: Add Renesas R-Car CAN FD driver")
Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
Cc: linux-stable <stable@vger.kernel.org>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/rcar/rcar_canfd.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
index 05410008aa6b..de34a4b82d4a 100644
--- a/drivers/net/can/rcar/rcar_canfd.c
+++ b/drivers/net/can/rcar/rcar_canfd.c
@@ -1508,10 +1508,11 @@ static int rcar_canfd_rx_poll(struct napi_struct *napi, int quota)
 
 	/* All packets processed */
 	if (num_pkts < quota) {
-		napi_complete_done(napi, num_pkts);
-		/* Enable Rx FIFO interrupts */
-		rcar_canfd_set_bit(priv->base, RCANFD_RFCC(ridx),
-				   RCANFD_RFCC_RFIE);
+		if (napi_complete_done(napi, num_pkts)) {
+			/* Enable Rx FIFO interrupts */
+			rcar_canfd_set_bit(priv->base, RCANFD_RFCC(ridx),
+					   RCANFD_RFCC_RFIE);
+		}
 	}
 	return num_pkts;
 }
-- 
2.20.1


^ permalink raw reply related

* [PATCH 4/7] can: flexcan: fix an use-after-free in flexcan_setup_stop_mode()
From: Marc Kleine-Budde @ 2019-07-24 13:03 UTC (permalink / raw)
  To: netdev; +Cc: davem, linux-can, kernel, Wen Yang, linux-stable,
	Marc Kleine-Budde
In-Reply-To: <20190724130322.31702-1-mkl@pengutronix.de>

From: Wen Yang <wen.yang99@zte.com.cn>

The gpr_np variable is still being used in dev_dbg() after the
of_node_put() call, which may result in use-after-free.

Fixes: de3578c198c6 ("can: flexcan: add self wakeup support")
Signed-off-by: Wen Yang <wen.yang99@zte.com.cn>
Cc: linux-stable <stable@vger.kernel.org> # >= v5.0
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/flexcan.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index f2fe344593d5..33ce45d51e15 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -1437,10 +1437,10 @@ static int flexcan_setup_stop_mode(struct platform_device *pdev)
 
 	priv = netdev_priv(dev);
 	priv->stm.gpr = syscon_node_to_regmap(gpr_np);
-	of_node_put(gpr_np);
 	if (IS_ERR(priv->stm.gpr)) {
 		dev_dbg(&pdev->dev, "could not find gpr regmap\n");
-		return PTR_ERR(priv->stm.gpr);
+		ret = PTR_ERR(priv->stm.gpr);
+		goto out_put_node;
 	}
 
 	priv->stm.req_gpr = out_val[1];
@@ -1455,7 +1455,9 @@ static int flexcan_setup_stop_mode(struct platform_device *pdev)
 
 	device_set_wakeup_capable(&pdev->dev, true);
 
-	return 0;
+out_put_node:
+	of_node_put(gpr_np);
+	return ret;
 }
 
 static const struct of_device_id flexcan_of_match[] = {
-- 
2.20.1


^ permalink raw reply related

* [PATCH 6/7] can: peak_usb: fix potential double kfree_skb()
From: Marc Kleine-Budde @ 2019-07-24 13:03 UTC (permalink / raw)
  To: netdev
  Cc: davem, linux-can, kernel, Stephane Grosjean, linux-stable,
	Marc Kleine-Budde
In-Reply-To: <20190724130322.31702-1-mkl@pengutronix.de>

From: Stephane Grosjean <s.grosjean@peak-system.com>

When closing the CAN device while tx skbs are inflight, echo skb could
be released twice. By calling close_candev() before unlinking all
pending tx urbs, then the internal echo_skb[] array is fully and
correctly cleared before the USB write callback and, therefore,
can_get_echo_skb() are called, for each aborted URB.

Fixes: bb4785551f64 ("can: usb: PEAK-System Technik USB adapters driver core")
Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>
Cc: linux-stable <stable@vger.kernel.org>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/peak_usb/pcan_usb_core.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.c b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
index 458154c9b482..22b9c8e6d040 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
@@ -568,16 +568,16 @@ static int peak_usb_ndo_stop(struct net_device *netdev)
 	dev->state &= ~PCAN_USB_STATE_STARTED;
 	netif_stop_queue(netdev);
 
+	close_candev(netdev);
+
+	dev->can.state = CAN_STATE_STOPPED;
+
 	/* unlink all pending urbs and free used memory */
 	peak_usb_unlink_all_urbs(dev);
 
 	if (dev->adapter->dev_stop)
 		dev->adapter->dev_stop(dev);
 
-	close_candev(netdev);
-
-	dev->can.state = CAN_STATE_STOPPED;
-
 	/* can set bus off now */
 	if (dev->adapter->dev_set_bus) {
 		int err = dev->adapter->dev_set_bus(dev, 0);
-- 
2.20.1


^ permalink raw reply related

* [PATCH 5/7] can: flexcan: fix stop mode acknowledgment
From: Marc Kleine-Budde @ 2019-07-24 13:03 UTC (permalink / raw)
  To: netdev
  Cc: davem, linux-can, kernel, Joakim Zhang, Marc Kleine-Budde,
	linux-stable
In-Reply-To: <20190724130322.31702-1-mkl@pengutronix.de>

From: Joakim Zhang <qiangqing.zhang@nxp.com>

To enter stop mode, the CPU should manually assert a global Stop Mode
request and check the acknowledgment asserted by FlexCAN. The CPU must
only consider the FlexCAN in stop mode when both request and
acknowledgment conditions are satisfied.

Fixes: de3578c198c6 ("can: flexcan: add self wakeup support")
Reported-by: Marc Kleine-Budde <mkl@pengutronix.de>
Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
Cc: linux-stable <stable@vger.kernel.org> # >= v5.0
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/flexcan.c | 31 +++++++++++++++++++++++++++----
 1 file changed, 27 insertions(+), 4 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 33ce45d51e15..fcec8bcb53d6 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -400,9 +400,10 @@ static void flexcan_enable_wakeup_irq(struct flexcan_priv *priv, bool enable)
 	priv->write(reg_mcr, &regs->mcr);
 }
 
-static inline void flexcan_enter_stop_mode(struct flexcan_priv *priv)
+static inline int flexcan_enter_stop_mode(struct flexcan_priv *priv)
 {
 	struct flexcan_regs __iomem *regs = priv->regs;
+	unsigned int ackval;
 	u32 reg_mcr;
 
 	reg_mcr = priv->read(&regs->mcr);
@@ -412,20 +413,37 @@ static inline void flexcan_enter_stop_mode(struct flexcan_priv *priv)
 	/* enable stop request */
 	regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr,
 			   1 << priv->stm.req_bit, 1 << priv->stm.req_bit);
+
+	/* get stop acknowledgment */
+	if (regmap_read_poll_timeout(priv->stm.gpr, priv->stm.ack_gpr,
+				     ackval, ackval & (1 << priv->stm.ack_bit),
+				     0, FLEXCAN_TIMEOUT_US))
+		return -ETIMEDOUT;
+
+	return 0;
 }
 
-static inline void flexcan_exit_stop_mode(struct flexcan_priv *priv)
+static inline int flexcan_exit_stop_mode(struct flexcan_priv *priv)
 {
 	struct flexcan_regs __iomem *regs = priv->regs;
+	unsigned int ackval;
 	u32 reg_mcr;
 
 	/* remove stop request */
 	regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr,
 			   1 << priv->stm.req_bit, 0);
 
+	/* get stop acknowledgment */
+	if (regmap_read_poll_timeout(priv->stm.gpr, priv->stm.ack_gpr,
+				     ackval, !(ackval & (1 << priv->stm.ack_bit)),
+				     0, FLEXCAN_TIMEOUT_US))
+		return -ETIMEDOUT;
+
 	reg_mcr = priv->read(&regs->mcr);
 	reg_mcr &= ~FLEXCAN_MCR_SLF_WAK;
 	priv->write(reg_mcr, &regs->mcr);
+
+	return 0;
 }
 
 static inline void flexcan_error_irq_enable(const struct flexcan_priv *priv)
@@ -1614,7 +1632,9 @@ static int __maybe_unused flexcan_suspend(struct device *device)
 		 */
 		if (device_may_wakeup(device)) {
 			enable_irq_wake(dev->irq);
-			flexcan_enter_stop_mode(priv);
+			err = flexcan_enter_stop_mode(priv);
+			if (err)
+				return err;
 		} else {
 			err = flexcan_chip_disable(priv);
 			if (err)
@@ -1664,10 +1684,13 @@ static int __maybe_unused flexcan_noirq_resume(struct device *device)
 {
 	struct net_device *dev = dev_get_drvdata(device);
 	struct flexcan_priv *priv = netdev_priv(dev);
+	int err;
 
 	if (netif_running(dev) && device_may_wakeup(device)) {
 		flexcan_enable_wakeup_irq(priv, false);
-		flexcan_exit_stop_mode(priv);
+		err = flexcan_exit_stop_mode(priv);
+		if (err)
+			return err;
 	}
 
 	return 0;
-- 
2.20.1


^ permalink raw reply related

* [PATCH 7/7] can: gw: Fix error path of cgw_module_init
From: Marc Kleine-Budde @ 2019-07-24 13:03 UTC (permalink / raw)
  To: netdev
  Cc: davem, linux-can, kernel, YueHaibing, Oliver Hartkopp,
	Marc Kleine-Budde
In-Reply-To: <20190724130322.31702-1-mkl@pengutronix.de>

From: YueHaibing <yuehaibing@huawei.com>

This patch add error path for cgw_module_init to avoid possible crash if
some error occurs.

Fixes: c1aabdf379bc ("can-gw: add netlink based CAN routing")
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 net/can/gw.c | 48 +++++++++++++++++++++++++++++++++---------------
 1 file changed, 33 insertions(+), 15 deletions(-)

diff --git a/net/can/gw.c b/net/can/gw.c
index 5275ddf580bc..72711053ebe6 100644
--- a/net/can/gw.c
+++ b/net/can/gw.c
@@ -1046,32 +1046,50 @@ static __init int cgw_module_init(void)
 	pr_info("can: netlink gateway (rev " CAN_GW_VERSION ") max_hops=%d\n",
 		max_hops);
 
-	register_pernet_subsys(&cangw_pernet_ops);
+	ret = register_pernet_subsys(&cangw_pernet_ops);
+	if (ret)
+		return ret;
+
+	ret = -ENOMEM;
 	cgw_cache = kmem_cache_create("can_gw", sizeof(struct cgw_job),
 				      0, 0, NULL);
-
 	if (!cgw_cache)
-		return -ENOMEM;
+		goto out_cache_create;
 
 	/* set notifier */
 	notifier.notifier_call = cgw_notifier;
-	register_netdevice_notifier(&notifier);
+	ret = register_netdevice_notifier(&notifier);
+	if (ret)
+		goto out_register_notifier;
 
 	ret = rtnl_register_module(THIS_MODULE, PF_CAN, RTM_GETROUTE,
 				   NULL, cgw_dump_jobs, 0);
-	if (ret) {
-		unregister_netdevice_notifier(&notifier);
-		kmem_cache_destroy(cgw_cache);
-		return -ENOBUFS;
-	}
-
-	/* Only the first call to rtnl_register_module can fail */
-	rtnl_register_module(THIS_MODULE, PF_CAN, RTM_NEWROUTE,
-			     cgw_create_job, NULL, 0);
-	rtnl_register_module(THIS_MODULE, PF_CAN, RTM_DELROUTE,
-			     cgw_remove_job, NULL, 0);
+	if (ret)
+		goto out_rtnl_register1;
+
+	ret = rtnl_register_module(THIS_MODULE, PF_CAN, RTM_NEWROUTE,
+				   cgw_create_job, NULL, 0);
+	if (ret)
+		goto out_rtnl_register2;
+	ret = rtnl_register_module(THIS_MODULE, PF_CAN, RTM_DELROUTE,
+				   cgw_remove_job, NULL, 0);
+	if (ret)
+		goto out_rtnl_register3;
 
 	return 0;
+
+out_rtnl_register3:
+	rtnl_unregister(PF_CAN, RTM_NEWROUTE);
+out_rtnl_register2:
+	rtnl_unregister(PF_CAN, RTM_GETROUTE);
+out_rtnl_register1:
+	unregister_netdevice_notifier(&notifier);
+out_register_notifier:
+	kmem_cache_destroy(cgw_cache);
+out_cache_create:
+	unregister_pernet_subsys(&cangw_pernet_ops);
+
+	return ret;
 }
 
 static __exit void cgw_module_exit(void)
-- 
2.20.1


^ permalink raw reply related

* Re: [Intel-wired-lan] [PATCH v2 00/10] XDP unaligned chunk placement support
From: Magnus Karlsson @ 2019-07-24 13:25 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Kevin Laatz, Jakub Kicinski, Daniel Borkmann, Network Development,
	ciara.loftus, Alexei Starovoitov, intel-wired-lan, Jonathan Lemon,
	bruce.richardson, bpf, Björn Töpel, Karlsson, Magnus
In-Reply-To: <CAADnVQLEdJwT7DRCpp2zuKWTg0uj=WKQkFc2LZ4o+1fDgnEFLg@mail.gmail.com>

On Tue, Jul 23, 2019 at 11:08 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> Johnathan, Bjorn, Jakub,
> Please review!
> The patch set has been pending for a week.

There is a v3 coming out shortly so I suggest we wait for that. It
will have Mellanox support for this feature too and some clean ups. I
refrained from posting a review on the mailing list due to the merge
window being closed last week, but maybe that was not correct. Should
I still post reviews for new features submitted during the closed
merge window period? I am happy to do it since I do not have any other
tasks during that time. It is a quite period for me. Just let me know.

/Magnus

> On Tue, Jul 16, 2019 at 4:21 AM Kevin Laatz <kevin.laatz@intel.com> wrote:
> >
> > This patch set adds the ability to use unaligned chunks in the XDP umem.
> >
> > Currently, all chunk addresses passed to the umem are masked to be chunk
> > size aligned (default is 2k, max is PAGE_SIZE). This limits where we can
> > place chunks within the umem as well as limiting the packet sizes that are
> > supported.
> >
> > The changes in this patch set removes these restrictions, allowing XDP to
> > be more flexible in where it can place a chunk within a umem. By relaxing
> > where the chunks can be placed, it allows us to use an arbitrary buffer
> > size and place that wherever we have a free address in the umem. These
> > changes add the ability to support arbitrary frame sizes up to 4k
> > (PAGE_SIZE) and make it easy to integrate with other existing frameworks
> > that have their own memory management systems, such as DPDK.
> >
> > Since we are now dealing with arbitrary frame sizes, we need also need to
> > update how we pass around addresses. Currently, the addresses can simply be
> > masked to 2k to get back to the original address. This becomes less trivial
> > when using frame sizes that are not a 'power of 2' size. This patch set
> > modifies the Rx/Tx descriptor format to use the upper 16-bits of the addr
> > field for an offset value, leaving the lower 48-bits for the address (this
> > leaves us with 256 Terabytes, which should be enough!). We only need to use
> > the upper 16-bits to store the offset when running in unaligned mode.
> > Rather than adding the offset (headroom etc) to the address, we will store
> > it in the upper 16-bits of the address field. This way, we can easily add
> > the offset to the address where we need it, using some bit manipulation and
> > addition, and we can also easily get the original address wherever we need
> > it (for example in i40e_zca_free) by simply masking to get the lower
> > 48-bits of the address field.
> >
> > The numbers below were recorded with the following set up:
> >   - Intel(R) Xeon(R) Gold 6140 CPU @ 2.30GHz
> >   - Intel Corporation Ethernet Controller XXV710 for 25GbE SFP28 (rev 02)
> >   - Driver: i40e
> >   - Application: xdpsock with l2fwd (single interface)
> >
> > These are solely for comparing performance with and without the patches.
> > The largest drop was ~1% (in zero-copy mode).
> >
> > +-------------------------+------------+-----------------+-------------+
> > | Buffer size: 2048       | SKB mode   | Zero-copy       | Copy        |
> > +-------------------------+------------+-----------------+-------------+
> > | Aligned (baseline)      | 1.7 Mpps   | 15.3 Mpps       | 2.08 Mpps   |
> > +-------------------------+------------+-----------------+-------------+
> > | Aligned (with patches)  | 1.7 Mpps   | 15.1 Mpps       | 2.08 Mpps   |
> > +-------------------------+------------+-----------------+-------------+
> > | Unaligned               | 1.7 Mpps   | 14.5 Mpps       | 2.08 Mpps   |
> > +-------------------------+------------+-----------------+-------------+
> >
> > NOTE: We are currently working on the changes required in the Mellanox
> > driver. We will include these in the v3.
> >
> > Structure of the patchset:
> > Patch 1:
> >   - Remove unnecessary masking and headroom addition during zero-copy Rx
> >     buffer recycling in i40e. This change is required in order for the
> >     buffer recycling to work in the unaligned chunk mode.
> >
> > Patch 2:
> >   - Remove unnecessary masking and headroom addition during
> >     zero-copy Rx buffer recycling in ixgbe. This change is required in
> >     order for the  buffer recycling to work in the unaligned chunk mode.
> >
> > Patch 3:
> >   - Add infrastructure for unaligned chunks. Since we are dealing with
> >     unaligned chunks that could potentially cross a physical page boundary,
> >     we add checks to keep track of that information. We can later use this
> >     information to correctly handle buffers that are placed at an address
> >     where they cross a page boundary.  This patch also modifies the
> >     existing Rx and Tx functions to use the new descriptor format. To
> >     handle addresses correctly, we need to mask appropriately based on
> >     whether we are in aligned or unaligned mode.
> >
> > Patch 4:
> >   - This patch updates the i40e driver to make use of the new descriptor
> >     format. The new format is particularly useful here since we can now
> >     retrieve the original address in places like i40e_zca_free with ease.
> >     This saves us doing various calculations to get the original address
> >     back.
> >
> > Patch 5:
> >   - This patch updates the ixgbe driver to make use of the new descriptor
> >     format. The new format is particularly useful here since we can now
> >     retrieve the original address in places like ixgbe_zca_free with ease.
> >     This saves us doing various calculations to get the original address
> >     back.
> >
> > Patch 6:
> >   - Add flags for umem configuration to libbpf
> >
> > Patch 7:
> >   - Modify xdpsock application to add a command line option for
> >     unaligned chunks
> >
> > Patch 8:
> >   - Since we can now run the application in unaligned chunk mode, we need
> >     to make sure we recycle the buffers appropriately.
> >
> > Patch 9:
> >   - Adds hugepage support to the xdpsock application
> >
> > Patch 10:
> >   - Documentation update to include the unaligned chunk scenario. We need
> >     to explicitly state that the incoming addresses are only masked in the
> >     aligned chunk mode and not the unaligned chunk mode.
> >
> > ---
> > v2:
> >   - fixed checkpatch issues
> >   - fixed Rx buffer recycling for unaligned chunks in xdpsock
> >   - removed unused defines
> >   - fixed how chunk_size is calculated in xsk_diag.c
> >   - added some performance numbers to cover letter
> >   - modified descriptor format to make it easier to retrieve original
> >     address
> >   - removed patch adding off_t off to the zero copy allocator. This is no
> >     longer needed with the new descriptor format.
> >
> > Kevin Laatz (10):
> >   i40e: simplify Rx buffer recycle
> >   ixgbe: simplify Rx buffer recycle
> >   xsk: add support to allow unaligned chunk placement
> >   i40e: modify driver for handling offsets
> >   ixgbe: modify driver for handling offsets
> >   libbpf: add flags to umem config
> >   samples/bpf: add unaligned chunks mode support to xdpsock
> >   samples/bpf: add buffer recycling for unaligned chunks to xdpsock
> >   samples/bpf: use hugepages in xdpsock app
> >   doc/af_xdp: include unaligned chunk case
> >
> >  Documentation/networking/af_xdp.rst          | 10 ++-
> >  drivers/net/ethernet/intel/i40e/i40e_xsk.c   | 39 +++++----
> >  drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c | 39 +++++----
> >  include/net/xdp_sock.h                       |  2 +
> >  include/uapi/linux/if_xdp.h                  |  9 ++
> >  net/xdp/xdp_umem.c                           | 17 ++--
> >  net/xdp/xsk.c                                | 89 ++++++++++++++++----
> >  net/xdp/xsk_diag.c                           |  2 +-
> >  net/xdp/xsk_queue.h                          | 70 +++++++++++++--
> >  samples/bpf/xdpsock_user.c                   | 61 ++++++++++----
> >  tools/include/uapi/linux/if_xdp.h            |  4 +
> >  tools/lib/bpf/xsk.c                          |  3 +
> >  tools/lib/bpf/xsk.h                          |  2 +
> >  13 files changed, 266 insertions(+), 81 deletions(-)
> >
> > --
> > 2.17.1
> >
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

^ 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