On 5/2/25 9:20 PM, Mina Almasry wrote:
On Fri, May 2, 2025 at 4:47 AM Paolo Abeni pabeni@redhat.com wrote:
@@ -1078,7 +1092,8 @@ int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size) zc = MSG_ZEROCOPY; } else if (sock_flag(sk, SOCK_ZEROCOPY)) { skb = tcp_write_queue_tail(sk);
uarg = msg_zerocopy_realloc(sk, size, skb_zcopy(skb));
uarg = msg_zerocopy_realloc(sk, size, skb_zcopy(skb),sockc_valid && !!sockc.dmabuf_id);If sock_cmsg_send() failed and the user did not provide a dmabuf_id, memory accounting will be incorrect.
Forgive me but I don't see it. sockc_valid will be false, so msg_zerocopy_realloc will do the normal MSG_ZEROCOPY accounting. Then below whech check sockc_valid in place of where we did the sock_cmsg_send before, and goto err. I assume the goto err should undo the memory accounting in the new code as in the old code. Can you elaborate on the bug you see?
Uhm, I think I misread the condition used for msg_zerocopy_realloc() last argument.
Re-reading it now it the problem I see is that if sock_cmsg_send() fails after correctly setting 'dmabuf_id', msg_zerocopy_realloc() will account the dmabuf memory, which looks unexpected.
Somewhat related, I don't see any change to the msg_zerocopy/ubuf complete/cleanup path(s): what will happen to the devmem ubuf memory at uarg->complete() time? It looks like it will go unexpectedly through mm_unaccount_pinned_pages()???
Thanks,
Paolo