Re: [RFC PATCH v2 12/25] net/core: Use per-node PISA allocators for skb_page_frag_refill()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential
issues to consider:
- [High] net: ip_make_skb() and ip6_make_skb() ignore sk_use_task_frag
  for PISA
- [Medium] net: skb_page_frag_refill_dev() checks ingress skb->dev on
  forwarded ESP packets
- [Medium] net: mismatched PISA and buddy page recycling in
  skb_page_frag_refill()
- [Medium] net: odd-offset checksum corruption in ping_getfrag() with
  PISA paged mode
- [Medium] net: plaintext TX data placed in PISA pages on MACsec in
  dst_tx_pisa_ok()
- [Low] iommu/pisa: netdev_tx_pisa_refresh() skips lower walk when
  dev.parent is set
--

commit 8f85f66c700b51ce7332c64edba9f65c4414c1f0
Author: Luigi Rizzo <lrizzo@google.com>

net/core: Use per-node PISA allocators for skb_page_frag_refill()

This commit adds per-task PISA page_frag support (current->task_frag_pisa)
and routes TX page_frag allocations to read-only PISA blocks when the
egress device has pisa_tx_bufs enabled.

> diff --git a/drivers/iommu/pisa-alloc.c b/drivers/iommu/pisa-alloc.c
> index 913706a503341..7b9d0a7acc101 100644
> --- a/drivers/iommu/pisa-alloc.c
> +++ b/drivers/iommu/pisa-alloc.c
> @@ -644,6 +644,63 @@ bool pisa_has_free_blocks_rx(const struct device *dev, unsigned int order, int n
[ ... ]
> +/* Slow path of __netdev_tx_pisa_ok(): refresh dev->pisa_tx_stamp. */
> +bool netdev_tx_pisa_refresh(struct net_device *dev, u32 now)
> +{
> +	bool ok;
> +
> +	if (dev->dev.parent) {
> +		ok = READ_ONCE(dev->dev.parent->pisa_tx_bufs);
> +	} else {
> +		rcu_read_lock();
> +		ok = netdev_walk_all_lower_dev_rcu(dev,
> +						   netdev_pisa_tx_bufs_walk,
> +						   NULL) != 0;
> +		rcu_read_unlock();
> +	}
> +	now |= ok ? PISA_TX_STAMP_OK : 0;
> +	WRITE_ONCE(dev->pisa_tx_stamp, now);
> +	return now & PISA_TX_STAMP_OK;
> +}

[Severity: Low]
Does checking dev->dev.parent before walking lower devices miss lower DMA
NICs under upper devices that set dev->dev.parent?

Stacked interfaces such as net_failover, hv_netvsc, and DSA user ports set
dev->dev.parent via SET_NETDEV_DEV() while linking the underlying PCI VF or
conduit NIC as a lower device.  When pisa_tx_bufs is enabled on that lower
device, netdev_tx_pisa_refresh() only reads the upper device's
dev->dev.parent->pisa_tx_bufs (false) and skips
netdev_walk_all_lower_dev_rcu(), leaving TX allocations on regular buddy
pages.

> diff --git a/include/net/sock.h b/include/net/sock.h
> index 173b37b2b8bb0..73906351d11db 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -2670,6 +2670,95 @@ static inline struct page_frag *sk_page_frag(struct sock *sk)
[ ... ]
> +/*
> + * May TX payload routed through @dst be copied into PISA pages? Those pages
> + * stay readable by every device that mapped them for as long as they are
> + * active, so only data that leaves the host in clear qualifies: IPsec payload
> + * is still plaintext here.
> + */
> +static inline bool dst_tx_pisa_ok(const struct dst_entry *dst)
> +{
> +	bool ok = false;
> +
> +	if (!net_pisa_tx_bufs_enabled() || !dst)
> +		return false;
> +
> +	rcu_read_lock();
> +	if (!dst_xfrm(dst))
> +		ok = __netdev_tx_pisa_ok(dst_dev_rcu(dst));
> +	rcu_read_unlock();
> +	return ok;
> +}

[Severity: Medium]
Can dst_tx_pisa_ok() place unencrypted plaintext into PISA pages when
routing over software MACsec?

A MACsec net_device has dev->dev.parent == NULL and links its physical NIC
as a lower device, so netdev_tx_pisa_refresh() propagates PISA_TX_STAMP_OK
from the NIC to the MACsec interface while !dst_xfrm(dst) is true.  Socket
send paths then copy plaintext into current->task_frag_pisa before
macsec_encrypt() runs, exposing cleartext in DMA-readable (and on CoCo
guests, shared/decrypted) PISA pages and holding it in tcp_rtx_queue until
ACKed.  In addition, forcing paged = true in __ip_append_data() and
__ip6_append_data() causes macsec_encrypt() -> skb_cow_data() to
immediately reallocate skb->head and linearize the fragments.

> diff --git a/net/core/sock.c b/net/core/sock.c
> index e2401d735c5a7..695fefe316925 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
[ ... ]
> @@ -3257,13 +3258,20 @@ DEFINE_STATIC_KEY_FALSE(net_high_order_alloc_disable_key);
>   */
>  bool skb_page_frag_refill(unsigned int sz, struct page_frag *pfrag, gfp_t gfp)
>  {
> +	/* PISA pages only go into current->task_frag_pisa, see sk_page_frag_dst(). */
> +	if (is_pisa_pfrag(pfrag))
> +		gfp |= __GFP_PISA_RO;
> +
>  	if (pfrag->page) {
> +		if ((gfp & __GFP_PISA_RO) && !pisa_page_can_recycle(pfrag->page))
> +			goto drop_page;
>  		if (page_ref_count(pfrag->page) == 1) {
>  			pfrag->offset = 0;
>  			return true;
>  		}
>  		if (pfrag->offset + sz <= pfrag->size)
>  			return true;
> +drop_page:
>  		put_page(pfrag->page);
>  	}

[Severity: Medium]
Does skb_page_frag_refill() recycle a page of the wrong type when
page_ref_count(pfrag->page) == 1?

pisa_page_can_recycle() returns true for non-PISA pages:

drivers/iommu/pisa-alloc.c:pisa_page_can_recycle() {
	if (!is_pisa_pfn(pfn))
		goto out;
    ...
}

If current->task_frag_pisa or x->xfrag caches a fallback buddy page (for
example after a high-order PISA pool miss or before pisa_tx_bufs was
enabled), skb_page_frag_refill() resets pfrag->offset = 0 whenever
page_ref_count(pfrag->page) == 1 and reuses the buddy page indefinitely
instead of allocating a PISA page.  Conversely, if x->xfrag holds a PISA
page and pisa_tx_bufs is later disabled on the egress device,
!(gfp & __GFP_PISA_RO) skips pisa_page_can_recycle() and keeps recycling
and pinning the 2MB PISA frame at refcount 1.

> @@ -3289,6 +3297,39 @@ bool skb_page_frag_refill(unsigned int sz, struct page_frag *pfrag, gfp_t gfp)
>  }
>  EXPORT_SYMBOL(skb_page_frag_refill);
>  
> +/**
> + * skb_page_frag_refill_dev - GFP_ATOMIC skb_page_frag_refill() for @skb TX
> + * @sz: minimum size of the fragment we want to get
> + * @pfrag: pointer to page_frag, private to the caller
> + * @skb: packet being transmitted
> + *
> + * Pages come from the per-node DMA_TO_DEVICE PISA allocators when they are
> + * useful for @skb's egress device (__netdev_tx_pisa_ok()), otherwise from the
> + * buddy allocator. PISA pages are readable by every device that maps them
> + * while they stay active, so @pfrag must only ever hold data meant for devices,
> + * e.g. ciphertext; it may mix PISA and ordinary pages.
> + */
> +bool skb_page_frag_refill_dev(unsigned int sz, struct page_frag *pfrag,
> +			      const struct sk_buff *skb)
> +{
> +	gfp_t gfp = GFP_ATOMIC;
> +
> +	/*
> +	 * Before ip_finish_output2() populates skb->dev (e.g. esp_output),
> +	 * the egress device is carried by skb_dst(skb).
> +	 */
> +	if (net_pisa_tx_bufs_enabled()) {
> +		const struct dst_entry *dst = skb_dst(skb);
> +
> +		rcu_read_lock();
> +		if (__netdev_tx_pisa_ok(skb->dev ?: (dst ? dst_dev_rcu(dst) : NULL)))
> +			gfp |= __GFP_PISA_RO;
> +		rcu_read_unlock();
> +	}
> +	return skb_page_frag_refill(sz, pfrag, gfp);
> +}
> +EXPORT_SYMBOL(skb_page_frag_refill_dev);

[Severity: Medium]
On forwarded packets entering esp_output() or esp6_output(), doesn't
skb->dev still point to the ingress device?

In ip_forward() -> xfrm4_output() -> esp_output() (and the IPv6 equivalent),
skb->dev is not updated to the egress device until ip_output() runs after
xfrm_output() finishes.  Preferring skb->dev over dst_dev_rcu(dst) checks
pisa_tx_bufs on the ingress interface instead of the egress interface for
all forwarded ESP traffic.

> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
> index 3396f177a18d0..a5ea530bc8b5b 100644
> --- a/net/ipv4/ip_output.c
> +++ b/net/ipv4/ip_output.c
> @@ -986,7 +986,10 @@ static int __ip_append_data(struct sock *sk,
>  
>  	exthdrlen = !skb ? rt->dst.header_len : 0;
>  	mtu = cork->gso_size ? IP_MAX_MTU : cork->fragsize;
> -	paged = !!cork->gso_size;
> +	paged = !!cork->gso_size ||
> +		(sk->sk_type != SOCK_RAW &&
> +		 (rt->dst.dev->features & NETIF_F_SG) &&
> +		 is_pisa_pfrag(pfrag));

[Severity: Medium]
Can setting paged = true for all sk->sk_type != SOCK_RAW sockets corrupt
IPv4 ICMP checksums on SOCK_DGRAM ping sockets?

IPv4 ping sockets have sk->sk_type == SOCK_DGRAM and use ping_getfrag():

net/ipv4/ping.c:ping_getfrag() {
	if (!csum_and_copy_from_iter_full(to, fraglen, &pfh->wcheck,
					  &pfh->msg->msg_iter))
		return -EFAULT;

#if IS_ENABLED(CONFIG_IPV6)
	if (pfh->family == AF_INET6) {
		skb->csum = csum_block_add(skb->csum, pfh->wcheck, odd);
		skb->ip_summed = CHECKSUM_NONE;
		pfh->wcheck = 0;
	}
#endif
	return 0;
}

When paged is true and a ping payload crosses the end of
current->task_frag_pisa.page starting from an odd pfrag->offset, the first
chunk has an odd length (pfrag->size - pfrag->offset) and the second
ping_getfrag() call starts at an odd packet offset.  Because the AF_INET
path accumulates directly into pfh->wcheck with progress = 0 and ignores
odd, the second chunk's bytes are added with inverted byte lane alignment,
producing an invalid ICMPv4 checksum.

[ ... ]

> @@ -1592,7 +1595,7 @@ struct sk_buff *ip_make_skb(struct sock *sk,
>  		return ERR_PTR(err);
>  
>  	err = __ip_append_data(sk, fl4, &queue, cork,
> -			       &current->task_frag, getfrag,
> +			       task_page_frag(cork->dst), getfrag,
>  			       from, length, transhdrlen, flags);
>  	if (err) {
>  		__ip_flush_pending_frames(sk, &queue, cork);

[Severity: High]
Can ip_make_skb() and ip6_make_skb() re-enter current->task_frag_pisa and
cause a use-after-free when a socket has sk_use_task_frag == false?

In-kernel sockets that transmit during memory reclaim or page faults (such
as SUNRPC/NFS over UDP, which sets sk->sk_use_task_frag = false in
xs_udp_finish_connecting()) call ip_make_skb() and ip6_make_skb().  Before
this patch, non-GSO sends in __ip_append_data() and __ip6_append_data() had
paged = false, so normal RPC datagrams never used pfrag.  Now ip_make_skb()
and ip6_make_skb() pass task_page_frag() without checking
sk->sk_use_task_frag, and __ip_append_data() / __ip6_append_data() force
paged = true when is_pisa_pfrag(pfrag) is true:

tcp_sendmsg_locked()
  pfrag = sk_page_frag_dst(sk, dst)  /* &current->task_frag_pisa */
  sk_page_frag_refill(sk, pfrag)
  skb_copy_to_page_nocache(..., pfrag->page, pfrag->offset, copy)
    <page fault on user buffer / direct reclaim>
      xs_udp_send_request()          /* sk->sk_use_task_frag == false */
        ip_make_skb()
          __ip_append_data(..., task_page_frag(cork->dst), ...)
            skb_page_frag_refill(..., pfrag, ...)
              put_page(pfrag->page)  /* frees outer page at refcount 1 */
  /* writes to freed pfrag->page, or corrupts pfrag->offset */

Passing sk_page_frag_dst(sk, ...) in ip_make_skb() and ip6_make_skb() would
respect sk->sk_use_task_frag and use &sk->sk_frag instead.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109182533.3594984-1-lrizzo@google.com?part=12