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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.