On 5/2/25 9:20 PM, Mina Almasry wrote:
> On Fri, May 2, 2025 at 4:47 AM Paolo Abeni <pab...@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
