On Thu, Aug 17, 2023 at 1:41 PM Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote:
>
> On 8/16/23 6:25 PM, Alexei Starovoitov wrote:
> > But I think we have to step back. Why do we need this whole thing in the first place?
> > _why_ sockopt bpf progs needs to read and write user memory?
> >
> > Yes there is one page limit, but what is the use case to actually read and write
> > beyond that? iptables sockopt was mentioned, but I don't think bpf prog can do
> > anything useful with iptables binary blobs. They are hard enough for kernel to parse.
>
> Usually the bpf prog is only interested in a very small number of optnames and
> no need to read the optval at all for most cases. The max size for our use cases
> is 16 bytes. The kernel currently is kind of doing it the opposite and always
> assumes the bpf prog needing to use the optval, allocate kernel memory and
> copy_from_user such that the non-sleepable bpf program can read/write it.
>
> The bpf prog usually checks the optname and then just returns for most cases:
>
> SEC("cgroup/getsockopt")
> int get_internal_sockopt(struct bpf_sockopt *ctx)
> {
> if (ctx->optname != MY_INTERNAL_SOCKOPT)
> return 1;
>
> /* change the ctx->optval and return to user space ... */
> }
>
> When the optlen is > PAGE_SIZE, the kernel only allocates PAGE_SIZE memory and
> copy the first PAGE_SIZE data from the user optval. We used to ask the bpf prog
> to explicitly set the optlen to 0 for > PAGE_SIZE case even it has not looked at
> the optval. Otherwise, the kernel used to conclude that the bpf prog had set an
> invalid optlen because optlen is larger than the optval_end - optval and
> returned -EFAULT incorrectly to the end-user.
>
> The bpf prog started doing this > PAGE_SIZE check and set optlen = 0 due to an
> internal kernel PAGE_SIZE limitation:
>
> SEC("cgroup/getsockopt")
> int get_internal_sockopt(struct bpf_sockopt *ctx)
> {
> if (ctx->optname != MY_INTERNAL_SOCKOPT) {
> /* only do that for ctx->optlen > PAGE_SIZE.
> * otherwise, the following cgroup bpf prog will
> * not be able to use the optval that it may
> * be interested.
> */
> if (ctx->optlen > PAGE_SIZE)
> ctx->optlen = 0;
> return 1;
> }
>
> /* change the ctx->optval and return to user space ... */
> }
>
> The above has been worked around in commit 29ebbba7d461 ("bpf: Don't EFAULT for
> {g,s}setsockopt with wrong optlen").
>
> Later, it was reported that an optname (NETLINK_LIST_MEMBERSHIPS) that the
> kernel allows a user passing NULL optval and using the optlen returned by
> getsockopt to learn the buffer space required. The bpf prog then needs to remove
> the optlen > PAGE_SIZE check and set optlen to 0 for _all_ optnames that it is
> not interested while risking the following cgroup prog may not be able to use
> some of the optval:
>
> SEC("cgroup/getsockopt")
> int get_internal_sockopt(struct bpf_sockopt *ctx)
> {
> if (ctx->optname != MY_INTERNAL_SOCKOPT) {
>
> /* Do that for all optname that you are not interested.
> * The latter cgroup bpf will not be able to use the optval.
> */
> ctx->optlen = 0;
> return 1;
> }
>
> /* chage the ctx->optval and return to user space ... */
> }
>
> The above case has been addressed in commit 00e74ae08638 ("bpf: Don't EFAULT for
> getsockopt with optval=NULL").
Agree with all of the above.
Existing bpf sockopt interfaces was problematic,
but with these workarounds we fixed all known issues.
> To avoid other potential optname cases that may run into similar situation and
> requires the bpf prog work around it again like the above, it needs a way to
> track whether a bpf prog has changed the optval in runtime. If it is not
> changed, use the result from the kernel set/getsockopt. If reading/writing to
> optval is done through a kfunc, this can be tracked. The kfunc can also directly
> read/write the user memory in optval, avoid the pre-alloc kernel memory and the
> PAGE_SIZE limit but this is a minor point.
so I'm still not following what sleepable progs that can access everything
would help the existing situation.
I agree that sleepable bpf sockopt should be free from old mistakes,
but people might still write old-style non-sleeptable bpf sockopt and
might repeat the same mistakes.
I'm missing the value of the new interface. It's better, sure, but why?
Do we expect to rewrite existing sockopt progs in sleepable way?
It might not be easy due to sleepable limitations like maps and whatever else.
