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

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 <redacted>
---
 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

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

Signed-off-by: Stanislav Fomichev <redacted>
---
 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

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 <redacted>

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 <redacted>

On 10/15, Martin Lau wrote:

On Tue, Oct 15, 2019 at 11:31:24AM -0700, Stanislav Fomichev wrote:

quoted

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 <redacted>

On Tue, Oct 15, 2019 at 2:26 PM Stanislav Fomichev [off-list ref] wrote:

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?..

On Tue, Oct 15, 2019 at 4:15 PM Andrii Nakryiko
[off-list ref] wrote:

On Tue, Oct 15, 2019 at 2:26 PM Stanislav Fomichev [off-list ref] wrote:

quoted

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 <redacted>
---
 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

On 10/15, Alexei Starovoitov wrote:

On Tue, Oct 15, 2019 at 4:15 PM Andrii Nakryiko
[off-list ref] wrote:

quoted

On Tue, Oct 15, 2019 at 2:26 PM Stanislav Fomichev [off-list ref] wrote:

quoted

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 <redacted>
---
 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.