Allow changing line info data in btf.Line

[MarcusWichelmann]

Describe the bug

I'd like to change the line field in the btf.Line structure for a single instruction of a program, but cannot find a way to do that.

Background

I'm templating an eBPF program by replacing a placeholder function in its instructions with some custom instructions. Then I use WithMetadata() (see #832) to copy over the metadata of the first placeholder instruction (that has the symbol) to the new one.

This works fine, but I'd like to also change the BTF line info for the first instruction to contain some information about the inserted logic so it can be recognized in an instruction dump / verifier log.

I could use .WithSource(asm.Comment("ABC")) for that, but this causes a missing bpf_line_info for 1 funcs starting from func#1 verifier error, because the line info seems to go missing then.

What I probably need to do is, to retrieve the existing bpf.Line structure using .Source().(*btf.Line) and change its fields, but these are unexported, so there doesn't seem to be a way to do that.

I'd like to discuss what would be the preferred solution here, before I make a pull request.

How to reproduce

Load an eBPF program and call this on the first instruction of a function (the one with the symbol):

insns[i] = insns[i].WithMetadata(insn.Metadata).WithSource(asm.Comment("ABC"))

Then try to load the program and check the verifier error.

Version information

github.com/cilium/ebpf v0.14.0

[lmb]

I'm using the same functionality in the library for https://github.com/cilium/ebpf/pull/1402/commits I think we could just export the fields in Line as you suggested. cc @dylandreimerink

[dylandreimerink]

Yes. I think that would be alright. There isn't any guarding of values needed, so just exposing fields should be safe.

Are you thinking of just exposing Line or the rest of the fields as well? (seem odd otherwise)

Edit: We do have some restrictions imposed on us, namely maximums for the line size and column (since column only gets 10 bits when encoded). So we might want to consider using Setter methods instead of exported fields

[MarcusWichelmann]

Wow, you're fast.

@dylandreimerink Yeah would probably make sense to export all of them then. Add setters for all of them?

Should I make a PR?

[dylandreimerink]

Just realized I missed something, see the edited message. We basically have a choice between exporting the fields which might cause marshaling errors when used incorrectly or add some input validation on the btf.Line object.

[MarcusWichelmann]

What would be your preferred solution? Are there other ways how invalid values can get into that structure which would make a validation before marshalling necessary instead of just checking in the setters? Or where would you put that validation?

[MarcusWichelmann]

Also there seems to be some validation already:

ebpf/btf/ext_info.go

Lines 594 to 600 in a330a78

	if line.lineNumber > bpfLineMax {
	return fmt.Errorf("line %d exceeds %d", line.lineNumber, bpfLineMax)
	}
	if line.lineColumn > bpfColumnMax {
	return fmt.Errorf("column %d exceeds %d", line.lineColumn, bpfColumnMax)
	}

Isn't that sufficient?

[dylandreimerink]

Isn't that sufficient?

Its a matter of preference and perhaps UX to some extent. The current types for the fields don't indicate there are any limits. So in the case of just exporting, someone may come along, have some code that modifies a field. It would be very possible that this wouldn't blow up until some day the code changes, field becomes to long, and then you get an error at a completely unrelated location (marshaling). That would be a pain in the ass to debug. While, if we have an erroneous setter, which fails on bad input, you place the error message closer to the source of it. At the cost of having validation logic in multiple locations and forcing the user to do additional error checking.

I can argue it both ways, so I will let @lmb get his say in first 😄 .

[lmb]

Do you care about line, column and file at all? The only thing that the verifier currently renders is the line, all the other bits can stay zero. We could consider just exporting Line.Line. As an alternative, we could turn any Source that is a Stringer into a Line:

if stringer, ok := ins.Source().(fmt.Stringer); ok {
   // synthesise line info with line 0, col 0, etc.
}

P.S. That would imply that Comment are emitted into the verifier log.

[MarcusWichelmann]

As an alternative, we could turn any Source that is a Stringer into a Line.

That would also work. All I want is adding a comment string to the line that is also visible in the verifier log / instruction dump. The file/line/column information of the placeholder that was replaced isn't that interesting in my case, anyway, so it could just be zero.

[MarcusWichelmann]

@lmb So we'll go that route? I'll submit a PR for that tomorrow.

[lmb]

@MarcusWichelmann SGTM.