diff mbox series

[Openvpn-devel,ovpn-net-next] ovpn: only signal sk readable if user_queue has packets

Message ID 20251014101948.250823-1-ralf@mandelbit.com
State Awaiting Upstream
Headers
Series [Openvpn-devel,ovpn-net-next] ovpn: only signal sk readable if user_queue has packets |

Commit Message

Ralf Lici Oct. 14, 2025, 10:19 a.m. UTC 
  datagram_poll marks a socket as readable whenever there are packets in
the standard sk_receive_queue. However, with TCP encapsulation, another
queue is used to hold packets destined for userspace.

To ensure correct EPOLLIN signaling, reset EPOLLIN and EPOLLRDNORM after
calling datagram_poll, and set them only if user_queue is not empty.

Signed-off-by: Ralf Lici <ralf@mandelbit.com>
---
 drivers/net/ovpn/tcp.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Sabrina Dubroca Oct. 14, 2025, 3:01 p.m. UTC | #1 
2025-10-14, 12:19:48 +0200, Ralf Lici wrote:
> datagram_poll marks a socket as readable whenever there are packets in
> the standard sk_receive_queue. However, with TCP encapsulation, another
> queue is used to hold packets destined for userspace.
> 
> To ensure correct EPOLLIN signaling, reset EPOLLIN and EPOLLRDNORM after
> calling datagram_poll, and set them only if user_queue is not empty.
> 
> Signed-off-by: Ralf Lici <ralf@mandelbit.com>
> ---
>  drivers/net/ovpn/tcp.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/net/ovpn/tcp.c b/drivers/net/ovpn/tcp.c
> index 289f62c5d2c7..4dbdd59a0b1c 100644
> --- a/drivers/net/ovpn/tcp.c
> +++ b/drivers/net/ovpn/tcp.c
> @@ -563,6 +563,11 @@ static __poll_t ovpn_tcp_poll(struct file *file, struct socket *sock,
>  	__poll_t mask = datagram_poll(file, sock, wait);
>  	struct ovpn_socket *ovpn_sock;
>  
> +	/* Mark socket as readable for userspace only if there are packets in
> +	 * the user_queue, not in sk_receive_queue.
> +	 */
> +	mask &= ~(EPOLLIN | EPOLLRDNORM);

This would clear EPOLLIN and EPOLLRDNORM if they were set (alongside
EPOLLRDHUP) because the socket was shutdown():

// https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/tree/net/core/datagram.c?h=v6.17#n952
	shutdown = READ_ONCE(sk->sk_shutdown);
	if (shutdown & RCV_SHUTDOWN)
		mask |= EPOLLRDHUP | EPOLLIN | EPOLLRDNORM;

which may not be desireable?


Maybe another way to deal with it would be to add a variant of
datagram_poll that takes the "real" RX queue as extra argument, and
make datagram_poll a wrapper (using sk_receive_queue) around it. Then
ovpn could use this new __datagram_poll helper with tcp.user_queue
(but I'm not sure we can call datagram_poll under rcu_read_lock).

>  	rcu_read_lock();
>  	ovpn_sock = rcu_dereference_sk_user_data(sock->sk);
>  	if (ovpn_sock && ovpn_sock->peer &&
> -- 
> 2.51.0
>
Ralf Lici Oct. 14, 2025, 4:29 p.m. UTC | #2 
On Tue, 2025-10-14 at 17:01 +0200, Sabrina Dubroca wrote:
> This would clear EPOLLIN and EPOLLRDNORM if they were set (alongside
> EPOLLRDHUP) because the socket was shutdown():
> 
> //
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/tree/net/core/datagram.c?h=v6.17#n952
> 	shutdown = READ_ONCE(sk->sk_shutdown);
> 	if (shutdown & RCV_SHUTDOWN)
> 		mask |= EPOLLRDHUP | EPOLLIN | EPOLLRDNORM;
> 
> which may not be desireable?
> 
> 
> Maybe another way to deal with it would be to add a variant of
> datagram_poll that takes the "real" RX queue as extra argument, and
> make datagram_poll a wrapper (using sk_receive_queue) around it. Then
> ovpn could use this new __datagram_poll helper with tcp.user_queue

I totally agree.

> (but I'm not sure we can call datagram_poll under rcu_read_lock).

I see the problem may arise from the call to sock_poll_wait, which isn't
safe inside an rcu_read_lock() section. Perhaps the __datagram_poll
helper could delegate the responsibility of calling sock_poll_wait to
the caller, allowing it to perform the rest of the polling logic safely
within the RCU read-side critical section.

>
Ralf Lici Oct. 15, 2025, 8:16 a.m. UTC | #3 
On Tue, 2025-10-14 at 18:29 +0200, ralf@mandelbit.com wrote:
> On Tue, 2025-10-14 at 17:01 +0200, Sabrina Dubroca wrote:
> > (but I'm not sure we can call datagram_poll under rcu_read_lock).
> 
> I see the problem may arise from the call to sock_poll_wait, which
> isn't
> safe inside an rcu_read_lock() section. Perhaps the __datagram_poll
> helper could delegate the responsibility of calling sock_poll_wait to
> the caller, allowing it to perform the rest of the polling logic
> safely
> within the RCU read-side critical section.
> 

After discussing this with Antonio, we realized that we can safely take
a reference to the queue and increment the peer's refcounter, allowing
us to call __datagram_poll outside the RCU section. This approach lets
us avoid modifying the original kernel function.

I'll send a patch shortly.

Thanks for your help!
diff mbox series

Patch

diff --git a/drivers/net/ovpn/tcp.c b/drivers/net/ovpn/tcp.c
index 289f62c5d2c7..4dbdd59a0b1c 100644
--- a/drivers/net/ovpn/tcp.c
+++ b/drivers/net/ovpn/tcp.c
@@ -563,6 +563,11 @@  static __poll_t ovpn_tcp_poll(struct file *file, struct socket *sock,
 	__poll_t mask = datagram_poll(file, sock, wait);
 	struct ovpn_socket *ovpn_sock;
 
+	/* Mark socket as readable for userspace only if there are packets in
+	 * the user_queue, not in sk_receive_queue.
+	 */
+	mask &= ~(EPOLLIN | EPOLLRDNORM);
+
 	rcu_read_lock();
 	ovpn_sock = rcu_dereference_sk_user_data(sock->sk);
 	if (ovpn_sock && ovpn_sock->peer &&