On Mon, Aug 19, 2024 at 6:42 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Sat, Aug 17, 2024 at 5:24 PM Jordan Rome <linux@xxxxxxxxxxxxxx> wrote:
> >
> > This adds tests for both the happy path and
> > the error path.
> >
> > Signed-off-by: Jordan Rome <linux@xxxxxxxxxxxxxx>
> > ---
> > .../selftests/bpf/prog_tests/attach_probe.c | 8 ++-
> > .../selftests/bpf/prog_tests/read_vsyscall.c | 1 +
> > .../selftests/bpf/progs/read_vsyscall.c | 9 ++-
> > .../selftests/bpf/progs/test_attach_probe.c | 57 ++++++++++++++++++-
> > 4 files changed, 68 insertions(+), 7 deletions(-)
> >
>
> Thanks for adding more test cases! See a small nit below, but otherwise LGTM
>
> Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
>
> [...]
>
> >
> > +static __always_inline bool verify_sleepable_user_copy_str(void)
> > +{
> > + int ret;
> > + char data_long[20];
> > + char data_long_pad[20];
> > + char data_long_err[20];
> > + char data_short[4];
> > + char data_short_pad[4];
> > +
> > + ret = bpf_copy_from_user_str(data_short, sizeof(data_short), user_ptr, 0);
> > +
> > + if (bpf_strncmp(data_short, 4, "tes\0") != 0 || ret != 4)
> > + return false;
> > +
> > + ret = bpf_copy_from_user_str(data_short_pad, sizeof(data_short_pad), user_ptr, BPF_F_PAD_ZEROS);
> > +
> > + if (bpf_strncmp(data_short, 4, "tes\0") != 0 || ret != 4)
> > + return false;
> > +
> > + ret = bpf_copy_from_user_str(data_long, sizeof(data_long), user_ptr, 0);
> > +
> > + if (bpf_strncmp(data_long, 10, "test_data\0") != 0 || ret != 10)
> > + return false;
> > +
> > + ret = bpf_copy_from_user_str(data_long_pad, sizeof(data_long_pad), user_ptr, BPF_F_PAD_ZEROS);
> > +
> > + if (bpf_strncmp(data_long_pad, 10, "test_data\0") != 0 || ret != 10 || data_long_pad[19] != '\0')
> > + return false;
> > +
> > + ret = bpf_copy_from_user_str(data_long_err, sizeof(data_long_err), (void *)data_long, BPF_F_PAD_ZEROS);
> > +
> > + if (ret > 0 || data_long_err[9] != '\0')
>
> shouldn't the condition be something along the data_long_err[0] !=
> '\0' || data_long_err[19] != '\0' to check that the entire buffer is
> zeroed out?
>
Good catch. I think in a previous iteration `data_long` was only 10
chars long. Will fix.
> > + return false;
> > +
> > + ret = bpf_copy_from_user_str(data_long, sizeof(data_long), user_ptr, 2);
> > +
> > + if (ret != -EINVAL)
> > + return false;
> > +
> > + return true;
> > +}
> > +
>
> [...]
