[PATCH bpf-next 1/2] bpf: allow __sk_buff tstamp in BPF_PROG_TEST_RUN

[PATCH bpf-next 1/2] bpf: allow __sk_buff tstamp in BPF_PROG_TEST_RUN

[Stanislav Fomichev]

It's useful for implementing EDT related tests (set tstamp, run the
test, see how the tstamp is changed or observe some other parameter).

Note that bpf_ktime_get_ns() helper is using monotonic clock, so for
the BPF programs that compare tstamp against it, tstamp should be
derived from clock_gettime(CLOCK_MONOTONIC, ...).

Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 net/bpf/test_run.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index 1153bbcdff72..0be4497cb832 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -218,10 +218,18 @@ static int convert___skb_to_skb(struct sk_buff *skb, struct __sk_buff *__skb)
 
 	if (!range_is_zero(__skb, offsetof(struct __sk_buff, cb) +
 			   FIELD_SIZEOF(struct __sk_buff, cb),
+			   offsetof(struct __sk_buff, tstamp)))
+		return -EINVAL;
+
+	/* tstamp is allowed */
+
+	if (!range_is_zero(__skb, offsetof(struct __sk_buff, tstamp) +
+			   FIELD_SIZEOF(struct __sk_buff, tstamp),
 			   sizeof(struct __sk_buff)))
 		return -EINVAL;
 
 	skb->priority = __skb->priority;
+	skb->tstamp = __skb->tstamp;
 	memcpy(&cb->data, __skb->cb, QDISC_CB_PRIV_LEN);
 
 	return 0;
@@ -235,6 +243,7 @@ static void convert_skb_to___skb(struct sk_buff *skb, struct __sk_buff *__skb)
 		return;
 
 	__skb->priority = skb->priority;
+	__skb->tstamp = skb->tstamp;
 	memcpy(__skb->cb, &cb->data, QDISC_CB_PRIV_LEN);
 }
 
-- 
2.23.0.700.g56cf767bdb-goog

[PATCH bpf-next 2/2] selftests: bpf: add selftest for __sk_buff tstamp

[Stanislav Fomichev]

Make sure BPF_PROG_TEST_RUN accepts tstamp and exports any
modifications that BPF program does.

Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 tools/testing/selftests/bpf/prog_tests/skb_ctx.c | 5 +++++
 tools/testing/selftests/bpf/progs/test_skb_ctx.c | 1 +
 2 files changed, 6 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/skb_ctx.c b/tools/testing/selftests/bpf/prog_tests/skb_ctx.c
index e95baa32e277..a2eb8db8dafb 100644
--- a/tools/testing/selftests/bpf/prog_tests/skb_ctx.c
+++ b/tools/testing/selftests/bpf/prog_tests/skb_ctx.c
@@ -10,6 +10,7 @@ void test_skb_ctx(void)
 		.cb[3] = 4,
 		.cb[4] = 5,
 		.priority = 6,
+		.tstamp = 7,
 	};
 	struct bpf_prog_test_run_attr tattr = {
 		.data_in = &pkt_v4,
@@ -86,4 +87,8 @@ void test_skb_ctx(void)
 		   "ctx_out_priority",
 		   "skb->priority == %d, expected %d\n",
 		   skb.priority, 7);
+	CHECK_ATTR(skb.tstamp != 8,
+		   "ctx_out_tstamp",
+		   "skb->tstamp == %lld, expected %d\n",
+		   skb.tstamp, 8);
 }
diff --git a/tools/testing/selftests/bpf/progs/test_skb_ctx.c b/tools/testing/selftests/bpf/progs/test_skb_ctx.c
index 7a80960d7df1..2a9f4c736ebc 100644
--- a/tools/testing/selftests/bpf/progs/test_skb_ctx.c
+++ b/tools/testing/selftests/bpf/progs/test_skb_ctx.c
@@ -16,6 +16,7 @@ int process(struct __sk_buff *skb)
 		skb->cb[i]++;
 	}
 	skb->priority++;
+	skb->tstamp++;
 
 	return 0;
 }
-- 
2.23.0.700.g56cf767bdb-goog

Re: [PATCH bpf-next 1/2] bpf: allow __sk_buff tstamp in BPF_PROG_TEST_RUN

[Martin Lau]

