ath10k.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: Wen Gong <wgong@codeaurora.org>
To: Brian Norris <briannorris@chromium.org>
Cc: linux-wireless <linux-wireless@vger.kernel.org>,
	Steven Rostedt <rostedt@goodmis.org>,
	ath10k <ath10k@lists.infradead.org>
Subject: Re: [PATCH] ath10k: change len of trace_ath10k_log_dbg_dump for large buffer size
Date: Wed, 10 Feb 2021 10:11:50 +0800	[thread overview]
Message-ID: <19b6ad684a01718c3823f882581fca36@codeaurora.org> (raw)
In-Reply-To: <CA+ASDXN1V2HYA7C6s-q5bQNSxE7L5GCJiqfiJ_67R_hpUn4b_g@mail.gmail.com>

On 2021-02-10 03:35, Brian Norris wrote:
> + Steven Rostedt
> 
> Hi Wen,
> 
> (Trimming down the description a bit:)
> 
> On Mon, Feb 8, 2021 at 6:59 PM Wen Gong <wgong@codeaurora.org> wrote:
>> 
>> Kernel panic every time in kernel when running below case:
>> steps:
>> 1. connect to an AP with good signal strength
>> 2. echo 0x7f > /sys/kernel/debug/ieee80211/phy0/ath10k/pktlog_filter
>> 3. echo 0xffff 0 > /sys/kernel/debug/ieee80211/phy0/ath10k/fw_dbglog
>> 4. echo 0 > /sys/module/ath10k_core/parameters/debug_mask
>> 5. sudo trace-cmd record  -e ath10k
>> 6. run "iperf -c 192.168.1.1 -u -b 100m -i 1 -t 30"
>> 7. kernel panic immeditely
>> 
>> It always crash at trace_event_raw_event_ath10k_xxx, below is 2 
>> sample:
> ...
>> The value of prog in filter_match_preds of 
>> kernel/trace/trace_events_filter.c
>> is overwrite to the content of the UDP packets's content like this
>> 0x0039383736353433, it is a invalid address, so crash.
> ...
>> ath10k_htc_send_bundle_skbs allocate skb with size 49792(1556*32), it
>> is bigger than PAGE_SIZE which is normally 4096, then 
>> ath10k_sdio_write
>> call ath10k_dbg_dump to trace the large size skb and corrupt the trace
>> data of tracing and lead crash. When disable the TX bundle of SDIO, it
>> does not crash, but TX bundle is for improve throughput, so it is 
>> enabled
>> by default. It is useless to call ath10k_dbg_dump to trace the large
>> bundled skb, so this patch trace the top part of large bundled skb.
> ...
>> trace_event_raw_event_ath10k_log_dbg_dump is generated by compiler, it
>> call trace_event_buffer_reserve got get a struct pointer *entry, its
>> type is trace_event_raw_ath10k_log_dbg_dump which is also generated by
>> compiler, trace_event_buffer_reserve of kernel/trace/trace_events.c
>> call trace_event_buffer_lock_reserve to get ring_buffer_event.
>> 
>> In function trace_event_buffer_lock_reserve of kernel/trace/trace.c,
>> the ring_buffer_time_stamp_abs is false and trace_file->flags is 0x40b
>> which is set bit of EVENT_FILE_FL_FILTERED by debugging, so it use the
>> temp buffer this_cpu_read(trace_buffered_event), and the buffer size
>> is 1 page size which allocatee in trace_buffered_event_enable by
>> alloc_pages_node, and then ath10k pass the buffer size > 1 page 
>> trigger
>> overflow and crash.
>> 
>> Based on upper test, try and debugging, pass large buff size to 
>> function
>> trace_ath10k_log_dbg_dump cause crash, and it has ath10k_dbg in
>> ath10k_sdio_write to print the length of skb/buffer, it is not 
>> necessary
>> to trace all content of the large skb.
> 
> Is this the same issue noted in this thread?
Hi Norris,
Yes, but it is not the total same issue.
> 
> [for-next][PATCH 2/2] tracing: Use temp buffer when filtering events
> https://lore.kernel.org/lkml/f16b14066317f6a926b6636df6974966@codeaurora.org/
> 
> It seems like we should still try to get that fixed somehow, even if
> the below change is fine on its own (it probably doesn't make sense to
> such a large amount of data via tracepoints). It would be unfortunate
> for next poor soul to hit the same issues, just because they wanted to
> dump a few KB.
For ath10k, we do not want to dump content which size > 1024.
otherwise it will generate a much big file while collecting log, it make 
us much
hard to analyze the log.
so this patch is to dump the top 1024 bytes only,
its 1st goal is make log smaller.
its 2nd effect is fix the crash issue,
> 
> Brian
> 
>>  drivers/net/wireless/ath/ath10k/debug.c | 2 +-
>>  drivers/net/wireless/ath/ath10k/debug.h | 2 ++
>>  2 files changed, 3 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/net/wireless/ath/ath10k/debug.c 
>> b/drivers/net/wireless/ath/ath10k/debug.c
>> index e8250a665433..5433dbdde0e5 100644
>> --- a/drivers/net/wireless/ath/ath10k/debug.c
>> +++ b/drivers/net/wireless/ath/ath10k/debug.c
>> @@ -2718,7 +2718,7 @@ void ath10k_dbg_dump(struct ath10k *ar,
>> 
>>         /* tracing code doesn't like null strings :/ */
>>         trace_ath10k_log_dbg_dump(ar, msg ? msg : "", prefix ? prefix 
>> : "",
>> -                                 buf, len);
>> +                                 buf, min_t(size_t, len, 
>> ATH10K_LOG_DUMP_MAX));
>>  }
>>  EXPORT_SYMBOL(ath10k_dbg_dump);
>> 
>> diff --git a/drivers/net/wireless/ath/ath10k/debug.h 
>> b/drivers/net/wireless/ath/ath10k/debug.h
>> index 997c1c80aba7..cec9ba92f28f 100644
>> --- a/drivers/net/wireless/ath/ath10k/debug.h
>> +++ b/drivers/net/wireless/ath/ath10k/debug.h
>> @@ -11,6 +11,8 @@
>>  #include <linux/types.h>
>>  #include "trace.h"
>> 
>> +#define ATH10K_LOG_DUMP_MAX 1024
>> +
>>  enum ath10k_debug_mask {
>>         ATH10K_DBG_PCI          = 0x00000001,
>>         ATH10K_DBG_WMI          = 0x00000002,
>> --
>> 2.23.0
>> 
>> 
>> _______________________________________________
>> ath10k mailing list
>> ath10k@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/ath10k

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

  parent reply	other threads:[~2021-02-10  2:12 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-09  2:59 Wen Gong
2021-02-09  7:34 ` Kalle Valo
2021-02-09 19:35 ` Brian Norris
2021-02-09 19:55   ` Steven Rostedt
2021-02-09 21:34     ` Steven Rostedt
2021-02-10  2:01       ` Wen Gong
2021-02-10 16:30         ` Steven Rostedt
2021-02-10 16:48           ` Steven Rostedt
2021-02-10 16:53             ` Steven Rostedt
2021-03-08  7:22               ` Wen Gong
2021-02-10  2:11   ` Wen Gong [this message]
2021-02-10  2:14     ` Brian Norris
2021-02-10  2:24       ` Wen Gong

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=19b6ad684a01718c3823f882581fca36@codeaurora.org \
    --to=wgong@codeaurora.org \
    --cc=ath10k@lists.infradead.org \
    --cc=briannorris@chromium.org \
    --cc=linux-wireless@vger.kernel.org \
    --cc=rostedt@goodmis.org \
    --subject='Re: [PATCH] ath10k: change len of trace_ath10k_log_dbg_dump for large buffer size' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).