imgact: Mark brandinfo and note structures as const

[markj]

Summary

No functional change intended.

[emaste]

LGTM excluding the accidentally included file. Also I think the commit message ought to reference any changes in ELF sections containing these.

sys/netinet/ip_carp.c

2366 ↗

(On Diff #164084)

this file should be excluded from this change

[markj]

Also I think the commit message ought to reference any changes in ELF sections containing these.

I don't understand, what changes are you referring to?

[emaste]

Do they get placed in rodata with the change?

[markj]

In D53062#1212373, @emaste wrote:

Do they get placed in rodata with the change?

I didn't verify, but I would expect so, yes.

[kib]

sys/kern/imgact_elf.c

288

I do not object against the change, but want to note that if we start linking brand notes into the list instead of using external elf_brand_list[], the issue of limited number of array elements would be solved. But then brandinfo cannot be const.

It is not relevant now, but was some time ago when brands were added regularly.

sys/netinet/ip_carp.c

2366 ↗

(On Diff #164084)

The whole file' changes are unrelated, not just this line.

[markj]

sys/kern/imgact_elf.c

288

Why can't we increase MAX_BRANDS?

[olce]

Please don't forget to exclude the changes in sys/netinet/ip_carp.c as they are completely unrelated.

sys/kern/imgact_elf.c

288

Why can't we increase MAX_BRANDS?

I don't see why personally.

In any case, this is in fact independent to passing pointers to constant Elf_Brandinfo eveywhere because, unless I'm missing something, we can easily point them from a mutable wrapper structure forming the list.

[kib]

sys/kern/imgact_elf.c

288

Increase by how much?

Fixes-sized tables in kernel are wrong, in this case in particular. It only works because projects like glibc/kfreebsd stagnated.

[markj]

sys/kern/imgact_elf.c

288

Increase by how much?

As much as needed to accommodate some new addition. I do not see why it cannot be changed whenever necessary.

Or change to a convention where there is a null terminator at the end of each table, then MAX_BRANDS is not needed.

[kib]

sys/kern/imgact_elf.c

288

I do not understand this reply. The elf_brand_list[] array is used to dynamically register brands (at module load time). E.g. linux{64,32}.ko do it.

[markj]

sys/kern/imgact_elf.c

288

I see now, I missed that.

[olce]

sys/kern/imgact_elf.c

288

Fixes-sized tables in kernel are wrong, in this case in particular. It only works because projects like glibc/kfreebsd stagnated.
I do not understand this reply. The elf_brand_list[] array is used to dynamically register brands (at module load time). E.g. linux{64,32}.ko do it.

I think I understand what you mean. Fixed-sizes tables are wrong in principle, yes, but in practice do we support more than a few brands? Are there many more brands supported outside the tree? Does it have any chance to change in the short/mid-term?

(This is a discussion independent of the changes here anyway as, as hinted above, "constifying" Elf_Brandinfo everywhere does not preclude switching to a linked list.)

[kib]

sys/kern/imgact_elf.c

288

I described this in earlier response: this is generic KPI, we might want to support more, I do not know about out-of-tree ABI modules.

Limit on the total of number of brands was problematic before, when the projects like kfreebsd were active.

[zlei]

sys/kern/imgact_elf.c

288

And, the elf_brand_list is not properly protected from concurrent access, from my first glance.

[kib]

sys/kern/imgact_elf.c

288

Well, formally you might be right, but practically the list is modified on module load by initializers, so kld_sx is probably provides enough serialization. I do not think we need to try to fix the non-existent problem there.