This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
net: tcp: block mixing readable and unreadable frags
This adds a check in tcp_sendmsg_locked() so an existing tail SKB is not
collapsed into when its readability does not match the presence of a
devmem binding, forcing a new segment instead.
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 455441f1b6949..b4237d0e994d6 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -1240,7 +1240,8 @@ 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)) {
> + if (copy <= 0 || !tcp_skb_can_collapse_to(skb) ||
> + unlikely(skb_frags_readable(skb) != !binding)) {
> bool first_skb;
>
> new_segment:
[Severity: Medium]
Is binding alone the right proxy here for "this send will append
unreadable frags"?
What actually selects the devmem filler is the zc dispatch in the loop
plus __zerocopy_sg_from_iter():
net/core/datagram.c:__zerocopy_sg_from_iter() {
...
else if (binding)
ret = zerocopy_fill_skb_from_devmem(skb, from, length, binding);
else
ret = zerocopy_fill_skb_from_iter(skb, from, length);
}
and that is only reached from the zc == MSG_ZEROCOPY arm of
tcp_sendmsg_locked().
Can binding != NULL coexist with zc == 0? In tcp_sendmsg_locked() the
binding is taken whenever the cmsg carries dmabuf_id:
if (sk->sk_route_caps & NETIF_F_SG)
zc = MSG_ZEROCOPY;
else
uarg_to_msgzc(uarg)->zerocopy = 0;
if (!sockc_err && sockc.dmabuf_id) {
binding = net_devmem_get_binding(sk, sockc.dmabuf_id);
while zc is only set under NETIF_F_SG. The one dmabuf_id validation
does not look at SG at all:
if (!sockc_err && sockc.dmabuf_id &&
(!(flags & MSG_ZEROCOPY) || !sock_flag(sk, SOCK_ZEROCOPY))) {
err = -EINVAL;
And sk_setup_caps() in net/core/sock.c only force-adds NETIF_F_SG when
sk_can_gso() holds:
if (unlikely(sk->sk_gso_disabled))
sk->sk_route_caps &= ~NETIF_F_GSO_MASK;
if (sk_can_gso(sk)) {
if (dst->header_len && !xfrm_dst_offload_ok(dst)) {
sk->sk_route_caps &= ~NETIF_F_GSO_MASK;
} else {
sk->sk_route_caps |= NETIF_F_SG | NETIF_F_HW_CSUM;
so a TCP-MD5/TCP-AO socket (sk_gso_disable()) or an IPsec dst on a device
with sg turned off ends up with binding != NULL and zc == 0.
In that state, does the new predicate go wrong in both directions?
If the tail is readable or newly allocated, skb_frags_readable(skb) !=
!binding evaluates to true != false, so every iteration takes
goto new_segment and the write is split into one skb per iteration, even
though the zc == 0 path only appends readable page frags via
skb_fill_page_desc().
If the tail is already unreadable from an earlier devmem send, the test
is false != false, collapsing is allowed, and skb_fill_page_desc() adds a
readable page frag to an skb that __skb_fill_netmem_desc() already marked
unreadable = 1 — the mixing this patch aims to prevent. Would
validate_xmit_unreadable_skb() catch that? It only looks at the first
frag:
net/core/dev.c:validate_xmit_unreadable_skb() {
if (shinfo->nr_frags > 0) {
niov = netmem_to_net_iov(skb_frag_netmem(&shinfo->frags[0]));
Would keying the new clause on binding && zc == MSG_ZEROCOPY, or
rejecting dmabuf_id when the route cannot do SG, line the predicate up
with the code that actually fills the frags?
Note the mixing described in the second direction pre-dates this patch
(there was no check at all before); the incorrect predicate and the
forced re-segmentation are new. Reaching this also needs a local
privileged setup (dmabuf TX binding plus MD5/AO or IPsec plus an ethtool
feature change), and in that configuration a devmem send through the copy
path would likely already fail in skb_copy_to_page_nocache() since the
iovec holds dmabuf offsets rather than user addresses, so this is stated
as an observation rather than a demonstrated corruption path.
--
This is an AI-generated review.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
