@@ -365,7 +365,27 @@ netdev_tx_t ovpn_net_xmit(struct sk_buff *skb, struct net_device *dev)
/* verify IP header size in network packet */
proto = ovpn_ip_check_protocol(skb);
if (unlikely(!proto || skb->protocol != proto))
- goto drop;
+ goto drop_no_peer;
+
+ /* retrieve peer serving the destination IP of this packet */
+ peer = ovpn_peer_get_by_dst(ovpn, skb);
+ if (unlikely(!peer)) {
+ switch (skb->protocol) {
+ case htons(ETH_P_IP):
+ net_dbg_ratelimited("%s: no peer to send data to dst=%pI4\n",
+ netdev_name(ovpn->dev),
+ &ip_hdr(skb)->daddr);
+ break;
+ case htons(ETH_P_IPV6):
+ net_dbg_ratelimited("%s: no peer to send data to dst=%pI6c\n",
+ netdev_name(ovpn->dev),
+ &ipv6_hdr(skb)->daddr);
+ break;
+ }
+ goto drop_no_peer;
+ }
+ /* dst was needed for peer selection - it can now be dropped */
+ skb_dst_drop(skb);
if (skb_is_gso(skb)) {
segments = skb_gso_segment(skb, 0);
@@ -396,34 +416,24 @@ netdev_tx_t ovpn_net_xmit(struct sk_buff *skb, struct net_device *dev)
__skb_queue_tail(&skb_list, curr);
}
- skb_list.prev->next = NULL;
- /* retrieve peer serving the destination IP of this packet */
- peer = ovpn_peer_get_by_dst(ovpn, skb);
- if (unlikely(!peer)) {
- switch (skb->protocol) {
- case htons(ETH_P_IP):
- net_dbg_ratelimited("%s: no peer to send data to dst=%pI4\n",
- netdev_name(ovpn->dev),
- &ip_hdr(skb)->daddr);
- break;
- case htons(ETH_P_IPV6):
- net_dbg_ratelimited("%s: no peer to send data to dst=%pI6c\n",
- netdev_name(ovpn->dev),
- &ipv6_hdr(skb)->daddr);
- break;
- }
- goto drop;
+ /* no segments survived: don't jump to 'drop' because we already
+ * incremented the counter for each failure in the loop
+ */
+ if (unlikely(skb_queue_empty(&skb_list))) {
+ ovpn_peer_put(peer);
+ return NETDEV_TX_OK;
}
- /* dst was needed for peer selection - it can now be dropped */
- skb_dst_drop(skb);
+ skb_list.prev->next = NULL;
- ovpn_peer_stats_increment_tx(&peer->vpn_stats, skb->len);
+ ovpn_peer_stats_increment_tx(&peer->vpn_stats, skb_list.next->len);
ovpn_send(ovpn, skb_list.next, peer);
return NETDEV_TX_OK;
drop:
+ ovpn_peer_put(peer);
+drop_no_peer:
dev_dstats_tx_dropped(ovpn->dev);
skb_tx_error(skb);
kfree_skb_list(skb);
When building the skb_list in ovpn_net_xmit, skb_share_check will free the original skb if it is shared. The current implementation continues to use the stale skb pointer for subsequent operations: - peer lookup, - skb_dst_drop (even though all segments produced by skb_gso_segment will have a dst attached), - ovpn_peer_stats_increment_tx. Fix this by moving the peer lookup and skb_dst_drop before segmentation so that the original skb is still valid when used. Return early if all segments fail skb_share_check and the list ends up empty. Also switch ovpn_peer_stats_increment_tx to use skb_list.next; the next patch fixes the stats logic. Fixes: 08857b5ec5d9 ("ovpn: implement basic TX path (UDP)") Signed-off-by: Ralf Lici <ralf@mandelbit.com> --- Changes since v2: - moved peer selection and skb_dst_drop before skb_gso_segment - added goto to drop the peer reference on error - stopped using 'skb' after building 'skb_list' and switched to skb_list.next for the stats update Changes since v1: - this is a new patch that replaces the previous "ovpn: use sk_buff_head properly in ovpn_net_xmit" drivers/net/ovpn/io.c | 52 ++++++++++++++++++++++++++----------------- 1 file changed, 31 insertions(+), 21 deletions(-)