Hi Padmanabha,
Am 30.12.21 um 15:54 schrieb Padmanabha Srinivasaiah:
> In service_callback path RCU dereferenced pointer struct vchiq_service
> need to be accessed inside rcu read-critical section.
>
> Accessing same with rcu_read_[lock/unlock] fixes the issue.
>
> [ 32.201659] =============================
> [ 32.201664] WARNING: suspicious RCU usage
> [ 32.201670] 5.15.11-rt24-v8+ #3 Not tainted
> [ 32.201680] -----------------------------
> [ 32.201685] drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h:529 suspicious rcu_dereference_check() usage!
> [ 32.201695]
> [ 32.201695] other info that might help us debug this:
> [ 32.201695]
> [ 32.201700]
> [ 32.201700] rcu_scheduler_active = 2, debug_locks = 1
> [ 32.201708] no locks held by vchiq-slot/0/98.
> [ 32.201715]
> [ 32.201715] stack backtrace:
> [ 32.201723] CPU: 1 PID: 98 Comm: vchiq-slot/0 Not tainted 5.15.11-rt24-v8+ #3
> [ 32.201733] Hardware name: Raspberry Pi 4 Model B Rev 1.4 (DT)
> [ 32.201739] Call trace:
> [ 32.201742] dump_backtrace+0x0/0x1b8
> [ 32.201772] show_stack+0x20/0x30
> [ 32.201784] dump_stack_lvl+0x8c/0xb8
> [ 32.201799] dump_stack+0x18/0x34
> [ 32.201808] lockdep_rcu_suspicious+0xe4/0xf8
> [ 32.201817] service_callback+0x124/0x400
> [ 32.201830] slot_handler_func+0xf60/0x1e20
> [ 32.201839] kthread+0x19c/0x1a8
> [ 32.201849] ret_from_fork+0x10/0x20
>
> Signed-off-by: Padmanabha Srinivasaiah <treasure4paddy@xxxxxxxxx>
> ---
> Changes in v2:
> RCU dereferenced pointer need to be accessed inside rcu
> read-side critical section.
>
> .../vc04_services/interface/vchiq_arm/vchiq_arm.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 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 6759a6261500..8ddd400ab2c3 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -1053,24 +1053,30 @@ service_callback(enum vchiq_reason reason, struct vchiq_header *header,
> struct vchiq_service *service;
> struct vchiq_instance *instance;
> bool skip_completion = false;
> + unsigned int localport;
>
> DEBUG_INITIALISE(g_state.local);
>
> DEBUG_TRACE(SERVICE_CALLBACK_LINE);
>
> + rcu_read_lock();
> service = handle_to_service(handle);
> - if (WARN_ON(!service))
> + if (WARN_ON(!service)) {
> + rcu_read_unlock();
> return VCHIQ_SUCCESS;
> + }
>
> user_service = (struct user_service *)service->base.userdata;
user_service is part of the service struct and it's modification below
in this function is protected by a spinlock ( msg_queue_spinlock ). So i
would expected that all read accesses to user_service before the
spinlock are protected by RCU. After applying this patch there would be
still the check for "user_service->is_vchi" unprotected. But i'm not
sure about this.
Best regards
> instance = user_service->instance;
> + localport = service->localport;
> + rcu_read_unlock();
>
> if (!instance || instance->closing)
> return VCHIQ_SUCCESS;
>
> vchiq_log_trace(vchiq_arm_log_level,
> "%s - service %lx(%d,%p), reason %d, header %lx, instance %lx, bulk_userdata %lx",
> - __func__, (unsigned long)user_service, service->localport,
> + __func__, (unsigned long)user_service, (int)localport,
> user_service->userdata, reason, (unsigned long)header,
> (unsigned long)instance, (unsigned long)bulk_userdata);
>
