Fix skb length accounting after XDP frag adjustment

Generic XDP exposes non-linear skb fragments through an xdp_buff. If an XDP program adjusts the fragment area, bpf_prog_run_generic_xdp() copies xdp_frags_size back to skb->data_len but leaves skb->len containing the old fragment contribution.

After a fragment shrink, this makes skb_headlen() larger than the actual linear area. In the reproduced UDP receive path, __skb_datagram_iter() copied 1024 bytes past the actual linear tail to userspace, starting at struct skb_shared_info. The copied bytes included the affected skb's nr_frags, xdp_frags_size and a kernel pointer from skb_shinfo(skb)->frags[0]. Real packet data was displaced by the same amount and truncated at the end.

Subtract the old data_len before replacing it and add the new data_len afterwards, keeping skb->len and skb->data_len synchronized.

A 60000-byte UDP datagram on a veth pair with MTU 64000 was shortened by 1024 bytes from its fragment area. Before the fix, all 10 runs produced corrupted payloads. After the fix, all 10 runs matched the expected payload exactly.

Fixes e6d5dbdd20aa ("xdp: add multi-buff support for xdp running in generic mode")

Cc [email protected]

Link https://lore.kernel.org/bpf/al9T9Eto%2FhRIzP5W@boxer/ open here

Signed-off-by Sun Jian <[email protected]>

---
  net/core/dev.c | 10 +++++++---
  1 file changed, 7 insertions(+), 3 deletions(-)

net/core/dev.c +3 -2

index 5933c5dab09e..5c37cf6c4aa1 100644
@@ -5517,12 +5517,16 @@ u32 bpf_prog_run_generic_xdp(struct sk_buff *skb, struct xdp_buff *xdp,
  	}
 
  	/* XDP frag metadata (e.g. nr_frags) are updated in eBPF helpers
-	 * (e.g. bpf_xdp_adjust_tail), we need to update data_len here.
+	 * (e.g. bpf_xdp_adjust_tail). Remove the old fragment contribution
+	 * from skb->len before updating data_len, then add the new one back.
  	 */
-	if (xdp_buff_has_frags(xdp))
+	skb->len -= skb->data_len;

Generic XDP exposes non-linear skb fragments through an xdp_buff. If an XDP program adjusts the fragment area, bpf_prog_run_generic_xdp() copies xdp_frags_size back to skb->data_len but leaves skb->len containing the old fragment contribution.

After a fragment shrink, this makes skb_headlen() larger than the actual linear area. In the reproduced UDP receive path, __skb_datagram_iter() copied 1024 bytes past the actual linear tail to userspace, starting at struct skb_shared_info. The copied bytes included the affected skb's nr_frags, xdp_frags_size and a kernel pointer from skb_shinfo(skb)->frags[0]. Real packet data was displaced by the same amount and truncated at the end.

Subtract the old data_len before replacing it and add the new data_len afterwards, keeping skb->len and skb->data_len synchronized.

A 60000-byte UDP datagram on a veth pair with MTU 64000 was shortened by 1024 bytes from its fragment area. Before the fix, all 10 runs produced corrupted payloads. After the fix, all 10 runs matched the expected payload exactly.

Fixes e6d5dbdd20aa ("xdp: add multi-buff support for xdp running in generic mode")

Cc [email protected]

Link https://lore.kernel.org/bpf/al9T9Eto%2FhRIzP5W@boxer/ open here

Signed-off-by Sun Jian <[email protected]>

---
 net/core/dev.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

net/core/dev.c +7 -3

index 5933c5dab09e..5c37cf6c4aa1 100644
@@ -5517,12 +5517,16 @@ u32 bpf_prog_run_generic_xdp(struct sk_buff *skb, struct xdp_buff *xdp,
 	}
 
 	/* XDP frag metadata (e.g. nr_frags) are updated in eBPF helpers
-	 * (e.g. bpf_xdp_adjust_tail), we need to update data_len here.
+	 * (e.g. bpf_xdp_adjust_tail). Remove the old fragment contribution
+	 * from skb->len before updating data_len, then add the new one back.
 	 */
-	if (xdp_buff_has_frags(xdp))
+	skb->len -= skb->data_len;
+	if (xdp_buff_has_frags(xdp)) {
 		skb->data_len = skb_shinfo(skb)->xdp_frags_size;
-	else
+		skb->len += skb->data_len;
+	} else {
 		skb->data_len = 0;
+	}
 
 	/* check if XDP changed eth hdr such SKB needs update */
 	eth = (struct ethhdr *)xdp->data;

