On Fri, Apr 28, 2023 at 5:44 PM Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote:
>
> On 4/28/23 5:32 PM, Stanislav Fomichev wrote:
> > On Fri, Apr 28, 2023 at 4:59 PM Stanislav Fomichev <sdf@xxxxxxxxxx> wrote:
> >>
> >> On Fri, Apr 28, 2023 at 4:57 PM Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote:
> >>>
> >>> On 4/27/23 1:04 PM, Stanislav Fomichev wrote:
> >>>> Instead of assuming EFAULT, let's assume the BPF program's
> >>>> output is ignored.
> >>>>
> >>>> Remove "getsockopt: deny arbitrary ctx->retval" because it
> >>>> was actually testing optlen. We have separate set of tests
> >>>> for retval.
> >>>>
> >>>> Signed-off-by: Stanislav Fomichev <sdf@xxxxxxxxxx>
> >>>> ---
> >>>> .../selftests/bpf/prog_tests/sockopt.c | 80 +++++++++++++++++--
> >>>> 1 file changed, 74 insertions(+), 6 deletions(-)
> >>>>
> >>>> diff --git a/tools/testing/selftests/bpf/prog_tests/sockopt.c b/tools/testing/selftests/bpf/prog_tests/sockopt.c
> >>>> index aa4debf62fc6..8dad30ce910e 100644
> >>>> --- a/tools/testing/selftests/bpf/prog_tests/sockopt.c
> >>>> +++ b/tools/testing/selftests/bpf/prog_tests/sockopt.c
> >>>> @@ -273,10 +273,30 @@ static struct sockopt_test {
> >>>> .error = EFAULT_GETSOCKOPT,
> >>>> },
> >>>> {
> >>>> - .descr = "getsockopt: deny arbitrary ctx->retval",
> >>>> + .descr = "getsockopt: ignore >PAGE_SIZE optlen",
> >>>> .insns = {
> >>>> - /* ctx->retval = 123 */
> >>>> - BPF_MOV64_IMM(BPF_REG_0, 123),
> >>>> + /* write 0xFF to the first optval byte */
> >>>> +
> >>>> + /* r6 = ctx->optval */
> >>>> + BPF_LDX_MEM(BPF_DW, BPF_REG_6, BPF_REG_1,
> >>>> + offsetof(struct bpf_sockopt, optval)),
> >>>> + /* r2 = ctx->optval */
> >>>> + BPF_MOV64_REG(BPF_REG_2, BPF_REG_6),
> >>>> + /* r6 = ctx->optval + 1 */
> >>>> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_6, 1),
> >>>> +
> >>>> + /* r7 = ctx->optval_end */
> >>>> + BPF_LDX_MEM(BPF_DW, BPF_REG_7, BPF_REG_1,
> >>>> + offsetof(struct bpf_sockopt, optval_end)),
> >>>> +
> >>>> + /* if (ctx->optval + 1 <= ctx->optval_end) { */
> >>>> + BPF_JMP_REG(BPF_JGT, BPF_REG_6, BPF_REG_7, 1),
> >>>> + /* ctx->optval[0] = 0xF0 */
> >>>> + BPF_ST_MEM(BPF_B, BPF_REG_2, 0, 0xFF),
> >>>> + /* } */
> >>>> +
> >>>> + /* ctx->retval = 0 */
> >>>> + BPF_MOV64_IMM(BPF_REG_0, 0),
> >>>> BPF_STX_MEM(BPF_W, BPF_REG_1, BPF_REG_0,
> >>>> offsetof(struct bpf_sockopt, retval)),
> >>>>
> >>>> @@ -287,9 +307,10 @@ static struct sockopt_test {
> >>>> .attach_type = BPF_CGROUP_GETSOCKOPT,
> >>>> .expected_attach_type = BPF_CGROUP_GETSOCKOPT,
> >>>>
> >>>> - .get_optlen = 64,
> >>>> -
> >>>> - .error = EFAULT_GETSOCKOPT,
> >>>> + .get_level = 1234,
> >>>> + .get_optname = 5678,
> >>>> + .get_optval = {}, /* the changes are ignored */
> >>>> + .get_optlen = 4096 + 1,
> >>>
> >>> The patchset looks good. Thanks for taking care of it.
> >>>
> >>> One question, is it safe to the assume 4096 page size for all platforms in the
> >>> selftests?
> >>
> >> Good question; let me respin with sysconf() just to be safe..
> >
> > Argh, the compiler yells at me:
> > error: initializer element is not a compile-time constant
> >
> > I guess I'm just gonna do #define PAGE_SIZE 4096 and if we do hit some
> > problems on the other archs, I'll ifdef it in one place.
>
> or run_test() can reinit optlen to sysconf_page_size + 1 if optlen == 4097.
Maybe I can do something like the following?
if (test->set_optlen >= PAGE_SIZE) {
int num_pages = test->set_optlen / PAGE_SIZE;
int remainder = test->set_optlen % PAGE_SIZE;
test->set_optlen = num_pages *
sysconf(_SC_PAGESIZE) + remainder;
}
More verbose, but less magical than depending on 4097. For the BPF
side, I can probably pass proper value via bss..
