[PATCH net-next v4 0/2] net: prevent mixing incompatible memory providers in skbs

[PATCH net-next v4 0/2] net: prevent mixing incompatible memory providers in skbs

[Mina Almasry]

Enforce that all fragments in a nonlinear skb come from compatible
memory providers (and for devmem, the exact same binding).

Patch 1 fixes skb->unreadable propagation in skb_split() and skb_shift().
Patch 2 adds skb_has_net_iovs(), skb_mem_provider_compat(), and
skb_frags_can_coalesce(), and enforces provider/binding compatibility
across TX, TCP, GRO, skb_shift(), and skb_try_coalesce().

v3 -> v4:
- Fold skb_shinfo(skb)->nr_frags into skb_has_net_iovs() and drop
  redundant nr_frags checks in skb_net_iov_provider() and
  net_devmem_skb_binding() (Stanislav Fomichev).
- Remove the empty-skb check and redundant !skb_has_net_iovs(skb1)
  branch from skb_mem_provider_compat(), and add a comment on the
  early skb_has_net_iovs() check (Stanislav Fomichev).
- Propagate skb->unreadable and compare skb_frag_netmem() directly in
  tcp_clone_payload() instead of rejecting unreadable skbs in
  tcp_can_coalesce_send_queue_head().
- v3: https://lore.kernel.org/netdev/20261104042521.3979880-1-almasrymina@google.com/

