Re: [PATCH v2 17/23] counter: intel-qep: Convert to new counter registration

[Jarkko Nikula <jarkko.nikula@linux.intel.com>]

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