[Letux-kernel] two accelerometers + iio_bridge considered harmful

[H. Nikolaus Schaller]

Hi,

> Am 31.07.2018 um 22:45 schrieb Andreas Kemnade <andreas at kemnade.info>:
> 
> On Tue, 31 Jul 2018 19:16:53 +0200
> Andreas Kemnade <andreas at kemnade.info> wrote:
> 
> 
>> [    6.419860] usb usb1: Product: MUSB HDRC host driver
>> [    6.430450] iio_device_register_inputbridge(): process channel 3
> 
> -> EVIL, EVIL, EVIL
> 
> this means access to channels[3] which is an out-of-bound access.
> Next thing in row probably
> static struct delayed_work input_work;
> 
> So how can it happen: between
> if (channel >= 3)
>                return 0;
> 
> and the final
>        switch (channel++) {

Ah, ok!

> 
> a lot ot things happen, so enough time for two threads to enter.

Especially if one of them does a input_register_device() because it finds
channel 0 and this loads udevd and a lot of things blocking the probe.

In the meantime someone else enters the same code also finds channel == 0,
and registers 3 morechannels. Then, after finishing all udev blocking, channel == 3
and the input_work is overwritten. It will not try to register a channel 4 but
it is already too late.

In that case you should always get a duplicate input_register_device().
Could you see such a thing happen?

> 
> So first question is it really worth to make it thread-safe

Well, a simple mutex would help.

But where could we initialize it before the first channel is registered?
And make sure that it is initialized only once (without another mutex)?

> if it is
> now so much more sensitive to threading issues or should we better fix
> userspace to use iio.

Depends on. As long as the kernel supports ACCEL input events and other
input drivers for accelerometers it makes sense.

> What is to fix? Qtmoko (probably easy), Replicant:
> Well it should support all the other sensors, too, so it has to use iio
> anyways.

IMHO fixing this driver should not be big effort - as soon as we understand
the problems (which is the bigger task).

User-space fixes might have less problems or problems easier to understand.

> Just a note from systemd changelog (which is the source of udev in
> stretch and jessie)
> * The udev accelerometer helper was removed. The functionality
>          is now fully included in iio-sensor-proxy. But this means,
>          older iio-sensor-proxy versions will no longer provide
>          accelerometer/orientation data with this systemd version.
>          Please upgrade iio-sensor-proxy to version 1.0.
> That is from 2015-07-07
> 
> So userspace has adopted a long time ago.

Well, there can be even older user-space code around (games) and
we still support wheezy.

> At least we should heavily test all the threading issues with multiple
> accelerometers if we decide to continue the bridge.

Yes, this is welcome!

Basically it was a quick hack after upstream decided to delete our bma180 input driver :(

BR and thanks,
Nikolaus

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://lists.goldelico.com/pipermail/letux-kernel/attachments/20180801/4e0f037d/attachment.asc>