diff mbox series

[Openvpn-devel,ovpn-net] ovpn: fix incorrect use of rcu_access_pointer()

Message ID 20260722015638.1295-1-qingfang.deng@linux.dev
State New
Headers
Series [Openvpn-devel,ovpn-net] ovpn: fix incorrect use of rcu_access_pointer() |

Commit Message

Qingfang Deng July 22, 2026, 1:56 a.m. UTC 
  rcu_access_pointer() should only be used to test the value of a pointer,
not to dereference it. Use rcu_dereference() instead. No extra
rcu_read_lock() is needed here, as the spin_lock_bh() above marks an RCU
critical section.

Fixes: f6226ae7a0cd ("ovpn: introduce the ovpn_socket object")
Signed-off-by: Qingfang Deng <qingfang.deng@linux.dev>
---
 drivers/net/ovpn/peer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Antonio Quartulli July 22, 2026, 8:21 a.m. UTC | #1 
Hi!

On 22/07/2026 03:56, Qingfang Deng wrote:
> rcu_access_pointer() should only be used to test the value of a pointer,
> not to dereference it. Use rcu_dereference() instead. No extra
> rcu_read_lock() is needed here, as the spin_lock_bh() above marks an RCU
> critical section.
> 
> Fixes: f6226ae7a0cd ("ovpn: introduce the ovpn_socket object")
> Signed-off-by: Qingfang Deng <qingfang.deng@linux.dev>
> ---
>   drivers/net/ovpn/peer.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ovpn/peer.c b/drivers/net/ovpn/peer.c
> index a09d61296425..8de737579112 100644
> --- a/drivers/net/ovpn/peer.c
> +++ b/drivers/net/ovpn/peer.c
> @@ -1164,7 +1164,7 @@ static void ovpn_peer_release_p2p(struct ovpn_priv *ovpn, struct sock *sk,
>   	}
>   
>   	if (sk) {
> -		ovpn_sock = rcu_access_pointer(peer->sock);
> +		ovpn_sock = rcu_dereference(peer->sock);

Sashiko says:

Using `rcu_dereference()` instead of `rcu_dereference_bh()` under 
`spin_lock_bh()` triggers a lockdep warning.
Qingfang Deng July 23, 2026, 1:38 a.m. UTC | #2 
Hi,
On 2026/7/22 16:21, Antonio Quartulli wrote:
> Hi!
>
> On 22/07/2026 03:56, Qingfang Deng wrote:
>> rcu_access_pointer() should only be used to test the value of a pointer,
>> not to dereference it. Use rcu_dereference() instead. No extra
>> rcu_read_lock() is needed here, as the spin_lock_bh() above marks an RCU
>> critical section.
>>
>> Fixes: f6226ae7a0cd ("ovpn: introduce the ovpn_socket object")
>> Signed-off-by: Qingfang Deng <qingfang.deng@linux.dev>
>> ---
>>   drivers/net/ovpn/peer.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ovpn/peer.c b/drivers/net/ovpn/peer.c
>> index a09d61296425..8de737579112 100644
>> --- a/drivers/net/ovpn/peer.c
>> +++ b/drivers/net/ovpn/peer.c
>> @@ -1164,7 +1164,7 @@ static void ovpn_peer_release_p2p(struct 
>> ovpn_priv *ovpn, struct sock *sk,
>>       }
>>         if (sk) {
>> -        ovpn_sock = rcu_access_pointer(peer->sock);
>> +        ovpn_sock = rcu_dereference(peer->sock);
>
> Sashiko says:
>
> Using `rcu_dereference()` instead of `rcu_dereference_bh()` under 
> `spin_lock_bh()` triggers a lockdep warning.
>
I think `rcu_dereference()` has more relax checks than 
`rcu_dereference_bh()` and Sashiko is wrong, i.e. using 
`rcu_dereference()` in a BH-disabled context is okay, but using 
`rcu_dereference_bh()` in a process context with only `rcu_read_lock()` 
is _not_ okay. Can you verify that?

Best regards,

Qingfang
Ralf Lici July 23, 2026, 7:59 a.m. UTC | #3 
On Thu, 23 Jul 2026 09:38:40 +0800, Qingfang Deng <qingfang.deng@linux.dev> wrote:
> Hi,

Hi Qingfang,

> On 2026/7/22 16:21, Antonio Quartulli wrote:
> > Hi!
> >
> > On 22/07/2026 03:56, Qingfang Deng wrote:
> >> rcu_access_pointer() should only be used to test the value of a pointer,
> >> not to dereference it. Use rcu_dereference() instead. No extra
> >> rcu_read_lock() is needed here, as the spin_lock_bh() above marks an RCU
> >> critical section.
> >>
> >> Fixes: f6226ae7a0cd ("ovpn: introduce the ovpn_socket object")
> >> Signed-off-by: Qingfang Deng <qingfang.deng@linux.dev>
> >> ---
> >>   drivers/net/ovpn/peer.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/net/ovpn/peer.c b/drivers/net/ovpn/peer.c
> >> index a09d61296425..8de737579112 100644
> >> --- a/drivers/net/ovpn/peer.c
> >> +++ b/drivers/net/ovpn/peer.c
> >> @@ -1164,7 +1164,7 @@ static void ovpn_peer_release_p2p(struct 
> >> ovpn_priv *ovpn, struct sock *sk,
> >>       }
> >>         if (sk) {
> >> -        ovpn_sock = rcu_access_pointer(peer->sock);
> >> +        ovpn_sock = rcu_dereference(peer->sock);
> >
> > Sashiko says:
> >
> > Using `rcu_dereference()` instead of `rcu_dereference_bh()` under 
> > `spin_lock_bh()` triggers a lockdep warning.
> >
> I think `rcu_dereference()` has more relax checks than 
> `rcu_dereference_bh()` and Sashiko is wrong, i.e. using 
> `rcu_dereference()` in a BH-disabled context is okay, but using 
> `rcu_dereference_bh()` in a process context with only `rcu_read_lock()` 
> is _not_ okay. Can you verify that?
>

rcu_dereference relies on rcu_read_lock_held to check that the caller
context is valid. However, ovpn_peer_release_p2p does not hold a normal
rcu_read_lock section, it relies on the BH-disabled section established
by spin_lock_bh instead.

rcu_read_lock_held does not consider BH-disabled context, so it will
return false here. In contrast, rcu_read_lock_bh_held explicitly
recognizes BH-disabled/softirq context.

So I think Sashiko is right: rcu_dereference will trigger its RCU
lockdep warning with CONFIG_PROVE_RCU enabled. This should use
rcu_dereference_bh instead.

Kind regards.
diff mbox series

Patch

diff --git a/drivers/net/ovpn/peer.c b/drivers/net/ovpn/peer.c
index a09d61296425..8de737579112 100644
--- a/drivers/net/ovpn/peer.c
+++ b/drivers/net/ovpn/peer.c
@@ -1164,7 +1164,7 @@  static void ovpn_peer_release_p2p(struct ovpn_priv *ovpn, struct sock *sk,
 	}
 
 	if (sk) {
-		ovpn_sock = rcu_access_pointer(peer->sock);
+		ovpn_sock = rcu_dereference(peer->sock);
 		if (!ovpn_sock || ovpn_sock->sk != sk) {
 			spin_unlock_bh(&ovpn->lock);
 			ovpn_peer_put(peer);