[PATCH 0/8] reduce open coded skb->next access for gso segment walking

[PATCH 0/8] reduce open coded skb->next access for gso segment walking

[Jason A. Donenfeld]

This patchset introduces the skb_list_walk_safe helper macro, in order
to add some sanity to the myrid ways drivers have of walking through gso
segments. The goal is to reduce future bugs commonly caused by open
coding these sorts of things, and to in the future make it easier to
swap out the underlying list representation.

This first patch series addresses the easy uses of drivers iterating
over the returned list of skb_gso_segments, for drivers that live in
drivers/net/*. There are still other use cases to tackle later for
net/*, and after these low-hanging fruits are taken care of, I imagine
there are more subtle cases of gso segment walking that isn't just a
direct return value from skb_gso_segments, and eventually this will have
to be tackled. This series is the first in that direction.

Jason A. Donenfeld (8):
  net: introduce skb_list_walk_safe for skb segment walking
  net: tap: use skb_list_walk_safe helper for gso segments
  net: r8152: use skb_list_walk_safe helper for gso segments
  net: tg3: use skb_list_walk_safe helper for gso segments
  net: sunvnet: use skb_list_walk_safe helper for gso segments
  net: sfc: use skb_list_walk_safe helper for gso segments
  net: myri10ge: use skb_list_walk_safe helper for gso segments
  net: iwlwifi: use skb_list_walk_safe helper for gso segments

 drivers/net/ethernet/broadcom/tg3.c              | 12 +++++-------
 drivers/net/ethernet/myricom/myri10ge/myri10ge.c |  8 +++-----
 drivers/net/ethernet/sfc/tx.c                    |  7 ++-----
 drivers/net/ethernet/sun/sunvnet_common.c        |  9 +++------
 drivers/net/tap.c                                | 14 ++++++--------
 drivers/net/usb/r8152.c                          | 12 +++++-------
 drivers/net/wireguard/device.h                   |  8 --------
 drivers/net/wireless/intel/iwlwifi/mvm/tx.c      |  9 ++-------
 include/linux/skbuff.h                           |  5 +++++
 9 files changed, 31 insertions(+), 53 deletions(-)

-- 
2.24.1

[PATCH 1/8] net: introduce skb_list_walk_safe for skb segment walking

[Jason A. Donenfeld]

As part of the continual effort to remove direct usage of skb->next and
skb->prev, this patch adds a helper for iterating through the
singly-linked variant of skb lists, which are used for lists of GSO
packet. The name "skb_list_..." has been chosen to match the existing
function, "kfree_skb_list, which also operates on these singly-linked
lists, and the "..._walk_safe" part is the same idiom as elsewhere in
the kernel.

This patch removes the helper from wireguard and puts it into
linux/skbuff.h, while making it a bit more robust for general usage. In
particular, parenthesis are added around the macro argument usage, and it
now accounts for trying to iterate through an already-null skb pointer,
which will simply run the iteration zero times. This latter enhancement
means it can be used to replace both do { ... } while and while (...)
open-coded idioms.

This should take care of these three possible usages, which match all
current methods of iterations.

skb_list_walk_safe(segs, skb, next) { ... }
skb_list_walk_safe(skb, skb, next) { ... }
skb_list_walk_safe(segs, skb, segs) { ... }

Gcc appears to generate efficient code for each of these.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 drivers/net/wireguard/device.h | 8 --------
 include/linux/skbuff.h         | 5 +++++
 2 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireguard/device.h b/drivers/net/wireguard/device.h
index c91f3051c5c7..b15a8be9d816 100644
--- a/drivers/net/wireguard/device.h
+++ b/drivers/net/wireguard/device.h
@@ -62,12 +62,4 @@ struct wg_device {
 int wg_device_init(void);
 void wg_device_uninit(void);
 
-/* Later after the dust settles, this can be moved into include/linux/skbuff.h,
- * where virtually all code that deals with GSO segs can benefit, around ~30
- * drivers as of writing.
- */
-#define skb_list_walk_safe(first, skb, next)                                   \
-	for (skb = first, next = skb->next; skb;                               \
-	     skb = next, next = skb ? skb->next : NULL)
-
 #endif /* _WG_DEVICE_H */
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index e9133bcf0544..64e5b1be9ff5 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1478,6 +1478,11 @@ static inline void skb_mark_not_on_list(struct sk_buff *skb)
 	skb->next = NULL;
 }
 
