| Message ID | 20250417232319.384094-1-andres.emb.sys@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | spi: offload: check ops and match pointers before use | expand |
On Fri, Apr 18, 2025 at 9:20 AM David Lechner <dlechner@baylibre.com> wrote: > > 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. Hi David. Thanks for your comments! I did not see the full picture and now it is clear to me that it is not required to check the trigger->ops in spi_offload_trigger_get(). Then I will create another patch to remove the trigger->ops check that you mentioned in spi_offload_trigger_get, and make match a required callback in devm_spi_offload_trigger_register() instead.
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) + match = trigger->ops->match(trigger, type, args->args, args->nargs); + if (match) break; }
Before checking if one of the triggers matches, check if 'ops' and 'match' exist Signed-off-by: Andres Urian Florez <andres.emb.sys@gmail.com> --- drivers/spi/spi-offload.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)