quic_sendmsg memory handling/accounting

[metze-samba]

I just looked at quic_sendmsg() and notices that quic_frame_create() is used before if (sk_stream_wspace(sk) < len || !sk_wmem_schedule(sk, len)), which means we already allocated and copied the memory before checking the send buffer is large enough.

Is that correct or should be be changed and use something like this

 modules/net/quic/socket.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/modules/net/quic/socket.c b/modules/net/quic/socket.c
index 99f0479defeb..54d1ced1078e 100644
--- a/modules/net/quic/socket.c
+++ b/modules/net/quic/socket.c
@@ -706,15 +706,7 @@ static int quic_sendmsg(struct sock *sk, struct msghdr *msg, size_t msg_len)
 		msginfo.level = hinfo.crypto_level;
 		msginfo.msg = &msg->msg_iter;
 		while (iov_iter_count(&msg->msg_iter) > 0) {
-			frame = quic_frame_create(sk, QUIC_FRAME_CRYPTO, &msginfo);
-			if (!frame) {
-				if (!bytes) {
-					err = -ENOMEM;
-					goto err;
-				}
-				goto out;
-			}
-			len = frame->bytes;
+			len = iov_iter_count(msginfo.msg);
 			if (sk_stream_wspace(sk) < len || !sk_wmem_schedule(sk, len)) {
 				if (delay) {
 					quic_outq_set_force_delay(outq, 0);
@@ -722,12 +714,19 @@ static int quic_sendmsg(struct sock *sk, struct msghdr *msg, size_t msg_len)
 				}
 				err = quic_wait_for_send(sk, flags, len);
 				if (err) {
-					quic_frame_put(frame);
 					if (err == -EPIPE || !bytes)
 						goto err;
 					goto out;
 				}
 			}
+			frame = quic_frame_create(sk, QUIC_FRAME_CRYPTO, &msginfo);
+			if (!frame) {
+				if (!bytes) {
+					err = -ENOMEM;
+					goto err;
+				}
+				goto out;
+			}
 			bytes += frame->bytes;
 			quic_outq_set_force_delay(outq, delay);
 			quic_outq_ctrl_tail(sk, frame, delay);

And something similar for the MSG_DATAGRAM case.

[lxin]

I just looked at quic_sendmsg() and notices that quic_frame_create() is used before if (sk_stream_wspace(sk) < len || !sk_wmem_schedule(sk, len)), which means we already allocated and copied the memory before checking the send buffer is large enough.

Is that correct or should be be changed and use something like this

Currently, quic_frame_create() is called before the check because the message length to send (based on the MSS) is calculated in quic_frame_crypto_create() each round in the loop, not iov_iter_count(msginfo.msg) .

But you're right, it would be an improvement to move quic_frame_create() after the check. However, to support this change, a few points need to be addressed:

1. You should use len = 1 when doing the wspace & wmem check. This allows sending data even if only 1 byte of wspace & wmem is available. Using len = iov_iter_count(msginfo.msg) would require waiting until the entire message fits, which isn’t ideal.
2. With this change, the available wspace (sk_stream_wspace(sk)) should be considered during message length calculation inside quic_frame_crypto_create(), like:

        if (msg_len > wspace)
                msg_len = wspace;
        if (msg_len > max_frame_len - hlen)
                msg_len = max_frame_len - hlen;

3. Since the exact length isn't available before quic_frame_create(), you’ll need to call sk_wmem_schedule() again after quic_frame_create() with the actual frame->bytes (NOTE, you don't need to check sk_stream_wspace() again, as wspace is already considered in the message length calculation). If that second check fails, you should revert the iterator and free the frame (with the actual len (frame->bytes), not 1), like this:

                        len = frame->bytes;
                        if (!sk_wmem_schedule(sk, len)) {
                                iov_iter_revert(msginfo.msg, len);
                                quic_frame_put(frame);
                                continue;
                        }
                        len = 1;

This pattern is already used in the loop for sending stream data in quic_sendmsg(), you can refer to it for the change.

Regarding MSG_DATAGRAM part, since QUIC (unreliable) datagram is not allowed to be fragmented, it’s safe to directly move quic_frame_create() after the check as your patch does.

Thanks.