Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 0 · Low: 1
- [Low] "rxrpc: Fix sendmsg length" carries a `Fixes:` tag and `cc:
stable@vger.kernel.org` while its own commit message states…
--
rxrpc: Fix sendmsg length
This makes rxrpc_send_data() consistently use its len argument rather than
msg->msg_iter.count, decrementing len as data is copied, and limits each
copy to the smaller of len and msg_data_left(msg).
quoted hunk ↗ jump to hunk
diff --git a/net/rxrpc/sendmsg.c b/net/rxrpc/sendmsg.c
index 1d66e98081624..565799548102c 100644
--- a/net/rxrpc/sendmsg.c
+++ b/net/rxrpc/sendmsg.c
@@ -379,9 +379,9 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
ret = -EMSGSIZE;
if (call->tx_total_len != -1) {
- if (len - copied > call->tx_total_len)
+ if (len > call->tx_total_len)
goto maybe_error;
- if (!more && len - copied != call->tx_total_len)
+ if (!more && len != call->tx_total_len)
goto maybe_error;
}
[Severity: Low]
This isn't a bug in the code, but a question about the tagging. The
commit carries:
Fixes: 382d7974de31 ("RxRPC: Use iov_iter_count() in rxrpc_send_data() instead of the len argument")
cc: stable@vger.kernel.org
while the message itself says:
Note that, currently, all the callers have len and msg->msg_iter.count the
same and so the problem won't occur.
Walking the callers agrees with that statement. rxrpc_do_sendmsg()
forwards the sendmsg() len that the syscall also used to build msg_iter.
In fs/afs/rxrpc.c, afs_make_call() and afs_send_simple_reply() pass
msg_data_left(&msg):
ret = rxrpc_kernel_send_data(call->net->socket, rxcall, &msg,
msg_data_left(&msg),
afs_notify_end_request_tx);
and afs_send_empty_reply() passes 0 for a zero-length iterator. In
net/rxrpc/rxperf.c, rxperf_process_call() passes the same local len used
to build the iterator:
iov_iter_bvec(&msg.msg_iter, WRITE, &bv, 1, len);
msg.msg_flags = MSG_MORE;
n = rxrpc_kernel_send_data(rxperf_socket, call->rxcall, &msg,
len, rxperf_notify_end_reply_tx);
So no in-tree caller can diverge. Given there is no symptom, reproducer
or affected caller named, are the Fixes: and cc: stable tags intended
here?
What stable trees would pick up instead is a rewrite of the sendmsg data
loop: len becomes mutable and is decremented in the loop, the copy bound
becomes min3(), the loop termination test gains a second clause, and the
-EMSGSIZE re-check that is re-executed via the reload label now tests the
decremented len instead of len - copied.
quoted hunk ↗ jump to hunk
@@ -405,7 +405,7 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
* the security header is going to be in the padded
* region (enc blocksize), but the trailer is not.
*/
- remain = more ? INT_MAX : msg_data_left(msg);
+ remain = more ? INT_MAX : len;
txb = call->conn->security->alloc_txbuf(call, remain, sk->sk_allocation);
if (!txb) {
ret = -ENOMEM;@@ -416,8 +416,8 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
_debug("append");
/* append next segment of data to the current buffer */
- if (msg_data_left(msg) > 0) {
- size_t copy = umin(txb->space, msg_data_left(msg));
+ if (len > 0) {
+ size_t copy = min3(txb->space, len, msg_data_left(msg));
_debug("add %zu", copy);
if (!copy_from_iter_full(txb->data + txb->offset,@@ -428,6 +428,7 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
txb->len += copy;
txb->offset += copy;
copied += copy;
+ len -= copy;
if (call->tx_total_len != -1)
call->tx_total_len -= copy;
}
@@ -439,8 +440,8 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
/* add the packet to the send queue if it's now full */
if (!txb->space ||
- (msg_data_left(msg) == 0 && !more)) {
- if (msg_data_left(msg) == 0 && !more)
+ (len == 0 && !more)) {
+ if (len == 0 && !more)
txb->flags |= RXRPC_LAST_PACKET;
ret = call->security->secure_packet(call, txb);@@ -449,7 +450,7 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
rxrpc_queue_packet(rx, call, txb, notify_end_tx);
txb = NULL;
}
- } while (msg_data_left(msg) > 0);
+ } while (len > 0 && msg_data_left(msg) > 0);
Making len mutable also appears to be a prerequisite for the later patch
in the series, "rxrpc: Fix packet encryption error handling", whose rewind
path tests:
if (copied && !more && !len) {
which only behaves as intended once len is decremented here. Could that
dependency be mentioned in the commit message?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260907113743.1453210-1-dhowells%40redhat.com