lkml.org 
[lkml]   [2021]   [Dec]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v2 17/23] counter: intel-qep: Convert to new counter registration
From
On 12/28/21 12:56, Uwe Kleine-König wrote:
> On Mon, Dec 27, 2021 at 05:02:39PM +0200, Jarkko Nikula wrote:
>> On 12/27/21 11:45, Uwe Kleine-König wrote:
>>> This fixes device lifetime issues where it was possible to free a live
>>> struct device.
>>>
>>> Fixes: b711f687a1c1 ("counter: Add support for Intel Quadrature Encoder Peripheral")
>>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>>> ---
>>> drivers/counter/intel-qep.c | 28 ++++++++++++++++------------
>>> 1 file changed, 16 insertions(+), 12 deletions(-)
>>>
>> Should the Fixes tag rather be b6c50affda59 ("counter: Add character device
>> interface") instead of when each drivers were introduced? I mean was it
>> possible to hit the issue before /dev/counter was introduced?
>
> I'm not sure if there is an issue before this, but it was already wrong
> before for sure. Maybe it's possible to hold a reference somehow via
> sysfs?
>
> The thought that made me tag the individual driver commits was: With the
> approach used to fix the issue all drivers need to be modified and an
> unconverted driver doesn't benefit from the availability of
> counter_alloc() / counter_add() if it isn't used. So all trees that
> include b711f687a1c1 but not "counter: intel-qep: Convert to new counter
> registration" are broken (more or less exploitable). So I think the
> added Fixes line is the right choice.
>
Fair enough. Noticed only now after this patch (with patches 1, 4, 8 and
13 applied) second counter instance initialization fails:

[ 8.113999] sysfs: cannot create duplicate filename '/dev/char/244:0
[ 8.139604] Call Trace:
[ 8.139606] <TASK>
[ 8.139608] show_stack+0x3d/0x3f
[ 8.139615] dump_stack_lvl+0x5b/0x82
[ 8.139619] dump_stack+0x10/0x12
[ 8.139621] sysfs_warn_dup.cold+0x17/0x27
[ 8.139624] sysfs_do_create_link_sd+0xc2/0xd0
[ 8.139629] sysfs_create_link+0x1c/0x30
[ 8.139631] device_add+0x54f/0x7c0
[ 8.139635] ? cdev_default_release+0x20/0x20
[ 8.139638] cdev_device_add+0x47/0x90
[ 8.139642] devm_counter_add+0x61/0xe0 [counter]
[ 8.139647] intel_qep_probe+0x16f/0x1b0
...
[ 8.325081] intel-qep 0000:00:18.6: error -EEXIST: Failed to add counter
[ 8.371793] intel-qep: probe of 0000:00:18.6 failed with error -17

Positive result is that following test doesn't produce an oops after a
few iterations like before this patch.

while :; do { sleep 5; echo bang; } > /dev/counter0 & sleep 1; echo
0000:00:18.4 >/sys/bus/pci/drivers/intel-qep/unbind; sleep 8; echo
0000:00:18.4 >/sys/bus/pci/drivers/intel-qep/bind; done

\
 
 \ /
  Last update: 2021-12-29 13:43    [W:0.193 / U:0.524 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site