bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf-next 1/2] bpf: allow __sk_buff tstamp in BPF_PROG_TEST_RUN
@ 2019-10-15 18:31 Stanislav Fomichev
  2019-10-15 18:31 ` [PATCH bpf-next 2/2] selftests: bpf: add selftest for __sk_buff tstamp Stanislav Fomichev
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Stanislav Fomichev @ 2019-10-15 18:31 UTC (permalink / raw)
  To: netdev, bpf; +Cc: davem, ast, daniel, 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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH bpf-next 2/2] selftests: bpf: add selftest for __sk_buff tstamp
  2019-10-15 18:31 [PATCH bpf-next 1/2] bpf: allow __sk_buff tstamp in BPF_PROG_TEST_RUN Stanislav Fomichev
@ 2019-10-15 18:31 ` Stanislav Fomichev
  2019-10-15 20:39   ` Martin Lau
  2019-10-15 20:34 ` [PATCH bpf-next 1/2] bpf: allow __sk_buff tstamp in BPF_PROG_TEST_RUN Martin Lau
  2019-10-15 23:14 ` Andrii Nakryiko
  2 siblings, 1 reply; 8+ messages in thread
From: Stanislav Fomichev @ 2019-10-15 18:31 UTC (permalink / raw)
  To: netdev, bpf; +Cc: davem, ast, daniel, 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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH bpf-next 1/2] bpf: allow __sk_buff tstamp in BPF_PROG_TEST_RUN
  2019-10-15 18:31 [PATCH bpf-next 1/2] bpf: allow __sk_buff tstamp in BPF_PROG_TEST_RUN Stanislav Fomichev
  2019-10-15 18:31 ` [PATCH bpf-next 2/2] selftests: bpf: add selftest for __sk_buff tstamp Stanislav Fomichev
@ 2019-10-15 20:34 ` Martin Lau
  2019-10-15 20:43   ` Stanislav Fomichev
  2019-10-15 23:14 ` Andrii Nakryiko
  2 siblings, 1 reply; 8+ messages in thread
From: Martin Lau @ 2019-10-15 20:34 UTC (permalink / raw)
  To: Stanislav Fomichev; +Cc: netdev, bpf, davem, ast, daniel

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>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH bpf-next 2/2] selftests: bpf: add selftest for __sk_buff tstamp
  2019-10-15 18:31 ` [PATCH bpf-next 2/2] selftests: bpf: add selftest for __sk_buff tstamp Stanislav Fomichev
@ 2019-10-15 20:39   ` Martin Lau
  0 siblings, 0 replies; 8+ messages in thread
From: Martin Lau @ 2019-10-15 20:39 UTC (permalink / raw)
  To: Stanislav Fomichev; +Cc: netdev, bpf, davem, ast, daniel

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>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH bpf-next 1/2] bpf: allow __sk_buff tstamp in BPF_PROG_TEST_RUN
  2019-10-15 20:34 ` [PATCH bpf-next 1/2] bpf: allow __sk_buff tstamp in BPF_PROG_TEST_RUN Martin Lau
@ 2019-10-15 20:43   ` Stanislav Fomichev
  0 siblings, 0 replies; 8+ messages in thread
From: Stanislav Fomichev @ 2019-10-15 20:43 UTC (permalink / raw)
  To: Martin Lau; +Cc: Stanislav Fomichev, netdev, bpf, davem, ast, daniel

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>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH bpf-next 1/2] bpf: allow __sk_buff tstamp in BPF_PROG_TEST_RUN
  2019-10-15 18:31 [PATCH bpf-next 1/2] bpf: allow __sk_buff tstamp in BPF_PROG_TEST_RUN Stanislav Fomichev
  2019-10-15 18:31 ` [PATCH bpf-next 2/2] selftests: bpf: add selftest for __sk_buff tstamp Stanislav Fomichev
  2019-10-15 20:34 ` [PATCH bpf-next 1/2] bpf: allow __sk_buff tstamp in BPF_PROG_TEST_RUN Martin Lau
@ 2019-10-15 23:14 ` Andrii Nakryiko
  2019-10-15 23:26   ` Alexei Starovoitov
  2 siblings, 1 reply; 8+ messages in thread
From: Andrii Nakryiko @ 2019-10-15 23:14 UTC (permalink / raw)
  To: Stanislav Fomichev
  Cc: Networking, bpf, David S. Miller, Alexei Starovoitov, Daniel Borkmann

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
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH bpf-next 1/2] bpf: allow __sk_buff tstamp in BPF_PROG_TEST_RUN
  2019-10-15 23:14 ` Andrii Nakryiko
@ 2019-10-15 23:26   ` Alexei Starovoitov
  2019-10-15 23:45     ` Stanislav Fomichev
  0 siblings, 1 reply; 8+ messages in thread
From: Alexei Starovoitov @ 2019-10-15 23:26 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Stanislav Fomichev, Networking, bpf, David S. Miller,
	Alexei Starovoitov, Daniel Borkmann

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH bpf-next 1/2] bpf: allow __sk_buff tstamp in BPF_PROG_TEST_RUN
  2019-10-15 23:26   ` Alexei Starovoitov
@ 2019-10-15 23:45     ` Stanislav Fomichev
  0 siblings, 0 replies; 8+ messages in thread
From: Stanislav Fomichev @ 2019-10-15 23:45 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Andrii Nakryiko, Stanislav Fomichev, Networking, bpf,
	David S. Miller, Alexei Starovoitov, Daniel Borkmann

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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2019-10-15 23:45 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-15 18:31 [PATCH bpf-next 1/2] bpf: allow __sk_buff tstamp in BPF_PROG_TEST_RUN Stanislav Fomichev
2019-10-15 18:31 ` [PATCH bpf-next 2/2] selftests: bpf: add selftest for __sk_buff tstamp Stanislav Fomichev
2019-10-15 20:39   ` Martin Lau
2019-10-15 20:34 ` [PATCH bpf-next 1/2] bpf: allow __sk_buff tstamp in BPF_PROG_TEST_RUN Martin Lau
2019-10-15 20:43   ` Stanislav Fomichev
2019-10-15 23:14 ` Andrii Nakryiko
2019-10-15 23:26   ` Alexei Starovoitov
2019-10-15 23:45     ` Stanislav Fomichev

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).