From: Yonghong Song <yhs@fb.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: bpf <bpf@vger.kernel.org>, Alexei Starovoitov <ast@kernel.org>,
Cong Wang <xiyou.wangcong@gmail.com>,
Daniel Borkmann <daniel@iogearbox.net>,
Kernel Team <kernel-team@fb.com>
Subject: Re: [PATCH bpf-next v2 08/11] libbpf: support local function pointer relocation
Date: Tue, 23 Feb 2021 11:47:28 -0800 [thread overview]
Message-ID: <2a242b27-5d12-5e1b-4bed-30db68c6ed09@fb.com> (raw)
In-Reply-To: <CAEf4Bzav42vH8PdRYg7_vV20EV7FL6CJiciXs=zv3rqu5TR_zg@mail.gmail.com>
On 2/23/21 11:19 AM, Andrii Nakryiko wrote:
> On Tue, Feb 23, 2021 at 10:56 AM Yonghong Song <yhs@fb.com> wrote:
>>
>>
>>
>> On 2/23/21 12:03 AM, Andrii Nakryiko wrote:
>>> On Wed, Feb 17, 2021 at 12:56 PM Yonghong Song <yhs@fb.com> wrote:
>>>>
>>>> A new relocation RELO_SUBPROG_ADDR is added to capture
>>>> local (static) function pointers loaded with ld_imm64
>>>> insns. Such ld_imm64 insns are marked with
>>>> BPF_PSEUDO_FUNC and will be passed to kernel so
>>>> kernel can replace them with proper actual jited
>>>> func addresses.
>>>>
>>>> Signed-off-by: Yonghong Song <yhs@fb.com>
>>>> ---
>>>> tools/lib/bpf/libbpf.c | 40 +++++++++++++++++++++++++++++++++++++---
>>>> 1 file changed, 37 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>>>> index 21a3eedf070d..772c7455f1a2 100644
>>>> --- a/tools/lib/bpf/libbpf.c
>>>> +++ b/tools/lib/bpf/libbpf.c
>>>> @@ -188,6 +188,7 @@ enum reloc_type {
>>>> RELO_CALL,
>>>> RELO_DATA,
>>>> RELO_EXTERN,
>>>> + RELO_SUBPROG_ADDR,
>>>> };
>>>>
>>>> struct reloc_desc {
>>>> @@ -579,6 +580,11 @@ static bool is_ldimm64(struct bpf_insn *insn)
>>>> return insn->code == (BPF_LD | BPF_IMM | BPF_DW);
>>>> }
>>>>
>>>> +static bool insn_is_pseudo_func(struct bpf_insn *insn)
>>>> +{
>>>> + return is_ldimm64(insn) && insn->src_reg == BPF_PSEUDO_FUNC;
>>>> +}
>>>> +
>>>> static int
>>>> bpf_object__init_prog(struct bpf_object *obj, struct bpf_program *prog,
>>>> const char *name, size_t sec_idx, const char *sec_name,
>>>> @@ -3406,6 +3412,16 @@ static int bpf_program__record_reloc(struct bpf_program *prog,
>>>> return -LIBBPF_ERRNO__RELOC;
>>>> }
>>>>
>>>> + if (GELF_ST_BIND(sym->st_info) == STB_LOCAL &&
>>>> + GELF_ST_TYPE(sym->st_info) == STT_SECTION &&
>>>
>>> STB_LOCAL + STT_SECTION is a section symbol. But STT_FUNC symbol could
>>> be referenced as well, no? So this is too strict.
>>
>> Yes, STT_FUNC symbol could be referenced but we do not have use
>> case yet. If we encode STT_FUNC (global), the kernel will reject
>> it. We can extend libbpf to support STT_FUNC once we got a use
>> case.
>
> I don't really like tailoring libbpf generic SUBPROG_ADDR relocation
> to one current specific use case, though. Taking the address of
> SUBPROG_ADDR is not, conceptually, tied with passing it to for_each as
> a callback. E.g., what if you just want to compare two registers
> pointing to subprogs, without actually passing them to for_each(). I
> don't know if it's possible right now, but I don't see why that
> shouldn't be supported. In the latter case, adding arbitrary
> restrictions about static vs global functions doesn't make much sense.
>
> So let's teach libbpf the right logic without assuming any specific
> use case. It pays off in the long run.
Okay, Will support global function as well. It won't hurt.
>
>>
>>>
>>>> + (!shdr_idx || shdr_idx == obj->efile.text_shndx) &&
>>>
>>> this doesn't look right, shdr_idx == 0 is a bad condition and should
>>> be rejected, not accepted.
>>
>> it is my fault. Will fix in the next revision.
>>
>>>
>>>> + !(sym->st_value % BPF_INSN_SZ)) {
>>>> + reloc_desc->type = RELO_SUBPROG_ADDR;
>>>> + reloc_desc->insn_idx = insn_idx;
>>>> + reloc_desc->sym_off = sym->st_value;
>>>> + return 0;
>>>> + }
>>>> +
>>>
>>> So see code right after sym_is_extern(sym) check. It checks for valid
>>> shrd_idx, which is good and would be good to use that. After that we
>>> can assume shdr_idx is valid and we can make a simple
>>> obj->efile.text_shndx check then and use that as a signal that this is
>>> SUBPROG_ADDR relocation (instead of deducing that from STT_SECTION).
>>>
>>> And !(sym->st_value % BPF_INSN_SZ) should be reported as an error, not
>>> silently skipped. Again, just how BPF_JMP | BPF_CALL does it. That way
>>> it's more user-friendly, if something goes wrong. So it will look like
>>> this:
>>>
>>> if (shdr_idx == obj->efile.text_shndx) {
>>> /* check sym->st_value, pr_warn(), return error */
>>>
>>> reloc_desc->type = RELO_SUBPROG_ADDR;
>>> ...
>>> return 0;
>>> }
>>
>> Okay. Will do similar checking to insn->code == (BPF_JMP | BPF_CALL)
>> in the next revision.
>>
>>>
>>>> if (sym_is_extern(sym)) {
>>>> int sym_idx = GELF_R_SYM(rel->r_info);
>>>> int i, n = obj->nr_extern;
>>>> @@ -6172,6 +6188,10 @@ bpf_object__relocate_data(struct bpf_object *obj, struct bpf_program *prog)
>>>> }
>>>> relo->processed = true;
>>>> break;
>>>> + case RELO_SUBPROG_ADDR:
>>>> + insn[0].src_reg = BPF_PSEUDO_FUNC;
>>>
>>> BTW, doesn't Clang emit instruction with BPF_PSEUDO_FUNC set properly
>>> already? If not, why not?
>>
>> This is really a contract between libbpf and kernel, similar to
>> BPF_PSEUDO_MAP_FD/BPF_PSEUDO_MAP_VALUE/BPF_PSEUDO_BTF_ID.
>> Adding encoding in clang is not needed as this is simply a load
>> of function address as far as clang concerned.
>
> Yeah, not a big deal, I was under the impression we do that for other
> BPF_PSEUDO cases, but checking other parts of libbpf, doesn't seem
> like that's the case.
>
>>
>>>
>>>> + /* will be handled as a follow up pass */
>>>> + break;
>>>> case RELO_CALL:
>>>> /* will be handled as a follow up pass */
>>>> break;
>>>> @@ -6358,11 +6378,11 @@ bpf_object__reloc_code(struct bpf_object *obj, struct bpf_program *main_prog,
>>>>
>>>> for (insn_idx = 0; insn_idx < prog->sec_insn_cnt; insn_idx++) {
>>>> insn = &main_prog->insns[prog->sub_insn_off + insn_idx];
>>>> - if (!insn_is_subprog_call(insn))
>>>> + if (!insn_is_subprog_call(insn) && !insn_is_pseudo_func(insn))
>>>> continue;
>>>>
>>>> relo = find_prog_insn_relo(prog, insn_idx);
>>>> - if (relo && relo->type != RELO_CALL) {
>>>> + if (relo && relo->type != RELO_CALL && relo->type != RELO_SUBPROG_ADDR) {
>>>> pr_warn("prog '%s': unexpected relo for insn #%zu, type %d\n",
>>>> prog->name, insn_idx, relo->type);
>>>> return -LIBBPF_ERRNO__RELOC;
>>>> @@ -6374,8 +6394,22 @@ bpf_object__reloc_code(struct bpf_object *obj, struct bpf_program *main_prog,
>>>> * call always has imm = -1, but for static functions
>>>> * relocation is against STT_SECTION and insn->imm
>>>> * points to a start of a static function
>>>> + *
>>>> + * for local func relocation, the imm field encodes
>>>> + * the byte offset in the corresponding section.
>>>> + */
>>>> + if (relo->type == RELO_CALL)
>>>> + sub_insn_idx = relo->sym_off / BPF_INSN_SZ + insn->imm + 1;
>>>> + else
>>>> + sub_insn_idx = relo->sym_off / BPF_INSN_SZ + insn->imm / BPF_INSN_SZ + 1;
>>>> + } else if (insn_is_pseudo_func(insn)) {
>>>> + /*
>>>> + * RELO_SUBPROG_ADDR relo is always emitted even if both
>>>> + * functions are in the same section, so it shouldn't reach here.
>>>> */
>>>> - sub_insn_idx = relo->sym_off / BPF_INSN_SZ + insn->imm + 1;
>>>> + pr_warn("prog '%s': missing relo for insn #%zu, type %d\n",
>>>
>>> nit: "missing subprog addr relo" to make it clearer?
>>
>> sure. will do.
>
> given the "generic support" comment above, I think we should still
> support this case as well. WDYT?
Even for global function with ldimm64, relocations should have been
generated previously. I will double verify to ensure this is true
for global function as well for different cases, in the same section or
different sections, etc.
>
>>
>>>
>>>> + prog->name, insn_idx, relo->type);
>>>> + return -LIBBPF_ERRNO__RELOC;
>>>> } else {
>>>> /* if subprogram call is to a static function within
>>>> * the same ELF section, there won't be any relocation
>>>> --
>>>> 2.24.1
>>>>
next prev parent reply other threads:[~2021-02-23 19:48 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-17 18:18 [PATCH bpf-next v2 00/11] bpf: add bpf_for_each_map_elem() helper Yonghong Song
2021-02-17 18:18 ` [PATCH bpf-next v2 01/11] bpf: factor out visit_func_call_insn() in check_cfg() Yonghong Song
2021-02-17 18:18 ` [PATCH bpf-next v2 02/11] bpf: factor out verbose_invalid_scalar() Yonghong Song
2021-02-17 18:18 ` [PATCH bpf-next v2 03/11] bpf: refactor check_func_call() to allow callback function Yonghong Song
2021-02-17 18:18 ` [PATCH bpf-next v2 04/11] bpf: add bpf_for_each_map_elem() helper Yonghong Song
2021-02-22 20:59 ` Alexei Starovoitov
2021-02-23 18:39 ` Yonghong Song
2021-02-23 18:46 ` Alexei Starovoitov
2021-02-23 19:37 ` Yonghong Song
2021-02-17 18:18 ` [PATCH bpf-next v2 05/11] bpf: add hashtab support for " Yonghong Song
2021-02-22 22:56 ` Alexei Starovoitov
2021-02-23 18:41 ` Yonghong Song
2021-02-17 18:18 ` [PATCH bpf-next v2 06/11] bpf: add arraymap " Yonghong Song
2021-02-17 18:18 ` [PATCH bpf-next v2 07/11] libbpf: move function is_ldimm64() earlier in libbpf.c Yonghong Song
2021-02-23 8:06 ` Andrii Nakryiko
2021-02-17 18:18 ` [PATCH bpf-next v2 08/11] libbpf: support local function pointer relocation Yonghong Song
2021-02-23 8:03 ` Andrii Nakryiko
2021-02-23 18:55 ` Yonghong Song
2021-02-23 19:07 ` Alexei Starovoitov
2021-02-23 19:21 ` Andrii Nakryiko
2021-02-23 19:19 ` Andrii Nakryiko
2021-02-23 19:47 ` Yonghong Song [this message]
2021-02-23 21:24 ` Andrii Nakryiko
2021-02-17 18:18 ` [PATCH bpf-next v2 09/11] bpftool: print local function pointer properly Yonghong Song
2021-02-23 8:06 ` Andrii Nakryiko
2021-02-23 19:00 ` Yonghong Song
2021-02-17 18:18 ` [PATCH bpf-next v2 10/11] selftests/bpf: add hashmap test for bpf_for_each_map_elem() helper Yonghong Song
2021-02-17 18:18 ` [PATCH bpf-next v2 11/11] selftests/bpf: add arraymap " Yonghong Song
2021-02-17 18:29 ` [PATCH bpf-next v2 00/11] bpf: add " Yonghong Song
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2a242b27-5d12-5e1b-4bed-30db68c6ed09@fb.com \
--to=yhs@fb.com \
--cc=andrii.nakryiko@gmail.com \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=kernel-team@fb.com \
--cc=xiyou.wangcong@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).