* [PATCH net 0/2] net_sched: Fix two tc_index filter init issues
From: Hangbin Liu @ 2018-08-13 10:44 UTC (permalink / raw)
To: netdev; +Cc: David Miller, WANG Cong, Hangbin Liu
These two patches fix two tc_index filter init issues. The first one fixes
missing exts info in new filter, which will cause NULL pointer dereference
when delete tcindex filter. The second one fixes missing res info when create
new filter, which will make filter unbind failed.
Hangbin Liu (2):
net_sched: fix NULL pointer dereference when delete tcindex filter
net_sched: Fix missing res info when create new filter
net/sched/cls_tcindex.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
--
2.5.5
^ permalink raw reply
* [PATCH net 1/2] net_sched: fix NULL pointer dereference when delete tcindex filter
From: Hangbin Liu @ 2018-08-13 10:44 UTC (permalink / raw)
To: netdev; +Cc: David Miller, WANG Cong, Hangbin Liu
In-Reply-To: <1534157044-19753-1-git-send-email-liuhangbin@gmail.com>
Li Shuang reported the following crash:
[ 71.267724] BUG: unable to handle kernel NULL pointer dereference at 0000000000000004
[ 71.276456] PGD 800000085d9bd067 P4D 800000085d9bd067 PUD 859a0b067 PMD 0
[ 71.284127] Oops: 0000 [#1] SMP PTI
[ 71.288015] CPU: 12 PID: 2386 Comm: tc Not tainted 4.18.0-rc8.latest+ #131
[ 71.295686] Hardware name: Dell Inc. PowerEdge R730/0WCJNT, BIOS 2.1.5 04/11/2016
[ 71.304037] RIP: 0010:tcindex_delete+0x72/0x280 [cls_tcindex]
[ 71.310446] Code: 00 31 f6 48 87 75 20 48 85 f6 74 11 48 8b 47 18 48 8b 40 08 48 8b 40 50 e8 fb a6 f8 fc 48 85 db 0f 84 dc 00 00 00 48 8b 73 18 <8b> 56 04 48 8d 7e 04 85 d2 0f 84 7b 01 00
[ 71.331517] RSP: 0018:ffffb45207b3f898 EFLAGS: 00010282
[ 71.337345] RAX: ffff8ad3d72d6360 RBX: ffff8acc84393680 RCX: 000000000000002e
[ 71.345306] RDX: ffff8ad3d72c8570 RSI: 0000000000000000 RDI: ffff8ad847a45800
[ 71.353277] RBP: ffff8acc84393688 R08: ffff8ad3d72c8400 R09: 0000000000000000
[ 71.361238] R10: ffff8ad3de786e00 R11: 0000000000000000 R12: ffffb45207b3f8c7
[ 71.369199] R13: ffff8ad3d93bd2a0 R14: 000000000000002e R15: ffff8ad3d72c9600
[ 71.377161] FS: 00007f9d3ec3e740(0000) GS:ffff8ad3df980000(0000) knlGS:0000000000000000
[ 71.386188] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 71.392597] CR2: 0000000000000004 CR3: 0000000852f06003 CR4: 00000000001606e0
[ 71.400558] Call Trace:
[ 71.403299] tcindex_destroy_element+0x25/0x40 [cls_tcindex]
[ 71.409611] tcindex_walk+0xbb/0x110 [cls_tcindex]
[ 71.414953] tcindex_destroy+0x44/0x90 [cls_tcindex]
[ 71.420492] ? tcindex_delete+0x280/0x280 [cls_tcindex]
[ 71.426323] tcf_proto_destroy+0x16/0x40
[ 71.430696] tcf_chain_flush+0x51/0x70
[ 71.434876] tcf_block_put_ext.part.30+0x8f/0x1b0
[ 71.440122] tcf_block_put+0x4d/0x70
[ 71.444108] cbq_destroy+0x4d/0xd0 [sch_cbq]
[ 71.448869] qdisc_destroy+0x62/0x130
[ 71.452951] dsmark_destroy+0x2a/0x70 [sch_dsmark]
[ 71.458300] qdisc_destroy+0x62/0x130
[ 71.462373] qdisc_graft+0x3ba/0x470
[ 71.466359] tc_get_qdisc+0x2a6/0x2c0
[ 71.470443] ? cred_has_capability+0x7d/0x130
[ 71.475307] rtnetlink_rcv_msg+0x263/0x2d0
[ 71.479875] ? rtnl_calcit.isra.30+0x110/0x110
[ 71.484832] netlink_rcv_skb+0x4d/0x130
[ 71.489109] netlink_unicast+0x1a3/0x250
[ 71.493482] netlink_sendmsg+0x2ae/0x3a0
[ 71.497859] sock_sendmsg+0x36/0x40
[ 71.501748] ___sys_sendmsg+0x26f/0x2d0
[ 71.506029] ? handle_pte_fault+0x586/0xdf0
[ 71.510694] ? __handle_mm_fault+0x389/0x500
[ 71.515457] ? __sys_sendmsg+0x5e/0xa0
[ 71.519636] __sys_sendmsg+0x5e/0xa0
[ 71.523626] do_syscall_64+0x5b/0x180
[ 71.527711] entry_SYSCALL_64_after_hwframe+0x44/0xa9
[ 71.533345] RIP: 0033:0x7f9d3e257f10
[ 71.537331] Code: c3 48 8b 05 82 6f 2c 00 f7 db 64 89 18 48 83 cb ff eb dd 0f 1f 80 00 00 00 00 83 3d 8d d0 2c 00 00 75 10 b8 2e 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 31 c3 48 83 ec 08 e8
[ 71.558401] RSP: 002b:00007fff6f893398 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
[ 71.566848] RAX: ffffffffffffffda RBX: 000000005b71274d RCX: 00007f9d3e257f10
[ 71.574810] RDX: 0000000000000000 RSI: 00007fff6f8933e0 RDI: 0000000000000003
[ 71.582770] RBP: 00007fff6f8933e0 R08: 000000000000ffff R09: 0000000000000003
[ 71.590729] R10: 00007fff6f892e20 R11: 0000000000000246 R12: 0000000000000000
[ 71.598689] R13: 0000000000662ee0 R14: 0000000000000000 R15: 0000000000000000
[ 71.606651] Modules linked in: sch_cbq cls_tcindex sch_dsmark xt_CHECKSUM iptable_mangle ipt_MASQUERADE iptable_nat nf_nat_ipv4 nf_nat nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack nf_coni
[ 71.685425] libahci i2c_algo_bit i2c_core i40e libata dca mdio megaraid_sas dm_mirror dm_region_hash dm_log dm_mod
[ 71.697075] CR2: 0000000000000004
[ 71.700792] ---[ end trace f604eb1acacd978b ]---
Reproducer:
tc qdisc add dev lo handle 1:0 root dsmark indices 64 set_tc_index
tc filter add dev lo parent 1:0 protocol ip prio 1 tcindex mask 0xfc shift 2
tc qdisc add dev lo parent 1:0 handle 2:0 cbq bandwidth 10Mbit cell 8 avpkt 1000 mpu 64
tc class add dev lo parent 2:0 classid 2:1 cbq bandwidth 10Mbit rate 1500Kbit avpkt 1000 prio 1 bounded isolated allot 1514 weight 1 maxburst 10
tc filter add dev lo parent 2:0 protocol ip prio 1 handle 0x2e tcindex classid 2:1 pass_on
tc qdisc add dev lo parent 2:1 pfifo limit 5
tc qdisc del dev lo root
This is because in tcindex_set_parms, when there is no old_r, we set new
exts to cr.exts. And we didn't set it to filter when r == &new_filter_result.
Then in tcindex_delete() -> tcf_exts_get_net(), we will get NULL pointer
dereference as we didn't init exts.
Fix it by moving tcf_exts_change() after "if (old_r && old_r != r)" check.
Then we don't need "cr" as there is no errout after that.
Fixes: bf63ac73b3e13 ("net_sched: fix an oops in tcindex filter")
Reported-by: Li Shuang <shuali@redhat.com>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
net/sched/cls_tcindex.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/net/sched/cls_tcindex.c b/net/sched/cls_tcindex.c
index 32f4bbd..ddaa4e6 100644
--- a/net/sched/cls_tcindex.c
+++ b/net/sched/cls_tcindex.c
@@ -447,11 +447,6 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
tcf_bind_filter(tp, &cr.res, base);
}
- if (old_r)
- tcf_exts_change(&r->exts, &e);
- else
- tcf_exts_change(&cr.exts, &e);
-
if (old_r && old_r != r) {
err = tcindex_filter_result_init(old_r);
if (err < 0) {
@@ -462,6 +457,8 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
oldp = p;
r->res = cr.res;
+ tcf_exts_change(&r->exts, &e);
+
rcu_assign_pointer(tp->root, cp);
if (r == &new_filter_result) {
--
2.5.5
^ permalink raw reply related
* [PATCH net 2/2] net_sched: Fix missing res info when create new tc_index filter
From: Hangbin Liu @ 2018-08-13 10:44 UTC (permalink / raw)
To: netdev; +Cc: David Miller, WANG Cong, Hangbin Liu
In-Reply-To: <1534157044-19753-1-git-send-email-liuhangbin@gmail.com>
Li Shuang reported the following warn:
[ 733.484610] WARNING: CPU: 6 PID: 21123 at net/sched/sch_cbq.c:1418 cbq_destroy_class+0x5d/0x70 [sch_cbq]
[ 733.495190] Modules linked in: sch_cbq cls_tcindex sch_dsmark rpcsec_gss_krb5 auth_rpcgss nfsv4 dns_resolver nfs lockd grace fscache xt_CHECKSUM iptable_mangle ipt_MASQUERADE iptable_nat l
[ 733.574155] syscopyarea sysfillrect sysimgblt fb_sys_fops ttm drm igb ixgbe ahci libahci i2c_algo_bit libata i40e i2c_core dca mdio megaraid_sas dm_mirror dm_region_hash dm_log dm_mod
[ 733.592500] CPU: 6 PID: 21123 Comm: tc Not tainted 4.18.0-rc8.latest+ #131
[ 733.600169] Hardware name: Dell Inc. PowerEdge R730/0WCJNT, BIOS 2.1.5 04/11/2016
[ 733.608518] RIP: 0010:cbq_destroy_class+0x5d/0x70 [sch_cbq]
[ 733.614734] Code: e7 d9 d2 48 8b 7b 48 e8 61 05 da d2 48 8d bb f8 00 00 00 e8 75 ae d5 d2 48 39 eb 74 0a 48 89 df 5b 5d e9 16 6c 94 d2 5b 5d c3 <0f> 0b eb b6 0f 1f 44 00 00 66 2e 0f 1f 84
[ 733.635798] RSP: 0018:ffffbfbb066bb9d8 EFLAGS: 00010202
[ 733.641627] RAX: 0000000000000001 RBX: ffff9cdd17392800 RCX: 000000008010000f
[ 733.649588] RDX: ffff9cdd1df547e0 RSI: ffff9cdd17392800 RDI: ffff9cdd0f84c800
[ 733.657547] RBP: ffff9cdd0f84c800 R08: 0000000000000001 R09: 0000000000000000
[ 733.665508] R10: ffff9cdd0f84d000 R11: 0000000000000001 R12: 0000000000000001
[ 733.673469] R13: 0000000000000000 R14: 0000000000000001 R15: ffff9cdd17392200
[ 733.681430] FS: 00007f911890a740(0000) GS:ffff9cdd1f8c0000(0000) knlGS:0000000000000000
[ 733.690456] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 733.696864] CR2: 0000000000b5544c CR3: 0000000859374002 CR4: 00000000001606e0
[ 733.704826] Call Trace:
[ 733.707554] cbq_destroy+0xa1/0xd0 [sch_cbq]
[ 733.712318] qdisc_destroy+0x62/0x130
[ 733.716401] dsmark_destroy+0x2a/0x70 [sch_dsmark]
[ 733.721745] qdisc_destroy+0x62/0x130
[ 733.725829] qdisc_graft+0x3ba/0x470
[ 733.729817] tc_get_qdisc+0x2a6/0x2c0
[ 733.733901] ? cred_has_capability+0x7d/0x130
[ 733.738761] rtnetlink_rcv_msg+0x263/0x2d0
[ 733.743330] ? rtnl_calcit.isra.30+0x110/0x110
[ 733.748287] netlink_rcv_skb+0x4d/0x130
[ 733.752576] netlink_unicast+0x1a3/0x250
[ 733.756949] netlink_sendmsg+0x2ae/0x3a0
[ 733.761324] sock_sendmsg+0x36/0x40
[ 733.765213] ___sys_sendmsg+0x26f/0x2d0
[ 733.769493] ? handle_pte_fault+0x586/0xdf0
[ 733.774158] ? __handle_mm_fault+0x389/0x500
[ 733.778919] ? __sys_sendmsg+0x5e/0xa0
[ 733.783099] __sys_sendmsg+0x5e/0xa0
[ 733.787087] do_syscall_64+0x5b/0x180
[ 733.791171] entry_SYSCALL_64_after_hwframe+0x44/0xa9
[ 733.796805] RIP: 0033:0x7f9117f23f10
[ 733.800791] Code: c3 48 8b 05 82 6f 2c 00 f7 db 64 89 18 48 83 cb ff eb dd 0f 1f 80 00 00 00 00 83 3d 8d d0 2c 00 00 75 10 b8 2e 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 31 c3 48 83 ec 08 e8
[ 733.821873] RSP: 002b:00007ffe96818398 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
[ 733.830319] RAX: ffffffffffffffda RBX: 000000005b71244c RCX: 00007f9117f23f10
[ 733.838280] RDX: 0000000000000000 RSI: 00007ffe968183e0 RDI: 0000000000000003
[ 733.846241] RBP: 00007ffe968183e0 R08: 000000000000ffff R09: 0000000000000003
[ 733.854202] R10: 00007ffe96817e20 R11: 0000000000000246 R12: 0000000000000000
[ 733.862161] R13: 0000000000662ee0 R14: 0000000000000000 R15: 0000000000000000
[ 733.870121] ---[ end trace 28edd4aad712ddca ]---
This is because we didn't update f->result.res when create new filter. Then in
tcindex_delete() -> tcf_unbind_filter(), we will failed to find out the res
and unbind filter, which will trigger the WARN_ON() in cbq_destroy_class().
Fix it by updating f->result.res when create new filter.
Fixes: 6e0565697a106 ("net_sched: fix another crash in cls_tcindex")
Reported-by: Li Shuang <shuali@redhat.com>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
net/sched/cls_tcindex.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/sched/cls_tcindex.c b/net/sched/cls_tcindex.c
index ddaa4e6..9ccc93f 100644
--- a/net/sched/cls_tcindex.c
+++ b/net/sched/cls_tcindex.c
@@ -465,6 +465,7 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
struct tcindex_filter *nfp;
struct tcindex_filter __rcu **fp;
+ f->result.res = r->res;
tcf_exts_change(&f->result.exts, &r->exts);
fp = cp->h + (handle % cp->hash);
--
2.5.5
^ permalink raw reply related
* Re: [PATCH bpf-next V3] net/xdp: Fix suspicious RCU usage warning
From: Tariq Toukan @ 2018-08-13 10:57 UTC (permalink / raw)
To: Jesper Dangaard Brouer, Tariq Toukan
Cc: Alexei Starovoitov, Daniel Borkmann, netdev, Eran Ben Elisha,
Neil Brown, Paul E. McKenney
In-Reply-To: <20180813123159.1f447108@redhat.com>
On 13/08/2018 1:31 PM, Jesper Dangaard Brouer wrote:
> On Mon, 13 Aug 2018 12:21:58 +0300
> Tariq Toukan <tariqt@mellanox.com> wrote:
>
>> Fix the warning below by calling rhashtable_lookup_fast.
>> Also, make some code movements for better quality and human
>> readability.
>>
>> [ 342.450870] WARNING: suspicious RCU usage
>> [ 342.455856] 4.18.0-rc2+ #17 Tainted: G O
>> [ 342.462210] -----------------------------
>> [ 342.467202] ./include/linux/rhashtable.h:481 suspicious rcu_dereference_check() usage!
>> [ 342.476568]
>> [ 342.476568] other info that might help us debug this:
>> [ 342.476568]
>> [ 342.486978]
>> [ 342.486978] rcu_scheduler_active = 2, debug_locks = 1
>> [ 342.495211] 4 locks held by modprobe/3934:
>> [ 342.500265] #0: 00000000e23116b2 (mlx5_intf_mutex){+.+.}, at:
>> mlx5_unregister_interface+0x18/0x90 [mlx5_core]
>> [ 342.511953] #1: 00000000ca16db96 (rtnl_mutex){+.+.}, at: unregister_netdev+0xe/0x20
>> [ 342.521109] #2: 00000000a46e2c4b (&priv->state_lock){+.+.}, at: mlx5e_close+0x29/0x60
>> [mlx5_core]
>> [ 342.531642] #3: 0000000060c5bde3 (mem_id_lock){+.+.}, at: xdp_rxq_info_unreg+0x93/0x6b0
>> [ 342.541206]
>> [ 342.541206] stack backtrace:
>> [ 342.547075] CPU: 12 PID: 3934 Comm: modprobe Tainted: G O 4.18.0-rc2+ #17
>> [ 342.556621] Hardware name: Dell Inc. PowerEdge R730/0H21J3, BIOS 1.5.4 10/002/2015
>> [ 342.565606] Call Trace:
>> [ 342.568861] dump_stack+0x78/0xb3
>> [ 342.573086] xdp_rxq_info_unreg+0x3f5/0x6b0
>> [ 342.578285] ? __call_rcu+0x220/0x300
>> [ 342.582911] mlx5e_free_rq+0x38/0xc0 [mlx5_core]
>> [ 342.588602] mlx5e_close_channel+0x20/0x120 [mlx5_core]
>> [ 342.594976] mlx5e_close_channels+0x26/0x40 [mlx5_core]
>> [ 342.601345] mlx5e_close_locked+0x44/0x50 [mlx5_core]
>> [ 342.607519] mlx5e_close+0x42/0x60 [mlx5_core]
>> [ 342.613005] __dev_close_many+0xb1/0x120
>> [ 342.617911] dev_close_many+0xa2/0x170
>> [ 342.622622] rollback_registered_many+0x148/0x460
>> [ 342.628401] ? __lock_acquire+0x48d/0x11b0
>> [ 342.633498] ? unregister_netdev+0xe/0x20
>> [ 342.638495] rollback_registered+0x56/0x90
>> [ 342.643588] unregister_netdevice_queue+0x7e/0x100
>> [ 342.649461] unregister_netdev+0x18/0x20
>> [ 342.654362] mlx5e_remove+0x2a/0x50 [mlx5_core]
>> [ 342.659944] mlx5_remove_device+0xe5/0x110 [mlx5_core]
>> [ 342.666208] mlx5_unregister_interface+0x39/0x90 [mlx5_core]
>> [ 342.673038] cleanup+0x5/0xbfc [mlx5_core]
>> [ 342.678094] __x64_sys_delete_module+0x16b/0x240
>> [ 342.683725] ? do_syscall_64+0x1c/0x210
>> [ 342.688476] do_syscall_64+0x5a/0x210
>> [ 342.693025] entry_SYSCALL_64_after_hwframe+0x49/0xbe
>>
>> Fixes: 8d5d88527587 ("xdp: rhashtable with allocator ID to pointer mapping")
>> Signed-off-by: Tariq Toukan <tariqt@mellanox.com>
>> Suggested-by: Daniel Borkmann <daniel@iogearbox.net>
>> Cc: Jesper Dangaard Brouer <brouer@redhat.com>
>> ---
>> net/core/xdp.c | 13 +++----------
>> 1 file changed, 3 insertions(+), 10 deletions(-)
>>
>> V2 -> V3:
>> * Fix return value test for rhashtable_remove_fast, per Jesper's comment.
>>
>> V1 -> V2:
>> * Use rhashtable_lookup_fast and make some code movements, per Daniel's
>> and Alexei's comments.
>>
>> Please queue to -stable v4.18.
>>
>> diff --git a/net/core/xdp.c b/net/core/xdp.c
>> index 3dd99e1c04f5..8b1c7b699982 100644
>> --- a/net/core/xdp.c
>> +++ b/net/core/xdp.c
>> @@ -105,16 +105,9 @@ static void __xdp_rxq_info_unreg_mem_model(struct xdp_rxq_info *xdp_rxq)
>>
>> mutex_lock(&mem_id_lock);
>>
>> - xa = rhashtable_lookup(mem_id_ht, &id, mem_id_rht_params);
>> - if (!xa) {
>> - mutex_unlock(&mem_id_lock);
>> - return;
>> - }
>> -
>> - err = rhashtable_remove_fast(mem_id_ht, &xa->node, mem_id_rht_params);
>> - WARN_ON(err);
>> -
>> - call_rcu(&xa->rcu, __xdp_mem_allocator_rcu_free);
>> + xa = rhashtable_lookup_fast(mem_id_ht, &id, mem_id_rht_params);
>> + if (xa && !rhashtable_remove_fast(mem_id_ht, &xa->node, mem_id_rht_params))
>> + call_rcu(&xa->rcu, __xdp_mem_allocator_rcu_free);
>>
>> mutex_unlock(&mem_id_lock);
>> }
>
> I would personally prefer to write it as in the example as "== 0", look
> at example in [1] section "Object removal", but is it semantically the
> same to write !rhashtable_remove_fast(). So, I'm fine with this.
>
I thought the coding convention is to not explicitly compare to zero,
just like we do not compare to NULL on page allocation, but do:
if (!page)
But I don't mind changing this one.
> In the example[1], the sequence is wrapped in rcu_read_lock/unlock,
> while you have not done so. The rhashtable_lookup_fast and
> rhashtable_remove_fast calls have their own rcu_read_lock/unlock, but
> the outer rcu_read_lock/unlock, makes sure that a RCU period cannot
> slip in between the two calls.
>
> I still think your fix is correct, due to the mutex_lock. Given the
> mutex sync removal and insert in this rhashtable.
>
Right, we rely here on the mutex to avoid the scenario you described.
So the outer rcu lock calls are not necessary.
> I do wonder if it would be better to add the outer rcu_read_lock/unlock,
> calls if someone else reads and copy-paste this code (and don't have an
> mutex sync scheme) ?
>
Yeah it'll be safer for future unaware developers, but I think reviewers
should always comment and make it clear that the best generic reference
is [1], not any specific/optimized use case.
If you guys still want to me to fix this then please let me know and
I'll re-spin.
> If you think this is all fine, and want to proceed as is then you have
> my ack:
>
> Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
>
>
> [1] https://lwn.net/Articles/751374/
>
^ permalink raw reply
* Re: [Query]: DSA Understanding
From: Lad, Prabhakar @ 2018-08-13 11:06 UTC (permalink / raw)
To: Florian Fainelli; +Cc: Andrew Lunn, netdev
In-Reply-To: <d54e8489-6ba0-2ad9-4eb5-c8d3f847f34a@gmail.com>
Hi Florian,
On Fri, Aug 10, 2018 at 6:36 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>
> On 08/10/2018 04:26 AM, Lad, Prabhakar wrote:
> > Hi Andrew,
> >
> > On Thu, Aug 9, 2018 at 6:23 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >>
> >>> Its coming from the switch lan4 I have attached the png, where
> >>> C4:F3:12:08:FE:7F is
> >>> the mac of lan4, which is broadcast to ff:ff:ff:ff:ff:ff, which is
> >>> causing rx counter on
> >>> PC to go up.
> >>
> >> So, big packets are making it from the switch to the PC. But the small
> >> ARP packets are not.
> >>
> >> This is what Florian was suggesting.
> >>
> >> ARP packets are smaller than 64 bytes, which is the minimum packet
> >> size for Ethernet. Any packets smaller than 64 bytes are called runt
> >> packets. They have to be padded upto 64 bytes in order to make them
> >> valid. Otherwise the destination, or any switch along the path, might
> >> throw them away.
> >>
> >> What could be happening is that the CSPW driver or hardware is padding
> >> the packet to 64 bytes. But that packet has a DSA header in it. The
> >> switch removes the header, recalculate the checksum and sends the
> >> packet. It is now either 4 or 8 bytes smaller, depending on what DSA
> >> header was used. It then becomes a runt packet.
> >>
> > Thank you for the clarification, this really helped me out.
> >
> >> Florian had to fix this problem recently.
> >>
> >> http://patchwork.ozlabs.org/patch/836534/
> >>
> > But seems like this patch was never accepted, instead
> > brcm_tag_xmit_ll() does it if I am understanding it correctly.
> > similarly to this ksz_xmit() is taking care of padding.
>
> net/dsa/tag_brcm.c ended up doing the padding because that was a more
> generic and central location:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/tree/net/dsa/tag_brcm.c#n73
>
> >
> >> You probably need something similar for the cpsw.
> >>
> > looking at the tag_ksz.c in xmit function this is taken care of
>
> I agree, this should be padding packets correctly, can you still
> instrument cpsw to make sure that what comes to its ndo_start_xmit() is
> ETH_ZLEN + tag_len or more?
>
Yes I can confirm the skb->len is always >= 62 (ETH_ZLEN + 2)
Cheers,
--Prabhakar
^ permalink raw reply
* [PATCH][bpf-next] bpf: test: fix spelling mistake "REUSEEPORT" -> "REUSEPORT"
From: Colin King @ 2018-08-13 14:00 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Shuah Khan, netdev,
linux-kselftest
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
Trivial fix to spelling mistake in error message
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
tools/testing/selftests/bpf/test_maps.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/bpf/test_maps.c b/tools/testing/selftests/bpf/test_maps.c
index 4b7c74f5faa7..6f54f84144a0 100644
--- a/tools/testing/selftests/bpf/test_maps.c
+++ b/tools/testing/selftests/bpf/test_maps.c
@@ -1191,7 +1191,7 @@ static void prepare_reuseport_grp(int type, int map_fd,
err = setsockopt(fd64, SOL_SOCKET, SO_REUSEPORT,
&optval, sizeof(optval));
- CHECK(err == -1, "setsockopt(SO_REUSEEPORT)",
+ CHECK(err == -1, "setsockopt(SO_REUSEPORT)",
"err:%d errno:%d\n", err, errno);
/* reuseport_array does not allow unbound sk */
--
2.17.1
^ permalink raw reply related
* [PATCH v2 iproute2-next] sch_cake: Make gso-splitting configurable
From: Toke Høiland-Jørgensen @ 2018-08-13 11:36 UTC (permalink / raw)
To: David Ahern; +Cc: netdev, Toke Høiland-Jørgensen, Dave Taht
This patch makes sch_cake's gso/gro splitting configurable
from userspace.
To disable breaking apart superpackets in sch_cake:
tc qdisc replace dev whatever root cake no-split-gso
to enable:
tc qdisc replace dev whatever root cake split-gso
Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
Signed-off-by: Dave Taht <dave.taht@gmail.com>
---
v2:
- Fix x-mas tree variable order
- Also update man page
man/man8/tc-cake.8 | 23 +++++++++++++++++++++++
tc/q_cake.c | 9 +++++++++
2 files changed, 32 insertions(+)
diff --git a/man/man8/tc-cake.8 b/man/man8/tc-cake.8
index 0e84bc6e..b8e3089b 100644
--- a/man/man8/tc-cake.8
+++ b/man/man8/tc-cake.8
@@ -73,6 +73,12 @@ TIME |
]
.br
[
+.BR split-gso*
+|
+.BR no-split-gso
+]
+.br
+[
.BR ack-filter
|
.BR ack-filter-aggressive
@@ -546,6 +552,23 @@ If you are shaping inbound, and cannot trust the diffserv markings (as is the
case for Comcast Cable, among others), it is best to use a single queue
"besteffort" mode with wash.
+.PP
+.B split-gso
+
+.br
+ This option controls whether CAKE will split General Segmentation
+Offload (GSO) super-packets into their on-the-wire components and
+dequeue them individually.
+
+.br
+Super-packets are created by the networking stack to improve efficiency.
+However, because they are larger they take longer to dequeue, which
+translates to higher latency for competing flows, especially at lower
+bandwidths. CAKE defaults to splitting GSO packets to achieve the lowest
+possible latency. At link speeds higher than 10 Gbps, setting the
+no-split-gso parameter can increase the maximum achievable throughput by
+retaining the full GSO packets.
+
.SH EXAMPLES
# tc qdisc delete root dev eth0
.br
diff --git a/tc/q_cake.c b/tc/q_cake.c
index f1e232a6..50de46a7 100644
--- a/tc/q_cake.c
+++ b/tc/q_cake.c
@@ -79,6 +79,7 @@ static void explain(void)
" dual-srchost | dual-dsthost | triple-isolate* ]\n"
" [ nat | nonat* ]\n"
" [ wash | nowash* ]\n"
+" [ split-gso* | no-split-gso ]\n"
" [ ack-filter | ack-filter-aggressive | no-ack-filter* ]\n"
" [ memlimit LIMIT ]\n"
" [ ptm | atm | noatm* ] [ overhead N | conservative | raw* ]\n"
@@ -99,6 +100,7 @@ static int cake_parse_opt(struct qdisc_util *qu, int argc, char **argv,
__u64 bandwidth = 0;
int ack_filter = -1;
struct rtattr *tail;
+ int split_gso = -1;
int unlimited = 0;
int flowmode = -1;
int autorate = -1;
@@ -155,6 +157,10 @@ static int cake_parse_opt(struct qdisc_util *qu, int argc, char **argv,
wash = 0;
} else if (strcmp(*argv, "wash") == 0) {
wash = 1;
+ } else if (strcmp(*argv, "split-gso") == 0) {
+ split_gso = 1;
+ } else if (strcmp(*argv, "no-split-gso") == 0) {
+ split_gso = 0;
} else if (strcmp(*argv, "flowblind") == 0) {
flowmode = CAKE_FLOW_NONE;
} else if (strcmp(*argv, "srchost") == 0) {
@@ -374,6 +380,9 @@ static int cake_parse_opt(struct qdisc_util *qu, int argc, char **argv,
addattr_l(n, 1024, TCA_CAKE_NAT, &nat, sizeof(nat));
if (wash != -1)
addattr_l(n, 1024, TCA_CAKE_WASH, &wash, sizeof(wash));
+ if (split_gso != -1)
+ addattr_l(n, 1024, TCA_CAKE_SPLIT_GSO, &split_gso,
+ sizeof(split_gso));
if (ingress != -1)
addattr_l(n, 1024, TCA_CAKE_INGRESS, &ingress, sizeof(ingress));
if (ack_filter != -1)
--
2.18.0
^ permalink raw reply related
* Re: [PATCH] net: Change the layout of structure trace_event_raw_fib_table_lookup
From: David Ahern @ 2018-08-13 14:40 UTC (permalink / raw)
To: Zong Li, rostedt, mingo, netdev, linux-kernel; +Cc: zongbox, greentime
In-Reply-To: <1534127212-13186-1-git-send-email-zong@andestech.com>
On 8/12/18 8:26 PM, Zong Li wrote:
> There is an unalignment access about the structure
> 'trace_event_raw_fib_table_lookup'.
>
> In include/trace/events/fib.h, there is a memory operation which casting
> the 'src' data member to a pointer, and then store a value to this
> pointer point to.
>
> p32 = (__be32 *) __entry->src;
> *p32 = flp->saddr;
>
> The offset of 'src' in structure trace_event_raw_fib_table_lookup is not
> four bytes alignment. On some architectures, they don't permit the
> unalignment access, it need to pay the price to handle this situation in
> exception handler.
>
> Adjust the layout of structure to avoid this case.
>
> Signed-off-by: Zong Li <zong@andestech.com>
> ---
> include/trace/events/fib.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
Fixes: 9f323973c915 ("net/ipv4: Udate fib_table_lookup tracepoint")
Acked-by: David Ahern <dsahern@gmail.com>
Do you also need a similar change to fib6_table_lookup in
include/trace/events/fib6.h?
^ permalink raw reply
* Re: [PATCH bpf-next V3] net/xdp: Fix suspicious RCU usage warning
From: Jesper Dangaard Brouer @ 2018-08-13 12:02 UTC (permalink / raw)
To: Tariq Toukan
Cc: Alexei Starovoitov, Daniel Borkmann, netdev, Eran Ben Elisha,
Neil Brown, Paul E. McKenney, brouer
In-Reply-To: <85462631-9e86-5681-3ee7-558079d4776c@mellanox.com>
On Mon, 13 Aug 2018 13:57:04 +0300
Tariq Toukan <tariqt@mellanox.com> wrote:
> On 13/08/2018 1:31 PM, Jesper Dangaard Brouer wrote:
> > On Mon, 13 Aug 2018 12:21:58 +0300
> > Tariq Toukan <tariqt@mellanox.com> wrote:
> >
> >> Fix the warning below by calling rhashtable_lookup_fast.
> >> Also, make some code movements for better quality and human
> >> readability.
> >>
> >> [ 342.450870] WARNING: suspicious RCU usage
> >> [ 342.455856] 4.18.0-rc2+ #17 Tainted: G O
> >> [ 342.462210] -----------------------------
> >> [ 342.467202] ./include/linux/rhashtable.h:481 suspicious rcu_dereference_check() usage!
> >> [ 342.476568]
> >> [ 342.476568] other info that might help us debug this:
> >> [ 342.476568]
> >> [ 342.486978]
> >> [ 342.486978] rcu_scheduler_active = 2, debug_locks = 1
> >> [ 342.495211] 4 locks held by modprobe/3934:
> >> [ 342.500265] #0: 00000000e23116b2 (mlx5_intf_mutex){+.+.}, at:
> >> mlx5_unregister_interface+0x18/0x90 [mlx5_core]
> >> [ 342.511953] #1: 00000000ca16db96 (rtnl_mutex){+.+.}, at: unregister_netdev+0xe/0x20
> >> [ 342.521109] #2: 00000000a46e2c4b (&priv->state_lock){+.+.}, at: mlx5e_close+0x29/0x60
> >> [mlx5_core]
> >> [ 342.531642] #3: 0000000060c5bde3 (mem_id_lock){+.+.}, at: xdp_rxq_info_unreg+0x93/0x6b0
> >> [ 342.541206]
> >> [ 342.541206] stack backtrace:
> >> [ 342.547075] CPU: 12 PID: 3934 Comm: modprobe Tainted: G O 4.18.0-rc2+ #17
> >> [ 342.556621] Hardware name: Dell Inc. PowerEdge R730/0H21J3, BIOS 1.5.4 10/002/2015
> >> [ 342.565606] Call Trace:
> >> [ 342.568861] dump_stack+0x78/0xb3
> >> [ 342.573086] xdp_rxq_info_unreg+0x3f5/0x6b0
> >> [ 342.578285] ? __call_rcu+0x220/0x300
> >> [ 342.582911] mlx5e_free_rq+0x38/0xc0 [mlx5_core]
> >> [ 342.588602] mlx5e_close_channel+0x20/0x120 [mlx5_core]
> >> [ 342.594976] mlx5e_close_channels+0x26/0x40 [mlx5_core]
> >> [ 342.601345] mlx5e_close_locked+0x44/0x50 [mlx5_core]
> >> [ 342.607519] mlx5e_close+0x42/0x60 [mlx5_core]
> >> [ 342.613005] __dev_close_many+0xb1/0x120
> >> [ 342.617911] dev_close_many+0xa2/0x170
> >> [ 342.622622] rollback_registered_many+0x148/0x460
> >> [ 342.628401] ? __lock_acquire+0x48d/0x11b0
> >> [ 342.633498] ? unregister_netdev+0xe/0x20
> >> [ 342.638495] rollback_registered+0x56/0x90
> >> [ 342.643588] unregister_netdevice_queue+0x7e/0x100
> >> [ 342.649461] unregister_netdev+0x18/0x20
> >> [ 342.654362] mlx5e_remove+0x2a/0x50 [mlx5_core]
> >> [ 342.659944] mlx5_remove_device+0xe5/0x110 [mlx5_core]
> >> [ 342.666208] mlx5_unregister_interface+0x39/0x90 [mlx5_core]
> >> [ 342.673038] cleanup+0x5/0xbfc [mlx5_core]
> >> [ 342.678094] __x64_sys_delete_module+0x16b/0x240
> >> [ 342.683725] ? do_syscall_64+0x1c/0x210
> >> [ 342.688476] do_syscall_64+0x5a/0x210
> >> [ 342.693025] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> >>
> >> Fixes: 8d5d88527587 ("xdp: rhashtable with allocator ID to pointer mapping")
> >> Signed-off-by: Tariq Toukan <tariqt@mellanox.com>
> >> Suggested-by: Daniel Borkmann <daniel@iogearbox.net>
> >> Cc: Jesper Dangaard Brouer <brouer@redhat.com>
> >> ---
> >> net/core/xdp.c | 13 +++----------
> >> 1 file changed, 3 insertions(+), 10 deletions(-)
> >>
> >> V2 -> V3:
> >> * Fix return value test for rhashtable_remove_fast, per Jesper's comment.
> >>
> >> V1 -> V2:
> >> * Use rhashtable_lookup_fast and make some code movements, per Daniel's
> >> and Alexei's comments.
> >>
> >> Please queue to -stable v4.18.
> >>
> >> diff --git a/net/core/xdp.c b/net/core/xdp.c
> >> index 3dd99e1c04f5..8b1c7b699982 100644
> >> --- a/net/core/xdp.c
> >> +++ b/net/core/xdp.c
> >> @@ -105,16 +105,9 @@ static void __xdp_rxq_info_unreg_mem_model(struct xdp_rxq_info *xdp_rxq)
> >>
> >> mutex_lock(&mem_id_lock);
> >>
> >> - xa = rhashtable_lookup(mem_id_ht, &id, mem_id_rht_params);
> >> - if (!xa) {
> >> - mutex_unlock(&mem_id_lock);
> >> - return;
> >> - }
> >> -
> >> - err = rhashtable_remove_fast(mem_id_ht, &xa->node, mem_id_rht_params);
> >> - WARN_ON(err);
> >> -
> >> - call_rcu(&xa->rcu, __xdp_mem_allocator_rcu_free);
> >> + xa = rhashtable_lookup_fast(mem_id_ht, &id, mem_id_rht_params);
> >> + if (xa && !rhashtable_remove_fast(mem_id_ht, &xa->node, mem_id_rht_params))
> >> + call_rcu(&xa->rcu, __xdp_mem_allocator_rcu_free);
> >>
> >> mutex_unlock(&mem_id_lock);
> >> }
> >
> > I would personally prefer to write it as in the example as "== 0", look
> > at example in [1] section "Object removal", but is it semantically the
> > same to write !rhashtable_remove_fast(). So, I'm fine with this.
> >
>
> I thought the coding convention is to not explicitly compare to zero,
> just like we do not compare to NULL on page allocation, but do:
> if (!page)
If the return value is a pointer, then I use the (!ptr) check, and also
if the return value is a bool. In this case where the success is 0, I
find it slightly confusing to read if(!remove) then success-case.
> But I don't mind changing this one.
I also don't care much... if you do respin, it would be nice to do.
> > In the example[1], the sequence is wrapped in rcu_read_lock/unlock,
> > while you have not done so. The rhashtable_lookup_fast and
> > rhashtable_remove_fast calls have their own rcu_read_lock/unlock,
> > but the outer rcu_read_lock/unlock, makes sure that a RCU period
> > cannot slip in between the two calls.
> >
> > I still think your fix is correct, due to the mutex_lock. Given the
> > mutex sync removal and insert in this rhashtable.
> >
>
> Right, we rely here on the mutex to avoid the scenario you described.
> So the outer rcu lock calls are not necessary.
>
> > I do wonder if it would be better to add the outer
> > rcu_read_lock/unlock, calls if someone else reads and copy-paste
> > this code (and don't have an mutex sync scheme) ?
> >
>
> Yeah it'll be safer for future unaware developers, but I think
> reviewers should always comment and make it clear that the best
> generic reference is [1], not any specific/optimized use case.
>
> If you guys still want to me to fix this then please let me know and
> I'll re-spin.
I'll let Daniel make the choice.
> > If you think this is all fine, and want to proceed as is then you
> > have my ack:
> >
> > Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
> >
> >
> > [1] https://lwn.net/Articles/751374/
> >
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* [PATCH 2/3] IB/ipoib: Stop using dev_id to expose port numbers
From: Arseny Maslennikov @ 2018-08-13 11:42 UTC (permalink / raw)
To: linux-rdma; +Cc: Arseny Maslennikov, Doug Ledford, Jason Gunthorpe, netdev
In-Reply-To: <20180813114224.7065-1-ar@cs.msu.ru>
Some InfiniBand network devices have multiple ports on the same PCI
function. Prior to this the kernel erroneously used the `dev_id' sysfs
field of those network interfaces to convey the port number to userspace.
`dev_id' is currently reserved for distinguishing stacked ifaces
(e.g: VLANs) with the same hardware address as their parent device.
Similar fixes to net/mlx4_en and many other drivers, which started
exporting this information through `dev_id' before 3.15, were accepted
into the kernel 4 years ago.
See 76a066f2a2a0268b565459c417b59724b5a3197b, commit message:
`net/mlx4_en: Expose port number through sysfs'.
I would be OK with this commit not being backported to stable, since
it might break admin-supplied udev rules and the likes.
Signed-off-by: Arseny Maslennikov <ar@cs.msu.ru>
---
drivers/infiniband/ulp/ipoib/ipoib_main.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index 6eb0594fffec..f64535038147 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -2252,7 +2252,6 @@ static struct net_device *ipoib_add_port(const char *format,
}
SET_NETDEV_DEV(priv->dev, hca->dev.parent);
- priv->dev->dev_id = port - 1;
priv->dev->dev_port = port - 1;
result = ib_query_port(hca, port, &attr);
--
2.18.0
^ permalink raw reply related
* [PATCH 3/3] Documentation/ABI: document /sys/class/net/*/dev_port
From: Arseny Maslennikov @ 2018-08-13 11:42 UTC (permalink / raw)
To: linux-rdma; +Cc: Arseny Maslennikov, Doug Ledford, Jason Gunthorpe, netdev
In-Reply-To: <20180813114224.7065-1-ar@cs.msu.ru>
The sysfs field was introduced 4 years ago along with fixes to various
drivers that erroneously used `dev_id' for that purpose, but it was not
properly documented anywhere.
See commit v3.14-rc3-739-g3f85944fe207.
Signed-off-by: Arseny Maslennikov <ar@cs.msu.ru>
---
Documentation/ABI/testing/sysfs-class-net | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/Documentation/ABI/testing/sysfs-class-net b/Documentation/ABI/testing/sysfs-class-net
index 2f1788111cd9..1593d8997ade 100644
--- a/Documentation/ABI/testing/sysfs-class-net
+++ b/Documentation/ABI/testing/sysfs-class-net
@@ -91,6 +91,16 @@ Description:
stacked (e.g: VLAN interfaces) but still have the same MAC
address as their parent device.
+What: /sys/class/net/<iface>/dev_port
+Date: February 2014
+KernelVersion: 3.15
+Contact: netdev@vger.kernel.org
+Description:
+ Indicates the port number of this network device, formatted
+ as a decimal value. Some NICs have multiple independent ports
+ on the same PCI bus, device and function. This field allows
+ userspace to distinguish the respective interfaces.
+
What: /sys/class/net/<iface>/dormant
Date: March 2006
KernelVersion: 2.6.17
--
2.18.0
^ permalink raw reply related
* [PATCH 1/3] IB/ipoib: Use dev_port to expose network interface port numbers
From: Arseny Maslennikov @ 2018-08-13 11:42 UTC (permalink / raw)
To: linux-rdma; +Cc: Arseny Maslennikov, Doug Ledford, Jason Gunthorpe, netdev
In-Reply-To: <20180813114224.7065-1-ar@cs.msu.ru>
Some InfiniBand network devices have multiple ports on the same PCI
function. This initializes the `dev_port' sysfs field of those
network interfaces with their port number.
The use of `dev_id' was considered correct until Linux 3.15,
when another field, `dev_port', was defined for this particular
purpose and `dev_id' was reserved for distinguishing stacked ifaces
(e.g: VLANs) with the same hardware address as their parent device.
Similar fixes to net/mlx4_en and many other drivers, which started
exporting this information through `dev_id' before 3.15, were accepted
into the kernel 4 years ago.
See 76a066f2a2a0268b565459c417b59724b5a3197b, commit message:
`net/mlx4_en: Expose port number through sysfs'.
Signed-off-by: Arseny Maslennikov <ar@cs.msu.ru>
---
drivers/infiniband/ulp/ipoib/ipoib_main.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index 26cde95bc0f3..6eb0594fffec 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -2253,6 +2253,7 @@ static struct net_device *ipoib_add_port(const char *format,
SET_NETDEV_DEV(priv->dev, hca->dev.parent);
priv->dev->dev_id = port - 1;
+ priv->dev->dev_port = port - 1;
result = ib_query_port(hca, port, &attr);
if (result) {
--
2.18.0
^ permalink raw reply related
* [PATCH 0/3] net, IB/ipoib: Use dev_port to disambiguate
From: Arseny Maslennikov @ 2018-08-13 11:42 UTC (permalink / raw)
To: linux-rdma; +Cc: Arseny Maslennikov, Doug Ledford, Jason Gunthorpe, netdev
Pre-3.15 userspace had trouble distinguishing different ports of a NIC
on a single PCI bus/device/function. To solve this, a sysfs field `dev_port'
was introduced quite a while ago (commit v3.14-rc3-739-g3f85944fe207), and
some relevant device drivers were fixed to use it, but not in case of IPoIB.
The convention for some reason never got documented in the kernel, but
was immediately adopted by userspace (notably udev[1][2], biosdevname[3])
3/3 documents the sysfs field — that's why I'm CC-ing netdev.
This series was tested on current LTS and 4.18.
[1] https://lists.freedesktop.org/archives/systemd-devel/2014-June/020788.html
[2] https://lists.freedesktop.org/archives/systemd-devel/2014-July/020804.html
[3] https://github.com/CloudAutomationNTools/biosdevname/blob/c795d51dd93a5309652f0d635f12a3ecfabfaa72/src/eths.c#L38
Arseny Maslennikov (3):
IB/ipoib: Use dev_port to expose network interface port numbers
IB/ipoib: Stop using dev_id to expose port numbers
Documentation/ABI: document /sys/class/net/*/dev_port
Documentation/ABI/testing/sysfs-class-net | 10 ++++++++++
drivers/infiniband/ulp/ipoib/ipoib_main.c | 2 +-
2 files changed, 11 insertions(+), 1 deletion(-)
--
2.18.0
^ permalink raw reply
* Re: [PATCH bpf-next V3] net/xdp: Fix suspicious RCU usage warning
From: Daniel Borkmann @ 2018-08-13 12:22 UTC (permalink / raw)
To: Jesper Dangaard Brouer, Tariq Toukan
Cc: Alexei Starovoitov, netdev, Eran Ben Elisha, Neil Brown,
Paul E. McKenney
In-Reply-To: <20180813140243.4484815e@redhat.com>
On 08/13/2018 02:02 PM, Jesper Dangaard Brouer wrote:
> On Mon, 13 Aug 2018 13:57:04 +0300
> Tariq Toukan <tariqt@mellanox.com> wrote:
>> On 13/08/2018 1:31 PM, Jesper Dangaard Brouer wrote:
>>> On Mon, 13 Aug 2018 12:21:58 +0300
>>> Tariq Toukan <tariqt@mellanox.com> wrote:
[...]
>>> In the example[1], the sequence is wrapped in rcu_read_lock/unlock,
>>> while you have not done so. The rhashtable_lookup_fast and
>>> rhashtable_remove_fast calls have their own rcu_read_lock/unlock,
>>> but the outer rcu_read_lock/unlock, makes sure that a RCU period
>>> cannot slip in between the two calls.
>>>
>>> I still think your fix is correct, due to the mutex_lock. Given the
>>> mutex sync removal and insert in this rhashtable.
>>
>> Right, we rely here on the mutex to avoid the scenario you described.
>> So the outer rcu lock calls are not necessary.
Agree.
>>> I do wonder if it would be better to add the outer
>>> rcu_read_lock/unlock, calls if someone else reads and copy-paste
>>> this code (and don't have an mutex sync scheme) ?
>>
>> Yeah it'll be safer for future unaware developers, but I think
>> reviewers should always comment and make it clear that the best
>> generic reference is [1], not any specific/optimized use case.
>>
>> If you guys still want to me to fix this then please let me know and
>> I'll re-spin.
>
> I'll let Daniel make the choice.
Patch is fine as is. If we would be adding the RCU read lock/unlock pair
even though it's not necessary but for other developers to copy paste
from, I think this might be double-confusing: in the one case for people
reading the current code as they will wonder why the additional RCU read
side is needed here (so it will leave them puzzling), and in the other
case for people trying to copy-paste from it wondering whether they would
need similar scheme with mutex in addition. So I strongly prefer to 'do
the right thing' based on the situation. Given BPF PR is still pending,
I'll get the patch in once it has been pulled.
Thanks,
Daniel
^ permalink raw reply
* Re: [PATCH net 2/2] net/mlx5e: Cleanup of dcbnl related fields
From: Yuval Shaia @ 2018-08-13 12:32 UTC (permalink / raw)
To: Saeed Mahameed; +Cc: David S. Miller, netdev, Huy Nguyen
In-Reply-To: <20180808224808.12600-3-saeedm@mellanox.com>
On Wed, Aug 08, 2018 at 03:48:08PM -0700, Saeed Mahameed wrote:
> From: Huy Nguyen <huyn@mellanox.com>
>
> Remove unused netdev_registered_init/remove in en.h
> Return ENOSUPPORT if the check MLX5_DSCP_SUPPORTED fails.
> Remove extra white space
>
> Fixes: 2a5e7a1344f4 ("net/mlx5e: Add dcbnl dscp to priority support")
> Signed-off-by: Huy Nguyen <huyn@mellanox.com>
> Cc: Yuval Shaia <yuval.shaia@oracle.com>
> Reviewed-by: Parav Pandit <parav@mellanox.com>
> Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
FWIW:
Reviewed-by: Yuval Shaia <yuval.shaia@oracle.com>
Tested-by: Yuval Shaia <yuval.shaia@oracle.com>
> ---
> drivers/net/ethernet/mellanox/mlx5/core/en.h | 2 --
> .../ethernet/mellanox/mlx5/core/en_dcbnl.c | 30 +++++++------------
> 2 files changed, 11 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en.h b/drivers/net/ethernet/mellanox/mlx5/core/en.h
> index eb9eb7aa953a..405236cf0b04 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en.h
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en.h
> @@ -858,8 +858,6 @@ struct mlx5e_profile {
> mlx5e_fp_handle_rx_cqe handle_rx_cqe;
> mlx5e_fp_handle_rx_cqe handle_rx_cqe_mpwqe;
> } rx_handlers;
> - void (*netdev_registered_init)(struct mlx5e_priv *priv);
> - void (*netdev_registered_remove)(struct mlx5e_priv *priv);
> int max_tc;
> };
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c b/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c
> index e33afa8d2417..722998d68564 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c
> @@ -443,16 +443,12 @@ static int mlx5e_dcbnl_ieee_setapp(struct net_device *dev, struct dcb_app *app)
> bool is_new;
> int err;
>
> - if (app->selector != IEEE_8021QAZ_APP_SEL_DSCP)
> - return -EINVAL;
> -
> - if (!MLX5_CAP_GEN(priv->mdev, vport_group_manager))
> - return -EINVAL;
> -
> - if (!MLX5_DSCP_SUPPORTED(priv->mdev))
> - return -EINVAL;
> + if (!MLX5_CAP_GEN(priv->mdev, vport_group_manager) ||
> + !MLX5_DSCP_SUPPORTED(priv->mdev))
> + return -EOPNOTSUPP;
>
> - if (app->protocol >= MLX5E_MAX_DSCP)
> + if ((app->selector != IEEE_8021QAZ_APP_SEL_DSCP) ||
> + (app->protocol >= MLX5E_MAX_DSCP))
> return -EINVAL;
>
> /* Save the old entry info */
> @@ -500,16 +496,12 @@ static int mlx5e_dcbnl_ieee_delapp(struct net_device *dev, struct dcb_app *app)
> struct mlx5e_priv *priv = netdev_priv(dev);
> int err;
>
> - if (app->selector != IEEE_8021QAZ_APP_SEL_DSCP)
> - return -EINVAL;
> -
> - if (!MLX5_CAP_GEN(priv->mdev, vport_group_manager))
> - return -EINVAL;
> -
> - if (!MLX5_DSCP_SUPPORTED(priv->mdev))
> - return -EINVAL;
> + if (!MLX5_CAP_GEN(priv->mdev, vport_group_manager) ||
> + !MLX5_DSCP_SUPPORTED(priv->mdev))
> + return -EOPNOTSUPP;
>
> - if (app->protocol >= MLX5E_MAX_DSCP)
> + if ((app->selector != IEEE_8021QAZ_APP_SEL_DSCP) ||
> + (app->protocol >= MLX5E_MAX_DSCP))
> return -EINVAL;
>
> /* Skip if no dscp app entry */
> @@ -1146,7 +1138,7 @@ static int mlx5e_set_trust_state(struct mlx5e_priv *priv, u8 trust_state)
> {
> int err;
>
> - err = mlx5_set_trust_state(priv->mdev, trust_state);
> + err = mlx5_set_trust_state(priv->mdev, trust_state);
> if (err)
> return err;
> priv->dcbx_dp.trust_state = trust_state;
> --
> 2.17.0
>
^ permalink raw reply
* Re: [PATCH 2/3] IB/ipoib: Stop using dev_id to expose port numbers
From: Yuval Shaia @ 2018-08-13 12:40 UTC (permalink / raw)
To: Arseny Maslennikov; +Cc: linux-rdma, Doug Ledford, Jason Gunthorpe, netdev
In-Reply-To: <20180813114224.7065-3-ar@cs.msu.ru>
On Mon, Aug 13, 2018 at 02:42:23PM +0300, Arseny Maslennikov wrote:
> Some InfiniBand network devices have multiple ports on the same PCI
> function. Prior to this the kernel erroneously used the `dev_id' sysfs
> field of those network interfaces to convey the port number to userspace.
>
> `dev_id' is currently reserved for distinguishing stacked ifaces
> (e.g: VLANs) with the same hardware address as their parent device.
>
> Similar fixes to net/mlx4_en and many other drivers, which started
> exporting this information through `dev_id' before 3.15, were accepted
> into the kernel 4 years ago.
> See 76a066f2a2a0268b565459c417b59724b5a3197b, commit message:
> `net/mlx4_en: Expose port number through sysfs'.
>
> I would be OK with this commit not being backported to stable, since
> it might break admin-supplied udev rules and the likes.
>
> Signed-off-by: Arseny Maslennikov <ar@cs.msu.ru>
> ---
> drivers/infiniband/ulp/ipoib/ipoib_main.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> index 6eb0594fffec..f64535038147 100644
> --- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
> +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> @@ -2252,7 +2252,6 @@ static struct net_device *ipoib_add_port(const char *format,
> }
>
> SET_NETDEV_DEV(priv->dev, hca->dev.parent);
> - priv->dev->dev_id = port - 1;
Correct me if i'm wrong here but besides some changes in commit message
looks like patch 1/3 is the same as 2/3, isn't it?
Yuval
> priv->dev->dev_port = port - 1;
>
> result = ib_query_port(hca, port, &attr);
> --
> 2.18.0
>
^ permalink raw reply
* Re: possible deadlock in flush_work (3)
From: Xin Long @ 2018-08-13 15:39 UTC (permalink / raw)
To: syzbot
Cc: Christian Brauner, davem, David Ahern, Florian Westphal,
Jiri Benc, Kirill Tkhai, LKML, network dev, Nicolas Dichtel,
syzkaller-bugs, Moni Shoua
In-Reply-To: <0000000000009637a6057350529b@google.com>
On Mon, Aug 13, 2018 at 8:35 PM, syzbot
<syzbot+a8371264572a6872b8a3@syzkaller.appspotmail.com> wrote:
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit: d6dd6431591b Merge branch 'fixes' of git://git.kernel.org/..
> git tree: upstream
> console output: https://syzkaller.appspot.com/x/log.txt?x=14e800aa400000
> kernel config: https://syzkaller.appspot.com/x/.config?x=152cb8ccd35b1f70
> dashboard link: https://syzkaller.appspot.com/bug?extid=a8371264572a6872b8a3
> compiler: gcc (GCC) 8.0.1 20180413 (experimental)
>
> Unfortunately, I don't have any reproducer for this crash yet.
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+a8371264572a6872b8a3@syzkaller.appspotmail.com
>
> 8021q: adding VLAN 0 to HW filter on device bond0
>
> ======================================================
> WARNING: possible circular locking dependency detected
> 4.18.0-rc8+ #185 Not tainted
> ------------------------------------------------------
> syz-executor2/6421 is trying to acquire lock:
> 00000000209b4e4b ((wq_completion)bond_dev->name){+.+.}, at: start_flush_work
> kernel/workqueue.c:2888 [inline]
> 00000000209b4e4b ((wq_completion)bond_dev->name){+.+.}, at:
> flush_work+0x4b8/0x900 kernel/workqueue.c:2917
>
> but task is already holding lock:
> 00000000f0f3d47a (rtnl_mutex){+.+.}, at: rtnl_lock+0x17/0x20
> net/core/rtnetlink.c:77
>
> which lock already depends on the new lock.
>
>
> the existing dependency chain (in reverse order) is:
>
> -> #2 (rtnl_mutex){+.+.}:
> __mutex_lock_common kernel/locking/mutex.c:757 [inline]
> __mutex_lock+0x176/0x1820 kernel/locking/mutex.c:894
> mutex_lock_nested+0x16/0x20 kernel/locking/mutex.c:909
> rtnl_lock+0x17/0x20 net/core/rtnetlink.c:77
> bond_netdev_notify drivers/net/bonding/bond_main.c:1310 [inline]
> bond_netdev_notify_work+0x44/0xd0
> drivers/net/bonding/bond_main.c:1320
> process_one_work+0xc73/0x1ba0 kernel/workqueue.c:2153
> worker_thread+0x189/0x13c0 kernel/workqueue.c:2296
> kthread+0x345/0x410 kernel/kthread.c:246
> ret_from_fork+0x3a/0x50 arch/x86/entry/entry_64.S:412
>
> -> #1 ((work_completion)(&(&nnw->work)->work)){+.+.}:
> process_one_work+0xc0b/0x1ba0 kernel/workqueue.c:2129
> worker_thread+0x189/0x13c0 kernel/workqueue.c:2296
> kthread+0x345/0x410 kernel/kthread.c:246
> ret_from_fork+0x3a/0x50 arch/x86/entry/entry_64.S:412
>
> -> #0 ((wq_completion)bond_dev->name){+.+.}:
> lock_acquire+0x1e4/0x540 kernel/locking/lockdep.c:3924
> start_flush_work kernel/workqueue.c:2889 [inline]
> flush_work+0x4dd/0x900 kernel/workqueue.c:2917
> __cancel_work_timer+0x4bd/0x830 kernel/workqueue.c:2989
> cancel_delayed_work_sync+0x1a/0x20 kernel/workqueue.c:3121
> bond_work_cancel_all drivers/net/bonding/bond_main.c:3318 [inline]
> bond_close+0x1b/0x130 drivers/net/bonding/bond_main.c:3381
> __dev_close_many+0x21e/0x380 net/core/dev.c:1476
> __dev_close net/core/dev.c:1488 [inline]
> __dev_change_flags+0x38d/0x9c0 net/core/dev.c:6989
> dev_change_flags+0x89/0x150 net/core/dev.c:7060
> dev_ifsioc+0x84f/0xb30 net/core/dev_ioctl.c:237
> dev_ioctl+0x1b5/0xcc0 net/core/dev_ioctl.c:493
> sock_do_ioctl+0x1d3/0x3e0 net/socket.c:993
> sock_ioctl+0x30d/0x680 net/socket.c:1094
> vfs_ioctl fs/ioctl.c:46 [inline]
> file_ioctl fs/ioctl.c:500 [inline]
> do_vfs_ioctl+0x1de/0x1720 fs/ioctl.c:684
> ksys_ioctl+0xa9/0xd0 fs/ioctl.c:701
> __do_sys_ioctl fs/ioctl.c:708 [inline]
> __se_sys_ioctl fs/ioctl.c:706 [inline]
> __x64_sys_ioctl+0x73/0xb0 fs/ioctl.c:706
> do_syscall_64+0x1b9/0x820 arch/x86/entry/common.c:290
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> other info that might help us debug this:
>
> Chain exists of:
> (wq_completion)bond_dev->name --> (work_completion)(&(&nnw->work)->work)
> --> rtnl_mutex
>
> Possible unsafe locking scenario:
>
> CPU0 CPU1
> ---- ----
> lock(rtnl_mutex);
> lock((work_completion)(&(&nnw->work)->work));
> lock(rtnl_mutex);
> lock((wq_completion)bond_dev->name);
nnw->work is queuing up into bond->wq, so it seems bond_netdev_notify()
should have used rtnl_trylock() instead of rtnl_lock(), as do other
delayed_work handlers in bond->wq.
>
> *** DEADLOCK ***
>
> 1 lock held by syz-executor2/6421:
> #0: 00000000f0f3d47a (rtnl_mutex){+.+.}, at: rtnl_lock+0x17/0x20
> net/core/rtnetlink.c:77
>
> stack backtrace:
> CPU: 0 PID: 6421 Comm: syz-executor2 Not tainted 4.18.0-rc8+ #185
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> Google 01/01/2011
> Call Trace:
> __dump_stack lib/dump_stack.c:77 [inline]
> dump_stack+0x1c9/0x2b4 lib/dump_stack.c:113
> print_circular_bug.isra.36.cold.57+0x1bd/0x27d
> kernel/locking/lockdep.c:1227
> check_prev_add kernel/locking/lockdep.c:1867 [inline]
> check_prevs_add kernel/locking/lockdep.c:1980 [inline]
> validate_chain kernel/locking/lockdep.c:2421 [inline]
> __lock_acquire+0x3449/0x5020 kernel/locking/lockdep.c:3435
> lock_acquire+0x1e4/0x540 kernel/locking/lockdep.c:3924
> start_flush_work kernel/workqueue.c:2889 [inline]
> flush_work+0x4dd/0x900 kernel/workqueue.c:2917
> __cancel_work_timer+0x4bd/0x830 kernel/workqueue.c:2989
> cancel_delayed_work_sync+0x1a/0x20 kernel/workqueue.c:3121
> bond_work_cancel_all drivers/net/bonding/bond_main.c:3318 [inline]
> bond_close+0x1b/0x130 drivers/net/bonding/bond_main.c:3381
> __dev_close_many+0x21e/0x380 net/core/dev.c:1476
> __dev_close net/core/dev.c:1488 [inline]
> __dev_change_flags+0x38d/0x9c0 net/core/dev.c:6989
> dev_change_flags+0x89/0x150 net/core/dev.c:7060
> dev_ifsioc+0x84f/0xb30 net/core/dev_ioctl.c:237
> dev_ioctl+0x1b5/0xcc0 net/core/dev_ioctl.c:493
> sock_do_ioctl+0x1d3/0x3e0 net/socket.c:993
> kernel msg: ebtables bug: please report to author: Wrong nr of counters
> sock_ioctl+0x30d/0x680 net/socket.c:1094
> vfs_ioctl fs/ioctl.c:46 [inline]
> file_ioctl fs/ioctl.c:500 [inline]
> do_vfs_ioctl+0x1de/0x1720 fs/ioctl.c:684
> ksys_ioctl+0xa9/0xd0 fs/ioctl.c:701
> __do_sys_ioctl fs/ioctl.c:708 [inline]
> __se_sys_ioctl fs/ioctl.c:706 [inline]
> __x64_sys_ioctl+0x73/0xb0 fs/ioctl.c:706
> do_syscall_64+0x1b9/0x820 arch/x86/entry/common.c:290
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
> RIP: 0033:0x457089
> Code: fd b4 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7
> 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff
> 0f 83 cb b4 fb ff c3 66 2e 0f 1f 84 00 00 00 00
> RSP: 002b:00007fee7b2ccc78 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
> RAX: ffffffffffffffda RBX: 00007fee7b2cd6d4 RCX: 0000000000457089
> RDX: 0000000020000140 RSI: 0000000000008914 RDI: 0000000000000004
> RBP: 00000000009300a0 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000246 R12: 00000000ffffffff
> R13: 00000000004d19e0 R14: 00000000004c7454 R15: 0000000000000000
> 8021q: adding VLAN 0 to HW filter on device bond0
> (unnamed net_device) (uninitialized): option miimon: invalid value
> (18446744073709551615)
> (unnamed net_device) (uninitialized): option miimon: allowed values 0 -
> 2147483647
> (unnamed net_device) (uninitialized): option miimon: invalid value
> (18446744073709551615)
> (unnamed net_device) (uninitialized): option miimon: allowed values 0 -
> 2147483647
> 8021q: adding VLAN 0 to HW filter on device bond0
> 8021q: adding VLAN 0 to HW filter on device bond0
> 8021q: adding VLAN 0 to HW filter on device bond0
> 8021q: adding VLAN 0 to HW filter on device bond0
> 8021q: adding VLAN 0 to HW filter on device bond0
> 8021q: adding VLAN 0 to HW filter on device bond0
> 8021q: adding VLAN 0 to HW filter on device bond0
> 8021q: adding VLAN 0 to HW filter on device bond0
> 8021q: adding VLAN 0 to HW filter on device bond0
> 8021q: adding VLAN 0 to HW filter on device bond0
> 8021q: adding VLAN 0 to HW filter on device bond0
> 8021q: adding VLAN 0 to HW filter on device bond0
> 8021q: adding VLAN 0 to HW filter on device bond0
> device bond0 left promiscuous mode
> IPv6: ADDRCONF(NETDEV_UP): bond0: link is not ready
> 8021q: adding VLAN 0 to HW filter on device bond0
> 8021q: adding VLAN 0 to HW filter on device bond0
> 8021q: adding VLAN 0 to HW filter on device bond0
> IPv6: ADDRCONF(NETDEV_UP): bond0: link is not ready
> 8021q: adding VLAN 0 to HW filter on device bond0
> 8021q: adding VLAN 0 to HW filter on device bond0
> 8021q: adding VLAN 0 to HW filter on device bond0
> IPv6: ADDRCONF(NETDEV_UP): bond0: link is not ready
> 8021q: adding VLAN 0 to HW filter on device bond0
> 8021q: adding VLAN 0 to HW filter on device bond0
> 8021q: adding VLAN 0 to HW filter on device bond0
> 8021q: adding VLAN 0 to HW filter on device bond0
> 8021q: adding VLAN 0 to HW filter on device bond0
> 8021q: adding VLAN 0 to HW filter on device bond0
> 8021q: adding VLAN 0 to HW filter on device bond0
> 8021q: adding VLAN 0 to HW filter on device bond0
>
>
> ---
> This bug is generated by a bot. It may contain errors.
> See https://goo.gl/tpsmEJ for more information about syzbot.
> syzbot engineers can be reached at syzkaller@googlegroups.com.
>
> syzbot will keep track of this bug report. See:
> https://goo.gl/tpsmEJ#bug-status-tracking for how to communicate with
> syzbot.
^ permalink raw reply
* Re: [PATCH v1 0/3] WireGuard: Secure Network Tunnel
From: James Bottomley @ 2018-08-13 15:40 UTC (permalink / raw)
To: Jason A. Donenfeld; +Cc: linux-kernel, netdev, davem, linux-crypto
In-Reply-To: <20180731191102.2434-1-Jason@zx2c4.com>
> Ample information, including documentation, installation
> instructions,
> and project details, is available at:
>
> * https://www.wireguard.com/
> * https://www.wireguard.com/papers/wireguard.pdf
In your paper you say this:
> Finally, WireGuard is cryptographically opinionated. It intentionally
> lacks cipher and protocol agility. If
> holes are found in the underlying primitives, all endpoints will be
> required to update.
The only thing that's certain (beyond death and taxes) is that your
crypto choice will one day need updating; either in response to an
urgent CVE because an algorithm is compromised or in response to a less
urgent one because it is deprecated. Assuming wireguard is reasonably
successful we'll have a large ecosystem dependent on it. On this day,
we're going to have the choice of either breaking the entire ecosystem
by rolling out a change that can't connect to lower protocol versions
or trying to wedge version agility into wireguard in a hurry. The
former is too awful to contemplate because of the almost universal
ecosystem breakage it would cause and the latter is going to lead to
additional bugs because people in a hurry aren't as careful as they
should be.
Could we please build planning for this crypto failure day into
wireguard now rather than have to do it later? It doesn't need to be
full cipher agility, it just needs to be the ability to handle multiple
protocol versions ... two should do it because that gives a template to
follow (and test version to try to find bugs in the implementation).
It looks like the protocol could simply be updated to put the version
into one (or more) of the three reserved bytes in the handshake
headers, so perhaps doing this before they get used for something else
would be a good first step?
James
^ permalink raw reply
* Re: [PATCH v2] Add icmp_echo_ignore_all support for ICMPv6
From: David Miller @ 2018-08-13 15:42 UTC (permalink / raw)
To: virgile; +Cc: kuznet, yoshfuji, netdev, linux-kernel
In-Reply-To: <20180810154815.GA18171@_>
From: Virgile Jarry <virgile@acceis.fr>
Date: Fri, 10 Aug 2018 17:48:15 +0200
> Preventing the kernel from responding to ICMP Echo Requests messages
> can be useful in several ways. The sysctl parameter
> 'icmp_echo_ignore_all' can be used to prevent the kernel from
> responding to IPv4 ICMP echo requests. For IPv6 pings, such
> a sysctl kernel parameter did not exist.
>
> Add the ability to prevent the kernel from responding to IPv6
> ICMP echo requests through the use of the following sysctl
> parameter : /proc/sys/net/ipv6/icmp/echo_ignore_all.
> Update the documentation to reflect this change.
>
> Signed-off-by: Virgile Jarry <virgile@acceis.fr>
> ---
> Changes in v2:
> - Added support for sysctl interface (and not just /proc filesystem)
Applied.
^ permalink raw reply
* Re: [V9fs-developer] [PATCH 2/2] 9p: Add refcount to p9_req_t
From: Dmitry Vyukov @ 2018-08-13 13:04 UTC (permalink / raw)
To: Dominique Martinet
Cc: piaojun, Tomas Bortoli, Eric Van Hensbergen, Ron Minnich,
Latchesar Ionkov, Dominique Martinet, netdev, LKML, syzkaller,
v9fs-developer, David Miller
In-Reply-To: <20180813014815.GB6777@nautica>
On Mon, Aug 13, 2018 at 3:48 AM, Dominique Martinet
<asmadeus@codewreck.org> wrote:
> piaojun wrote on Mon, Aug 13, 2018:
>> Could you help paste the reason of the crash bug to help others
>> understand more clearly? And I have another question below.
>
> The problem for tcp (but other transports have a similar problem) is
> that with a malicious server like syzkaller they can try to submit
> replies before the request came in.
>
> This leads in the writer thread trying to write a buffer that has
> already been freed, and if memory has been reused could potentially leak
> some information.
>
> Now, with the previous patches this is based on this would be a slab and
> the likeliness of it being sensitive information is rather low (it would
> likely be some other packet being sent twice, or a mix and match of two
> packets that would have been sent anyway), but it would nevertheless be
> a use after free.
>
>
> There is a second advantage to this reference counting, that is now we
> have this system we will be able to implement flush asynchronously.
> This will remove the need for the 'goto again' in p9_client_rpc which
> was making 9p threads unkillable in practice if the server would not
> reply to the flush requests.
Fixing unkillalble task would be nice. Don't know how much they are of
a problem in real life, but fixing them would allow fuzzer to find
other, potentially more critical bugs in 9p. These "task hung" crashes
are quite unpleasant for the fuzzer.
Thanks for all recent 9p work, Tomas!
> Even if the server replies I've always found myself needing to hit ^C
> multiple times to exit a process doing I/Os and I think fixing that
> behaviour will make 9p more comfortable to use.
>
>
>> > diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
>> > index 20f46f13fe83..686e24e355d0 100644
>> > --- a/net/9p/trans_fd.c
>> > +++ b/net/9p/trans_fd.c
>> > @@ -132,6 +132,7 @@ struct p9_conn {
>> > struct list_head req_list;
>> > struct list_head unsent_req_list;
>> > struct p9_req_t *req;
>> > + struct p9_req_t *wreq;
>>
>> Why adding a wreq for write work? And I wonder we should rename req to
>> rreq?
>
> We need to store a pointer to the request for the write thread because
> we need to put the reference to it when we're done writing its content.
>
> Previously, the worker would only store the write buffer there but
> that's not enough to figure what request to dereference.
>
>
> I personally don't think renaming req to rreq would bring much but it
> could be done in another patch if you think that'd be helpful; I think
> it shouldn't be done here at least to make the patch more readable.
>
> --
> Dominique
>
> --
> You received this message because you are subscribed to the Google Groups "syzkaller" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
^ permalink raw reply
* Re: [PATCH 0/2] net: nixge: Minor cleanups
From: David Miller @ 2018-08-13 15:49 UTC (permalink / raw)
To: mdf; +Cc: keescook, netdev, linux-kernel, alex.williams
In-Reply-To: <20180811011941.6187-1-mdf@kernel.org>
From: Moritz Fischer <mdf@kernel.org>
Date: Fri, 10 Aug 2018 18:19:39 -0700
> in preparation of my 64-bit support series, here's some
> minor cleanup in preparation that gets rid of unneccesary
> accesses to the descriptor application fields.
>
> I've confirmed that the hardware does not access the fields
> in all our configurations.
Series applied, thank you.
^ permalink raw reply
* Re: [PATCH v2] xen-netfront: fix warn message as irq device name has '/'
From: David Miller @ 2018-08-13 15:52 UTC (permalink / raw)
To: xiliang; +Cc: netdev, xen-devel, jgross, boris.ostrovsky, linux-kernel
In-Reply-To: <20180811152137.3627-1-xiliang@redhat.com>
From: Xiao Liang <xiliang@redhat.com>
Date: Sat, 11 Aug 2018 23:21:37 +0800
> There is a call trace generated after commit 2d408c0d4574b01b9ed45e02516888bf925e11a9(
> xen-netfront: fix queue name setting). There is no 'device/vif/xx-q0-tx' file found
> under /proc/irq/xx/.
>
> This patch only picks up device type and id as its name.
This adds a compile warning:
drivers/net/xen-netfront.c: In function ‘xennet_init_queue’:
drivers/net/xen-netfront.c:1614:2: warning: ignoring return value of ‘kstrtoint’, declared with attribute warn_unused_result [-Wunused-result]
kstrtoint(strrchr(queue->info->xbdev->nodename, '/') + 1, 10, &devid);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
^ permalink raw reply
* Re: [PATCH v1 0/3] WireGuard: Secure Network Tunnel
From: Willy Tarreau @ 2018-08-13 15:53 UTC (permalink / raw)
To: James Bottomley
Cc: Jason A. Donenfeld, linux-kernel, netdev, davem, linux-crypto
In-Reply-To: <1534174811.7872.3.camel@HansenPartnership.com>
On Mon, Aug 13, 2018 at 08:40:11AM -0700, James Bottomley wrote:
> Could we please build planning for this crypto failure day into
> wireguard now rather than have to do it later? It doesn't need to be
> full cipher agility, it just needs to be the ability to handle multiple
> protocol versions ... two should do it because that gives a template to
> follow (and test version to try to find bugs in the implementation).
It's also what provides a *real* upgrade path to future versions :
before deploying you need something which works, and the only way to
get something working at a large scale is to have early adopters. Those
willing to deploy a beta version will not do it if it requires to lose
all their users and possibly to make rollbacks impossible. At least for
this it's important to support an optionnal new version on top of the
existing one (i.e. prod + beta together).
Cheers,
Willy
^ permalink raw reply
* Re: [PATCH net-next 1/9] net: hns3: Add support for serdes loopback selftest
From: David Miller @ 2018-08-13 15:54 UTC (permalink / raw)
To: salil.mehta
Cc: yisen.zhuang, lipeng321, mehta.salil.lnk, netdev, linux-kernel,
linuxarm, linyunsheng
In-Reply-To: <20180812094738.14852-2-salil.mehta@huawei.com>
From: Salil Mehta <salil.mehta@huawei.com>
Date: Sun, 12 Aug 2018 10:47:30 +0100
> -#define HNS3_SELF_TEST_TPYE_NUM 1
> +#define HNS3_SELF_TEST_TPYE_NUM 2
Is this supposed to be the number of self test "types"? If so, this CPP
macro should be named "HNS3_SELF_TEST_TYPE_NUM".
> +
> + count ++;
^
Please remove that unnecessary space.
> + } while (++i < HCLGE_SERDES_RETRY_NUM &&
^^
Only need one space there, not two.
^ permalink raw reply
* Re: [PATCH net-next 5/9] net: hns3: Fix for vf vlan delete failed problem
From: David Miller @ 2018-08-13 15:56 UTC (permalink / raw)
To: salil.mehta
Cc: yisen.zhuang, lipeng321, mehta.salil.lnk, netdev, linux-kernel,
linuxarm, linyunsheng
In-Reply-To: <20180812094738.14852-6-salil.mehta@huawei.com>
From: Salil Mehta <salil.mehta@huawei.com>
Date: Sun, 12 Aug 2018 10:47:34 +0100
> Fixes: 9dba194574e3 ("{topost} net: hns3: fix for vlan table problem")
This commit ID doesn't exist.
Also, I really don't think the string "{topost}" would be in the commit
header line.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox