On Mon, Apr 24, 2023 at 03:58:24PM +0200, Daan De Meyer 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.
>
> 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.
I still don't understand how it's possible to modify the callers to
have correct behavior.
- return bpf_prog_run_array_cg();
+ ret = bpf_prog_run_array_cg();
+ if (ret)
+ return ret;
+
+ return (int) ctx.uaddrlen;
It used to return 0 or 1.
Now 1 is indistinguishable between 1 from prog and 0 from prog, but uaddrlen == 1.
I don't see how callers can deal with that.
