2024-05-15, 14:54:49 +0200, Antonio Quartulli wrote:

On 15/05/2024 12:19, Sabrina Dubroca wrote:

quoted

2024-05-15, 00:11:28 +0200, Antonio Quartulli wrote:

quoted

On 14/05/2024 10:58, Sabrina Dubroca wrote:

quoted

quoted

quoted

The UDP code differentiates "socket already owned by this interface"
from "already taken by other user". That doesn't apply to TCP?

This makes me wonder: how safe it is to interpret the user data as an object
of type ovpn_socket?

When we find the user data already assigned, we don't know what was really
stored in there, right?
Technically this socket could have gone through another module which
assigned its own state.

Therefore I think that what UDP does [ dereferencing ((struct ovpn_socket
*)user_data)->ovpn ] is probably not safe. Would you agree?

Hmmm, yeah, I think you're right. If you checked encap_type ==
UDP_ENCAP_OVPNINUDP before (sk_prot for TCP), then you'd know it's
really your data. Basically call ovpn_from_udp_sock during attach if
you want to check something beyond EBUSY.

right. Maybe we can leave with simply reporting EBUSY and be done with it,
without adding extra checks and what not.

I don't know. What was the reason for the EALREADY handling in udp.c
and the corresponding refcount increase in ovpn_socket_new?

it's just me that likes to be verbose when doing error reporting.

With the "already owned by this interface" message? Sure, I get that.

But eventually the exact error is ignored and we release the reference. From
netlink.c:

342                 peer->sock = ovpn_socket_new(sock, peer);
343                 if (IS_ERR(peer->sock)) {
344                         sockfd_put(sock);
345                         peer->sock = NULL;
346                         ret = -ENOTSOCK;

so no added value in distinguishing the two cases.

But ovpn_socket_new currently turns EALREADY into a valid result, so
we won't go through the error hanadling here. That's the part I'm
unclear about.

-- 
Sabrina