Signature · 1 line

2.43.0

veth exposes non-linear skb fragments through an xdp_buff. If an XDP program adjusts the fragment area, veth_xdp_rcv_skb() copies xdp_frags_size back to skb->data_len but leaves skb->len containing the old fragment contribution.

After a fragment shrink, this makes skb_headlen() larger than the actual linear area. In the reproduced UDP receive path, __skb_datagram_iter() copied 1024 bytes past the actual linear tail to userspace, starting at struct skb_shared_info. The copied bytes included the affected skb's nr_frags, xdp_frags_size and a kernel pointer from skb_shinfo(skb)->frags[0]. Real packet data was displaced by the same amount and truncated at the end.

Subtract the old data_len before replacing it and add the new data_len afterwards, keeping skb->len and skb->data_len synchronized.

The fragment accounting must run before the linear tail adjustment:
when bpf_xdp_adjust_tail() shrinks the packet into the linear area it releases all fragments, and __skb_put() requires skb->data_len == 0 by that point.

A 60000-byte UDP datagram on a veth pair with MTU 64000 was shortened by 1024 bytes from its fragment area. Before the fix, all 10 runs produced corrupted payloads. After the fix, all 10 runs matched the expected payload exactly.

Fixes 718a18a0c8a6 ("veth: Rework veth_xdp_rcv_skb in order to accept non-linear skb")

Cc [email protected]

Link https://lore.kernel.org/bpf/al9T9Eto%2FhRIzP5W@boxer/ open here

Signed-off-by Sun Jian <[email protected]>

---
 drivers/net/veth.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

drivers/net/veth.c +15 -8

index 00e34afd858e..348391e87e14 100644
@@ -865,18 +865,25 @@ static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq,
 
 	skb_reset_mac_header(skb);
 
-	/* check if bpf_xdp_adjust_tail was used */
-	off = xdp->data_end - orig_data_end;
-	if (off != 0)
-		__skb_put(skb, off); /* positive on grow, negative on shrink */
-
 	/* XDP frag metadata (e.g. nr_frags) are updated in eBPF helpers
-	 * (e.g. bpf_xdp_adjust_tail), we need to update data_len here.
+	 * (e.g. bpf_xdp_adjust_tail). Remove the old fragment contribution
+	 * from skb->len before updating data_len, then add the new one back.
+	 * This must precede the linear tail adjustment below: a changed
+	 * data_end implies that no fragments remain, and __skb_put() requires
+	 * a linear skb.
 	 */
-	if (xdp_buff_has_frags(xdp))
+	skb->len -= skb->data_len;
+	if (xdp_buff_has_frags(xdp)) {
 		skb->data_len = skb_shinfo(skb)->xdp_frags_size;
-	else
+		skb->len += skb->data_len;
+	} else {
 		skb->data_len = 0;
+	}
+
+	/* check if bpf_xdp_adjust_tail was used */
+	off = xdp->data_end - orig_data_end;
+	if (off != 0)
+		__skb_put(skb, off); /* positive on grow, negative on shrink */
 
 	skb->protocol = eth_type_trans(skb, rq->dev);
 

Signature · 1 line

2.43.0

veth exposes non-linear skb fragments through an xdp_buff. If an XDP program adjusts the fragment area, veth_xdp_rcv_skb() copies xdp_frags_size back to skb->data_len but leaves skb->len containing the old fragment contribution.

After a fragment shrink, this makes skb_headlen() larger than the actual linear area. In the reproduced UDP receive path, __skb_datagram_iter() copied 1024 bytes past the actual linear tail to userspace, starting at struct skb_shared_info. The copied bytes included the affected skb's nr_frags, xdp_frags_size and a kernel pointer from skb_shinfo(skb)->frags[0]. Real packet data was displaced by the same amount and truncated at the end.

Subtract the old data_len before replacing it and add the new data_len afterwards, keeping skb->len and skb->data_len synchronized.

The fragment accounting must run before the linear tail adjustment:
when bpf_xdp_adjust_tail() shrinks the packet into the linear area it releases all fragments, and __skb_put() requires skb->data_len == 0 by that point.

A 60000-byte UDP datagram on a veth pair with MTU 64000 was shortened by 1024 bytes from its fragment area. Before the fix, all 10 runs produced corrupted payloads. After the fix, all 10 runs matched the expected payload exactly.

