On Wed, 2024-02-28 at 15:55 -0800, Andrii Nakryiko wrote: > On Tue, Feb 27, 2024 at 6:11 PM Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote: [...] > > Instead of adding struct_ops_refs and autoload_user_set, > > > > for BPF_PROG_TYPE_STRUCT_OPS, how about deciding to load it or not by checking > > prog->attach_btf_id (non zero) alone. The prog->attach_btf_id is now decided at > > load time and is only set if it is used by at least one autocreate map, if I > > read patch 2 & 3 correctly. > > > > Meaning ignore prog->autoload*. Load the struct_ops prog as long as it is used > > by one struct_ops map with autocreate == true. > > > > If the struct_ops prog is not used in any struct_ops map, the bpf prog cannot be > > loaded even the autoload is set. If bpf prog is used in a struct_ops map and its > > autoload is set to false, the struct_ops map will be in broken state. Thus, > > We can easily detect this condition and report meaningful error. > > > prog->autoload does not fit very well with struct_ops prog and may as well > > depend on whether the struct_ops prog is used by a struct_ops map alone? > > I think it's probably fine from a usability standpoint to disable > loading the BPF program if its struct_ops map was explicitly set to > not auto-create. It's a bit of deviation from other program types, but > in practice this logic will make it easier for users. > > One question I have, though, is whether we should report as an error a > stand-alone struct_ops BPF program that is not used from any > struct_ops map? Or should we load it nevertheless? Or should we > silently not load it? > > I feel like silently not loading is the worst behavior here. So either > loading it anyway or reporting an error would be my preference, > probably. The following properties of the struct_ops program are set based on the corresponding struct_ops map: - attach_btf_id - BTF id of the kernel struct_ops type; - expected_attach_type - member index of function pointer inside the kernel type. No corresponding map means above fields are not set, means program fails to load with error report. So I think it is fine to try loading such programs w/o any additional processing.
