netdev - Re: [PATCH bpf v2 1/3] bpf: fix a rcu_sched stall issue with bpf task/task

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200819000547.7qv32me2fxviwdkx@ast-mbp.dhcp.thefacebook.com>
Date:   Tue, 18 Aug 2020 17:05:47 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Yonghong Song <yhs@...com>
Cc:     bpf@...r.kernel.org, netdev@...r.kernel.org,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>, kernel-team@...com,
        "Paul E . McKenney" <paulmck@...nel.org>
Subject: Re: [PATCH bpf v2 1/3] bpf: fix a rcu_sched stall issue with bpf
 task/task_file iterator

On Tue, Aug 18, 2020 at 03:23:09PM -0700, Yonghong Song wrote:
> 
> We did not use cond_resched() since for some iterators, e.g.,
> netlink iterator, where rcu read_lock critical section spans between
> consecutive seq_ops->next(), which makes impossible to do cond_resched()
> in the key while loop of function bpf_seq_read().

but after this patch we can, right?

>  
> +/* maximum visited objects before bailing out */
> +#define MAX_ITER_OBJECTS	1000000
> +
>  /* bpf_seq_read, a customized and simpler version for bpf iterator.
>   * no_llseek is assumed for this file.
>   * The following are differences from seq_read():
> @@ -79,7 +82,7 @@ static ssize_t bpf_seq_read(struct file *file, char __user *buf, size_t size,
>  {
>  	struct seq_file *seq = file->private_data;
>  	size_t n, offs, copied = 0;
> -	int err = 0;
> +	int err = 0, num_objs = 0;
>  	void *p;
>  
>  	mutex_lock(&seq->lock);
> @@ -135,6 +138,7 @@ static ssize_t bpf_seq_read(struct file *file, char __user *buf, size_t size,
>  	while (1) {
>  		loff_t pos = seq->index;
>  
> +		num_objs++;
>  		offs = seq->count;
>  		p = seq->op->next(seq, p, &seq->index);
>  		if (pos == seq->index) {
> @@ -153,6 +157,15 @@ static ssize_t bpf_seq_read(struct file *file, char __user *buf, size_t size,
>  		if (seq->count >= size)
>  			break;
>  
> +		if (num_objs >= MAX_ITER_OBJECTS) {
> +			if (offs == 0) {
> +				err = -EAGAIN;
> +				seq->op->stop(seq, p);
> +				goto done;
> +			}
> +			break;
> +		}
> +

should this block be after op->show() and error processing?
Otherwise bpf_iter_inc_seq_num() will be incorrectly incremented?

>  		err = seq->op->show(seq, p);
>  		if (err > 0) {
>  			bpf_iter_dec_seq_num(seq);

After op->stop() we can do cond_resched() in all cases,
since rhashtable walk does rcu_unlock in stop() callback, right?
I think copy_to_user() and mutex_unlock() don't do cond_resched()
equivalent work.