Re: [PATCH bpf-next v2 1/2] bpf: drop duplicate check_app_limited in tcp_bpf_push

Before commit c5c37af6ecad9 ("tcp: Convert do_tcp_sendpages() to use
MSG_SPLICE_PAGES"), do_tcp_sendpages() did not call
tcp_rate_check_app_limited() internally, so callers needed an explicit
tcp_rate_check_app_limited() to cover it. That commit replaced
do_tcp_sendpages() with direct tcp_sendmsg_locked() calls, which always
provide the check internally. The outer call became redundant but was
left in place.

The site changed here, tcp_bpf_push(), is a MSG_SPLICE_PAGES loop that
holds the socket lock and only iterates when size > 0; tcp_sendmsg_locked()
is invoked on every iteration with state identical to what the outer call
sees, so dropping the outer call is safe and behavior-preserving.

A potential benefit of this change is that it facilitates future reuse of
tcp_bpf_push() for sockmap support in protocols beyond TCP, such as MPTCP.
Since tcp_rate_check_app_limited() is TCP-specific while sendmsg_locked()
is a generic interface in struct proto_ops, this change allows us to switch
to different protocols via sk->sk_socket->ops->sendmsg_locked() without
carrying protocol-specific assumptions.

Signed-off-by: Geliang Tang <redacted>

This isn't a bug, but the state-equivalence argument here looks right.
Would it be worth tightening "which always provide the check internally"
to something like "which performs the check on every path that queues
data"?

Looking at tcp_sendmsg_locked(), it reaches tcp_rate_check_app_limited()
only after the MSG_ZEROCOPY and MSG_FASTOPEN early returns:

net/ipv4/tcp.c:tcp_sendmsg_locked() {
    ...
		uarg = msg_zerocopy_realloc(...);
		if (!uarg) {
			err = -ENOBUFS;
			goto out_err;
		}
    ...
	err = tcp_sendmsg_fastopen(sk, msg, &copied_syn, size, uarg);
	if (err == -EINPROGRESS && copied_syn > 0)
		goto out;
	else if (err)
		goto out_err;
	}

	timeo = sock_sndtimeo(sk, flags & MSG_DONTWAIT);

	tcp_rate_check_app_limited(sk);  /* is sending application-limited? */
    ...
}

Those branches are reachable from tcp_bpf_push(), since tcp_bpf_sendmsg()
forwards the user's flags almost verbatim and tcp_bpf_push() passes them
on as `msghdr.msg_flags = flags | MSG_SPLICE_PAGES`, so MSG_ZEROCOPY and
MSG_FASTOPEN both reach tcp_sendmsg_locked(). On those paths the old code
updated tp->app_limited and the new code does not. Every one of them
returns without queueing SPLICE_PAGES data, so there is no functional
consequence -- but "always" is doing load-bearing work in the
justification, and a narrower phrasing would match the code.

Also, is the "only iterates when size > 0" clause needed? The internal
check at net/ipv4/tcp.c:1190 is called unconditionally regardless of
size, so the size > 0 property is not what makes the removal safe.


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/34964239423