Fixes 718a18a0c8a6 ("veth: Rework veth_xdp_rcv_skb in order to accept non-linear skb")

Cc [email protected]

Link https://lore.kernel.org/bpf/al9T9Eto%2FhRIzP5W@boxer/ open here

Signed-off-by Sun Jian <[email protected]>

---
  drivers/net/veth.c | 23 +++++++++++++++--------
  1 file changed, 15 insertions(+), 8 deletions(-)

drivers/net/veth.c +9 -8

index 00e34afd858e..348391e87e14 100644
@@ -865,18 +865,25 @@ static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq,
 
  	skb_reset_mac_header(skb);
 
-	/* check if bpf_xdp_adjust_tail was used */
-	off = xdp->data_end - orig_data_end;
-	if (off != 0)
-		__skb_put(skb, off); /* positive on grow, negative on shrink */
-
  	/* XDP frag metadata (e.g. nr_frags) are updated in eBPF helpers
-	 * (e.g. bpf_xdp_adjust_tail), we need to update data_len here.
+	 * (e.g. bpf_xdp_adjust_tail). Remove the old fragment contribution
+	 * from skb->len before updating data_len, then add the new one back.
+	 * This must precede the linear tail adjustment below: a changed
+	 * data_end implies that no fragments remain, and __skb_put() requires
+	 * a linear skb.
  	 */
