* 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
* Re: [RFC PATCH net-next v2 00/17] ethtool netlink interface (WiP)
From: Jakub Kicinski @ 2018-07-31 0:38 UTC (permalink / raw)
To: Jiri Pirko
Cc: Michal Kubecek, netdev, linux-kernel, David Miller,
Florian Fainelli, Roopa Prabhu, John W. Linville
In-Reply-To: <20180730130747.GH2058@nanopsycho>
On Mon, 30 Jul 2018 15:07:47 +0200, Jiri Pirko wrote:
> >- find reasonable format for data transfers (e.g. eeprom dump or flash);
> > I don't have clear idea how big these can get and if 64 KB limit on
> > attribute size (including nested ones) is a problem; if so, dumps can
> > be an answer for dumps, some kind of multi-message requests would be
> > needed for flashes
>
> This is another thing that should go into devlink as it is not
> netdev-handled. We have now a "region" facility there which can be
> re-used.
+1 regions should cover the current use cases and are a better API IMHO.
^ permalink raw reply
* [PATCH v2 bpf-next 4/4] selftests/bpf: Test for get_socket_cookie
From: Andrey Ignatov @ 2018-07-31 0:42 UTC (permalink / raw)
To: netdev; +Cc: Andrey Ignatov, ast, daniel, yhs, brakmo, kernel-team
In-Reply-To: <cover.1532996972.git.rdna@fb.com>
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>
---
tools/testing/selftests/bpf/Makefile | 6 +-
.../selftests/bpf/socket_cookie_prog.c | 60 +++++
.../selftests/bpf/test_socket_cookie.c | 225 ++++++++++++++++++
3 files changed, 289 insertions(+), 2 deletions(-)
create mode 100644 tools/testing/selftests/bpf/socket_cookie_prog.c
create mode 100644 tools/testing/selftests/bpf/test_socket_cookie.c
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 478bf1bcbbf5..1b28277998e2 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -22,7 +22,8 @@ $(TEST_CUSTOM_PROGS): $(OUTPUT)/%: %.c
# Order correspond to 'make run_tests' order
TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test_progs \
test_align test_verifier_log test_dev_cgroup test_tcpbpf_user \
- test_sock test_btf test_sockmap test_lirc_mode2_user get_cgroup_id_user
+ test_sock test_btf test_sockmap test_lirc_mode2_user get_cgroup_id_user \
+ test_socket_cookie
TEST_GEN_FILES = test_pkt_access.o test_xdp.o test_l4lb.o test_tcp_estats.o test_obj_id.o \
test_pkt_md_access.o test_xdp_redirect.o test_xdp_meta.o sockmap_parse_prog.o \
@@ -33,7 +34,7 @@ TEST_GEN_FILES = test_pkt_access.o test_xdp.o test_l4lb.o test_tcp_estats.o test
test_btf_haskv.o test_btf_nokv.o test_sockmap_kern.o test_tunnel_kern.o \
test_get_stack_rawtp.o test_sockmap_kern.o test_sockhash_kern.o \
test_lwt_seg6local.o sendmsg4_prog.o sendmsg6_prog.o test_lirc_mode2_kern.o \
- get_cgroup_id_kern.o
+ get_cgroup_id_kern.o socket_cookie_prog.o
# Order correspond to 'make run_tests' order
TEST_PROGS := test_kmod.sh \
@@ -60,6 +61,7 @@ $(TEST_GEN_PROGS_EXTENDED): $(OUTPUT)/libbpf.a
$(OUTPUT)/test_dev_cgroup: cgroup_helpers.c
$(OUTPUT)/test_sock: cgroup_helpers.c
$(OUTPUT)/test_sock_addr: cgroup_helpers.c
+$(OUTPUT)/test_socket_cookie: cgroup_helpers.c
$(OUTPUT)/test_sockmap: cgroup_helpers.c
$(OUTPUT)/test_tcpbpf_user: cgroup_helpers.c
$(OUTPUT)/test_progs: trace_helpers.c
diff --git a/tools/testing/selftests/bpf/socket_cookie_prog.c b/tools/testing/selftests/bpf/socket_cookie_prog.c
new file mode 100644
index 000000000000..9ff8ac4b0bf6
--- /dev/null
+++ b/tools/testing/selftests/bpf/socket_cookie_prog.c
@@ -0,0 +1,60 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2018 Facebook
+
+#include <linux/bpf.h>
+#include <sys/socket.h>
+
+#include "bpf_helpers.h"
+#include "bpf_endian.h"
+
+struct bpf_map_def SEC("maps") socket_cookies = {
+ .type = BPF_MAP_TYPE_HASH,
+ .key_size = sizeof(__u64),
+ .value_size = sizeof(__u32),
+ .max_entries = 1 << 8,
+};
+
+SEC("cgroup/connect6")
+int set_cookie(struct bpf_sock_addr *ctx)
+{
+ __u32 cookie_value = 0xFF;
+ __u64 cookie_key;
+
+ if (ctx->family != AF_INET6 || ctx->user_family != AF_INET6)
+ return 1;
+
+ cookie_key = bpf_get_socket_cookie(ctx);
+ if (bpf_map_update_elem(&socket_cookies, &cookie_key, &cookie_value, 0))
+ return 0;
+
+ return 1;
+}
+
+SEC("sockops")
+int update_cookie(struct bpf_sock_ops *ctx)
+{
+ __u32 new_cookie_value;
+ __u32 *cookie_value;
+ __u64 cookie_key;
+
+ if (ctx->family != AF_INET6)
+ return 1;
+
+ if (ctx->op != BPF_SOCK_OPS_TCP_CONNECT_CB)
+ return 1;
+
+ cookie_key = bpf_get_socket_cookie(ctx);
+
+ cookie_value = bpf_map_lookup_elem(&socket_cookies, &cookie_key);
+ if (!cookie_value)
+ return 1;
+
+ new_cookie_value = (ctx->local_port << 8) | *cookie_value;
+ bpf_map_update_elem(&socket_cookies, &cookie_key, &new_cookie_value, 0);
+
+ return 1;
+}
+
+int _version SEC("version") = 1;
+
+char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/test_socket_cookie.c b/tools/testing/selftests/bpf/test_socket_cookie.c
new file mode 100644
index 000000000000..68e108e4687a
--- /dev/null
+++ b/tools/testing/selftests/bpf/test_socket_cookie.c
@@ -0,0 +1,225 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2018 Facebook
+
+#include <string.h>
+#include <unistd.h>
+
+#include <arpa/inet.h>
+#include <netinet/in.h>
+#include <sys/types.h>
+#include <sys/socket.h>
+
+#include <bpf/bpf.h>
+#include <bpf/libbpf.h>
+
+#include "bpf_rlimit.h"
+#include "cgroup_helpers.h"
+
+#define CG_PATH "/foo"
+#define SOCKET_COOKIE_PROG "./socket_cookie_prog.o"
+
+static int start_server(void)
+{
+ struct sockaddr_in6 addr;
+ int fd;
+
+ fd = socket(AF_INET6, SOCK_STREAM, 0);
+ if (fd == -1) {
+ log_err("Failed to create server socket");
+ goto out;
+ }
+
+ memset(&addr, 0, sizeof(addr));
+ addr.sin6_family = AF_INET6;
+ addr.sin6_addr = in6addr_loopback;
+ addr.sin6_port = 0;
+
+ if (bind(fd, (const struct sockaddr *)&addr, sizeof(addr)) == -1) {
+ log_err("Failed to bind server socket");
+ goto close_out;
+ }
+
+ if (listen(fd, 128) == -1) {
+ log_err("Failed to listen on server socket");
+ goto close_out;
+ }
+
+ goto out;
+
+close_out:
+ close(fd);
+ fd = -1;
+out:
+ return fd;
+}
+
+static int connect_to_server(int server_fd)
+{
+ struct sockaddr_storage addr;
+ socklen_t len = sizeof(addr);
+ int fd;
+
+ fd = socket(AF_INET6, SOCK_STREAM, 0);
+ if (fd == -1) {
+ log_err("Failed to create client socket");
+ goto out;
+ }
+
+ if (getsockname(server_fd, (struct sockaddr *)&addr, &len)) {
+ log_err("Failed to get server addr");
+ goto close_out;
+ }
+
+ if (connect(fd, (const struct sockaddr *)&addr, len) == -1) {
+ log_err("Fail to connect to server");
+ goto close_out;
+ }
+
+ goto out;
+
+close_out:
+ close(fd);
+ fd = -1;
+out:
+ return fd;
+}
+
+static int validate_map(struct bpf_map *map, int client_fd)
+{
+ __u32 cookie_expected_value;
+ struct sockaddr_in6 addr;
+ socklen_t len = sizeof(addr);
+ __u32 cookie_value;
+ __u64 cookie_key;
+ int err = 0;
+ int map_fd;
+
+ if (!map) {
+ log_err("Map not found in BPF object");
+ goto err;
+ }
+
+ map_fd = bpf_map__fd(map);
+
+ err = bpf_map_get_next_key(map_fd, NULL, &cookie_key);
+ if (err) {
+ log_err("Can't get cookie key from map");
+ goto out;
+ }
+
+ err = bpf_map_lookup_elem(map_fd, &cookie_key, &cookie_value);
+ if (err) {
+ log_err("Can't get cookie value from map");
+ goto out;
+ }
+
+ err = getsockname(client_fd, (struct sockaddr *)&addr, &len);
+ if (err) {
+ log_err("Can't get client local addr");
+ goto out;
+ }
+
+ cookie_expected_value = (ntohs(addr.sin6_port) << 8) | 0xFF;
+ if (cookie_value != cookie_expected_value) {
+ log_err("Unexpected value in map: %x != %x", cookie_value,
+ cookie_expected_value);
+ goto err;
+ }
+
+ goto out;
+err:
+ err = -1;
+out:
+ return err;
+}
+
+static int run_test(int cgfd)
+{
+ enum bpf_attach_type attach_type;
+ struct bpf_prog_load_attr attr;
+ struct bpf_program *prog;
+ struct bpf_object *pobj;
+ const char *prog_name;
+ int server_fd = -1;
+ int client_fd = -1;
+ int prog_fd = -1;
+ int err = 0;
+
+ memset(&attr, 0, sizeof(attr));
+ attr.file = SOCKET_COOKIE_PROG;
+ attr.prog_type = BPF_PROG_TYPE_UNSPEC;
+
+ err = bpf_prog_load_xattr(&attr, &pobj, &prog_fd);
+ if (err) {
+ log_err("Failed to load %s", attr.file);
+ goto out;
+ }
+
+ bpf_object__for_each_program(prog, pobj) {
+ prog_name = bpf_program__title(prog, /*needs_copy*/ false);
+
+ if (strcmp(prog_name, "cgroup/connect6") == 0) {
+ attach_type = BPF_CGROUP_INET6_CONNECT;
+ } else if (strcmp(prog_name, "sockops") == 0) {
+ attach_type = BPF_CGROUP_SOCK_OPS;
+ } else {
+ log_err("Unexpected prog: %s", prog_name);
+ goto err;
+ }
+
+ err = bpf_prog_attach(bpf_program__fd(prog), cgfd, attach_type,
+ BPF_F_ALLOW_OVERRIDE);
+ if (err) {
+ log_err("Failed to attach prog %s", prog_name);
+ goto out;
+ }
+ }
+
+ server_fd = start_server();
+ if (server_fd == -1)
+ goto err;
+
+ client_fd = connect_to_server(server_fd);
+ if (client_fd == -1)
+ goto err;
+
+ if (validate_map(bpf_map__next(NULL, pobj), client_fd))
+ goto err;
+
+ goto out;
+err:
+ err = -1;
+out:
+ close(client_fd);
+ close(server_fd);
+ bpf_object__close(pobj);
+ printf("%s\n", err ? "FAILED" : "PASSED");
+ return err;
+}
+
+int main(int argc, char **argv)
+{
+ int cgfd = -1;
+ int err = 0;
+
+ if (setup_cgroup_environment())
+ goto err;
+
+ cgfd = create_and_get_cgroup(CG_PATH);
+ if (!cgfd)
+ goto err;
+
+ if (join_cgroup(CG_PATH))
+ goto err;
+
+ if (run_test(cgfd))
+ goto err;
+
+ goto out;
+err:
+ err = -1;
+out:
+ close(cgfd);
+ cleanup_cgroup_environment();
+ return err;
+}
--
2.17.1
^ permalink raw reply related
* [PATCH v2 bpf-next 1/4] bpf: Support bpf_get_socket_cookie in more prog types
From: Andrey Ignatov @ 2018-07-31 0:42 UTC (permalink / raw)
To: netdev; +Cc: Andrey Ignatov, ast, daniel, yhs, brakmo, kernel-team
In-Reply-To: <cover.1532996972.git.rdna@fb.com>
bpf_get_socket_cookie() helper can be used to identify skb that
correspond to the same socket.
Though socket cookie can be useful in many other use-cases where socket is
available in program context. Specifically BPF_PROG_TYPE_CGROUP_SOCK_ADDR
and BPF_PROG_TYPE_SOCK_OPS programs can benefit from it so that one of
them can augment a value in a map prepared earlier by other program for
the same socket.
The patch adds support to call bpf_get_socket_cookie() from
BPF_PROG_TYPE_CGROUP_SOCK_ADDR and BPF_PROG_TYPE_SOCK_OPS.
It doesn't introduce new helpers. Instead it reuses same helper name
bpf_get_socket_cookie() but adds support to this helper to accept
`struct bpf_sock_addr` and `struct bpf_sock_ops`.
Documentation in bpf.h is changed in a way that should not break
automatic generation of markdown.
Signed-off-by: Andrey Ignatov <rdna@fb.com>
Acked-by: Yonghong Song <yhs@fb.com>
---
include/uapi/linux/bpf.h | 14 ++++++++++++++
net/core/filter.c | 28 ++++++++++++++++++++++++++++
2 files changed, 42 insertions(+)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 870113916cac..0ebaaf7f3568 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1371,6 +1371,20 @@ union bpf_attr {
* A 8-byte long non-decreasing number on success, or 0 if the
* socket field is missing inside *skb*.
*
+ * u64 bpf_get_socket_cookie(struct bpf_sock_addr *ctx)
+ * Description
+ * Equivalent to bpf_get_socket_cookie() helper that accepts
+ * *skb*, but gets socket from **struct bpf_sock_addr** contex.
+ * Return
+ * A 8-byte long non-decreasing number.
+ *
+ * u64 bpf_get_socket_cookie(struct bpf_sock_ops *ctx)
+ * Description
+ * Equivalent to bpf_get_socket_cookie() helper that accepts
+ * *skb*, but gets socket from **struct bpf_sock_ops** contex.
+ * Return
+ * A 8-byte long non-decreasing number.
+ *
* u32 bpf_get_socket_uid(struct sk_buff *skb)
* Return
* The owner UID of the socket associated to *skb*. If the socket
diff --git a/net/core/filter.c b/net/core/filter.c
index 104d560946da..0b3c59468282 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -3812,6 +3812,30 @@ static const struct bpf_func_proto bpf_get_socket_cookie_proto = {
.arg1_type = ARG_PTR_TO_CTX,
};
+BPF_CALL_1(bpf_get_socket_cookie_sock_addr, struct bpf_sock_addr_kern *, ctx)
+{
+ return sock_gen_cookie(ctx->sk);
+}
+
+static const struct bpf_func_proto bpf_get_socket_cookie_sock_addr_proto = {
+ .func = bpf_get_socket_cookie_sock_addr,
+ .gpl_only = false,
+ .ret_type = RET_INTEGER,
+ .arg1_type = ARG_PTR_TO_CTX,
+};
+
+BPF_CALL_1(bpf_get_socket_cookie_sock_ops, struct bpf_sock_ops_kern *, ctx)
+{
+ return sock_gen_cookie(ctx->sk);
+}
+
+static const struct bpf_func_proto bpf_get_socket_cookie_sock_ops_proto = {
+ .func = bpf_get_socket_cookie_sock_ops,
+ .gpl_only = false,
+ .ret_type = RET_INTEGER,
+ .arg1_type = ARG_PTR_TO_CTX,
+};
+
BPF_CALL_1(bpf_get_socket_uid, struct sk_buff *, skb)
{
struct sock *sk = sk_to_full_sk(skb->sk);
@@ -4788,6 +4812,8 @@ sock_addr_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
default:
return NULL;
}
+ case BPF_FUNC_get_socket_cookie:
+ return &bpf_get_socket_cookie_sock_addr_proto;
default:
return bpf_base_func_proto(func_id);
}
@@ -4930,6 +4956,8 @@ sock_ops_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
return &bpf_sock_map_update_proto;
case BPF_FUNC_sock_hash_update:
return &bpf_sock_hash_update_proto;
+ case BPF_FUNC_get_socket_cookie:
+ return &bpf_get_socket_cookie_sock_ops_proto;
default:
return bpf_base_func_proto(func_id);
}
--
2.17.1
^ permalink raw reply related
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