> On Fri, Apr 21, 2023 at 06:27:11PM +0200, Daan De Meyer wrote:
> > *
> > * This function will return %-EPERM if an attached program is found and
> > - * returned value != 1 during execution. In all other cases, 0 is returned.
> > + * returned value != 1 during execution. In all other cases, the new address
> > + * length of the sockaddr is returned.
> > */
> > int __cgroup_bpf_run_filter_sock_addr(struct sock *sk,
> > struct sockaddr *uaddr,
> > + u32 uaddrlen,
> > enum cgroup_bpf_attach_type atype,
> > void *t_ctx,
> > u32 *flags)
> > @@ -1469,9 +1472,11 @@ int __cgroup_bpf_run_filter_sock_addr(struct sock *sk,
> > .sk = sk,
> > .uaddr = uaddr,
> > .t_ctx = t_ctx,
> > + .uaddrlen = uaddrlen,
> > };
> > struct sockaddr_storage unspec;
> > struct cgroup *cgrp;
> > + int ret;
> >
> > /* Check socket family since not all sockets represent network
> > * endpoint (e.g. AF_UNIX).
> > @@ -1482,11 +1487,16 @@ int __cgroup_bpf_run_filter_sock_addr(struct sock *sk,
> > if (!ctx.uaddr) {
> > memset(&unspec, 0, sizeof(unspec));
> > ctx.uaddr = (struct sockaddr *)&unspec;
> > + ctx.uaddrlen = sizeof(unspec);
> > }
> >
> > cgrp = sock_cgroup_ptr(&sk->sk_cgrp_data);
> > - return bpf_prog_run_array_cg(&cgrp->bpf, atype, &ctx, bpf_prog_run,
> > - 0, flags);
> > + ret = bpf_prog_run_array_cg(&cgrp->bpf, atype, &ctx, bpf_prog_run,
> > + 0, flags);
> > + if (ret)
> > + return ret;
> > +
> > + return (int) ctx.uaddrlen;
>
> But that is big behavioral change..
> instead of 0 or 1 now it will be sizeof(unspec) or 1?
> That will surely break some of the __cgroup_bpf_run_filter_sock_addr callers.
It will now always return the size of the addrlen as set by the bpf
program or the original addrlen if the bpf program did not change it.
I modified all the callers of __cgroup_bpf_run_filter_sock_addr() to
ignore the returned addrlen so as to not introduce any breakages. Only
when unix socket support is introduced in the following patch do we
actually start making use of the addrlen returned by
__cgroup_bpf_run_filter_sock_addr(). Alternatively, I can pass in an
optional pointer to __cgroup_bpf_run_filter_sock_addr() which is set
to the modified addrlen if it is provided and then only make use of
this in af_unix.c, but I figure since we're already returning an int,
we can use that to propagate the modified address length as well.
bpf_prog_run_array_cg() will return either 0 or -EPERM so we'll either
return an error if an error occurs or the modified address length if
no error occurs.
For the default size of the address length if none is provided, I used
sizeof(unspec) since that's the size of the memory we provide to the
BPF program, but I suppose that zero could also make sense here, to
indicate that we're providing an empty address. Let me know which one
is preferred.
On Fri, 21 Apr 2023 at 22:55, Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Fri, Apr 21, 2023 at 06:27:11PM +0200, Daan De Meyer wrote:
> > *
> > * This function will return %-EPERM if an attached program is found and
> > - * returned value != 1 during execution. In all other cases, 0 is returned.
> > + * returned value != 1 during execution. In all other cases, the new address
> > + * length of the sockaddr is returned.
> > */
> > int __cgroup_bpf_run_filter_sock_addr(struct sock *sk,
> > struct sockaddr *uaddr,
> > + u32 uaddrlen,
> > enum cgroup_bpf_attach_type atype,
> > void *t_ctx,
> > u32 *flags)
> > @@ -1469,9 +1472,11 @@ int __cgroup_bpf_run_filter_sock_addr(struct sock *sk,
> > .sk = sk,
> > .uaddr = uaddr,
> > .t_ctx = t_ctx,
> > + .uaddrlen = uaddrlen,
> > };
> > struct sockaddr_storage unspec;
> > struct cgroup *cgrp;
> > + int ret;
> >
> > /* Check socket family since not all sockets represent network
> > * endpoint (e.g. AF_UNIX).
> > @@ -1482,11 +1487,16 @@ int __cgroup_bpf_run_filter_sock_addr(struct sock *sk,
> > if (!ctx.uaddr) {
> > memset(&unspec, 0, sizeof(unspec));
> > ctx.uaddr = (struct sockaddr *)&unspec;
> > + ctx.uaddrlen = sizeof(unspec);
> > }
> >
> > cgrp = sock_cgroup_ptr(&sk->sk_cgrp_data);
> > - return bpf_prog_run_array_cg(&cgrp->bpf, atype, &ctx, bpf_prog_run,
> > - 0, flags);
> > + ret = bpf_prog_run_array_cg(&cgrp->bpf, atype, &ctx, bpf_prog_run,
> > + 0, flags);
> > + if (ret)
> > + return ret;
> > +
> > + return (int) ctx.uaddrlen;
>
> But that is big behavioral change..
> instead of 0 or 1 now it will be sizeof(unspec) or 1?
> That will surely break some of the __cgroup_bpf_run_filter_sock_addr callers.
