Re: [PATCH bpf-next 1/2] bpf: track contents of read-only maps as scalars

[Martin Lau <kafai@xxxxxx>]

On Tue, Oct 08, 2019 at 12:45:47PM -0700, Andrii Nakryiko wrote:
> Maps that are read-only both from BPF program side and user space side
> have their contents constant, so verifier can track referenced values
> precisely and use that knowledge for dead code elimination, branch
> pruning, etc. This patch teaches BPF verifier how to do this.
> 
> Signed-off-by: Andrii Nakryiko <andriin@xxxxxx>
> ---
>  kernel/bpf/verifier.c | 58 +++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 56 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index ffc3e53f5300..1e4e4bd64ca5 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -2739,6 +2739,42 @@ static void coerce_reg_to_size(struct bpf_reg_state *reg, int size)
>  	reg->smax_value = reg->umax_value;
>  }
>  
> +static bool bpf_map_is_rdonly(const struct bpf_map *map)
> +{
> +	return (map->map_flags & BPF_F_RDONLY_PROG) &&
> +	       ((map->map_flags & BPF_F_RDONLY) || map->frozen);
> +}
> +
> +static int bpf_map_direct_read(struct bpf_map *map, int off, int size, u64 *val)
> +{
> +	void *ptr;
> +	u64 addr;
> +	int err;
> +
> +	err = map->ops->map_direct_value_addr(map, &addr, off + size);
Should it be "off" instead of "off + size"?

> +	if (err)
> +		return err;
> +	ptr = (void *)addr + off;
> +
> +	switch (size) {
> +	case sizeof(u8):
> +		*val = (u64)*(u8 *)ptr;
> +		break;
> +	case sizeof(u16):
> +		*val = (u64)*(u16 *)ptr;
> +		break;
> +	case sizeof(u32):
> +		*val = (u64)*(u32 *)ptr;
> +		break;
> +	case sizeof(u64):
> +		*val = *(u64 *)ptr;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
>  /* check whether memory at (regno + off) is accessible for t = (read | write)
>   * if t==write, value_regno is a register which value is stored into memory
>   * if t==read, value_regno is a register which will receive the value from memory
> @@ -2776,9 +2812,27 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn
>  		if (err)
>  			return err;
>  		err = check_map_access(env, regno, off, size, false);
> -		if (!err && t == BPF_READ && value_regno >= 0)
> -			mark_reg_unknown(env, regs, value_regno);
> +		if (!err && t == BPF_READ && value_regno >= 0) {
> +			struct bpf_map *map = reg->map_ptr;
> +
> +			/* if map is read-only, track its contents as scalars */
> +			if (tnum_is_const(reg->var_off) &&
> +			    bpf_map_is_rdonly(map) &&
> +			    map->ops->map_direct_value_addr) {
> +				int map_off = off + reg->var_off.value;
> +				u64 val = 0;
>  
> +				err = bpf_map_direct_read(map, map_off, size,
> +							  &val);
> +				if (err)
> +					return err;
> +
> +				regs[value_regno].type = SCALAR_VALUE;
> +				__mark_reg_known(&regs[value_regno], val);
> +			} else {
> +				mark_reg_unknown(env, regs, value_regno);
> +			}
> +		}
>  	} else if (reg->type == PTR_TO_CTX) {
>  		enum bpf_reg_type reg_type = SCALAR_VALUE;
>  
> -- 
> 2.17.1
>