ff-core effect id handling in case of a failed effect upload

ff-core effect id handling in case of a failed effect upload

[Elias Vanderstuyft]

Hi,


In the process of reviewing the Wine DInput translation layer, I
noticed an inconvenience (in the ff-core implementation?) that can
possibly lead to confusing problems to application developers (not
only for Wine), in short:
If a new (id==-1) effect was uploaded (look at
ff-core.c::input_ff_upload(...)) that failed (e.g. returning EINVAL),
ff-core will have assigned a positive number to the effect id. This
can be confusing because the dev->ff->effects[] array will not contain
an element at the index of that new effect id.

Here is a more elaborated description/discussion:
- This is a bug that is either the responsibility of the Linux kernel,
or of Wine (and possibly other applications that do the same thing as
described below):
    It is caused by the following situation:
        When uploading an effect, the specific kernel device driver
may return an error,
        e.g. EINVAL for example when a periodic's effect period is set to zero.
        This error will then be returned by "ioctl(*(This->fd),
EVIOCSFF, &This->effect)".
        With or without error, one can find out that
/drivers/input/ff-core.c:input_ff_upload(...) is called,
        which will set effect->id >= 0, if it was set to -1 (=> new
effect created) by the user.

        But in case of an error:
        - Assume effect->id was set to -1 by the user:
            The error is reported by ff->upload(...) at
/drivers/input/ff-core.c:167,
            the effect->id will also be set >= 0 (*).
            The offending effect will not be saved in the
ff->effects[] array (***).
        - Assume effect->id was set >= 0 by the user (and
ff->effects[effect->id] is a valid existing effect):
            The error is reported by ff->upload(...) at
/drivers/input/ff-core.c:167,
            the effect->id will remain unchanged (**).
            The offending effect will not overwrite the
