* Re: [RFC PATCH] lib: Introduce generic __cmpxchg_u64() and use it where needed
From: Peter Zijlstra @ 2018-11-01 16:32 UTC (permalink / raw)
To: Trond Myklebust
Cc: mark.rutland@arm.com, linux-kernel@vger.kernel.org,
ralf@linux-mips.org, jlayton@kernel.org,
linuxppc-dev@lists.ozlabs.org, bfields@fieldses.org,
linux-mips@linux-mips.org, linux@roeck-us.net,
linux-nfs@vger.kernel.org, akpm@linux-foundation.org,
will.deacon@arm.com, boqun.feng@gmail.com, paul.burton@mips.com,
anna.schumaker@netapp.com, jhogan@kernel.org, "netdev@vger.ke
In-Reply-To: <f38e272f7a96e983549e4281aa9fd02833a4277a.camel@hammerspace.com>
On Thu, Nov 01, 2018 at 03:22:15PM +0000, Trond Myklebust wrote:
> On Thu, 2018-11-01 at 15:59 +0100, Peter Zijlstra wrote:
> > On Thu, Nov 01, 2018 at 01:18:46PM +0000, Mark Rutland wrote:
> > > > My one question (and the reason why I went with cmpxchg() in the
> > > > first place) would be about the overflow behaviour for
> > > > atomic_fetch_inc() and friends. I believe those functions should
> > > > be OK on x86, so that when we overflow the counter, it behaves
> > > > like an unsigned value and wraps back around. Is that the case
> > > > for all architectures?
> > > >
> > > > i.e. are atomic_t/atomic64_t always guaranteed to behave like
> > > > u32/u64 on increment?
> > > >
> > > > I could not find any documentation that explicitly stated that
> > > > they should.
> > >
> > > Peter, Will, I understand that the atomic_t/atomic64_t ops are
> > > required to wrap per 2's-complement. IIUC the refcount code relies
> > > on this.
> > >
> > > Can you confirm?
> >
> > There is quite a bit of core code that hard assumes 2s-complement.
> > Not only for atomics but for any signed integer type. Also see the
> > kernel using -fno-strict-overflow which implies -fwrapv, which
> > defines signed overflow to behave like 2s-complement (and rids us of
> > that particular UB).
>
> Fair enough, but there have also been bugfixes to explicitly fix unsafe
> C standards assumptions for signed integers. See, for instance commit
> 5a581b367b5d "jiffies: Avoid undefined behavior from signed overflow"
> from Paul McKenney.
Yes, I feel Paul has been to too many C/C++ committee meetings and got
properly paranoid. Which isn't always a bad thing :-)
But for us using -fno-strict-overflow which actually defines signed
overflow, I myself am really not worried. I'm also not sure if KASAN has
been taught about this, or if it will still (incorrectly) warn about UB
for signed types.
> Anyhow, if the atomic maintainers are willing to stand up and state for
> the record that the atomic counters are guaranteed to wrap modulo 2^n
> just like unsigned integers, then I'm happy to take Paul's patch.
I myself am certainly relying on it.
^ permalink raw reply
* Re: [PATCH net] net: hns3: Fix for out-of-bounds access when setting pfc back pressure
From: Yunsheng Lin @ 2018-11-02 0:48 UTC (permalink / raw)
To: John Garry, davem
Cc: huangdaode, xuwei5, liguozhu, Yisen.Zhuang, linuxarm, salil.mehta,
lipeng321, netdev, linux-kernel
In-Reply-To: <4b5d0e04-df53-6123-0562-7f0146d33ae9@huawei.com>
On 2018/11/1 20:49, John Garry wrote:
> On 01/11/2018 12:12, Yunsheng Lin wrote:
>> The vport should be initialized to hdev->vport for each bp group,
>> otherwise it will cause out-of-bounds access and bp setting not
>> correct problem.
>>
>> [ 35.254124] BUG: KASAN: slab-out-of-bounds in hclge_pause_setup_hw+0x2a0/0x3f8 [hclge]
>> [ 35.254126] Read of size 2 at addr ffff803b6651581a by task kworker/0:1/14
>>
>> [ 35.254132] CPU: 0 PID: 14 Comm: kworker/0:1 Not tainted 4.19.0-rc7-hulk+ #85
>> [ 35.254133] Hardware name: Huawei D06/D06, BIOS Hisilicon D06 UEFI RC0 - B052 (V0.52) 09/14/2018
>> [ 35.254141] Workqueue: events work_for_cpu_fn
>> [ 35.254144] Call trace:
>> [ 35.254147] dump_backtrace+0x0/0x2f0
>> [ 35.254149] show_stack+0x24/0x30
>> [ 35.254154] dump_stack+0x110/0x184
>> [ 35.254157] print_address_description+0x168/0x2b0
>> [ 35.254160] kasan_report+0x184/0x310
>> [ 35.254162] __asan_load2+0x7c/0xa0
>> [ 35.254170] hclge_pause_setup_hw+0x2a0/0x3f8 [hclge]
>> [ 35.254177] hclge_tm_init_hw+0x794/0x9f0 [hclge]
>> [ 35.254184] hclge_tm_schd_init+0x48/0x58 [hclge]
>> [ 35.254191] hclge_init_ae_dev+0x778/0x1168 [hclge]
>> [ 35.254196] hnae3_register_ae_dev+0x14c/0x298 [hnae3]
>> [ 35.254206] hns3_probe+0x88/0xa8 [hns3]
>> [ 35.254210] local_pci_probe+0x7c/0xf0
>> [ 35.254212] work_for_cpu_fn+0x34/0x50
>> [ 35.254214] process_one_work+0x4d4/0xa38
>> [ 35.254216] worker_thread+0x55c/0x8d8
>> [ 35.254219] kthread+0x1b0/0x1b8
>> [ 35.254222] ret_from_fork+0x10/0x1c
>>
>> [ 35.254224] The buggy address belongs to the page:
>> [ 35.254228] page:ffff7e00ed994400 count:1 mapcount:0 mapping:0000000000000000 index:0x0 compound_mapcount: 0
>> [ 35.273835] flags: 0xfffff8000008000(head)
>> [ 35.282007] raw: 0fffff8000008000 dead000000000100 dead000000000200 0000000000000000
>> [ 35.282010] raw: 0000000000000000 0000000000000000 00000001ffffffff 0000000000000000
>> [ 35.282012] page dumped because: kasan: bad access detected
>>
>> [ 35.282014] Memory state around the buggy address:
>> [ 35.282017] ffff803b66515700: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
>> [ 35.282019] ffff803b66515780: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
>> [ 35.282021] >ffff803b66515800: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
>> [ 35.282022] ^
>> [ 35.282024] ffff803b66515880: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
>> [ 35.282026] ffff803b66515900: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
>> [ 35.282028] ==================================================================
>> [ 35.282029] Disabling lock debugging due to kernel taint
>> [ 35.282747] hclge driver initialization finished.
>>
>> Fixes: 67bf2541f4b9 ("net: hns3: Fixes the back pressure setting when sriov is enabled")
>> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
>> ---
>> drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_tm.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_tm.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_tm.c
>> index aa5cb98..0c9c6ae 100644
>> --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_tm.c
>> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_tm.c
>> @@ -1168,11 +1168,12 @@ static int hclge_pfc_setup_hw(struct hclge_dev *hdev)
>> */
>> static int hclge_bp_setup_hw(struct hclge_dev *hdev, u8 tc)
>> {
>> - struct hclge_vport *vport = hdev->vport;
>> u32 i, k, qs_bitmap;
>> int ret;
>>
>> for (i = 0; i < HCLGE_BP_GRP_NUM; i++) {
>> + struct hclge_vport *vport = hdev->vport;
>> +
>
> It seems to me that the bug is fixed but the code is not much better.
>
> Since variable vport is only referenced in the inner loop, I would define it there.
I am agree that it is always better to have better and safer code.
I will provide another verion to limit local variable as much as possible.
Thanks for the reviewing and suggestion.
>
>> qs_bitmap = 0;
>>
>> for (k = 0; k < hdev->num_alloc_vport; k++) {
>>
>
> static int hclge_bp_setup_hw(struct hclge_dev *hdev, u8 tc)
> {
> - struct hclge_vport *vport = hdev->vport;
> - u32 i, k, qs_bitmap;
> - int ret;
> + int ret, i;
>
> for (i = 0; i < HCLGE_BP_GRP_NUM; i++) {
> - qs_bitmap = 0;
> + u32 qs_bitmap = 0;
> + int k;
>
> for (k = 0; k < hdev->num_alloc_vport; k++) {
> + struct hclge_vport *vport = &hdev->vport[k];
> u16 qs_id = vport->qs_offset + tc;
> u8 grp, sub_grp;
>
> ...
>
> if (i == grp)
> qs_bitmap |= (1 << sub_grp);
> -
> - vport++;
>
>
> ... so this seems better and safer, as you're explicitly limiting vport within bounds of hdev->num_alloc_vport.
>
>
>
> .
>
^ permalink raw reply
* Re: [PATCH v3 1/2] kretprobe: produce sane stack traces
From: Steven Rostedt @ 2018-11-02 0:47 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Naveen N. Rao, Anil S Keshavamurthy, David S. Miller,
Masami Hiramatsu, Jonathan Corbet, Peter Zijlstra, Ingo Molnar,
Arnaldo Carvalho de Melo, Alexander Shishkin, Jiri Olsa,
Namhyung Kim, Shuah Khan, Alexei Starovoitov, Daniel Borkmann,
Brendan Gregg, Christian Brauner, Aleksa Sarai, netdev, linux-doc,
linux-kernel
In-Reply-To: <20181101083551.3805-2-cyphar@cyphar.com>
On Thu, 1 Nov 2018 19:35:50 +1100
Aleksa Sarai <cyphar@cyphar.com> wrote:
> Historically, kretprobe has always produced unusable stack traces
> (kretprobe_trampoline is the only entry in most cases, because of the
> funky stack pointer overwriting). This has caused quite a few annoyances
> when using tracing to debug problems[1] -- since return values are only
> available with kretprobes but stack traces were only usable for kprobes,
> users had to probe both and then manually associate them.
>
> With the advent of bpf_trace, users would have been able to do this
> association in bpf, but this was less than ideal (because
> bpf_get_stackid would still produce rubbish and programs that didn't
> know better would get silly results). The main usecase for stack traces
> (at least with bpf_trace) is for DTrace-style aggregation on stack
> traces (both entry and exit). Therefore we cannot simply correct the
> stack trace on exit -- we must stash away the stack trace and return the
> entry stack trace when it is requested.
>
> [1]: https://github.com/iovisor/bpftrace/issues/101
>
> Cc: Brendan Gregg <bgregg@netflix.com>
> Cc: Christian Brauner <christian@brauner.io>
> Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> ---
> Documentation/kprobes.txt | 6 +-
> include/linux/kprobes.h | 27 +++++
> kernel/events/callchain.c | 8 +-
> kernel/kprobes.c | 101 +++++++++++++++++-
> kernel/trace/trace.c | 11 +-
> .../test.d/kprobe/kretprobe_stacktrace.tc | 25 +++++
> 6 files changed, 173 insertions(+), 5 deletions(-)
> create mode 100644 tools/testing/selftests/ftrace/test.d/kprobe/kretprobe_stacktrace.tc
>
> diff --git a/Documentation/kprobes.txt b/Documentation/kprobes.txt
> index 10f4499e677c..1965585848f4 100644
> --- a/Documentation/kprobes.txt
> +++ b/Documentation/kprobes.txt
> @@ -597,7 +597,11 @@ address with the trampoline's address, stack backtraces and calls
> to __builtin_return_address() will typically yield the trampoline's
> address instead of the real return address for kretprobed functions.
> (As far as we can tell, __builtin_return_address() is used only
> -for instrumentation and error reporting.)
> +for instrumentation and error reporting.) However, since return probes
> +are used extensively in tracing (where stack backtraces are useful),
> +return probes will stash away the stack backtrace during function entry
> +so that return probe handlers can use the entry backtrace instead of
> +having a trace with just kretprobe_trampoline.
>
> If the number of times a function is called does not match the number
> of times it returns, registering a return probe on that function may
> diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
> index e909413e4e38..1a1629544e56 100644
> --- a/include/linux/kprobes.h
> +++ b/include/linux/kprobes.h
> @@ -40,6 +40,8 @@
> #include <linux/rcupdate.h>
> #include <linux/mutex.h>
> #include <linux/ftrace.h>
> +#include <linux/stacktrace.h>
> +#include <linux/perf_event.h>
> #include <asm/kprobes.h>
>
> #ifdef CONFIG_KPROBES
> @@ -168,11 +170,18 @@ struct kretprobe {
> raw_spinlock_t lock;
> };
>
> +#define KRETPROBE_TRACE_SIZE 127
> +struct kretprobe_trace {
> + int nr_entries;
> + unsigned long entries[KRETPROBE_TRACE_SIZE];
> +};
> +
> struct kretprobe_instance {
> struct hlist_node hlist;
> struct kretprobe *rp;
> kprobe_opcode_t *ret_addr;
> struct task_struct *task;
> + struct kretprobe_trace entry;
> char data[0];
> };
>
> @@ -371,6 +380,12 @@ void unregister_kretprobe(struct kretprobe *rp);
> int register_kretprobes(struct kretprobe **rps, int num);
> void unregister_kretprobes(struct kretprobe **rps, int num);
>
> +struct kretprobe_instance *current_kretprobe_instance(void);
> +void kretprobe_save_stack_trace(struct kretprobe_instance *ri,
> + struct stack_trace *trace);
> +void kretprobe_perf_callchain_kernel(struct kretprobe_instance *ri,
> + struct perf_callchain_entry_ctx *ctx);
> +
> void kprobe_flush_task(struct task_struct *tk);
> void recycle_rp_inst(struct kretprobe_instance *ri, struct hlist_head *head);
>
> @@ -397,6 +412,18 @@ static inline struct kprobe *kprobe_running(void)
> {
> return NULL;
> }
> +static inline struct kretprobe_instance *current_kretprobe_instance(void)
> +{
> + return NULL;
> +}
> +static inline void kretprobe_save_stack_trace(struct kretprobe_instance *ri,
> + struct stack_trace *trace)
> +{
> +}
> +static inline void kretprobe_perf_callchain_kernel(struct kretprobe_instance *ri,
> + struct perf_callchain_entry_ctx *ctx)
> +{
> +}
> static inline int register_kprobe(struct kprobe *p)
> {
> return -ENOSYS;
> diff --git a/kernel/events/callchain.c b/kernel/events/callchain.c
> index 24a77c34e9ad..98edcd8a6987 100644
> --- a/kernel/events/callchain.c
> +++ b/kernel/events/callchain.c
> @@ -12,6 +12,7 @@
> #include <linux/perf_event.h>
> #include <linux/slab.h>
> #include <linux/sched/task_stack.h>
> +#include <linux/kprobes.h>
>
> #include "internal.h"
>
> @@ -197,9 +198,14 @@ get_perf_callchain(struct pt_regs *regs, u32 init_nr, bool kernel, bool user,
> ctx.contexts_maxed = false;
>
> if (kernel && !user_mode(regs)) {
> + struct kretprobe_instance *ri = current_kretprobe_instance();
> +
> if (add_mark)
> perf_callchain_store_context(&ctx, PERF_CONTEXT_KERNEL);
> - perf_callchain_kernel(&ctx, regs);
> + if (ri)
> + kretprobe_perf_callchain_kernel(ri, &ctx);
> + else
> + perf_callchain_kernel(&ctx, regs);
> }
>
> if (user) {
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 90e98e233647..fca3964d18cd 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1206,6 +1206,16 @@ __releases(hlist_lock)
> }
> NOKPROBE_SYMBOL(kretprobe_table_unlock);
>
> +static bool kretprobe_hash_is_locked(struct task_struct *tsk)
> +{
> + unsigned long hash = hash_ptr(tsk, KPROBE_HASH_BITS);
> + raw_spinlock_t *hlist_lock;
> +
> + hlist_lock = kretprobe_table_lock_ptr(hash);
> + return raw_spin_is_locked(hlist_lock);
> +}
> +NOKPROBE_SYMBOL(kretprobe_hash_is_locked);
> +
> /*
> * This function is called from finish_task_switch when task tk becomes dead,
> * so that we can recycle any function-return probe instances associated
> @@ -1800,6 +1810,13 @@ unsigned long __weak arch_deref_entry_point(void *entry)
> return (unsigned long)entry;
> }
>
> +static int pre_handler_kretprobe(struct kprobe *p, struct pt_regs *regs);
> +
> +static inline bool kprobe_is_retprobe(struct kprobe *kp)
> +{
> + return kp->pre_handler == pre_handler_kretprobe;
> +}
> +
> #ifdef CONFIG_KRETPROBES
> /*
> * This kprobe pre_handler is registered with every kretprobe. When probe
> @@ -1826,6 +1843,8 @@ static int pre_handler_kretprobe(struct kprobe *p, struct pt_regs *regs)
> hash = hash_ptr(current, KPROBE_HASH_BITS);
> raw_spin_lock_irqsave(&rp->lock, flags);
> if (!hlist_empty(&rp->free_instances)) {
> + struct stack_trace trace = {};
> +
> ri = hlist_entry(rp->free_instances.first,
> struct kretprobe_instance, hlist);
> hlist_del(&ri->hlist);
> @@ -1834,6 +1853,11 @@ static int pre_handler_kretprobe(struct kprobe *p, struct pt_regs *regs)
> ri->rp = rp;
> ri->task = current;
>
> + trace.entries = &ri->entry.entries[0];
> + trace.max_entries = KRETPROBE_TRACE_SIZE;
> + save_stack_trace_regs(regs, &trace);
> + ri->entry.nr_entries = trace.nr_entries;
> +
So basically your saving a copy of the stack trace for each probe, for
something that may not ever be used?
This adds quite a lot of overhead for something not used often.
I think the real answer is to fix kretprobes (and I just checked, the
return call of function graph tracer stack trace doesn't go past the
return_to_handler function either. It's just that a stack trace was
never done on the return side).
The more I look at this patch, the less I like it.
-- Steve
> if (rp->entry_handler && rp->entry_handler(ri, regs)) {
> raw_spin_lock_irqsave(&rp->lock, flags);
> hlist_add_head(&ri->hlist, &rp->free_instances);
> @@ -1856,6 +1880,65 @@ static int pre_handler_kretprobe(struct kprobe *p, struct pt_regs *regs)
> }
> NOKPROBE_SYMBOL(pre_handler_kretprobe);
>
> +/*
> + * Return the kretprobe_instance associated with the current_kprobe. Calling
> + * this is only reasonable from within a kretprobe handler context (otherwise
> + * return NULL).
> + *
> + * Must be called within a kretprobe_hash_lock(current, ...) context.
> + */
> +struct kretprobe_instance *current_kretprobe_instance(void)
> +{
> + struct kprobe *kp;
> + struct kretprobe *rp;
> + struct kretprobe_instance *ri;
> + struct hlist_head *head;
> + unsigned long hash = hash_ptr(current, KPROBE_HASH_BITS);
> +
> + kp = kprobe_running();
> + if (!kp || !kprobe_is_retprobe(kp))
> + return NULL;
> + if (WARN_ON(!kretprobe_hash_is_locked(current)))
> + return NULL;
> +
> + rp = container_of(kp, struct kretprobe, kp);
> + head = &kretprobe_inst_table[hash];
> +
> + hlist_for_each_entry(ri, head, hlist) {
> + if (ri->task == current && ri->rp == rp)
> + return ri;
> + }
> + return NULL;
> +}
> +EXPORT_SYMBOL_GPL(current_kretprobe_instance);
> +NOKPROBE_SYMBOL(current_kretprobe_instance);
> +
> +void kretprobe_save_stack_trace(struct kretprobe_instance *ri,
> + struct stack_trace *trace)
> +{
> + int i;
> + struct kretprobe_trace *krt = &ri->entry;
> +
> + for (i = trace->skip; i < krt->nr_entries; i++) {
> + if (trace->nr_entries >= trace->max_entries)
> + break;
> + trace->entries[trace->nr_entries++] = krt->entries[i];
> + }
> +}
> +
> +void kretprobe_perf_callchain_kernel(struct kretprobe_instance *ri,
> + struct perf_callchain_entry_ctx *ctx)
> +{
> + int i;
> + struct kretprobe_trace *krt = &ri->entry;
> +
> + for (i = 0; i < krt->nr_entries; i++) {
> + if (krt->entries[i] == ULONG_MAX)
> + break;
> + perf_callchain_store(ctx, (u64) krt->entries[i]);
> + }
> +}
> +
> bool __weak arch_kprobe_on_func_entry(unsigned long offset)
> {
> return !offset;
> @@ -2005,6 +2088,22 @@ static int pre_handler_kretprobe(struct kprobe *p, struct pt_regs *regs)
> }
> NOKPROBE_SYMBOL(pre_handler_kretprobe);
>
> +struct kretprobe_instance *current_kretprobe_instance(void)
> +{
> + return NULL;
> +}
> +EXPORT_SYMBOL_GPL(current_kretprobe_instance);
> +NOKPROBE_SYMBOL(current_kretprobe_instance);
> +
> +void kretprobe_save_stack_trace(struct kretprobe_instance *ri,
> + struct stack_trace *trace)
> +{
> +}
> +
> +void kretprobe_perf_callchain_kernel(struct kretprobe_instance *ri,
> + struct perf_callchain_entry_ctx *ctx)
> +{
> +}
> #endif /* CONFIG_KRETPROBES */
>
> /* Set the kprobe gone and remove its instruction buffer. */
> @@ -2241,7 +2340,7 @@ static void report_probe(struct seq_file *pi, struct kprobe *p,
> char *kprobe_type;
> void *addr = p->addr;
>
> - if (p->pre_handler == pre_handler_kretprobe)
> + if (kprobe_is_retprobe(p))
> kprobe_type = "r";
> else
> kprobe_type = "k";
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index bf6f1d70484d..2210d38a4dbf 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -42,6 +42,7 @@
> #include <linux/nmi.h>
> #include <linux/fs.h>
> #include <linux/trace.h>
> +#include <linux/kprobes.h>
> #include <linux/sched/clock.h>
> #include <linux/sched/rt.h>
>
> @@ -2590,6 +2591,7 @@ static void __ftrace_trace_stack(struct ring_buffer *buffer,
> struct ring_buffer_event *event;
> struct stack_entry *entry;
> struct stack_trace trace;
> + struct kretprobe_instance *ri = current_kretprobe_instance();
> int use_stack;
> int size = FTRACE_STACK_ENTRIES;
>
> @@ -2626,7 +2628,9 @@ static void __ftrace_trace_stack(struct ring_buffer *buffer,
> trace.entries = this_cpu_ptr(ftrace_stack.calls);
> trace.max_entries = FTRACE_STACK_MAX_ENTRIES;
>
> - if (regs)
> + if (ri)
> + kretprobe_save_stack_trace(ri, &trace);
> + else if (regs)
> save_stack_trace_regs(regs, &trace);
> else
> save_stack_trace(&trace);
> @@ -2653,7 +2657,10 @@ static void __ftrace_trace_stack(struct ring_buffer *buffer,
> else {
> trace.max_entries = FTRACE_STACK_ENTRIES;
> trace.entries = entry->caller;
> - if (regs)
> +
> + if (ri)
> + kretprobe_save_stack_trace(ri, &trace);
> + else if (regs)
> save_stack_trace_regs(regs, &trace);
> else
> save_stack_trace(&trace);
> diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kretprobe_stacktrace.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kretprobe_stacktrace.tc
> new file mode 100644
> index 000000000000..03146c6a1a3c
> --- /dev/null
> +++ b/tools/testing/selftests/ftrace/test.d/kprobe/kretprobe_stacktrace.tc
> @@ -0,0 +1,25 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0+
> +# description: Kretprobe dynamic event with a stacktrace
> +
> +[ -f kprobe_events ] || exit_unsupported # this is configurable
> +
> +echo 0 > events/enable
> +echo 1 > options/stacktrace
> +
> +echo 'r:teststackprobe sched_fork $retval' > kprobe_events
> +grep teststackprobe kprobe_events
> +test -d events/kprobes/teststackprobe
> +
> +clear_trace
> +echo 1 > events/kprobes/teststackprobe/enable
> +( echo "forked")
> +echo 0 > events/kprobes/teststackprobe/enable
> +
> +# Make sure we don't see kretprobe_trampoline and we see _do_fork.
> +! grep 'kretprobe' trace
> +grep '_do_fork' trace
> +
> +echo '-:teststackprobe' >> kprobe_events
> +clear_trace
> +test -d events/kprobes/teststackprobe && exit_fail || exit_pass
^ permalink raw reply
* [PATCH] iwlegacy: fix small typo
From: Yangtao Li @ 2018-11-01 15:40 UTC (permalink / raw)
To: sgruszka, kvalo, davem; +Cc: linux-wireless, netdev, linux-kernel, Yangtao Li
Signed-off-by: Yangtao Li <tiny.windzz@gmail.com>
---
drivers/net/wireless/intel/iwlegacy/3945-rs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/intel/iwlegacy/3945-rs.c b/drivers/net/wireless/intel/iwlegacy/3945-rs.c
index e8983c6a2b7b..a697edd46e7f 100644
--- a/drivers/net/wireless/intel/iwlegacy/3945-rs.c
+++ b/drivers/net/wireless/intel/iwlegacy/3945-rs.c
@@ -781,7 +781,7 @@ il3945_rs_get_rate(void *il_r, struct ieee80211_sta *sta, void *il_sta,
switch (scale_action) {
case -1:
- /* Decrese rate */
+ /* Decrease rate */
if (low != RATE_INVALID)
idx = low;
break;
--
2.17.0
^ permalink raw reply related
* Re: Kernel 4.19 network performance - forwarding/routing normal users traffic
From: Aaron Lu @ 2018-11-01 15:27 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: Paweł Staszewski, Eric Dumazet, netdev, Tariq Toukan,
Ilias Apalodimas, Yoel Caspersen, Mel Gorman
In-Reply-To: <20181101102213.2fa2643d@redhat.com>
On Thu, Nov 01, 2018 at 10:22:13AM +0100, Jesper Dangaard Brouer wrote:
... ...
> Section copied out:
>
> mlx5e_poll_tx_cq
> |
> --16.34%--napi_consume_skb
> |
> |--12.65%--__free_pages_ok
> | |
> | --11.86%--free_one_page
> | |
> | |--10.10%--queued_spin_lock_slowpath
> | |
> | --0.65%--_raw_spin_lock
This callchain looks like it is freeing higher order pages than order 0:
__free_pages_ok is only called for pages whose order are bigger than 0.
> |
> |--1.55%--page_frag_free
> |
> --1.44%--skb_release_data
>
>
> Let me explain what (I think) happens. The mlx5 driver RX-page recycle
> mechanism is not effective in this workload, and pages have to go
> through the page allocator. The lock contention happens during mlx5
> DMA TX completion cycle. And the page allocator cannot keep up at
> these speeds.
>
> One solution is extend page allocator with a bulk free API. (This have
> been on my TODO list for a long time, but I don't have a
> micro-benchmark that trick the driver page-recycle to fail). It should
> fit nicely, as I can see that kmem_cache_free_bulk() does get
> activated (bulk freeing SKBs), which means that DMA TX completion do
> have a bulk of packets.
>
> We can (and should) also improve the page recycle scheme in the driver.
> After LPC, I have a project with Tariq and Ilias (Cc'ed) to improve the
> page_pool, and we will (attempt) to generalize this, for both high-end
> mlx5 and more low-end ARM64-boards (macchiatobin and espressobin).
>
> The MM-people is in parallel working to improve the performance of
> order-0 page returns. Thus, the explicit page bulk free API might
> actually become less important. I actually think (Cc.) Aaron have a
> patchset he would like you to test, which removes the (zone->)lock
> you hit in free_one_page().
Thanks Jesper.
Yes, the said patchset is in this branch:
https://github.com/aaronlu/linux no_merge_cluster_alloc_4.19-rc5
But as I said above, I think the lock contention here is for
order > 0 pages so my current patchset will not work here, unfortunately.
BTW, Mel Gorman has suggested an alternative way to improve page
allocator's scalability and I'm working on it right now, it will
improve page allocator's scalability for all order pages. I might be
able to post it some time next week, will CC all of you when it's ready.
^ permalink raw reply
* Re: [PATCH 12/31] netfilter: cttimeout: remove superfluous check on layer 4 netlink functions
From: Eric Dumazet @ 2018-11-01 14:57 UTC (permalink / raw)
To: Pablo Neira Ayuso, netfilter-devel; +Cc: davem, netdev
In-Reply-To: <20181008230125.2330-13-pablo@netfilter.org>
On 10/08/2018 04:01 PM, Pablo Neira Ayuso wrote:
> We assume they are always set accordingly since a874752a10da
> ("netfilter: conntrack: timeout interface depend on
> CONFIG_NF_CONNTRACK_TIMEOUT"), so we can get rid of this checks.
>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
> net/netfilter/nfnetlink_cttimeout.c | 48 ++++++++++++++-----------------------
> net/netfilter/nft_ct.c | 3 ---
> 2 files changed, 18 insertions(+), 33 deletions(-)
>
> diff --git a/net/netfilter/nfnetlink_cttimeout.c b/net/netfilter/nfnetlink_cttimeout.c
> index a30f8ba4b89a..6ca0df7f416f 100644
> --- a/net/netfilter/nfnetlink_cttimeout.c
> +++ b/net/netfilter/nfnetlink_cttimeout.c
> @@ -53,9 +53,6 @@ ctnl_timeout_parse_policy(void *timeout,
> struct nlattr **tb;
> int ret = 0;
>
> - if (!l4proto->ctnl_timeout.nlattr_to_obj)
> - return 0;
> -
> tb = kcalloc(l4proto->ctnl_timeout.nlattr_max + 1, sizeof(*tb),
> GFP_KERNEL);
>
> @@ -167,6 +164,8 @@ ctnl_timeout_fill_info(struct sk_buff *skb, u32 portid, u32 seq, u32 type,
> struct nfgenmsg *nfmsg;
> unsigned int flags = portid ? NLM_F_MULTI : 0;
> const struct nf_conntrack_l4proto *l4proto = timeout->timeout.l4proto;
> + struct nlattr *nest_parms;
> + int ret;
>
> event = nfnl_msg_type(NFNL_SUBSYS_CTNETLINK_TIMEOUT, event);
> nlh = nlmsg_put(skb, portid, seq, event, sizeof(*nfmsg), flags);
> @@ -186,22 +185,15 @@ ctnl_timeout_fill_info(struct sk_buff *skb, u32 portid, u32 seq, u32 type,
> htonl(refcount_read(&timeout->refcnt))))
> goto nla_put_failure;
>
> - if (likely(l4proto->ctnl_timeout.obj_to_nlattr)) {
> - struct nlattr *nest_parms;
> - int ret;
> -
> - nest_parms = nla_nest_start(skb,
> - CTA_TIMEOUT_DATA | NLA_F_NESTED);
> - if (!nest_parms)
> - goto nla_put_failure;
> + nest_parms = nla_nest_start(skb, CTA_TIMEOUT_DATA | NLA_F_NESTED);
> + if (!nest_parms)
> + goto nla_put_failure;
>
> - ret = l4proto->ctnl_timeout.obj_to_nlattr(skb,
> - &timeout->timeout.data);
> - if (ret < 0)
> - goto nla_put_failure;
> + ret = l4proto->ctnl_timeout.obj_to_nlattr(skb, &timeout->timeout.data);
> + if (ret < 0)
> + goto nla_put_failure;
>
> - nla_nest_end(skb, nest_parms);
> - }
> + nla_nest_end(skb, nest_parms);
>
> nlmsg_end(skb, nlh);
> return skb->len;
> @@ -397,6 +389,8 @@ cttimeout_default_fill_info(struct net *net, struct sk_buff *skb, u32 portid,
> struct nlmsghdr *nlh;
> struct nfgenmsg *nfmsg;
> unsigned int flags = portid ? NLM_F_MULTI : 0;
> + struct nlattr *nest_parms;
> + int ret;
>
> event = nfnl_msg_type(NFNL_SUBSYS_CTNETLINK_TIMEOUT, event);
> nlh = nlmsg_put(skb, portid, seq, event, sizeof(*nfmsg), flags);
> @@ -412,21 +406,15 @@ cttimeout_default_fill_info(struct net *net, struct sk_buff *skb, u32 portid,
> nla_put_u8(skb, CTA_TIMEOUT_L4PROTO, l4proto->l4proto))
> goto nla_put_failure;
>
> - if (likely(l4proto->ctnl_timeout.obj_to_nlattr)) {
> - struct nlattr *nest_parms;
> - int ret;
> -
> - nest_parms = nla_nest_start(skb,
> - CTA_TIMEOUT_DATA | NLA_F_NESTED);
> - if (!nest_parms)
> - goto nla_put_failure;
> + nest_parms = nla_nest_start(skb, CTA_TIMEOUT_DATA | NLA_F_NESTED);
> + if (!nest_parms)
> + goto nla_put_failure;
>
> - ret = l4proto->ctnl_timeout.obj_to_nlattr(skb, NULL);
> - if (ret < 0)
> - goto nla_put_failure;
> + ret = l4proto->ctnl_timeout.obj_to_nlattr(skb, NULL);
Hi Pablo
None of the obj_to_nlattr handlers can handle a NULL pointer.
What is the intent here ?
netlink: 24 bytes leftover after parsing attributes in process `syz-executor2'.
kasan: CONFIG_KASAN_INLINE enabled
kasan: GPF could be caused by NULL-ptr deref or user memory access
general protection fault: 0000 [#1] PREEMPT SMP KASAN
CPU: 0 PID: 9575 Comm: syz-executor1 Not tainted 4.19.0+ #312
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
RIP: 0010:icmp_timeout_obj_to_nlattr+0x77/0x170 net/netfilter/nf_conntrack_proto_icmp.c:297
kobject: 'loop5' (00000000d8ff612b): kobject_uevent_env
Code: b5 41 c7 00 f1 f1 f1 f1 c7 40 04 04 f2 f2 f2 65 48 8b 04 25 28 00 00 00 48 89 45 d0 31 c0 e8 c0 06 26 fb 4c 89 e8 48 c1 e8 03 <42> 0f b6 14 38 4c 89 e8 83 e0 07 83 c0 03 38 d0 7c 08 84 d2 0f 85
kobject: 'loop5' (00000000d8ff612b): fill_kobj_path: path = '/devices/virtual/block/loop5'
RSP: 0018:ffff88017def7220 EFLAGS: 00010246
RAX: 0000000000000000 RBX: 1ffff1002fbdee45 RCX: ffffc90004561000
RDX: 000000000000064f RSI: ffffffff865970c0 RDI: ffff8801ba602bc0
RBP: ffff88017def72b0 R08: ffff8801879e2400 R09: ffff880188d264a8
R10: ffffed00311a4c94 R11: ffff880188d264a0 R12: ffff8801ba602bc0
R13: 0000000000000000 R14: ffff88017def7288 R15: dffffc0000000000
FS: 00007fb495a6f700(0000) GS:ffff8801dae00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
kobject: 'loop3' (00000000bc48af2d): kobject_uevent_env
CR2: 00007f4879a55000 CR3: 00000001b7b96000 CR4: 00000000001406f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
kobject: 'loop3' (00000000bc48af2d): fill_kobj_path: path = '/devices/virtual/block/loop3'
cttimeout_default_fill_info net/netfilter/nfnetlink_cttimeout.c:411 [inline]
cttimeout_default_get+0x644/0x9e0 net/netfilter/nfnetlink_cttimeout.c:457
nfnetlink_rcv_msg+0xdd3/0x10c0 net/netfilter/nfnetlink.c:228
kobject: 'loop3' (00000000bc48af2d): kobject_uevent_env
kobject: 'loop3' (00000000bc48af2d): fill_kobj_path: path = '/devices/virtual/block/loop3'
netlink_rcv_skb+0x172/0x440 net/netlink/af_netlink.c:2477
nfnetlink_rcv+0x1c0/0x4d0 net/netfilter/nfnetlink.c:560
netlink_unicast_kernel net/netlink/af_netlink.c:1310 [inline]
netlink_unicast+0x5a5/0x760 net/netlink/af_netlink.c:1336
netlink_sendmsg+0xa18/0xfc0 net/netlink/af_netlink.c:1917
sock_sendmsg_nosec net/socket.c:621 [inline]
sock_sendmsg+0xd5/0x120 net/socket.c:631
___sys_sendmsg+0x7fd/0x930 net/socket.c:2116
__sys_sendmsg+0x11d/0x280 net/socket.c:2154
__do_sys_sendmsg net/socket.c:2163 [inline]
__se_sys_sendmsg net/socket.c:2161 [inline]
__x64_sys_sendmsg+0x78/0xb0 net/socket.c:2161
do_syscall_64+0x1b9/0x820 arch/x86/entry/common.c:290
> + if (ret < 0)
> + goto nla_put_failure;
>
> - nla_nest_end(skb, nest_parms);
> - }
> + nla_nest_end(skb, nest_parms);
>
> nlmsg_end(skb, nlh);
> return skb->len;
> diff --git a/net/netfilter/nft_ct.c b/net/netfilter/nft_ct.c
> index 5dd87748afa8..17ae5059c312 100644
> --- a/net/netfilter/nft_ct.c
> +++ b/net/netfilter/nft_ct.c
> @@ -776,9 +776,6 @@ nft_ct_timeout_parse_policy(void *timeouts,
> struct nlattr **tb;
> int ret = 0;
>
> - if (!l4proto->ctnl_timeout.nlattr_to_obj)
> - return 0;
> -
> tb = kcalloc(l4proto->ctnl_timeout.nlattr_max + 1, sizeof(*tb),
> GFP_KERNEL);
>
>
^ permalink raw reply
* Re: [PULL] vhost: cleanups and fixes
From: Michael S. Tsirkin @ 2018-11-01 23:55 UTC (permalink / raw)
To: Linus Torvalds
Cc: Kees Cook, kvm, virtualization, netdev, Linux Kernel Mailing List,
Andrew Morton, bijan.mottahedeh, gedwards, joe, lenaic,
liang.z.li, mhocko, mhocko, stefanha, wei.w.wang
In-Reply-To: <CAHk-=wicvws38JqzVF6oNEZ0jGzP6RecR6yAGtyzX3AkoJ321g@mail.gmail.com>
On Thu, Nov 01, 2018 at 04:06:19PM -0700, Linus Torvalds wrote:
> On Thu, Nov 1, 2018 at 4:00 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > + memset(&rsp, 0, sizeof(rsp));
> > + rsp.response = VIRTIO_SCSI_S_FUNCTION_REJECTED;
> > + resp = vq->iov[out].iov_base;
> > + ret = __copy_to_user(resp, &rsp, sizeof(rsp));
> >
> > Is it actually safe to trust that iov_base has passed an earlier
> > access_ok() check here? Why not just use copy_to_user() instead?
>
> Good point.
>
> We really should have removed those double-underscore things ages ago.
Well in case of vhost there are a bunch of reasons to keep them.
One is that all access_ok checks take place
way earlier in context of the owner task. Result is saved
and then used for access repeatedly. Skipping reding access_ok twice
did seem to give a small speedup in the past.
In fact I am looking
into switching some of the uses to unsafe_put_user/unsafe_get_user
after doing something like barrier_nospec after the
access_ok checks. Seems to give a measureable speedup.
Another is that the double underscore things actually can be done
in softirq context if you do preempt_disable/preempt_enable.
IIUC Jason's looking into using that to cut down the latency
for when the access is very small.
> Also, apart from the address, what about the size? Wouldn't it be
> better to use copy_to_iter() rather than implement it badly by hand?
>
> Linus
Generally size is checked when we retrieve the iov but I will recheck
this case and reply here.
^ permalink raw reply
* Re: [PULL] vhost: cleanups and fixes
From: Michael S. Tsirkin @ 2018-11-01 23:38 UTC (permalink / raw)
To: Kees Cook
Cc: lenaic, Michal Hocko, bijan.mottahedeh, KVM, Network Development,
liang.z.li, LKML, virtualization, Stefan Hajnoczi, Joe Perches,
Andrew Morton, Michal Hocko, Linus Torvalds
In-Reply-To: <CAGXu5jJ0HgV2qN=wohEgro6ixqXHOHBTsvS5a9Dcpz8gxVo3bA@mail.gmail.com>
On Thu, Nov 01, 2018 at 04:00:23PM -0700, Kees Cook wrote:
> On Thu, Nov 1, 2018 at 2:19 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > The following changes since commit 84df9525b0c27f3ebc2ebb1864fa62a97fdedb7d:
> >
> > Linux 4.19 (2018-10-22 07:37:37 +0100)
> >
> > are available in the Git repository at:
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git tags/for_linus
> >
> > for you to fetch changes up to 79f800b2e76923cd8ce0aa659cb5c019d9643bc9:
> >
> > MAINTAINERS: remove reference to bogus vsock file (2018-10-24 21:16:14 -0400)
> >
> > ----------------------------------------------------------------
> > virtio, vhost: fixes, tweaks
> >
> > virtio balloon page hinting support
> > vhost scsi control queue
> >
> > misc fixes.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> > ----------------------------------------------------------------
> > Bijan Mottahedeh (3):
> > vhost/scsi: Respond to control queue operations
>
> +static void
> +vhost_scsi_send_tmf_resp(struct vhost_scsi *vs,
> + struct vhost_virtqueue *vq,
> + int head, unsigned int out)
> +{
> + struct virtio_scsi_ctrl_tmf_resp __user *resp;
> + struct virtio_scsi_ctrl_tmf_resp rsp;
> + int ret;
> +
> + pr_debug("%s\n", __func__);
> + memset(&rsp, 0, sizeof(rsp));
> + rsp.response = VIRTIO_SCSI_S_FUNCTION_REJECTED;
> + resp = vq->iov[out].iov_base;
> + ret = __copy_to_user(resp, &rsp, sizeof(rsp));
>
> Is it actually safe to trust that iov_base has passed an earlier
> access_ok() check here? Why not just use copy_to_user() instead?
>
> -Kees
I am not sure copy_to_user will do the right thing here, because all
this runs in context of a kernel thread. We do need access_ok which
takes place way earlier in context of the task.
Another reason it is safe is because the address is not
coming from userspace at all.
> > vhost/scsi: Extract common handling code from control queue handler
> > vhost/scsi: Use common handling code in request queue handler
> >
> > Greg Edwards (1):
> > vhost/scsi: truncate T10 PI iov_iter to prot_bytes
> >
> > Lénaïc Huard (1):
> > kvm_config: add CONFIG_VIRTIO_MENU
> >
> > Stefan Hajnoczi (1):
> > MAINTAINERS: remove reference to bogus vsock file
> >
> > Wei Wang (3):
> > virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT
> > mm/page_poison: expose page_poisoning_enabled to kernel modules
> > virtio-balloon: VIRTIO_BALLOON_F_PAGE_POISON
> >
> > MAINTAINERS | 1 -
> > drivers/vhost/scsi.c | 426 ++++++++++++++++++++++++++++--------
> > drivers/virtio/virtio_balloon.c | 380 +++++++++++++++++++++++++++++---
> > include/uapi/linux/virtio_balloon.h | 8 +
> > kernel/configs/kvm_guest.config | 1 +
> > mm/page_poison.c | 6 +
> > 6 files changed, 688 insertions(+), 134 deletions(-)
>
>
>
> --
> Kees Cook
^ permalink raw reply
* Re: [net 1/8] igb: shorten maximum PHC timecounter update interval
From: Miroslav Lichvar @ 2018-11-01 14:12 UTC (permalink / raw)
To: Jeff Kirsher
Cc: davem, netdev, nhorman, sassmann, Jacob Keller, Richard Cochran
In-Reply-To: <20181031194254.16417-2-jeffrey.t.kirsher@intel.com>
On Wed, Oct 31, 2018 at 12:42:47PM -0700, Jeff Kirsher wrote:
> From: Miroslav Lichvar <mlichvar@redhat.com>
>
> The timecounter needs to be updated at least once per ~550 seconds in
> order to avoid a 40-bit SYSTIM timestamp to be misinterpreted as an old
> timestamp.
>
> Since commit 500462a9d ("timers: Switch to a non-cascading wheel"),
> scheduling of delayed work seems to be less accurate and a requested
> delay of 540 seconds may actually be longer than 550 seconds. Shorten
> the delay to 480 seconds to be sure the timecounter is updated in time.
It looks like this is the v1 of the patch. There was a v2 I sent on
Oct 26, which made the interval even shorter. I can send a separate
patch for that change.
--
Miroslav Lichvar
^ permalink raw reply
* Re: [PULL] vhost: cleanups and fixes
From: Linus Torvalds @ 2018-11-01 23:06 UTC (permalink / raw)
To: Kees Cook
Cc: mst, kvm, virtualization, netdev, Linux Kernel Mailing List,
Andrew Morton, bijan.mottahedeh, gedwards, joe, lenaic,
liang.z.li, mhocko, mhocko, stefanha, wei.w.wang
In-Reply-To: <CAGXu5jJ0HgV2qN=wohEgro6ixqXHOHBTsvS5a9Dcpz8gxVo3bA@mail.gmail.com>
On Thu, Nov 1, 2018 at 4:00 PM Kees Cook <keescook@chromium.org> wrote:
>
> + memset(&rsp, 0, sizeof(rsp));
> + rsp.response = VIRTIO_SCSI_S_FUNCTION_REJECTED;
> + resp = vq->iov[out].iov_base;
> + ret = __copy_to_user(resp, &rsp, sizeof(rsp));
>
> Is it actually safe to trust that iov_base has passed an earlier
> access_ok() check here? Why not just use copy_to_user() instead?
Good point.
We really should have removed those double-underscore things ages ago.
Also, apart from the address, what about the size? Wouldn't it be
better to use copy_to_iter() rather than implement it badly by hand?
Linus
^ permalink raw reply
* [PATCH net 2/2] net: systemport: Protect stop from timeout
From: Florian Fainelli @ 2018-11-01 22:55 UTC (permalink / raw)
To: netdev; +Cc: jaedon.shin, opendmb, Florian Fainelli, David S. Miller,
open list
In-Reply-To: <20181101225538.18632-1-f.fainelli@gmail.com>
A timing hazard exists when the network interface is stopped that
allows a watchdog timeout to be processed by a separate core in
parallel. This creates the potential for the timeout handler to
wake the queues while the driver is shutting down, or access
registers after their clocks have been removed.
The more common case is that the watchdog timeout will produce a
warning message which doesn't lead to a crash. The chances of this
are greatly increased by the fact that bcm_sysport_netif_stop stops
the transmit queues which can easily precipitate a watchdog time-
out because of stale trans_start data in the queues.
This commit corrects the behavior by ensuring that the watchdog
timeout is disabled before enterring bcm_sysport_netif_stop. There
are currently only two users of the bcm_sysport_netif_stop function:
close and suspend.
The close case already handles the issue by exiting the RUNNING
state before invoking the driver close service.
The suspend case now performs the netif_device_detach to exit the
PRESENT state before the call to bcm_sysport_netif_stop rather than
after it.
These behaviors prevent any future scheduling of the driver timeout
service during the window. The netif_tx_stop_all_queues function
in bcm_sysport_netif_stop is replaced with netif_tx_disable to ensure
synchronization with any transmit or timeout threads that may
already be executing on other cores.
For symmetry, the netif_device_attach call upon resume is moved to
after the call to bcm_sysport_netif_start. Since it wakes the transmit
queues it is not necessary to invoke netif_tx_start_all_queues from
bcm_sysport_netif_start so it is moved into the driver open service.
Fixes: 40755a0fce17 ("net: systemport: add suspend and resume support")
Fixes: 80105befdb4b ("net: systemport: add Broadcom SYSTEMPORT Ethernet MAC driver")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/net/ethernet/broadcom/bcmsysport.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
index 4122553e224b..0e2d99c737e3 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.c
+++ b/drivers/net/ethernet/broadcom/bcmsysport.c
@@ -1902,9 +1902,6 @@ static void bcm_sysport_netif_start(struct net_device *dev)
intrl2_1_mask_clear(priv, 0xffffffff);
else
intrl2_0_mask_clear(priv, INTRL2_0_TDMA_MBDONE_MASK);
-
- /* Last call before we start the real business */
- netif_tx_start_all_queues(dev);
}
static void rbuf_init(struct bcm_sysport_priv *priv)
@@ -2048,6 +2045,8 @@ static int bcm_sysport_open(struct net_device *dev)
bcm_sysport_netif_start(dev);
+ netif_tx_start_all_queues(dev);
+
return 0;
out_clear_rx_int:
@@ -2071,7 +2070,7 @@ static void bcm_sysport_netif_stop(struct net_device *dev)
struct bcm_sysport_priv *priv = netdev_priv(dev);
/* stop all software from updating hardware */
- netif_tx_stop_all_queues(dev);
+ netif_tx_disable(dev);
napi_disable(&priv->napi);
cancel_work_sync(&priv->dim.dim.work);
phy_stop(dev->phydev);
@@ -2658,12 +2657,12 @@ static int __maybe_unused bcm_sysport_suspend(struct device *d)
if (!netif_running(dev))
return 0;
+ netif_device_detach(dev);
+
bcm_sysport_netif_stop(dev);
phy_suspend(dev->phydev);
- netif_device_detach(dev);
-
/* Disable UniMAC RX */
umac_enable_set(priv, CMD_RX_EN, 0);
@@ -2746,8 +2745,6 @@ static int __maybe_unused bcm_sysport_resume(struct device *d)
goto out_free_rx_ring;
}
- netif_device_attach(dev);
-
/* RX pipe enable */
topctrl_writel(priv, 0, RX_FLUSH_CNTL);
@@ -2788,6 +2785,8 @@ static int __maybe_unused bcm_sysport_resume(struct device *d)
bcm_sysport_netif_start(dev);
+ netif_device_attach(dev);
+
return 0;
out_free_rx_ring:
--
2.17.1
^ permalink raw reply related
* [PATCH net 1/2] net: bcmgenet: protect stop from timeout
From: Florian Fainelli @ 2018-11-01 22:55 UTC (permalink / raw)
To: netdev; +Cc: jaedon.shin, opendmb, Florian Fainelli, David S. Miller,
open list
In-Reply-To: <20181101225538.18632-1-f.fainelli@gmail.com>
From: Doug Berger <opendmb@gmail.com>
A timing hazard exists when the network interface is stopped that
allows a watchdog timeout to be processed by a separate core in
parallel. This creates the potential for the timeout handler to
wake the queues while the driver is shutting down, or access
registers after their clocks have been removed.
The more common case is that the watchdog timeout will produce a
warning message which doesn't lead to a crash. The chances of this
are greatly increased by the fact that bcmgenet_netif_stop stops
the transmit queues which can easily precipitate a watchdog time-
out because of stale trans_start data in the queues.
This commit corrects the behavior by ensuring that the watchdog
timeout is disabled before enterring bcmgenet_netif_stop. There
are currently only two users of the bcmgenet_netif_stop function:
close and suspend.
The close case already handles the issue by exiting the RUNNING
state before invoking the driver close service.
The suspend case now performs the netif_device_detach to exit the
PRESENT state before the call to bcmgenet_netif_stop rather than
after it.
These behaviors prevent any future scheduling of the driver timeout
service during the window. The netif_tx_stop_all_queues function
in bcmgenet_netif_stop is replaced with netif_tx_disable to ensure
synchronization with any transmit or timeout threads that may
already be executing on other cores.
For symmetry, the netif_device_attach call upon resume is moved to
after the call to bcmgenet_netif_start. Since it wakes the transmit
queues it is not necessary to invoke netif_tx_start_all_queues from
bcmgenet_netif_start so it is moved into the driver open service.
Fixes: 1c1008c793fa ("net: bcmgenet: add main driver file")
Signed-off-by: Doug Berger <opendmb@gmail.com>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/net/ethernet/broadcom/genet/bcmgenet.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 20c1681bb1af..2d6f090bf644 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -2855,7 +2855,6 @@ static void bcmgenet_netif_start(struct net_device *dev)
umac_enable_set(priv, CMD_TX_EN | CMD_RX_EN, true);
- netif_tx_start_all_queues(dev);
bcmgenet_enable_tx_napi(priv);
/* Monitor link interrupts now */
@@ -2937,6 +2936,8 @@ static int bcmgenet_open(struct net_device *dev)
bcmgenet_netif_start(dev);
+ netif_tx_start_all_queues(dev);
+
return 0;
err_irq1:
@@ -2958,7 +2959,7 @@ static void bcmgenet_netif_stop(struct net_device *dev)
struct bcmgenet_priv *priv = netdev_priv(dev);
bcmgenet_disable_tx_napi(priv);
- netif_tx_stop_all_queues(dev);
+ netif_tx_disable(dev);
/* Disable MAC receive */
umac_enable_set(priv, CMD_RX_EN, false);
@@ -3620,13 +3621,13 @@ static int bcmgenet_suspend(struct device *d)
if (!netif_running(dev))
return 0;
+ netif_device_detach(dev);
+
bcmgenet_netif_stop(dev);
if (!device_may_wakeup(d))
phy_suspend(dev->phydev);
- netif_device_detach(dev);
-
/* Prepare the device for Wake-on-LAN and switch to the slow clock */
if (device_may_wakeup(d) && priv->wolopts) {
ret = bcmgenet_power_down(priv, GENET_POWER_WOL_MAGIC);
@@ -3700,8 +3701,6 @@ static int bcmgenet_resume(struct device *d)
/* Always enable ring 16 - descriptor ring */
bcmgenet_enable_dma(priv, dma_ctrl);
- netif_device_attach(dev);
-
if (!device_may_wakeup(d))
phy_resume(dev->phydev);
@@ -3710,6 +3709,8 @@ static int bcmgenet_resume(struct device *d)
bcmgenet_netif_start(dev);
+ netif_device_attach(dev);
+
return 0;
out_clk_disable:
--
2.17.1
^ permalink raw reply related
* Re: Kernel 4.19 network performance - forwarding/routing normal users traffic
From: Paweł Staszewski @ 2018-11-01 13:52 UTC (permalink / raw)
To: Jesper Dangaard Brouer, David Ahern; +Cc: netdev, Yoel Caspersen
In-Reply-To: <20181101115522.10b0dd0a@redhat.com>
W dniu 01.11.2018 o 11:55, Jesper Dangaard Brouer pisze:
> On Wed, 31 Oct 2018 21:37:16 -0600 David Ahern <dsahern@gmail.com> wrote:
>
>> This is mainly a forwarding use case? Seems so based on the perf report.
>> I suspect forwarding with XDP would show pretty good improvement.
> Yes, significant performance improvements.
>
> Notice Davids talk: "Leveraging Kernel Tables with XDP"
> http://vger.kernel.org/lpc-networking2018.html#session-1
It will be rly interesting
> It looks like that you are doing "pure" IP-routing, without any
> iptables conntrack stuff (from your perf report data). That will
> actually be a really good use-case for accelerating this with XDP.
Yes pure IP routing
iptables used only for some local input filtering.
>
> I want you to understand the philosophy behind how David and I want
> people to leverage XDP. Think of XDP as a software offload layer for
> the kernel network stack. Setup and use Linux kernel network stack, but
> accelerate parts of it with XDP, e.g. the route FIB lookup.
>
> Sample code avail here:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/samples/bpf/xdp_fwd_kern.c
I can try some tests on same hw but testlab configuration - will give it
a try :)
> (I do warn, what we just found a bug/crash in setup+tairdown for the
> mlx5 driver you are using, that we/mlx _will_ fix soon)
Ok
>
>
>> You need the vlan changes I have queued up though.
> I know Yoel will be very interested in those changes too! I've
> convinced Yoel to write an XDP program for his Border Network Gateway
> (BNG) production system[1], and his is a heavy VLAN user. And the plan
> is to Open Source this when he have-something-working.
>
> [1] https://www.version2.dk/blog/software-router-del-5-linux-bng-1086060
Ok - for now i need to split traffic into two separate 100G ports placed
in two different x16 pciexpress slots to check if the problem is mainly
caused by no more pciex x16 bandwidth available.
^ permalink raw reply
* Re: [PATCH net-next 5/6] net/ncsi: Reset channel state in ncsi_start_dev()
From: Samuel Mendoza-Jonas @ 2018-11-01 22:53 UTC (permalink / raw)
To: Justin.Lee1, netdev; +Cc: davem, linux-kernel, openbmc
In-Reply-To: <96158616f0774e3d9fae0e99bb16e605@AUSX13MPS306.AMER.DELL.COM>
On Tue, 2018-10-30 at 21:26 +0000, Justin.Lee1@Dell.com wrote:
> > +int ncsi_reset_dev(struct ncsi_dev *nd)
> > +{
> > + struct ncsi_dev_priv *ndp = TO_NCSI_DEV_PRIV(nd);
> > + struct ncsi_channel *nc, *active;
> > + struct ncsi_package *np;
> > + unsigned long flags;
> > + bool enabled;
> > + int state;
> > +
> > + active = NULL;
> > + NCSI_FOR_EACH_PACKAGE(ndp, np) {
> > + NCSI_FOR_EACH_CHANNEL(np, nc) {
> > + spin_lock_irqsave(&nc->lock, flags);
> > + enabled = nc->monitor.enabled;
> > + state = nc->state;
> > + spin_unlock_irqrestore(&nc->lock, flags);
> > +
> > + if (enabled)
> > + ncsi_stop_channel_monitor(nc);
> > + if (state == NCSI_CHANNEL_ACTIVE) {
> > + active = nc;
> > + break;
>
> Is the original intention to process the channel one by one?
> If it is the case, there are two loops and we might need to use
> "goto found" instead.
Yes we'll need to break out of the package loop here as well.
>
> > + }
> > + }
> > + }
> > +
>
> found: ?
>
> > + if (!active) {
> > + /* Done */
> > + spin_lock_irqsave(&ndp->lock, flags);
> > + ndp->flags &= ~NCSI_DEV_RESET;
> > + spin_unlock_irqrestore(&ndp->lock, flags);
> > + return ncsi_choose_active_channel(ndp);
> > + }
> > +
> > + spin_lock_irqsave(&ndp->lock, flags);
> > + ndp->flags |= NCSI_DEV_RESET;
> > + ndp->active_channel = active;
> > + ndp->active_package = active->package;
> > + spin_unlock_irqrestore(&ndp->lock, flags);
> > +
> > + nd->state = ncsi_dev_state_suspend;
> > + schedule_work(&ndp->work);
> > + return 0;
> > +}
>
> Also similar issue in ncsi_choose_active_channel() function below.
>
> > @@ -916,32 +1045,49 @@ static int ncsi_choose_active_channel(struct ncsi_dev_priv *ndp)
> >
> > ncm = &nc->modes[NCSI_MODE_LINK];
> > if (ncm->data[2] & 0x1) {
> > - spin_unlock_irqrestore(&nc->lock, flags);
> > found = nc;
> > - goto out;
> > + with_link = true;
> > }
> >
> > - spin_unlock_irqrestore(&nc->lock, flags);
> > + /* If multi_channel is enabled configure all valid
> > + * channels whether or not they currently have link
> > + * so they will have AENs enabled.
> > + */
> > + if (with_link || np->multi_channel) {
>
> I notice that there is a case that we will misconfigure the interface.
> For example below, multi-channel is not enable for package 1.
> But we enable the channel for ncsi2 below (package 1 channel 0) as that interface is the first
> channel for that package with link.
I don't think I see the issue here; multi-channel is not set on package
1, but both channels are in the channel whitelist. Channel 0 is
configured since it's the first found on package 1, and channel 1 is not
since channel 0 is already found. Are you expecting something different?
>
> cat /sys/kernel/debug/ncsi_protocol/ncsi_device_
> IFIDX IFNAME NAME PID CID RX TX MP MC WP WC PC CS PS LS RU CR NQ HA
> =====================================================================
> 2 eth2 ncsi0 000 000 1 1 1 1 1 1 0 2 1 1 1 1 0 1
> 2 eth2 ncsi1 000 001 1 0 1 1 1 1 0 2 1 1 1 1 0 1
> 2 eth2 ncsi2 001 000 1 0 1 0 1 1 0 2 1 1 1 1 0 1
> 2 eth2 ncsi3 001 001 0 0 1 0 1 1 0 1 0 1 1 1 0 1
> =====================================================================
> MP: Multi-mode Package WP: Whitelist Package
> MC: Multi-mode Channel WC: Whitelist Channel
> PC: Primary Channel CS: Channel State IA/A/IV 1/2/3
> PS: Poll Status LS: Link Status
> RU: Running CR: Carrier OK
> NQ: Queue Stopped HA: Hardware Arbitration
>
> I temporally change to the following to avoid that.
> if ((with_link &&
> !np->multi_channel &&
> list_empty(&ndp->channel_queue)) || np->multi_channel) {
>
> > + spin_lock_irqsave(&ndp->lock, flags);
> > + list_add_tail_rcu(&nc->link,
> > + &ndp->channel_queue);
> > + spin_unlock_irqrestore(&ndp->lock, flags);
> > +
> > + netdev_dbg(ndp->ndev.dev,
> > + "NCSI: Channel %u added to queue (link %s)\n",
> > + nc->id,
> > + ncm->data[2] & 0x1 ? "up" : "down");
> > + }
> > +
> > + spin_unlock_irqrestore(&nc->lock, cflags);
> > +
> > + if (with_link && !np->multi_channel)
> > + break;
>
> Similar issue here. As we are using break, so each package will configure one active TX.
>
I believe this is handled properly in ncsi_channel_is_tx() in the most
recent revision.
> > }
> > + if (with_link && !ndp->multi_package)
> > + break;
> > }
> >
> > - if (!found) {
> > + if (list_empty(&ndp->channel_queue) && found) {
> > + netdev_info(ndp->ndev.dev,
> > + "NCSI: No channel with link found, configuring channel %u\n",
> > + found->id);
> > + spin_lock_irqsave(&ndp->lock, flags);
> > + list_add_tail_rcu(&found->link, &ndp->channel_queue);
> > + spin_unlock_irqrestore(&ndp->lock, flags);
> > + } else if (!found) {
> > netdev_warn(ndp->ndev.dev,
> > - "NCSI: No channel found with link\n");
> > + "NCSI: No channel found to configure!\n");
> > ncsi_report_link(ndp, true);
> > return -ENODEV;
> > }
>
> Also, for deselect package handler function, do we want to set to inactive here?
> If we just change the state, the cached data still keeps the old value. If the new
> ncsi_reset_dev() function is handling one by one, can we skip this part?
Technically yes we could skip the state change here since
ncsi_reset_dev() will have already done it. However if we send a DP
command via some other means then it is probably best to ensure we treat
all channels on that package as inactive.
>
> static int ncsi_rsp_handler_dp(struct ncsi_request *nr)
> {
> struct ncsi_rsp_pkt *rsp;
> struct ncsi_dev_priv *ndp = nr->ndp;
> struct ncsi_package *np;
> struct ncsi_channel *nc;
> unsigned long flags;
>
> /* Find the package */
> rsp = (struct ncsi_rsp_pkt *)skb_network_header(nr->rsp);
> ncsi_find_package_and_channel(ndp, rsp->rsp.common.channel,
> &np, NULL);
> if (!np)
> return -ENODEV;
>
> /* Change state of all channels attached to the package */
> NCSI_FOR_EACH_CHANNEL(np, nc) {
> spin_lock_irqsave(&nc->lock, flags);
> nc->state = NCSI_CHANNEL_INACTIVE;
>
> spin_unlock_irqrestore(&nc->lock, flags);
> }
>
> return 0;
> }
>
>
^ permalink raw reply
* Re: [PATCH v1 net] net: dsa: microchip: initialize mutex before use
From: Pavel Machek @ 2018-11-01 13:47 UTC (permalink / raw)
To: Andrew Lunn
Cc: Tristram.Ha, David S. Miller, Florian Fainelli, UNGLinuxDriver,
netdev
In-Reply-To: <20181101121719.GA7652@lunn.ch>
[-- Attachment #1: Type: text/plain, Size: 634 bytes --]
On Thu 2018-11-01 13:17:19, Andrew Lunn wrote:
> On Wed, Oct 31, 2018 at 07:49:08PM -0700, Tristram.Ha@microchip.com wrote:
> > From: Tristram Ha <Tristram.Ha@microchip.com>
> >
> > Initialize mutex before use. Avoid kernel complaint when
> > CONFIG_DEBUG_LOCK_ALLOC is enabled.
> >
> > Fixes: b987e98e50ab90e5 ("dsa: add DSA switch driver for Microchip KSZ9477")
> > Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
Reviewed-by: Pavel Machek <pavel@ucw.cz>
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH net] rtnetlink: invoke 'cb->done' destructor before 'cb->args' reset
From: Alexey Kodanev @ 2018-11-01 13:42 UTC (permalink / raw)
To: David Ahern, netdev; +Cc: David Miller
In-Reply-To: <dbd48fde-3a7f-2800-4317-4eab2ecfd41d@oracle.com>
On 11/01/2018 04:11 PM, Alexey Kodanev wrote:
> On 10/31/2018 08:35 PM, David Ahern wrote:
>> On 10/31/18 10:55 AM, David Ahern wrote:
>>> I think the simplest fix for 4.20 is to break the loop if ret is non-0 -
>>> restore the previous behavior.
>>
>> that is the only recourse. It has to bail if ret is non-0. Do you want
>> to send a patch with that fix?
>>
>
> I see, and inet6_dump_fib() cleanups fib6_walker if ret is zero. Will send the fix.
Can it happen that inet6_dump_fib() returns skb->len (0) in the below cases?
* if (arg.filter.flags & RTM_F_CLONED)
return skb->len;
...
w = (void *)cb->args[2];
if (!w) {
...
w = kzalloc(...)
...
* if (arg.filter.table_id) {
...
if (!tb) {
if (arg.filter.dump_all_families)
return skb->len;
Would it be safer to add "res = skb->len; goto out;" instead of "return skb->len;"
so that it can call fib6_dump_end() for "res <= 0"? Or use cb->data instead of
cb->args?
^ permalink raw reply
* RE: [PATCH iproute2-next] rdma: Refresh help section of resource information
From: Steve Wise @ 2018-11-01 13:22 UTC (permalink / raw)
To: 'Leon Romanovsky', 'David Ahern'
Cc: 'Leon Romanovsky', 'netdev',
'RDMA mailing list', 'Stephen Hemminger'
In-Reply-To: <20181101083505.25152-1-leon@kernel.org>
> -----Original Message-----
> From: Leon Romanovsky <leon@kernel.org>
> Sent: Thursday, November 1, 2018 3:35 AM
> To: David Ahern <dsahern@gmail.com>
> Cc: Leon Romanovsky <leonro@mellanox.com>; netdev
> <netdev@vger.kernel.org>; RDMA mailing list <linux-rdma@vger.kernel.org>;
> Stephen Hemminger <stephen@networkplumber.org>; Steve Wise
> <swise@opengridcomputing.com>
> Subject: [PATCH iproute2-next] rdma: Refresh help section of resource
> information
>
> From: Leon Romanovsky <leonro@mellanox.com>
>
> After commit 4060e4c0d257 ("rdma: Add PD resource tracking
> information"), the resource information shows PDs and MRs,
> but help pages didn't fully reflect it.
>
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
Oops. Thanks. Looks fine.
Reviewed-by: Steve Wise <swise@opengridcomputing.com>
^ permalink raw reply
* Re: [PATCH net] rtnetlink: invoke 'cb->done' destructor before 'cb->args' reset
From: Alexey Kodanev @ 2018-11-01 13:11 UTC (permalink / raw)
To: David Ahern, netdev; +Cc: David Miller
In-Reply-To: <50d88abb-bfba-6c70-af68-7bac60a0d4b1@gmail.com>
On 10/31/2018 08:35 PM, David Ahern wrote:
> On 10/31/18 10:55 AM, David Ahern wrote:
>> I think the simplest fix for 4.20 is to break the loop if ret is non-0 -
>> restore the previous behavior.
>
> that is the only recourse. It has to bail if ret is non-0. Do you want
> to send a patch with that fix?
>
I see, and inet6_dump_fib() cleanups fib6_walker if ret is zero. Will send the fix.
^ permalink raw reply
* Re: [RFC PATCH] lib: Introduce generic __cmpxchg_u64() and use it where needed
From: Peter Zijlstra @ 2018-11-01 21:45 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: Paul E. McKenney, Trond Myklebust, mark.rutland@arm.com,
linux-kernel@vger.kernel.org, ralf@linux-mips.org,
jlayton@kernel.org, linuxppc-dev@lists.ozlabs.org,
bfields@fieldses.org, linux-mips@linux-mips.org,
linux@roeck-us.net, linux-nfs@vger.kernel.org,
akpm@linux-foundation.org, will.deacon@arm.com,
boqun.feng@gmail.com, paul.burton@mips.com,
"anna.schumaker@netapp.com
In-Reply-To: <CACT4Y+aC45BtS88DXarn3A+LV2RRRsPQoSs_3_DnKjU4O3AMHQ@mail.gmail.com>
On Thu, Nov 01, 2018 at 06:46:50PM +0100, Dmitry Vyukov wrote:
> If there is a warning that we don't want to see at all, then we can
> disable it. It supposed to be a useful tool, rather than a thing in
> itself that lives own life. We already I think removed 1 particularly
> noisy warning and made another optional via a config.
> But the thing with overflows is that, even if it's defined, it's not
> necessary the intended behavior. For example, take allocation size
> calculation done via unsigned size_t. If it overflows it does not help
> if C defines result or not, it still gives a user controlled write
> primitive. We've seen similar cases with timeout/deadline calculation
> in kernel, we really don't want it to just wrap modulo-2, right. Some
> user-space projects even test with unsigned overflow warnings or
> implicit truncation warnings, which are formally legal, but frequently
> bugs.
Sure; but then don't call it UB.
If we want to have an additional integer over/underflow checker (ideally
with a gcc plugin that has explicit annotations like __wrap to make it
go away) that is fine; and it can be done on unsigned and signed.
^ permalink raw reply
* Re: [RFC PATCH] lib: Introduce generic __cmpxchg_u64() and use it where needed
From: Peter Zijlstra @ 2018-11-01 21:38 UTC (permalink / raw)
To: Paul E. McKenney
Cc: Eric Dumazet, Trond Myklebust, mark.rutland@arm.com,
linux-kernel@vger.kernel.org, ralf@linux-mips.org,
jlayton@kernel.org, linuxppc-dev@lists.ozlabs.org,
bfields@fieldses.org, linux-mips@linux-mips.org,
linux@roeck-us.net, linux-nfs@vger.kernel.org,
akpm@linux-foundation.org, will.deacon@arm.com,
boqun.feng@gmail.com, paul.burton@mips.com,
"anna.schumaker@netapp.com" <an
In-Reply-To: <20181101202910.GB4170@linux.ibm.com>
On Thu, Nov 01, 2018 at 01:29:10PM -0700, Paul E. McKenney wrote:
> On Thu, Nov 01, 2018 at 06:27:39PM +0100, Peter Zijlstra wrote:
> > On Thu, Nov 01, 2018 at 06:14:32PM +0100, Peter Zijlstra wrote:
> > > > This reminds me of this sooooo silly patch :/
> > > >
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=adb03115f4590baa280ddc440a8eff08a6be0cb7
> >
> > You'd probably want to write it like so; +- some ordering stuff, that
> > code didn't look like it really needs the memory barriers implied by
> > these, but I didn't look too hard.
>
> The atomic_fetch_add() API would need to be propagated out to the other
> architectures, correct?
Like these commits I did like 2 years ago ? :-)
$ git log --oneline 6dc25876cdb1...1f51dee7ca74
6dc25876cdb1 locking/atomic, arch/xtensa: Implement atomic_fetch_{add,sub,and,or,xor}()
a8bcccaba162 locking/atomic, arch/x86: Implement atomic{,64}_fetch_{add,sub,and,or,xor}()
1af5de9af138 locking/atomic, arch/tile: Implement atomic{,64}_fetch_{add,sub,and,or,xor}()
3a1adb23a52c locking/atomic, arch/sparc: Implement atomic{,64}_fetch_{add,sub,and,or,xor}()
7d9794e75237 locking/atomic, arch/sh: Implement atomic_fetch_{add,sub,and,or,xor}()
56fefbbc3f13 locking/atomic, arch/s390: Implement atomic{,64}_fetch_{add,sub,and,or,xor}()
a28cc7bbe8e3 locking/atomic, arch/powerpc: Implement atomic{,64}_fetch_{add,sub,and,or,xor}{,_relaxed,_acquire,_release}()
e5857a6ed600 locking/atomic, arch/parisc: Implement atomic{,64}_fetch_{add,sub,and,or,xor}()
f8d638e28d7c locking/atomic, arch/mn10300: Implement atomic_fetch_{add,sub,and,or,xor}()
4edac529eb62 locking/atomic, arch/mips: Implement atomic{,64}_fetch_{add,sub,and,or,xor}()
e898eb27ffd8 locking/atomic, arch/metag: Implement atomic_fetch_{add,sub,and,or,xor}()
e39d88ea3ce4 locking/atomic, arch/m68k: Implement atomic_fetch_{add,sub,and,or,xor}()
f64937052303 locking/atomic, arch/m32r: Implement atomic_fetch_{add,sub,and,or,xor}()
cc102507fac7 locking/atomic, arch/ia64: Implement atomic{,64}_fetch_{add,sub,and,or,xor}()
4be7dd393515 locking/atomic, arch/hexagon: Implement atomic_fetch_{add,sub,and,or,xor}()
0c074cbc3309 locking/atomic, arch/h8300: Implement atomic_fetch_{add,sub,and,or,xor}()
d9c730281617 locking/atomic, arch/frv: Implement atomic{,64}_fetch_{add,sub,and,or,xor}()
e87fc0ec0705 locking/atomic, arch/blackfin: Implement atomic_fetch_{add,sub,and,or,xor}()
1a6eafacd481 locking/atomic, arch/avr32: Implement atomic_fetch_{add,sub,and,or,xor}()
2efe95fe6952 locking/atomic, arch/arm64: Implement atomic{,64}_fetch_{add,sub,and,andnot,or,xor}{,_relaxed,_acquire,_release}() for LSE instructions
6822a84dd4e3 locking/atomic, arch/arm64: Generate LSE non-return cases using common macros
e490f9b1d3b4 locking/atomic, arch/arm64: Implement atomic{,64}_fetch_{add,sub,and,andnot,or,xor}{,_relaxed,_acquire,_release}()
6da068c1beba locking/atomic, arch/arm: Implement atomic{,64}_fetch_{add,sub,and,andnot,or,xor}{,_relaxed,_acquire,_release}()
fbffe892e525 locking/atomic, arch/arc: Implement atomic_fetch_{add,sub,and,andnot,or,xor}()
^ permalink raw reply
* [PATCH net] bonding/802.3ad: fix link_failure_count tracking
From: Jarod Wilson @ 2018-11-01 21:22 UTC (permalink / raw)
To: linux-kernel
Cc: Jarod Wilson, Mahesh Bandewar, David S . Miller, netdev, stable
Commit 4d2c0cda07448ea6980f00102dc3964eb25e241c set slave->link to
BOND_LINK_DOWN for 802.3ad bonds whenever invalid speed/duplex values
were read, to fix a problem with slaves getting into weird states, but
in the process, broke tracking of link failures, as going straight to
BOND_LINK_DOWN when a link is indeed down (cable pulled, switch rebooted)
means we broke out of bond_miimon_inspect()'s BOND_LINK_DOWN case because
!link_state was already true, we never incremented commit, and never got
a chance to call bond_miimon_commit(), where slave->link_failure_count
would be incremented. I believe the simple fix here is to mark the slave
as BOND_LINK_FAIL, and let bond_miimon_inspect() transition the link from
_FAIL to either _UP or _DOWN, and in the latter case, we now get proper
incrementing of link_failure_count again.
Fixes: 4d2c0cda07448ea6980f00102dc3964eb25e241c
CC: Mahesh Bandewar <maheshb@google.com>
CC: David S. Miller <davem@davemloft.net>
CC: netdev@vger.kernel.org
CC: stable@vger.kernel.org
Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
drivers/net/bonding/bond_main.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index ffa37adb7681..333387f1f1fe 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -3112,13 +3112,13 @@ static int bond_slave_netdev_event(unsigned long event,
case NETDEV_CHANGE:
/* For 802.3ad mode only:
* Getting invalid Speed/Duplex values here will put slave
- * in weird state. So mark it as link-down for the time
+ * in weird state. So mark it as link-fail for the time
* being and let link-monitoring (miimon) set it right when
* correct speeds/duplex are available.
*/
if (bond_update_speed_duplex(slave) &&
BOND_MODE(bond) == BOND_MODE_8023AD)
- slave->link = BOND_LINK_DOWN;
+ slave->link = BOND_LINK_FAIL;
if (BOND_MODE(bond) == BOND_MODE_8023AD)
bond_3ad_adapter_speed_duplex_changed(slave);
--
2.16.1
^ permalink raw reply related
* Re: [PATCH v1 net] net: dsa: microchip: initialize mutex before use
From: Andrew Lunn @ 2018-11-01 12:17 UTC (permalink / raw)
To: Tristram.Ha
Cc: David S. Miller, Florian Fainelli, Pavel Machek, UNGLinuxDriver,
netdev
In-Reply-To: <1541040548-13465-1-git-send-email-Tristram.Ha@microchip.com>
On Wed, Oct 31, 2018 at 07:49:08PM -0700, Tristram.Ha@microchip.com wrote:
> From: Tristram Ha <Tristram.Ha@microchip.com>
>
> Initialize mutex before use. Avoid kernel complaint when
> CONFIG_DEBUG_LOCK_ALLOC is enabled.
>
> Fixes: b987e98e50ab90e5 ("dsa: add DSA switch driver for Microchip KSZ9477")
> Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* [PULL] vhost: cleanups and fixes
From: Michael S. Tsirkin @ 2018-11-01 21:19 UTC (permalink / raw)
To: Linus Torvalds
Cc: lenaic, mhocko, bijan.mottahedeh, kvm, mst, netdev, liang.z.li,
linux-kernel, virtualization, stefanha, joe, akpm, mhocko,
torvalds
The following changes since commit 84df9525b0c27f3ebc2ebb1864fa62a97fdedb7d:
Linux 4.19 (2018-10-22 07:37:37 +0100)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git tags/for_linus
for you to fetch changes up to 79f800b2e76923cd8ce0aa659cb5c019d9643bc9:
MAINTAINERS: remove reference to bogus vsock file (2018-10-24 21:16:14 -0400)
----------------------------------------------------------------
virtio, vhost: fixes, tweaks
virtio balloon page hinting support
vhost scsi control queue
misc fixes.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
----------------------------------------------------------------
Bijan Mottahedeh (3):
vhost/scsi: Respond to control queue operations
vhost/scsi: Extract common handling code from control queue handler
vhost/scsi: Use common handling code in request queue handler
Greg Edwards (1):
vhost/scsi: truncate T10 PI iov_iter to prot_bytes
Lénaïc Huard (1):
kvm_config: add CONFIG_VIRTIO_MENU
Stefan Hajnoczi (1):
MAINTAINERS: remove reference to bogus vsock file
Wei Wang (3):
virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT
mm/page_poison: expose page_poisoning_enabled to kernel modules
virtio-balloon: VIRTIO_BALLOON_F_PAGE_POISON
MAINTAINERS | 1 -
drivers/vhost/scsi.c | 426 ++++++++++++++++++++++++++++--------
drivers/virtio/virtio_balloon.c | 380 +++++++++++++++++++++++++++++---
include/uapi/linux/virtio_balloon.h | 8 +
kernel/configs/kvm_guest.config | 1 +
mm/page_poison.c | 6 +
6 files changed, 688 insertions(+), 134 deletions(-)
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v3 1/2] kretprobe: produce sane stack traces
From: Aleksa Sarai @ 2018-11-01 21:13 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: Naveen N. Rao, Anil S Keshavamurthy, David S. Miller,
Jonathan Corbet, Peter Zijlstra, Ingo Molnar,
Arnaldo Carvalho de Melo, Alexander Shishkin, Jiri Olsa,
Namhyung Kim, Steven Rostedt, Shuah Khan, Alexei Starovoitov,
Daniel Borkmann, Brendan Gregg, Christian Brauner, Aleksa Sarai,
netdev, linux-doc, linux-kernel
In-Reply-To: <20181102002039.8f22c10fa47cae75fa709165@kernel.org>
[-- Attachment #1: Type: text/plain, Size: 2107 bytes --]
On 2018-11-02, Masami Hiramatsu <mhiramat@kernel.org> wrote:
> Please split the test case as an independent patch.
Will do. Should the Documentation/ change also be a separate patch?
> > new file mode 100644
> > index 000000000000..03146c6a1a3c
> > --- /dev/null
> > +++ b/tools/testing/selftests/ftrace/test.d/kprobe/kretprobe_stacktrace.tc
> > @@ -0,0 +1,25 @@
> > +#!/bin/sh
> > +# SPDX-License-Identifier: GPL-2.0+
> > +# description: Kretprobe dynamic event with a stacktrace
> > +
> > +[ -f kprobe_events ] || exit_unsupported # this is configurable
> > +
> > +echo 0 > events/enable
> > +echo 1 > options/stacktrace
> > +
> > +echo 'r:teststackprobe sched_fork $retval' > kprobe_events
> > +grep teststackprobe kprobe_events
> > +test -d events/kprobes/teststackprobe
>
> Hmm, what happen if we have 2 or more kretprobes on same stack?
> It seems you just save stack in pre_handler, but that stack can already
> includes another kretprobe's trampline address...
Yeah, you're quite right...
My first instinct was to do something awful like iterate over the set of
"kretprobe_instance"s with ->task == current, and then correct
kretprobe_trampoline entries using ->ret_addr. (I think this would be
correct because each task can only be in one context at once, and in
order to get to a particular kretprobe all of your caller kretprobes
were inserted before you and any sibling or later kretprobe_instances
will have been removed. But I might be insanely wrong on this front.)
However (as I noted in the other thread), there is a problem where
kretprobe_trampoline actually stops the unwinder in its tracks and thus
you only get the first kretprobe_trampoline. This is something I'm going
to look into some more (despite not having made progress on it last
time) since now it's something that actually needs to be fixed (and
as I mentioned in the other thread, show_stack() actually works on x86
in this context unlike the other stack_trace users).
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH net] net: hns3: Fix for out-of-bounds access when setting pfc back pressure
From: Yunsheng Lin @ 2018-11-01 12:12 UTC (permalink / raw)
To: davem
Cc: huangdaode, xuwei5, liguozhu, Yisen.Zhuang, john.garry, linuxarm,
yisen.zhuang, salil.mehta, lipeng321, netdev, linux-kernel
The vport should be initialized to hdev->vport for each bp group,
otherwise it will cause out-of-bounds access and bp setting not
correct problem.
[ 35.254124] BUG: KASAN: slab-out-of-bounds in hclge_pause_setup_hw+0x2a0/0x3f8 [hclge]
[ 35.254126] Read of size 2 at addr ffff803b6651581a by task kworker/0:1/14
[ 35.254132] CPU: 0 PID: 14 Comm: kworker/0:1 Not tainted 4.19.0-rc7-hulk+ #85
[ 35.254133] Hardware name: Huawei D06/D06, BIOS Hisilicon D06 UEFI RC0 - B052 (V0.52) 09/14/2018
[ 35.254141] Workqueue: events work_for_cpu_fn
[ 35.254144] Call trace:
[ 35.254147] dump_backtrace+0x0/0x2f0
[ 35.254149] show_stack+0x24/0x30
[ 35.254154] dump_stack+0x110/0x184
[ 35.254157] print_address_description+0x168/0x2b0
[ 35.254160] kasan_report+0x184/0x310
[ 35.254162] __asan_load2+0x7c/0xa0
[ 35.254170] hclge_pause_setup_hw+0x2a0/0x3f8 [hclge]
[ 35.254177] hclge_tm_init_hw+0x794/0x9f0 [hclge]
[ 35.254184] hclge_tm_schd_init+0x48/0x58 [hclge]
[ 35.254191] hclge_init_ae_dev+0x778/0x1168 [hclge]
[ 35.254196] hnae3_register_ae_dev+0x14c/0x298 [hnae3]
[ 35.254206] hns3_probe+0x88/0xa8 [hns3]
[ 35.254210] local_pci_probe+0x7c/0xf0
[ 35.254212] work_for_cpu_fn+0x34/0x50
[ 35.254214] process_one_work+0x4d4/0xa38
[ 35.254216] worker_thread+0x55c/0x8d8
[ 35.254219] kthread+0x1b0/0x1b8
[ 35.254222] ret_from_fork+0x10/0x1c
[ 35.254224] The buggy address belongs to the page:
[ 35.254228] page:ffff7e00ed994400 count:1 mapcount:0 mapping:0000000000000000 index:0x0 compound_mapcount: 0
[ 35.273835] flags: 0xfffff8000008000(head)
[ 35.282007] raw: 0fffff8000008000 dead000000000100 dead000000000200 0000000000000000
[ 35.282010] raw: 0000000000000000 0000000000000000 00000001ffffffff 0000000000000000
[ 35.282012] page dumped because: kasan: bad access detected
[ 35.282014] Memory state around the buggy address:
[ 35.282017] ffff803b66515700: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
[ 35.282019] ffff803b66515780: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
[ 35.282021] >ffff803b66515800: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
[ 35.282022] ^
[ 35.282024] ffff803b66515880: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
[ 35.282026] ffff803b66515900: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
[ 35.282028] ==================================================================
[ 35.282029] Disabling lock debugging due to kernel taint
[ 35.282747] hclge driver initialization finished.
Fixes: 67bf2541f4b9 ("net: hns3: Fixes the back pressure setting when sriov is enabled")
Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
---
drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_tm.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_tm.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_tm.c
index aa5cb98..0c9c6ae 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_tm.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_tm.c
@@ -1168,11 +1168,12 @@ static int hclge_pfc_setup_hw(struct hclge_dev *hdev)
*/
static int hclge_bp_setup_hw(struct hclge_dev *hdev, u8 tc)
{
- struct hclge_vport *vport = hdev->vport;
u32 i, k, qs_bitmap;
int ret;
for (i = 0; i < HCLGE_BP_GRP_NUM; i++) {
+ struct hclge_vport *vport = hdev->vport;
+
qs_bitmap = 0;
for (k = 0; k < hdev->num_alloc_vport; k++) {
--
2.8.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