All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] staging: vc04_services: Fix RCU dereference check
@ 2021-12-29 15:32 ` Padmanabha Srinivasaiah
  0 siblings, 0 replies; 16+ messages in thread
From: Padmanabha Srinivasaiah @ 2021-12-29 15:32 UTC (permalink / raw)
  To: linux-rpi-kernel, linux-arm-kernel, linux-staging, linux-kernel
  Cc: gregkh, nsaenz, treasure4paddy, Stefan Wahren, Gaston Gonzalez,
	Ojaswin Mujoo, Arnd Bergmann, Phil Elwell,
	bcm-kernel-feedback-list

In service_callback path RCU dereference done without
rcu_read_[lock/unlock] pair, fixing same by using them.

[   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>
---
 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 3 +++
 1 file changed, 3 insertions(+)

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..ee1b48db9681 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -1058,7 +1058,10 @@ service_callback(enum vchiq_reason reason, struct vchiq_header *header,
 
 	DEBUG_TRACE(SERVICE_CALLBACK_LINE);
 
+	rcu_read_lock();
 	service = handle_to_service(handle);
+	rcu_read_unlock();
+
 	if (WARN_ON(!service))
 		return VCHIQ_SUCCESS;
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread
* [PATCH] staging: vc04_services: Fix RCU dereference check
@ 2021-12-29 15:32 ` Padmanabha Srinivasaiah
  0 siblings, 0 replies; 16+ messages in thread
From: Padmanabha Srinivasaiah @ 2021-12-29 15:32 UTC (permalink / raw)
  To: linux-rpi-kernel, linux-arm-kernel, linux-staging, linux-kernel
  Cc: gregkh, nsaenz, treasure4paddy, Stefan Wahren, Gaston Gonzalez,
	Ojaswin Mujoo, Arnd Bergmann, Phil Elwell,
	bcm-kernel-feedback-list

In service_callback path RCU dereference done without
rcu_read_[lock/unlock] pair, fixing same by using them.

