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 <[email protected]> > > > > --- > > > > 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? -- Ralf Lici Mandelbit Srl _______________________________________________ Openvpn-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openvpn-devel