On Tue, Oct 15, 2019 at 11:31:24AM -0700, Stanislav Fomichev wrote:
> It's useful for implementing EDT related tests (set tstamp, run the
> test, see how the tstamp is changed or observe some other parameter).
> 
> Note that bpf_ktime_get_ns() helper is using monotonic clock, so for
> the BPF programs that compare tstamp against it, tstamp should be
> derived from clock_gettime(CLOCK_MONOTONIC, ...).
Please provide a cover letter next time.  It makes ack-all possible.

Acked-by: Martin KaFai Lau <kafai@fb.com>

Re: [PATCH bpf-next 2/2] selftests: bpf: add selftest for __sk_buff tstamp

[Martin Lau]

On Tue, Oct 15, 2019 at 11:31:25AM -0700, Stanislav Fomichev wrote:
> Make sure BPF_PROG_TEST_RUN accepts tstamp and exports any
> modifications that BPF program does.
Acked-by: Martin KaFai Lau <kafai@fb.com>

Re: [PATCH bpf-next 1/2] bpf: allow __sk_buff tstamp in BPF_PROG_TEST_RUN

[Stanislav Fomichev]

On 10/15, Martin Lau wrote:
> On Tue, Oct 15, 2019 at 11:31:24AM -0700, Stanislav Fomichev wrote:
> > It's useful for implementing EDT related tests (set tstamp, run the
> > test, see how the tstamp is changed or observe some other parameter).
> > 
> > Note that bpf_ktime_get_ns() helper is using monotonic clock, so for
> > the BPF programs that compare tstamp against it, tstamp should be
> > derived from clock_gettime(CLOCK_MONOTONIC, ...).
> Please provide a cover letter next time.  It makes ack-all possible.
SG, I'll try to add cover letter in the future if that helps.

If I remember correctly, acked-by to the cover letter was not
showing up in the patchwork and people usually do it for each patch
anyway. That's why I didn't bother to do it for this small change.

> Acked-by: Martin KaFai Lau <kafai@fb.com>

Re: [PATCH bpf-next 1/2] bpf: allow __sk_buff tstamp in BPF_PROG_TEST_RUN

[Andrii Nakryiko]

On Tue, Oct 15, 2019 at 2:26 PM Stanislav Fomichev <sdf@google.com> wrote:
>
> It's useful for implementing EDT related tests (set tstamp, run the
> test, see how the tstamp is changed or observe some other parameter).
>
> Note that bpf_ktime_get_ns() helper is using monotonic clock, so for
> the BPF programs that compare tstamp against it, tstamp should be
> derived from clock_gettime(CLOCK_MONOTONIC, ...).
>
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> ---
>  net/bpf/test_run.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
> index 1153bbcdff72..0be4497cb832 100644
> --- a/net/bpf/test_run.c
> +++ b/net/bpf/test_run.c
> @@ -218,10 +218,18 @@ static int convert___skb_to_skb(struct sk_buff *skb, struct __sk_buff *__skb)
>
>         if (!range_is_zero(__skb, offsetof(struct __sk_buff, cb) +
>                            FIELD_SIZEOF(struct __sk_buff, cb),
> +                          offsetof(struct __sk_buff, tstamp)))
> +               return -EINVAL;
> +
> +       /* tstamp is allowed */
> +
> +       if (!range_is_zero(__skb, offsetof(struct __sk_buff, tstamp) +
> +                          FIELD_SIZEOF(struct __sk_buff, tstamp),

with no context on this particular change whatsoever: isn't this the
same as offsetofend(struct __sk_buff, tstamp)? Same above for cb.

Overall, this seems like the 4th similar check, would it make sense to
add a static array of ranges we want to check for zeros and just loop
over it?..

>                            sizeof(struct __sk_buff)))
>                 return -EINVAL;
>
>         skb->priority = __skb->priority;
> +       skb->tstamp = __skb->tstamp;
>         memcpy(&cb->data, __skb->cb, QDISC_CB_PRIV_LEN);
>
>         return 0;
> @@ -235,6 +243,7 @@ static void convert_skb_to___skb(struct sk_buff *skb, struct __sk_buff *__skb)
>                 return;
>
>         __skb->priority = skb->priority;
> +       __skb->tstamp = skb->tstamp;
>         memcpy(__skb->cb, &cb->data, QDISC_CB_PRIV_LEN);
>  }
>
> --
> 2.23.0.700.g56cf767bdb-goog
>

Re: [PATCH bpf-next 1/2] bpf: allow __sk_buff tstamp in BPF_PROG_TEST_RUN

[Alexei Starovoitov]

On Tue, Oct 15, 2019 at 4:15 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Oct 15, 2019 at 2:26 PM Stanislav Fomichev <sdf@google.com> wrote:
> >
> > It's useful for implementing EDT related tests (set tstamp, run the
> > test, see how the tstamp is changed or observe some other parameter).
> >
> > Note that bpf_ktime_get_ns() helper is using monotonic clock, so for
> > the BPF programs that compare tstamp against it, tstamp should be
> > derived from clock_gettime(CLOCK_MONOTONIC, ...).
> >
> > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > ---
> >  net/bpf/test_run.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
> > index 1153bbcdff72..0be4497cb832 100644
> > --- a/net/bpf/test_run.c
> > +++ b/net/bpf/test_run.c
> > @@ -218,10 +218,18 @@ static int convert___skb_to_skb(struct sk_buff *skb, struct __sk_buff *__skb)
> >
> >         if (!range_is_zero(__skb, offsetof(struct __sk_buff, cb) +
> >                            FIELD_SIZEOF(struct __sk_buff, cb),
> > +                          offsetof(struct __sk_buff, tstamp)))
> > +               return -EINVAL;
> > +
> > +       /* tstamp is allowed */
> > +
> > +       if (!range_is_zero(__skb, offsetof(struct __sk_buff, tstamp) +
> > +                          FIELD_SIZEOF(struct __sk_buff, tstamp),
>
> with no context on this particular change whatsoever: isn't this the
> same as offsetofend(struct __sk_buff, tstamp)? Same above for cb.
>
> Overall, this seems like the 4th similar check, would it make sense to
> add a static array of ranges we want to check for zeros and just loop
> over it?..

I wouldn't bother, but offsetofend() is a good suggestion that
can be done in a followup.

Applied both patches. Thanks

Re: [PATCH bpf-next 1/2] bpf: allow __sk_buff tstamp in BPF_PROG_TEST_RUN

[Stanislav Fomichev]

On 10/15, Alexei Starovoitov wrote:
> On Tue, Oct 15, 2019 at 4:15 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Tue, Oct 15, 2019 at 2:26 PM Stanislav Fomichev <sdf@google.com> wrote:
> > >
> > > It's useful for implementing EDT related tests (set tstamp, run the
> > > test, see how the tstamp is changed or observe some other parameter).
> > >
> > > Note that bpf_ktime_get_ns() helper is using monotonic clock, so for
> > > the BPF programs that compare tstamp against it, tstamp should be
> > > derived from clock_gettime(CLOCK_MONOTONIC, ...).
> > >
> > > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > > ---
> > >  net/bpf/test_run.c | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > >
> > > diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
> > > index 1153bbcdff72..0be4497cb832 100644
> > > --- a/net/bpf/test_run.c
> > > +++ b/net/bpf/test_run.c
> > > @@ -218,10 +218,18 @@ static int convert___skb_to_skb(struct sk_buff *skb, struct __sk_buff *__skb)
> > >
> > >         if (!range_is_zero(__skb, offsetof(struct __sk_buff, cb) +
> > >                            FIELD_SIZEOF(struct __sk_buff, cb),
> > > +                          offsetof(struct __sk_buff, tstamp)))
> > > +               return -EINVAL;
> > > +
> > > +       /* tstamp is allowed */
> > > +
> > > +       if (!range_is_zero(__skb, offsetof(struct __sk_buff, tstamp) +
> > > +                          FIELD_SIZEOF(struct __sk_buff, tstamp),
> >
> > with no context on this particular change whatsoever: isn't this the
> > same as offsetofend(struct __sk_buff, tstamp)? Same above for cb.
> >
> > Overall, this seems like the 4th similar check, would it make sense to
> > add a static array of ranges we want to check for zeros and just loop
> > over it?..
> 
> I wouldn't bother, but offsetofend() is a good suggestion that
> can be done in a followup.
> 
> Applied both patches. Thanks
Thanks. I'll follow up with offsetofend, sounds like a good suggestion
that can eliminate a bit of copy paste.