Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Jiri Olsa <olsajiri@gmail.com>
Date:   Wed, 3 Aug 2022 13:53:20 +0200
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().
Message-ID: <YuphsM7BVKfN+0ro@krava>
References: <20220801205039.2755281-1-haoluo@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220801205039.2755281-1-haoluo@google.com>
Precedence: bulk

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
>