On 08/01/2018 02:28 AM, Roman Gushchin wrote:

On Wed, Aug 01, 2018 at 12:50:16AM +0200, Daniel Borkmann wrote:

quoted

On 07/27/2018 11:52 PM, Roman Gushchin wrote:
[...]

quoted

@@ -2533,6 +2541,16 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn
 	}
 
 	regs = cur_regs(env);
+
+	/* check that flags argument in get_local_storage(map, flags) is 0,
+	 * this is required because get_local_storage() can't return an error.
+	 */
+	if (func_id == BPF_FUNC_get_local_storage &&
+	    !tnum_equals_const(regs[BPF_REG_2].var_off, 0)) {
+		verbose(env, "get_local_storage() doesn't support non-zero flags\n");
+		return -EINVAL;
+	}

Hmm, this check is actually not correct. You will still be able to pass non-zero
values in there. arg2_type from the helper is ARG_ANYTHING, so the register type
could for example be one of the pointer types and it will still pass the verifier.
The correct way to check would be to use register_is_null().

quoted

+
 	/* reset caller saved regs */
 	for (i = 0; i < CALLER_SAVED_REGS; i++) {
 		mark_reg_not_init(env, regs, caller_saved[i]);

Oh, perfect catch!
The diff is below. Please, let me know if you prefer me to resend
the whole patch/patchset.

Yeah, please resend at that point. There are also some other minor things which
would be great if you could roll them in as well in a respin along with this fix
and the uapi helper description adjustment with test case fix:

- patch 1: bpf_map_release_memlock() should also use bpf_uncharge_memlock() directly
- patch 2: cgroup_storage_map_alloc() only checks attr->key_size and attr->value_size
  but what about attr->max_entries and attr->map_flags? Should attr->max_entries be
  forced to 0 and at least attr->map_flags that don't make any sense in this context
  get rejected on map creation?
- patch 9: not all uapi changes were copied over into tools' uapi header

Thanks,
Daniel