trigger->ops structure optional?

trigger->ops structure optional?

[Gwendal Grignou]

When I decided to compile a driver with a trigger as a module, I
realized that I needed to add a trigger ops structure with the .owner
field set to THIS_MODULE.
The reason is iio_trigger_put() calls module_put(trig->ops->owner),
which is a noop if the driver is compiled within the kernel.
Otherwise, trigger->ops is considered optional.

We could add a test there to not call module_put() if ops is NULL, but
we wouldn't have the module refcount protection anymore; the trigger
could be removed while still in use given iio_trigger_unregister()
does not check if the trigger is still in use.

To address that issue, should we make trigger->ops mandatory - its
function members could still be optional?
If we go this way, shall we go even further and fold the ops structure
within iio_trigger?

Gwendal.

Re: trigger->ops structure optional?

[Jonathan Cameron]

On 03/05/16 17:22, Gwendal Grignou wrote:
> When I decided to compile a driver with a trigger as a module, I
> realized that I needed to add a trigger ops structure with the .owner
> field set to THIS_MODULE.
> The reason is iio_trigger_put() calls module_put(trig->ops->owner),
> which is a noop if the driver is compiled within the kernel.
> Otherwise, trigger->ops is considered optional.
It's only a partial protection as module removal can still be forced,
but it does stop 'accidental' removals when in use.

Indeed, the ops structure in general is not optional though as you
say all the function pointers are.  It is rather unusual not
to have a set_state callback though as most sources of triggers can
be disabled at source (exception perhaps being interrupt sources
from external sources...)
> 
> We could add a test there to not call module_put() if ops is NULL, but
> we wouldn't have the module refcount protection anymore; the trigger
> could be removed while still in use given iio_trigger_unregister()
> does not check if the trigger is still in use.
> 
> To address that issue, should we make trigger->ops mandatory - its
> function members could still be optional?
> If we go this way, shall we go even further and fold the ops structure
> within iio_trigger?
The purpose for the separation is to keep the function pointers in a
constant structure rather than alongside elements that are modified.

Standard practice from a simple security point of view if nothing else.

So fundamentally trigger_ops is not optional and we aren't going to
merge it into struct iio_trigger.

Jonathan

> 
> Gwendal.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>