Re: [Question]: BPF_CGROUP_{GET,SET}SOCKOPT handling when optlen > PAGE_SIZE

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

On 10/27/22 1:46 PM, Andrii Nakryiko wrote:
> On Wed, Oct 26, 2022 at 6:17 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>>
>> The cgroup-bpf {get,set}sockopt prog is useful to change the optname behavior.
>> The bpf prog usually just handles a few specific optnames and ignores most
>> others.  For the optnames that it ignores, it usually does not need to change
>> the optlen.  The exception is when optlen > PAGE_SIZE (or optval_end - optval).
>> The bpf prog needs to set the optlen to 0 for this case or else the kernel will
>> return -EFAULT to the userspace.  It is usually not what the bpf prog wants
>> because the bpf prog only expects error returning to userspace when it has
>> explicitly 'return 0;' or used bpf_set_retval().  If a bpf prog always changes
>> optlen for optnames that it does not care to 0,  it may risk if the latter bpf
>> prog in the same cgroup may want to change/look-at it.
>>
>> Would like to explore if there is an easier way for the bpf prog to handle it.
>> eg. does it make sense to track if the bpf prog has changed the ctx->optlen
>> before returning -EFAULT to the user space when ctx.optlen > max_optlen?
> 
> Maybe optlen + optval/optval_end could be replaced with dynptr? If we
> do a new type of dynptr (DYNPTR_CTXBUF or something like that), we can
> implement tracking of whether it was ever modified through
> bpf_dynptr_write() or if direct memory access was ever used (was
> bpf_dynptr_data() called). Not sure how you'd know if
> bpf_dynptr_data() was used to modify data, though (this is where
> bpf_dynptr_data_rdonly() vs bpf_dynptr_data() would be helpful,
> perhaps). But just a seed of an idea, maybe you guys can somehow fit
> it here?

Yep, dynptr can be used here.  May be one dynptr specifically for sockptr_t. The 
dynptr will have the __user pointer first to avoid a big (and potentially bogus) 
kernel buffer pre allocation.  Then only read and write it through the 
bpf_dynptr_read() and write().  Since it is __user pointer, it won't be directly 
accessible through data slice with bpf_dynptr_data().  It should not be a perf 
issue, most of the common optval is an integer.  The worst common case is the 16 
bytes tcp-cc name.   The bpf prog has to be sleepable first though which I think 
will be a useful thing to have.