On Wed, Oct 20, 2021 at 4:28 PM Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: > > it's subjective, but "bpf_export_errno" name is quite confusing. What > are we "exporting" and where? > > I actually like Song's proposal for two helpers, > bpf_set_err()/bpf_get_err(). It makes the semantics less confusing. I > honestly don't remember the requirement to have one combined helper > from the BPF office hour discussion, but if there was a good reason > for that, please remind us. > > > + * Description > > + * If *errno_val* is positive, set the syscall's return error code; > > This inversion of error code is also confusing. If we are to return > -EXXX, bpf_set_err(EXXX) is quite confusing. > > > + * if *errno_val* is zero, retrieve the previously set code. > > Also, are there use cases where zero is the valid "error" (or lack of > it, rather). I.e., wouldn't there be cases where you want to clear a > previous error? We might have discussed this, sorry if I forgot. Hmm, originally I thought it's best to assume the underlying assumption is that filters may set policies and it would violate it if policies become ignored; however one could argue that debugging would be a use case for an error-clearing filter. Let's say we do bpf_set_err()/bpf_get_err(), with the ability to clear errors. I'm having trouble thinking of the best way to have it interact with the getsockopt "retval" in its context: * Let's say the kernel initially sets an error code in the retval. I think it would be a surprising behavior if only "retval" but not bpf_get_err() shows the error. Therefore we'd need to initialize "err" with the "retval" if retval is an error. * If we initialize "err" with the "retval", then for a prog to clear the error they'd need to clear it twice, once with bpf_set_err(0) with and another with ctx->retval = 0. This will immediately break backward compatibility. Therefore, we'd need to mirror the setting of ctx->retval = 0 to bpf_set_err(0) * In that case, what to do if a user uses ctx->retval as a way to pass data between filters? I mean, whether ctx->retval is set to 0 or the original is only checked after all filters are run. It could be any value while the filters are running. * A second issue, if we have first a legacy filter that returns 0 to set EPERM, and then there's another filter that does a ctx->retval = 0. The original behavior would be that the syscall fails with EPERM, but if we mirror ctx->retval = 0 to bpf_set_err(0), then that EPERM would be cleared. One of the reasons I liked "export" is that it's slightly clearer that this value is strictly from the BPF's side and has nothing to do with what the kernel sets (as in the getsockopt case). But yeah I agree it's not an ideal name. > But either way, if bpf_set_err() accepted <= 0 and used that as error > value as-is (> 0 should be rejected, probably) that would make for > straightforward logic. Then for getting the current error we can have > a well-paired bpf_get_err()? > > > BTW, "errno" is very strongly associated with user-space errno, do we > want to have this naming association (this is the reason I used "err" > terminology above). Ack. YiFei Zhu
