| Message ID | 20260119131400.424161-3-ralf@mandelbit.com |
|---|---|
| State | New |
| Headers | show |
| Series | [Openvpn-devel,ovpn,net,1/3] ovpn: set sk_user_data before overriding callbacks | expand |
You're submitting this for "net" but this looks more like a clean up that should be sent to net-next. I don't know how Antonio will handle this, but for netdev submissions this distinction matters, see https://docs.kernel.org/process/maintainer-netdev.html 2026-01-19, 14:14:00 +0100, Ralf Lici wrote: > The current code builds an sk_buff_head after GSO segmentation but then > treats it as a raw skb list: accessing elements via skb_list.next and > breaking the list circularity by setting skb_list.prev->next to NULL. > > Clean this up by changing ovpn_send to take an sk_buff_head parameter > and use standard sk_buff_head APIs. Introduce ovpn_send_one helper to > wrap single skbs in an sk_buff_head for ovpn_xmit_special. Not a strong objection, but I'm not so convinced by this clean up. Using a sk_buff_head is maybe a little bit nicer conceptually, but it doesn't result in a code improvement, especially since you have to add ovpn_send_one(). A few small comments on the implementation: > Signed-off-by: Ralf Lici <ralf@mandelbit.com> > --- > drivers/net/ovpn/io.c | 74 +++++++++++++++++++++++++++---------------- > 1 file changed, 46 insertions(+), 28 deletions(-) > > diff --git a/drivers/net/ovpn/io.c b/drivers/net/ovpn/io.c > index c59501344d97..249751cd630b 100644 > --- a/drivers/net/ovpn/io.c > +++ b/drivers/net/ovpn/io.c > @@ -329,8 +329,8 @@ static bool ovpn_encrypt_one(struct ovpn_peer *peer, struct sk_buff *skb) > return true; > } > > -/* send skb to connected peer, if any */ > -static void ovpn_send(struct ovpn_priv *ovpn, struct sk_buff *skb, > +/* send skb_list to connected peer, if any */ > +static void ovpn_send(struct ovpn_priv *ovpn, struct sk_buff_head *skb_list, > struct ovpn_peer *peer) > { > struct sk_buff *curr, *next; > @@ -338,7 +338,8 @@ static void ovpn_send(struct ovpn_priv *ovpn, struct sk_buff *skb, > /* this might be a GSO-segmented skb list: process each skb > * independently > */ > - skb_list_walk_safe(skb, curr, next) { > + skb_queue_walk_safe(skb_list, curr, next) { > + __skb_unlink(curr, skb_list); Why is this needed now but wasn't before? > if (unlikely(!ovpn_encrypt_one(peer, curr))) { > dev_dstats_tx_dropped(ovpn->dev); > kfree_skb(curr); > @@ -368,6 +369,26 @@ netdev_tx_t ovpn_net_xmit(struct sk_buff *skb, struct net_device *dev) > if (unlikely(!proto || skb->protocol != proto)) > goto drop; > > + /* 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; > + } > + /* dst was needed for peer selection - it can now be dropped */ > + skb_dst_drop(skb); Moving this code looks like a separate clean up? Or is this needed for the conversion to sk_buff_head?
On Wed, 2026-01-21 at 17:53 +0100, Sabrina Dubroca wrote: > You're submitting this for "net" but this looks more like a clean up > that should be sent to net-next. I don't know how Antonio will handle > this, but for netdev submissions this distinction matters, see > https://docs.kernel.org/process/maintainer-netdev.html I think you are right. I initially considered this a fix, but I agree it's more appropriate for net-next. I'll leave it to Antonio to place it in the right PR. > 2026-01-19, 14:14:00 +0100, Ralf Lici wrote: > > The current code builds an sk_buff_head after GSO segmentation but > > then > > treats it as a raw skb list: accessing elements via skb_list.next > > and > > breaking the list circularity by setting skb_list.prev->next to > > NULL. > > > > Clean this up by changing ovpn_send to take an sk_buff_head > > parameter > > and use standard sk_buff_head APIs. Introduce ovpn_send_one helper > > to > > wrap single skbs in an sk_buff_head for ovpn_xmit_special. > > Not a strong objection, but I'm not so convinced by this clean > up. Using a sk_buff_head is maybe a little bit nicer conceptually, but > it doesn't result in a code improvement, especially since you have to > add ovpn_send_one(). > I agree this adds a small helper, but what motivated this change is that the current usage of sk_buff_head in ovpn_net_xmit looks a bit like a hack or an API violation. In the current code, we build an sk_buff_head but then manually break the ring (skb_list.prev->next = NULL) to treat it as a raw singly linked chain. This relies on implementation details rather than the API. Even though we do not (and likely will not) need the circularity, staying within the API feels safer and more maintainable. Also, the overhead added by ovpn_send_one should be negligible, since it is only used for keepalives. Hopefully that makes sense. > A few small comments on the implementation: > > > Signed-off-by: Ralf Lici <ralf@mandelbit.com> > > --- > > drivers/net/ovpn/io.c | 74 +++++++++++++++++++++++++++------------- > > --- > > 1 file changed, 46 insertions(+), 28 deletions(-) > > > > diff --git a/drivers/net/ovpn/io.c b/drivers/net/ovpn/io.c > > index c59501344d97..249751cd630b 100644 > > --- a/drivers/net/ovpn/io.c > > +++ b/drivers/net/ovpn/io.c > > @@ -329,8 +329,8 @@ static bool ovpn_encrypt_one(struct ovpn_peer > > *peer, struct sk_buff *skb) > > return true; > > } > > > > -/* send skb to connected peer, if any */ > > -static void ovpn_send(struct ovpn_priv *ovpn, struct sk_buff *skb, > > +/* send skb_list to connected peer, if any */ > > +static void ovpn_send(struct ovpn_priv *ovpn, struct sk_buff_head > > *skb_list, > > struct ovpn_peer *peer) > > { > > struct sk_buff *curr, *next; > > @@ -338,7 +338,8 @@ static void ovpn_send(struct ovpn_priv *ovpn, > > struct sk_buff *skb, > > /* this might be a GSO-segmented skb list: process each skb > > * independently > > */ > > - skb_list_walk_safe(skb, curr, next) { > > + skb_queue_walk_safe(skb_list, curr, next) { > > + __skb_unlink(curr, skb_list); > > Why is this needed now but wasn't before? Since the new code aims to use skb_list as a proper circular doubly linked list and to comply with the sk_buff_head API, the unlinking is necessary to maintain a valid skb_list while processing each of its elements. This also seems to be a common pattern throughout the kernel. It is true, though, that after ovpn_send we do not use skb_list anymore, so this is not strictly necessary. Let me know if you think I should remove it. > > if (unlikely(!ovpn_encrypt_one(peer, curr))) { > > dev_dstats_tx_dropped(ovpn->dev); > > kfree_skb(curr); > > @@ -368,6 +369,26 @@ netdev_tx_t ovpn_net_xmit(struct sk_buff *skb, > > struct net_device *dev) > > if (unlikely(!proto || skb->protocol != proto)) > > goto drop; > > > > + /* 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; > > + } > > + /* dst was needed for peer selection - it can now be > > dropped */ > > + skb_dst_drop(skb); > > Moving this code looks like a separate clean up? Or is this needed for > the conversion to sk_buff_head? While this could technically be a separate cleanup, I included it here because it reorganizes the function to accommodate the new lifecycle of the skb variable. Once we commit to populating the sk_buff_head and nullifying the original skb, the peer lookup logically has to happen earlier. Grouping these changes keeps the function coherent throughout the refactoring. Otherwise we would have had to pass skb_list.next to ovpn_peer_get_by_dst, which would have defeated the purpose of the patch. That said, I recognize it could also be viewed as a standalone patch. If you prefer a 2-step cleanup for better atomicity, I am happy to split this into two commits: one for the peer retrieval move and another for the sk_buff_head API transition. Please let me know your preference.
2026-01-22, 13:48:11 +0100, Ralf Lici wrote: > On Wed, 2026-01-21 at 17:53 +0100, Sabrina Dubroca wrote: > > You're submitting this for "net" but this looks more like a clean up > > that should be sent to net-next. I don't know how Antonio will handle > > this, but for netdev submissions this distinction matters, see > > https://docs.kernel.org/process/maintainer-netdev.html > > I think you are right. I initially considered this a fix, but I agree > it's more appropriate for net-next. I'll leave it to Antonio to place it > in the right PR. > > > 2026-01-19, 14:14:00 +0100, Ralf Lici wrote: > > > The current code builds an sk_buff_head after GSO segmentation but > > > then > > > treats it as a raw skb list: accessing elements via skb_list.next > > > and > > > breaking the list circularity by setting skb_list.prev->next to > > > NULL. > > > > > > Clean this up by changing ovpn_send to take an sk_buff_head > > > parameter > > > and use standard sk_buff_head APIs. Introduce ovpn_send_one helper > > > to > > > wrap single skbs in an sk_buff_head for ovpn_xmit_special. > > > > Not a strong objection, but I'm not so convinced by this clean > > up. Using a sk_buff_head is maybe a little bit nicer conceptually, but > > it doesn't result in a code improvement, especially since you have to > > add ovpn_send_one(). > > > > I agree this adds a small helper, but what motivated this change is that > the current usage of sk_buff_head in ovpn_net_xmit looks a bit like a > hack or an API violation. In the current code, we build an sk_buff_head > but then manually break the ring (skb_list.prev->next = NULL) to treat > it as a raw singly linked chain. I'd say we're building a skb list and happen to use an sk_buff_head to make it easier. So I'm not really convinced by the benefit of this patch. > This relies on implementation details > rather than the API. Even though we do not (and likely will not) need > the circularity, staying within the API feels safer and more > maintainable. > > Also, the overhead added by ovpn_send_one should be negligible, since it > is only used for keepalives. Sure, my comment was more about code simplicity than runtime overhead. > Hopefully that makes sense. > > > > A few small comments on the implementation: > > > > > Signed-off-by: Ralf Lici <ralf@mandelbit.com> > > > --- > > > drivers/net/ovpn/io.c | 74 +++++++++++++++++++++++++++------------- > > > --- > > > 1 file changed, 46 insertions(+), 28 deletions(-) > > > > > > diff --git a/drivers/net/ovpn/io.c b/drivers/net/ovpn/io.c > > > index c59501344d97..249751cd630b 100644 > > > --- a/drivers/net/ovpn/io.c > > > +++ b/drivers/net/ovpn/io.c > > > @@ -329,8 +329,8 @@ static bool ovpn_encrypt_one(struct ovpn_peer > > > *peer, struct sk_buff *skb) > > > return true; > > > } > > > > > > -/* send skb to connected peer, if any */ > > > -static void ovpn_send(struct ovpn_priv *ovpn, struct sk_buff *skb, > > > +/* send skb_list to connected peer, if any */ > > > +static void ovpn_send(struct ovpn_priv *ovpn, struct sk_buff_head > > > *skb_list, > > > struct ovpn_peer *peer) > > > { > > > struct sk_buff *curr, *next; > > > @@ -338,7 +338,8 @@ static void ovpn_send(struct ovpn_priv *ovpn, > > > struct sk_buff *skb, > > > /* this might be a GSO-segmented skb list: process each skb > > > * independently > > > */ > > > - skb_list_walk_safe(skb, curr, next) { > > > + skb_queue_walk_safe(skb_list, curr, next) { > > > + __skb_unlink(curr, skb_list); > > > > Why is this needed now but wasn't before? > > Since the new code aims to use skb_list as a proper circular doubly > linked list and to comply with the sk_buff_head API, the unlinking is > necessary to maintain a valid skb_list while processing each of its > elements. This also seems to be a common pattern throughout the kernel. > > It is true, though, that after ovpn_send we do not use skb_list anymore, > so this is not strictly necessary. Let me know if you think I should > remove it. If the aim is to properly use the sk_buff_head API, then I guess it makes sense. The skb_mark_not_on_list() call in ovpn_encrypt_post is not needed anymore. > > > if (unlikely(!ovpn_encrypt_one(peer, curr))) { > > > dev_dstats_tx_dropped(ovpn->dev); > > > kfree_skb(curr); > > > @@ -368,6 +369,26 @@ netdev_tx_t ovpn_net_xmit(struct sk_buff *skb, > > > struct net_device *dev) > > > if (unlikely(!proto || skb->protocol != proto)) > > > goto drop; > > > > > > + /* 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; > > > + } > > > + /* dst was needed for peer selection - it can now be > > > dropped */ > > > + skb_dst_drop(skb); > > > > Moving this code looks like a separate clean up? Or is this needed for > > the conversion to sk_buff_head? > > While this could technically be a separate cleanup, I included it here > because it reorganizes the function to accommodate the new lifecycle of > the skb variable. Once we commit to populating the sk_buff_head and > nullifying the original skb, the peer lookup logically has to happen > earlier. Grouping these changes keeps the function coherent throughout > the refactoring. Otherwise we would have had to pass skb_list.next to > ovpn_peer_get_by_dst, which would have defeated the purpose of the > patch. > > That said, I recognize it could also be viewed as a standalone patch. If > you prefer a 2-step cleanup for better atomicity, I am happy to split > this into two commits: one for the peer retrieval move and another for > the sk_buff_head API transition. Please let me know your preference. Maybe. Either way, I think you're missing a call to ovpn_peer_put() in the error cases that happen after fetching the peer. Before this patch there was no possible failure once we'd grabbed the peer. One more semi-nitpicky comment: > + if (unlikely(skb_queue_empty(&skb_list))) > goto drop; Is this needed? If the queue is empty we've already counted one drop for each segment during the list-to-sk_buff_head loop, and ovpn_send should handle an empty queue without problem?
On Mon, 2026-01-26 at 16:48 +0100, Sabrina Dubroca wrote: > 2026-01-22, 13:48:11 +0100, Ralf Lici wrote: > > On Wed, 2026-01-21 at 17:53 +0100, Sabrina Dubroca wrote: > > > You're submitting this for "net" but this looks more like a clean > > > up > > > that should be sent to net-next. I don't know how Antonio will > > > handle > > > this, but for netdev submissions this distinction matters, see > > > https://docs.kernel.org/process/maintainer-netdev.html > > > > I think you are right. I initially considered this a fix, but I > > agree > > it's more appropriate for net-next. I'll leave it to Antonio to > > place it > > in the right PR. > > > > > 2026-01-19, 14:14:00 +0100, Ralf Lici wrote: > > > > The current code builds an sk_buff_head after GSO segmentation > > > > but > > > > then > > > > treats it as a raw skb list: accessing elements via > > > > skb_list.next > > > > and > > > > breaking the list circularity by setting skb_list.prev->next to > > > > NULL. > > > > > > > > Clean this up by changing ovpn_send to take an sk_buff_head > > > > parameter > > > > and use standard sk_buff_head APIs. Introduce ovpn_send_one > > > > helper > > > > to > > > > wrap single skbs in an sk_buff_head for ovpn_xmit_special. > > > > > > Not a strong objection, but I'm not so convinced by this clean > > > up. Using a sk_buff_head is maybe a little bit nicer conceptually, > > > but > > > it doesn't result in a code improvement, especially since you have > > > to > > > add ovpn_send_one(). > > > > > > > I agree this adds a small helper, but what motivated this change is > > that > > the current usage of sk_buff_head in ovpn_net_xmit looks a bit like > > a > > hack or an API violation. In the current code, we build an > > sk_buff_head > > but then manually break the ring (skb_list.prev->next = NULL) to > > treat > > it as a raw singly linked chain. > > I'd say we're building a skb list and happen to use an sk_buff_head to > make it easier. So I'm not really convinced by the benefit of this > patch. I understand your point regarding the simplicity and convenience of the original implementation. After discussing this further with Antonio, we have decided to drop this change and maintain the current structure and sequence of operations in ovpn_net_xmit. Anyway, since there's currently a risk of UAF when using the 'skb' variable after calling skb_share_check, I'll add a patch to address that specific issue maintaining the current pattern. Thank you for the feedback. > > > This relies on implementation details > > rather than the API. Even though we do not (and likely will not) > > need > > the circularity, staying within the API feels safer and more > > maintainable. > > > > Also, the overhead added by ovpn_send_one should be negligible, > > since it > > is only used for keepalives. > > Sure, my comment was more about code simplicity than runtime overhead. > > > Hopefully that makes sense. > > > > > > > A few small comments on the implementation: > > > > > > > Signed-off-by: Ralf Lici <ralf@mandelbit.com> > > > > --- > > > > drivers/net/ovpn/io.c | 74 +++++++++++++++++++++++++++--------- > > > > ---- > > > > --- > > > > 1 file changed, 46 insertions(+), 28 deletions(-) > > > > > > > > diff --git a/drivers/net/ovpn/io.c b/drivers/net/ovpn/io.c > > > > index c59501344d97..249751cd630b 100644 > > > > --- a/drivers/net/ovpn/io.c > > > > +++ b/drivers/net/ovpn/io.c > > > > @@ -329,8 +329,8 @@ static bool ovpn_encrypt_one(struct > > > > ovpn_peer > > > > *peer, struct sk_buff *skb) > > > > return true; > > > > } > > > > > > > > -/* send skb to connected peer, if any */ > > > > -static void ovpn_send(struct ovpn_priv *ovpn, struct sk_buff > > > > *skb, > > > > +/* send skb_list to connected peer, if any */ > > > > +static void ovpn_send(struct ovpn_priv *ovpn, struct > > > > sk_buff_head > > > > *skb_list, > > > > struct ovpn_peer *peer) > > > > { > > > > struct sk_buff *curr, *next; > > > > @@ -338,7 +338,8 @@ static void ovpn_send(struct ovpn_priv > > > > *ovpn, > > > > struct sk_buff *skb, > > > > /* this might be a GSO-segmented skb list: process each > > > > skb > > > > * independently > > > > */ > > > > - skb_list_walk_safe(skb, curr, next) { > > > > + skb_queue_walk_safe(skb_list, curr, next) { > > > > + __skb_unlink(curr, skb_list); > > > > > > Why is this needed now but wasn't before? > > > > Since the new code aims to use skb_list as a proper circular doubly > > linked list and to comply with the sk_buff_head API, the unlinking > > is > > necessary to maintain a valid skb_list while processing each of its > > elements. This also seems to be a common pattern throughout the > > kernel. > > > > It is true, though, that after ovpn_send we do not use skb_list > > anymore, > > so this is not strictly necessary. Let me know if you think I should > > remove it. > > If the aim is to properly use the sk_buff_head API, then I guess it > makes sense. > > The skb_mark_not_on_list() call in ovpn_encrypt_post is not needed > anymore. > > > > > if (unlikely(!ovpn_encrypt_one(peer, curr))) { > > > > dev_dstats_tx_dropped(ovpn->dev); > > > > kfree_skb(curr); > > > > @@ -368,6 +369,26 @@ netdev_tx_t ovpn_net_xmit(struct sk_buff > > > > *skb, > > > > struct net_device *dev) > > > > if (unlikely(!proto || skb->protocol != proto)) > > > > goto drop; > > > > > > > > + /* 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; > > > > + } > > > > + /* dst was needed for peer selection - it can now be > > > > dropped */ > > > > + skb_dst_drop(skb); > > > > > > Moving this code looks like a separate clean up? Or is this needed > > > for > > > the conversion to sk_buff_head? > > > > While this could technically be a separate cleanup, I included it > > here > > because it reorganizes the function to accommodate the new lifecycle > > of > > the skb variable. Once we commit to populating the sk_buff_head and > > nullifying the original skb, the peer lookup logically has to happen > > earlier. Grouping these changes keeps the function coherent > > throughout > > the refactoring. Otherwise we would have had to pass skb_list.next > > to > > ovpn_peer_get_by_dst, which would have defeated the purpose of the > > patch. > > > > That said, I recognize it could also be viewed as a standalone > > patch. If > > you prefer a 2-step cleanup for better atomicity, I am happy to > > split > > this into two commits: one for the peer retrieval move and another > > for > > the sk_buff_head API transition. Please let me know your preference. > > Maybe. Either way, I think you're missing a call to ovpn_peer_put() in > the error cases that happen after fetching the peer. Before this patch > there was no possible failure once we'd grabbed the peer. > > > One more semi-nitpicky comment: > > > + if (unlikely(skb_queue_empty(&skb_list))) > > goto drop; > > Is this needed? If the queue is empty we've already counted one drop > for each segment during the list-to-sk_buff_head loop, and ovpn_send > should handle an empty queue without problem?
diff --git a/drivers/net/ovpn/io.c b/drivers/net/ovpn/io.c index c59501344d97..249751cd630b 100644 --- a/drivers/net/ovpn/io.c +++ b/drivers/net/ovpn/io.c @@ -329,8 +329,8 @@ static bool ovpn_encrypt_one(struct ovpn_peer *peer, struct sk_buff *skb) return true; } -/* send skb to connected peer, if any */ -static void ovpn_send(struct ovpn_priv *ovpn, struct sk_buff *skb, +/* send skb_list to connected peer, if any */ +static void ovpn_send(struct ovpn_priv *ovpn, struct sk_buff_head *skb_list, struct ovpn_peer *peer) { struct sk_buff *curr, *next; @@ -338,7 +338,8 @@ static void ovpn_send(struct ovpn_priv *ovpn, struct sk_buff *skb, /* this might be a GSO-segmented skb list: process each skb * independently */ - skb_list_walk_safe(skb, curr, next) { + skb_queue_walk_safe(skb_list, curr, next) { + __skb_unlink(curr, skb_list); if (unlikely(!ovpn_encrypt_one(peer, curr))) { dev_dstats_tx_dropped(ovpn->dev); kfree_skb(curr); @@ -368,6 +369,26 @@ netdev_tx_t ovpn_net_xmit(struct sk_buff *skb, struct net_device *dev) if (unlikely(!proto || skb->protocol != proto)) goto drop; + /* 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; + } + /* 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); if (IS_ERR(segments)) { @@ -381,8 +402,9 @@ netdev_tx_t ovpn_net_xmit(struct sk_buff *skb, struct net_device *dev) skb = segments; } - /* from this moment on, "skb" might be a list */ - + /* "skb" might be a raw list of skbs, transform it into a proper + * sk_buff_head list + */ __skb_queue_head_init(&skb_list); skb_list_walk_safe(skb, curr, next) { skb_mark_not_on_list(curr); @@ -399,40 +421,36 @@ netdev_tx_t ovpn_net_xmit(struct sk_buff *skb, struct net_device *dev) tx_bytes += curr->len; __skb_queue_tail(&skb_list, curr); } - skb_list.prev->next = NULL; + skb = 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; - } + if (unlikely(skb_queue_empty(&skb_list))) goto drop; - } - /* dst was needed for peer selection - it can now be dropped */ - skb_dst_drop(skb); ovpn_peer_stats_increment_tx(&peer->vpn_stats, tx_bytes); - ovpn_send(ovpn, skb_list.next, peer); + ovpn_send(ovpn, &skb_list, peer); return NETDEV_TX_OK; drop: dev_dstats_tx_dropped(ovpn->dev); - skb_tx_error(skb); - kfree_skb_list(skb); + if (skb) { + skb_tx_error(skb); + kfree_skb_list(skb); + } return NETDEV_TX_OK; } +/* wrap a single skb in a list in order to pass it to ovpn_send */ +static void ovpn_send_one(struct ovpn_priv *ovpn, struct sk_buff *skb, + struct ovpn_peer *peer) +{ + struct sk_buff_head list; + + __skb_queue_head_init(&list); + __skb_queue_tail(&list, skb); + ovpn_send(ovpn, &list, peer); +} + /** * ovpn_xmit_special - encrypt and transmit an out-of-band message to peer * @peer: peer to send the message to @@ -464,5 +482,5 @@ void ovpn_xmit_special(struct ovpn_peer *peer, const void *data, skb->priority = TC_PRIO_BESTEFFORT; __skb_put_data(skb, data, len); - ovpn_send(ovpn, skb, peer); + ovpn_send_one(ovpn, skb, peer); }
The current code builds an sk_buff_head after GSO segmentation but then treats it as a raw skb list: accessing elements via skb_list.next and breaking the list circularity by setting skb_list.prev->next to NULL. Clean this up by changing ovpn_send to take an sk_buff_head parameter and use standard sk_buff_head APIs. Introduce ovpn_send_one helper to wrap single skbs in an sk_buff_head for ovpn_xmit_special. Signed-off-by: Ralf Lici <ralf@mandelbit.com> --- drivers/net/ovpn/io.c | 74 +++++++++++++++++++++++++++---------------- 1 file changed, 46 insertions(+), 28 deletions(-)