| Message ID | 20220614182544.690630-1-athierry@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | staging: vchiq_arm: check vchiq_instance for NULL before dereferencing | expand |
On Tue, Jun 14, 2022 at 02:25:44PM -0400, Adrien Thierry wrote: > In service_callback(), the vchiq_instance is checked for NULL after > dereferencing it. Switch the order of those operations. > > This was reported by https://lore.kernel.org/all/Yqc+Oavmh0zMRVPQ@kili/ > > I moved the NULL check before the call to rcu_read_lock() since access > to the vchiq_instance is not RCU-protected (the RCU is only used to > access the vchiq_service). > > Signed-off-by: Adrien Thierry <athierry@redhat.com> > --- > .../staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c > index 3bcb893d14a1..ba1f86799b7f 100644 > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c > @@ -1058,6 +1058,9 @@ service_callback(struct vchiq_instance *instance, enum vchiq_reason reason, > > DEBUG_TRACE(SERVICE_CALLBACK_LINE); > > + if (!instance || instance->closing) > + return VCHIQ_SUCCESS; > + How can instance ever be NULL? And closing needs to be checked under a lock, right? thanks, greg k-h
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c index 3bcb893d14a1..ba1f86799b7f 100644 --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c @@ -1058,6 +1058,9 @@ service_callback(struct vchiq_instance *instance, enum vchiq_reason reason, DEBUG_TRACE(SERVICE_CALLBACK_LINE); + if (!instance || instance->closing) + return VCHIQ_SUCCESS; + rcu_read_lock(); service = handle_to_service(instance, handle); if (WARN_ON(!service)) { @@ -1067,11 +1070,6 @@ service_callback(struct vchiq_instance *instance, enum vchiq_reason reason, user_service = (struct user_service *)service->base.userdata; - if (!instance || instance->closing) { - rcu_read_unlock(); - return VCHIQ_SUCCESS; - } - /* * As hopping around different synchronization mechanism, * taking an extra reference results in simpler implementation.
In service_callback(), the vchiq_instance is checked for NULL after dereferencing it. Switch the order of those operations. This was reported by https://lore.kernel.org/all/Yqc+Oavmh0zMRVPQ@kili/ I moved the NULL check before the call to rcu_read_lock() since access to the vchiq_instance is not RCU-protected (the RCU is only used to access the vchiq_service). Signed-off-by: Adrien Thierry <athierry@redhat.com> --- .../staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) base-commit: de9257ae1d3b0d8856955045d194e3ff4f278394