+/* Iterate through singly-linked GSO fragments of an skb. */
+#define skb_list_walk_safe(first, skb, next)                                   \
+	for ((skb) = (first), (next) = (skb) ? (skb)->next : NULL; (skb);      \
+	     (skb) = (next), (next) = (skb) ? (skb)->next : NULL)
+
 static inline void skb_list_del_init(struct sk_buff *skb)
 {
 	__list_del_entry(&skb->list);
-- 
2.24.1

[PATCH 2/8] net: tap: use skb_list_walk_safe helper for gso segments

[Jason A. Donenfeld]

This is a straight-forward conversion case for the new function, and
while we're at it, we can remove a null write to skb->next by replacing
it with skb_mark_not_on_list.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 drivers/net/tap.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/net/tap.c b/drivers/net/tap.c
index a6d63665ad03..1f4bdd94407a 100644
--- a/drivers/net/tap.c
+++ b/drivers/net/tap.c
@@ -341,6 +341,7 @@ rx_handler_result_t tap_handle_frame(struct sk_buff **pskb)
 		features |= tap->tap_features;
 	if (netif_needs_gso(skb, features)) {
 		struct sk_buff *segs = __skb_gso_segment(skb, features, false);
+		struct sk_buff *next;
 
 		if (IS_ERR(segs))
 			goto drop;
@@ -352,16 +353,13 @@ rx_handler_result_t tap_handle_frame(struct sk_buff **pskb)
 		}
 
 		consume_skb(skb);
-		while (segs) {
-			struct sk_buff *nskb = segs->next;
-
-			segs->next = NULL;
-			if (ptr_ring_produce(&q->ring, segs)) {
-				kfree_skb(segs);
-				kfree_skb_list(nskb);
+		skb_list_walk_safe(segs, skb, next) {
+			skb_mark_not_on_list(skb);
+			if (ptr_ring_produce(&q->ring, skb)) {
+				kfree_skb(skb);
+				kfree_skb_list(next);
 				break;
 			}
-			segs = nskb;
 		}
 	} else {
 		/* If we receive a partial checksum and the tap side
-- 
2.24.1

[PATCH 3/8] net: r8152: use skb_list_walk_safe helper for gso segments

[Jason A. Donenfeld]

This is a straight-forward conversion case for the new function, and
while we're at it, we can remove a null write to skb->next by replacing
it with skb_mark_not_on_list.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 drivers/net/usb/r8152.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 9ec1da429514..fe22a582373b 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -1897,8 +1897,8 @@ static void r8152_csum_workaround(struct r8152 *tp, struct sk_buff *skb,
 {
 	if (skb_shinfo(skb)->gso_size) {
 		netdev_features_t features = tp->netdev->features;
+		struct sk_buff *segs, *seg, *next;
 		struct sk_buff_head seg_list;
-		struct sk_buff *segs, *nskb;
 
 		features &= ~(NETIF_F_SG | NETIF_F_IPV6_CSUM | NETIF_F_TSO6);
 		segs = skb_gso_segment(skb, features);
@@ -1907,12 +1907,10 @@ static void r8152_csum_workaround(struct r8152 *tp, struct sk_buff *skb,
 
 		__skb_queue_head_init(&seg_list);
 
-		do {
-			nskb = segs;
-			segs = segs->next;
-			nskb->next = NULL;
-			__skb_queue_tail(&seg_list, nskb);
-		} while (segs);
+		skb_list_walk_safe(segs, seg, next) {
+			skb_mark_not_on_list(seg);
+			__skb_queue_tail(&seg_list, seg);
+		}
 
 		skb_queue_splice(&seg_list, list);
 		dev_kfree_skb(skb);
-- 
2.24.1

[PATCH 4/8] net: tg3: use skb_list_walk_safe helper for gso segments

[Jason A. Donenfeld]

This is a straight-forward conversion case for the new function, and
while we're at it, we can remove a null write to skb->next by replacing
it with skb_mark_not_on_list.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 drivers/net/ethernet/broadcom/tg3.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index 460b4992914a..88466255bf66 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -7874,8 +7874,8 @@ static netdev_tx_t tg3_start_xmit(struct sk_buff *, struct net_device *);
 static int tg3_tso_bug(struct tg3 *tp, struct tg3_napi *tnapi,
 		       struct netdev_queue *txq, struct sk_buff *skb)
 {
-	struct sk_buff *segs, *nskb;
 	u32 frag_cnt_est = skb_shinfo(skb)->gso_segs * 3;
+	struct sk_buff *segs, *seg, *next;
 
 	/* Estimate the number of fragments in the worst case */
 	if (unlikely(tg3_tx_avail(tnapi) <= frag_cnt_est)) {
@@ -7898,12 +7898,10 @@ static int tg3_tso_bug(struct tg3 *tp, struct tg3_napi *tnapi,
 	if (IS_ERR(segs) || !segs)
 		goto tg3_tso_bug_end;
 
-	do {
-		nskb = segs;
-		segs = segs->next;
-		nskb->next = NULL;
-		tg3_start_xmit(nskb, tp->dev);
-	} while (segs);
+	skb_list_walk_safe(segs, seg, next) {
+		skb_mark_not_on_list(seg);
+		tg3_start_xmit(seg, tp->dev);
+	}
 
 tg3_tso_bug_end:
 	dev_consume_skb_any(skb);
-- 
2.24.1

[PATCH 5/8] net: sunvnet: use skb_list_walk_safe helper for gso segments

[Jason A. Donenfeld]

This is a straight-forward conversion case for the new function, and
while we're at it, we can remove a null write to skb->next by replacing
it with skb_mark_not_on_list.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 drivers/net/ethernet/sun/sunvnet_common.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/sun/sunvnet_common.c b/drivers/net/ethernet/sun/sunvnet_common.c
index a601a306f9a5..c23ce838ff63 100644
--- a/drivers/net/ethernet/sun/sunvnet_common.c
+++ b/drivers/net/ethernet/sun/sunvnet_common.c
@@ -1223,7 +1223,7 @@ vnet_handle_offloads(struct vnet_port *port, struct sk_buff *skb,
 {
 	struct net_device *dev = VNET_PORT_TO_NET_DEVICE(port);
 	struct vio_dring_state *dr = &port->vio.drings[VIO_DRIVER_TX_RING];
-	struct sk_buff *segs;
+	struct sk_buff *segs, *curr, *next;
 	int maclen, datalen;
 	int status;
 	int gso_size, gso_type, gso_segs;
@@ -1282,11 +1282,8 @@ vnet_handle_offloads(struct vnet_port *port, struct sk_buff *skb,
 	skb_reset_mac_header(skb);
 
 	status = 0;
-	while (segs) {
-		struct sk_buff *curr = segs;
-
-		segs = segs->next;
-		curr->next = NULL;
+	skb_list_walk_safe(segs, curr, next) {
+		skb_mark_not_on_list(curr);
 		if (port->tso && curr->len > dev->mtu) {
 			skb_shinfo(curr)->gso_size = gso_size;
 			skb_shinfo(curr)->gso_type = gso_type;
-- 
2.24.1

[PATCH 6/8] net: sfc: use skb_list_walk_safe helper for gso segments

[Jason A. Donenfeld]

This is a straight-forward conversion case for the new function, and
while we're at it, we can remove a null write to skb->next by replacing
it with skb_mark_not_on_list.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 drivers/net/ethernet/sfc/tx.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/sfc/tx.c b/drivers/net/ethernet/sfc/tx.c
index 00c1c4402451..547692b33b4d 100644
--- a/drivers/net/ethernet/sfc/tx.c
+++ b/drivers/net/ethernet/sfc/tx.c
@@ -473,12 +473,9 @@ static int efx_tx_tso_fallback(struct efx_tx_queue *tx_queue,
 	dev_consume_skb_any(skb);
 	skb = segments;
 
-	while (skb) {
-		next = skb->next;
-		skb->next = NULL;
-
+	skb_list_walk_safe(skb, skb, next) {
+		skb_mark_not_on_list(skb);
 		efx_enqueue_skb(tx_queue, skb);
-		skb = next;
 	}
 
 	return 0;
-- 
2.24.1

[PATCH 7/8] net: myri10ge: use skb_list_walk_safe helper for gso segments

[Jason A. Donenfeld]

This is a straight-forward conversion case for the new function, and
while we're at it, we can remove a null write to skb->next by replacing
it with skb_mark_not_on_list.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 drivers/net/ethernet/myricom/myri10ge/myri10ge.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/myricom/myri10ge/myri10ge.c b/drivers/net/ethernet/myricom/myri10ge/myri10ge.c
index c979f38a2e0c..2ee0d0be113a 100644
--- a/drivers/net/ethernet/myricom/myri10ge/myri10ge.c
+++ b/drivers/net/ethernet/myricom/myri10ge/myri10ge.c
@@ -2892,7 +2892,7 @@ static netdev_tx_t myri10ge_xmit(struct sk_buff *skb,
 static netdev_tx_t myri10ge_sw_tso(struct sk_buff *skb,
 					 struct net_device *dev)
 {
-	struct sk_buff *segs, *curr;
+	struct sk_buff *segs, *curr, *next;
 	struct myri10ge_priv *mgp = netdev_priv(dev);
 	struct myri10ge_slice_state *ss;
 	netdev_tx_t status;
@@ -2901,10 +2901,8 @@ static netdev_tx_t myri10ge_sw_tso(struct sk_buff *skb,
 	if (IS_ERR(segs))
 		goto drop;
 
-	while (segs) {
-		curr = segs;
-		segs = segs->next;
-		curr->next = NULL;
+	skb_list_walk_safe(segs, curr, next) {
+		skb_mark_not_on_list(curr);
 		status = myri10ge_xmit(curr, dev);
 		if (status != 0) {
 			dev_kfree_skb_any(curr);
-- 
2.24.1

[PATCH 8/8] net: iwlwifi: use skb_list_walk_safe helper for gso segments

[Jason A. Donenfeld]

This is a straight-forward conversion case for the new function, and
while we're at it, we can remove a null write to skb->next by replacing
it with skb_mark_not_on_list.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 drivers/net/wireless/intel/iwlwifi/mvm/tx.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/tx.c b/drivers/net/wireless/intel/iwlwifi/mvm/tx.c
index dc5c02fbc65a..6a241d37a057 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/tx.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/tx.c
@@ -847,10 +847,7 @@ iwl_mvm_tx_tso_segment(struct sk_buff *skb, unsigned int num_subframes,
 	else if (next)
 		consume_skb(skb);
 
-	while (next) {
-		tmp = next;
-		next = tmp->next;
-
+	skb_list_walk_safe(next, tmp, next) {
 		memcpy(tmp->cb, cb, sizeof(tmp->cb));
 		/*
 		 * Compute the length of all the data added for the A-MSDU.
@@ -880,9 +877,7 @@ iwl_mvm_tx_tso_segment(struct sk_buff *skb, unsigned int num_subframes,
 			skb_shinfo(tmp)->gso_size = 0;
 		}
 
-		tmp->prev = NULL;
-		tmp->next = NULL;
-
+		skb_mark_not_on_list(tmp);
 		__skb_queue_tail(mpdus_skb, tmp);
 		i++;
 	}
-- 
2.24.1

Re: [PATCH 0/8] reduce open coded skb->next access for gso segment walking

[David Miller]

From: "Jason A. Donenfeld" <Jason@zx2c4.com>
Date: Wed,  8 Jan 2020 16:59:01 -0500

> This patchset introduces the skb_list_walk_safe helper macro, in order
> to add some sanity to the myrid ways drivers have of walking through gso
> segments. The goal is to reduce future bugs commonly caused by open
> coding these sorts of things, and to in the future make it easier to
> swap out the underlying list representation.
> 
> This first patch series addresses the easy uses of drivers iterating
> over the returned list of skb_gso_segments, for drivers that live in
> drivers/net/*. There are still other use cases to tackle later for
> net/*, and after these low-hanging fruits are taken care of, I imagine
> there are more subtle cases of gso segment walking that isn't just a
> direct return value from skb_gso_segments, and eventually this will have
> to be tackled. This series is the first in that direction.

I like this, applied to net-next and build testing.  Let's see where this
goes.

In the iwlwifi case, the skb_mark_not_on_list() is really redundant because
deep down inside the skb queue tail insert, __skb_insert() will unconditionally
always write both ->next and ->prev and there are no debugging checks along
the way which would trigger if skb->next was non-NULL.

I guess you could argue for defensive programming here, so there's that.

Anyways, thanks.

Re: [PATCH 6/8] net: sfc: use skb_list_walk_safe helper for gso segments

[Edward Cree]

On 08/01/2020 21:59, Jason A. Donenfeld wrote:
> This is a straight-forward conversion case for the new function, and
> while we're at it, we can remove a null write to skb->next by replacing
> it with skb_mark_not_on_list.
>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
>  drivers/net/ethernet/sfc/tx.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/ethernet/sfc/tx.c b/drivers/net/ethernet/sfc/tx.c
> index 00c1c4402451..547692b33b4d 100644
> --- a/drivers/net/ethernet/sfc/tx.c
> +++ b/drivers/net/ethernet/sfc/tx.c
> @@ -473,12 +473,9 @@ static int efx_tx_tso_fallback(struct efx_tx_queue *tx_queue,
>  	dev_consume_skb_any(skb);
>  	skb = segments;
>  
> -	while (skb) {
> -		next = skb->next;
> -		skb->next = NULL;
> -
> +	skb_list_walk_safe(skb, skb, next) {
Could this be replaced with
    skb_list_walk_safe(segments, skb, next) {
and elide the assignment just above?
Or is there some reason I'm missing not to do that?
-ed
> +		skb_mark_not_on_list(skb);
>  		efx_enqueue_skb(tx_queue, skb);
> -		skb = next;
>  	}
>  
>  	return 0;

Re: [PATCH 6/8] net: sfc: use skb_list_walk_safe helper for gso segments

[Jason A. Donenfeld]

On Tue, Feb 18, 2020 at 4:35 PM Edward Cree <ecree@solarflare.com> wrote:
>
> On 08/01/2020 21:59, Jason A. Donenfeld wrote:
> > This is a straight-forward conversion case for the new function, and
> > while we're at it, we can remove a null write to skb->next by replacing
> > it with skb_mark_not_on_list.
> >
> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> > ---
> >  drivers/net/ethernet/sfc/tx.c | 7 ++-----
> >  1 file changed, 2 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/sfc/tx.c b/drivers/net/ethernet/sfc/tx.c
> > index 00c1c4402451..547692b33b4d 100644
> > --- a/drivers/net/ethernet/sfc/tx.c
> > +++ b/drivers/net/ethernet/sfc/tx.c
> > @@ -473,12 +473,9 @@ static int efx_tx_tso_fallback(struct efx_tx_queue *tx_queue,
> >       dev_consume_skb_any(skb);
> >       skb = segments;
> >
> > -     while (skb) {
> > -             next = skb->next;
> > -             skb->next = NULL;
> > -
> > +     skb_list_walk_safe(skb, skb, next) {
> Could this be replaced with
>     skb_list_walk_safe(segments, skb, next) {
> and elide the assignment just above?
> Or is there some reason I'm missing not to do that?

Yes that's probably correct.