Message-ID: <5e26aa0bc382b_32772acafb17c5b410@john-XPS-13-9370.notmuch>
Date:   Mon, 20 Jan 2020 23:36:43 -0800
From:   John Fastabend <john.fastabend@...il.com>
To:     Alexei Starovoitov <ast@...nel.org>, davem@...emloft.net
Cc:     daniel@...earbox.net, netdev@...r.kernel.org, bpf@...r.kernel.org,
        kernel-team@...com
Subject: RE: [PATCH v2 bpf-next 1/3] bpf: Introduce dynamic program extensions

Alexei Starovoitov wrote:
> Introduce dynamic program extensions. The users can load additional BPF
> functions and replace global functions in previously loaded BPF programs while
> these programs are executing.
> 
> Global functions are verified individually by the verifier based on their types only.
> Hence the global function in the new program which types match older function can
> safely replace that corresponding function.
> 
> This new function/program is called 'an extension' of old program. At load time
> the verifier uses (attach_prog_fd, attach_btf_id) pair to identify the function
> to be replaced. The BPF program type is derived from the target program into
> extension program. Technically bpf_verifier_ops is copied from target program.
> The BPF_PROG_TYPE_EXT program type is a placeholder. It has empty verifier_ops.
> The extension program can call the same bpf helper functions as target program.
> Single BPF_PROG_TYPE_EXT type is used to extend XDP, SKB and all other program
> types. The verifier allows only one level of replacement. Meaning that the
> extension program cannot recursively extend an extension. That also means that
> the maximum stack size is increasing from 512 to 1024 bytes and maximum
> function nesting level from 8 to 16. The programs don't always consume that
> much. The stack usage is determined by the number of on-stack variables used by
> the program. The verifier could have enforced 512 limit for combined original
> plus extension program, but it makes for difficult user experience. The main
> use case for extensions is to provide generic mechanism to plug external
> programs into policy program or function call chaining.
> 
> BPF trampoline is used to track both fentry/fexit and program extensions
> because both are using the same nop slot at the beginning of every BPF
> function. Attaching fentry/fexit to a function that was replaced is not
> allowed. The opposite is true as well. Replacing a function that currently
> being analyzed with fentry/fexit is not allowed. The executable page allocated
> by BPF trampoline is not used by program extensions. This inefficiency will be
> optimized in future patches.
> 
> Function by function verification of global function supports scalars and
> pointer to context only. Hence program extensions are supported for such class
> of global functions only. In the future the verifier will be extended with
> support to pointers to structures, arrays with sizes, etc.
> 
> Signed-off-by: Alexei Starovoitov <ast@...nel.org>
> ---

[...]

> +
> +	t1 = btf_type_skip_modifiers(btf1, t1->type, NULL);
> +	t2 = btf_type_skip_modifiers(btf2, t2->type, NULL);

Is it really best to skip modifiers? I would expect that if the
signature is different including modifiers then we should just reject it.
OTOH its not really C code here either so modifiers may not have the same
meaning. With just integers and struct it may be ok but if we add pointers
to ints then what would we expect from a const int*?

So whats the reasoning for skipping modifiers? Is it purely an argument
that its not required for safety so solve it elsewhere? In that case then
checking names of functions is also equally not required.

Otherwise LGTM.


> +	if (t1->info != t2->info) {
> +		bpf_log(log,
> +			"Return type %s of %s() doesn't match type %s of %s()\n",
> +			btf_type_str(t1), fn1,
> +			btf_type_str(t2), fn2);
> +		return -EINVAL;
> +	}
> +
> +	for (i = 0; i < nargs1; i++) {
> +		t1 = btf_type_skip_modifiers(btf1, args1[i].type, NULL);
> +		t2 = btf_type_skip_modifiers(btf2, args2[i].type, NULL);
> +
> +		if (t1->info != t2->info) {
> +			bpf_log(log, "arg%d in %s() is %s while %s() has %s\n",
> +				i, fn1, btf_type_str(t1),
> +				fn2, btf_type_str(t2));
> +			return -EINVAL;
> +		}
> +		if (btf_type_has_size(t1) && t1->size != t2->size) {
> +			bpf_log(log,
> +				"arg%d in %s() has size %d while %s() has %d\n",
> +				i, fn1, t1->size,
> +				fn2, t2->size);
> +			return -EINVAL;
> +		}
> +
> +		/* global functions are validated with scalars and pointers
> +		 * to context only. And only global functions can be replaced.
> +		 * Hence type check only those types.
> +		 */
> +		if (btf_type_is_int(t1) || btf_type_is_enum(t1))
> +			continue;
> +		if (!btf_type_is_ptr(t1)) {
> +			bpf_log(log,
> +				"arg%d in %s() has unrecognized type\n",
> +				i, fn1);
> +			return -EINVAL;
> +		}
> +		t1 = btf_type_skip_modifiers(btf1, t1->type, NULL);
> +		t2 = btf_type_skip_modifiers(btf2, t2->type, NULL);
> +		if (!btf_type_is_struct(t1)) {
> +			bpf_log(log,
> +				"arg%d in %s() is not a pointer to context\n",
> +				i, fn1);
> +			return -EINVAL;
> +		}
> +		if (!btf_type_is_struct(t2)) {
> +			bpf_log(log,
> +				"arg%d in %s() is not a pointer to context\n",
> +				i, fn2);
> +			return -EINVAL;
> +		}
> +		/* This is an optional check to make program writing easier.
> +		 * Compare names of structs and report an error to the user.
> +		 * btf_prepare_func_args() already checked that t2 struct
> +		 * is a context type. btf_prepare_func_args() will check
> +		 * later that t1 struct is a context type as well.
> +		 */
> +		s1 = btf_name_by_offset(btf1, t1->name_off);
> +		s2 = btf_name_by_offset(btf2, t2->name_off);
> +		if (strcmp(s1, s2)) {
> +			bpf_log(log,
> +				"arg%d %s(struct %s *) doesn't match %s(struct %s *)\n",
> +				i, fn1, s1, fn2, s2);
> +			return -EINVAL;
> +		}
> +	}
> +	return 0;
> +}

Powered by blists - more mailing lists