Re: [PATCH bpf 6/9] bpf: Only allow sleepable program for resched-able iterator

[Yonghong Song <yhs@fb.com>]

On 8/6/22 12:40 AM, Hou Tao wrote:
> From: Hou Tao <houtao1@huawei.com>
> 
> When a sleepable program is attached to a hash map iterator, might_fault()
> will report "BUG: sleeping function called from invalid context..." if
> CONFIG_DEBUG_ATOMIC_SLEEP is enabled. The reason is that rcu_read_lock()
> is held in bpf_hash_map_seq_next() and won't be released until all elements
> are traversed or bpf_hash_map_seq_stop() is called.
> 
> Fixing it by reusing BPF_ITER_RESCHED to indicate that only non-sleepable
> program is allowed for iterator without BPF_ITER_RESCHED. Another fine-grained
> flag can be added later if needed.

I think this is okay. BPF_ITER_RESCHED will enable cond_resched() which
won't work in a rcu_read_lock()/rcu_read_unlock() context. We can
revisit bpf_iter_link_attach() later if later there are other
conditions which may cause rcu_read_lock() issues.

> 
> Signed-off-by: Hou Tao <houtao1@huawei.com>

Acked-by: Yonghong Song <yhs@fb.com>

> ---
>   kernel/bpf/bpf_iter.c | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/bpf/bpf_iter.c b/kernel/bpf/bpf_iter.c
> index 7e8fd49406f6..f4db589d1dc5 100644
> --- a/kernel/bpf/bpf_iter.c
> +++ b/kernel/bpf/bpf_iter.c
> @@ -68,13 +68,18 @@ static void bpf_iter_done_stop(struct seq_file *seq)
>   	iter_priv->done_stop = true;
>   }
>   
> +static inline bool bpf_iter_target_support_resched(const struct bpf_iter_target_info *tinfo)
> +{
> +	return tinfo->reg_info->feature & BPF_ITER_RESCHED;
> +}
> +
>   static bool bpf_iter_support_resched(struct seq_file *seq)
>   {
>   	struct bpf_iter_priv_data *iter_priv;
>   
>   	iter_priv = container_of(seq->private, struct bpf_iter_priv_data,
>   				 target_private);
> -	return iter_priv->tinfo->reg_info->feature & BPF_ITER_RESCHED;
> +	return bpf_iter_target_support_resched(iter_priv->tinfo);
>   }
>   
>   /* maximum visited objects before bailing out */
> @@ -538,6 +543,10 @@ int bpf_iter_link_attach(const union bpf_attr *attr, bpfptr_t uattr,
>   	if (!tinfo)
>   		return -ENOENT;
>   
> +	/* Only allow sleepable program for resched-able iterator */
> +	if (prog->aux->sleepable && !bpf_iter_target_support_resched(tinfo))
> +		return -EINVAL;
> +
>   	link = kzalloc(sizeof(*link), GFP_USER | __GFP_NOWARN);
>   	if (!link)
>   		return -ENOMEM;