From: Jiri Olsa <olsajiri@gmail.com>
To: Hao Luo <haoluo@google.com>
Cc: linux-kernel@vger.kernel.org, bpf@vger.kernel.org,
Alexei Starovoitov <ast@kernel.org>,
Andrii Nakryiko <andrii@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
Martin KaFai Lau <martin.lau@linux.dev>,
Song Liu <song@kernel.org>, Yonghong Song <yhs@fb.com>,
KP Singh <kpsingh@kernel.org>,
John Fastabend <john.fastabend@gmail.com>,
Stanislav Fomichev <sdf@google.com>
Subject: Re: [PATCH bpf-next v1] bpf, iter: clean up bpf_seq_read().
Date: Wed, 3 Aug 2022 13:53:20 +0200 [thread overview]
Message-ID: <YuphsM7BVKfN+0ro@krava> (raw)
In-Reply-To: <20220801205039.2755281-1-haoluo@google.com>
On Mon, Aug 01, 2022 at 01:50:39PM -0700, Hao Luo wrote:
SNIP
> - err = seq->op->show(seq, p);
> - if (err > 0) {
> - /* object is skipped, decrease seq_num, so next
> - * valid object can reuse the same seq_num.
> - */
> - bpf_iter_dec_seq_num(seq);
> - seq->count = 0;
> - } else if (err < 0 || seq_has_overflowed(seq)) {
> - if (!err)
> - err = -E2BIG;
> - seq->op->stop(seq, p);
> + err = do_seq_show(seq, p, 0);
> + if (err < 0) {
> + do_seq_stop(seq, p, 0);
> seq->count = 0;
> goto done;
> }
> @@ -153,7 +208,7 @@ static ssize_t bpf_seq_read(struct file *file, char __user *buf, size_t size,
> num_objs++;
> offs = seq->count;
> p = seq->op->next(seq, p, &seq->index);
> - if (pos == seq->index) {
> + if (unlikely(pos == seq->index)) {
> pr_info_ratelimited("buggy seq_file .next function %ps "
> "did not updated position index\n",
> seq->op->next);
> @@ -161,7 +216,7 @@ static ssize_t bpf_seq_read(struct file *file, char __user *buf, size_t size,
> }
>
> if (IS_ERR_OR_NULL(p))
> - break;
> + goto stop;
we could still keep the break here
>
> /* got a valid next object, increase seq_num */
> bpf_iter_inc_seq_num(seq);
> @@ -172,22 +227,16 @@ static ssize_t bpf_seq_read(struct file *file, char __user *buf, size_t size,
> if (num_objs >= MAX_ITER_OBJECTS) {
> if (offs == 0) {
> err = -EAGAIN;
> - seq->op->stop(seq, p);
> + do_seq_stop(seq, p, seq->count);
> goto done;
> }
> break;
> }
>
> - err = seq->op->show(seq, p);
> - if (err > 0) {
> - bpf_iter_dec_seq_num(seq);
> - seq->count = offs;
> - } else if (err < 0 || seq_has_overflowed(seq)) {
> - seq->count = offs;
> + err = do_seq_show(seq, p, offs);
> + if (err < 0) {
> if (offs == 0) {
> - if (!err)
> - err = -E2BIG;
> - seq->op->stop(seq, p);
> + do_seq_stop(seq, p, seq->count);
> goto done;
> }
> break;
> @@ -197,30 +246,11 @@ static ssize_t bpf_seq_read(struct file *file, char __user *buf, size_t size,
> cond_resched();
> }
> stop:
> - offs = seq->count;
> - /* bpf program called if !p */
> - seq->op->stop(seq, p);
> - if (!p) {
> - if (!seq_has_overflowed(seq)) {
> - bpf_iter_done_stop(seq);
> - } else {
> - seq->count = offs;
> - if (offs == 0) {
> - err = -E2BIG;
> - goto done;
> - }
> - }
> - }
> -
> - n = min(seq->count, size);
> - err = copy_to_user(buf, seq->buf, n);
> - if (err) {
> - err = -EFAULT;
> + err = do_seq_stop(seq, p, seq->count);
> + if (err)
> goto done;
looks like we tried to copy the data before when stop failed,
now it's skipped
jirka
> - }
> - copied = n;
> - seq->count -= n;
> - seq->from = n;
> +
> + err = do_copy_to_user(seq, buf, size, &copied);
> done:
> if (!copied)
> copied = err;
> --
> 2.37.1.455.g008518b4e5-goog
>
prev parent reply other threads:[~2022-08-03 11:53 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-01 20:50 [PATCH bpf-next v1] bpf, iter: clean up bpf_seq_read() Hao Luo
2022-08-02 11:14 ` Jiri Olsa
2022-08-03 0:15 ` Hao Luo
2022-08-03 11:50 ` Jiri Olsa
2022-08-03 11:53 ` Jiri Olsa [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YuphsM7BVKfN+0ro@krava \
--to=olsajiri@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=haoluo@google.com \
--cc=john.fastabend@gmail.com \
--cc=kpsingh@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=martin.lau@linux.dev \
--cc=sdf@google.com \
--cc=song@kernel.org \
--cc=yhs@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox