> 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.
I think I'm missing something crucial somewhere. If I understand
bpf_prog_run_array_cg(() correctly, when called with retval = 0, it
will return -EPERM on failure and 0 on success. If I understand that
correctly, with this change, we'll still return -EPERM on failure but
instead of returning 0 on success, we'll now return the address
length. Am I misunderstanding how bpf_prog_run_array_cg() behaves in
this case?
On Wed, 26 Apr 2023 at 02:06, Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> 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.
