Re: [PATCH bpf-next 2/3] bpf: Add cgroup helper bpf_export_errno to get/set exported errno value

[YiFei Zhu <zhuyifei@google.com>]

Yeah it felt like we only needed one helper for the parameters and
return values to be unambiguous. But if two better avoid confusion for
users, we can do that.

YiFei Zhu

On Thu, Oct 7, 2021 at 8:11 AM <sdf@google.com> wrote:
>
> On 10/06, Song Liu wrote:
> > On Wed, Oct 6, 2021 at 5:41 PM Song Liu <song@kernel.org> wrote:
> > >
> > > On Wed, Oct 6, 2021 at 9:04 AM YiFei Zhu <zhuyifei1999@gmail.com> wrote:
> > > >
> > > > From: YiFei Zhu <zhuyifei@google.com>
> > > >
> > > > When passed in a positive errno, it sets the errno and returns 0.
> > > > When passed in 0, it gets the previously set errno. When passed in
> > > > an out of bound number, it returns -EINVAL. This is unambiguous:
> > > > negative return values are error in invoking the helper itself,
> > > > and positive return values are errnos being exported. Errnos once
> > > > set cannot be unset, but can be overridden.
> > > >
> > > > The errno value is stored inside bpf_cg_run_ctx for ease of access
> > > > different prog types with different context structs layouts. The
> > > > helper implementation can simply perform a container_of from
> > > > current->bpf_ctx to retrieve bpf_cg_run_ctx.
> > > >
> > > > For backward compatibility, if a program rejects without calling
> > > > the helper, and the errno has not been set by any prior progs, the
> > > > BPF_PROG_RUN_ARRAY_CG family macros automatically set the errno to
> > > > EPERM. If a prog sets an errno but returns 1 (allow), the outcome
> > > > is considered implementation-defined. This patch treat it the same
> > > > way as if 0 (reject) is returned.
> > > >
> > > > For BPF_PROG_CGROUP_INET_EGRESS_RUN_ARRAY, the prior behavior is
> > > > that, if the return value is NET_XMIT_DROP, the packet is silently
> > > > dropped. We preserve this behavior for backward compatibility
> > > > reasons, so even if an errno is set, the errno does not return to
> > > > caller.
> > > >
> > > > For getsockopt hooks, they are different in that bpf progs runs
> > > > after kernel processes the getsockopt syscall instead of before.
> > > > There is also a retval in its context struct in which bpf progs
> > > > can unset the retval, and can force an -EPERM by returning 0.
> > > > We preseve the same semantics. Even though there is retval,
> > > > that value can only be unset, while progs can set (and not unset)
> > > > additional errno by using the helper, and that will override
> > > > whatever is in retval.
> > > >
> > > > Signed-off-by: YiFei Zhu <zhuyifei@google.com>
> > > > Reviewed-by: Stanislav Fomichev <sdf@google.com>
> > >
> > > This is pretty complicated, but the logic looks all correct. Thus,
> > >
> > > Acked-by: Song Liu <songliubraving@fb.com>
> > >
> > > One question, if the program want to retrieve existing errno_val, and
> > > set a different one, it needs to call the helper twice, right? I guess
> > it
> > > is possible to do that in one call with a "swap" logic. Would this work?
>
> > Actually, how about we split this into two helpers:bpf_set_errno() and
> > bpf_get_errno(). This should avoid some confusion in long term.
>
> We've agreed on the single helper during bpf office hours (about 2 weeks
> ago), but we can do two, I don't think it matters that much.