| Message ID | 20240627130843.21042-18-antonio@openvpn.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Introducing OpenVPN Data Channel Offload | expand |
2024-06-27, 15:08:35 +0200, Antonio Quartulli wrote: > +static const unsigned char ovpn_keepalive_message[] = { > + 0x2a, 0x18, 0x7b, 0xf3, 0x64, 0x1e, 0xb4, 0xcb, > + 0x07, 0xed, 0x2d, 0x0a, 0x98, 0x1f, 0xc7, 0x48 > +}; > + > +/** > + * ovpn_is_keepalive - check if skb contains a keepalive message > + * @skb: packet to check > + * > + * Assumes that the first byte of skb->data is defined. > + * > + * Return: true if skb contains a keepalive or false otherwise > + */ > +static bool ovpn_is_keepalive(struct sk_buff *skb) > +{ > + if (*skb->data != OVPN_KEEPALIVE_FIRST_BYTE) You could use ovpn_keepalive_message[0], and then you wouldn't need this extra constant. > + return false; > + > + if (!pskb_may_pull(skb, sizeof(ovpn_keepalive_message))) > + return false; > + > + return !memcmp(skb->data, ovpn_keepalive_message, > + sizeof(ovpn_keepalive_message)); Is a packet that contains some extra bytes after the exact keepalive considered a valid keepalive, or does it need to be the correct length? > +} > + > /* Called after decrypt to write the IP packet to the device. > * This method is expected to manage/free the skb. > */ > @@ -91,6 +116,9 @@ void ovpn_decrypt_post(struct sk_buff *skb, int ret) > goto drop; > } > > + /* note event of authenticated packet received for keepalive */ > + ovpn_peer_keepalive_recv_reset(peer); > + > /* point to encapsulated IP packet */ > __skb_pull(skb, ovpn_skb_cb(skb)->payload_offset); > > @@ -107,6 +135,12 @@ void ovpn_decrypt_post(struct sk_buff *skb, int ret) > goto drop; > } > > + if (ovpn_is_keepalive(skb)) { > + netdev_dbg(peer->ovpn->dev, > + "ping received from peer %u\n", peer->id); That should probably be _ratelimited, but it seems we don't have _ratelimited variants for the netdev_* helpers. > +/** > + * ovpn_xmit_special - encrypt and transmit an out-of-band message to peer > + * @peer: peer to send the message to > + * @data: message content > + * @len: message length > + * > + * Assumes that caller holds a reference to peer > + */ > +static void ovpn_xmit_special(struct ovpn_peer *peer, const void *data, > + const unsigned int len) > +{ > + struct ovpn_struct *ovpn; > + struct sk_buff *skb; > + > + ovpn = peer->ovpn; > + if (unlikely(!ovpn)) > + return; > + > + skb = alloc_skb(256 + len, GFP_ATOMIC); Where is that 256 coming from? > + if (unlikely(!skb)) > + return; Failure to send a keepalive should probably have a counter, to help users troubleshoot why their connection dropped. (can be done later unless someone insists) > + skb_reserve(skb, 128); And that 128? > + skb->priority = TC_PRIO_BESTEFFORT; > + memcpy(__skb_put(skb, len), data, len); nit: that's __skb_put_data > + /* increase reference counter when passing peer to sending queue */ > + if (!ovpn_peer_hold(peer)) { > + netdev_dbg(ovpn->dev, "%s: cannot hold peer reference for sending special packet\n", > + __func__); > + kfree_skb(skb); > + return; > + } > + > + ovpn_send(ovpn, skb, peer); > +} > + > +/** > + * ovpn_keepalive_xmit - send keepalive message to peer > + * @peer: the peer to send the message to > + */ > +void ovpn_keepalive_xmit(struct ovpn_peer *peer) > +{ > + ovpn_xmit_special(peer, ovpn_keepalive_message, > + sizeof(ovpn_keepalive_message)); > +} I don't see other users of ovpn_xmit_special in this series, if you don't have more planned in the future you could drop the extra function. > +/** > + * ovpn_peer_expire - timer task for incoming keepialive timeout typo: s/keepialive/keepalive/ > +/** > + * ovpn_peer_keepalive_set - configure keepalive values for peer > + * @peer: the peer to configure > + * @interval: outgoing keepalive interval > + * @timeout: incoming keepalive timeout > + */ > +void ovpn_peer_keepalive_set(struct ovpn_peer *peer, u32 interval, u32 timeout) > +{ > + u32 delta; > + > + netdev_dbg(peer->ovpn->dev, > + "%s: scheduling keepalive for peer %u: interval=%u timeout=%u\n", > + __func__, peer->id, interval, timeout); > + > + peer->keepalive_interval = interval; > + if (interval > 0) { > + delta = msecs_to_jiffies(interval * MSEC_PER_SEC); > + mod_timer(&peer->keepalive_xmit, jiffies + delta); Maybe something to consider in the future: this could be resetting a timer that was just about to go off to a somewhat distant time in the future. Not sure the peer will be happy about that (and not consider it a timeout). > + } else { > + timer_delete(&peer->keepalive_xmit); > + } > + > + peer->keepalive_timeout = timeout; > + if (timeout) { pedantic nit: inconsistent style with the "interval > 0" test just above > + delta = msecs_to_jiffies(timeout * MSEC_PER_SEC); > + mod_timer(&peer->keepalive_recv, jiffies + delta); > + } else { > + timer_delete(&peer->keepalive_recv); > + } > +} > + [...] > +/** > + * ovpn_peer_keepalive_recv_reset - reset keepalive timeout > + * @peer: peer for which the timeout should be reset > + * > + * To be invoked upon reception of an authenticated packet from peer in order > + * to report valid activity and thus reset the keepalive timeout > + */ > +static inline void ovpn_peer_keepalive_recv_reset(struct ovpn_peer *peer) > +{ > + u32 delta = msecs_to_jiffies(peer->keepalive_timeout * MSEC_PER_SEC); > + > + if (unlikely(!delta)) > + return; > + > + mod_timer(&peer->keepalive_recv, jiffies + delta); This (and ovpn_peer_keepalive_xmit_reset) is going to be called for each packet. I wonder how well the timer subsystem deals with one timer getting updated possibly thousands of time per second.
Hi, On 15/07/2024 16:44, Sabrina Dubroca wrote: > 2024-06-27, 15:08:35 +0200, Antonio Quartulli wrote: >> +static const unsigned char ovpn_keepalive_message[] = { >> + 0x2a, 0x18, 0x7b, 0xf3, 0x64, 0x1e, 0xb4, 0xcb, >> + 0x07, 0xed, 0x2d, 0x0a, 0x98, 0x1f, 0xc7, 0x48 >> +}; >> + >> +/** >> + * ovpn_is_keepalive - check if skb contains a keepalive message >> + * @skb: packet to check >> + * >> + * Assumes that the first byte of skb->data is defined. >> + * >> + * Return: true if skb contains a keepalive or false otherwise >> + */ >> +static bool ovpn_is_keepalive(struct sk_buff *skb) >> +{ >> + if (*skb->data != OVPN_KEEPALIVE_FIRST_BYTE) > > You could use ovpn_keepalive_message[0], and then you wouldn't need > this extra constant. Indeed, shame on me, will do as suggested > >> + return false; >> + >> + if (!pskb_may_pull(skb, sizeof(ovpn_keepalive_message))) >> + return false; >> + >> + return !memcmp(skb->data, ovpn_keepalive_message, >> + sizeof(ovpn_keepalive_message)); > > Is a packet that contains some extra bytes after the exact keepalive > considered a valid keepalive, or does it need to be the correct > length? I checked the userspace code and it assumes the length of the received keepalive message to be the same as the ovpn_keepalive_message array. So no extra byte expected, otherwise the message is not considered a keepalive anymore. This means I must add an extra check before the memcmp to make sure there is no extra data. Good catch, thanks! > >> +} >> + >> /* Called after decrypt to write the IP packet to the device. >> * This method is expected to manage/free the skb. >> */ >> @@ -91,6 +116,9 @@ void ovpn_decrypt_post(struct sk_buff *skb, int ret) >> goto drop; >> } >> >> + /* note event of authenticated packet received for keepalive */ >> + ovpn_peer_keepalive_recv_reset(peer); >> + >> /* point to encapsulated IP packet */ >> __skb_pull(skb, ovpn_skb_cb(skb)->payload_offset); >> >> @@ -107,6 +135,12 @@ void ovpn_decrypt_post(struct sk_buff *skb, int ret) >> goto drop; >> } >> >> + if (ovpn_is_keepalive(skb)) { >> + netdev_dbg(peer->ovpn->dev, >> + "ping received from peer %u\n", peer->id); > > That should probably be _ratelimited, but it seems we don't have > _ratelimited variants for the netdev_* helpers. Right. I have used the net_*_ratelimited() variants when needed. Too bad we don't have those. > > > >> +/** >> + * ovpn_xmit_special - encrypt and transmit an out-of-band message to peer >> + * @peer: peer to send the message to >> + * @data: message content >> + * @len: message length >> + * >> + * Assumes that caller holds a reference to peer >> + */ >> +static void ovpn_xmit_special(struct ovpn_peer *peer, const void *data, >> + const unsigned int len) >> +{ >> + struct ovpn_struct *ovpn; >> + struct sk_buff *skb; >> + >> + ovpn = peer->ovpn; >> + if (unlikely(!ovpn)) >> + return; >> + >> + skb = alloc_skb(256 + len, GFP_ATOMIC); > > Where is that 256 coming from? "Reasonable number" which should be enough[tm] to hold the entire packet. > >> + if (unlikely(!skb)) >> + return; > > Failure to send a keepalive should probably have a counter, to help > users troubleshoot why their connection dropped. > (can be done later unless someone insists) This will be part of a more sophisticated error counting that I will introduce later on. > > >> + skb_reserve(skb, 128); > > And that 128? same "logic" as 256. > >> + skb->priority = TC_PRIO_BESTEFFORT; >> + memcpy(__skb_put(skb, len), data, len); > > nit: that's __skb_put_data oh cool, thanks! > >> + /* increase reference counter when passing peer to sending queue */ >> + if (!ovpn_peer_hold(peer)) { >> + netdev_dbg(ovpn->dev, "%s: cannot hold peer reference for sending special packet\n", >> + __func__); >> + kfree_skb(skb); >> + return; >> + } >> + >> + ovpn_send(ovpn, skb, peer); >> +} >> + >> +/** >> + * ovpn_keepalive_xmit - send keepalive message to peer >> + * @peer: the peer to send the message to >> + */ >> +void ovpn_keepalive_xmit(struct ovpn_peer *peer) >> +{ >> + ovpn_xmit_special(peer, ovpn_keepalive_message, >> + sizeof(ovpn_keepalive_message)); >> +} > > I don't see other users of ovpn_xmit_special in this series, if you > don't have more planned in the future you could drop the extra function. initially there were plans, but I have always fought back any idea about adding more unnecessary logic to the kernel side. So for now there is nothing planned. I'll remove the extra wrapper. > > >> +/** >> + * ovpn_peer_expire - timer task for incoming keepialive timeout > > typo: s/keepialive/keepalive/ Thanks > > > >> +/** >> + * ovpn_peer_keepalive_set - configure keepalive values for peer >> + * @peer: the peer to configure >> + * @interval: outgoing keepalive interval >> + * @timeout: incoming keepalive timeout >> + */ >> +void ovpn_peer_keepalive_set(struct ovpn_peer *peer, u32 interval, u32 timeout) >> +{ >> + u32 delta; >> + >> + netdev_dbg(peer->ovpn->dev, >> + "%s: scheduling keepalive for peer %u: interval=%u timeout=%u\n", >> + __func__, peer->id, interval, timeout); >> + >> + peer->keepalive_interval = interval; >> + if (interval > 0) { >> + delta = msecs_to_jiffies(interval * MSEC_PER_SEC); >> + mod_timer(&peer->keepalive_xmit, jiffies + delta); > > Maybe something to consider in the future: this could be resetting a > timer that was just about to go off to a somewhat distant time in the > future. Not sure the peer will be happy about that (and not consider > it a timeout). Normally this timer is only set upon connection, or maybe upon some future parameter exchange. In both cases we can assume the connection is alive, so this case should not scare us. But thanks for pointing it out > >> + } else { >> + timer_delete(&peer->keepalive_xmit); >> + } >> + >> + peer->keepalive_timeout = timeout; >> + if (timeout) { > > pedantic nit: inconsistent style with the "interval > 0" test just > above ACK, will make them uniform. > >> + delta = msecs_to_jiffies(timeout * MSEC_PER_SEC); >> + mod_timer(&peer->keepalive_recv, jiffies + delta); >> + } else { >> + timer_delete(&peer->keepalive_recv); >> + } >> +} >> + > > [...] >> +/** >> + * ovpn_peer_keepalive_recv_reset - reset keepalive timeout >> + * @peer: peer for which the timeout should be reset >> + * >> + * To be invoked upon reception of an authenticated packet from peer in order >> + * to report valid activity and thus reset the keepalive timeout >> + */ >> +static inline void ovpn_peer_keepalive_recv_reset(struct ovpn_peer *peer) >> +{ >> + u32 delta = msecs_to_jiffies(peer->keepalive_timeout * MSEC_PER_SEC); >> + >> + if (unlikely(!delta)) >> + return; >> + >> + mod_timer(&peer->keepalive_recv, jiffies + delta); > > This (and ovpn_peer_keepalive_xmit_reset) is going to be called for > each packet. I wonder how well the timer subsystem deals with one > timer getting updated possibly thousands of time per second. > May it even introduce some performance penalty? Maybe we should get rid of the timer object and introduce a periodic (1s) worker which checks some last_recv timestamp on every known peer? What do you think? Thanks!
Hi, On Wed, Jul 17, 2024 at 8:29 AM Antonio Quartulli <antonio@openvpn.net> wrote: > > On 15/07/2024 16:44, Sabrina Dubroca wrote: > > This (and ovpn_peer_keepalive_xmit_reset) is going to be called for > > each packet. I wonder how well the timer subsystem deals with one > > timer getting updated possibly thousands of time per second. > > > > May it even introduce some performance penalty? > > Maybe we should get rid of the timer object and introduce a periodic > (1s) worker which checks some last_recv timestamp on every known peer? > What do you think? FWIW In NATT keepalive for IPsec the first RFC was using timers and the workqueue approach was suggested [1], and later implemented [2]. The code is currently in net-next. Eyal. [1] https://linux-ipsec.org/pipermail/devel/2023/000283.html [2] https://patchwork.kernel.org/project/netdevbpf/patch/20240528032914.2551267-1-eyal.birger@gmail.com/
2024-07-17, 17:30:17 +0200, Antonio Quartulli wrote: > Hi, > > On 15/07/2024 16:44, Sabrina Dubroca wrote: > > 2024-06-27, 15:08:35 +0200, Antonio Quartulli wrote: > > > +/** > > > + * ovpn_xmit_special - encrypt and transmit an out-of-band message to peer > > > + * @peer: peer to send the message to > > > + * @data: message content > > > + * @len: message length > > > + * > > > + * Assumes that caller holds a reference to peer > > > + */ > > > +static void ovpn_xmit_special(struct ovpn_peer *peer, const void *data, > > > + const unsigned int len) > > > +{ > > > + struct ovpn_struct *ovpn; > > > + struct sk_buff *skb; > > > + > > > + ovpn = peer->ovpn; > > > + if (unlikely(!ovpn)) > > > + return; > > > + > > > + skb = alloc_skb(256 + len, GFP_ATOMIC); > > > > Where is that 256 coming from? > > "Reasonable number" which should be enough[tm] to hold the entire packet. Ok, let's go with that for now, unless someone else wants you to change it. > > > + if (unlikely(!skb)) > > > + return; > > > > Failure to send a keepalive should probably have a counter, to help > > users troubleshoot why their connection dropped. > > (can be done later unless someone insists) > > This will be part of a more sophisticated error counting that I will > introduce later on. Cool, thanks. > > > +/** > > > + * ovpn_peer_keepalive_set - configure keepalive values for peer > > > + * @peer: the peer to configure > > > + * @interval: outgoing keepalive interval > > > + * @timeout: incoming keepalive timeout > > > + */ > > > +void ovpn_peer_keepalive_set(struct ovpn_peer *peer, u32 interval, u32 timeout) > > > +{ > > > + u32 delta; > > > + > > > + netdev_dbg(peer->ovpn->dev, > > > + "%s: scheduling keepalive for peer %u: interval=%u timeout=%u\n", > > > + __func__, peer->id, interval, timeout); > > > + > > > + peer->keepalive_interval = interval; > > > + if (interval > 0) { > > > + delta = msecs_to_jiffies(interval * MSEC_PER_SEC); > > > + mod_timer(&peer->keepalive_xmit, jiffies + delta); > > > > Maybe something to consider in the future: this could be resetting a > > timer that was just about to go off to a somewhat distant time in the > > future. Not sure the peer will be happy about that (and not consider > > it a timeout). > > Normally this timer is only set upon connection, or maybe upon some future > parameter exchange. In both cases we can assume the connection is alive, so > this case should not scare us. > > But thanks for pointing it out Ok, I was thinking about updates while the connection is fully established. If it's only done during setup, it shouldn't be a problem. > > > +/** > > > + * ovpn_peer_keepalive_recv_reset - reset keepalive timeout > > > + * @peer: peer for which the timeout should be reset > > > + * > > > + * To be invoked upon reception of an authenticated packet from peer in order > > > + * to report valid activity and thus reset the keepalive timeout > > > + */ > > > +static inline void ovpn_peer_keepalive_recv_reset(struct ovpn_peer *peer) > > > +{ > > > + u32 delta = msecs_to_jiffies(peer->keepalive_timeout * MSEC_PER_SEC); > > > + > > > + if (unlikely(!delta)) > > > + return; > > > + > > > + mod_timer(&peer->keepalive_recv, jiffies + delta); > > > > This (and ovpn_peer_keepalive_xmit_reset) is going to be called for > > each packet. I wonder how well the timer subsystem deals with one > > timer getting updated possibly thousands of time per second. > > > > May it even introduce some performance penalty? That's what I was worried about, yes. I asked Paolo, he suggested checking that we're actually doing any change to the timer: if (new_timeout_time != old_timeout_time) mod_timer(...) This would reduce the update frequency to one per jiffy, which should be acceptable. > Maybe we should get rid of the timer object and introduce a periodic (1s) > worker which checks some last_recv timestamp on every known peer? > What do you think? That should work, or the workqueue like Eyal is saying.
> > > + if (ovpn_is_keepalive(skb)) { > > > + netdev_dbg(peer->ovpn->dev, > > > + "ping received from peer %u\n", peer->id); > > > > That should probably be _ratelimited, but it seems we don't have > > _ratelimited variants for the netdev_* helpers. > > Right. > I have used the net_*_ratelimited() variants when needed. > Too bad we don't have those. If you think netdev_dbg_ratelimited() would be useful, i don't see why you cannot add it. I just did an search and found something interesting in the history: https://lore.kernel.org/all/20190809002941.15341-1-liuhangbin@gmail.com/T/#u Maybe limit it to netdev_dbg_ratelimited() to avoid the potential abuse DaveM was worried about. Andrew
On 18/07/2024 04:01, Andrew Lunn wrote: >>>> + if (ovpn_is_keepalive(skb)) { >>>> + netdev_dbg(peer->ovpn->dev, >>>> + "ping received from peer %u\n", peer->id); >>> >>> That should probably be _ratelimited, but it seems we don't have >>> _ratelimited variants for the netdev_* helpers. >> >> Right. >> I have used the net_*_ratelimited() variants when needed. >> Too bad we don't have those. > > If you think netdev_dbg_ratelimited() would be useful, i don't see why > you cannot add it. > > I just did an search and found something interesting in the history: > > https://lore.kernel.org/all/20190809002941.15341-1-liuhangbin@gmail.com/T/#u > > Maybe limit it to netdev_dbg_ratelimited() to avoid the potential > abuse DaveM was worried about. I see what Dave says however... ...along the packet processing routine there are several messages (some are err or warn or info) which require ratelimiting. Otherwise you end up with a gazilion log entries in case of a long lasting issue. Right now I am using net_dbg/warn/err/info_ratelimited(), therefore not having a netdev counterpart is not really helping with Dave's argument. I can try to take on this mission after this patchset is in and see what Dave/Jakub think about it. Regards,
Hi Eyal, thanks a lot for chiming in. On 17/07/2024 18:19, Eyal Birger wrote: > Hi, > > On Wed, Jul 17, 2024 at 8:29 AM Antonio Quartulli <antonio@openvpn.net> wrote: >> >> On 15/07/2024 16:44, Sabrina Dubroca wrote: > >>> This (and ovpn_peer_keepalive_xmit_reset) is going to be called for >>> each packet. I wonder how well the timer subsystem deals with one >>> timer getting updated possibly thousands of time per second. >>> >> >> May it even introduce some performance penalty? >> >> Maybe we should get rid of the timer object and introduce a periodic >> (1s) worker which checks some last_recv timestamp on every known peer? >> What do you think? > > FWIW In NATT keepalive for IPsec the first RFC was using timers and > the workqueue > approach was suggested [1], and later implemented [2]. > The code is currently in net-next. > > Eyal. > > [1] https://linux-ipsec.org/pipermail/devel/2023/000283.html > [2] https://patchwork.kernel.org/project/netdevbpf/patch/20240528032914.2551267-1-eyal.birger@gmail.com/ Thanks for these pointers. Basically this is pretty much what I had in mind, but rather than running the worker every second, the next run is scheduled based on the closest expiration time. I like it! Since this worker has no other housekeeping to do, I will go with this approach as well. Thanks! Regards,
On 17/07/2024 22:40, Sabrina Dubroca wrote: >>>> +/** >>>> + * ovpn_peer_keepalive_recv_reset - reset keepalive timeout >>>> + * @peer: peer for which the timeout should be reset >>>> + * >>>> + * To be invoked upon reception of an authenticated packet from peer in order >>>> + * to report valid activity and thus reset the keepalive timeout >>>> + */ >>>> +static inline void ovpn_peer_keepalive_recv_reset(struct ovpn_peer *peer) >>>> +{ >>>> + u32 delta = msecs_to_jiffies(peer->keepalive_timeout * MSEC_PER_SEC); >>>> + >>>> + if (unlikely(!delta)) >>>> + return; >>>> + >>>> + mod_timer(&peer->keepalive_recv, jiffies + delta); >>> >>> This (and ovpn_peer_keepalive_xmit_reset) is going to be called for >>> each packet. I wonder how well the timer subsystem deals with one >>> timer getting updated possibly thousands of time per second. >>> >> >> May it even introduce some performance penalty? > > That's what I was worried about, yes. > > I asked Paolo, he suggested checking that we're actually doing any > change to the timer: > > if (new_timeout_time != old_timeout_time) > mod_timer(...) > > This would reduce the update frequency to one per jiffy, which should > be acceptable. > >> Maybe we should get rid of the timer object and introduce a periodic (1s) >> worker which checks some last_recv timestamp on every known peer? >> What do you think? > > That should work, or the workqueue like Eyal is saying. I will go with Eyal's approach. This way we also eliminate any timer related operation from the fast path. Cheers!
On Thu, Jul 18, 2024 at 09:46:00AM +0200, Antonio Quartulli wrote: > On 18/07/2024 04:01, Andrew Lunn wrote: > > > > > + if (ovpn_is_keepalive(skb)) { > > > > > + netdev_dbg(peer->ovpn->dev, > > > > > + "ping received from peer %u\n", peer->id); > > > > > > > > That should probably be _ratelimited, but it seems we don't have > > > > _ratelimited variants for the netdev_* helpers. > > > > > > Right. > > > I have used the net_*_ratelimited() variants when needed. > > > Too bad we don't have those. > > > > If you think netdev_dbg_ratelimited() would be useful, i don't see why > > you cannot add it. > > > > I just did an search and found something interesting in the history: > > > > https://lore.kernel.org/all/20190809002941.15341-1-liuhangbin@gmail.com/T/#u > > > > Maybe limit it to netdev_dbg_ratelimited() to avoid the potential > > abuse DaveM was worried about. > > I see what Dave says however... > > ...along the packet processing routine there are several messages (some are > err or warn or info) which require ratelimiting. > Otherwise you end up with a gazilion log entries in case of a long lasting > issue. > > Right now I am using net_dbg/warn/err/info_ratelimited(), therefore not > having a netdev counterpart is not really helping with Dave's argument. Yes, i think Dave' argument is weak because these alternatives exist. Maybe they did not at the time? I suspect he was using it as a way to force fixing the real issue. A driver should not be issues lots of err or info messages. Protocol errors are part of normal behaviour, just increment a counter and keep going. Peer devices disappearing is normal behaviour, count it and keep going. _err is generally reserved for something fatal happened, and there is no recovery, other than unload the kernel module and reload it. Andrew
On 19/07/2024 05:31, Andrew Lunn wrote: > On Thu, Jul 18, 2024 at 09:46:00AM +0200, Antonio Quartulli wrote: >> On 18/07/2024 04:01, Andrew Lunn wrote: >>>>>> + if (ovpn_is_keepalive(skb)) { >>>>>> + netdev_dbg(peer->ovpn->dev, >>>>>> + "ping received from peer %u\n", peer->id); >>>>> >>>>> That should probably be _ratelimited, but it seems we don't have >>>>> _ratelimited variants for the netdev_* helpers. >>>> >>>> Right. >>>> I have used the net_*_ratelimited() variants when needed. >>>> Too bad we don't have those. >>> >>> If you think netdev_dbg_ratelimited() would be useful, i don't see why >>> you cannot add it. >>> >>> I just did an search and found something interesting in the history: >>> >>> https://lore.kernel.org/all/20190809002941.15341-1-liuhangbin@gmail.com/T/#u >>> >>> Maybe limit it to netdev_dbg_ratelimited() to avoid the potential >>> abuse DaveM was worried about. >> >> I see what Dave says however... >> >> ...along the packet processing routine there are several messages (some are >> err or warn or info) which require ratelimiting. >> Otherwise you end up with a gazilion log entries in case of a long lasting >> issue. >> >> Right now I am using net_dbg/warn/err/info_ratelimited(), therefore not >> having a netdev counterpart is not really helping with Dave's argument. > > Yes, i think Dave' argument is weak because these alternatives > exist. Maybe they did not at the time? They did and it's exactly what Hangbin was introducing: https://lore.kernel.org/all/20190801090347.8258-1-liuhangbin@gmail.com/ before being suggested by Joe to implement the new macros: https://lore.kernel.org/all/209f7fe62e2a79cd8c02b104b8e3babdd16bff30.camel@perches.com/ > > I suspect he was using it as a way to force fixing the real issue. A > driver should not be issues lots of err or info messages. Protocol > errors are part of normal behaviour, just increment a counter and keep > going. Peer devices disappearing is normal behaviour, count it and > keep going. _err is generally reserved for something fatal happened, > and there is no recovery, other than unload the kernel module and > reload it. Yeah, it looks like it and David pushed them to rethink the error handling rather than making the error less painful. But I can still netdev_dbg_ratelimited useful for debugging/development. Anyway, I will add this to my todo and see if this specific variant can be accepted. Regards,
diff --git a/drivers/net/ovpn/io.c b/drivers/net/ovpn/io.c index 764b3df996bc..dedc4d4a1c72 100644 --- a/drivers/net/ovpn/io.c +++ b/drivers/net/ovpn/io.c @@ -25,6 +25,31 @@ #include "udp.h" #include "skb.h" +static const unsigned char ovpn_keepalive_message[] = { + 0x2a, 0x18, 0x7b, 0xf3, 0x64, 0x1e, 0xb4, 0xcb, + 0x07, 0xed, 0x2d, 0x0a, 0x98, 0x1f, 0xc7, 0x48 +}; + +/** + * ovpn_is_keepalive - check if skb contains a keepalive message + * @skb: packet to check + * + * Assumes that the first byte of skb->data is defined. + * + * Return: true if skb contains a keepalive or false otherwise + */ +static bool ovpn_is_keepalive(struct sk_buff *skb) +{ + if (*skb->data != OVPN_KEEPALIVE_FIRST_BYTE) + return false; + + if (!pskb_may_pull(skb, sizeof(ovpn_keepalive_message))) + return false; + + return !memcmp(skb->data, ovpn_keepalive_message, + sizeof(ovpn_keepalive_message)); +} + /* Called after decrypt to write the IP packet to the device. * This method is expected to manage/free the skb. */ @@ -91,6 +116,9 @@ void ovpn_decrypt_post(struct sk_buff *skb, int ret) goto drop; } + /* note event of authenticated packet received for keepalive */ + ovpn_peer_keepalive_recv_reset(peer); + /* point to encapsulated IP packet */ __skb_pull(skb, ovpn_skb_cb(skb)->payload_offset); @@ -107,6 +135,12 @@ void ovpn_decrypt_post(struct sk_buff *skb, int ret) goto drop; } + if (ovpn_is_keepalive(skb)) { + netdev_dbg(peer->ovpn->dev, + "ping received from peer %u\n", peer->id); + goto drop; + } + net_info_ratelimited("%s: unsupported protocol received from peer %u\n", peer->ovpn->dev->name, peer->id); goto drop; @@ -203,6 +237,7 @@ void ovpn_encrypt_post(struct sk_buff *skb, int ret) /* no transport configured yet */ goto err; } + ovpn_peer_keepalive_xmit_reset(peer); /* skb passed down the stack - don't free it */ skb = NULL; err: @@ -345,3 +380,50 @@ netdev_tx_t ovpn_net_xmit(struct sk_buff *skb, struct net_device *dev) kfree_skb_list(skb); return NET_XMIT_DROP; } + +/** + * ovpn_xmit_special - encrypt and transmit an out-of-band message to peer + * @peer: peer to send the message to + * @data: message content + * @len: message length + * + * Assumes that caller holds a reference to peer + */ +static void ovpn_xmit_special(struct ovpn_peer *peer, const void *data, + const unsigned int len) +{ + struct ovpn_struct *ovpn; + struct sk_buff *skb; + + ovpn = peer->ovpn; + if (unlikely(!ovpn)) + return; + + skb = alloc_skb(256 + len, GFP_ATOMIC); + if (unlikely(!skb)) + return; + + skb_reserve(skb, 128); + skb->priority = TC_PRIO_BESTEFFORT; + memcpy(__skb_put(skb, len), data, len); + + /* increase reference counter when passing peer to sending queue */ + if (!ovpn_peer_hold(peer)) { + netdev_dbg(ovpn->dev, "%s: cannot hold peer reference for sending special packet\n", + __func__); + kfree_skb(skb); + return; + } + + ovpn_send(ovpn, skb, peer); +} + +/** + * ovpn_keepalive_xmit - send keepalive message to peer + * @peer: the peer to send the message to + */ +void ovpn_keepalive_xmit(struct ovpn_peer *peer) +{ + ovpn_xmit_special(peer, ovpn_keepalive_message, + sizeof(ovpn_keepalive_message)); +} diff --git a/drivers/net/ovpn/io.h b/drivers/net/ovpn/io.h index ad741552d742..63fcd576ddaf 100644 --- a/drivers/net/ovpn/io.h +++ b/drivers/net/ovpn/io.h @@ -13,6 +13,7 @@ netdev_tx_t ovpn_net_xmit(struct sk_buff *skb, struct net_device *dev); void ovpn_recv(struct ovpn_peer *peer, struct sk_buff *skb); +void ovpn_keepalive_xmit(struct ovpn_peer *peer); void ovpn_encrypt_post(struct sk_buff *skb, int ret); void ovpn_decrypt_post(struct sk_buff *skb, int ret); diff --git a/drivers/net/ovpn/peer.c b/drivers/net/ovpn/peer.c index 8c23268db916..289fe3f84ed4 100644 --- a/drivers/net/ovpn/peer.c +++ b/drivers/net/ovpn/peer.c @@ -22,6 +22,63 @@ #include "peer.h" #include "socket.h" +/** + * ovpn_peer_ping - timer task for sending periodic keepalive + * @t: timer object that triggered the task + */ +static void ovpn_peer_ping(struct timer_list *t) +{ + struct ovpn_peer *peer = from_timer(peer, t, keepalive_xmit); + + netdev_dbg(peer->ovpn->dev, "%s: sending ping to peer %u\n", __func__, + peer->id); + ovpn_keepalive_xmit(peer); +} + +/** + * ovpn_peer_expire - timer task for incoming keepialive timeout + * @t: the timer that triggered the task + */ +static void ovpn_peer_expire(struct timer_list *t) +{ + struct ovpn_peer *peer = from_timer(peer, t, keepalive_recv); + + netdev_dbg(peer->ovpn->dev, "%s: peer %u expired\n", __func__, + peer->id); + ovpn_peer_del(peer, OVPN_DEL_PEER_REASON_EXPIRED); +} + +/** + * ovpn_peer_keepalive_set - configure keepalive values for peer + * @peer: the peer to configure + * @interval: outgoing keepalive interval + * @timeout: incoming keepalive timeout + */ +void ovpn_peer_keepalive_set(struct ovpn_peer *peer, u32 interval, u32 timeout) +{ + u32 delta; + + netdev_dbg(peer->ovpn->dev, + "%s: scheduling keepalive for peer %u: interval=%u timeout=%u\n", + __func__, peer->id, interval, timeout); + + peer->keepalive_interval = interval; + if (interval > 0) { + delta = msecs_to_jiffies(interval * MSEC_PER_SEC); + mod_timer(&peer->keepalive_xmit, jiffies + delta); + } else { + timer_delete(&peer->keepalive_xmit); + } + + peer->keepalive_timeout = timeout; + if (timeout) { + delta = msecs_to_jiffies(timeout * MSEC_PER_SEC); + mod_timer(&peer->keepalive_recv, jiffies + delta); + } else { + timer_delete(&peer->keepalive_recv); + } +} + /** * ovpn_peer_new - allocate and initialize a new peer object * @ovpn: the openvpn instance inside which the peer should be created @@ -63,9 +120,22 @@ struct ovpn_peer *ovpn_peer_new(struct ovpn_struct *ovpn, u32 id) netdev_hold(ovpn->dev, NULL, GFP_KERNEL); + timer_setup(&peer->keepalive_xmit, ovpn_peer_ping, 0); + timer_setup(&peer->keepalive_recv, ovpn_peer_expire, 0); + return peer; } +/** + * ovpn_peer_timer_delete_all - killall keepalive timers + * @peer: peer for which timers should be killed + */ +static void ovpn_peer_timer_delete_all(struct ovpn_peer *peer) +{ + timer_shutdown_sync(&peer->keepalive_xmit); + timer_shutdown_sync(&peer->keepalive_recv); +} + /** * ovpn_peer_release - release peer private members * @peer: the peer to release @@ -77,6 +147,8 @@ static void ovpn_peer_release(struct ovpn_peer *peer) ovpn_crypto_state_release(&peer->crypto); ovpn_bind_reset(peer, NULL); + ovpn_peer_timer_delete_all(peer); + dst_cache_destroy(&peer->dst_cache); } diff --git a/drivers/net/ovpn/peer.h b/drivers/net/ovpn/peer.h index 6e92e09a3504..fd7e9d57b38a 100644 --- a/drivers/net/ovpn/peer.h +++ b/drivers/net/ovpn/peer.h @@ -50,6 +50,10 @@ * @crypto: the crypto configuration (ciphers, keys, etc..) * @dst_cache: cache for dst_entry used to send to peer * @bind: remote peer binding + * @keepalive_xmit: timer used to send the next keepalive + * @keepalive_interval: seconds after which a new keepalive should be sent + * @keepalive_recv: timer used to check for received keepalives + * @keepalive_timeout: seconds after which an inactive peer is considered dead * @halt: true if ovpn_peer_mark_delete was called * @vpn_stats: per-peer in-VPN TX/RX stays * @link_stats: per-peer link/transport TX/RX stats @@ -98,6 +102,10 @@ struct ovpn_peer { struct ovpn_crypto_state crypto; struct dst_cache dst_cache; struct ovpn_bind __rcu *bind; + struct timer_list keepalive_xmit; + unsigned long keepalive_interval; + struct timer_list keepalive_recv; + unsigned long keepalive_timeout; bool halt; struct ovpn_peer_stats vpn_stats; struct ovpn_peer_stats link_stats; @@ -144,4 +152,41 @@ struct ovpn_peer *ovpn_peer_get_by_dst(struct ovpn_struct *ovpn, bool ovpn_peer_check_by_src(struct ovpn_struct *ovpn, struct sk_buff *skb, struct ovpn_peer *peer); +/** + * ovpn_peer_keepalive_recv_reset - reset keepalive timeout + * @peer: peer for which the timeout should be reset + * + * To be invoked upon reception of an authenticated packet from peer in order + * to report valid activity and thus reset the keepalive timeout + */ +static inline void ovpn_peer_keepalive_recv_reset(struct ovpn_peer *peer) +{ + u32 delta = msecs_to_jiffies(peer->keepalive_timeout * MSEC_PER_SEC); + + if (unlikely(!delta)) + return; + + mod_timer(&peer->keepalive_recv, jiffies + delta); +} + +/** + * ovpn_peer_keepalive_xmit_reset - reset keepalive sending timer + * @peer: peer for which the timer should be reset + * + * To be invoked upon sending of an authenticated packet to peer in order + * to report valid outgoing activity and thus reset the keepalive sending + * timer + */ +static inline void ovpn_peer_keepalive_xmit_reset(struct ovpn_peer *peer) +{ + u32 delta = msecs_to_jiffies(peer->keepalive_interval * MSEC_PER_SEC); + + if (unlikely(!delta)) + return; + + mod_timer(&peer->keepalive_xmit, jiffies + delta); +} + +void ovpn_peer_keepalive_set(struct ovpn_peer *peer, u32 interval, u32 timeout); + #endif /* _NET_OVPN_OVPNPEER_H_ */
OpenVPN supports configuring a periodic keepalive "ping" message to allow the remote endpoint detect link failures. This change implements the ping sending and timer expiring logic. Signed-off-by: Antonio Quartulli <antonio@openvpn.net> --- drivers/net/ovpn/io.c | 82 +++++++++++++++++++++++++++++++++++++++++ drivers/net/ovpn/io.h | 1 + drivers/net/ovpn/peer.c | 72 ++++++++++++++++++++++++++++++++++++ drivers/net/ovpn/peer.h | 45 ++++++++++++++++++++++ 4 files changed, 200 insertions(+)