-	if (xdp_buff_has_frags(xdp))
+	skb->len -= skb->data_len;
+	if (xdp_buff_has_frags(xdp)) {
  		skb->data_len = skb_shinfo(skb)->xdp_frags_size;
-	else
+		skb->len += skb->data_len;
+	} else {

Quoted 70 lines · Sun Jian

veth exposes non-linear skb fragments through an xdp_buff. If an XDP program adjusts the fragment area, veth_xdp_rcv_skb() copies xdp_frags_size back to skb->data_len but leaves skb->len containing the old fragment contribution.

After a fragment shrink, this makes skb_headlen() larger than the actual linear area. In the reproduced UDP receive path, __skb_datagram_iter() copied 1024 bytes past the actual linear tail to userspace, starting at struct skb_shared_info. The copied bytes included the affected skb's nr_frags, xdp_frags_size and a kernel pointer from skb_shinfo(skb)->frags[0]. Real packet data was displaced by the same amount and truncated at the end.

Subtract the old data_len before replacing it and add the new data_len afterwards, keeping skb->len and skb->data_len synchronized.

The fragment accounting must run before the linear tail adjustment:
when bpf_xdp_adjust_tail() shrinks the packet into the linear area it releases all fragments, and __skb_put() requires skb->data_len == 0 by that point.

A 60000-byte UDP datagram on a veth pair with MTU 64000 was shortened by 1024 bytes from its fragment area. Before the fix, all 10 runs produced corrupted payloads. After the fix, all 10 runs matched the expected payload exactly.

Fixes 718a18a0c8a6 ("veth: Rework veth_xdp_rcv_skb in order to accept non-linear skb")

Cc [email protected]

Link https://lore.kernel.org/bpf/al9T9Eto%2FhRIzP5W@boxer/ open here

Signed-off-by Sun Jian <[email protected]>

---
  drivers/net/veth.c | 23 +++++++++++++++--------
  1 file changed, 15 insertions(+), 8 deletions(-)

drivers/net/veth.c +15 -8

index 00e34afd858e..348391e87e14 100644
@@ -865,18 +865,25 @@ static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq,
 
      skb_reset_mac_header(skb);
 
-     /* check if bpf_xdp_adjust_tail was used */
-     off = xdp->data_end - orig_data_end;
-     if (off != 0)
-             __skb_put(skb, off); /* positive on grow, negative on shrink */
-
      /* XDP frag metadata (e.g. nr_frags) are updated in eBPF helpers
-      * (e.g. bpf_xdp_adjust_tail), we need to update data_len here.
+      * (e.g. bpf_xdp_adjust_tail). Remove the old fragment contribution
+      * from skb->len before updating data_len, then add the new one back.
+      * This must precede the linear tail adjustment below: a changed
+      * data_end implies that no fragments remain, and __skb_put() requires
+      * a linear skb.
       */
-     if (xdp_buff_has_frags(xdp))
+     skb->len -= skb->data_len;
+     if (xdp_buff_has_frags(xdp)) {
              skb->data_len = skb_shinfo(skb)->xdp_frags_size;
-     else
+             skb->len += skb->data_len;
+     } else {
              skb->data_len = 0;
+     }
+
+     /* check if bpf_xdp_adjust_tail was used */
+     off = xdp->data_end - orig_data_end;
+     if (off != 0)
+             __skb_put(skb, off); /* positive on grow, negative on shrink */
 

drivers/net/veth.c static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq, · line 889

Quoted 2 lines

drivers/net/veth.c +0 -0

      skb->protocol = eth_type_trans(skb, rq->dev);
 

I am most likely missing something here but what happens if we have frags and we attempt to advance data_end while leaving some frags present (e.g., bpf_xdp_pull_data())? Looks like, in that case we would issue __skb_put(skb, off) with off > 0 and we would hit SKB_LINEAR_ASSERT() because skb is still non-linear?

Quoted 74 lines · Sun Jian

Quoted 70 lines

veth exposes non-linear skb fragments through an xdp_buff. If an XDP program adjusts the fragment area, veth_xdp_rcv_skb() copies xdp_frags_size back to skb->data_len but leaves skb->len containing the old fragment contribution.

After a fragment shrink, this makes skb_headlen() larger than the actual linear area. In the reproduced UDP receive path, __skb_datagram_iter() copied 1024 bytes past the actual linear tail to userspace, starting at struct skb_shared_info. The copied bytes included the affected skb's nr_frags, xdp_frags_size and a kernel pointer from skb_shinfo(skb)->frags[0]. Real packet data was displaced by the same amount and truncated at the end.

Subtract the old data_len before replacing it and add the new data_len afterwards, keeping skb->len and skb->data_len synchronized.

The fragment accounting must run before the linear tail adjustment:
when bpf_xdp_adjust_tail() shrinks the packet into the linear area it releases all fragments, and __skb_put() requires skb->data_len == 0 by that point.

A 60000-byte UDP datagram on a veth pair with MTU 64000 was shortened by 1024 bytes from its fragment area. Before the fix, all 10 runs produced corrupted payloads. After the fix, all 10 runs matched the expected payload exactly.

Fixes 718a18a0c8a6 ("veth: Rework veth_xdp_rcv_skb in order to accept non-linear skb")

Cc [email protected]

Link https://lore.kernel.org/bpf/al9T9Eto%2FhRIzP5W@boxer/ open here

Signed-off-by Sun Jian <[email protected]>

---
  drivers/net/veth.c | 23 +++++++++++++++--------
  1 file changed, 15 insertions(+), 8 deletions(-)

drivers/net/veth.c +15 -8

index 00e34afd858e..348391e87e14 100644
@@ -865,18 +865,25 @@ static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq,
 
      skb_reset_mac_header(skb);
 
-     /* check if bpf_xdp_adjust_tail was used */
-     off = xdp->data_end - orig_data_end;
-     if (off != 0)
-             __skb_put(skb, off); /* positive on grow, negative on shrink */
-
      /* XDP frag metadata (e.g. nr_frags) are updated in eBPF helpers
-      * (e.g. bpf_xdp_adjust_tail), we need to update data_len here.
+      * (e.g. bpf_xdp_adjust_tail). Remove the old fragment contribution
+      * from skb->len before updating data_len, then add the new one back.
+      * This must precede the linear tail adjustment below: a changed
+      * data_end implies that no fragments remain, and __skb_put() requires
+      * a linear skb.
       */
-     if (xdp_buff_has_frags(xdp))
+     skb->len -= skb->data_len;
+     if (xdp_buff_has_frags(xdp)) {
              skb->data_len = skb_shinfo(skb)->xdp_frags_size;
-     else
+             skb->len += skb->data_len;
+     } else {
              skb->data_len = 0;
+     }
+
+     /* check if bpf_xdp_adjust_tail was used */
+     off = xdp->data_end - orig_data_end;
+     if (off != 0)
+             __skb_put(skb, off); /* positive on grow, negative on shrink */
 

Quoted 8 lines

drivers/net/veth.c static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq, · line 889

Quoted 2 lines

drivers/net/veth.c +0 -0

      skb->protocol = eth_type_trans(skb, rq->dev);
 

I am most likely missing something here but what happens if we have frags and we attempt to advance data_end while leaving some frags present (e.g., bpf_xdp_pull_data())? Looks like, in that case we would issue __skb_put(skb, off) with off > 0 and we would hit SKB_LINEAR_ASSERT() because skb is still non-linear?

Hi Mohsin,

Thanks for catching this. You're right that bpf_xdp_pull_data() can advance data_end while leaving frags present. I'll investigate this path and follow up.

Thanks,
Sun Jian

Quoted 85 lines · Mohsin Bashir

Quoted 82 lines · Sun Jian

Quoted 62 lines

veth exposes non-linear skb fragments through an xdp_buff. If an XDP program adjusts the fragment area, veth_xdp_rcv_skb() copies xdp_frags_size back to skb->data_len but leaves skb->len containing the old fragment contribution.

After a fragment shrink, this makes skb_headlen() larger than the actual linear area. In the reproduced UDP receive path, __skb_datagram_iter() copied 1024 bytes past the actual linear tail to userspace, starting at struct skb_shared_info. The copied bytes included the affected skb's nr_frags, xdp_frags_size and a kernel pointer from skb_shinfo(skb)->frags[0]. Real packet data was displaced by the same amount and truncated at the end.

Subtract the old data_len before replacing it and add the new data_len afterwards, keeping skb->len and skb->data_len synchronized.

The fragment accounting must run before the linear tail adjustment:
when bpf_xdp_adjust_tail() shrinks the packet into the linear area it releases all fragments, and __skb_put() requires skb->data_len == 0 by that point.

A 60000-byte UDP datagram on a veth pair with MTU 64000 was shortened by 1024 bytes from its fragment area. Before the fix, all 10 runs produced corrupted payloads. After the fix, all 10 runs matched the expected payload exactly.

Fixes 718a18a0c8a6 ("veth: Rework veth_xdp_rcv_skb in order to accept non-linear skb")

Cc [email protected]

Link https://lore.kernel.org/bpf/al9T9Eto%2FhRIzP5W@boxer/ open here

Signed-off-by Sun Jian <[email protected]>

---
  drivers/net/veth.c | 23 +++++++++++++++--------
  1 file changed, 15 insertions(+), 8 deletions(-)

drivers/net/veth.c +9 -8

index 00e34afd858e..348391e87e14 100644
@@ -865,18 +865,25 @@ static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq,
 
      skb_reset_mac_header(skb);
 
-     /* check if bpf_xdp_adjust_tail was used */
-     off = xdp->data_end - orig_data_end;
-     if (off != 0)
-             __skb_put(skb, off); /* positive on grow, negative on shrink */
-
      /* XDP frag metadata (e.g. nr_frags) are updated in eBPF helpers
-      * (e.g. bpf_xdp_adjust_tail), we need to update data_len here.
+      * (e.g. bpf_xdp_adjust_tail). Remove the old fragment contribution
+      * from skb->len before updating data_len, then add the new one back.
+      * This must precede the linear tail adjustment below: a changed
+      * data_end implies that no fragments remain, and __skb_put() requires
+      * a linear skb.
       */
-     if (xdp_buff_has_frags(xdp))
+     skb->len -= skb->data_len;
+     if (xdp_buff_has_frags(xdp)) {
              skb->data_len = skb_shinfo(skb)->xdp_frags_size;
-     else
+             skb->len += skb->data_len;
+     } else {

drivers/net/veth.c static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq, · line 889

Quoted 10 lines

drivers/net/veth.c +6 -0

              skb->data_len = 0;
+     }
+
+     /* check if bpf_xdp_adjust_tail was used */
+     off = xdp->data_end - orig_data_end;
+     if (off != 0)
+             __skb_put(skb, off); /* positive on grow, negative on shrink */
 
      skb->protocol = eth_type_trans(skb, rq->dev);
 

I am most likely missing something here but what happens if we have frags and we attempt to advance data_end while leaving some frags present (e.g., bpf_xdp_pull_data())? Looks like, in that case we would issue __skb_put(skb, off) with off > 0 and we would hit SKB_LINEAR_ASSERT() because skb is still non-linear?

Quoted 7 lines

Hi Mohsin,

Thanks for catching this. You're right that bpf_xdp_pull_data() can advance data_end while leaving frags present. I'll investigate this path and follow up.

Thanks,
Sun Jian

I guess we should implement the same approach used in bpf_prog_run_generic_xdp(), something like:

        if (off) {
                skb_set_tail_pointer();
                skb->len += off;
        }

Regards,
Lorenzo