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

[sdf@google.com]

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.