Re: [PATCH 0/2] gas,bpf: cleanup bad symbols created while parsing

[David Faust <david.faust@oracle.com>]

On 11/15/23 01:49, Jan Beulich wrote:
> On 14.11.2023 18:58, David Faust wrote:
>> To support the "pseudo-C" asm dialect in BPF, the BPF parser must
>> often attempt multiple different templates for a single instruction.
>> In some cases, this can lead to a call to expression () which creates a
>> symbol, and only later the template is determined not to match and the
>> expression is discarded.
>>
>> However, symbols created during this process are added to the symbol
>> table and are not removed if the expression is discarded.
>>
>> This is a problem for BPF: generally the assembled object will be
>> loaded directly by the Linux kernel, without being linked.  The kernel
>> BPF loader requires that BTF information is available for every symbol
>> in a loaded BPF program, which will not be available for these symbols
>> erroneously created by the parser.
>>
>> Patch 1 adds a symbol_table_remove () function to symbols.c and
>> exposes it in the header, since symbol_remove () is not sufficient to
>> prevent such symbols being written in the symbol table of the resulting
>> ELF object.
>>
>> Patch 2 detects cases in the BPF parser where a call to expression ()
>> created any new symbol(s), but the parsing of the instruction as a
>> whole against the current template failed.  In that case the created
>> symbols are deleted.
> 
> While this may be a workaround (and perhaps even a viable one), I think
> it would be better to suppress symbol table insertion in the first place.
> I had to solve a somewhat related issue for RISC-V (and I expect MIPS
> would want to also be switched to a similar approach), see 7a29ee290307
> ("RISC-V: adjust logic to avoid register name symbols"). I've looked at
> the testcases added by patch 2. While the first one's purpose is clear
> (thanks to the comment there), I can't really figure what the 2nd aims to
> test. Which may mean that I'm not properly understanding what (set of)
> condition(s) is/are involved here, and hence whether using one or more of
> the existing target hooks can indeed help here (without needing to
> transiently insert any symbols, and then having target-specific code
> depend on how exactly symbols are inserted).

Hi Jan,
Thank you for the review. This is very helpful.

I agree that it is be better to suppress symbol table insertion. To be
honest, I did not think of a good way to do it before this workaround.
So, thank you very much for the pointer to 7a29ee290307 because that is
certainly a nicer approach :). I think something similar to that will
work for BPF too, and I plan to rewrite the patch to use that approach.

As for the 2nd test, before adding and using symbol_table_remove ()
it would fail because the symbol 'foo' was not emitted in the object.
This happened because a failed instruction parse would now remove
any symbol created while parsing the instruction, including the real and
used 'foo'. The symbol was never "un-removed", because it remained in
the symbol hash table. So all later calls to find_symbol ('foo')
returned a reference to the removed symbol, rather than failing and
allowing 'foo' to be remade by a later instruction parse. With an
approach that suppresses insertion of wrong symbols rather than trying
to remove them later, this will not be an issue.
(The existing gas/bpf/indcall-1-pseudoc test was also failing for the
same reason.)

Thanks
David