| Message ID | 20241021013705.14105-1-mrpre@163.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | bpf: fix filed access without lock | expand |
On Mon, Oct 21, 2024 at 09:37:05AM +0800, mrpre wrote: > The tcp_bpf_recvmsg_parser() function, running in user context, > retrieves seq_copied from tcp_sk without holding the socket lock, and > stores it in a local variable seq. However, the softirq context can > modify tcp_sk->seq_copied concurrently, for example, n tcp_read_sock(). > > As a result, the seq value is stale when it is assigned back to > tcp_sk->copied_seq at the end of tcp_bpf_recvmsg_parser(), leading to > incorrect behavior. Good catch! This makes sense to me. Mind to be more specific on the "incorrect behavior" here? What error or misbehavior did you see? > > Signed-off-by: mrpre <mrpre@163.com> Please use your real name for SoB, see https://docs.kernel.org/process/submitting-patches.html Thanks.
diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c index e7658c5d6b79..7b44d4ece8b2 100644 --- a/net/ipv4/tcp_bpf.c +++ b/net/ipv4/tcp_bpf.c @@ -221,9 +221,9 @@ static int tcp_bpf_recvmsg_parser(struct sock *sk, int flags, int *addr_len) { - struct tcp_sock *tcp = tcp_sk(sk); + struct tcp_sock *tcp; + u32 seq; int peek = flags & MSG_PEEK; - u32 seq = tcp->copied_seq; struct sk_psock *psock; int copied = 0; @@ -238,7 +238,8 @@ static int tcp_bpf_recvmsg_parser(struct sock *sk, return tcp_recvmsg(sk, msg, len, flags, addr_len); lock_sock(sk); - + tcp = tcp_sk(sk); + seq = tcp->copied_seq; /* We may have received data on the sk_receive_queue pre-accept and * then we can not use read_skb in this context because we haven't * assigned a sk_socket yet so have no link to the ops. The work-around
The tcp_bpf_recvmsg_parser() function, running in user context, retrieves seq_copied from tcp_sk without holding the socket lock, and stores it in a local variable seq. However, the softirq context can modify tcp_sk->seq_copied concurrently, for example, n tcp_read_sock(). As a result, the seq value is stale when it is assigned back to tcp_sk->copied_seq at the end of tcp_bpf_recvmsg_parser(), leading to incorrect behavior. Signed-off-by: mrpre <mrpre@163.com> --- net/ipv4/tcp_bpf.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)