v2 -> v3:
- Drop patch 2 ('netmem: store memory provider private pointer directly
  in struct net_iov') and compare net_iov_owner(niov) (for devmem) /
  niov->desc.pp (for other net_iov providers) directly in
  skb_mem_provider_compat() (Stanislav Fomichev).
- Squash the skb_frags_can_coalesce() helper patch into its caller patch
  (Stanislav Fomichev).
- v2: https://lore.kernel.org/netdev/20261102231827.4088254-1-almasrymina@google.com/

v1 -> v2:
- Generalize the check across TX, TCP collapse, GRO, IP defrag,
  skb_shift(), and skb_try_coalesce() by storing mp_priv directly in
  struct net_iov and introducing skb_frags_can_coalesce().
- Fix skb->unreadable propagation in skb_split() and skb_shift().
- Add KUnit coverage for cross-provider and cross-binding rejection.
- v1: https://lore.kernel.org/netdev/20261008034027.1403188-1-almasrymina@google.com/

Mina Almasry (2):
  net: fix skb->unreadable propagation in skb_split() and skb_shift()
  net: prevent mixing frags from different memory providers or bindings

 include/linux/skbuff.h | 65 ++++++++++++++++++++++++++++++++++++++++++
 include/net/tcp.h      |  5 ++--
 net/core/datagram.c    |  4 +--
 net/core/dev.c         | 14 ++++-----
 net/core/devmem.h      | 22 ++++++++++++++
 net/core/gro.c         | 16 ++++-------
 net/core/net_test.c    | 42 +++++++++++++++++++++++----
 net/core/skbuff.c      | 32 ++++++++++-----------
 net/ipv4/tcp.c         |  2 +-
 net/ipv4/tcp_output.c  |  3 +-
 10 files changed, 157 insertions(+), 48 deletions(-)


base-commit: 7b6ab4dbc156e2f96419715130b3d5738a306c93
-- 
2.56.0.455.g6c5a503b38-goog

[PATCH net-next v4 1/2] net: fix skb->unreadable propagation in skb_split() and skb_shift()

[Mina Almasry]

skb_split() and skb_shift() do not accurately maintain skb->unreadable
when moving fragments between skbs:

- skb_split_inside_header() moves all fragments out of skb into skb1
  (leaving skb with nr_frags == 0 and only host-readable linear memory),
  but leaves skb->unreadable set on skb.
- skb_split_no_header() unconditionally copies skb->unreadable to skb1
  even if 0 fragments were moved to skb1 (k == 0), and leaves
  skb->unreadable set on skb when splitting at skb_headlen(skb) moves
  all fragments into skb1 (!skb_shinfo(skb)->nr_frags).
- skb_shift() does not clear skb->unreadable after all fragments are
  shifted out of skb.

Fix all three cases so skb->unreadable accurately tracks whether the
remaining fragments in each skb are unreadable.

This issue is theoretical and was found by manual code inspection and
LLM review; it was not triggered in production. Verified with the
net_core KUnit test suite; not tested on real hardware.

Fixes: 65249feb6b3d ("net: add support for skbs with unreadable frags")
Acked-by: Stanislav Fomichev <sdf@fomichev.me>
Signed-off-by: Mina Almasry <almasrymina@google.com>
---
 net/core/skbuff.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 48f6764117243..43e720d30b67b 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -4225,6 +4225,7 @@ static inline void skb_split_inside_header(struct sk_buff *skb,
 
 	skb_shinfo(skb1)->nr_frags = skb_shinfo(skb)->nr_frags;
 	skb1->unreadable	   = skb->unreadable;
+	skb->unreadable		   = 0;
 	skb_shinfo(skb)->nr_frags  = 0;
 	skb1->data_len		   = skb->data_len;
 	skb1->len		   += skb1->data_len;
@@ -4273,7 +4274,10 @@ static inline void skb_split_no_header(struct sk_buff *skb,
 	}
 	skb_shinfo(skb1)->nr_frags = k;
 
-	skb1->unreadable = skb->unreadable;
+	if (k)
+		skb1->unreadable = skb->unreadable;
+	if (!skb_shinfo(skb)->nr_frags)
+		skb->unreadable = 0;
 }
 
 /**
@@ -4447,6 +4451,8 @@ int skb_shift(struct sk_buff *tgt, struct sk_buff *skb, int shiftlen)
 	skb->ip_summed = CHECKSUM_PARTIAL;
 
 	skb_shinfo(tgt)->flags |= skb_shinfo(skb)->flags & SKBFL_SHARED_FRAG;
+	if (!skb_shinfo(skb)->nr_frags && !skb_has_frag_list(skb))
+		skb->unreadable = 0;
 
 	skb_len_add(skb, -shiftlen);
 	skb_len_add(tgt, shiftlen);
-- 
2.56.0.455.g6c5a503b38-goog

[PATCH net-next v4 2/2] net: prevent mixing frags from different memory providers or bindings

[Mina Almasry]

Enforce that all fragments in a nonlinear skb come from compatible
memory providers (and for devmem, the exact same binding):

- Add skb_has_net_iovs(skb) (skb->unreadable && skb_shinfo(skb)->nr_frags),
  assuming skb->unreadable and net_iov-backed frags are equivalent for
  now (with a comment noting the two can be decoupled in the future if
  readable net_iovs are introduced).
- Add skb_net_iov_provider(), skb_mem_provider_compat(), and
  skb_frags_can_coalesce() so multi-skb coalescing, shifting, and GRO
  paths can share a single check across pp_recycle, skb_zcopy(), and
  fragment memory providers:
  - page vs net_iov is rejected via skb_has_net_iovs() without chasing
    frags[0] or struct net_iov
  - page vs page and net_iov vs net_iov compare skb_net_iov_provider()
    (NULL == NULL with 0 pointer chases for page-backed skbs;
    net_iov_owner(niov) for devmem and niov->desc.pp for other net_iov
    providers such as io_uring zcrx)
  - Because skb_has_net_iovs() requires nr_frags > 0, coalescing or
    shifting net_iov frags into a frag-less (nr_frags == 0) skb is also
    rejected (no in-tree TCP caller shifts net_iov frags into an empty
    skb; update netmem_ref_test_net_iov to pre-populate the target skb
    with a frag from the same binding before calling skb_shift()).
- Add net_devmem_skb_binding() and use it in
  validate_xmit_unreadable_skb(), zerocopy_fill_skb_from_devmem(), and
  tcp_sendmsg_locked() so frags from different devmem bindings or memory
  providers cannot be mixed on TX.
- Use skb_frags_can_coalesce() across skb_try_coalesce(), skb_shift(),
  and skb_gro_receive().
- Use skb_mem_provider_compat() in skb_gro_receive_list(),
  tcp_skb_can_collapse(), and tcp_skb_can_collapse_rx().
- Reject converting linear head_frag payloads into page frags on
  unreadable skbs in skb_try_coalesce() and skb_gro_receive().
- Propagate skb->unreadable and compare skb_frag_netmem() instead of
  skb_frag_page() (which returns NULL for net_iovs) when merging frags
  in tcp_clone_payload().

This issue is theoretical and was found by manual code inspection and
LLM review; it was not triggered in production. Verified with the
net_core KUnit test suite; not tested on real hardware.

Fixes: 65249feb6b3d ("net: add support for skbs with unreadable frags")
Fixes: bd61848900bf ("net: devmem: Implement TX path")
Signed-off-by: Mina Almasry <almasrymina@google.com>
---
 include/linux/skbuff.h | 65 ++++++++++++++++++++++++++++++++++++++++++
 include/net/tcp.h      |  5 ++--
 net/core/datagram.c    |  4 +--
 net/core/dev.c         | 14 ++++-----
 net/core/devmem.h      | 22 ++++++++++++++
 net/core/gro.c         | 16 ++++-------
 net/core/net_test.c    | 42 +++++++++++++++++++++++----
 net/core/skbuff.c      | 24 ++++++----------
 net/ipv4/tcp.c         |  2 +-
 net/ipv4/tcp_output.c  |  3 +-
 10 files changed, 150 insertions(+), 47 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 6787fcf6ac3e4..e6f63450a193a 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1882,6 +1882,17 @@ static inline bool skb_frags_readable(const struct sk_buff *skb)
 	return !skb->unreadable;
 }
 
+/* Return true if frags in this skb are backed by net_iovs.
+ * Assumes skb->unreadable and net_iov-backed frags are equivalent for now;
+ * the two can be decoupled in the future if readable net_iovs in skbs are
+ * introduced. Also check nr_frags because not all frag-removal paths clear
+ * skb->unreadable when nr_frags drops to 0.
+ */
+static inline bool skb_has_net_iovs(const struct sk_buff *skb)
+{
+	return skb->unreadable && skb_shinfo(skb)->nr_frags;
+}
+
 static inline void skb_mark_not_on_list(struct sk_buff *skb)
 {
 	skb->next = NULL;
@@ -3763,6 +3774,60 @@ static inline void *skb_frag_address(const skb_frag_t *frag)
 	return page_address(skb_frag_page(frag)) + skb_frag_off(frag);
 }
 
+/* Return an opaque pointer identifying the memory provider instance of @skb's
+ * net_iov fragments:
+ * - For devmem (NET_IOV_DMABUF), net_iov_owner(niov) (&binding->area, 1:1 with
+ *   the binding across RX queues and TX).
+ * - For other net_iov providers (e.g., io_uring zcrx), niov->desc.pp (shared
+ *   across all areas of an io_zcrx_ifq).
+ */
+static inline void *skb_net_iov_provider(const struct sk_buff *skb)
+{
+	struct net_iov *niov;
+
+	if (!skb_has_net_iovs(skb))
+		return NULL;
+
+	niov = skb_frag_net_iov(&skb_shinfo(skb)->frags[0]);
+	if (!niov)
+		return NULL;
+
+	if (net_is_devmem_iov(niov))
+		return net_iov_owner(niov);
+
+	return niov->desc.pp;
+}
+
+/* Return true if the memory providers of @skb1 and @skb2 are compatible
+ * for coalescing. For any struct page-backed provider (the buddy allocator
+ * or a future provider handing out struct pages), coalescing is assumed
+ * feasible. For net_iov-backed providers, the provider instance pointer
+ * must match.
+ */
+static inline bool skb_mem_provider_compat(const struct sk_buff *skb1,
+					   const struct sk_buff *skb2)
+{
+	/* Check skb_has_net_iovs() first to avoid chasing frags[0] and
+	 * struct net_iov when only one skb is net_iov-backed.
+	 */
+	if (skb_has_net_iovs(skb1) != skb_has_net_iovs(skb2))
+		return false;
+
+	return skb_net_iov_provider(skb1) == skb_net_iov_provider(skb2);
+}
+
+static inline bool skb_frags_can_coalesce(struct sk_buff *to,
+					  struct sk_buff *from)
+{
+	if (to->pp_recycle != from->pp_recycle)
+		return false;
+
+	if (skb_zcopy(to) || skb_zcopy(from))
+		return false;
+
+	return skb_mem_provider_compat(to, from);
+}
+
 /**
  * skb_frag_address_safe - gets the address of the data contained in a paged fragment
  * @frag: the paged fragment buffer
diff --git a/include/net/tcp.h b/include/net/tcp.h
index 3560adddf8215..07a222c0fe2d7 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -1268,14 +1268,15 @@ static inline bool tcp_skb_can_collapse(const struct sk_buff *to,
 		      mptcp_skb_can_collapse(to, from) &&
 		      !skb_cmp_decrypted(to, from) &&
 		      skb_pure_zcopy_same(to, from) &&
-		      skb_frags_readable(to) == skb_frags_readable(from));
+		      skb_mem_provider_compat(to, from));
 }
 
 static inline bool tcp_skb_can_collapse_rx(const struct sk_buff *to,
 					   const struct sk_buff *from)
 {
 	return likely(mptcp_skb_can_collapse(to, from) &&
-		      !skb_cmp_decrypted(to, from));
+		      !skb_cmp_decrypted(to, from) &&
+		      skb_mem_provider_compat(to, from));
 }
 
 /* Events passed to congestion control interface */
diff --git a/net/core/datagram.c b/net/core/datagram.c
index ecc30a6f83023..1e7db61e25a56 100644
--- a/net/core/datagram.c
+++ b/net/core/datagram.c
@@ -712,8 +712,8 @@ zerocopy_fill_skb_from_devmem(struct sk_buff *skb, struct iov_iter *from,
 	size_t virt_addr, size, off;
 	struct net_iov *niov;
 
-	if (i && skb_frags_readable(skb))
-		return -EFAULT;
+	if (skb->len && net_devmem_skb_binding(skb) != binding)
+		return -EMSGSIZE;
 
 	/* Devmem filling works by taking an IOVEC from the user where the
 	 * iov_addrs are interpreted as an offset in bytes into the dma-buf to
diff --git a/net/core/dev.c b/net/core/dev.c
index 1aece2fe2429f..f241d8c650fcd 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4169,8 +4169,7 @@ static struct sk_buff *sk_validate_xmit_skb(struct sk_buff *skb,
 static struct sk_buff *validate_xmit_unreadable_skb(struct sk_buff *skb,
 						    struct net_device *dev)
 {
-	struct skb_shared_info *shinfo;
-	struct net_iov *niov;
+	struct net_devmem_dmabuf_binding *binding;
 
 	if (likely(skb_frags_readable(skb) ||
 		   dev->netmem_tx == NETMEM_TX_NO_DMA))
@@ -4179,14 +4178,11 @@ static struct sk_buff *validate_xmit_unreadable_skb(struct sk_buff *skb,
 	if (dev->netmem_tx == NETMEM_TX_NONE)
 		goto out_free;
 
-	shinfo = skb_shinfo(skb);
-
 	/* All net_iovs in an unreadable skb share the same binding. */
-	if (shinfo->nr_frags > 0) {
-		niov = netmem_to_net_iov(skb_frag_netmem(&shinfo->frags[0]));
-		if (net_is_devmem_iov(niov) &&
-		    (net_devmem_iov_binding(niov)->direction != DMA_TO_DEVICE ||
-		     READ_ONCE(net_devmem_iov_binding(niov)->dev) != dev))
+	if (skb_shinfo(skb)->nr_frags > 0) {
+		binding = net_devmem_skb_binding(skb);
+		if (!binding || binding->direction != DMA_TO_DEVICE ||
+		    READ_ONCE(binding->dev) != dev)
 			goto out_free;
 	}
 
diff --git a/net/core/devmem.h b/net/core/devmem.h
index c4f476947dfaa..9d7fc2914de14 100644
--- a/net/core/devmem.h
+++ b/net/core/devmem.h
@@ -10,6 +10,7 @@
 #ifndef _NET_DEVMEM_H
 #define _NET_DEVMEM_H
 
+#include <linux/skbuff.h>
 #include <net/net_namespace.h>
 #include <net/netmem.h>
 #include <net/netdev_netlink.h>
@@ -117,6 +118,21 @@ net_devmem_iov_binding(const struct net_iov *niov)
 			    struct net_devmem_dmabuf_binding, area);
 }
 
+static inline struct net_devmem_dmabuf_binding *
+net_devmem_skb_binding(const struct sk_buff *skb)
+{
+	struct net_iov *niov;
+
+	if (!skb_has_net_iovs(skb))
+		return NULL;
+
+	niov = skb_frag_net_iov(&skb_shinfo(skb)->frags[0]);
+	if (!niov || !net_is_devmem_iov(niov))
+		return NULL;
+
+	return net_devmem_iov_binding(niov);
+}
+
 static inline u32 net_devmem_iov_binding_id(const struct net_iov *niov)
 {
 	return net_devmem_iov_binding(niov)->id;
@@ -250,6 +266,12 @@ net_devmem_iov_binding(const struct net_iov *niov)
 {
 	return NULL;
 }
+
+static inline struct net_devmem_dmabuf_binding *
+net_devmem_skb_binding(const struct sk_buff *skb)
+{
+	return NULL;
+}
 #endif
 
 #endif /* _NET_DEVMEM_H */
diff --git a/net/core/gro.c b/net/core/gro.c
index 1b033855a20db..51a694b26401b 100644
--- a/net/core/gro.c
+++ b/net/core/gro.c
@@ -100,16 +100,7 @@ int skb_gro_receive(struct sk_buff *p, struct sk_buff *skb)
 	struct sk_buff *lp;
 	int segs;
 
-	/* Do not splice page pool based packets w/ non-page pool
-	 * packets. This can result in reference count issues as page
-	 * pool pages will not decrement the reference count and will
-	 * instead be immediately returned to the pool or have frag
-	 * count decremented.
-	 */
-	if (p->pp_recycle != skb->pp_recycle)
-		return -ETOOMANYREFS;
-
-	if (skb_zcopy(p) || skb_zcopy(skb))
+	if (!skb_frags_can_coalesce(p, skb))
 		return -ETOOMANYREFS;
 
 	if (unlikely(p->len + len >= netif_get_gro_max_size(p->dev, p) ||
@@ -158,7 +149,7 @@ int skb_gro_receive(struct sk_buff *p, struct sk_buff *skb)
 
 		NAPI_GRO_CB(skb)->free = NAPI_GRO_FREE;
 		goto done;
-	} else if (skb->head_frag) {
+	} else if (skb->head_frag && skb_frags_readable(skb)) {
 		int nr_frags = pinfo->nr_frags;
 		skb_frag_t *frag = pinfo->frags + nr_frags;
 		struct page *page = virt_to_head_page(skb->head);
@@ -233,6 +224,9 @@ int skb_gro_receive(struct sk_buff *p, struct sk_buff *skb)
 
 int skb_gro_receive_list(struct sk_buff *p, struct sk_buff *skb)
 {
+	if (!skb_mem_provider_compat(p, skb))
+		return -ETOOMANYREFS;
+
 	/* make sure to check flush flag and to not merge */
 	if (unlikely(p->len + skb->len >= 65536 ||
 		     NAPI_GRO_CB(skb)->flush))
diff --git a/net/core/net_test.c b/net/core/net_test.c
index 2b5080c226636..4a56b44f4addb 100644
--- a/net/core/net_test.c
+++ b/net/core/net_test.c
@@ -947,6 +947,9 @@ static void netmem_ref_test_net_iov(struct kunit *test)
 	copy = alloc_skb(0, GFP_KERNEL);
 	KUNIT_ASSERT_NOT_NULL(test, copy);
 	copy->pp_recycle = 1;
+	skb_fill_netmem_desc(copy, 0, netmem, 64, 16);
+	copy->len = 16;
+	copy->data_len = 16;
 	KUNIT_EXPECT_EQ(test, skb_shift(copy, skb, 16), 16);
 	KUNIT_EXPECT_EQ(test, atomic_long_read(&niov.desc.pp_ref_count), 3L);
 	KUNIT_EXPECT_EQ(test, atomic_long_read(&binding.ref.data->count),
@@ -1485,9 +1488,10 @@ static void netmem_ref_test_try_coalesce(struct kunit *test)
 	};
 	struct sk_buff *to, *from, *clone;
 	struct net_devmem_dmabuf_binding binding = {};
-	struct net_iov niov = {};
+	struct net_iov_area area2 = {};
+	struct net_iov niov = {}, niov2 = {};
 	struct page *pp_page, *reg_page;
-	netmem_ref pp_nm, niov_nm;
+	netmem_ref pp_nm, niov_nm, niov2_nm;
 	struct page_pool *pool;
 	long base_pp_ref, base_iov_ref;
 	int base_pp_page_ref, base_reg_ref;
@@ -1578,18 +1582,19 @@ static void netmem_ref_test_try_coalesce(struct kunit *test)
 	from = alloc_skb(0, GFP_KERNEL);
 	KUNIT_ASSERT_NOT_NULL(test, to);
 	KUNIT_ASSERT_NOT_NULL(test, from);
-	skb_put(to, skb_tailroom(to));
 	to->pp_recycle = 1;
-	to->unreadable = 1;
 	from->pp_recycle = 1;
-	skb_add_rx_frag_netmem(from, 0, niov_nm, 0, 64, PAGE_SIZE);
+	skb_add_rx_frag_netmem(to, 0, niov_nm, 0, 32, PAGE_SIZE);
+	napi_pp_get_page(niov_nm);
+	skb_add_rx_frag_netmem(from, 0, niov_nm, 32, 64, PAGE_SIZE);
 	napi_pp_get_page(niov_nm);
 
 	clone = skb_clone(from, GFP_KERNEL);
 	KUNIT_ASSERT_NOT_NULL(test, clone);
 	KUNIT_EXPECT_TRUE(test, skb_try_coalesce(to, from, &fragstolen,
 						 &delta));
-	KUNIT_EXPECT_EQ(test, atomic_long_read(&niov.desc.pp_ref_count), 4L);
+	KUNIT_EXPECT_TRUE(test, to->unreadable);
+	KUNIT_EXPECT_EQ(test, atomic_long_read(&niov.desc.pp_ref_count), 5L);
 	KUNIT_EXPECT_EQ(test, atomic_long_read(&binding.ref.data->count),
 			base_iov_ref);
 	consume_skb(from);
@@ -1597,6 +1602,31 @@ static void netmem_ref_test_try_coalesce(struct kunit *test)
 	consume_skb(to);
 	KUNIT_EXPECT_EQ(test, atomic_long_read(&niov.desc.pp_ref_count), 2L);
 
+	/* 4. Reject coalescing net_iov skbs from different bindings. */
+	net_iov_init(&niov2, &area2, NET_IOV_DMABUF);
+	niov2_nm = net_iov_to_netmem(&niov2);
+	netmem_set_pp(niov2_nm, pool);
+	netmem_or_pp_magic(niov2_nm, PP_SIGNATURE);
+	atomic_long_set(&niov2.desc.pp_ref_count, 2);
+	to = alloc_skb(0, GFP_KERNEL);
+	from = alloc_skb(0, GFP_KERNEL);
+	KUNIT_ASSERT_NOT_NULL(test, to);
+	KUNIT_ASSERT_NOT_NULL(test, from);
+	to->pp_recycle = 1;
+	from->pp_recycle = 1;
+	skb_add_rx_frag_netmem(to, 0, niov_nm, 0, 32, PAGE_SIZE);
+	napi_pp_get_page(niov_nm);
+	skb_add_rx_frag_netmem(from, 0, niov2_nm, 0, 32, PAGE_SIZE);
+	napi_pp_get_page(niov2_nm);
+	KUNIT_EXPECT_FALSE(test, skb_try_coalesce(to, from, &fragstolen,
+						  &delta));
+	consume_skb(from);
+	consume_skb(to);
+	KUNIT_EXPECT_EQ(test, atomic_long_read(&niov.desc.pp_ref_count), 2L);
+	KUNIT_EXPECT_EQ(test, atomic_long_read(&niov2.desc.pp_ref_count), 2L);
+	netmem_clear_pp_magic(niov2_nm);
+	netmem_set_pp(niov2_nm, NULL);
+
 	page_pool_put_full_page(pool, pp_page, false);
 	page_pool_put_full_page(pool, pp_page, false);
 	__free_page(reg_page);
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 43e720d30b67b..90d7691e5f545 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -4342,10 +4342,9 @@ int skb_shift(struct sk_buff *tgt, struct sk_buff *skb, int shiftlen)
 
 	if (skb_headlen(skb))
 		return 0;
-	if (skb_zcopy(tgt) || skb_zcopy(skb))
+	if (!skb_frags_can_coalesce(tgt, skb))
 		return 0;
 
-	DEBUG_NET_WARN_ON_ONCE(tgt->pp_recycle != skb->pp_recycle);
 	DEBUG_NET_WARN_ON_ONCE(skb_cmp_decrypted(tgt, skb));
 
 	todo = shiftlen;
@@ -6300,17 +6299,8 @@ bool skb_try_coalesce(struct sk_buff *to, struct sk_buff *from,
 	 * tricky (due to potential race with the clone disappearing) and
 	 * rare, so not worth dealing with.
 	 */
-	if (to->pp_recycle != from->pp_recycle)
-		return false;
-
-	/* All frags in an skb must either all be struct page-backed or all
-	 * belong to the same memory provider instance; do not coalesce skbs
-	 * that would mix them.
-	 */
-	if (skb_frags_readable(from) != skb_frags_readable(to))
-		return false;
-
-	if (len <= skb_tailroom(to) && skb_frags_readable(from)) {
+	if (len <= skb_tailroom(to) && to->pp_recycle == from->pp_recycle &&
+	    skb_frags_readable(to) && skb_frags_readable(from)) {
 		if (len) {
 			if (unlikely(skb_copy_bits(from, 0,
 						   skb_tail_pointer(to),
@@ -6324,17 +6314,21 @@ bool skb_try_coalesce(struct sk_buff *to, struct sk_buff *from,
 		return true;
 	}
 
+	if (!skb_frags_can_coalesce(to, from))
+		return false;
+
 	to_shinfo = skb_shinfo(to);
 	from_shinfo = skb_shinfo(from);
 	if (to_shinfo->frag_list || from_shinfo->frag_list)
 		return false;
-	if (skb_zcopy(to) || skb_zcopy(from))
-		return false;
 
 	if (skb_headlen(from) != 0) {
 		struct page *page;
 		unsigned int offset;
 
+		if (!skb_frags_readable(to) || !skb_frags_readable(from))
+			return false;
+
 		if (to_shinfo->nr_frags +
 		    from_shinfo->nr_frags >= MAX_SKB_FRAGS)
 			return false;
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index b9de7c302ba83..67651b1936a8d 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -1251,7 +1251,7 @@ int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size)
 		trace_tcp_sendmsg_locked(sk, msg, skb, size_goal);
 
 		if (copy <= 0 || !tcp_skb_can_collapse_to(skb) ||
-		    unlikely(skb_frags_readable(skb) != !binding)) {
+		    unlikely(net_devmem_skb_binding(skb) != binding)) {
 			bool first_skb;
 
 new_segment:
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index cf3ddc055d48d..b8a5c3556243b 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -2692,8 +2692,9 @@ static int tcp_clone_payload(struct sock *sk, struct sk_buff *to,
 				     probe_size - len);
 			len += todo;
 			skb_shinfo(to)->flags |= skb_shinfo(skb)->flags & SKBFL_SHARED_FRAG;
+			to->unreadable |= skb->unreadable;
 			if (lastfrag &&
-			    skb_frag_page(fragfrom) == skb_frag_page(lastfrag) &&
+			    skb_frag_netmem(fragfrom) == skb_frag_netmem(lastfrag) &&
 			    skb_frag_off(fragfrom) == skb_frag_off(lastfrag) +
 						      skb_frag_size(lastfrag)) {
 				skb_frag_size_add(lastfrag, todo);
-- 
2.56.0.455.g6c5a503b38-goog

Re: [PATCH net-next v4 2/2] net: prevent mixing frags from different memory providers or bindings

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message says the whole change is "theoretical", but
  one of the bundled fixes closes a bug that any devmem TX socket can…
- [Medium] skb_has_net_iovs() returns `skb->unreadable &&
  skb_shinfo(skb)->nr_frags`.

--

net: prevent mixing frags from different memory providers or bindings

The patch adds skb_has_net_iovs(), skb_net_iov_provider(),
skb_mem_provider_compat(), skb_frags_can_coalesce() and
net_devmem_skb_binding(). These are used in the coalesce, shift, GRO,
TCP collapse and devmem TX paths so that frags from different memory
providers or devmem bindings are never mixed in one skb. It also
propagates skb->unreadable and compares netmems when merging frags in
tcp_clone_payload().

> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 6787fcf6ac3e4..e6f63450a193a 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -1882,6 +1882,17 @@ static inline bool skb_frags_readable(const struct sk_buff *skb)
>  	return !skb->unreadable;
>  }
>  
> +/* Return true if frags in this skb are backed by net_iovs.
> + * Assumes skb->unreadable and net_iov-backed frags are equivalent for now;
> + * the two can be decoupled in the future if readable net_iovs in skbs are
> + * introduced. Also check nr_frags because not all frag-removal paths clear
> + * skb->unreadable when nr_frags drops to 0.
> + */
> +static inline bool skb_has_net_iovs(const struct sk_buff *skb)
> +{
> +	return skb->unreadable && skb_shinfo(skb)->nr_frags;
> +}

[Severity: Medium]

Does the nr_frags gate make the checks this patch replaces weaker, for
exactly the state the comment above describes?

An skb with unreadable=1 and nr_frags==0 is now classified like a
page-backed skb. So skb_mem_provider_compat() and skb_frags_can_coalesce()
report it as compatible with a readable skb. The checks being replaced
rejected that case:

skb_try_coalesce():
	if (skb_frags_readable(from) != skb_frags_readable(to))
		return false;

tcp_skb_can_collapse():
	skb_frags_readable(to) == skb_frags_readable(from)

tcp_sendmsg_locked():
	unlikely(skb_frags_readable(skb) != !binding)

Take skb_try_coalesce() with to stale-unreadable with 0 frags, and from
readable with only page frags and headlen 0. It skips the copy path and
passes skb_frags_can_coalesce(). It also misses the new readability check,
because that check only runs when skb_headlen(from) != 0. The page frags
are then copied into to, and to->unreadable stays set.

skb_shift() behaves the same way and only clears the source skb's
unreadable bit. In tcp_sendmsg_locked(), net_devmem_skb_binding(skb)
returns NULL for such a tail. With binding == NULL, page data is appended
to the tail instead of starting a new segment.

The resulting skb is flagged unreadable but holds struct page frags.
net_devmem_skb_binding() returns NULL for it, so would
validate_xmit_unreadable_skb() drop it on every transmit and retransmit?

	if (!binding || binding->direction != DMA_TO_DEVICE ||
	    READ_ONCE(binding->dev) != dev)
		goto out_free;

On RX, the dmabuf recvmsg path would reject the non-niov frags.

The stale state can be produced. __pskb_trim_head() in tcp_output.c
removes frags without clearing unreadable, and it is reached through
__tcp_retransmit_skb()->tcp_trim_head() for a FIN skb whose data has been
acked.

I could not find a live path where such an skb later becomes the target
of a coalesce, shift or sendmsg append, because that FIN skb is the last
one in its queue. So this may only weaken the old checks rather than
cause a reachable failure.

Would it make sense to keep rejecting a readable/unreadable mismatch in
these paths as well?

[ ... ]

> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index cf3ddc055d48d..b8a5c3556243b 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -2692,8 +2692,9 @@ static int tcp_clone_payload(struct sock *sk, struct sk_buff *to,
>  				     probe_size - len);
>  			len += todo;
>  			skb_shinfo(to)->flags |= skb_shinfo(skb)->flags & SKBFL_SHARED_FRAG;
> +			to->unreadable |= skb->unreadable;
>  			if (lastfrag &&
> -			    skb_frag_page(fragfrom) == skb_frag_page(lastfrag) &&
> +			    skb_frag_netmem(fragfrom) == skb_frag_netmem(lastfrag) &&
>  			    skb_frag_off(fragfrom) == skb_frag_off(lastfrag) +
>  						      skb_frag_size(lastfrag)) {
>  				skb_frag_size_add(lastfrag, todo);

[Severity: Medium]

The commit message says:

  This issue is theoretical and was found by manual code inspection and
  LLM review; it was not triggered in production.

Is that accurate for this tcp_clone_payload() change? It looks like it
fixes a reachable data corruption bug that any devmem TX socket can hit
when MTU probing is enabled.

Before this patch, the path was:

tcp_write_xmit()
  tcp_mtu_probe()
    tcp_can_coalesce_send_queue_head()
      tcp_skb_can_collapse()   <- only required equal readability
    tcp_clone_payload()

The probe nskb comes from tcp_stream_alloc_skb() with unreadable=0.
Nothing set the flag when net_iov frags were copied into it. So
validate_xmit_unreadable_skb() returned early without checking the
binding's direction or device. Readers gated only by skb_frags_readable(),
such as skb_copy_bits() for packet taps, could treat the net_iovs as
struct pages.

There is a second problem in the old merge test. skb_frag_page() returns
NULL for every net_iov frag, so the test was NULL == NULL. The frag
offset comes from the user iovec address in net_devmem_get_niov_at():

	*off = virt_addr % PAGE_SIZE;

Consider an iovec at dmabuf offset 0 with length 100 (niov 0, off 0),
followed by one at offset 2*PAGE_SIZE+100 (niov 2, off 100). The two
frags have contiguous offsets but sit on different niovs. They would be
merged into one 200 byte frag on niov 0. Bytes 100..199 of the probe
would then be sent from the wrong dmabuf page as valid TCP payload.

Since this puts wrong data on the wire, could the commit message describe
it accurately? Could this fix also be split into its own patch, so it is
easier to spot for stable backports?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261108013838.1454450-1-almasrymina%40google.com