Re: [PATCH] spi: offload: check ops and match pointers before use

[David Lechner <dlechner@baylibre.com>]

On 4/17/25 6:23 PM, Andres Urian Florez wrote:
> Before checking if one of the triggers matches, check if 'ops' and 'match'
> exist

Can you please explain in more detail why this change is needed? For example,
show the code path where we could actually have null pointer de-reference here
that would be fixed by this change.

> 
> Signed-off-by: Andres Urian Florez <andres.emb.sys@gmail.com>
> ---
>  drivers/spi/spi-offload.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi-offload.c b/drivers/spi/spi-offload.c
> index 6bad042fe437..fcb226887488 100644
> --- a/drivers/spi/spi-offload.c
> +++ b/drivers/spi/spi-offload.c
> @@ -173,7 +173,9 @@ static struct spi_offload_trigger
>  		if (trigger->fwnode != args->fwnode)
>  			continue;
>  
> -		match = trigger->ops->match(trigger, type, args->args, args->nargs);
> +		if (trigger->ops && trigger->ops->match)

The check for trigger->ops != NULL here should not be necessary. The only place
where trigger->ops = NULL is when the trigger is removed from the list and that
operation is done with the spi_offload_triggers_lock held. The same lock is also
currently held here, so it should not be possible for ops to be set to NULL here.

In fact, there is this later in the same function:

	if (!trigger->ops)
		return ERR_PTR(-ENODEV);

that could be removed (since we have shown that the condition can never be true).


> +			match = trigger->ops->match(trigger, type, args->args, args->nargs);
> +
>  		if (match)
>  			break;
>  	}

If trigger->ops->match == NULL then the trigger could never be used because it
would never be matched. So instead, I think it would be better to check for
that in devm_spi_offload_trigger_register() and fail registration if it is
missing. In other words, make match a required callback.