| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <fa974bd9-96bc-3647-a5b7-5d268e459753@foss.st.com>
Date: Thu, 18 Mar 2021 10:21:11 +0100
From: Fabrice Gasnier <fabrice.gasnier@...s.st.com>
To: William Breathitt Gray <vilhelm.gray@...il.com>
CC: <jic23@...nel.org>, <kamel.bouhara@...tlin.com>,
<gwendal@...omium.org>, <david@...hnology.com>,
<linux-iio@...r.kernel.org>, <patrick.havelange@...ensium.com>,
<alexandre.belloni@...tlin.com>, <mcoquelin.stm32@...il.com>,
<linux-kernel@...r.kernel.org>, <o.rempel@...gutronix.de>,
Dan Carpenter <dan.carpenter@...cle.com>,
<kernel@...gutronix.de>, <fabrice.gasnier@...com>,
<syednwaris@...il.com>, <linux-stm32@...md-mailman.stormreply.com>,
<linux-arm-kernel@...ts.infradead.org>, <alexandre.torgue@...com>
Subject: Re: [Linux-stm32] [PATCH v9 22/33] counter: Internalize sysfs
interface code
On 3/14/21 10:08 AM, William Breathitt Gray wrote:
> On Sun, Mar 14, 2021 at 04:56:44PM +0900, William Breathitt Gray wrote:
>> On Fri, Mar 12, 2021 at 04:02:42PM +0100, Fabrice Gasnier wrote:
>>> On 3/9/21 2:19 PM, William Breathitt Gray wrote:
>>>> +static ssize_t enums_available_show(const u32 *const enums,
>>>> + const size_t num_enums,
>>>> + const char *const strs[], char *buf)
>>>> +{
>>>> + size_t len = 0;
>>>> + size_t index;
>>>> +
>>>> + for (index = 0; index < num_enums; index++)
>>>> + len += sysfs_emit(buf + len, "%s\n", strs[enums[index]]);
>>>> +
>>>> + return len;
>>>> +}
>>>> +
>>>> +static ssize_t strs_available_show(const struct counter_available *const avail,
>>>> + char *buf)
>>>> +{
>>>> + size_t len = 0;
>>>> + size_t index;
>>>> +
>>>> + for (index = 0; index < avail->num_items; index++)
>>>> + len += sysfs_emit(buf + len, "%s\n", avail->strs[index]);
>>>> +
>>>> + return len;
>>>> +}
>>>
>>> Hi William,
>>>
>>> I was willing to do some testing on this series, on the stm32 counter
>>> drivers, since we released few fixes around them.
>>>
>>> I tried to apply this series against current testing branch, with few
>>> patches applied (so it applies cleanly):
>>> - dt-bindings: counter: add interrupt-counter binding
>>> - counter: add IRQ or GPIO based counter
>>> - counter: stm32-timer-cnt: fix ceiling miss-alignment with reload register
>>> - counter: stm32-timer-cnt: fix ceiling write max value
>>> counter: stm32-timer-cnt: Report count function when SLAVE_MODE_DISABLED
>>>
>>>
>>> For both the "stm32-lptimer-cnt" and "stm32-timer-cnt" drivers, I get a
>>> warning message and stack dump in "sysfs_emit" when reading the
>>> available functions from sysfs.
>>> I started to do some testing on v8 of this series last week. I didn't
>>> noticed that.
>>>
>>> For both the "stm32-lptimer-cnt", there are 2 functions currently I get
>>> 1 stack dump. Only the "increase" function is printed correctly.
>>>
>>> For the "stm32-timer-cnt", there are 4 functions currently, I get 3
>>> stack dumps. Only the "increase" function is printed correctly
>>>
>>> Sample log for "stm32-timer-cnt:
>>>
>>> root@...32mp1:/sys/devices/platform/soc/44000000.timer/44000000.timer:counter/counter0#
>>> cat count0/function_available
>>> [ 4689.195506] ------------[ cut here ]------------
>>> [ 4689.198747] WARNING: CPU: 1 PID: 5841 at fs/sysfs/file.c:737
>>> sysfs_emit+0x88/0x94
>>> [ 4689.206233] invalid sysfs_emit: buf:f4a66208
>>> [ 4689.210553] Modules linked in: sha256_generic libsha256 sha256_arm
>>> cfg80211 panel_orisetech_otm8009a snd_soc_hdmi_codec
>>> snd_soc_stm32_sai_sub stm32_lptimers
>>> [ 4689.261444] CPU: 1 PID: 5841 Comm: cat Tainted: G W
>>> 5.12.0-rc1 #534
>>> [ 4689.268999] Hardware name: STM32 (Device Tree Support)
>>> [ 4689.274166] [<c0310b38>] (unwind_backtrace) from [<c030b4ec>]
>>> (show_stack+0x10/0x14)
>>> [ 4689.281942] [<c030b4ec>] (show_stack) from [<c0fede70>]
>>> (dump_stack+0xc0/0xd4)
>>> [ 4689.289199] [<c0fede70>] (dump_stack) from [<c0345624>]
>>> (__warn+0xec/0x148)
>>> [ 4689.296194] [<c0345624>] (__warn) from [<c0fe9e90>]
>>> (warn_slowpath_fmt+0x98/0xbc)
>>> [ 4689.303714] [<c0fe9e90>] (warn_slowpath_fmt) from [<c0548ee0>]
>>> (sysfs_emit+0x88/0x94)
>>> [ 4689.311586] [<c0548ee0>] (sysfs_emit) from [<bf115de8>]
>>> (counter_comp_available_show+0x11c/0x1a4 [counter])
>>> [ 4689.321382] [<bf115de8>] (counter_comp_available_show [counter]) from
>>> [<c0a21b70>] (dev_attr_show+0x18/0x48)
>>> [ 4689.331263] [<c0a21b70>] (dev_attr_show) from [<c0549014>]
>>> (sysfs_kf_seq_show+0x88/0xf0)
>>> [ 4689.339394] [<c0549014>] (sysfs_kf_seq_show) from [<c04da6e8>]
>>> (seq_read_iter+0x1a4/0x554)
>>> [ 4689.347703] [<c04da6e8>] (seq_read_iter) from [<c04af6f0>]
>>> (vfs_read+0x1ac/0x2c4)
>>> [ 4689.355224] [<c04af6f0>] (vfs_read) from [<c04afc20>]
>>> (ksys_read+0x64/0xdc)
>>> [ 4689.362219] [<c04afc20>] (ksys_read) from [<c03000c0>]
>>> (ret_fast_syscall+0x0/0x58)
>>> [ 4689.369827] Exception stack(0xc7261fa8 to 0xc7261ff0)
>>> [ 4689.374906] 1fa0: 00000000 00020000 00000003
>>> b6f35000 00020000 00000000
>>> [ 4689.383126] 1fc0: 00000000 00020000 b6f56ce0 00000003 00000003
>>> 00000000 00020000 00000000
>>> [ 4689.391344] 1fe0: 00000003 be8239a8 410bff27 4104c066
>>> ...
>>> 2 more stack dumps follow
>>> ...
>>> [ 4689.810479] ---[ end trace 59ed79949efe984c ]---
>>> increase
>>>
>>> I get similar backtrace with other _available attributes:
>>> $ cat signal0_action_available
>>> $ cat signal1_action_available
>>>
>>> Do you think I'm doing something wrong ?
>>>
>>> I tested then "quadrature x4" on the timer driver... It seems all fine.
>>>
>>> Best regards
>>> Fabrice
>>>
>>>> +
>>>> +static ssize_t counter_comp_available_show(struct device *dev,
>>>> + struct device_attribute *attr,
>>>> + char *buf)
>>>> +{
>>>> + const struct counter_attribute *const a = to_counter_attribute(attr);
>>>> + const struct counter_count *const count = a->parent;
>>>> + const struct counter_synapse *const synapse = a->comp.priv;
>>>> + const struct counter_available *const avail = a->comp.priv;
>>>> +
>>>> + switch (a->comp.type) {
>>>> + case COUNTER_COMP_FUNCTION:
>>>> + return enums_available_show(count->functions_list,
>>>> + count->num_functions,
>>>> + counter_function_str, buf);
>>>> + case COUNTER_COMP_SYNAPSE_ACTION:
>>>> + return enums_available_show(synapse->actions_list,
>>>> + synapse->num_actions,
>>>> + counter_synapse_action_str, buf);
>>>> + case COUNTER_COMP_ENUM:
>>>> + return strs_available_show(avail, buf);
>>>> + case COUNTER_COMP_COUNT_MODE:
>>>> + return enums_available_show(avail->enums, avail->num_items,
>>>> + counter_count_mode_str, buf);
>>>> + default:
>>>> + return -EINVAL;
>>>> + }
>>>> +}
>>
>> Hi Fabrice,
>>
>> I can confirm that I'm hitting this regression as well with the
>> 104-quad-8 driver. The warning seems to be caused by the
>> offset_in_page(buf) check in sysfs_emit(). It looks like the first loop
>> in enums_available_show() calls sysfs_emit() correctly, but subsequent
>> loops have an invalid buf offset.
>>
>> The enums_available_show() callback is rather simple: call sysfs_emit()
>> for each enum string and increment buf by the length written each time.
>> I haven't modified this function since v8, so I am somewhat confused
>> about why the buf offset would be invalid here now. I wonder if there
>> has been a change somewhere else in the kernel that is causing
>> sysfs_emit() to now return an incorrect length.
>>
>> William Breathitt Gray
>
> Fabrice,
>
> Would you be able to check the values of buf and len before they enter
> sysfs_emit()? I think redefining the enums_available_show() function
> like this should suffice:
>
> static ssize_t enums_available_show(const u32 *const enums,
> const size_t num_enums,
> const char *const strs[], char *buf)
> {
> size_t len = 0;
> size_t index;
>
> for (index = 0; index < num_enums; index++){
> pr_info("buf: %p\tbuf+len: %p\tlen: %zu\n", buf, buf + len, len);
> len += sysfs_emit(buf + len, "%s\n", strs[enums[index]]);
> }
>
> return len;
> }
>
> I want to see whether the issue is due to the sysfs_emit() return value
> or the value of buf.
Hi William,
Sorry for the delay,
I'm getting strange results on buf+len. Here's the result I'm getting
with same test as above:
[ 170.190995] buf: 5daf3333 buf+len: 5daf3333 len: 0
[ 170.194383] buf: 5daf3333 buf+len: 22c37039 len: 9
[ 170.199268] ------------[ cut here ]------------
...
[ 170.404810] buf: 5daf3333 buf+len: 22c37039 len: 9
[ 170.409663] ------------[ cut here ]------------
...
[ 170.615265] buf: 5daf3333 buf+len: 22c37039 len: 9
[ 170.620117] ------------[ cut here ]------------
...
increase
Hope this helps,
Fabrice
>
> Thank you,
>
> William Breathitt Gray
>
Powered by blists - more mailing lists