* Re: [PATCH] mwifiex: Fix skipped vendor specific IEs
From: Kalle Valo @ 2018-07-31 6:58 UTC (permalink / raw)
To: roman.stratiienko
Cc: akarwar, nishants, linux-wireless, netdev, linux-kernel,
sergei.shtylyov, Roman Stratiienko
In-Reply-To: <1532432847-2885-1-git-send-email-roman.stratiienko@globallogic.com>
roman.stratiienko@globallogic.com wrote:
> From: Roman Stratiienko <roman.stratiienko@globallogic.com>
>
> Mwifiex firmware inserts only Microsoft information element
> Allow other vendor specific IEs to pass from userspace
>
> Signed-off-by: Roman Stratiienko <roman.stratiienko@globallogic.com>
When you submit a new version fo a patch please always add a version number to
the title and include a changelog:
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches#patch_version_missing
But this is for the future, no need to resend this patch.
--
https://patchwork.kernel.org/patch/10541923/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply
* Re: [PATCH] ipvs: don't show negative times in ip_vs_conn
From: Eric Dumazet @ 2018-07-31 5:10 UTC (permalink / raw)
To: Matteo Croce
Cc: Wensong Zhang, Simon Horman, Julian Anastasov, lvs-devel, netdev,
Jozsef Kadlecsik, Pablo Neira Ayuso, Florian Westphal,
netfilter-devel
In-Reply-To: <CAGnkfhzxz715p23PH+QgK2zduhv95O85wa92ktBRtw5xy1sZ8w@mail.gmail.com>
On 07/30/2018 10:49 AM, Matteo Croce wrote:
> On Fri, Jul 20, 2018 at 4:19 PM Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>
>>
>>
>> On 07/20/2018 08:19 AM, Matteo Croce wrote:
>>> Since commit 500462a9de65 ("timers: Switch to a non-cascading wheel"),
>>> timers duration can last even 12.5% more than the scheduled interval.
>>>
>>> Signed-off-by: Matteo Croce <mcroce@redhat.com>
>>> ---
>>> net/netfilter/ipvs/ip_vs_conn.c | 22 ++++++++++++++--------
>>> 1 file changed, 14 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/net/netfilter/ipvs/ip_vs_conn.c b/net/netfilter/ipvs/ip_vs_conn.c
>>> index 99e0aa350dc5..c78c48a6d53f 100644
>>> --- a/net/netfilter/ipvs/ip_vs_conn.c
>>> +++ b/net/netfilter/ipvs/ip_vs_conn.c
>>> @@ -1066,6 +1066,12 @@ static void ip_vs_conn_seq_stop(struct seq_file *seq, void *v)
>>> rcu_read_unlock();
>>> }
>>>
>>> +static unsigned int time_left(unsigned long time)
>>> +{
>>> + return time_is_after_jiffies(time) ?
>>> + jiffies_to_msecs(time - jiffies) / 1000 : 0;
>>> +}
>>
>>
>> I would suggest adding jiffies_delta_to_msecs(), because we will need elsewhere,
>> like in inet_sk_diag_fill()
>>
>>
>> diff --git a/include/linux/jiffies.h b/include/linux/jiffies.h
>> index a27cf66523279c1a5d4aaa0d0087f1e9d48d170f..fa928242567db30769839ac8738be5dc58e372ab 100644
>> --- a/include/linux/jiffies.h
>> +++ b/include/linux/jiffies.h
>> @@ -447,6 +447,11 @@ static inline clock_t jiffies_delta_to_clock_t(long delta)
>> return jiffies_to_clock_t(max(0L, delta));
>> }
>>
>> +static inline unsigned int jiffies_delta_to_msecs(long delta)
>> +{
>> + return jiffies_to_msecs(max(0L, delta));
>> +}
>> +
>> extern unsigned long clock_t_to_jiffies(unsigned long x);
>> extern u64 jiffies_64_to_clock_t(u64 x);
>> extern u64 nsec_to_clock_t(u64 x);
>>
>
> Hi Eric,
> What about a function which returns directly the delta from a
> timestamp, and 0 if elapsed?
> So we can rely on time_is_after_jiffies() for overflows, it should be
> less error prone.
>
> static unsigned int jiffies_delta_to_msecs(unsigned long time)
> {
> return time_is_after_jiffies(time) ?
> jiffies_to_msecs(time - jiffies) / 1000 : 0;
> }
>
I dunno, I suggested jiffies_delta_to_msecs(long delta) because it is built
on the same model than jiffies_delta_to_clock_t(long delta)
And it really does what you want.
Remember that jiffies can change, so what you wrote is buggy/racy.
if (time_is_after_jiffies(time)) {
... jiffies is updated, and now (time - jiffies) might be negative,
since compiler reads jiffies a second time (jiffies is a volatile)
return jiffies_to_msecs(time - jiffies)/ 1000;
}
^ permalink raw reply
* Re: [RFC PATCH net-next v2 06/17] ethtool: support for netlink notifications
From: Jiri Pirko @ 2018-07-31 6:46 UTC (permalink / raw)
To: Michal Kubecek
Cc: netdev, linux-kernel, David Miller, Florian Fainelli,
Roopa Prabhu, Jakub Kicinski, John W. Linville
In-Reply-To: <20180730170121.e6k5slbhdamclnt6@unicorn.suse.cz>
Mon, Jul 30, 2018 at 07:01:21PM CEST, mkubecek@suse.cz wrote:
>On Mon, Jul 30, 2018 at 03:16:55PM +0200, Jiri Pirko wrote:
>> Mon, Jul 30, 2018 at 02:53:12PM CEST, mkubecek@suse.cz wrote:
>>
>> [...]
>>
>> >diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> >index c1295c7a452e..c4b0c575d57e 100644
>> >--- a/include/linux/netdevice.h
>> >+++ b/include/linux/netdevice.h
>> >@@ -2444,6 +2444,7 @@ enum netdev_cmd {
>> > NETDEV_CVLAN_FILTER_DROP_INFO,
>> > NETDEV_SVLAN_FILTER_PUSH_INFO,
>> > NETDEV_SVLAN_FILTER_DROP_INFO,
>> >+ NETDEV_ETHTOOL,
>>
>> I don't understand why this goes through netdev notifier. What's the
>> reason?
>
>To allow triggering a notification from other code (ethtool ioctl or
>e.g. netdev_features_change()) when netlink interface is built as a
>module. If it's a (performance?) problem, an alternative could be having
>a global pointer which would be either null or point to ethtool_notify()
>depending on whether the module is loaded (and ready).
>
>(Which made me realize I forgot to handle a race between module
>unloading and processing a notification.)
>
>Another question is if we really need the option to build the netlink
>interface as a module. I must admit my main motivation to have it as
>a module is that it makes testing and debugging easier.
Yeah. It is very core thing. I think it does not have to be a module.
^ permalink raw reply
* Re: WARNING: ODEBUG bug in vsock_stream_connect
From: syzbot @ 2018-07-31 6:45 UTC (permalink / raw)
To: davem, dvlasenk, jhansen, linux-kernel, netdev, stefanha,
syzkaller-bugs, viro, xiyou.wangcong
In-Reply-To: <000000000000ea418a0572384417@google.com>
syzbot has found a reproducer for the following crash on:
HEAD commit: 527838d470e3 Merge branch 'x86-urgent-for-linus' of git://..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=17abacf0400000
kernel config: https://syzkaller.appspot.com/x/.config?x=2dc0cd7c2eefb46f
dashboard link: https://syzkaller.appspot.com/bug?extid=8a9b1bd330476a4f3db6
compiler: gcc (GCC) 8.0.1 20180413 (experimental)
syzkaller repro:https://syzkaller.appspot.com/x/repro.syz?x=16c0cc54400000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1747bb62400000
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+8a9b1bd330476a4f3db6@syzkaller.appspotmail.com
------------[ cut here ]------------
ODEBUG: init active (active state 0) object type: timer_list hint:
delayed_work_timer_fn+0x0/0x90 kernel/workqueue.c:1414
WARNING: CPU: 0 PID: 17364 at lib/debugobjects.c:329
debug_print_object+0x16a/0x210 lib/debugobjects.c:326
Kernel panic - not syncing: panic_on_warn set ...
CPU: 0 PID: 17364 Comm: syz-executor746 Not tainted 4.18.0-rc7+ #170
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
panic+0x238/0x4e7 kernel/panic.c:184
__warn.cold.8+0x163/0x1ba kernel/panic.c:536
report_bug+0x252/0x2d0 lib/bug.c:186
fixup_bug arch/x86/kernel/traps.c:178 [inline]
do_error_trap+0x1fc/0x4d0 arch/x86/kernel/traps.c:296
do_invalid_op+0x1b/0x20 arch/x86/kernel/traps.c:316
invalid_op+0x14/0x20 arch/x86/entry/entry_64.S:992
RIP: 0010:debug_print_object+0x16a/0x210 lib/debugobjects.c:326
Code: 3a 87 48 89 fa 48 c1 ea 03 80 3c 02 00 0f 85 92 00 00 00 48 8b 14 dd
20 74 3a 87 4c 89 f6 48 c7 c7 c0 69 3a 87 e8 c6 b0 e6 fd <0f> 0b 83 05 a9
e6 29 05 01 48 83 c4 18 5b 41 5c 41 5d 41 5e 41 5f
RSP: 0018:ffff8801c535f658 EFLAGS: 00010082
RAX: 0000000000000000 RBX: 0000000000000003 RCX: 0000000000000000
RDX: 0000000000000000 RSI: ffffffff81632481 RDI: 0000000000000001
RBP: ffff8801c535f698 R08: ffff8801b5638440 R09: ffffed003b603ec2
R10: ffffed003b603ec2 R11: ffff8801db01f617 R12: 0000000000000001
R13: ffffffff87f9c9e0 R14: ffffffff873a6fc0 R15: ffffffff816918a0
__debug_object_init+0x8e9/0x12e0 lib/debugobjects.c:403
debug_object_init+0x16/0x20 lib/debugobjects.c:429
debug_timer_init kernel/time/timer.c:704 [inline]
debug_init kernel/time/timer.c:757 [inline]
init_timer_key+0xa9/0x490 kernel/time/timer.c:806
vsock_stream_connect+0xc7a/0xfc0 net/vmw_vsock/af_vsock.c:1224
__sys_connect+0x37d/0x4c0 net/socket.c:1673
__do_sys_connect net/socket.c:1684 [inline]
__se_sys_connect net/socket.c:1681 [inline]
__x64_sys_connect+0x73/0xb0 net/socket.c:1681
do_syscall_64+0x1b9/0x820 arch/x86/entry/common.c:290
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x445d59
Code: e8 ac e7 ff ff 48 83 c4 18 c3 0f 1f 80 00 00 00 00 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 fb 0d fc ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007fe17fb16da8 EFLAGS: 00000246 ORIG_RAX: 000000000000002a
RAX: ffffffffffffffda RBX: 00000000006dac38 RCX: 0000000000445d59
RDX: 0000000000000010 RSI: 0000000020000100 RDI: 0000000000000004
RBP: 00000000006dac30 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00000000006dac3c
R13: 6b636f73762d7473 R14: 6f68762f7665642f R15: 0000000000000001
======================================================
WARNING: possible circular locking dependency detected
4.18.0-rc7+ #170 Not tainted
------------------------------------------------------
syz-executor746/17364 is trying to acquire lock:
(____ptrval____) ((console_sem).lock){-...}, at: down_trylock+0x13/0x70
kernel/locking/semaphore.c:136
but task is already holding lock:
(____ptrval____) (&obj_hash[i].lock){-.-.}, at:
__debug_object_init+0x127/0x12e0 lib/debugobjects.c:381
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #3 (&obj_hash[i].lock){-.-.}:
__raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline]
_raw_spin_lock_irqsave+0x96/0xc0 kernel/locking/spinlock.c:152
__debug_object_init+0x127/0x12e0 lib/debugobjects.c:381
debug_object_init+0x16/0x20 lib/debugobjects.c:429
debug_hrtimer_init kernel/time/hrtimer.c:410 [inline]
debug_init kernel/time/hrtimer.c:458 [inline]
hrtimer_init+0x97/0x480 kernel/time/hrtimer.c:1308
init_dl_task_timer+0x1b/0x50 kernel/sched/deadline.c:1056
__sched_fork+0x2ae/0x590 kernel/sched/core.c:2185
init_idle+0x75/0x7a0 kernel/sched/core.c:5405
sched_init+0xbf3/0xd2c kernel/sched/core.c:6103
start_kernel+0x47d/0x949 init/main.c:602
x86_64_start_reservations+0x29/0x2b arch/x86/kernel/head64.c:452
x86_64_start_kernel+0x76/0x79 arch/x86/kernel/head64.c:433
secondary_startup_64+0xa5/0xb0 arch/x86/kernel/head_64.S:242
-> #2 (&rq->lock){-.-.}:
__raw_spin_lock include/linux/spinlock_api_smp.h:142 [inline]
_raw_spin_lock+0x2a/0x40 kernel/locking/spinlock.c:144
rq_lock kernel/sched/sched.h:1812 [inline]
task_fork_fair+0x93/0x680 kernel/sched/fair.c:9952
sched_fork+0x446/0xb40 kernel/sched/core.c:2381
copy_process.part.39+0x1bf5/0x70b0 kernel/fork.c:1796
copy_process kernel/fork.c:1639 [inline]
_do_fork+0x291/0x12a0 kernel/fork.c:2122
kernel_thread+0x34/0x40 kernel/fork.c:2181
rest_init+0x22/0xe4 init/main.c:408
start_kernel+0x90e/0x949 init/main.c:738
x86_64_start_reservations+0x29/0x2b arch/x86/kernel/head64.c:452
x86_64_start_kernel+0x76/0x79 arch/x86/kernel/head64.c:433
secondary_startup_64+0xa5/0xb0 arch/x86/kernel/head_64.S:242
-> #1 (&p->pi_lock){-.-.}:
__raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline]
_raw_spin_lock_irqsave+0x96/0xc0 kernel/locking/spinlock.c:152
try_to_wake_up+0xd2/0x12a0 kernel/sched/core.c:1985
wake_up_process+0x10/0x20 kernel/sched/core.c:2148
__up.isra.1+0x1c0/0x2a0 kernel/locking/semaphore.c:262
up+0x13c/0x1c0 kernel/locking/semaphore.c:187
__up_console_sem+0xbe/0x1b0 kernel/printk/printk.c:242
console_unlock+0x7a2/0x10b0 kernel/printk/printk.c:2411
vprintk_emit+0x6c6/0xdf0 kernel/printk/printk.c:1907
vprintk_default+0x28/0x30 kernel/printk/printk.c:1948
vprintk_func+0x7a/0xe7 kernel/printk/printk_safe.c:382
printk+0xa7/0xcf kernel/printk/printk.c:1981
load_umh+0x51/0xbd net/bpfilter/bpfilter_kern.c:98
do_one_initcall+0x127/0x913 init/main.c:884
do_initcall_level init/main.c:952 [inline]
do_initcalls init/main.c:960 [inline]
do_basic_setup init/main.c:978 [inline]
kernel_init_freeable+0x49b/0x58e init/main.c:1135
kernel_init+0x11/0x1b3 init/main.c:1061
ret_from_fork+0x3a/0x50 arch/x86/entry/entry_64.S:412
-> #0 ((console_sem).lock){-...}:
lock_acquire+0x1e4/0x540 kernel/locking/lockdep.c:3924
__raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline]
_raw_spin_lock_irqsave+0x96/0xc0 kernel/locking/spinlock.c:152
down_trylock+0x13/0x70 kernel/locking/semaphore.c:136
__down_trylock_console_sem+0xae/0x200 kernel/printk/printk.c:225
console_trylock+0x15/0xa0 kernel/printk/printk.c:2230
console_trylock_spinning kernel/printk/printk.c:1643 [inline]
vprintk_emit+0x6ad/0xdf0 kernel/printk/printk.c:1906
vprintk_default+0x28/0x30 kernel/printk/printk.c:1948
vprintk_func+0x7a/0xe7 kernel/printk/printk_safe.c:382
printk+0xa7/0xcf kernel/printk/printk.c:1981
__warn_printk+0x8c/0xe0 kernel/panic.c:590
debug_print_object+0x16a/0x210 lib/debugobjects.c:326
__debug_object_init+0x8e9/0x12e0 lib/debugobjects.c:403
debug_object_init+0x16/0x20 lib/debugobjects.c:429
debug_timer_init kernel/time/timer.c:704 [inline]
debug_init kernel/time/timer.c:757 [inline]
init_timer_key+0xa9/0x490 kernel/time/timer.c:806
vsock_stream_connect+0xc7a/0xfc0 net/vmw_vsock/af_vsock.c:1224
__sys_connect+0x37d/0x4c0 net/socket.c:1673
__do_sys_connect net/socket.c:1684 [inline]
__se_sys_connect net/socket.c:1681 [inline]
__x64_sys_connect+0x73/0xb0 net/socket.c:1681
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:
(console_sem).lock --> &rq->lock --> &obj_hash[i].lock
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(&obj_hash[i].lock);
lock(&rq->lock);
lock(&obj_hash[i].lock);
lock((console_sem).lock);
*** DEADLOCK ***
2 locks held by syz-executor746/17364:
#0: (____ptrval____) (sk_lock-AF_VSOCK){+.+.}, at: lock_sock
include/net/sock.h:1474 [inline]
#0: (____ptrval____) (sk_lock-AF_VSOCK){+.+.}, at:
vsock_stream_connect+0x1e3/0xfc0 net/vmw_vsock/af_vsock.c:1152
#1: (____ptrval____) (&obj_hash[i].lock){-.-.}, at:
__debug_object_init+0x127/0x12e0 lib/debugobjects.c:381
stack backtrace:
CPU: 0 PID: 17364 Comm: syz-executor746 Not tainted 4.18.0-rc7+ #170
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
__raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline]
_raw_spin_lock_irqsave+0x96/0xc0 kernel/locking/spinlock.c:152
down_trylock+0x13/0x70 kernel/locking/semaphore.c:136
__down_trylock_console_sem+0xae/0x200 kernel/printk/printk.c:225
console_trylock+0x15/0xa0 kernel/printk/printk.c:2230
console_trylock_spinning kernel/printk/printk.c:1643 [inline]
vprintk_emit+0x6ad/0xdf0 kernel/printk/printk.c:1906
vprintk_default+0x28/0x30 kernel/printk/printk.c:1948
vprintk_func+0x7a/0xe7 kernel/printk/printk_safe.c:382
printk+0xa7/0xcf kernel/printk/printk.c:1981
__warn_printk+0x8c/0xe0 kernel/panic.c:590
debug_print_object+0x16a/0x210 lib/debugobjects.c:326
__debug_object_init+0x8e9/0x12e0 lib/debugobjects.c:403
debug_object_init+0x16/0x20 lib/debugobjects.c:429
debug_timer_init kernel/time/timer.c:704 [inline]
debug_init kernel/time/timer.c:757 [inline]
init_timer_key+0xa9/0x490 kernel/time/timer.c:806
vsock_stream_connect+0xc7a/0xfc0 net/vmw_vsock/af_vsock.c:1224
__sys_connect+0x37d/0x4c0 net/socket.c:1673
__do_sys_connect net/socket.c:1684 [inline]
__se_sys_connect net/socket.c:1681 [inline]
__x64_sys_connect+0x73/0xb0 net/socket.c:1681
do_syscall_64+0x1b9/0x820 arch/x86/entry/common.c:290
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x445d59
Code: e8 ac e7 ff ff 48 83 c4 18 c3 0f 1f 80 00 00 00 00 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 fb 0d fc ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007fe17fb16da8 EFLAGS: 00000246 ORIG_RAX: 000000000000002a
RAX: ffffffffffffffda RBX: 00000000006dac38 RCX: 0000000000445d59
RDX: 0000000000000010 RSI: 0000000020000100 RDI: 0000000000000004
RBP: 00000000006dac30 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00000000006dac3c
R13: 6b636f73762d7473 R14: 6f68762f7665642f R15: 0000000000000001
Shutting down cpus with NMI
Dumping ftrace buffer:
(ftrace buffer empty)
Kernel Offset: disabled
Rebooting in 86400 seconds..
^ permalink raw reply
* Re: [PATCH net-next] net: phy: mscc: the extended page access register is 16 bits
From: Quentin Schulz @ 2018-07-31 6:43 UTC (permalink / raw)
To: Florian Fainelli
Cc: andrew, davem, netdev, alexandre.belloni, linux-kernel,
thomas.petazzoni
In-Reply-To: <4dd7d451-a1e3-cab8-dc5a-6666c5d29089@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 759 bytes --]
Hi Florian,
On Mon, Jul 30, 2018 at 09:53:31AM -0700, Florian Fainelli wrote:
> On 07/30/2018 05:53 AM, Quentin Schulz wrote:
> > The Extended Page Access is a 16-bit register, so change the page
> > parameter of vsc85xx_phy_page_set to a u16.
> >
> > Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
>
> Since you targeted net-next for this patch, I am assuming this is not
> yet a problem, but would soon be one with the changes you plan on
> introducing?
>
Yes indeed. When I get the quad-port PHY things to go as planned, then
I'll need sometimes to change to a page that is above the size of a u16.
For now, all the pages are accessed with a number that is lower than the
size of a u16, so it isn't a problem.
Quentin
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 2/5] rhashtable: don't hold lock on first table throughout insertion.
From: NeilBrown @ 2018-07-31 5:04 UTC (permalink / raw)
To: paulmck; +Cc: Herbert Xu, Thomas Graf, netdev, linux-kernel
In-Reply-To: <20180731041425.GI24813@linux.vnet.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 5679 bytes --]
On Mon, Jul 30 2018, Paul E. McKenney wrote:
> On Tue, Jul 31, 2018 at 10:45:45AM +1000, NeilBrown wrote:
>> On Fri, Jul 27 2018, Paul E. McKenney wrote:
>>
>> > On Thu, Jul 26, 2018 at 08:18:15PM -0700, Paul E. McKenney wrote:
>> >> On Fri, Jul 27, 2018 at 11:04:37AM +1000, NeilBrown wrote:
>> >> > On Wed, Jul 25 2018, Paul E. McKenney wrote:
>> >> > >>
>> >> > >> Looks good ... except ... naming is hard.
>> >> > >>
>> >> > >> is_after_call_rcu_init() asserts where in the lifecycle we are,
>> >> > >> is_after_call_rcu() tests where in the lifecycle we are.
>> >> > >>
>> >> > >> The names are similar but the purpose is quite different.
>> >> > >> Maybe s/is_after_call_rcu_init/call_rcu_init/ ??
>> >> > >
>> >> > > How about rcu_head_init() and rcu_head_after_call_rcu()?
>> >>
>> >> Very well, I will pull this change in on my next rebase.
>> >
>> > Like this?
>>
>> Hard to say - unwinding white-space damage in my head is too challenging
>> when newlines have been deleted :-(
>
> What??? Don't you like block-structured code?
>
> All kidding aside, how about the following more conventionally formatted
> version?
Wow - it's like I just got new glasses!
Yes - nice an clear and now flaws to be found. Thanks a lot.
NeilBrown
>
> Thanx, Paul
>
> ------------------------------------------------------------------------
>
> commit e3408141ed7d702995b2fdc94703af88aadd226b
> Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Date: Tue Jul 24 15:28:09 2018 -0700
>
> rcu: Provide functions for determining if call_rcu() has been invoked
>
> This commit adds rcu_head_init() and rcu_head_after_call_rcu() functions
> to help RCU users detect when another CPU has passed the specified
> rcu_head structure and function to call_rcu(). The rcu_head_init()
> should be invoked before making the structure visible to RCU readers,
> and then the rcu_head_after_call_rcu() may be invoked from within
> an RCU read-side critical section on an rcu_head structure that
> was obtained during a traversal of the data structure in question.
> The rcu_head_after_call_rcu() function will return true if the rcu_head
> structure has already been passed (with the specified function) to
> call_rcu(), otherwise it will return false.
>
> If rcu_head_init() has not been invoked on the rcu_head structure
> or if the rcu_head (AKA callback) has already been invoked, then
> rcu_head_after_call_rcu() will do WARN_ON_ONCE().
>
> Reported-by: NeilBrown <neilb@suse.com>
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> [ paulmck: Apply neilb naming feedback. ]
>
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index e4f821165d0b..4db8bcacc51a 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -857,6 +857,46 @@ static inline notrace void rcu_read_unlock_sched_notrace(void)
> #endif /* #else #ifdef CONFIG_ARCH_WEAK_RELEASE_ACQUIRE */
>
>
> +/* Has the specified rcu_head structure been handed to call_rcu()? */
> +
> +/*
> + * rcu_head_init - Initialize rcu_head for rcu_head_after_call_rcu()
> + * @rhp: The rcu_head structure to initialize.
> + *
> + * If you intend to invoke rcu_head_after_call_rcu() to test whether a
> + * given rcu_head structure has already been passed to call_rcu(), then
> + * you must also invoke this rcu_head_init() function on it just after
> + * allocating that structure. Calls to this function must not race with
> + * calls to call_rcu(), rcu_head_after_call_rcu(), or callback invocation.
> + */
> +static inline void rcu_head_init(struct rcu_head *rhp)
> +{
> + rhp->func = (rcu_callback_t)~0L;
> +}
> +
> +/*
> + * rcu_head_after_call_rcu - Has this rcu_head been passed to call_rcu()?
> + * @rhp: The rcu_head structure to test.
> + * @func: The function passed to call_rcu() along with @rhp.
> + *
> + * Returns @true if the @rhp has been passed to call_rcu() with @func,
> + * and @false otherwise. Emits a warning in any other case, including
> + * the case where @rhp has already been invoked after a grace period.
> + * Calls to this function must not race with callback invocation. One way
> + * to avoid such races is to enclose the call to rcu_head_after_call_rcu()
> + * in an RCU read-side critical section that includes a read-side fetch
> + * of the pointer to the structure containing @rhp.
> + */
> +static inline bool
> +rcu_head_after_call_rcu(struct rcu_head *rhp, rcu_callback_t f)
> +{
> + if (READ_ONCE(rhp->func) == f)
> + return true;
> + WARN_ON_ONCE(READ_ONCE(rhp->func) != (rcu_callback_t)~0L);
> + return false;
> +}
> +
> +
> /* Transitional pre-consolidation compatibility definitions. */
>
> static inline void synchronize_rcu_bh(void)
> diff --git a/kernel/rcu/rcu.h b/kernel/rcu/rcu.h
> index 5dec94509a7e..4c56c1d98fb3 100644
> --- a/kernel/rcu/rcu.h
> +++ b/kernel/rcu/rcu.h
> @@ -224,6 +224,7 @@ void kfree(const void *);
> */
> static inline bool __rcu_reclaim(const char *rn, struct rcu_head *head)
> {
> + rcu_callback_t f;
> unsigned long offset = (unsigned long)head->func;
>
> rcu_lock_acquire(&rcu_callback_map);
> @@ -234,7 +235,9 @@ static inline bool __rcu_reclaim(const char *rn, struct rcu_head *head)
> return true;
> } else {
> RCU_TRACE(trace_rcu_invoke_callback(rn, head);)
> - head->func(head);
> + f = head->func;
> + WRITE_ONCE(head->func, (rcu_callback_t)0L);
> + f(head);
> rcu_lock_release(&rcu_callback_map);
> return false;
> }
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply
* WARNING: ODEBUG bug in rfcomm_dlc_free
From: syzbot @ 2018-07-31 6:34 UTC (permalink / raw)
To: davem, johan.hedberg, keescook, linux-bluetooth, linux-kernel,
marcel, netdev, syzkaller-bugs
Hello,
syzbot found the following crash on:
HEAD commit: 527838d470e3 Merge branch 'x86-urgent-for-linus' of git://..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=17ebce52400000
kernel config: https://syzkaller.appspot.com/x/.config?x=2dc0cd7c2eefb46f
dashboard link: https://syzkaller.appspot.com/bug?extid=1f6d4ad860c650c2f215
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+1f6d4ad860c650c2f215@syzkaller.appspotmail.com
bond0: Error: Device is in use and cannot be enslaved
Unknown ioctl -1070573808
------------[ cut here ]------------
ODEBUG: free active (active state 0) object type: timer_list hint:
rfcomm_dlc_timeout+0x0/0x70 include/linux/list.h:64
WARNING: CPU: 0 PID: 1696 at lib/debugobjects.c:329
debug_print_object+0x16a/0x210 lib/debugobjects.c:326
Kernel panic - not syncing: panic_on_warn set ...
CPU: 0 PID: 1696 Comm: syz-executor4 Not tainted 4.18.0-rc7+ #170
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
panic+0x238/0x4e7 kernel/panic.c:184
__warn.cold.8+0x163/0x1ba kernel/panic.c:536
report_bug+0x252/0x2d0 lib/bug.c:186
fixup_bug arch/x86/kernel/traps.c:178 [inline]
do_error_trap+0x1fc/0x4d0 arch/x86/kernel/traps.c:296
do_invalid_op+0x1b/0x20 arch/x86/kernel/traps.c:316
invalid_op+0x14/0x20 arch/x86/entry/entry_64.S:992
RIP: 0010:debug_print_object+0x16a/0x210 lib/debugobjects.c:326
Code: 3a 87 48 89 fa 48 c1 ea 03 80 3c 02 00 0f 85 92 00 00 00 48 8b 14 dd
20 74 3a 87 4c 89 f6 48 c7 c7 c0 69 3a 87 e8 c6 b0 e6 fd <0f> 0b 83 05 a9
e6 29 05 01 48 83 c4 18 5b 41 5c 41 5d 41 5e 41 5f
RSP: 0018:ffff880191937648 EFLAGS: 00010086
RAX: 0000000000000000 RBX: 0000000000000003 RCX: ffffc90007a4a000
RDX: 00000000000292e2 RSI: ffffffff81632481 RDI: 0000000000000001
RBP: ffff880191937688 R08: ffff8801b67b6340 R09: ffffed003b603ec2
R10: ffffed003b603ec2 R11: ffff8801db01f617 R12: 0000000000000001
R13: ffffffff87f9c9e0 R14: ffffffff873a6e60 R15: ffffffff816918a0
__debug_check_no_obj_freed lib/debugobjects.c:783 [inline]
debug_check_no_obj_freed+0x3b2/0x595 lib/debugobjects.c:815
kfree+0xc7/0x260 mm/slab.c:3812
rfcomm_dlc_free+0x1e/0x30 net/bluetooth/rfcomm/core.c:328
rfcomm_dlc_put include/net/bluetooth/rfcomm.h:258 [inline]
__rfcomm_create_dev net/bluetooth/rfcomm/tty.c:417 [inline]
rfcomm_create_dev net/bluetooth/rfcomm/tty.c:486 [inline]
rfcomm_dev_ioctl+0x1dfe/0x2240 net/bluetooth/rfcomm/tty.c:588
rfcomm_sock_ioctl+0x89/0xb0 net/bluetooth/rfcomm/sock.c:902
sock_do_ioctl+0xe4/0x3e0 net/socket.c:969
sock_ioctl+0x30d/0x680 net/socket.c:1093
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:0x456a09
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:00007f521726dc78 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
RAX: ffffffffffffffda RBX: 00007f521726e6d4 RCX: 0000000000456a09
RDX: 0000000020000140 RSI: 00000000400452c8 RDI: 0000000000000014
RBP: 0000000000930140 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00000000ffffffff
R13: 00000000004cddc0 R14: 00000000004c472b R15: 0000000000000001
======================================================
WARNING: possible circular locking dependency detected
4.18.0-rc7+ #170 Not tainted
------------------------------------------------------
syz-executor4/1696 is trying to acquire lock:
0000000072b74d86 ((console_sem).lock){-.-.}, at: down_trylock+0x13/0x70
kernel/locking/semaphore.c:136
but task is already holding lock:
000000001082e10b (&obj_hash[i].lock){-.-.}, at: __debug_check_no_obj_freed
lib/debugobjects.c:774 [inline]
000000001082e10b (&obj_hash[i].lock){-.-.}, at:
debug_check_no_obj_freed+0x16c/0x595 lib/debugobjects.c:815
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #3 (&obj_hash[i].lock){-.-.}:
__raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline]
_raw_spin_lock_irqsave+0x96/0xc0 kernel/locking/spinlock.c:152
__debug_object_init+0x127/0x12e0 lib/debugobjects.c:381
debug_object_init+0x16/0x20 lib/debugobjects.c:429
debug_hrtimer_init kernel/time/hrtimer.c:410 [inline]
debug_init kernel/time/hrtimer.c:458 [inline]
hrtimer_init+0x97/0x480 kernel/time/hrtimer.c:1308
init_dl_task_timer+0x1b/0x50 kernel/sched/deadline.c:1056
__sched_fork+0x2ae/0x590 kernel/sched/core.c:2185
init_idle+0x75/0x7a0 kernel/sched/core.c:5405
sched_init+0xbf3/0xd2c kernel/sched/core.c:6103
start_kernel+0x47d/0x949 init/main.c:602
x86_64_start_reservations+0x29/0x2b arch/x86/kernel/head64.c:452
x86_64_start_kernel+0x76/0x79 arch/x86/kernel/head64.c:433
secondary_startup_64+0xa5/0xb0 arch/x86/kernel/head_64.S:242
-> #2 (&rq->lock){-.-.}:
__raw_spin_lock include/linux/spinlock_api_smp.h:142 [inline]
_raw_spin_lock+0x2a/0x40 kernel/locking/spinlock.c:144
rq_lock kernel/sched/sched.h:1812 [inline]
task_fork_fair+0x93/0x680 kernel/sched/fair.c:9952
sched_fork+0x446/0xb40 kernel/sched/core.c:2381
copy_process.part.39+0x1bf5/0x70b0 kernel/fork.c:1796
copy_process kernel/fork.c:1639 [inline]
_do_fork+0x291/0x12a0 kernel/fork.c:2122
kernel_thread+0x34/0x40 kernel/fork.c:2181
rest_init+0x22/0xe4 init/main.c:408
start_kernel+0x90e/0x949 init/main.c:738
x86_64_start_reservations+0x29/0x2b arch/x86/kernel/head64.c:452
x86_64_start_kernel+0x76/0x79 arch/x86/kernel/head64.c:433
secondary_startup_64+0xa5/0xb0 arch/x86/kernel/head_64.S:242
-> #1 (&p->pi_lock){-.-.}:
__raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline]
_raw_spin_lock_irqsave+0x96/0xc0 kernel/locking/spinlock.c:152
try_to_wake_up+0xd2/0x12a0 kernel/sched/core.c:1985
wake_up_process+0x10/0x20 kernel/sched/core.c:2148
__up.isra.1+0x1c0/0x2a0 kernel/locking/semaphore.c:262
up+0x13c/0x1c0 kernel/locking/semaphore.c:187
__up_console_sem+0xbe/0x1b0 kernel/printk/printk.c:242
console_unlock+0x7a2/0x10b0 kernel/printk/printk.c:2411
vprintk_emit+0x6c6/0xdf0 kernel/printk/printk.c:1907
vprintk_default+0x28/0x30 kernel/printk/printk.c:1948
vprintk_func+0x7a/0xe7 kernel/printk/printk_safe.c:382
printk+0xa7/0xcf kernel/printk/printk.c:1981
load_umh+0x51/0xbd net/bpfilter/bpfilter_kern.c:98
do_one_initcall+0x127/0x913 init/main.c:884
do_initcall_level init/main.c:952 [inline]
do_initcalls init/main.c:960 [inline]
do_basic_setup init/main.c:978 [inline]
kernel_init_freeable+0x49b/0x58e init/main.c:1135
kernel_init+0x11/0x1b3 init/main.c:1061
ret_from_fork+0x3a/0x50 arch/x86/entry/entry_64.S:412
-> #0 ((console_sem).lock){-.-.}:
lock_acquire+0x1e4/0x540 kernel/locking/lockdep.c:3924
__raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline]
_raw_spin_lock_irqsave+0x96/0xc0 kernel/locking/spinlock.c:152
down_trylock+0x13/0x70 kernel/locking/semaphore.c:136
__down_trylock_console_sem+0xae/0x200 kernel/printk/printk.c:225
console_trylock+0x15/0xa0 kernel/printk/printk.c:2230
console_trylock_spinning kernel/printk/printk.c:1643 [inline]
vprintk_emit+0x6ad/0xdf0 kernel/printk/printk.c:1906
vprintk_default+0x28/0x30 kernel/printk/printk.c:1948
vprintk_func+0x7a/0xe7 kernel/printk/printk_safe.c:382
printk+0xa7/0xcf kernel/printk/printk.c:1981
__warn_printk+0x8c/0xe0 kernel/panic.c:590
debug_print_object+0x16a/0x210 lib/debugobjects.c:326
__debug_check_no_obj_freed lib/debugobjects.c:783 [inline]
debug_check_no_obj_freed+0x3b2/0x595 lib/debugobjects.c:815
kfree+0xc7/0x260 mm/slab.c:3812
rfcomm_dlc_free+0x1e/0x30 net/bluetooth/rfcomm/core.c:328
rfcomm_dlc_put include/net/bluetooth/rfcomm.h:258 [inline]
__rfcomm_create_dev net/bluetooth/rfcomm/tty.c:417 [inline]
rfcomm_create_dev net/bluetooth/rfcomm/tty.c:486 [inline]
rfcomm_dev_ioctl+0x1dfe/0x2240 net/bluetooth/rfcomm/tty.c:588
rfcomm_sock_ioctl+0x89/0xb0 net/bluetooth/rfcomm/sock.c:902
sock_do_ioctl+0xe4/0x3e0 net/socket.c:969
sock_ioctl+0x30d/0x680 net/socket.c:1093
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:
(console_sem).lock --> &rq->lock --> &obj_hash[i].lock
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(&obj_hash[i].lock);
lock(&rq->lock);
lock(&obj_hash[i].lock);
lock((console_sem).lock);
*** DEADLOCK ***
3 locks held by syz-executor4/1696:
#0: 000000007adfc19a (sk_lock-AF_BLUETOOTH-BTPROTO_RFCOMM){+.+.}, at:
lock_sock include/net/sock.h:1474 [inline]
#0: 000000007adfc19a (sk_lock-AF_BLUETOOTH-BTPROTO_RFCOMM){+.+.}, at:
rfcomm_sock_ioctl+0x7b/0xb0 net/bluetooth/rfcomm/sock.c:901
#1: 00000000f97b5970 (rfcomm_ioctl_mutex){+.+.}, at: rfcomm_create_dev
net/bluetooth/rfcomm/tty.c:485 [inline]
#1: 00000000f97b5970 (rfcomm_ioctl_mutex){+.+.}, at:
rfcomm_dev_ioctl+0x4a5/0x2240 net/bluetooth/rfcomm/tty.c:588
#2: 000000001082e10b (&obj_hash[i].lock){-.-.}, at:
__debug_check_no_obj_freed lib/debugobjects.c:774 [inline]
#2: 000000001082e10b (&obj_hash[i].lock){-.-.}, at:
debug_check_no_obj_freed+0x16c/0x595 lib/debugobjects.c:815
stack backtrace:
CPU: 0 PID: 1696 Comm: syz-executor4 Not tainted 4.18.0-rc7+ #170
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
__raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline]
_raw_spin_lock_irqsave+0x96/0xc0 kernel/locking/spinlock.c:152
down_trylock+0x13/0x70 kernel/locking/semaphore.c:136
__down_trylock_console_sem+0xae/0x200 kernel/printk/printk.c:225
console_trylock+0x15/0xa0 kernel/printk/printk.c:2230
console_trylock_spinning kernel/printk/printk.c:1643 [inline]
vprintk_emit+0x6ad/0xdf0 kernel/printk/printk.c:1906
vprintk_default+0x28/0x30 kernel/printk/printk.c:1948
vprintk_func+0x7a/0xe7 kernel/printk/printk_safe.c:382
printk+0xa7/0xcf kernel/printk/printk.c:1981
__warn_printk+0x8c/0xe0 kernel/panic.c:590
debug_print_object+0x16a/0x210 lib/debugobjects.c:326
__debug_check_no_obj_freed lib/debugobjects.c:783 [inline]
debug_check_no_obj_freed+0x3b2/0x595 lib/debugobjects.c:815
kfree+0xc7/0x260 mm/slab.c:3812
rfcomm_dlc_free+0x1e/0x30 net/bluetooth/rfcomm/core.c:328
rfcomm_dlc_put include/net/bluetooth/rfcomm.h:258 [inline]
__rfcomm_create_dev net/bluetooth/rfcomm/tty.c:417 [inline]
rfcomm_create_dev net/bluetooth/rfcomm/tty.c:486 [inline]
rfcomm_dev_ioctl+0x1dfe/0x2240 net/bluetooth/rfcomm/tty.c:588
rfcomm_sock_ioctl+0x89/0xb0 net/bluetooth/rfcomm/sock.c:902
sock_do_ioctl+0xe4/0x3e0 net/socket.c:969
sock_ioctl+0x30d/0x680 net/socket.c:1093
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:0x456a09
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:00007f521726dc78 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
RAX: ffffffffffffffda RBX: 00007f521726e6d4 RCX: 0000000000456a09
RDX: 0000000020000140 RSI: 00000000400452c8 RDI: 0000000000000014
RBP: 0000000000930140 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00000000ffffffff
R13: 00000000004cddc0 R14: 00000000004c472b R15: 0000000000000001
Dumping ftrace buffer:
(ftrace buffer empty)
Kernel Offset: disabled
Rebooting in 86400 seconds..
---
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
* [PATCH net] ipv4: frags: handle possible skb truesize change
From: Eric Dumazet @ 2018-07-31 4:50 UTC (permalink / raw)
To: David S . Miller; +Cc: netdev, Eric Dumazet, Eric Dumazet
ip_frag_queue() might call pskb_pull() on one skb that
is already in the fragment queue.
We need to take care of possible truesize change, or we
might have an imbalance of the netns frags memory usage.
IPv6 is immune to this bug, because RFC5722, Section 4,
amended by Errata ID 3089 states :
When reassembling an IPv6 datagram, if
one or more its constituent fragments is determined to be an
overlapping fragment, the entire datagram (and any constituent
fragments) MUST be silently discarded.
Fixes: 158f323b9868 ("net: adjust skb->truesize in pskb_expand_head()")
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
net/ipv4/ip_fragment.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/net/ipv4/ip_fragment.c b/net/ipv4/ip_fragment.c
index 8e9528ebaa8e1af91172466cc161a83301a217ca..d14d741fb05e571d6d0b03248da77fd993debdee 100644
--- a/net/ipv4/ip_fragment.c
+++ b/net/ipv4/ip_fragment.c
@@ -383,11 +383,16 @@ static int ip_frag_queue(struct ipq *qp, struct sk_buff *skb)
int i = end - next->ip_defrag_offset; /* overlap is 'i' bytes */
if (i < next->len) {
+ int delta = -next->truesize;
+
/* Eat head of the next overlapped fragment
* and leave the loop. The next ones cannot overlap.
*/
if (!pskb_pull(next, i))
goto err;
+ delta += next->truesize;
+ if (delta)
+ add_frag_mem_limit(qp->q.net, delta);
next->ip_defrag_offset += i;
qp->q.meat -= i;
if (next->ip_summed != CHECKSUM_UNNECESSARY)
--
2.18.0.345.g5c9ce644c3-goog
^ permalink raw reply related
* Re: WARNING: ODEBUG bug in vsock_stream_connect
From: syzbot @ 2018-07-31 5:47 UTC (permalink / raw)
To: davem, dvlasenk, jhansen, linux-kernel, netdev, stefanha,
syzkaller-bugs, viro, xiyou.wangcong
In-Reply-To: <000000000000ea418a0572384417@google.com>
syzbot has found a reproducer for the following crash on:
HEAD commit: 527838d470e3 Merge branch 'x86-urgent-for-linus' of git://..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=160e3d78400000
kernel config: https://syzkaller.appspot.com/x/.config?x=2dc0cd7c2eefb46f
dashboard link: https://syzkaller.appspot.com/bug?extid=8a9b1bd330476a4f3db6
compiler: gcc (GCC) 8.0.1 20180413 (experimental)
userspace arch: i386
syzkaller repro:https://syzkaller.appspot.com/x/repro.syz?x=15100f2c400000
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+8a9b1bd330476a4f3db6@syzkaller.appspotmail.com
8021q: adding VLAN 0 to HW filter on device team0
8021q: adding VLAN 0 to HW filter on device team0
8021q: adding VLAN 0 to HW filter on device team0
------------[ cut here ]------------
ODEBUG: init active (active state 0) object type: timer_list hint:
delayed_work_timer_fn+0x0/0x90 kernel/workqueue.c:1414
WARNING: CPU: 1 PID: 6513 at lib/debugobjects.c:329
debug_print_object+0x16a/0x210 lib/debugobjects.c:326
Kernel panic - not syncing: panic_on_warn set ...
CPU: 1 PID: 6513 Comm: syz-executor4 Not tainted 4.18.0-rc7+ #73
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
panic+0x238/0x4e7 kernel/panic.c:184
__warn.cold.8+0x163/0x1ba kernel/panic.c:536
report_bug+0x252/0x2d0 lib/bug.c:186
fixup_bug arch/x86/kernel/traps.c:178 [inline]
do_error_trap+0x1fc/0x4d0 arch/x86/kernel/traps.c:296
do_invalid_op+0x1b/0x20 arch/x86/kernel/traps.c:316
invalid_op+0x14/0x20 arch/x86/entry/entry_64.S:992
RIP: 0010:debug_print_object+0x16a/0x210 lib/debugobjects.c:326
Code: 3a 87 48 89 fa 48 c1 ea 03 80 3c 02 00 0f 85 92 00 00 00 48 8b 14 dd
20 74 3a 87 4c 89 f6 48 c7 c7 c0 69 3a 87 e8 c6 b0 e6 fd <0f> 0b 83 05 a9
e6 29 05 01 48 83 c4 18 5b 41 5c 41 5d 41 5e 41 5f
RSP: 0018:ffff8801b446f608 EFLAGS: 00010086
RAX: 0000000000000000 RBX: 0000000000000003 RCX: 0000000000000000
RDX: 0000000000000000 RSI: ffffffff81632481 RDI: 0000000000000001
RBP: ffff8801b446f648 R08: ffff8801ae066340 R09: ffffed003b623ec2
R10: ffffed003b623ec2 R11: ffff8801db11f617 R12: 0000000000000001
R13: ffffffff87f9c9e0 R14: ffffffff873a6fc0 R15: ffffffff816918a0
__debug_object_init+0x8e9/0x12e0 lib/debugobjects.c:403
debug_object_init+0x16/0x20 lib/debugobjects.c:429
debug_timer_init kernel/time/timer.c:704 [inline]
debug_init kernel/time/timer.c:757 [inline]
init_timer_key+0xa9/0x490 kernel/time/timer.c:806
vsock_stream_connect+0xc7a/0xfc0 net/vmw_vsock/af_vsock.c:1224
__sys_connect+0x37d/0x4c0 net/socket.c:1673
__do_sys_connect net/socket.c:1684 [inline]
__se_sys_connect net/socket.c:1681 [inline]
__ia32_sys_connect+0x72/0xb0 net/socket.c:1681
do_syscall_32_irqs_on arch/x86/entry/common.c:326 [inline]
do_fast_syscall_32+0x34d/0xfb2 arch/x86/entry/common.c:397
entry_SYSENTER_compat+0x70/0x7f arch/x86/entry/entry_64_compat.S:139
RIP: 0023:0xf7f95cb9
Code: 55 08 8b 88 64 cd ff ff 8b 98 68 cd ff ff 89 c8 85 d2 74 02 89 0a 5b
5d c3 8b 04 24 c3 8b 1c 24 c3 51 52 55 89 e5 0f 34 cd 80 <5d> 5a 59 c3 90
90 90 90 eb 0d 90 90 90 90 90 90 90 90 90 90 90 90
RSP: 002b:00000000f7f4e0cc EFLAGS: 00000296 ORIG_RAX: 000000000000016a
RAX: ffffffffffffffda RBX: 0000000000000004 RCX: 0000000020000100
RDX: 0000000000000010 RSI: 0000000000000000 RDI: 0000000000000000
RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
======================================================
WARNING: possible circular locking dependency detected
4.18.0-rc7+ #73 Not tainted
------------------------------------------------------
syz-executor4/6513 is trying to acquire lock:
(____ptrval____) ((console_sem).lock){-...}, at: down_trylock+0x13/0x70
kernel/locking/semaphore.c:136
but task is already holding lock:
(____ptrval____) (&obj_hash[i].lock){-.-.}, at:
__debug_object_init+0x127/0x12e0 lib/debugobjects.c:381
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #3 (&obj_hash[i].lock){-.-.}:
__raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline]
_raw_spin_lock_irqsave+0x96/0xc0 kernel/locking/spinlock.c:152
__debug_object_init+0x127/0x12e0 lib/debugobjects.c:381
debug_object_init+0x16/0x20 lib/debugobjects.c:429
debug_hrtimer_init kernel/time/hrtimer.c:410 [inline]
debug_init kernel/time/hrtimer.c:458 [inline]
hrtimer_init+0x97/0x480 kernel/time/hrtimer.c:1308
init_dl_task_timer+0x1b/0x50 kernel/sched/deadline.c:1056
__sched_fork+0x2ae/0x590 kernel/sched/core.c:2185
init_idle+0x75/0x7a0 kernel/sched/core.c:5405
sched_init+0xbf3/0xd2c kernel/sched/core.c:6103
start_kernel+0x47d/0x949 init/main.c:602
x86_64_start_reservations+0x29/0x2b arch/x86/kernel/head64.c:452
x86_64_start_kernel+0x76/0x79 arch/x86/kernel/head64.c:433
secondary_startup_64+0xa5/0xb0 arch/x86/kernel/head_64.S:242
-> #2 (&rq->lock){-.-.}:
__raw_spin_lock include/linux/spinlock_api_smp.h:142 [inline]
_raw_spin_lock+0x2a/0x40 kernel/locking/spinlock.c:144
rq_lock kernel/sched/sched.h:1812 [inline]
task_fork_fair+0x93/0x680 kernel/sched/fair.c:9952
sched_fork+0x446/0xb40 kernel/sched/core.c:2381
copy_process.part.39+0x1bf5/0x70b0 kernel/fork.c:1796
copy_process kernel/fork.c:1639 [inline]
_do_fork+0x291/0x12a0 kernel/fork.c:2122
kernel_thread+0x34/0x40 kernel/fork.c:2181
rest_init+0x22/0xe4 init/main.c:408
start_kernel+0x90e/0x949 init/main.c:738
x86_64_start_reservations+0x29/0x2b arch/x86/kernel/head64.c:452
x86_64_start_kernel+0x76/0x79 arch/x86/kernel/head64.c:433
secondary_startup_64+0xa5/0xb0 arch/x86/kernel/head_64.S:242
-> #1 (&p->pi_lock){-.-.}:
__raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline]
_raw_spin_lock_irqsave+0x96/0xc0 kernel/locking/spinlock.c:152
try_to_wake_up+0xd2/0x12a0 kernel/sched/core.c:1985
wake_up_process+0x10/0x20 kernel/sched/core.c:2148
__up.isra.1+0x1c0/0x2a0 kernel/locking/semaphore.c:262
up+0x13c/0x1c0 kernel/locking/semaphore.c:187
__up_console_sem+0xbe/0x1b0 kernel/printk/printk.c:242
console_unlock+0x7a2/0x10b0 kernel/printk/printk.c:2411
vprintk_emit+0x6c6/0xdf0 kernel/printk/printk.c:1907
vprintk_default+0x28/0x30 kernel/printk/printk.c:1948
vprintk_func+0x7a/0xe7 kernel/printk/printk_safe.c:382
printk+0xa7/0xcf kernel/printk/printk.c:1981
load_umh+0x51/0xbd net/bpfilter/bpfilter_kern.c:98
do_one_initcall+0x127/0x913 init/main.c:884
do_initcall_level init/main.c:952 [inline]
do_initcalls init/main.c:960 [inline]
do_basic_setup init/main.c:978 [inline]
kernel_init_freeable+0x49b/0x58e init/main.c:1135
kernel_init+0x11/0x1b3 init/main.c:1061
ret_from_fork+0x3a/0x50 arch/x86/entry/entry_64.S:412
-> #0 ((console_sem).lock){-...}:
lock_acquire+0x1e4/0x540 kernel/locking/lockdep.c:3924
__raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline]
_raw_spin_lock_irqsave+0x96/0xc0 kernel/locking/spinlock.c:152
down_trylock+0x13/0x70 kernel/locking/semaphore.c:136
__down_trylock_console_sem+0xae/0x200 kernel/printk/printk.c:225
console_trylock+0x15/0xa0 kernel/printk/printk.c:2230
console_trylock_spinning kernel/printk/printk.c:1643 [inline]
vprintk_emit+0x6ad/0xdf0 kernel/printk/printk.c:1906
vprintk_default+0x28/0x30 kernel/printk/printk.c:1948
vprintk_func+0x7a/0xe7 kernel/printk/printk_safe.c:382
printk+0xa7/0xcf kernel/printk/printk.c:1981
__warn_printk+0x8c/0xe0 kernel/panic.c:590
debug_print_object+0x16a/0x210 lib/debugobjects.c:326
__debug_object_init+0x8e9/0x12e0 lib/debugobjects.c:403
debug_object_init+0x16/0x20 lib/debugobjects.c:429
debug_timer_init kernel/time/timer.c:704 [inline]
debug_init kernel/time/timer.c:757 [inline]
init_timer_key+0xa9/0x490 kernel/time/timer.c:806
vsock_stream_connect+0xc7a/0xfc0 net/vmw_vsock/af_vsock.c:1224
__sys_connect+0x37d/0x4c0 net/socket.c:1673
__do_sys_connect net/socket.c:1684 [inline]
__se_sys_connect net/socket.c:1681 [inline]
__ia32_sys_connect+0x72/0xb0 net/socket.c:1681
do_syscall_32_irqs_on arch/x86/entry/common.c:326 [inline]
do_fast_syscall_32+0x34d/0xfb2 arch/x86/entry/common.c:397
entry_SYSENTER_compat+0x70/0x7f arch/x86/entry/entry_64_compat.S:139
other info that might help us debug this:
Chain exists of:
(console_sem).lock --> &rq->lock --> &obj_hash[i].lock
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(&obj_hash[i].lock);
lock(&rq->lock);
lock(&obj_hash[i].lock);
lock((console_sem).lock);
*** DEADLOCK ***
2 locks held by syz-executor4/6513:
#0: (____ptrval____) (sk_lock-AF_VSOCK){+.+.}, at: lock_sock
include/net/sock.h:1474 [inline]
#0: (____ptrval____) (sk_lock-AF_VSOCK){+.+.}, at:
vsock_stream_connect+0x1e3/0xfc0 net/vmw_vsock/af_vsock.c:1152
#1: (____ptrval____) (&obj_hash[i].lock){-.-.}, at:
__debug_object_init+0x127/0x12e0 lib/debugobjects.c:381
stack backtrace:
CPU: 1 PID: 6513 Comm: syz-executor4 Not tainted 4.18.0-rc7+ #73
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
__raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline]
_raw_spin_lock_irqsave+0x96/0xc0 kernel/locking/spinlock.c:152
down_trylock+0x13/0x70 kernel/locking/semaphore.c:136
__down_trylock_console_sem+0xae/0x200 kernel/printk/printk.c:225
console_trylock+0x15/0xa0 kernel/printk/printk.c:2230
console_trylock_spinning kernel/printk/printk.c:1643 [inline]
vprintk_emit+0x6ad/0xdf0 kernel/printk/printk.c:1906
vprintk_default+0x28/0x30 kernel/printk/printk.c:1948
vprintk_func+0x7a/0xe7 kernel/printk/printk_safe.c:382
printk+0xa7/0xcf kernel/printk/printk.c:1981
__warn_printk+0x8c/0xe0 kernel/panic.c:590
debug_print_object+0x16a/0x210 lib/debugobjects.c:326
__debug_object_init+0x8e9/0x12e0 lib/debugobjects.c:403
debug_object_init+0x16/0x20 lib/debugobjects.c:429
debug_timer_init kernel/time/timer.c:704 [inline]
debug_init kernel/time/timer.c:757 [inline]
init_timer_key+0xa9/0x490 kernel/time/timer.c:806
vsock_stream_connect+0xc7a/0xfc0 net/vmw_vsock/af_vsock.c:1224
__sys_connect+0x37d/0x4c0 net/socket.c:1673
__do_sys_connect net/socket.c:1684 [inline]
__se_sys_connect net/socket.c:1681 [inline]
__ia32_sys_connect+0x72/0xb0 net/socket.c:1681
do_syscall_32_irqs_on arch/x86/entry/common.c:326 [inline]
do_fast_syscall_32+0x34d/0xfb2 arch/x86/entry/common.c:397
entry_SYSENTER_compat+0x70/0x7f arch/x86/entry/entry_64_compat.S:139
RIP: 0023:0xf7f95cb9
Code: 55 08 8b 88 64 cd ff ff 8b 98 68 cd ff ff 89 c8 85 d2 74 02 89 0a 5b
5d c3 8b 04 24 c3 8b 1c 24 c3 51 52 55 89 e5 0f 34 cd 80 <5d> 5a 59 c3 90
90 90 90 eb 0d 90 90 90 90 90 90 90 90 90 90 90 90
RSP: 002b:00000000f7f4e0cc EFLAGS: 00000296 ORIG_RAX: 000000000000016a
RAX: ffffffffffffffda RBX: 0000000000000004 RCX: 0000000020000100
RDX: 0000000000000010 RSI: 0000000000000000 RDI: 0000000000000000
RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
Dumping ftrace buffer:
(ftrace buffer empty)
Kernel Offset: disabled
Rebooting in 86400 seconds..
^ permalink raw reply
* Re: [PATCH v2 bpf-next 4/4] selftests/bpf: Test for get_socket_cookie
From: Yonghong Song @ 2018-07-31 3:44 UTC (permalink / raw)
To: Andrey Ignatov, netdev; +Cc: ast, daniel, brakmo, kernel-team
In-Reply-To: <1d192899abca496d54d76b1b991e05f5733a382f.1532996972.git.rdna@fb.com>
On 7/30/18 5:42 PM, Andrey Ignatov wrote:
> Add test to use get_socket_cookie() from BPF programs of types
> BPF_PROG_TYPE_SOCK_OPS and BPF_PROG_TYPE_CGROUP_SOCK_ADDR.
>
> The test attaches two programs to cgroup, runs TCP server and client in
> the cgroup and checks that two operations are done properly on client
> socket when user calls connect(2):
>
> 1. In BPF_CGROUP_INET6_CONNECT socket cookie is used as the key to write
> new value in a map for client socket.
>
> 2. In BPF_CGROUP_SOCK_OPS (BPF_SOCK_OPS_TCP_CONNECT_CB callback) the
> value written in "1." is found by socket cookie, since it's the same
> socket, and updated.
>
> Finally the test verifies the value in the map.
>
> Signed-off-by: Andrey Ignatov <rdna@fb.com>
Acked-by: Yonghong Song <yhs@fb.com>
^ permalink raw reply
* [PATCH net-next 3/3] xsk: don't allow umem replace at stack level
From: Jakub Kicinski @ 2018-07-31 3:43 UTC (permalink / raw)
To: davem, alexei.starovoitov, daniel
Cc: netdev, oss-drivers, bjorn.topel, Jakub Kicinski
In-Reply-To: <20180731034353.3774-1-jakub.kicinski@netronome.com>
Currently drivers have to check if they already have a umem
installed for a given queue and return an error if so. Make
better use of XDP_QUERY_XSK_UMEM and move this functionality
to the core.
We need to keep rtnl across the calls now.
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
Acked-by: Björn Töpel <bjorn.topel@gmail.com>
---
include/linux/netdevice.h | 7 ++++---
net/xdp/xdp_umem.c | 37 ++++++++++++++++++++++++++++---------
2 files changed, 32 insertions(+), 12 deletions(-)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 3bf7e93c9e96..282e2e95ad5b 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -872,10 +872,10 @@ struct netdev_bpf {
struct {
struct bpf_offloaded_map *offmap;
};
- /* XDP_SETUP_XSK_UMEM */
+ /* XDP_QUERY_XSK_UMEM, XDP_SETUP_XSK_UMEM */
struct {
- struct xdp_umem *umem;
- u16 queue_id;
+ struct xdp_umem *umem; /* out for query*/
+ u16 queue_id; /* in for query */
} xsk;
};
};
@@ -3568,6 +3568,7 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
int fd, u32 flags);
u32 __dev_xdp_query(struct net_device *dev, bpf_op_t xdp_op,
enum bpf_netdev_command cmd);
+int xdp_umem_query(struct net_device *dev, u16 queue_id);
int __dev_forward_skb(struct net_device *dev, struct sk_buff *skb);
int dev_forward_skb(struct net_device *dev, struct sk_buff *skb);
diff --git a/net/xdp/xdp_umem.c b/net/xdp/xdp_umem.c
index c199d66b5f3f..911ca6d3cb5a 100644
--- a/net/xdp/xdp_umem.c
+++ b/net/xdp/xdp_umem.c
@@ -11,6 +11,8 @@
#include <linux/slab.h>
#include <linux/bpf.h>
#include <linux/mm.h>
+#include <linux/netdevice.h>
+#include <linux/rtnetlink.h>
#include "xdp_umem.h"
#include "xsk_queue.h"
@@ -40,6 +42,21 @@ void xdp_del_sk_umem(struct xdp_umem *umem, struct xdp_sock *xs)
}
}
+int xdp_umem_query(struct net_device *dev, u16 queue_id)
+{
+ struct netdev_bpf bpf;
+
+ ASSERT_RTNL();
+
+ memset(&bpf, 0, sizeof(bpf));
+ bpf.command = XDP_QUERY_XSK_UMEM;
+ bpf.xsk.queue_id = queue_id;
+
+ if (!dev->netdev_ops->ndo_bpf)
+ return 0;
+ return dev->netdev_ops->ndo_bpf(dev, &bpf) ?: !!bpf.xsk.umem;
+}
+
int xdp_umem_assign_dev(struct xdp_umem *umem, struct net_device *dev,
u32 queue_id, u16 flags)
{
@@ -62,28 +79,30 @@ int xdp_umem_assign_dev(struct xdp_umem *umem, struct net_device *dev,
bpf.command = XDP_QUERY_XSK_UMEM;
rtnl_lock();
- err = dev->netdev_ops->ndo_bpf(dev, &bpf);
- rtnl_unlock();
-
- if (err)
- return force_zc ? -ENOTSUPP : 0;
+ err = xdp_umem_query(dev, queue_id);
+ if (err) {
+ err = err < 0 ? -ENOTSUPP : -EBUSY;
+ goto err_rtnl_unlock;
+ }
bpf.command = XDP_SETUP_XSK_UMEM;
bpf.xsk.umem = umem;
bpf.xsk.queue_id = queue_id;
- rtnl_lock();
err = dev->netdev_ops->ndo_bpf(dev, &bpf);
- rtnl_unlock();
-
if (err)
- return force_zc ? err : 0; /* fail or fallback */
+ goto err_rtnl_unlock;
+ rtnl_unlock();
dev_hold(dev);
umem->dev = dev;
umem->queue_id = queue_id;
umem->zc = true;
return 0;
+
+err_rtnl_unlock:
+ rtnl_unlock();
+ return force_zc ? err : 0; /* fail or fallback */
}
static void xdp_umem_clear_dev(struct xdp_umem *umem)
--
2.17.1
^ permalink raw reply related
* [PATCH net-next 2/3] xsk: refactor xdp_umem_assign_dev()
From: Jakub Kicinski @ 2018-07-31 3:43 UTC (permalink / raw)
To: davem, alexei.starovoitov, daniel
Cc: netdev, oss-drivers, bjorn.topel, Jakub Kicinski
In-Reply-To: <20180731034353.3774-1-jakub.kicinski@netronome.com>
Return early and only take the ref on dev once there is no possibility
of failing.
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
Acked-by: Björn Töpel <bjorn.topel@gmail.com>
---
net/xdp/xdp_umem.c | 49 ++++++++++++++++++++--------------------------
1 file changed, 21 insertions(+), 28 deletions(-)
diff --git a/net/xdp/xdp_umem.c b/net/xdp/xdp_umem.c
index f47abb46c587..c199d66b5f3f 100644
--- a/net/xdp/xdp_umem.c
+++ b/net/xdp/xdp_umem.c
@@ -56,41 +56,34 @@ int xdp_umem_assign_dev(struct xdp_umem *umem, struct net_device *dev,
if (force_copy)
return 0;
- dev_hold(dev);
+ if (!dev->netdev_ops->ndo_bpf || !dev->netdev_ops->ndo_xsk_async_xmit)
+ return force_zc ? -ENOTSUPP : 0; /* fail or fallback */
- if (dev->netdev_ops->ndo_bpf && dev->netdev_ops->ndo_xsk_async_xmit) {
- bpf.command = XDP_QUERY_XSK_UMEM;
+ bpf.command = XDP_QUERY_XSK_UMEM;
- rtnl_lock();
- err = dev->netdev_ops->ndo_bpf(dev, &bpf);
- rtnl_unlock();
+ rtnl_lock();
+ err = dev->netdev_ops->ndo_bpf(dev, &bpf);
+ rtnl_unlock();
- if (err) {
- dev_put(dev);
- return force_zc ? -ENOTSUPP : 0;
- }
+ if (err)
+ return force_zc ? -ENOTSUPP : 0;
- bpf.command = XDP_SETUP_XSK_UMEM;
- bpf.xsk.umem = umem;
- bpf.xsk.queue_id = queue_id;
+ bpf.command = XDP_SETUP_XSK_UMEM;
+ bpf.xsk.umem = umem;
+ bpf.xsk.queue_id = queue_id;
- rtnl_lock();
- err = dev->netdev_ops->ndo_bpf(dev, &bpf);
- rtnl_unlock();
+ rtnl_lock();
+ err = dev->netdev_ops->ndo_bpf(dev, &bpf);
+ rtnl_unlock();
- if (err) {
- dev_put(dev);
- return force_zc ? err : 0; /* fail or fallback */
- }
-
- umem->dev = dev;
- umem->queue_id = queue_id;
- umem->zc = true;
- return 0;
- }
+ if (err)
+ return force_zc ? err : 0; /* fail or fallback */
- dev_put(dev);
- return force_zc ? -ENOTSUPP : 0; /* fail or fallback */
+ dev_hold(dev);
+ umem->dev = dev;
+ umem->queue_id = queue_id;
+ umem->zc = true;
+ return 0;
}
static void xdp_umem_clear_dev(struct xdp_umem *umem)
--
2.17.1
^ permalink raw reply related
* [PATCH net-next 1/3] net: update real_num_rx_queues even when !CONFIG_SYSFS
From: Jakub Kicinski @ 2018-07-31 3:43 UTC (permalink / raw)
To: davem, alexei.starovoitov, daniel
Cc: netdev, oss-drivers, bjorn.topel, Jakub Kicinski
In-Reply-To: <20180731034353.3774-1-jakub.kicinski@netronome.com>
We used to depend on real_num_rx_queues as a upper bound for sanity
checks. For AF_XDP socket validation it's useful if the check behaves
the same regardless of CONFIG_SYSFS setting.
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
---
include/linux/netdevice.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 9c917467a2c7..3bf7e93c9e96 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3431,8 +3431,9 @@ int netif_set_real_num_tx_queues(struct net_device *dev, unsigned int txq);
int netif_set_real_num_rx_queues(struct net_device *dev, unsigned int rxq);
#else
static inline int netif_set_real_num_rx_queues(struct net_device *dev,
- unsigned int rxq)
+ unsigned int rxqs)
{
+ dev->real_num_rx_queues = rxqs;
return 0;
}
#endif
--
2.17.1
^ permalink raw reply related
* [PATCH net-next 0/3] xsk: improvements to RX queue check and replace
From: Jakub Kicinski @ 2018-07-31 3:43 UTC (permalink / raw)
To: davem, alexei.starovoitov, daniel
Cc: netdev, oss-drivers, bjorn.topel, Jakub Kicinski
Hi!
First 3 patches of my recent RFC. The first one make the check against
real_num_rx_queues slightly more reliable, while the latter two redefine
XDP_QUERY_XSK_UMEM slightly to disallow replacing UMEM in the driver at
the stack level.
I'm not sure where this lays on the bpf vs net trees scale, but there
should be no conflicts with either tree.
Jakub Kicinski (3):
net: update real_num_rx_queues even when !CONFIG_SYSFS
xsk: refactor xdp_umem_assign_dev()
xsk: don't allow umem replace at stack level
include/linux/netdevice.h | 10 +++---
net/xdp/xdp_umem.c | 70 +++++++++++++++++++++++----------------
2 files changed, 47 insertions(+), 33 deletions(-)
--
2.17.1
^ permalink raw reply
* [PATCH bpf-next] nfp: fix variable dereferenced before check in nfp_app_ctrl_rx_raw()
From: Jakub Kicinski @ 2018-07-31 3:33 UTC (permalink / raw)
To: alexei.starovoitov, daniel; +Cc: netdev, oss-drivers, Jakub Kicinski
'app' is dereferenced before used for the devlink trace point.
In case FW is buggy and sends a control message to a VF queue
we should make sure app is not NULL.
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Dirk van der Merwe <dirk.vandermerwe@netronome.com>
---
drivers/net/ethernet/netronome/nfp/nfp_app.h | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_app.h b/drivers/net/ethernet/netronome/nfp/nfp_app.h
index ccb244cf6c30..4e1eb3395648 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_app.h
+++ b/drivers/net/ethernet/netronome/nfp/nfp_app.h
@@ -392,10 +392,11 @@ static inline void nfp_app_ctrl_rx(struct nfp_app *app, struct sk_buff *skb)
static inline void
nfp_app_ctrl_rx_raw(struct nfp_app *app, const void *data, unsigned int len)
{
- trace_devlink_hwmsg(priv_to_devlink(app->pf), true, 0, data, len);
+ if (!app || !app->type->ctrl_msg_rx_raw)
+ return;
- if (app && app->type->ctrl_msg_rx_raw)
- app->type->ctrl_msg_rx_raw(app, data, len);
+ trace_devlink_hwmsg(priv_to_devlink(app->pf), true, 0, data, len);
+ app->type->ctrl_msg_rx_raw(app, data, len);
}
static inline int nfp_app_eswitch_mode_get(struct nfp_app *app, u16 *mode)
--
2.17.1
^ permalink raw reply related
* Re: [net-next 10/16] net/mlx5: Support PCIe buffer congestion handling via Devlink
From: Alexander Duyck @ 2018-07-31 3:19 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Moshe Shemesh, Jiri Pirko, Jakub Kicinski, Eran Ben Elisha,
Saeed Mahameed, David S. Miller, netdev@vger.kernel.org,
linux-pci
In-Reply-To: <20180731023339.GG45322@bhelgaas-glaptop.roam.corp.google.com>
On Mon, Jul 30, 2018 at 7:33 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Mon, Jul 30, 2018 at 08:02:48AM -0700, Alexander Duyck wrote:
>> On Mon, Jul 30, 2018 at 7:07 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>> > On Sun, Jul 29, 2018 at 03:00:28PM -0700, Alexander Duyck wrote:
>> >> On Sun, Jul 29, 2018 at 2:23 AM, Moshe Shemesh <moshes20.il@gmail.com> wrote:
>> >> > On Sat, Jul 28, 2018 at 7:06 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>> >> >> On Thu, Jul 26, 2018 at 07:00:20AM -0700, Alexander Duyck wrote:
>> >> >> > On Thu, Jul 26, 2018 at 12:14 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> >> >> > > Thu, Jul 26, 2018 at 02:43:59AM CEST, jakub.kicinski@netronome.com
>> >> >> > > wrote:
>> >> >> > >>On Wed, 25 Jul 2018 08:23:26 -0700, Alexander Duyck wrote:
>> >> >> > >>> On Wed, Jul 25, 2018 at 5:31 AM, Eran Ben Elisha wrote:
>> >> >> > >>> > On 7/24/2018 10:51 PM, Jakub Kicinski wrote:
>> >> >> > >>> >>>> The devlink params haven't been upstream even for a full cycle
>> >> >> > >>> >>>> and
>> >> >> > >>> >>>> already you guys are starting to use them to configure standard
>> >> >> > >>> >>>> features like queuing.
>> >> >> > >>> >>>
>> >> >> > >>> >>> We developed the devlink params in order to support non-standard
>> >> >> > >>> >>> configuration only. And for non-standard, there are generic and
>> >> >> > >>> >>> vendor
>> >> >> > >>> >>> specific options.
>> >> >> > >>> >>
>> >> >> > >>> >> I thought it was developed for performing non-standard and
>> >> >> > >>> >> possibly
>> >> >> > >>> >> vendor specific configuration. Look at DEVLINK_PARAM_GENERIC_*
>> >> >> > >>> >> for
>> >> >> > >>> >> examples of well justified generic options for which we have no
>> >> >> > >>> >> other API. The vendor mlx4 options look fairly vendor specific
>> >> >> > >>> >> if you
>> >> >> > >>> >> ask me, too.
>> >> >> > >>> >>
>> >> >> > >>> >> Configuring queuing has an API. The question is it acceptable to
>> >> >> > >>> >> enter
>> >> >> > >>> >> into the risky territory of controlling offloads via devlink
>> >> >> > >>> >> parameters
>> >> >> > >>> >> or would we rather make vendors take the time and effort to model
>> >> >> > >>> >> things to (a subset) of existing APIs. The HW never fits the
>> >> >> > >>> >> APIs
>> >> >> > >>> >> perfectly.
>> >> >> > >>> >
>> >> >> > >>> > I understand what you meant here, I would like to highlight that
>> >> >> > >>> > this
>> >> >> > >>> > mechanism was not meant to handle SRIOV, Representors, etc.
>> >> >> > >>> > The vendor specific configuration suggested here is to handle a
>> >> >> > >>> > congestion
>> >> >> > >>> > state in Multi Host environment (which includes PF and multiple
>> >> >> > >>> > VFs per
>> >> >> > >>> > host), where one host is not aware to the other hosts, and each is
>> >> >> > >>> > running
>> >> >> > >>> > on its own pci/driver. It is a device working mode configuration.
>> >> >> > >>> >
>> >> >> > >>> > This couldn't fit into any existing API, thus creating this
>> >> >> > >>> > vendor specific
>> >> >> > >>> > unique API is needed.
>> >> >> > >>>
>> >> >> > >>> If we are just going to start creating devlink interfaces in for
>> >> >> > >>> every
>> >> >> > >>> one-off option a device wants to add why did we even bother with
>> >> >> > >>> trying to prevent drivers from using sysfs? This just feels like we
>> >> >> > >>> are back to the same arguments we had back in the day with it.
>> >> >> > >>>
>> >> >> > >>> I feel like the bigger question here is if devlink is how we are
>> >> >> > >>> going
>> >> >> > >>> to deal with all PCIe related features going forward, or should we
>> >> >> > >>> start looking at creating a new interface/tool for PCI/PCIe related
>> >> >> > >>> features? My concern is that we have already had features such as
>> >> >> > >>> DMA
>> >> >> > >>> Coalescing that didn't really fit into anything and now we are
>> >> >> > >>> starting to see other things related to DMA and PCIe bus credits.
>> >> >> > >>> I'm
>> >> >> > >>> wondering if we shouldn't start looking at a tool/interface to
>> >> >> > >>> configure all the PCIe related features such as interrupts, error
>> >> >> > >>> reporting, DMA configuration, power management, etc. Maybe we could
>> >> >> > >>> even look at sharing it across subsystems and include things like
>> >> >> > >>> storage, graphics, and other subsystems in the conversation.
>> >> >> > >>
>> >> >> > >>Agreed, for actual PCIe configuration (i.e. not ECN marking) we do
>> >> >> > >> need
>> >> >> > >>to build up an API. Sharing it across subsystems would be very cool!
>> >> >>
>> >> >> I read the thread (starting at [1], for anybody else coming in late)
>> >> >> and I see this has something to do with "configuring outbound PCIe
>> >> >> buffers", but I haven't seen the connection to PCIe protocol or
>> >> >> features, i.e., I can't connect this to anything in the PCIe spec.
>> >> >>
>> >> >> Can somebody help me understand how the PCI core is relevant? If
>> >> >> there's some connection with a feature defined by PCIe, or if it
>> >> >> affects the PCIe transaction protocol somehow, I'm definitely
>> >> >> interested in this. But if this only affects the data transferred
>> >> >> over PCIe, i.e., the data payloads of PCIe TLP packets, then I'm not
>> >> >> sure why the PCI core should care.
>> >> >
>> >> > As you wrote, this is not a PCIe feature or affects the PCIe transaction
>> >> > protocol.
>> >> >
>> >> > Actually, due to hardware limitation in current device, we have enabled a
>> >> > workaround in hardware.
>> >> >
>> >> > This mode is proprietary and not relevant to other PCIe devices, thus is set
>> >> > using driver-specific parameter in devlink
>> >>
>> >> Essentially what this feature is doing is communicating the need for
>> >> PCIe back-pressure to the network fabric. So as the buffers on the
>> >> device start to fill because the device isn't able to get back PCIe
>> >> credits fast enough it will then start to send congestion
>> >> notifications to the network stack itself if I understand this
>> >> correctly.
>> >
>> > This sounds like a hook that allows the device to tell its driver
>> > about PCIe flow control credits, and the driver can pass that on to
>> > the network stack. IIUC, that would be a device-specific feature
>> > outside the scope of the PCI core.
>> >
>> >> For now there are no major conflicts, but when we start getting into
>> >> stuff like PCIe DMA coalescing, and on a more general basis just PCIe
>> >> active state power management that is going to start making things
>> >> more complicated going forward.
>> >
>> > We do support ASPM already in the PCI core, and we do have the
>> > pci_disable_link_state() interface, which is currently the only way
>> > drivers can influence it. There are several drivers that do their own
>> > ASPM configuration, but this is not safe because it's not coordinated
>> > with what the PCI core does. If/when drivers need more control, we
>> > should enhance the PCI core interfaces.
>>
>> This is kind of what I was getting at. It would be useful to have an
>> interface of some sort so that drivers get notified when a user is
>> making changes to configuration space and I don't know if anything
>> like that exists now.
>
> You mean something like this?
>
> - driver registers a callback with PCI core
> - user runs setpci, which writes PCI config space via sysfs
> - kernel hook in pci_write_config() notices write and calls driver
> callback
> - driver callback receives config address and data written
> - driver parses PCI capability lists to identify register
>
> Nothing like that exists today, and this is not what I had in mind by
> "enhance the PCI core interfaces". I'm not sure what the utility of
> this is (but I'm not a networking guy by any means).
Well in general I have been wondering if setpci is really the cleanest
way to do any of this. I have found it can be a fast way to really
mess things up. For example using setpci to trigger an FLR is a fast
way to cripple an interface. I recall using that approach when testing
the fm10k driver to deal with surprise resets.
The problem is setpci is REALLY powerful. It lets you change a ton
that can impact the driver, and many drivers just read the config
space at the probe function and assume it is static (which in most
cases it is). That is why I was thinking as a small step it might be
useful to have the notifications delivered to the driver if it is
registered on the interface so it could prepare or clean-up after a
change to the PCI configuration space.
> I think it's a bad idea for drivers to directly write config space.
> It would be much better to provide a PCI core interface so we can
> implement things once and coordinate things that need to be
> coordinated.
I agree with that.
>> > I don't know what PCIe DMA coalescing means, so I can't comment on
>> > that.
>>
>> There are devices, specifically network devices, that will hold off on
>> switching between either L0s or L1 and L0 by deferring DMA operations.
>> Basically the idea is supposed to be to hold off bringing the link up
>> for as long as possible in order to maximize power savings for the
>> ASPM state. This is something that has come up in the past, and I
>> don't know if there has been any interface determined for how to
>> handle this sort of configuration. Most of it occurs through MMIO.
>
> The device can certainly delay L0s or L1 exit if it wants to. If
> there are knobs to control this, e.g., how long it can defer a DMA, it
> makes sense that they would be device-specific and in MMIO space. The
> PCI core can't be involved in that because in general it knows nothing
> about the contents of MMIO BARs. Presumably those knobs would work
> within the framework of ASPM as defined by the PCIe spec, e.g., if we
> disable ASPM, the knobs would do nothing because there is no L0s or L1
> at all.
>
> That's not to say that device designers couldn't get together and
> define a common model for such knobs that puts them at well-known
> offsets in well-known BARs. All I'm saying is that this sounds like
> it's currently outside the realm of the PCI specs and the PCI core.
Some of this was already handled in LTR and OBFF. I think the DMA
coalescing was meant to work either with those, or optionally on its
own. As far as being ASPM dependent or not I don't think it really
cared all that much other than knowing if ASPM was enabled and how
long it takes for a given device to come out of either of those
states. The general idea with DMA coalescing was to try and save power
on the CPU and then possibly the PCIe link if the value was set high
enough and ASPM was enabled.
Anyway we have kind of gotten off into a tangent as I was just citing
something that might end up having an interaction with a feature such
as notifying the stack that the Rx buffer on a given device has become
congested.
>> >> I assume the devices we are talking about supporting this new feature
>> >> on either don't deal with ASPM or assume a quick turnaround to get out
>> >> of the lower power states? Otherwise that would definitely cause some
>> >> back-pressure buildups that would hurt performance.
>> >
>> > Devices can communicate the ASPM exit latency they can tolerate via
>> > the Device Capabilities register (PCIe r4.0, sec 76.5.3.3). Linux
>> > should be configuring ASPM to respect those device requirements.
>>
>> Right. But my point was something like ASPM will add extra complexity
>> to a feature such as what has been described here. My concern is that
>> I don't want us implementing stuff on a per-driver basis that is not
>> all that unique to the device. I don't really see the feature that was
>> described above as being something that will stay specific to this one
>> device for very long, especially if it provides added value. Basically
>> all it is doing is allowing exposing PCIe congestion management to
>> upper levels in the network stack. I don't even necessarily see it as
>> being networking specific as I would imagine there might be other
>> types of devices that could make use of knowing how many transactions
>> and such they could process at the same time.
>
> It sounds to me like you need a library that can be used by all the
> drivers that need this functionality. Unless it's something
> documented in the PCIe specs so we can rely on a standard way of
> discovering and configuring things, I don't see how the PCI core can
> really be involved.
>
> Bjorn
I am kind of thinking that a common library would be a preferred way
to go, or at least a common interface to share between the drivers. It
wasn't my intention to imply that the PCI core needed to get involved.
I was including the linux-pci list as more of a way of checking to get
the broader audience's input since we were just discussing this on
netdev.
My main concern is that those of us on netdev have historically we
have been dealing with features such as SR-IOV as a networking thing,
when really it has been a combination of a PCI feature and some
network switching. I might have ratholed this a bit, as I kind of see
this topic as something similar.
- Alex
^ permalink raw reply
* IT Governance article
From: Reciprocity Labs @ 2018-07-31 3:19 UTC (permalink / raw)
To: Netdev
[-- Attachment #1: Type: text/plain, Size: 2286 bytes --]
Greetings,
My name is Michael and I am reaching out to ubuntu.com on behalf of Reciprocity
Labs
<https://reciprocitylabsmarketing-dot-yamm-track.appspot.com/Redirect?ukey=1EY60qIlcsIVcvRZh2ijOT3QM8F8ybbAaS3TVnW3nR2Q-832499154&key=YAMMID-07151688&link=https%3A%2F%2Freciprocitylabs.com%2F>.
We help companies with compliance and risk management. Recently we've
helped educate people on combating fraud (COSO), data-security and the
handling of sensitive information (GDPR, HIPAA, etc) for enterprises and IT
governance professionals.
When I came across
http://manpages.ubuntu.com/manpages/xenial/en/man7/unicode.7.html
<https://reciprocitylabsmarketing-dot-yamm-track.appspot.com/Redirect?ukey=1EY60qIlcsIVcvRZh2ijOT3QM8F8ybbAaS3TVnW3nR2Q-832499154&key=YAMMID-07151688&link=http%3A%2F%2Fmanpages.ubuntu.com%2Fmanpages%2Fxenial%2Fen%2Fman7%2Funicode.7.html>
I decided to reach out. We'd love to contribute an article to ubuntu.com.
Our aim is to educate your users with content specific to your site!
If you have any guidelines for the article or another idea, send them
along!
I would be happy to set up a time to chat over the phone to answer any
questions or further discuss.
Feel free to check out some of the topics we've been covering in the
meantime:
- https://www.x-cart.com/blog/pci-dss-compliance.html
<https://reciprocitylabsmarketing-dot-yamm-track.appspot.com/Redirect?ukey=1EY60qIlcsIVcvRZh2ijOT3QM8F8ybbAaS3TVnW3nR2Q-832499154&key=YAMMID-07151688&link=https%3A%2F%2Fwww.x-cart.com%2Fblog%2Fpci-dss-compliance.html>
- https://www.intelligenthq.com/resources/iso-certification-needs/
<https://reciprocitylabsmarketing-dot-yamm-track.appspot.com/Redirect?ukey=1EY60qIlcsIVcvRZh2ijOT3QM8F8ybbAaS3TVnW3nR2Q-832499154&key=YAMMID-07151688&link=https%3A%2F%2Fwww.intelligenthq.com%2Fresources%2Fiso-certification-needs%2F>
- https://minutehack.com/opinions/from-soc-compliance-to-soc2-compliance
<https://reciprocitylabsmarketing-dot-yamm-track.appspot.com/Redirect?ukey=1EY60qIlcsIVcvRZh2ijOT3QM8F8ybbAaS3TVnW3nR2Q-832499154&key=YAMMID-07151688&link=https%3A%2F%2Fminutehack.com%2Fopinions%2Ffrom-soc-compliance-to-soc2-compliance>
Thank you and I look forward to working together!
Cheers,
Michael
reciprocitylabsmarketing@gmail.com
[image: beacon]
[-- Attachment #2: Type: text/html, Size: 5881 bytes --]
^ permalink raw reply
* [PATCH net] inet: frag: enforce memory limits earlier
From: Eric Dumazet @ 2018-07-31 3:09 UTC (permalink / raw)
To: David S . Miller
Cc: netdev, Eric Dumazet, Jann Horn, Eric Dumazet, Florian Westphal,
Peter Oskolkov, Paolo Abeni
We currently check current frags memory usage only when
a new frag queue is created. This allows attackers to first
consume the memory budget (default : 4 MB) creating thousands
of frag queues, then sending tiny skbs to exceed high_thresh
limit by 2 to 3 order of magnitude.
Note that before commit 648700f76b03 ("inet: frags: use rhashtables
for reassembly units"), work queue could be starved under DOS,
getting no cpu cycles.
After commit 648700f76b03, only the per frag queue timer can eventually
remove an incomplete frag queue and its skbs.
Fixes: b13d3cbfb8e8 ("inet: frag: move eviction of queues to work queue")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Jann Horn <jannh@google.com>
Cc: Florian Westphal <fw@strlen.de>
Cc: Peter Oskolkov <posk@google.com>
Cc: Paolo Abeni <pabeni@redhat.com>
---
net/ipv4/inet_fragment.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/net/ipv4/inet_fragment.c b/net/ipv4/inet_fragment.c
index 1e4cf3ab560fac154fefb7acd3539eb6e91ed84e..0d70608cc2e18bfa0df35f331e12fbe9b45b168b 100644
--- a/net/ipv4/inet_fragment.c
+++ b/net/ipv4/inet_fragment.c
@@ -157,9 +157,6 @@ static struct inet_frag_queue *inet_frag_alloc(struct netns_frags *nf,
{
struct inet_frag_queue *q;
- if (!nf->high_thresh || frag_mem_limit(nf) > nf->high_thresh)
- return NULL;
-
q = kmem_cache_zalloc(f->frags_cachep, GFP_ATOMIC);
if (!q)
return NULL;
@@ -204,6 +201,9 @@ struct inet_frag_queue *inet_frag_find(struct netns_frags *nf, void *key)
{
struct inet_frag_queue *fq;
+ if (!nf->high_thresh || frag_mem_limit(nf) > nf->high_thresh)
+ return NULL;
+
rcu_read_lock();
fq = rhashtable_lookup(&nf->rhashtable, key, nf->f->rhash_params);
--
2.18.0.345.g5c9ce644c3-goog
^ permalink raw reply related
* Re: [RFC bpf-next 0/6] net: xsk: minor improvements around queue handling
From: Jakub Kicinski @ 2018-07-31 2:49 UTC (permalink / raw)
To: Björn Töpel
Cc: Alexei Starovoitov, Daniel Borkmann, Jesper Dangaard Brouer,
Björn Töpel, Karlsson, Magnus, oss-drivers, Netdev,
Ilias Apalodimas, Francois Ozog, MykytaI Iziumtsev
In-Reply-To: <CAJ+HfNj2uePj1Ec5+WvexQ=gR-G5E-X0uJU4_e+mmDN-p2PXSw@mail.gmail.com>
On Mon, 30 Jul 2018 14:49:32 +0200, Björn Töpel wrote:
> Den tors 26 juli 2018 kl 23:44 skrev Jakub Kicinski:
> >
> > Hi!
>
> Thanks for spending your time on this, Jakub. I'm (temporarily) back
> for a week, so you can expect faster replies now...
>
> > This set tries to make the core take care of error checking for the
> > drivers. In particular making sure that the AF_XDP UMEM is not installed
> > on queues which don't exist (or are disabled) and that changing queue
> > (AKA ethtool channel) count cannot disable queues with active AF_XDF
> > zero-copy sockets.
> >
> > I'm sending as an RFC because I'm not entirely sure what the desired
> > behaviour is here. Is it Okay to install AF_XDP on queues which don't
> > exist? I presume not?
>
> Your presumption is correct. The idea with the
> real_num_rx_queues/real_num_tx_queues check in xsk_bind, is to bail
> out if the queue doesn't exist at bind call. Note that we *didn't* add
> any code to avoid the bound queue from being removed via set channel
> (your patch 6). Our idea was that if you remove a queue, the ingress
> frames would simply stop flowing, and the queue config change checking
> was "out-of-band".
>
> I think I prefer your approach, i.e. not allowing the channels/queues
> to change if they're bound to an AF_XDP socket. However, your
> xdp_umem_query used in ethtool only works for ZC enabled drivers, not
> for the existing non-ZC/copy case. If we'd like to go the route of
> disabling ethtool_set_channels for an AF_XDP enabled queue this
> functionality needs to move the query into netdev core, so we have a
> consistent behavior.
Agreed. There seems to be no notification for changing the number of
queues and therefore no very clean way to solve this today. The last
two patches are more of a courtesy to the drivers, to simplify the
data structure for holding the UMEMs.
I could argue that driver and stack are not really apples to apples.
Much like Generic XDP, the skb-based AF_XDP is basically a development
tool and last-resort fallback. For TX driver will most likely allocate
separate queues, while skb-based will use the stack's queues. These
are actually different queues. Stack will also fallback to other queue
in __netdev_pick_tx() if number of queues changes.
But yes, preferably skb-based and ZC should behave the same..
> > Are the AF_XDP queue_ids referring to TX queues
> > as well as RX queues in case of the driver? I presume not?
>
> We've had a lot of discussions about this internally. Ideally, we'd
> like to give driver implementors the most freedom, and not enforcing a
> certain queue scheme for Tx.
You say freedom I hear diverging implementations and per-driver
checks in user space ;-)
Practically speaking unless you take the xmit lock there is little
chance of reusing stack's TX queues, so you'd have to allocate a
separate queue one way or the other.. At which point the number of
stack's TX queues has no bearing on AF_XDP ZC.
> OTOH it makes it weird for the userland
> application *not* to have the same id, e.g. if a userland application
> would like to get stats or configure the AF_XDP bound Tx queue --
> which id is it? Should the Tx queue id for an xsk be exposed in
> sysfs?
I'd not go there.
> If the id is *not* the same, would it be OK to change the number of
> channels and Tx would continue to operate correctly? A related
> question; An xsk with *only* Tx, should it be constrained by the
> number of (enabled) Rx queues?
Good question, are drivers even supposed to care about tx-only/rx-only?
From driver's perspective rx-only socket will simply never have
anything to transmit, and tx-only socket will never successfully
xdp_do_redirect(). So IMHO - yes, tx-only XSK still only cares about
RX queues, driver doesn't know no RX will ever happen. The fact that
user has to populate the FILL queue on a tx-only socket may be counter
intuitive to many...
I was considering proposing a change to drop the *x-only option in the
net tree, I don't really see much use for it :S And it potentially
creates weird corner cases.
> I'd be happy to hear some more opinions/thoughts on this...
+1
> > Should we try to prevent disabling queues which have non zero-copy
> > sockets installed as well? :S
> >
>
> Yes, the ZC/non-ZC case must be consistent IMO. See comment above.
>
> > Anyway, if any of those patches seem useful and reasonable, please
> > let me know I will repost as non-RFC.
> >
>
> I definitely think patch 2 and 3 (and probably 1) should go as
> non-RFC!
Thanks Björn, I will submit the first three!
> Thanks for spotting that we're not holding the rtnl lock when checking
> the # queues (patch 4)!
>
> Björn
^ permalink raw reply
* Re: [PATCH 2/5] rhashtable: don't hold lock on first table throughout insertion.
From: Paul E. McKenney @ 2018-07-31 4:14 UTC (permalink / raw)
To: NeilBrown; +Cc: Herbert Xu, Thomas Graf, netdev, linux-kernel
In-Reply-To: <87zhy8s05i.fsf@notabene.neil.brown.name>
On Tue, Jul 31, 2018 at 10:45:45AM +1000, NeilBrown wrote:
> On Fri, Jul 27 2018, Paul E. McKenney wrote:
>
> > On Thu, Jul 26, 2018 at 08:18:15PM -0700, Paul E. McKenney wrote:
> >> On Fri, Jul 27, 2018 at 11:04:37AM +1000, NeilBrown wrote:
> >> > On Wed, Jul 25 2018, Paul E. McKenney wrote:
> >> > >>
> >> > >> Looks good ... except ... naming is hard.
> >> > >>
> >> > >> is_after_call_rcu_init() asserts where in the lifecycle we are,
> >> > >> is_after_call_rcu() tests where in the lifecycle we are.
> >> > >>
> >> > >> The names are similar but the purpose is quite different.
> >> > >> Maybe s/is_after_call_rcu_init/call_rcu_init/ ??
> >> > >
> >> > > How about rcu_head_init() and rcu_head_after_call_rcu()?
> >>
> >> Very well, I will pull this change in on my next rebase.
> >
> > Like this?
>
> Hard to say - unwinding white-space damage in my head is too challenging
> when newlines have been deleted :-(
What??? Don't you like block-structured code?
All kidding aside, how about the following more conventionally formatted
version?
Thanx, Paul
------------------------------------------------------------------------
commit e3408141ed7d702995b2fdc94703af88aadd226b
Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Date: Tue Jul 24 15:28:09 2018 -0700
rcu: Provide functions for determining if call_rcu() has been invoked
This commit adds rcu_head_init() and rcu_head_after_call_rcu() functions
to help RCU users detect when another CPU has passed the specified
rcu_head structure and function to call_rcu(). The rcu_head_init()
should be invoked before making the structure visible to RCU readers,
and then the rcu_head_after_call_rcu() may be invoked from within
an RCU read-side critical section on an rcu_head structure that
was obtained during a traversal of the data structure in question.
The rcu_head_after_call_rcu() function will return true if the rcu_head
structure has already been passed (with the specified function) to
call_rcu(), otherwise it will return false.
If rcu_head_init() has not been invoked on the rcu_head structure
or if the rcu_head (AKA callback) has already been invoked, then
rcu_head_after_call_rcu() will do WARN_ON_ONCE().
Reported-by: NeilBrown <neilb@suse.com>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
[ paulmck: Apply neilb naming feedback. ]
diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index e4f821165d0b..4db8bcacc51a 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -857,6 +857,46 @@ static inline notrace void rcu_read_unlock_sched_notrace(void)
#endif /* #else #ifdef CONFIG_ARCH_WEAK_RELEASE_ACQUIRE */
+/* Has the specified rcu_head structure been handed to call_rcu()? */
+
+/*
+ * rcu_head_init - Initialize rcu_head for rcu_head_after_call_rcu()
+ * @rhp: The rcu_head structure to initialize.
+ *
+ * If you intend to invoke rcu_head_after_call_rcu() to test whether a
+ * given rcu_head structure has already been passed to call_rcu(), then
+ * you must also invoke this rcu_head_init() function on it just after
+ * allocating that structure. Calls to this function must not race with
+ * calls to call_rcu(), rcu_head_after_call_rcu(), or callback invocation.
+ */
+static inline void rcu_head_init(struct rcu_head *rhp)
+{
+ rhp->func = (rcu_callback_t)~0L;
+}
+
+/*
+ * rcu_head_after_call_rcu - Has this rcu_head been passed to call_rcu()?
+ * @rhp: The rcu_head structure to test.
+ * @func: The function passed to call_rcu() along with @rhp.
+ *
+ * Returns @true if the @rhp has been passed to call_rcu() with @func,
+ * and @false otherwise. Emits a warning in any other case, including
+ * the case where @rhp has already been invoked after a grace period.
+ * Calls to this function must not race with callback invocation. One way
+ * to avoid such races is to enclose the call to rcu_head_after_call_rcu()
+ * in an RCU read-side critical section that includes a read-side fetch
+ * of the pointer to the structure containing @rhp.
+ */
+static inline bool
+rcu_head_after_call_rcu(struct rcu_head *rhp, rcu_callback_t f)
+{
+ if (READ_ONCE(rhp->func) == f)
+ return true;
+ WARN_ON_ONCE(READ_ONCE(rhp->func) != (rcu_callback_t)~0L);
+ return false;
+}
+
+
/* Transitional pre-consolidation compatibility definitions. */
static inline void synchronize_rcu_bh(void)
diff --git a/kernel/rcu/rcu.h b/kernel/rcu/rcu.h
index 5dec94509a7e..4c56c1d98fb3 100644
--- a/kernel/rcu/rcu.h
+++ b/kernel/rcu/rcu.h
@@ -224,6 +224,7 @@ void kfree(const void *);
*/
static inline bool __rcu_reclaim(const char *rn, struct rcu_head *head)
{
+ rcu_callback_t f;
unsigned long offset = (unsigned long)head->func;
rcu_lock_acquire(&rcu_callback_map);
@@ -234,7 +235,9 @@ static inline bool __rcu_reclaim(const char *rn, struct rcu_head *head)
return true;
} else {
RCU_TRACE(trace_rcu_invoke_callback(rn, head);)
- head->func(head);
+ f = head->func;
+ WRITE_ONCE(head->func, (rcu_callback_t)0L);
+ f(head);
rcu_lock_release(&rcu_callback_map);
return false;
}
^ permalink raw reply related
* Re: [net-next 10/16] net/mlx5: Support PCIe buffer congestion handling via Devlink
From: Bjorn Helgaas @ 2018-07-31 2:33 UTC (permalink / raw)
To: Alexander Duyck
Cc: Moshe Shemesh, Jiri Pirko, Jakub Kicinski, Eran Ben Elisha,
Saeed Mahameed, David S. Miller, netdev@vger.kernel.org,
linux-pci
In-Reply-To: <CAKgT0UcbXT6EXt=zwoOzZ-wm7uS70wvm9xMcsoSf6OzuJsLxMQ@mail.gmail.com>
On Mon, Jul 30, 2018 at 08:02:48AM -0700, Alexander Duyck wrote:
> On Mon, Jul 30, 2018 at 7:07 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Sun, Jul 29, 2018 at 03:00:28PM -0700, Alexander Duyck wrote:
> >> On Sun, Jul 29, 2018 at 2:23 AM, Moshe Shemesh <moshes20.il@gmail.com> wrote:
> >> > On Sat, Jul 28, 2018 at 7:06 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> >> >> On Thu, Jul 26, 2018 at 07:00:20AM -0700, Alexander Duyck wrote:
> >> >> > On Thu, Jul 26, 2018 at 12:14 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> >> >> > > Thu, Jul 26, 2018 at 02:43:59AM CEST, jakub.kicinski@netronome.com
> >> >> > > wrote:
> >> >> > >>On Wed, 25 Jul 2018 08:23:26 -0700, Alexander Duyck wrote:
> >> >> > >>> On Wed, Jul 25, 2018 at 5:31 AM, Eran Ben Elisha wrote:
> >> >> > >>> > On 7/24/2018 10:51 PM, Jakub Kicinski wrote:
> >> >> > >>> >>>> The devlink params haven't been upstream even for a full cycle
> >> >> > >>> >>>> and
> >> >> > >>> >>>> already you guys are starting to use them to configure standard
> >> >> > >>> >>>> features like queuing.
> >> >> > >>> >>>
> >> >> > >>> >>> We developed the devlink params in order to support non-standard
> >> >> > >>> >>> configuration only. And for non-standard, there are generic and
> >> >> > >>> >>> vendor
> >> >> > >>> >>> specific options.
> >> >> > >>> >>
> >> >> > >>> >> I thought it was developed for performing non-standard and
> >> >> > >>> >> possibly
> >> >> > >>> >> vendor specific configuration. Look at DEVLINK_PARAM_GENERIC_*
> >> >> > >>> >> for
> >> >> > >>> >> examples of well justified generic options for which we have no
> >> >> > >>> >> other API. The vendor mlx4 options look fairly vendor specific
> >> >> > >>> >> if you
> >> >> > >>> >> ask me, too.
> >> >> > >>> >>
> >> >> > >>> >> Configuring queuing has an API. The question is it acceptable to
> >> >> > >>> >> enter
> >> >> > >>> >> into the risky territory of controlling offloads via devlink
> >> >> > >>> >> parameters
> >> >> > >>> >> or would we rather make vendors take the time and effort to model
> >> >> > >>> >> things to (a subset) of existing APIs. The HW never fits the
> >> >> > >>> >> APIs
> >> >> > >>> >> perfectly.
> >> >> > >>> >
> >> >> > >>> > I understand what you meant here, I would like to highlight that
> >> >> > >>> > this
> >> >> > >>> > mechanism was not meant to handle SRIOV, Representors, etc.
> >> >> > >>> > The vendor specific configuration suggested here is to handle a
> >> >> > >>> > congestion
> >> >> > >>> > state in Multi Host environment (which includes PF and multiple
> >> >> > >>> > VFs per
> >> >> > >>> > host), where one host is not aware to the other hosts, and each is
> >> >> > >>> > running
> >> >> > >>> > on its own pci/driver. It is a device working mode configuration.
> >> >> > >>> >
> >> >> > >>> > This couldn't fit into any existing API, thus creating this
> >> >> > >>> > vendor specific
> >> >> > >>> > unique API is needed.
> >> >> > >>>
> >> >> > >>> If we are just going to start creating devlink interfaces in for
> >> >> > >>> every
> >> >> > >>> one-off option a device wants to add why did we even bother with
> >> >> > >>> trying to prevent drivers from using sysfs? This just feels like we
> >> >> > >>> are back to the same arguments we had back in the day with it.
> >> >> > >>>
> >> >> > >>> I feel like the bigger question here is if devlink is how we are
> >> >> > >>> going
> >> >> > >>> to deal with all PCIe related features going forward, or should we
> >> >> > >>> start looking at creating a new interface/tool for PCI/PCIe related
> >> >> > >>> features? My concern is that we have already had features such as
> >> >> > >>> DMA
> >> >> > >>> Coalescing that didn't really fit into anything and now we are
> >> >> > >>> starting to see other things related to DMA and PCIe bus credits.
> >> >> > >>> I'm
> >> >> > >>> wondering if we shouldn't start looking at a tool/interface to
> >> >> > >>> configure all the PCIe related features such as interrupts, error
> >> >> > >>> reporting, DMA configuration, power management, etc. Maybe we could
> >> >> > >>> even look at sharing it across subsystems and include things like
> >> >> > >>> storage, graphics, and other subsystems in the conversation.
> >> >> > >>
> >> >> > >>Agreed, for actual PCIe configuration (i.e. not ECN marking) we do
> >> >> > >> need
> >> >> > >>to build up an API. Sharing it across subsystems would be very cool!
> >> >>
> >> >> I read the thread (starting at [1], for anybody else coming in late)
> >> >> and I see this has something to do with "configuring outbound PCIe
> >> >> buffers", but I haven't seen the connection to PCIe protocol or
> >> >> features, i.e., I can't connect this to anything in the PCIe spec.
> >> >>
> >> >> Can somebody help me understand how the PCI core is relevant? If
> >> >> there's some connection with a feature defined by PCIe, or if it
> >> >> affects the PCIe transaction protocol somehow, I'm definitely
> >> >> interested in this. But if this only affects the data transferred
> >> >> over PCIe, i.e., the data payloads of PCIe TLP packets, then I'm not
> >> >> sure why the PCI core should care.
> >> >
> >> > As you wrote, this is not a PCIe feature or affects the PCIe transaction
> >> > protocol.
> >> >
> >> > Actually, due to hardware limitation in current device, we have enabled a
> >> > workaround in hardware.
> >> >
> >> > This mode is proprietary and not relevant to other PCIe devices, thus is set
> >> > using driver-specific parameter in devlink
> >>
> >> Essentially what this feature is doing is communicating the need for
> >> PCIe back-pressure to the network fabric. So as the buffers on the
> >> device start to fill because the device isn't able to get back PCIe
> >> credits fast enough it will then start to send congestion
> >> notifications to the network stack itself if I understand this
> >> correctly.
> >
> > This sounds like a hook that allows the device to tell its driver
> > about PCIe flow control credits, and the driver can pass that on to
> > the network stack. IIUC, that would be a device-specific feature
> > outside the scope of the PCI core.
> >
> >> For now there are no major conflicts, but when we start getting into
> >> stuff like PCIe DMA coalescing, and on a more general basis just PCIe
> >> active state power management that is going to start making things
> >> more complicated going forward.
> >
> > We do support ASPM already in the PCI core, and we do have the
> > pci_disable_link_state() interface, which is currently the only way
> > drivers can influence it. There are several drivers that do their own
> > ASPM configuration, but this is not safe because it's not coordinated
> > with what the PCI core does. If/when drivers need more control, we
> > should enhance the PCI core interfaces.
>
> This is kind of what I was getting at. It would be useful to have an
> interface of some sort so that drivers get notified when a user is
> making changes to configuration space and I don't know if anything
> like that exists now.
You mean something like this?
- driver registers a callback with PCI core
- user runs setpci, which writes PCI config space via sysfs
- kernel hook in pci_write_config() notices write and calls driver
callback
- driver callback receives config address and data written
- driver parses PCI capability lists to identify register
Nothing like that exists today, and this is not what I had in mind by
"enhance the PCI core interfaces". I'm not sure what the utility of
this is (but I'm not a networking guy by any means).
I think it's a bad idea for drivers to directly write config space.
It would be much better to provide a PCI core interface so we can
implement things once and coordinate things that need to be
coordinated.
> > I don't know what PCIe DMA coalescing means, so I can't comment on
> > that.
>
> There are devices, specifically network devices, that will hold off on
> switching between either L0s or L1 and L0 by deferring DMA operations.
> Basically the idea is supposed to be to hold off bringing the link up
> for as long as possible in order to maximize power savings for the
> ASPM state. This is something that has come up in the past, and I
> don't know if there has been any interface determined for how to
> handle this sort of configuration. Most of it occurs through MMIO.
The device can certainly delay L0s or L1 exit if it wants to. If
there are knobs to control this, e.g., how long it can defer a DMA, it
makes sense that they would be device-specific and in MMIO space. The
PCI core can't be involved in that because in general it knows nothing
about the contents of MMIO BARs. Presumably those knobs would work
within the framework of ASPM as defined by the PCIe spec, e.g., if we
disable ASPM, the knobs would do nothing because there is no L0s or L1
at all.
That's not to say that device designers couldn't get together and
define a common model for such knobs that puts them at well-known
offsets in well-known BARs. All I'm saying is that this sounds like
it's currently outside the realm of the PCI specs and the PCI core.
> >> I assume the devices we are talking about supporting this new feature
> >> on either don't deal with ASPM or assume a quick turnaround to get out
> >> of the lower power states? Otherwise that would definitely cause some
> >> back-pressure buildups that would hurt performance.
> >
> > Devices can communicate the ASPM exit latency they can tolerate via
> > the Device Capabilities register (PCIe r4.0, sec 76.5.3.3). Linux
> > should be configuring ASPM to respect those device requirements.
>
> Right. But my point was something like ASPM will add extra complexity
> to a feature such as what has been described here. My concern is that
> I don't want us implementing stuff on a per-driver basis that is not
> all that unique to the device. I don't really see the feature that was
> described above as being something that will stay specific to this one
> device for very long, especially if it provides added value. Basically
> all it is doing is allowing exposing PCIe congestion management to
> upper levels in the network stack. I don't even necessarily see it as
> being networking specific as I would imagine there might be other
> types of devices that could make use of knowing how many transactions
> and such they could process at the same time.
It sounds to me like you need a library that can be used by all the
drivers that need this functionality. Unless it's something
documented in the PCIe specs so we can rely on a standard way of
discovering and configuring things, I don't see how the PCI core can
really be involved.
Bjorn
^ permalink raw reply
* Re: [PATCH v2] Add BPF_SYNCHRONIZE_MAPS bpf(2) command
From: Y Song @ 2018-07-31 4:03 UTC (permalink / raw)
To: Joel Fernandes
Cc: Alexei Starovoitov, Daniel Colascione, Joel Fernandes, LKML,
Tim Murray, Network Development, Lorenzo Colitti, Chenbo Feng,
Mathieu Desnoyers, Alexei Starovoitov, Daniel Borkmann
In-Reply-To: <20180731020629.GB22311@joelaf.mtv.corp.google.com>
On Mon, Jul 30, 2018 at 7:06 PM, Joel Fernandes <joel@joelfernandes.org> wrote:
> On Mon, Jul 30, 2018 at 07:01:22PM -0700, Joel Fernandes wrote:
>> On Sun, Jul 29, 2018 at 06:51:18PM +0300, Alexei Starovoitov wrote:
>> > On Thu, Jul 26, 2018 at 7:51 PM, Daniel Colascione <dancol@google.com> wrote:
>> > > BPF_SYNCHRONIZE_MAPS waits for the release of any references to a BPF
>> > > map made by a BPF program that is running at the time the
>> > > BPF_SYNCHRONIZE_MAPS command is issued. The purpose of this command is
>> > > to provide a means for userspace to replace a BPF map with another,
>> > > newer version, then ensure that no component is still using the "old"
>> > > map before manipulating the "old" map in some way.
>> > >
>> > > Signed-off-by: Daniel Colascione <dancol@google.com>
>> > > ---
>> > > include/uapi/linux/bpf.h | 9 +++++++++
>> > > kernel/bpf/syscall.c | 13 +++++++++++++
>> > > 2 files changed, 22 insertions(+)
>> > >
>> > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>> > > index b7db3261c62d..5b27e9117d3e 100644
>> > > --- a/include/uapi/linux/bpf.h
>> > > +++ b/include/uapi/linux/bpf.h
>> > > @@ -75,6 +75,14 @@ struct bpf_lpm_trie_key {
>> > > __u8 data[0]; /* Arbitrary size */
>> > > };
>> > >
>> > > +/* BPF_SYNCHRONIZE_MAPS waits for the release of any references to a
>> > > + * BPF map made by a BPF program that is running at the time the
>> > > + * BPF_SYNCHRONIZE_MAPS command is issued. The purpose of this command
>> >
>> > that doesn't sound right to me.
>> > such command won't wait for the release of the references.
>> > in case of map-in-map the program does not hold
>> > the references to inner map (only to outer map).
>>
>> I didn't follow this completely.
>>
>> The userspace program is using the inner map per your description of the
>
> Sorry just to correct myself, here I meant "The kernel eBPF program is using
> the inner map on multiple CPUs" instead of "userspace".
>
> thanks,
>
> - Joel
>
>
>
>
>
>> algorithm for using map-in-map to solve the race conditions that this patch
>> is trying to address:
>>
>> If you don't mind, I copy-pasted it below from your netdev post:
>>
>> if you use map-in-map you don't need extra boolean map.
>> 0. bpf prog can do
>> inner_map = lookup(map_in_map, key=0);
>> lookup(inner_map, your_real_key);
>> 1. user space writes into map_in_map[0] <- FD of new map
>> 2. some cpus are using old inner map and some a new
>> 3. user space does sys_membarrier(CMD_GLOBAL) which will do synchronize_sched()
>> which in CONFIG_PREEMPT_NONE=y servers is the same as synchronize_rcu()
>> which will guarantee that progs finished.
>> 4. scan old inner map
>>
>> In step 2, as you mentioned there are CPUs using different inner maps. So
>> could you clarify how the synchronize_rcu mechanism will even work if you're
>> now saying "program does not hold references to the inner maps"?
The program only held references to the outer maps, and the outer map
held references to the inner maps. The user space program can add/remove
the inner map for a particular outer map while the prog <-> outer-map
relationship is not changed.
>>
>> Thanks!
>>
>> - Joel
>>
^ permalink raw reply
* Re: [PATCH v2] Add BPF_SYNCHRONIZE_MAPS bpf(2) command
From: Joel Fernandes @ 2018-07-31 2:01 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Daniel Colascione, Joel Fernandes, LKML, Tim Murray,
Network Development, Lorenzo Colitti, Chenbo Feng,
Mathieu Desnoyers, Alexei Starovoitov, Daniel Borkmann
In-Reply-To: <CAADnVQL4twmGF3d26TgLsQJvnqLnM0F6hqL_42DSs95OuZ1y9A@mail.gmail.com>
On Sun, Jul 29, 2018 at 06:51:18PM +0300, Alexei Starovoitov wrote:
> On Thu, Jul 26, 2018 at 7:51 PM, Daniel Colascione <dancol@google.com> wrote:
> > BPF_SYNCHRONIZE_MAPS waits for the release of any references to a BPF
> > map made by a BPF program that is running at the time the
> > BPF_SYNCHRONIZE_MAPS command is issued. The purpose of this command is
> > to provide a means for userspace to replace a BPF map with another,
> > newer version, then ensure that no component is still using the "old"
> > map before manipulating the "old" map in some way.
> >
> > Signed-off-by: Daniel Colascione <dancol@google.com>
> > ---
> > include/uapi/linux/bpf.h | 9 +++++++++
> > kernel/bpf/syscall.c | 13 +++++++++++++
> > 2 files changed, 22 insertions(+)
> >
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index b7db3261c62d..5b27e9117d3e 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -75,6 +75,14 @@ struct bpf_lpm_trie_key {
> > __u8 data[0]; /* Arbitrary size */
> > };
> >
> > +/* BPF_SYNCHRONIZE_MAPS waits for the release of any references to a
> > + * BPF map made by a BPF program that is running at the time the
> > + * BPF_SYNCHRONIZE_MAPS command is issued. The purpose of this command
>
> that doesn't sound right to me.
> such command won't wait for the release of the references.
> in case of map-in-map the program does not hold
> the references to inner map (only to outer map).
I didn't follow this completely.
The userspace program is using the inner map per your description of the
algorithm for using map-in-map to solve the race conditions that this patch
is trying to address:
If you don't mind, I copy-pasted it below from your netdev post:
if you use map-in-map you don't need extra boolean map.
0. bpf prog can do
inner_map = lookup(map_in_map, key=0);
lookup(inner_map, your_real_key);
1. user space writes into map_in_map[0] <- FD of new map
2. some cpus are using old inner map and some a new
3. user space does sys_membarrier(CMD_GLOBAL) which will do synchronize_sched()
which in CONFIG_PREEMPT_NONE=y servers is the same as synchronize_rcu()
which will guarantee that progs finished.
4. scan old inner map
In step 2, as you mentioned there are CPUs using different inner maps. So
could you clarify how the synchronize_rcu mechanism will even work if you're
now saying "program does not hold references to the inner maps"?
Thanks!
- Joel
^ permalink raw reply
* Attn: IT Report
From: Diane Chang @ 2018-07-31 1:28 UTC (permalink / raw)
[-- Attachment #1: Type: text/plain, Size: 413 bytes --]
Dear User
This is a courtesy notice by Admin Team, and to inform you that your e-mail account has exceeded its quota of mail on the database server.
E-mail accounts will be blocked from sending and receiving emails if your email account is not verified.
Click on<https://goo.gl/cesfD4> to restore access to unlock your e-mail account.
Restore Access
Douglas Barr
Webmail Team
[-- Attachment #2: Type: text/html, Size: 3180 bytes --]
^ permalink raw reply
* Re: [PATCH net-next] virtio_net: force_napi_tx module param.
From: Jason Wang @ 2018-07-31 1:41 UTC (permalink / raw)
To: Stephen Hemminger, David Miller; +Cc: caleb.raitto, mst, netdev, caraitto
In-Reply-To: <20180730125146.19c37975@xeon-e3>
On 2018年07月31日 03:51, Stephen Hemminger wrote:
> On Sun, 29 Jul 2018 09:00:27 -0700 (PDT)
> David Miller <davem@davemloft.net> wrote:
>
>> From: Caleb Raitto <caleb.raitto@gmail.com>
>> Date: Mon, 23 Jul 2018 16:11:19 -0700
>>
>>> From: Caleb Raitto <caraitto@google.com>
>>>
>>> The driver disables tx napi if it's not certain that completions will
>>> be processed affine with tx service.
>>>
>>> Its heuristic doesn't account for some scenarios where it is, such as
>>> when the queue pair count matches the core but not hyperthread count.
>>>
>>> Allow userspace to override the heuristic. This is an alternative
>>> solution to that in the linked patch. That added more logic in the
>>> kernel for these cases, but the agreement was that this was better left
>>> to user control.
>>>
>>> Do not expand the existing napi_tx variable to a ternary value,
>>> because doing so can break user applications that expect
>>> boolean ('Y'/'N') instead of integer output. Add a new param instead.
>>>
>>> Link: https://patchwork.ozlabs.org/patch/725249/
>>> Acked-by: Willem de Bruijn <willemb@google.com>
>>> Acked-by: Jon Olson <jonolson@google.com>
>>> Signed-off-by: Caleb Raitto <caraitto@google.com>
>> So I looked into the history surrounding these issues.
>>
>> First of all, it's always ends up turning out crummy when drivers start
>> to set affinities themselves. The worst possible case is to do it
>> _conditionally_, and that is exactly what virtio_net is doing.
>>
>> From the user's perspective, this provides a really bad experience.
>>
>> So if I have a 32-queue device and there are 32 cpus, you'll do all
>> the affinity settings, stopping Irqbalanced from doing anything
>> right?
>>
>> So if I add one more cpu, you'll say "oops, no idea what to do in
>> this situation" and not touch the affinities at all?
>>
>> That makes no sense at all.
>>
>> If the driver is going to set affinities at all, OWN that decision
>> and set it all the time to something reasonable.
>>
>> Or accept that you shouldn't be touching this stuff in the first place
>> and leave the affinities alone.
>>
>> Right now we're kinda in a situation where the driver has been setting
>> affinities in the ncpus==nqueues cases for some time, so we can't stop
>> doing it.
>>
>> Which means we have to set them in all cases to make the user
>> experience sane again.
>>
>> I looked at the linked to patch again:
>>
>> https://patchwork.ozlabs.org/patch/725249/
>>
>> And I think the strategy should be made more generic, to get rid of
>> the hyperthreading assumptions. I also agree that the "assign
>> to first N cpus" logic doesn't make much sense either.
>>
>> Just distribute across the available cpus evenly, and be done with it.
>> If you have 64 cpus and 32 queues, this assigns queues to every other
>> cpu.
>>
>> Then we don't need this weird new module parameter.
> I wonder if it would be possible to give irqbalanced hints
> with irq_set_affinity_hint instead of doing direct affinity setting?
We do use hints instead of affinity directly.
See vp_set_vq_affinity(), which did:
if (vp_dev->msix_enabled) {
mask = vp_dev->msix_affinity_masks[info->msix_vector];
irq = pci_irq_vector(vp_dev->pci_dev, info->msix_vector);
if (cpu == -1)
irq_set_affinity_hint(irq, NULL);
else {
cpumask_clear(mask);
cpumask_set_cpu(cpu, mask);
irq_set_affinity_hint(irq, mask);
}
}
Thanks.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox