[PATCH bpf-next v7 1/2] bpf: Add bpf_ktime_get_real_ns

[PATCH bpf-next v7 1/2] bpf: Add bpf_ktime_get_real_ns

[bimmy.pujari]

From: Bimmy Pujari <bimmy.pujari@intel.com>

The existing bpf helper functions to get timestamp return the time
elapsed since system boot. This timestamp is not particularly useful
where epoch timestamp is required or more than one server is involved
and time sync is required. Instead, you want to use CLOCK_REALTIME,
which provides epoch timestamp. Hence adding a new helper
bfp_ktime_get_real_ns() based around CLOCK_REALTIME.

Signed-off-by: Ashkan Nikravesh <ashkan.nikravesh@intel.com>
Signed-off-by: Bimmy Pujari <bimmy.pujari@intel.com>
---
 drivers/media/rc/bpf-lirc.c    |  2 ++
 include/linux/bpf.h            |  1 +
 include/uapi/linux/bpf.h       | 11 +++++++++++
 kernel/bpf/core.c              |  1 +
 kernel/bpf/helpers.c           | 14 ++++++++++++++
 kernel/trace/bpf_trace.c       |  2 ++
 tools/include/uapi/linux/bpf.h | 11 +++++++++++
 7 files changed, 42 insertions(+)

diff --git a/drivers/media/rc/bpf-lirc.c b/drivers/media/rc/bpf-lirc.c
index 3fe3edd80876..fe0fd07a473f 100644
--- a/drivers/media/rc/bpf-lirc.c
+++ b/drivers/media/rc/bpf-lirc.c
@@ -105,6 +105,8 @@ lirc_mode2_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return &bpf_ktime_get_ns_proto;
 	case BPF_FUNC_ktime_get_boot_ns:
 		return &bpf_ktime_get_boot_ns_proto;
+	case BPF_FUNC_ktime_get_real_ns:
+		return &bpf_ktime_get_real_ns_proto;
 	case BPF_FUNC_tail_call:
 		return &bpf_tail_call_proto;
 	case BPF_FUNC_get_prandom_u32:
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 50e5c4b52bd1..01866d714438 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1791,6 +1791,7 @@ extern const struct bpf_func_proto bpf_get_numa_node_id_proto;
 extern const struct bpf_func_proto bpf_tail_call_proto;
 extern const struct bpf_func_proto bpf_ktime_get_ns_proto;
 extern const struct bpf_func_proto bpf_ktime_get_boot_ns_proto;
+extern const struct bpf_func_proto bpf_ktime_get_real_ns_proto;
 extern const struct bpf_func_proto bpf_get_current_pid_tgid_proto;
 extern const struct bpf_func_proto bpf_get_current_uid_gid_proto;
 extern const struct bpf_func_proto bpf_get_current_comm_proto;
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 1f17c6752deb..f944d9a14137 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -3665,6 +3665,16 @@ union bpf_attr {
  * 	Return
  * 		The helper returns **TC_ACT_REDIRECT** on success or
  * 		**TC_ACT_SHOT** on error.
+ *
+ * u64 bpf_ktime_get_real_ns(void)
+ *	Description
+ *		Return the real time since epoch in nanoseconds.
+ *		See: **clock_gettime**\ (**CLOCK_REALTIME**)
+ *
+ *		As REALCLOCK can jump around, this helper should not be used to
+ *		measure passage of time.
+ *	Return
+ *		Current *ktime*.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -3820,6 +3830,7 @@ union bpf_attr {
 	FN(seq_printf_btf),		\
 	FN(skb_cgroup_classid),		\
 	FN(redirect_neigh),		\
+	FN(ktime_get_real_ns),		\
 	/* */
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index cda674f1392f..7fb353ccb8ae 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -2211,6 +2211,7 @@ const struct bpf_func_proto bpf_get_smp_processor_id_proto __weak;
 const struct bpf_func_proto bpf_get_numa_node_id_proto __weak;
 const struct bpf_func_proto bpf_ktime_get_ns_proto __weak;
 const struct bpf_func_proto bpf_ktime_get_boot_ns_proto __weak;
+const struct bpf_func_proto bpf_ktime_get_real_ns_proto __weak;
 
 const struct bpf_func_proto bpf_get_current_pid_tgid_proto __weak;
 const struct bpf_func_proto bpf_get_current_uid_gid_proto __weak;
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index e825441781ab..0fefcd076d7b 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -167,6 +167,18 @@ const struct bpf_func_proto bpf_ktime_get_boot_ns_proto = {
 	.ret_type	= RET_INTEGER,
 };
 
+BPF_CALL_0(bpf_ktime_get_real_ns)
+{
+	/* NMI safe access to clock realtime */
+	return ktime_get_real_fast_ns();
+}
+
+const struct bpf_func_proto bpf_ktime_get_real_ns_proto = {
+	.func		= bpf_ktime_get_real_ns,
+	.gpl_only	= false,
+	.ret_type	= RET_INTEGER,
+};
+
 BPF_CALL_0(bpf_get_current_pid_tgid)
 {
 	struct task_struct *task = current;
@@ -657,6 +669,8 @@ bpf_base_func_proto(enum bpf_func_id func_id)
 		return &bpf_ktime_get_ns_proto;
 	case BPF_FUNC_ktime_get_boot_ns:
 		return &bpf_ktime_get_boot_ns_proto;
+	case BPF_FUNC_ktime_get_real_ns:
+		return &bpf_ktime_get_real_ns_proto;
 	case BPF_FUNC_ringbuf_output:
 		return &bpf_ringbuf_output_proto;
 	case BPF_FUNC_ringbuf_reserve:
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index e118a83439c3..1ee1c29cb711 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1259,6 +1259,8 @@ bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return &bpf_ktime_get_ns_proto;
 	case BPF_FUNC_ktime_get_boot_ns:
 		return &bpf_ktime_get_boot_ns_proto;
+	case BPF_FUNC_ktime_get_real_ns:
+		return &bpf_ktime_get_real_ns_proto;
 	case BPF_FUNC_tail_call:
 		return &bpf_tail_call_proto;
 	case BPF_FUNC_get_current_pid_tgid:
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 1f17c6752deb..f944d9a14137 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -3665,6 +3665,16 @@ union bpf_attr {
  * 	Return
  * 		The helper returns **TC_ACT_REDIRECT** on success or
  * 		**TC_ACT_SHOT** on error.
+ *
+ * u64 bpf_ktime_get_real_ns(void)
+ *	Description
+ *		Return the real time since epoch in nanoseconds.
+ *		See: **clock_gettime**\ (**CLOCK_REALTIME**)
+ *
+ *		As REALCLOCK can jump around, this helper should not be used to
+ *		measure passage of time.
+ *	Return
+ *		Current *ktime*.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -3820,6 +3830,7 @@ union bpf_attr {
 	FN(seq_printf_btf),		\
 	FN(skb_cgroup_classid),		\
 	FN(redirect_neigh),		\
+	FN(ktime_get_real_ns),		\
 	/* */
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
-- 
2.17.1

[PATCH bpf-next v7 2/2] selftests/bpf: Selftest for real time helper

[bimmy.pujari]

From: Bimmy Pujari <bimmy.pujari@intel.com>

Add test validating that bpf_ktime_get_real_ns works fine.

Signed-off-by: Bimmy Pujari <bimmy.pujari@intel.com>
---
 .../selftests/bpf/prog_tests/ktime_real.c     | 42 +++++++++++++++++++
 .../bpf/progs/test_ktime_get_real_ns.c        | 36 ++++++++++++++++
 2 files changed, 78 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/ktime_real.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_ktime_get_real_ns.c

diff --git a/tools/testing/selftests/bpf/prog_tests/ktime_real.c b/tools/testing/selftests/bpf/prog_tests/ktime_real.c
new file mode 100644
index 000000000000..85235f2786b2
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/ktime_real.c
@@ -0,0 +1,42 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <test_progs.h>
+#include <network_helpers.h>
+
+static void *time_thread(void *arg)
+{
+	__u32 duration, retval;
+	int err, prog_fd = *(u32 *) arg;
+
+	err = bpf_prog_test_run(prog_fd, 10000, &pkt_v4, sizeof(pkt_v4),
+				NULL, NULL, &retval, &duration);
+	CHECK(err || retval, "",
+	      "err %d errno %d retval %d duration %d\n",
+	      err, errno, retval, duration);
+	pthread_exit(arg);
+}
+
+void test_ktime_real(void)
+{
+	const char *file = "./test_ktime_get_real_ns.o";
+	struct bpf_object *obj = NULL;
+	pthread_t thread_id;
+	int prog_fd;
+	int err = 0;
+	void *ret;
+
+	err = bpf_prog_load(file, BPF_PROG_TYPE_CGROUP_SKB, &obj, &prog_fd);
+	if (CHECK_FAIL(err)) {
+		printf("test_ktime_get_real_ns:bpf_prog_load errno %d\n", errno);
+		goto close_prog;
+	}
+
+	if (CHECK_FAIL(pthread_create(&thread_id, NULL,
+				      &time_thread, &prog_fd)))
+		goto close_prog;
+
+	if (CHECK_FAIL(pthread_join(thread_id, &ret) ||
+				    ret != (void *)&prog_fd))
+		goto close_prog;
+close_prog:
+	bpf_object__close(obj);
+}
diff --git a/tools/testing/selftests/bpf/progs/test_ktime_get_real_ns.c b/tools/testing/selftests/bpf/progs/test_ktime_get_real_ns.c
new file mode 100644
index 000000000000..37132c59de61
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_ktime_get_real_ns.c
@@ -0,0 +1,36 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/bpf.h>
+#include <linux/version.h>
+#include <bpf/bpf_helpers.h>
+
+struct {
+	__uint(type, BPF_MAP_TYPE_ARRAY);
+	__uint(max_entries, 1);
+	__type(key, int);
+	__type(value, unsigned long long);
+} time_map SEC(".maps");
+
+SEC("realtime_helper")
+int realtime_helper_test(struct __sk_buff *skb)
+{
+	unsigned long long *lasttime;
+	unsigned long long curtime;
+	int key = 0;
+	int err = 0;
+
+	lasttime = bpf_map_lookup_elem(&time_map, &key);
+	if (!lasttime)
+		goto err;
+
+	curtime = bpf_ktime_get_real_ns();
+	if (curtime <= *lasttime) {
+		err = 1;
+		goto err;
+	}
+	*lasttime = curtime;
+
+err:
+	return err;
+}
+
+char _license[] SEC("license") = "GPL";
-- 
2.17.1

Re: [PATCH bpf-next v7 2/2] selftests/bpf: Selftest for real time helper

[Alexei Starovoitov]

On Wed, Sep 30, 2020 at 07:05:04PM -0700, bimmy.pujari@intel.com wrote:
> +SEC("realtime_helper")
> +int realtime_helper_test(struct __sk_buff *skb)
> +{
> +	unsigned long long *lasttime;
> +	unsigned long long curtime;
> +	int key = 0;
> +	int err = 0;
> +
> +	lasttime = bpf_map_lookup_elem(&time_map, &key);
> +	if (!lasttime)
> +		goto err;
> +
> +	curtime = bpf_ktime_get_real_ns();
> +	if (curtime <= *lasttime) {
> +		err = 1;
> +		goto err;
> +	}
> +	*lasttime = curtime;

so the test is doing exactly what comment in patch 1 is saying not to do.
I'm sorry but Andrii's comments are correct. If the authors of the patch
cannot make it right we should not add this helper to general audience.
Just because POSIX allows it it doesn't mean that it did the good choice.

RE: [PATCH bpf-next v7 2/2] selftests/bpf: Selftest for real time helper

[Pujari, Bimmy]

Thanks everyone for putting your valuable time to review these patches. Can any one from you suggest the best way to test this function in selftest?

Regards
Bimmy

-----Original Message-----
From: Alexei Starovoitov <alexei.starovoitov@gmail.com> 
Sent: Wednesday, September 30, 2020 10:35 PM
To: Pujari, Bimmy <bimmy.pujari@intel.com>
Cc: bpf@vger.kernel.org; netdev@vger.kernel.org; mchehab@kernel.org; ast@kernel.org; daniel@iogearbox.net; kafai@fb.com; maze@google.com; Nikravesh, Ashkan <ashkan.nikravesh@intel.com>; Alvarez, Daniel A <daniel.a.alvarez@intel.com>
Subject: Re: [PATCH bpf-next v7 2/2] selftests/bpf: Selftest for real time helper

On Wed, Sep 30, 2020 at 07:05:04PM -0700, bimmy.pujari@intel.com wrote:
> +SEC("realtime_helper")
> +int realtime_helper_test(struct __sk_buff *skb) {
> +	unsigned long long *lasttime;
> +	unsigned long long curtime;
> +	int key = 0;
> +	int err = 0;
> +
> +	lasttime = bpf_map_lookup_elem(&time_map, &key);
> +	if (!lasttime)
> +		goto err;
> +
> +	curtime = bpf_ktime_get_real_ns();
> +	if (curtime <= *lasttime) {
> +		err = 1;
> +		goto err;
> +	}
> +	*lasttime = curtime;

so the test is doing exactly what comment in patch 1 is saying not to do.
I'm sorry but Andrii's comments are correct. If the authors of the patch cannot make it right we should not add this helper to general audience.
Just because POSIX allows it it doesn't mean that it did the good choice.

Re: [PATCH bpf-next v7 2/2] selftests/bpf: Selftest for real time helper

[Alexei Starovoitov]

On Thu, Oct 1, 2020 at 2:52 PM Pujari, Bimmy <bimmy.pujari@intel.com> wrote:
>
> Thanks everyone for putting your valuable time to review these patches. Can any one from you suggest the best way to test this function in selftest?

Don't bother. This helper is no go.

Please trim your replies next time and do not top post.

Re: [PATCH bpf-next v7 2/2] selftests/bpf: Selftest for real time helper

[Maciej Żenczykowski]

> Don't bother. This helper is no go.

I disagree on the 'no go' -- I do think we should have this helper.

The problem is it should only be used in some limited circumstances -
for example when processing packets coming in off the wire with real
times in them...  For example for something like measuring delay of
ntp frames.  This is of course testable but annoying (create a fake
ntp frame with gettimeofday injected timestamp in it, run it through
bpf, see what processing delay it reports).

Lets not make bpf even harder to use then it already is...

Re: [PATCH bpf-next v7 2/2] selftests/bpf: Selftest for real time helper

[David Ahern]

On 10/5/20 10:36 AM, Maciej Żenczykowski wrote:
>> Don't bother. This helper is no go.
> 
> I disagree on the 'no go' -- I do think we should have this helper.

+1

> 
> Lets not make bpf even harder to use then it already is...
> 

Logging is done using time of day; that is not a posix mistake but a
real need. Users do not file complaints based on a server's boot time or
some random monotonic time; they report a problem based on time-of-day.
Allowing bpf programs to timeofday makes it easier to troubleshoot and
correlate kernel side events to userspace logs.