ff->effects[effect->id] element (****).

        Is this (see *, **, *** and ****) desired behaviour?
        - If yes:
            Change the following in Wine's dinput/effect_linuxinput.c:90 :
                if (ioctl(*(This->fd), EVIOCSFF, &This->effect) == -1) {
            to :
                int effectId_old = This->effect.id;
                if (ioctl(*(This->fd), EVIOCSFF, &This->effect) == -1) {
                    This->effect.id = effectId_old;
        - If no for *:
            Kernel code /drivers/input/ff-core.c:input_ff_upload(...)
has to be patched to revert "effect->id" back to its original value
set by the user,
            which is only needed when the initial (by user) value of
"effect->id" was equal to -1.
        - If no for **** (or maybe also ***):
            ff->effects[effect->id] could be replaced by an 'empty'
effect (however this can get complex because the effect's type has to
remain unchanged)
            This would be a change in the kernel code
/drivers/input/ff-core.c:input_ff_upload(...).
        - If no for **:
            I don't really know. Discussion is needed.

        - In my opinion, **, *** and **** are desired behaviour, while
* should leave the effect->id at -1.
            In that case, Wine's dinput implementation does not have
to be patched, and the kernel code only should apply a minor patch.


Best regards,

Elias

Re: ff-core effect id handling in case of a failed effect upload

[Anssi Hannula]

(added Dmitry to CC)

19.02.2014 13:42, Elias Vanderstuyft kirjoitti:
> Hi,
> 

Hi,

> In the process of reviewing the Wine DInput translation layer, I
> noticed an inconvenience (in the ff-core implementation?) that can
> possibly lead to confusing problems to application developers (not
> only for Wine), in short:
> If a new (id==-1) effect was uploaded (look at
> ff-core.c::input_ff_upload(...)) that failed (e.g. returning EINVAL),
> ff-core will have assigned a positive number to the effect id. This
> can be confusing because the dev->ff->effects[] array will not contain
> an element at the index of that new effect id.

I agree that this is a bit confusing, and the kernel code should
probably be modified to not clobber the ioctl-provided effect on failure
(effect->id is set to an "undefined" value, i.e. next free effect slot).

Dmitry, WDYT?

The downside is that if changed, any new userspace code may unknowingly
depend on the fixed non-clobbering behavior (though apparently they
already do, as evidenced by Wine DInput, so might not be much of an
argument...).

> Here is a more elaborated description/discussion:
> - This is a bug that is either the responsibility of the Linux kernel,
> or of Wine (and possibly other applications that do the same thing as
> described below):
>     It is caused by the following situation:
>         When uploading an effect, the specific kernel device driver
> may return an error,
>         e.g. EINVAL for example when a periodic's effect period is set to zero.
>         This error will then be returned by "ioctl(*(This->fd),
> EVIOCSFF, &This->effect)".
>         With or without error, one can find out that
> /drivers/input/ff-core.c:input_ff_upload(...) is called,
>         which will set effect->id >= 0, if it was set to -1 (=> new
> effect created) by the user.
> 
>         But in case of an error:
>         - Assume effect->id was set to -1 by the user:
>             The error is reported by ff->upload(...) at
> /drivers/input/ff-core.c:167,
>             the effect->id will also be set >= 0 (*).
>             The offending effect will not be saved in the
> ff->effects[] array (***).
>         - Assume effect->id was set >= 0 by the user (and
> ff->effects[effect->id] is a valid existing effect):
>             The error is reported by ff->upload(...) at
> /drivers/input/ff-core.c:167,
>             the effect->id will remain unchanged (**).
>             The offending effect will not overwrite the
> ff->effects[effect->id] element (****).
> 
>         Is this (see *, **, *** and ****) desired behaviour?
>         - If yes:
>             Change the following in Wine's dinput/effect_linuxinput.c:90 :
>                 if (ioctl(*(This->fd), EVIOCSFF, &This->effect) == -1) {
>             to :
>                 int effectId_old = This->effect.id;
>                 if (ioctl(*(This->fd), EVIOCSFF, &This->effect) == -1) {
>                     This->effect.id = effectId_old;
>         - If no for *:
>             Kernel code /drivers/input/ff-core.c:input_ff_upload(...)
> has to be patched to revert "effect->id" back to its original value
> set by the user,
>             which is only needed when the initial (by user) value of
> "effect->id" was equal to -1.
>         - If no for **** (or maybe also ***):
>             ff->effects[effect->id] could be replaced by an 'empty'
> effect (however this can get complex because the effect's type has to
> remain unchanged)
>             This would be a change in the kernel code
> /drivers/input/ff-core.c:input_ff_upload(...).
>         - If no for **:
>             I don't really know. Discussion is needed.
> 
>         - In my opinion, **, *** and **** are desired behaviour, while
> * should leave the effect->id at -1.

Right.

>             In that case, Wine's dinput implementation does not have
> to be patched, and the kernel code only should apply a minor patch.

Well, maybe better to change dinput anyway so that it can work properly
with current kernel versions (assuming this gets changed in the future)?
Not my call, though.

-- 
Anssi Hannula

Re: ff-core effect id handling in case of a failed effect upload

[Elias Vanderstuyft]

On Wed, Feb 19, 2014 at 5:49 PM, Anssi Hannula <anssi.hannula@iki.fi> wrote:
> (added Dmitry to CC)
>
> 19.02.2014 13:42, Elias Vanderstuyft kirjoitti:
>> Hi,
>>
>
> Hi,
>
>> In the process of reviewing the Wine DInput translation layer, I
>> noticed an inconvenience (in the ff-core implementation?) that can
>> possibly lead to confusing problems to application developers (not
>> only for Wine), in short:
>> If a new (id==-1) effect was uploaded (look at
>> ff-core.c::input_ff_upload(...)) that failed (e.g. returning EINVAL),
>> ff-core will have assigned a positive number to the effect id. This
>> can be confusing because the dev->ff->effects[] array will not contain
>> an element at the index of that new effect id.
>
> I agree that this is a bit confusing, and the kernel code should
> probably be modified to not clobber the ioctl-provided effect on failure
> (effect->id is set to an "undefined" value, i.e. next free effect slot).
>
> Dmitry, WDYT?
>
> The downside is that if changed, any new userspace code may unknowingly
> depend on the fixed non-clobbering behavior (though apparently they
> already do, as evidenced by Wine DInput, so might not be much of an
> argument...).

I don't think that's a problem.
Something that can be more problematic, is that a newer application
assumes the fixed non-clobbering behaviour, but runs on a kernel with
the old clobbering behaviour, could experience problems.

>
>> Here is a more elaborated description/discussion:
>> - This is a bug that is either the responsibility of the Linux kernel,
>> or of Wine (and possibly other applications that do the same thing as
>> described below):
>>     It is caused by the following situation:
>>         When uploading an effect, the specific kernel device driver
>> may return an error,
>>         e.g. EINVAL for example when a periodic's effect period is set to zero.
>>         This error will then be returned by "ioctl(*(This->fd),
>> EVIOCSFF, &This->effect)".
>>         With or without error, one can find out that
>> /drivers/input/ff-core.c:input_ff_upload(...) is called,
>>         which will set effect->id >= 0, if it was set to -1 (=> new
>> effect created) by the user.
>>
>>         But in case of an error:
>>         - Assume effect->id was set to -1 by the user:
>>             The error is reported by ff->upload(...) at
>> /drivers/input/ff-core.c:167,
>>             the effect->id will also be set >= 0 (*).
>>             The offending effect will not be saved in the
>> ff->effects[] array (***).
>>         - Assume effect->id was set >= 0 by the user (and
>> ff->effects[effect->id] is a valid existing effect):
>>             The error is reported by ff->upload(...) at
>> /drivers/input/ff-core.c:167,
>>             the effect->id will remain unchanged (**).
>>             The offending effect will not overwrite the
>> ff->effects[effect->id] element (****).
>>
>>         Is this (see *, **, *** and ****) desired behaviour?
>>         - If yes:
>>             Change the following in Wine's dinput/effect_linuxinput.c:90 :
>>                 if (ioctl(*(This->fd), EVIOCSFF, &This->effect) == -1) {
>>             to :
>>                 int effectId_old = This->effect.id;
>>                 if (ioctl(*(This->fd), EVIOCSFF, &This->effect) == -1) {
>>                     This->effect.id = effectId_old;
>>         - If no for *:
>>             Kernel code /drivers/input/ff-core.c:input_ff_upload(...)
>> has to be patched to revert "effect->id" back to its original value
>> set by the user,
>>             which is only needed when the initial (by user) value of
>> "effect->id" was equal to -1.
>>         - If no for **** (or maybe also ***):
>>             ff->effects[effect->id] could be replaced by an 'empty'
>> effect (however this can get complex because the effect's type has to
>> remain unchanged)
>>             This would be a change in the kernel code
>> /drivers/input/ff-core.c:input_ff_upload(...).
>>         - If no for **:
>>             I don't really know. Discussion is needed.
>>
>>         - In my opinion, **, *** and **** are desired behaviour, while
>> * should leave the effect->id at -1.
>
> Right.
>
>>             In that case, Wine's dinput implementation does not have
>> to be patched, and the kernel code only should apply a minor patch.
>
> Well, maybe better to change dinput anyway so that it can work properly
> with current kernel versions (assuming this gets changed in the future)?
> Not my call, though.

Yes, agreed.
I'll include the code (with effectId_old variable) and this discussion
when I submit my whole patchset for Wine's DInput libs to Wine-devel
mailing list. Right now, I don't know who I should personally mail to.

>
> --
> Anssi Hannula


Elias

Re: ff-core effect id handling in case of a failed effect upload

[Andrew Eikum]

On Wed, Feb 19, 2014 at 06:32:54PM +0100, Elias Vanderstuyft wrote:
> I'll include the code (with effectId_old variable) and this discussion
> when I submit my whole patchset for Wine's DInput libs to Wine-devel
> mailing list. Right now, I don't know who I should personally mail to.
> 

Patches ready to be included in Wine should go to
wine-patches@winehq.org.  I've done a bit of work in dinput myself, so
you can feel free to send them to me and/or wine-devel first if you'd
like a review.

Andrew

Re: ff-core effect id handling in case of a failed effect upload

[Elias Vanderstuyft]

On Wed, Feb 19, 2014 at 8:32 PM, Andrew Eikum <aeikum@codeweavers.com> wrote:
> On Wed, Feb 19, 2014 at 06:32:54PM +0100, Elias Vanderstuyft wrote:
>> I'll include the code (with effectId_old variable) and this discussion
>> when I submit my whole patchset for Wine's DInput libs to Wine-devel
>> mailing list. Right now, I don't know who I should personally mail to.
>>
>
> Patches ready to be included in Wine should go to
> wine-patches@winehq.org.  I've done a bit of work in dinput myself, so
> you can feel free to send them to me and/or wine-devel first if you'd
> like a review.

<offtopic>
Alright, thanks Andrew!
I've also done some work on reverse engineering the FFE file format,
which is needed to complete some missing functionality
(http://wiki.winehq.org/ForceFeedbackSummerOfCode2005Summary) of the
following functions:
IDirectInputDevice8::WriteEffectToFile and
IDirectInputDevice8::EnumEffectsInFile.
Can I send it to you (in a new mail subject, of course) and cc wine-devel?
</offtopic>

>
> Andrew

Elias

Re: ff-core effect id handling in case of a failed effect upload

[Dmitry Torokhov]

On Wed, Feb 19, 2014 at 06:49:36PM +0200, Anssi Hannula wrote:
> (added Dmitry to CC)
> 
> 19.02.2014 13:42, Elias Vanderstuyft kirjoitti:
> > Hi,
> > 
> 
> Hi,
> 
> > In the process of reviewing the Wine DInput translation layer, I
> > noticed an inconvenience (in the ff-core implementation?) that can
> > possibly lead to confusing problems to application developers (not
> > only for Wine), in short:
> > If a new (id==-1) effect was uploaded (look at
> > ff-core.c::input_ff_upload(...)) that failed (e.g. returning EINVAL),
> > ff-core will have assigned a positive number to the effect id. This
> > can be confusing because the dev->ff->effects[] array will not contain
> > an element at the index of that new effect id.
> 
> I agree that this is a bit confusing, and the kernel code should
> probably be modified to not clobber the ioctl-provided effect on failure
> (effect->id is set to an "undefined" value, i.e. next free effect slot).
> 
> Dmitry, WDYT?

Yeah, it looks like we need to change evdev.c to read:

		error = input_ff_upload(dev, &effect, file);
		if (error)
			return error;

		if (put_user(effect.id, &(((struct ff_effect __user *)p)->id)))
			return -EFAULT;

		return 0;

Unfortunately applications should still expect changed effect ID for
quite some time.

Thanks.

-- 
Dmitry

Re: ff-core effect id handling in case of a failed effect upload

[Elias Vanderstuyft]

On Wed, Feb 19, 2014 at 10:05 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Wed, Feb 19, 2014 at 06:49:36PM +0200, Anssi Hannula wrote:
>> (added Dmitry to CC)
>>
>> 19.02.2014 13:42, Elias Vanderstuyft kirjoitti:
>> > Hi,
>> >
>>
>> Hi,
>>
>> > In the process of reviewing the Wine DInput translation layer, I
>> > noticed an inconvenience (in the ff-core implementation?) that can
>> > possibly lead to confusing problems to application developers (not
>> > only for Wine), in short:
>> > If a new (id==-1) effect was uploaded (look at
>> > ff-core.c::input_ff_upload(...)) that failed (e.g. returning EINVAL),
>> > ff-core will have assigned a positive number to the effect id. This
>> > can be confusing because the dev->ff->effects[] array will not contain
>> > an element at the index of that new effect id.
>>
>> I agree that this is a bit confusing, and the kernel code should
>> probably be modified to not clobber the ioctl-provided effect on failure
>> (effect->id is set to an "undefined" value, i.e. next free effect slot).
>>
>> Dmitry, WDYT?
>
> Yeah, it looks like we need to change evdev.c to read:
>
>                 error = input_ff_upload(dev, &effect, file);
>                 if (error)
>                         return error;
>
>                 if (put_user(effect.id, &(((struct ff_effect __user *)p)->id)))
>                         return -EFAULT;
>
>                 return 0;

Alright, who will create the patch?
Do I may / have to do it?

>
> Unfortunately applications should still expect changed effect ID for
> quite some time.
>
> Thanks.
>
> --
> Dmitry

Best regards,
Elias

Re: ff-core effect id handling in case of a failed effect upload

[Dmitry Torokhov]

On Wed, Feb 19, 2014 at 11:14:18PM +0100, Elias Vanderstuyft wrote:
> On Wed, Feb 19, 2014 at 10:05 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > On Wed, Feb 19, 2014 at 06:49:36PM +0200, Anssi Hannula wrote:
> >> (added Dmitry to CC)
> >>
> >> 19.02.2014 13:42, Elias Vanderstuyft kirjoitti:
> >> > Hi,
> >> >
> >>
> >> Hi,
> >>
> >> > In the process of reviewing the Wine DInput translation layer, I
> >> > noticed an inconvenience (in the ff-core implementation?) that can
> >> > possibly lead to confusing problems to application developers (not
> >> > only for Wine), in short:
> >> > If a new (id==-1) effect was uploaded (look at
> >> > ff-core.c::input_ff_upload(...)) that failed (e.g. returning EINVAL),
> >> > ff-core will have assigned a positive number to the effect id. This
> >> > can be confusing because the dev->ff->effects[] array will not contain
> >> > an element at the index of that new effect id.
> >>
> >> I agree that this is a bit confusing, and the kernel code should
> >> probably be modified to not clobber the ioctl-provided effect on failure
> >> (effect->id is set to an "undefined" value, i.e. next free effect slot).
> >>
> >> Dmitry, WDYT?
> >
> > Yeah, it looks like we need to change evdev.c to read:
> >
> >                 error = input_ff_upload(dev, &effect, file);
> >                 if (error)
> >                         return error;
> >
> >                 if (put_user(effect.id, &(((struct ff_effect __user *)p)->id)))
> >                         return -EFAULT;
> >
> >                 return 0;
> 
> Alright, who will create the patch?
> Do I may / have to do it?

If you could create the patch that would be appreciated.

Thanks.

-- 
Dmitry

Re: ff-core effect id handling in case of a failed effect upload

[Elias Vanderstuyft]

I sent a patch, see:
http://www.mail-archive.com/linux-input@vger.kernel.org/msg08572.html
("[PATCH 1/1] Input: don't modify the id of ioctl-provided ff effect
on upload failure")

Elias

On Sun, Feb 23, 2014 at 7:30 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Wed, Feb 19, 2014 at 11:14:18PM +0100, Elias Vanderstuyft wrote:
>> On Wed, Feb 19, 2014 at 10:05 PM, Dmitry Torokhov
>> <dmitry.torokhov@gmail.com> wrote:
>> > On Wed, Feb 19, 2014 at 06:49:36PM +0200, Anssi Hannula wrote:
>> >> (added Dmitry to CC)
>> >>
>> >> 19.02.2014 13:42, Elias Vanderstuyft kirjoitti:
>> >> > Hi,
>> >> >
>> >>
>> >> Hi,
>> >>
>> >> > In the process of reviewing the Wine DInput translation layer, I
>> >> > noticed an inconvenience (in the ff-core implementation?) that can
>> >> > possibly lead to confusing problems to application developers (not
>> >> > only for Wine), in short:
>> >> > If a new (id==-1) effect was uploaded (look at
>> >> > ff-core.c::input_ff_upload(...)) that failed (e.g. returning EINVAL),
>> >> > ff-core will have assigned a positive number to the effect id. This
>> >> > can be confusing because the dev->ff->effects[] array will not contain
>> >> > an element at the index of that new effect id.
>> >>
>> >> I agree that this is a bit confusing, and the kernel code should
>> >> probably be modified to not clobber the ioctl-provided effect on failure
>> >> (effect->id is set to an "undefined" value, i.e. next free effect slot).
>> >>
>> >> Dmitry, WDYT?
>> >
>> > Yeah, it looks like we need to change evdev.c to read:
>> >
>> >                 error = input_ff_upload(dev, &effect, file);
>> >                 if (error)
>> >                         return error;
>> >
>> >                 if (put_user(effect.id, &(((struct ff_effect __user *)p)->id)))
>> >                         return -EFAULT;
>> >
>> >                 return 0;
>>
>> Alright, who will create the patch?
>> Do I may / have to do it?
>
> If you could create the patch that would be appreciated.
>
> Thanks.
>
> --
> Dmitry