Re: [PATCH net-next v24 18/23] ovpn: implement peer add/get/dump/delete via netlink

[Sabrina Dubroca <sd@queasysnail.net>]

2025-03-25, 00:15:48 +0100, Antonio Quartulli wrote:
> On 24/03/2025 11:48, Sabrina Dubroca wrote:
> > Hello Antonio,
> > 
> > A few questions wrt the API:
> > 
> > 2025-03-18, 02:40:53 +0100, Antonio Quartulli wrote:
> > > +static bool ovpn_nl_attr_sockaddr_remote(struct nlattr **attrs,
> > > +					 struct sockaddr_storage *ss)
> > > +{
> > > +	struct sockaddr_in6 *sin6;
> > > +	struct sockaddr_in *sin;
> > > +	struct in6_addr *in6;
> > > +	__be16 port = 0;
> > > +	__be32 *in;
> > > +
> > > +	ss->ss_family = AF_UNSPEC;
> > > +
> > > +	if (attrs[OVPN_A_PEER_REMOTE_PORT])
> > > +		port = nla_get_be16(attrs[OVPN_A_PEER_REMOTE_PORT]);
> > 
> > What's the expected behavior if REMOTE_PORT isn't provided? We'll send
> > packets do port 0 (which I'm guessing will get dropped on the other
> > side) until we get a message from the peer and float sets the correct
> > port/address?
> 
> I have never seen a packet going out with port 0 :)

It will if you hack into ovpn-cli to skip OVPN_A_PEER_REMOTE_PORT.
I don't know how networks/admins react to such packets.

> But being dropped is most likely what's going to happen.
> 
> I'd say this is not something that we expect the user to do:
> if the remote address if specified, the user should specify a non-zero port
> too.
> 
> We could add a check to ensure that a port is always specified if the remote
> address is there too, just to avoid the user to shoot himself in the foot.
> But we expect the user to pass an addr:port where the peer is listening to
> (and that can't be a 0 port).

If we expect that (even if a well-behaved userspace would never do
it), I have a preference for enforcing that expectation. Since there's
already a policy rejecting OVPN_A_PEER_REMOTE_PORT == 0, this would be
more consistent IMO.

An alternative would be to select a default (non-zero) port if none is
provided.

> > 
> > 
> > > +static int ovpn_nl_peer_modify(struct ovpn_peer *peer, struct genl_info *info,
> > > +			       struct nlattr **attrs)
> > > +{
> > [...]
> > > +	/* when setting the keepalive, both parameters have to be configured */
> > > +	if (attrs[OVPN_A_PEER_KEEPALIVE_INTERVAL] &&
> > > +	    attrs[OVPN_A_PEER_KEEPALIVE_TIMEOUT]) {
> > > +		interv = nla_get_u32(attrs[OVPN_A_PEER_KEEPALIVE_INTERVAL]);
> > > +		timeout = nla_get_u32(attrs[OVPN_A_PEER_KEEPALIVE_TIMEOUT]);
> > > +		ovpn_peer_keepalive_set(peer, interv, timeout);
> > 
> > Should we interpret OVPN_A_PEER_KEEPALIVE_INTERVAL = 0 &&
> > OVPN_A_PEER_KEEPALIVE_TIMEOUT == 0 as "disable keepalive/timeout" on
> > an active peer?  And maybe "one set to 0, the other set to some
> > non-zero value" as invalid?  Setting either value to 0 doesn't seem
> > very useful (timeout = 0 will probably kill the peer immediately, and
> > I suspect interval = 0 would be quite spammy).
> > 
> 
> Considering "0" as "disable keepalive" is the current intention.
> 
> In ovpn_peer_keepalive_work_single() you can see that if either one if 0, we
> just skip the peer:
> 
> 1217         /* we expect both timers to be configured at the same time,
> 1218          * therefore bail out if either is not set
> 1219          */
> 1220         if (!peer->keepalive_timeout || !peer->keepalive_interval) {
> 1221                 spin_unlock_bh(&peer->lock);
> 1222                 return 0;
> 1223         }
> 
> does it make sense?

Ah, true. Sorry, I forgot about that.  So after _NEW/_SET we'll run
the work once, and that peer will be ignored. And if there's no other
peer requiring keepalive, next_run will be 0 and we don't
reschedule. That's good, thanks.

-- 
Sabrina