diff mbox series

[v2] staging: vc04_services: Fix RCU dereference check

Message ID 20211230145415.11962-1-treasure4paddy@gmail.com (mailing list archive)
State New, archived
Headers show
Series [v2] staging: vc04_services: Fix RCU dereference check | expand

Commit Message

Padmanabha Srinivasaiah Dec. 30, 2021, 2:54 p.m. UTC 
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@gmail.com>
---
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(-)

Comments

Stefan Wahren Dec. 30, 2021, 9:39 p.m. UTC | #1 
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@gmail.com>
> ---
> 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);
>
Padmanabha Srinivasaiah Dec. 31, 2021, 6:56 p.m. UTC | #2 
On Thu, Dec 30, 2021 at 10:39:58PM +0100, Stefan Wahren wrote:
> 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@gmail.com>
> > ---
> > 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.
>
Thank you stefan for identfying it. Yes, userdata/user_service can be
potentially released after graceperiod.

Also here pointer is used around different synchronization mechanism,
taking an extra reference will keep semantics simpler and will not
prolong the graceperiod. Will regenerate the patch again.

> 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);
> >  
>
diff mbox series

Patch

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;
 	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);