| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <pxqoucw6bdnchyfvgyfpxaxnjwbf3t6zzaliyoul5vwn3ycqpd@jtv52cji6b4k>
Date: Wed, 15 Jan 2025 17:04:31 +0800
From: Jiayuan Chen <mrpre@....com>
To: bpf@...r.kernel.org, jakub@...udflare.com, john.fastabend@...il.com
Cc: netdev@...r.kernel.org, martin.lau@...ux.dev, ast@...nel.org,
edumazet@...gle.com, davem@...emloft.net, dsahern@...nel.org, kuba@...nel.org,
pabeni@...hat.com, linux-kernel@...r.kernel.org, song@...nel.org, andrii@...nel.org,
mhal@...x.co, yonghong.song@...ux.dev, daniel@...earbox.net,
xiyou.wangcong@...il.com, horms@...nel.org, corbet@....net, eddyz87@...il.com,
cong.wang@...edance.com, shuah@...nel.org, mykolal@...com, jolsa@...nel.org,
haoluo@...gle.com, sdf@...ichev.me, kpsingh@...nel.org, linux-doc@...r.kernel.org
Subject: Re: [PATCH bpf v6 1/3] bpf: fix wrong copied_seq calculation
On Tue, Jan 14, 2025 at 09:23:09PM +0800, Jiayuan Chen wrote:
> --- a/include/net/strparser.h
> +++ b/include/net/strparser.h
> @@ -43,6 +43,8 @@ struct strparser;
> struct strp_callbacks {
> int (*parse_msg)(struct strparser *strp, struct sk_buff *skb);
> void (*rcv_msg)(struct strparser *strp, struct sk_buff *skb);
> + int (*read_sock)(struct strparser *strp, read_descriptor_t *desc,
> + sk_read_actor_t recv_actor);
> int (*read_sock_done)(struct strparser *strp, int err);
> void (*abort_parser)(struct strparser *strp, int err);
> void (*lock)(struct strparser *strp);
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index e9b37b76e894..5a40bea15016 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -729,6 +729,9 @@ void tcp_get_info(struct sock *, struct tcp_info *);
> /* Read 'sendfile()'-style from a TCP socket */
> int tcp_read_sock(struct sock *sk, read_descriptor_t *desc,
> sk_read_actor_t recv_actor);
> +int tcp_read_sock_noack(struct sock *sk, read_descriptor_t *desc,
> + sk_read_actor_t recv_actor, bool noack,
> + u32 *copied_seq);
> int tcp_read_skb(struct sock *sk, skb_read_actor_t recv_actor);
> struct sk_buff *tcp_recv_skb(struct sock *sk, u32 seq, u32 *off);
> void tcp_read_done(struct sock *sk, size_t len);
> @@ -2609,6 +2612,11 @@ static inline void tcp_eat_skb(struct sock *sk, struct sk_buff *skb)
> }
> #endif
>
> +#ifdef CONFIG_BPF_STREAM_PARSER
> +int tcp_bpf_strp_read_sock(struct sock *sk, read_descriptor_t *desc,
> + sk_read_actor_t recv_actor);
> +#endif /* CONFIG_BPF_STREAM_PARSER */
> +
> int tcp_bpf_sendmsg_redir(struct sock *sk, bool ingress,
> struct sk_msg *msg, u32 bytes, int flags);
> #endif /* CONFIG_NET_SOCK_MSG */
> diff --git a/net/core/skmsg.c b/net/core/skmsg.c
> index 61f3f3d4e528..2e404caf10f2 100644
> --- a/net/core/skmsg.c
> +++ b/net/core/skmsg.c
> @@ -548,7 +548,9 @@ static int sk_psock_skb_ingress_enqueue(struct sk_buff *skb,
> if (unlikely(num_sge < 0))
> return num_sge;
> }
> -
> +#if IS_ENABLED(CONFIG_BPF_STREAM_PARSER)
> + psock->ingress_bytes += len;
> +#endif
> copied = len;
> msg->sg.start = 0;
> msg->sg.size = copied;
> @@ -1092,6 +1094,20 @@ static int sk_psock_strp_read_done(struct strparser *strp, int err)
> return err;
> }
>
> +static int sk_psock_strp_read_sock(struct strparser *strp,
> + read_descriptor_t *desc,
> + sk_read_actor_t recv_actor)
> +{
> + struct sock *sk = strp->sk;
> +
> + if (WARN_ON(!sk_is_tcp(sk))) {
> + desc->error = -EINVAL;
> + return 0;
> + }
> +
> + return tcp_bpf_strp_read_sock(sk, desc, recv_actor);
> +}
> +
well, reply to myself. This code doesn't feel right if we want support
other stream socket like unix, which has been supported by stream_verdict.
Still an abstraction should be used. Should we just add a handler like
'read_sock_nock' into 'struct proto_ops' like read_skb or read_sock does,
without some replacement action which introduced by v1.
or just add handler into 'struct psock' if we do not want to break
'struct proto_ops' definition.
CC jakub.
> static int sk_psock_strp_parse(struct strparser *strp, struct sk_buff *skb)
> {
> struct sk_psock *psock = container_of(strp, struct sk_psock, strp);
> @@ -1136,6 +1152,7 @@ int sk_psock_init_strp(struct sock *sk, struct sk_psock *psock)
>
> static const struct strp_callbacks cb = {
> .rcv_msg = sk_psock_strp_read,
> + .read_sock = sk_psock_strp_read_sock,
> .read_sock_done = sk_psock_strp_read_done,
> .parse_msg = sk_psock_strp_parse,
> };
> @@ -1144,6 +1161,9 @@ int sk_psock_init_strp(struct sock *sk, struct sk_psock *psock)
> if (!ret)
> sk_psock_set_state(psock, SK_PSOCK_RX_STRP_ENABLED);
>
> + if (sk_is_tcp(sk))
> + psock->copied_seq = tcp_sk(sk)->copied_seq;
> +
> return ret;
> }
>
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 0d704bda6c41..285678d8ce07 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -1565,12 +1565,13 @@ EXPORT_SYMBOL(tcp_recv_skb);
> * or for 'peeking' the socket using this routine
> * (although both would be easy to implement).
> */
> -int tcp_read_sock(struct sock *sk, read_descriptor_t *desc,
> - sk_read_actor_t recv_actor)
> +static int __tcp_read_sock(struct sock *sk, read_descriptor_t *desc,
> + sk_read_actor_t recv_actor, bool noack,
> + u32 *copied_seq)
> {
> struct sk_buff *skb;
> struct tcp_sock *tp = tcp_sk(sk);
> - u32 seq = tp->copied_seq;
> + u32 seq = *copied_seq;
> u32 offset;
> int copied = 0;
>
> @@ -1624,9 +1625,12 @@ int tcp_read_sock(struct sock *sk, read_descriptor_t *desc,
> tcp_eat_recv_skb(sk, skb);
> if (!desc->count)
> break;
> - WRITE_ONCE(tp->copied_seq, seq);
> + WRITE_ONCE(*copied_seq, seq);
> }
> - WRITE_ONCE(tp->copied_seq, seq);
> + WRITE_ONCE(*copied_seq, seq);
> +
> + if (noack)
> + goto out;
>
> tcp_rcv_space_adjust(sk);
>
> @@ -1635,10 +1639,25 @@ int tcp_read_sock(struct sock *sk, read_descriptor_t *desc,
> tcp_recv_skb(sk, seq, &offset);
> tcp_cleanup_rbuf(sk, copied);
> }
> +out:
> return copied;
> }
> +
> +int tcp_read_sock(struct sock *sk, read_descriptor_t *desc,
> + sk_read_actor_t recv_actor)
> +{
> + return __tcp_read_sock(sk, desc, recv_actor, false,
> + &tcp_sk(sk)->copied_seq);
> +}
> EXPORT_SYMBOL(tcp_read_sock);
>
> +int tcp_read_sock_noack(struct sock *sk, read_descriptor_t *desc,
> + sk_read_actor_t recv_actor, bool noack,
> + u32 *copied_seq)
> +{
> + return __tcp_read_sock(sk, desc, recv_actor, noack, copied_seq);
> +}
> +
> int tcp_read_skb(struct sock *sk, skb_read_actor_t recv_actor)
> {
> struct sk_buff *skb;
> diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> index 47f65b1b70ca..d303486bfc59 100644
> --- a/net/ipv4/tcp_bpf.c
> +++ b/net/ipv4/tcp_bpf.c
> @@ -698,3 +698,44 @@ void tcp_bpf_clone(const struct sock *sk, struct sock *newsk)
> newsk->sk_prot = sk->sk_prot_creator;
> }
> #endif /* CONFIG_BPF_SYSCALL */
> +
> +#if IS_ENABLED(CONFIG_BPF_STREAM_PARSER)
> +int tcp_bpf_strp_read_sock(struct sock *sk, read_descriptor_t *desc,
> + sk_read_actor_t recv_actor)
> +{
> + struct sk_psock *psock;
> + struct tcp_sock *tp;
> + int copied = 0;
> +
> + tp = tcp_sk(sk);
> + rcu_read_lock();
> + psock = sk_psock(sk);
> + if (WARN_ON(!psock)) {
> + desc->error = -EINVAL;
> + goto out;
> + }
> +
> + psock->ingress_bytes = 0;
> + /* We could easily add copied_seq and noack into desc then call
> + * ops->read_sock without calling symbol directly. But unfortunately
> + * most descriptors used by other modules are not inited with zero.
> + * Also replacing ops->read_sock can't be workd without introducing
> + * new ops as ops itself is located in rodata segment.
> + */
> + copied = tcp_read_sock_noack(sk, desc, recv_actor, true,
> + &psock->copied_seq);
> + if (copied < 0)
> + goto out;
> + /* recv_actor may redirect skb to another socket(SK_REDIRECT) or
> + * just put skb into ingress queue of current socket(SK_PASS).
> + * For SK_REDIRECT, we need 'ack' the frame immediately but for
> + * SK_PASS, the 'ack' was delay to tcp_bpf_recvmsg_parser()
> + */
> + tp->copied_seq = psock->copied_seq - psock->ingress_bytes;
> + tcp_rcv_space_adjust(sk);
> + __tcp_cleanup_rbuf(sk, copied - psock->ingress_bytes);
> +out:
> + rcu_read_unlock();
> + return copied;
> +}
> +#endif /* CONFIG_BPF_STREAM_PARSER */
> diff --git a/net/strparser/strparser.c b/net/strparser/strparser.c
> index 8299ceb3e373..95696f42647e 100644
> --- a/net/strparser/strparser.c
> +++ b/net/strparser/strparser.c
> @@ -347,7 +347,10 @@ static int strp_read_sock(struct strparser *strp)
> struct socket *sock = strp->sk->sk_socket;
> read_descriptor_t desc;
>
> - if (unlikely(!sock || !sock->ops || !sock->ops->read_sock))
> + if (unlikely(!sock || !sock->ops))
> + return -EBUSY;
> +
> + if (unlikely(!strp->cb.read_sock && !sock->ops->read_sock))
> return -EBUSY;
>
> desc.arg.data = strp;
> @@ -355,7 +358,10 @@ static int strp_read_sock(struct strparser *strp)
> desc.count = 1; /* give more than one skb per call */
>
> /* sk should be locked here, so okay to do read_sock */
> - sock->ops->read_sock(strp->sk, &desc, strp_recv);
> + if (strp->cb.read_sock)
> + strp->cb.read_sock(strp, &desc, strp_recv);
> + else
> + sock->ops->read_sock(strp->sk, &desc, strp_recv);
>
> desc.error = strp->cb.read_sock_done(strp, desc.error);
>
> @@ -468,6 +474,7 @@ int strp_init(struct strparser *strp, struct sock *sk,
> strp->cb.unlock = cb->unlock ? : strp_sock_unlock;
> strp->cb.rcv_msg = cb->rcv_msg;
> strp->cb.parse_msg = cb->parse_msg;
> + strp->cb.read_sock = cb->read_sock;
> strp->cb.read_sock_done = cb->read_sock_done ? : default_read_sock_done;
> strp->cb.abort_parser = cb->abort_parser ? : strp_abort_strp;
>
> --
> 2.43.5
Powered by blists - more mailing lists