Re: [PATCH bpf-next 3/6] bpf: Don't EFAULT for {g,s}setsockopt with wrong optlen

[Martin KaFai Lau <martin.lau@linux.dev>]

On 4/21/23 9:09 AM, Stanislav Fomichev wrote:
> On Fri, Apr 21, 2023 at 8:24 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>>
>> On 4/19/23 12:53 AM, Stanislav Fomichev wrote:
>>> Over time, we've found out several special socket option cases which need
>>> special treatment. And if BPF program doesn't handle them correctly, this
>>> might EFAULT perfectly valid {g,s}setsockopt calls.
>>>
>>> The intention of the EFAULT was to make it apparent to the
>>> developers that the program is doing something wrong.
>>> However, this inadvertently might affect production workloads
>>> with the BPF programs that are not too careful.
>>
>> Took in the first two for now. It would be good if the commit description
>> in here could have more details for posterity given this is too vague.
> 
> Thanks! Will try to repost next week with more details.
> 
>>> Let's try to minimize the chance of BPF program screwing up userspace
>>> by ignoring the output of those BPF programs (instead of returning
>>> EFAULT to the userspace). pr_info_ratelimited those cases to
>>> the dmesg to help with figuring out what's going wrong.
>>>
>>> Fixes: 0d01da6afc54 ("bpf: implement getsockopt and setsockopt hooks")
>>> Suggested-by: Martin KaFai Lau <martin.lau@kernel.org>
>>> Signed-off-by: Stanislav Fomichev <sdf@google.com>
>>> ---
>>>    kernel/bpf/cgroup.c | 8 ++++++--
>>>    1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
>>> index a06e118a9be5..af4d20864fb4 100644
>>> --- a/kernel/bpf/cgroup.c
>>> +++ b/kernel/bpf/cgroup.c
>>> @@ -1826,7 +1826,9 @@ int __cgroup_bpf_run_filter_setsockopt(struct sock *sk, int *level,
>>>                ret = 1;
>>>        } else if (ctx.optlen > max_optlen || ctx.optlen < -1) {
>>>                /* optlen is out of bounds */
>>> -             ret = -EFAULT;
>>> +             pr_info_ratelimited(
>>> +                     "bpf setsockopt returned unexpected optlen=%d (max_optlen=%d)\n",
>>> +                     ctx.optlen, max_optlen);
>>
>> Does it help any regular user if this log message is seen? I kind of doubt it a bit,
>> it might create more confusion if log gets spammed with it, imo.
> 
> Agreed, but we need some way to let the users know that their bpf
> program is doing the wrong thing (if they set the optlen too high for
> example).

imo, I also think a printk here will be a noise in dmesg most of the time (but 
much better than an unexpected -EFAULT).

> Any other better alternatives to expose those events?

Is it possible to only -EFAULT when the bpf prog setting a ctx.optlen larger 
than the "original" user provided optlen?
Ignore for all other cases that is due to the current PAGE_SIZE limitation?

> 
>>>        } else {
>>>                /* optlen within bounds, run kernel handler */
>>>                ret = 0;
>>> @@ -1922,7 +1924,9 @@ int __cgroup_bpf_run_filter_getsockopt(struct sock *sk, int level,
>>>                goto out;
>>>
>>>        if (optval && (ctx.optlen > max_optlen || ctx.optlen < 0)) {
>>> -             ret = -EFAULT;
>>> +             pr_info_ratelimited(
>>> +                     "bpf getsockopt returned unexpected optlen=%d (max_optlen=%d)\n",
>>> +                     ctx.optlen, max_optlen);
>>>                goto out;
>>>        }
>>>
>>>
>>