| 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: <ojwjcubviyjxpucryc3ypi4b77h5f5g6ouv7ovaljah5harfyj@jue7hqit2t5n>
Date: Thu, 19 Dec 2024 17:30:53 +0800
From: Jiayuan Chen <mrpre@....com>
To: Jakub Sitnicki <jakub@...udflare.com>
Cc: bpf@...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,
john.fastabend@...il.com, andrii@...nel.org, mhal@...x.co, yonghong.song@...ux.dev,
daniel@...earbox.net, xiyou.wangcong@...il.com, horms@...nel.org
Subject: Re: [PATCH bpf v3 1/2] bpf: fix wrong copied_seq calculation
On Wed, Dec 18, 2024 at 04:35:53PM +0800, Jakub Sitnicki wrote:
[...]
> On Wed, Dec 18, 2024 at 01:34 PM +08, Jiayuan Chen wrote:
> > + if (tcp_flags & TCPHDR_FIN)
> > + break;
> > + }
> > +
> > + WRITE_ONCE(psock->strp_offset, offset);
> > + return copied;
> > +}
> > +
> > enum {
> > TCP_BPF_IPV4,
> > TCP_BPF_IPV6,
>
> [...]
>
> To reiterate my earlier question / suggestion [1] - it would be great if
> we can avoid duplicating what tcp_read_skb / tcp_read_sock already do.
>
> Keeping extra state in sk_psock / strparser seems to be the key. I think
> you should be able to switch strp_data_ready / str_read_sock to
> ->read_skb and make an adapter around strp_recv.
>
> Rough code below is what I have in mind. Not tested, compiled
> only. Don't expect it to work. And I haven't even looked how to address
> the kTLS path. But you get the idea.
>
> [1] https://msgid.link/87o71bx1l4.fsf@cloudflare.com
>
> ---8<---
>
> diff --git a/include/net/strparser.h b/include/net/strparser.h
> index 41e2ce9e9e10..0dd48c1bc23b 100644
> --- a/include/net/strparser.h
> +++ b/include/net/strparser.h
> @@ -95,9 +95,14 @@ struct strparser {
> u32 interrupted : 1;
> u32 unrecov_intr : 1;
>
> + unsigned int need_bytes;
> +
> struct sk_buff **skb_nextp;
> struct sk_buff *skb_head;
> - unsigned int need_bytes;
> +
> + int rcv_err;
> + unsigned int rcv_off;
> +
> struct delayed_work msg_timer_work;
> struct work_struct work;
> struct strp_stats stats;
> diff --git a/net/strparser/strparser.c b/net/strparser/strparser.c
> index 8299ceb3e373..8a08996429d3 100644
> --- a/net/strparser/strparser.c
> +++ b/net/strparser/strparser.c
> @@ -18,6 +18,7 @@
> #include <linux/poll.h>
> #include <linux/rculist.h>
> #include <linux/skbuff.h>
> +#include <linux/skmsg.h>
> #include <linux/socket.h>
> #include <linux/uaccess.h>
> #include <linux/workqueue.h>
> @@ -327,13 +328,39 @@ int strp_process(struct strparser *strp, struct sk_buff *orig_skb,
> }
> EXPORT_SYMBOL_GPL(strp_process);
>
> -static int strp_recv(read_descriptor_t *desc, struct sk_buff *orig_skb,
> - unsigned int orig_offset, size_t orig_len)
> +static int strp_read_skb(struct sock *sk, struct sk_buff *skb)
> {
> - struct strparser *strp = (struct strparser *)desc->arg.data;
> -
> - return __strp_recv(desc, orig_skb, orig_offset, orig_len,
> - strp->sk->sk_rcvbuf, strp->sk->sk_rcvtimeo);
> + struct sk_psock *psock = sk_psock_get(sk);
> + struct strparser *strp = &psock->strp;
> + read_descriptor_t desc = {
> + .arg.data = strp,
> + .count = 1,
> + .error = 0,
> + };
> + unsigned int off;
> + size_t len;
> + int used;
> +
> + off = strp->rcv_off;
> + len = skb->len - off;
> + used = __strp_recv(&desc, skb, off, len,
> + sk->sk_rcvbuf, sk->sk_rcvtimeo);
> + /* skb not consumed */
> + if (used <= 0) {
> + strp->rcv_err = used;
> + return used;
> + }
> + /* skb partially consumed */
> + if (used < len) {
> + strp->rcv_err = 0;
> + strp->rcv_off += used;
> + return -EPIPE; /* stop reading */
> + }
> + /* skb fully consumed */
> + strp->rcv_err = 0;
> + strp->rcv_off = 0;
> + tcp_eat_recv_skb(sk, skb);
> + return used;
> }
>
> static int default_read_sock_done(struct strparser *strp, int err)
> @@ -345,21 +372,14 @@ static int default_read_sock_done(struct strparser *strp, int err)
> 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 || !sock->ops->read_skb))
> return -EBUSY;
>
> - desc.arg.data = strp;
> - desc.error = 0;
> - 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);
> -
> - desc.error = strp->cb.read_sock_done(strp, desc.error);
> + sock->ops->read_skb(strp->sk, strp_read_skb);
>
> - return desc.error;
> + return strp->cb.read_sock_done(strp, strp->rcv_err);
> }
>
> /* Lower sock lock held */
Thanks Jakub Sitnicki.
I understand your point about using tcp_read_skb to replace
tcp_read_sock, avoiding code duplication and reducing the number of
interfaces.
Currently, not all modules using strparser have issues with
copied_seq miscalculation. The issue exists mainly with
bpf::sockmap + strparser because bpf::sockmap implements a
proprietary read interface for user-land: tcp_bpf_recvmsg_parser().
Both this and strp_recv->tcp_read_sock update copied_seq, leading
to errors.
This is why I rewrote the tcp_read_sock() interface specifically for
bpf::sockmap.
So far, I found two other modules that use the standard strparser module:
1.kcmsock.c
2.espintcp.c (ESP over TCP implementation)
(Interesting, these two don't have self-tests)
Take kcm as an example: its custom read interface kcm_recvmsg()
does not conflict with copied_seq updates in tcp_read_sock().
Therefore, for kcmsock, updating copied_seq in tcp_read_sock is
necessary and aligns with the read semantics. espintcp is similar.
In summary, different modules using strp_recv have different needs
for copied_seq. I still insist on implementing tcp_bpf_read_sock()
specifically for bpf::sockmap without affecting others.
Otherwise, we may need tcp_read_skb() to determine whether
to update copied_seq according to the different needs of each module.
Additionally,
I've found that KTLS has its own read_sock() and
a strparser-like implementation (in tls_strp.c), separate from the
standard strparser module. Therefore, even with your proposed
solution, KTLS may be not affected.
regards
Powered by blists - more mailing lists