| Message ID | 20251014101948.250823-1-ralf@mandelbit.com |
|---|---|
| State | New |
| Headers | show |
| Series | [Openvpn-devel,ovpn-net-next] ovpn: only signal sk readable if user_queue has packets | expand |
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 >
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. >
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 &&
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(+)