On Wed, 2026-01-21 at 17:48 +0100, Sabrina Dubroca wrote:
> 2026-01-19, 14:13:58 +0100, Ralf Lici wrote:
> > During initialization, we override some socket callbacks and set
> > sk_user_data to ovpn_sock. Currently these two operations are
> > decoupled:
> > the callbacks are overridden before sk_user_data is set, leaving a
> > potentially ill-formed state while socket ownership is not yet
> > complete.
> > For example, if a packet arrives after ovpn_udp_socket_attach has
> > been
> > called but before ovpn_socket_new finishes, ovpn_udp_encap_recv may
> > be
> > invoked without a configured sk_user_data pointer.
>
> Since all the callbacks check whether sk_user_data is set before doing
> anything, can you describe the problem?
True, but if we receive UDP traffic before the socket setup is
completed, we get some net_err_ratelimited messages in the logs. While
this is not necessarily a real bug, setting user_data before the
callbacks ensures we do not hit this situation and avoids polluting the
logs. This should also be safe, since no other user is "owning" the
socket at that point.
I will update the commit message with this info.
> > Set sk_user_data before overriding the callbacks so that it can be
> > accessed safely from them. Since we already check that the socket
> > has no
> > sk_user_data before setting it, this remains safe even if an
> > interrupt
> > accesses the socket after sk_user_data is set but before the
> > callbacks
> > are overridden.
>
> A Fixes: tag would be good to add here. (probably f6226ae7a0cd ("ovpn:
> introduce the ovpn_socket object"), don't worry too much about which
> exact patch within the initial series is best)
Sure, I'll add it.
> > Signed-off-by: Ralf Lici <[email protected]>
> > ---
> > drivers/net/ovpn/socket.c | 38 +++++++++++++++++++++---------------
> > --
> > drivers/net/ovpn/tcp.c | 1 +
> > drivers/net/ovpn/udp.c | 1 +
> > 3 files changed, 23 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/net/ovpn/socket.c b/drivers/net/ovpn/socket.c
> > index 9750871ab65c..053b8abe5619 100644
> > --- a/drivers/net/ovpn/socket.c
> > +++ b/drivers/net/ovpn/socket.c
> > @@ -200,6 +200,22 @@ struct ovpn_socket *ovpn_socket_new(struct
> > socket *sock, struct ovpn_peer *peer)
> > ovpn_sock->sk = sk;
> > kref_init(&ovpn_sock->refcount);
> >
> > + /* TCP sockets are per-peer, therefore they are linked to
> > their unique
> > + * peer
> > + */
> > + if (sk->sk_protocol == IPPROTO_TCP) {
> > + INIT_WORK(&ovpn_sock->tcp_tx_work,
> > ovpn_tcp_tx_work);
> > + ovpn_sock->peer = peer;
> > + ovpn_peer_hold(peer);
> > + } else if (sk->sk_protocol == IPPROTO_UDP) {
> > + /* in UDP we only link the ovpn instance since the
> > socket is
> > + * shared among multiple peers
> > + */
> > + ovpn_sock->ovpn = peer->ovpn;
> > + netdev_hold(peer->ovpn->dev, &ovpn_sock-
> > >dev_tracker,
> > + GFP_KERNEL);
> > + }
> > +
>
> Can you add a few words in the commit message to describe why this
> needs to be moved?
Of course, I'll specify why this was moved.
> > /* the newly created ovpn_socket is holding reference to
> > sk,
> > * therefore we increase its refcounter.
> > *
> > @@ -212,29 +228,17 @@ struct ovpn_socket *ovpn_socket_new(struct
> > socket *sock, struct ovpn_peer *peer)
> >
> > ret = ovpn_socket_attach(ovpn_sock, sock, peer);
> > if (ret < 0) {
> > + if (sk->sk_protocol == IPPROTO_TCP)
> > + ovpn_peer_put(peer);
> > + else if (sk->sk_protocol == IPPROTO_UDP)
> > + netdev_put(peer->ovpn->dev, &ovpn_sock-
> > >dev_tracker);
> > +
> > sock_put(sk);
> > kfree(ovpn_sock);
> > ovpn_sock = ERR_PTR(ret);
> > goto sock_release;
> > }
> >
> > - /* TCP sockets are per-peer, therefore they are linked to
> > their unique
> > - * peer
> > - */
> > - if (sk->sk_protocol == IPPROTO_TCP) {
> > - INIT_WORK(&ovpn_sock->tcp_tx_work,
> > ovpn_tcp_tx_work);
> > - ovpn_sock->peer = peer;
> > - ovpn_peer_hold(peer);
> > - } else if (sk->sk_protocol == IPPROTO_UDP) {
> > - /* in UDP we only link the ovpn instance since the
> > socket is
> > - * shared among multiple peers
> > - */
> > - ovpn_sock->ovpn = peer->ovpn;
> > - netdev_hold(peer->ovpn->dev, &ovpn_sock-
> > >dev_tracker,
> > - GFP_KERNEL);
> > - }
> > -
> > - rcu_assign_sk_user_data(sk, ovpn_sock);
> > sock_release:
> > release_sock(sk);
> > return ovpn_sock;
> > diff --git a/drivers/net/ovpn/tcp.c b/drivers/net/ovpn/tcp.c
> > index 0d7f30360d87..e078f9b39122 100644
> > --- a/drivers/net/ovpn/tcp.c
> > +++ b/drivers/net/ovpn/tcp.c
> > @@ -487,6 +487,7 @@ int ovpn_tcp_socket_attach(struct ovpn_socket
> > *ovpn_sock,
> > /* make sure no pre-existing encapsulation handler exists
> > */
> > if (ovpn_sock->sk->sk_user_data)
> > return -EBUSY;
> > + rcu_assign_sk_user_data(ovpn_sock->sk, ovpn_sock);
>
> And reset sk_user_data to NULL in the error cases that can happen
> later in ovpn_tcp_socket_attach?
Right, will fix.
>
--
Ralf Lici
Mandelbit Srl
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