[   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>
---
 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 3 +++
 1 file changed, 3 insertions(+)

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..ee1b48db9681 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -1058,7 +1058,10 @@ service_callback(enum vchiq_reason reason, struct vchiq_header *header,
 
 	DEBUG_TRACE(SERVICE_CALLBACK_LINE);
 
+	rcu_read_lock();
 	service = handle_to_service(handle);
+	rcu_read_unlock();
+
 	if (WARN_ON(!service))
 		return VCHIQ_SUCCESS;
 
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 16+ messages in thread
* Re: [PATCH] staging: vc04_services: Fix RCU dereference check
  2021-12-29 15:32 ` Padmanabha Srinivasaiah
@ 2021-12-29 19:13   ` Stefan Wahren
  -1 siblings, 0 replies; 16+ messages in thread
From: Stefan Wahren @ 2021-12-29 19:13 UTC (permalink / raw)
  To: Padmanabha Srinivasaiah, linux-rpi-kernel, linux-arm-kernel,
	linux-staging, linux-kernel
  Cc: gregkh, nsaenz, Gaston Gonzalez, Ojaswin Mujoo, Arnd Bergmann,
	Phil Elwell, bcm-kernel-feedback-list

Hi Padmanabha,

Am 29.12.21 um 16:32 schrieb Padmanabha Srinivasaiah:
> In service_callback path RCU dereference done without
> rcu_read_[lock/unlock] pair, fixing same by using them.
>
> [   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>
> ---
>  drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> 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..ee1b48db9681 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -1058,7 +1058,10 @@ service_callback(enum vchiq_reason reason, struct vchiq_header *header,
>  
>  	DEBUG_TRACE(SERVICE_CALLBACK_LINE);
>  
> +	rcu_read_lock();
>  	service = handle_to_service(handle);
> +	rcu_read_unlock();
> +
>  	if (WARN_ON(!service))
>  		return VCHIQ_SUCCESS;
>  

thanks for reporting this issue.

Could you please explain how to reproduce this issue?

What makes you sure that your patch fixes the issue and not just hiding
the warning?

Best regards


^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH] staging: vc04_services: Fix RCU dereference check
@ 2021-12-29 19:13   ` Stefan Wahren
  0 siblings, 0 replies; 16+ messages in thread
From: Stefan Wahren @ 2021-12-29 19:13 UTC (permalink / raw)
  To: Padmanabha Srinivasaiah, linux-rpi-kernel, linux-arm-kernel,
	linux-staging, linux-kernel
  Cc: gregkh, nsaenz, Gaston Gonzalez, Ojaswin Mujoo, Arnd Bergmann,
	Phil Elwell, bcm-kernel-feedback-list

Hi Padmanabha,

Am 29.12.21 um 16:32 schrieb Padmanabha Srinivasaiah:
> In service_callback path RCU dereference done without
> rcu_read_[lock/unlock] pair, fixing same by using them.
>
> [   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>
> ---
>  drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> 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..ee1b48db9681 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -1058,7 +1058,10 @@ service_callback(enum vchiq_reason reason, struct vchiq_header *header,
>  
>  	DEBUG_TRACE(SERVICE_CALLBACK_LINE);
>  
> +	rcu_read_lock();
>  	service = handle_to_service(handle);
> +	rcu_read_unlock();
> +
>  	if (WARN_ON(!service))
>  		return VCHIQ_SUCCESS;
>  

thanks for reporting this issue.

Could you please explain how to reproduce this issue?

What makes you sure that your patch fixes the issue and not just hiding
the warning?

Best regards


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH] staging: vc04_services: Fix RCU dereference check
  2021-12-29 19:13   ` Stefan Wahren
@ 2021-12-30 13:34     ` Padmanabha Srinivasaiah
  -1 siblings, 0 replies; 16+ messages in thread
From: Padmanabha Srinivasaiah @ 2021-12-30 13:34 UTC (permalink / raw)
  To: Stefan Wahren
  Cc: linux-rpi-kernel, linux-arm-kernel, linux-staging, linux-kernel,
	gregkh, nsaenz, Gaston Gonzalez, Ojaswin Mujoo, Arnd Bergmann,
	Phil Elwell, bcm-kernel-feedback-list

On Wed, Dec 29, 2021 at 08:13:40PM +0100, Stefan Wahren wrote:
> Hi Padmanabha,
> 
> Am 29.12.21 um 16:32 schrieb Padmanabha Srinivasaiah:
> > In service_callback path RCU dereference done without
> > rcu_read_[lock/unlock] pair, fixing same by using them.
> >
> > [   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>
> > ---
> >  drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > 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..ee1b48db9681 100644
> > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > @@ -1058,7 +1058,10 @@ service_callback(enum vchiq_reason reason, struct vchiq_header *header,
> >  
> >  	DEBUG_TRACE(SERVICE_CALLBACK_LINE);
> >  
> > +	rcu_read_lock();
> >  	service = handle_to_service(handle);
> > +	rcu_read_unlock();
> > +
> >  	if (WARN_ON(!service))
> >  		return VCHIQ_SUCCESS;
> >  
> 
> thanks for reporting this issue.
> 
> Could you please explain how to reproduce this issue?

Have enabled lock dependency check in 5.15 plus rt patchset have been
applied. And to catch this warning not loading any modules from default
build.

> 
> What makes you sure that your patch fixes the issue and not just hiding
> the warning?

Thank you stefan for the question. Yes, you are right dereferenced pointer
access should be inside the rcu read-side critical section. Will regenerate the patch
and will share for the review.

> Best regards
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH] staging: vc04_services: Fix RCU dereference check
@ 2021-12-30 13:34     ` Padmanabha Srinivasaiah
  0 siblings, 0 replies; 16+ messages in thread
From: Padmanabha Srinivasaiah @ 2021-12-30 13:34 UTC (permalink / raw)
  To: Stefan Wahren
  Cc: linux-rpi-kernel, linux-arm-kernel, linux-staging, linux-kernel,
	gregkh, nsaenz, Gaston Gonzalez, Ojaswin Mujoo, Arnd Bergmann,
	Phil Elwell, bcm-kernel-feedback-list

On Wed, Dec 29, 2021 at 08:13:40PM +0100, Stefan Wahren wrote:
> Hi Padmanabha,
> 
> Am 29.12.21 um 16:32 schrieb Padmanabha Srinivasaiah:
> > In service_callback path RCU dereference done without
> > rcu_read_[lock/unlock] pair, fixing same by using them.
> >
> > [   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>
> > ---
> >  drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > 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..ee1b48db9681 100644
> > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > @@ -1058,7 +1058,10 @@ service_callback(enum vchiq_reason reason, struct vchiq_header *header,
> >  
> >  	DEBUG_TRACE(SERVICE_CALLBACK_LINE);
> >  
> > +	rcu_read_lock();
> >  	service = handle_to_service(handle);
> > +	rcu_read_unlock();
> > +
> >  	if (WARN_ON(!service))
> >  		return VCHIQ_SUCCESS;
> >  
> 
> thanks for reporting this issue.
> 
> Could you please explain how to reproduce this issue?

Have enabled lock dependency check in 5.15 plus rt patchset have been
applied. And to catch this warning not loading any modules from default
build.

> 
> What makes you sure that your patch fixes the issue and not just hiding
> the warning?

Thank you stefan for the question. Yes, you are right dereferenced pointer
access should be inside the rcu read-side critical section. Will regenerate the patch
and will share for the review.

> Best regards
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 16+ messages in thread
* [PATCH v2] staging: vc04_services: Fix RCU dereference check
  2021-12-30 13:34     ` Padmanabha Srinivasaiah
@ 2021-12-30 14:54       ` Padmanabha Srinivasaiah
  -1 siblings, 0 replies; 16+ messages in thread
From: Padmanabha Srinivasaiah @ 2021-12-30 14:54 UTC (permalink / raw)
  To: linux-rpi-kernel, linux-arm-kernel, linux-staging, linux-kernel,
	stefan.wahren
  Cc: gregkh, nsaenz, treasure4paddy, Gaston Gonzalez, Ojaswin Mujoo,
	Arnd Bergmann, Phil Elwell, bcm-kernel-feedback-list

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


^ permalink raw reply related	[flat|nested] 16+ messages in thread
* [PATCH v2] staging: vc04_services: Fix RCU dereference check
@ 2021-12-30 14:54       ` Padmanabha Srinivasaiah
  0 siblings, 0 replies; 16+ messages in thread
From: Padmanabha Srinivasaiah @ 2021-12-30 14:54 UTC (permalink / raw)
  To: linux-rpi-kernel, linux-arm-kernel, linux-staging, linux-kernel,
	stefan.wahren
  Cc: gregkh, nsaenz, treasure4paddy, Gaston Gonzalez, Ojaswin Mujoo,
	Arnd Bergmann, Phil Elwell, bcm-kernel-feedback-list

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 16+ messages in thread
* Re: [PATCH v2] staging: vc04_services: Fix RCU dereference check
  2021-12-30 14:54       ` Padmanabha Srinivasaiah
@ 2021-12-30 21:39         ` Stefan Wahren
  -1 siblings, 0 replies; 16+ messages in thread
From: Stefan Wahren @ 2021-12-30 21:39 UTC (permalink / raw)
  To: Padmanabha Srinivasaiah, linux-rpi-kernel, linux-arm-kernel,
	linux-staging, linux-kernel
  Cc: gregkh, nsaenz, Gaston Gonzalez, Ojaswin Mujoo, Arnd Bergmann,
	Phil Elwell, bcm-kernel-feedback-list

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


^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH v2] staging: vc04_services: Fix RCU dereference check
@ 2021-12-30 21:39         ` Stefan Wahren
  0 siblings, 0 replies; 16+ messages in thread
From: Stefan Wahren @ 2021-12-30 21:39 UTC (permalink / raw)
  To: Padmanabha Srinivasaiah, linux-rpi-kernel, linux-arm-kernel,
	linux-staging, linux-kernel
  Cc: gregkh, nsaenz, Gaston Gonzalez, Ojaswin Mujoo, Arnd Bergmann,
	Phil Elwell, bcm-kernel-feedback-list

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH v2] staging: vc04_services: Fix RCU dereference check
  2021-12-30 21:39         ` Stefan Wahren
@ 2021-12-31 18:56           ` Padmanabha Srinivasaiah
  -1 siblings, 0 replies; 16+ messages in thread
From: Padmanabha Srinivasaiah @ 2021-12-31 18:56 UTC (permalink / raw)
  To: Stefan Wahren
  Cc: linux-rpi-kernel, linux-arm-kernel, linux-staging, linux-kernel,
	gregkh, nsaenz, Gaston Gonzalez, Ojaswin Mujoo, Arnd Bergmann,
	Phil Elwell, bcm-kernel-feedback-list

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

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH v2] staging: vc04_services: Fix RCU dereference check
@ 2021-12-31 18:56           ` Padmanabha Srinivasaiah
  0 siblings, 0 replies; 16+ messages in thread
From: Padmanabha Srinivasaiah @ 2021-12-31 18:56 UTC (permalink / raw)
  To: Stefan Wahren
  Cc: linux-rpi-kernel, linux-arm-kernel, linux-staging, linux-kernel,
	gregkh, nsaenz, Gaston Gonzalez, Ojaswin Mujoo, Arnd Bergmann,
	Phil Elwell, bcm-kernel-feedback-list

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 16+ messages in thread
* [PATCH v3] staging: vc04_services: Fix RCU dereference check
  2021-12-31 18:56           ` Padmanabha Srinivasaiah
@ 2021-12-31 19:54             ` Padmanabha Srinivasaiah
  -1 siblings, 0 replies; 16+ messages in thread
From: Padmanabha Srinivasaiah @ 2021-12-31 19:54 UTC (permalink / raw)
  To: linux-rpi-kernel, linux-arm-kernel, linux-staging, linux-kernel,
	stefan.wahren
  Cc: gregkh, nsaenz, treasure4paddy, Gaston Gonzalez, Ojaswin Mujoo,
	Arnd Bergmann, Phil Elwell, bcm-kernel-feedback-list

In service_callback path RCU dereferenced pointer struct vchiq_service
need to be accessed inside rcu read-critical section.

Also userdata/user_service part of vchiq_service is accessed around
different synchronization mechanism, getting an extra reference to a
pointer keeps sematics simpler and avoids prolonged graceperiod.

Accessing vchiq_service with rcu_read_[lock/unlock] fixes below 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>
---
Change in v3:
	Taking an extra reference for service pointer to keep semantics
simpler to accesses in different synchronization mechanism.

Changes in v2:
        RCU dereferenced pointer need to be accessed inside rcu
read-side critical section.

 .../interface/vchiq_arm/vchiq_arm.c           | 20 +++++++++++++++++--
 1 file changed, 18 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..3a2e4582db8e 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -1058,15 +1058,27 @@ service_callback(enum vchiq_reason reason, struct vchiq_header *header,
 
 	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;
 
-	if (!instance || instance->closing)
+	if (!instance || instance->closing) {
+		rcu_read_unlock();
 		return VCHIQ_SUCCESS;
+	}
+
+	/*
+	 * As hopping around different synchronization mechanism,
+	 * taking an extra reference results in simpler implementation.
+	 */
+	vchiq_service_get(service);
+	rcu_read_unlock();
 
 	vchiq_log_trace(vchiq_arm_log_level,
 			"%s - service %lx(%d,%p), reason %d, header %lx, instance %lx, bulk_userdata %lx",
@@ -1097,6 +1109,7 @@ service_callback(enum vchiq_reason reason, struct vchiq_header *header,
 							bulk_userdata);
 				if (status != VCHIQ_SUCCESS) {
 					DEBUG_TRACE(SERVICE_CALLBACK_LINE);
+					vchiq_service_put(service);
 					return status;
 				}
 			}
@@ -1105,10 +1118,12 @@ service_callback(enum vchiq_reason reason, struct vchiq_header *header,
 			if (wait_for_completion_interruptible(&user_service->remove_event)) {
 				vchiq_log_info(vchiq_arm_log_level, "%s interrupted", __func__);
 				DEBUG_TRACE(SERVICE_CALLBACK_LINE);
+				vchiq_service_put(service);
 				return VCHIQ_RETRY;
 			} else if (instance->closing) {
 				vchiq_log_info(vchiq_arm_log_level, "%s closing", __func__);
 				DEBUG_TRACE(SERVICE_CALLBACK_LINE);
+				vchiq_service_put(service);
 				return VCHIQ_ERROR;
 			}
 			DEBUG_TRACE(SERVICE_CALLBACK_LINE);
@@ -1137,6 +1152,7 @@ service_callback(enum vchiq_reason reason, struct vchiq_header *header,
 		header = NULL;
 	}
 	DEBUG_TRACE(SERVICE_CALLBACK_LINE);
+	vchiq_service_put(service);
 
 	if (skip_completion)
 		return VCHIQ_SUCCESS;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread
* [PATCH v3] staging: vc04_services: Fix RCU dereference check
@ 2021-12-31 19:54             ` Padmanabha Srinivasaiah
  0 siblings, 0 replies; 16+ messages in thread
From: Padmanabha Srinivasaiah @ 2021-12-31 19:54 UTC (permalink / raw)
  To: linux-rpi-kernel, linux-arm-kernel, linux-staging, linux-kernel,
	stefan.wahren
  Cc: gregkh, nsaenz, treasure4paddy, Gaston Gonzalez, Ojaswin Mujoo,
	Arnd Bergmann, Phil Elwell, bcm-kernel-feedback-list

In service_callback path RCU dereferenced pointer struct vchiq_service
need to be accessed inside rcu read-critical section.

Also userdata/user_service part of vchiq_service is accessed around
different synchronization mechanism, getting an extra reference to a
pointer keeps sematics simpler and avoids prolonged graceperiod.

Accessing vchiq_service with rcu_read_[lock/unlock] fixes below 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>
---
Change in v3:
	Taking an extra reference for service pointer to keep semantics
simpler to accesses in different synchronization mechanism.

Changes in v2:
        RCU dereferenced pointer need to be accessed inside rcu
read-side critical section.

 .../interface/vchiq_arm/vchiq_arm.c           | 20 +++++++++++++++++--
 1 file changed, 18 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..3a2e4582db8e 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -1058,15 +1058,27 @@ service_callback(enum vchiq_reason reason, struct vchiq_header *header,
 
 	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;
 
-	if (!instance || instance->closing)
+	if (!instance || instance->closing) {
+		rcu_read_unlock();
 		return VCHIQ_SUCCESS;
+	}
+
+	/*
+	 * As hopping around different synchronization mechanism,
+	 * taking an extra reference results in simpler implementation.
+	 */
+	vchiq_service_get(service);
+	rcu_read_unlock();
 
 	vchiq_log_trace(vchiq_arm_log_level,
 			"%s - service %lx(%d,%p), reason %d, header %lx, instance %lx, bulk_userdata %lx",
@@ -1097,6 +1109,7 @@ service_callback(enum vchiq_reason reason, struct vchiq_header *header,
 							bulk_userdata);
 				if (status != VCHIQ_SUCCESS) {
 					DEBUG_TRACE(SERVICE_CALLBACK_LINE);
+					vchiq_service_put(service);
 					return status;
 				}
 			}
@@ -1105,10 +1118,12 @@ service_callback(enum vchiq_reason reason, struct vchiq_header *header,
 			if (wait_for_completion_interruptible(&user_service->remove_event)) {
 				vchiq_log_info(vchiq_arm_log_level, "%s interrupted", __func__);
 				DEBUG_TRACE(SERVICE_CALLBACK_LINE);
+				vchiq_service_put(service);
 				return VCHIQ_RETRY;
 			} else if (instance->closing) {
 				vchiq_log_info(vchiq_arm_log_level, "%s closing", __func__);
 				DEBUG_TRACE(SERVICE_CALLBACK_LINE);
+				vchiq_service_put(service);
 				return VCHIQ_ERROR;
 			}
 			DEBUG_TRACE(SERVICE_CALLBACK_LINE);
@@ -1137,6 +1152,7 @@ service_callback(enum vchiq_reason reason, struct vchiq_header *header,
 		header = NULL;
 	}
 	DEBUG_TRACE(SERVICE_CALLBACK_LINE);
+	vchiq_service_put(service);
 
 	if (skip_completion)
 		return VCHIQ_SUCCESS;
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 16+ messages in thread
* Re: [PATCH v3] staging: vc04_services: Fix RCU dereference check
  2021-12-31 19:54             ` Padmanabha Srinivasaiah
@ 2022-01-23 17:32               ` Stefan Wahren
  -1 siblings, 0 replies; 16+ messages in thread
From: Stefan Wahren @ 2022-01-23 17:32 UTC (permalink / raw)
  To: Padmanabha Srinivasaiah, linux-rpi-kernel, linux-arm-kernel,
	linux-staging, linux-kernel
  Cc: gregkh, nsaenz, Gaston Gonzalez, Ojaswin Mujoo, Arnd Bergmann,
	Phil Elwell, bcm-kernel-feedback-list

Am 31.12.21 um 20:54 schrieb Padmanabha Srinivasaiah:
> In service_callback path RCU dereferenced pointer struct vchiq_service
> need to be accessed inside rcu read-critical section.
>
> Also userdata/user_service part of vchiq_service is accessed around
> different synchronization mechanism, getting an extra reference to a
> pointer keeps sematics simpler and avoids prolonged graceperiod.
>
> Accessing vchiq_service with rcu_read_[lock/unlock] fixes below 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>

Tested-by: Stefan Wahren <stefan.wahren@i2se.com>

Thanks


^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH v3] staging: vc04_services: Fix RCU dereference check
@ 2022-01-23 17:32               ` Stefan Wahren
  0 siblings, 0 replies; 16+ messages in thread
From: Stefan Wahren @ 2022-01-23 17:32 UTC (permalink / raw)
  To: Padmanabha Srinivasaiah, linux-rpi-kernel, linux-arm-kernel,
	linux-staging, linux-kernel
  Cc: gregkh, nsaenz, Gaston Gonzalez, Ojaswin Mujoo, Arnd Bergmann,
	Phil Elwell, bcm-kernel-feedback-list

Am 31.12.21 um 20:54 schrieb Padmanabha Srinivasaiah:
> In service_callback path RCU dereferenced pointer struct vchiq_service
> need to be accessed inside rcu read-critical section.
>
> Also userdata/user_service part of vchiq_service is accessed around
> different synchronization mechanism, getting an extra reference to a
> pointer keeps sematics simpler and avoids prolonged graceperiod.
>
> Accessing vchiq_service with rcu_read_[lock/unlock] fixes below 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>

Tested-by: Stefan Wahren <stefan.wahren@i2se.com>

Thanks


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 16+ messages in thread
end of thread, other threads:[~2022-01-23 17:33 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-29 15:32 [PATCH] staging: vc04_services: Fix RCU dereference check Padmanabha Srinivasaiah
2021-12-29 15:32 ` Padmanabha Srinivasaiah
2021-12-29 19:13 ` Stefan Wahren
2021-12-29 19:13   ` Stefan Wahren
2021-12-30 13:34   ` Padmanabha Srinivasaiah
2021-12-30 13:34     ` Padmanabha Srinivasaiah
2021-12-30 14:54     ` [PATCH v2] " Padmanabha Srinivasaiah
2021-12-30 14:54       ` Padmanabha Srinivasaiah
2021-12-30 21:39       ` Stefan Wahren
2021-12-30 21:39         ` Stefan Wahren
2021-12-31 18:56         ` Padmanabha Srinivasaiah
2021-12-31 18:56           ` Padmanabha Srinivasaiah
2021-12-31 19:54           ` [PATCH v3] " Padmanabha Srinivasaiah
2021-12-31 19:54             ` Padmanabha Srinivasaiah
2022-01-23 17:32             ` Stefan Wahren
2022-01-23 17:32               ` Stefan Wahren

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.