Message ID | 20200924000326.8913-1-bimmy.pujari@intel.com |
---|---|
State | Changes Requested |
Delegated to: | BPF Maintainers |
Headers | show |
Series | [bpf-next,v2,1/2] bpf: Add bpf_ktime_get_real_ns | expand |
On Wed, Sep 23, 2020 at 5:03 PM <bimmy.pujari@intel.com> wrote: > > 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 add 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 | 8 ++++++++ > kernel/bpf/core.c | 1 + > kernel/bpf/helpers.c | 13 +++++++++++++ > kernel/trace/bpf_trace.c | 2 ++ > tools/include/uapi/linux/bpf.h | 8 ++++++++ > 7 files changed, 35 insertions(+) > > diff --git a/drivers/media/rc/bpf-lirc.c b/drivers/media/rc/bpf-lirc.c > index 5bb144435c16..1cae0cfdcbaf 100644 > --- a/drivers/media/rc/bpf-lirc.c > +++ b/drivers/media/rc/bpf-lirc.c > @@ -103,6 +103,8 @@ lirc_mode2_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) > return &bpf_map_peek_elem_proto; > case BPF_FUNC_ktime_get_ns: > return &bpf_ktime_get_ns_proto; > + case BPF_FUNC_ktime_get_real_ns: > + return &bpf_ktime_get_real_ns_proto; > case BPF_FUNC_ktime_get_boot_ns: > return &bpf_ktime_get_boot_ns_proto; > case BPF_FUNC_tail_call: > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > index fc5c901c7542..3179fcc4ef53 100644 > --- a/include/linux/bpf.h > +++ b/include/linux/bpf.h > @@ -1756,6 +1756,7 @@ extern const struct bpf_func_proto bpf_get_smp_processor_id_proto; > 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_real_ns_proto; > extern const struct bpf_func_proto bpf_ktime_get_boot_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; > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > index a22812561064..198e69a6508d 100644 > --- a/include/uapi/linux/bpf.h > +++ b/include/uapi/linux/bpf.h > @@ -3586,6 +3586,13 @@ union bpf_attr { > * the data in *dst*. This is a wrapper of **copy_from_user**\ (). > * Return > * 0 on success, or a negative error in case of failure. > + * > + * u64 bpf_ktime_get_real_ns(void) > + * Description > + * Return the real time in nanoseconds. > + * See: **clock_gettime**\ (**CLOCK_REALTIME**) > + * Return > + * Current *ktime*. > */ > #define __BPF_FUNC_MAPPER(FN) \ > FN(unspec), \ > @@ -3737,6 +3744,7 @@ union bpf_attr { > FN(inode_storage_delete), \ > FN(d_path), \ > FN(copy_from_user), \ > + 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 c4811b139caa..af38af5ffc8b 100644 > --- a/kernel/bpf/core.c > +++ b/kernel/bpf/core.c > @@ -2207,6 +2207,7 @@ const struct bpf_func_proto bpf_get_prandom_u32_proto __weak; > 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_real_ns_proto __weak; > const struct bpf_func_proto bpf_ktime_get_boot_ns_proto __weak; > > const struct bpf_func_proto bpf_get_current_pid_tgid_proto __weak; > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c > index 5cc7425ee476..776ff58f969d 100644 > --- a/kernel/bpf/helpers.c > +++ b/kernel/bpf/helpers.c > @@ -155,6 +155,17 @@ const struct bpf_func_proto bpf_ktime_get_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 = true, imho should be false, this is normally accessible to userspace code via syscall, no reason why it should be gpl only for bpf > + .ret_type = RET_INTEGER, > +}; > BPF_CALL_0(bpf_ktime_get_boot_ns) > { > /* NMI safe access to clock boottime */ > @@ -655,6 +666,8 @@ bpf_base_func_proto(enum bpf_func_id func_id) > return &bpf_tail_call_proto; > case BPF_FUNC_ktime_get_ns: > return &bpf_ktime_get_ns_proto; > + case BPF_FUNC_ktime_get_real_ns: > + return &bpf_ktime_get_real_ns_proto; > case BPF_FUNC_ktime_get_boot_ns: > return &bpf_ktime_get_boot_ns_proto; > case BPF_FUNC_ringbuf_output: > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > index 36508f46a8db..18b644fff0be 100644 > --- a/kernel/trace/bpf_trace.c > +++ b/kernel/trace/bpf_trace.c > @@ -1165,6 +1165,8 @@ bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) > return &bpf_map_peek_elem_proto; > case BPF_FUNC_ktime_get_ns: > return &bpf_ktime_get_ns_proto; > + case BPF_FUNC_ktime_get_real_ns: > + return &bpf_ktime_get_real_ns_proto; > case BPF_FUNC_ktime_get_boot_ns: > return &bpf_ktime_get_boot_ns_proto; should probably be here to stay alpha sorted more or less (also applies to other places) > case BPF_FUNC_tail_call: > diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h > index a22812561064..198e69a6508d 100644 > --- a/tools/include/uapi/linux/bpf.h > +++ b/tools/include/uapi/linux/bpf.h > @@ -3586,6 +3586,13 @@ union bpf_attr { > * the data in *dst*. This is a wrapper of **copy_from_user**\ (). > * Return > * 0 on success, or a negative error in case of failure. > + * > + * u64 bpf_ktime_get_real_ns(void) > + * Description > + * Return the real time in nanoseconds. > + * See: **clock_gettime**\ (**CLOCK_REALTIME**) > + * Return > + * Current *ktime*. > */ > #define __BPF_FUNC_MAPPER(FN) \ > FN(unspec), \ > @@ -3737,6 +3744,7 @@ union bpf_attr { > FN(inode_storage_delete), \ > FN(d_path), \ > FN(copy_from_user), \ > + FN(ktime_get_real_ns), \ > /* */ > > /* integer value in 'imm' field of BPF_CALL instruction selects which helper > -- > 2.17.1 > Maciej Żenczykowski, Kernel Networking Developer @ Google
On 9/23/20 7:19 PM, Maciej Żenczykowski wrote: >> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c >> index 5cc7425ee476..776ff58f969d 100644 >> --- a/kernel/bpf/helpers.c >> +++ b/kernel/bpf/helpers.c >> @@ -155,6 +155,17 @@ const struct bpf_func_proto bpf_ktime_get_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 = true, > > imho should be false, this is normally accessible to userspace code > via syscall, no reason why it should be gpl only for bpf > agreed, no reason for the bpf hook to be gpl_only. Glad to see the v2 of this patch; I wondered what happened to this helper.
-----Original Message----- From: David Ahern <dsahern@gmail.com> Sent: Wednesday, September 23, 2020 7:19 PM To: Maciej Żenczykowski <maze@google.com>; Pujari, Bimmy <bimmy.pujari@intel.com> Cc: bpf <bpf@vger.kernel.org>; Linux NetDev <netdev@vger.kernel.org>; mchehab@kernel.org; Alexei Starovoitov <ast@kernel.org>; Daniel Borkmann <daniel@iogearbox.net>; Martin Lau <kafai@fb.com>; Nikravesh, Ashkan <ashkan.nikravesh@intel.com> Subject: Re: [PATCH bpf-next v2 1/2] bpf: Add bpf_ktime_get_real_ns On 9/23/20 7:19 PM, Maciej Żenczykowski wrote: >> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c index >> 5cc7425ee476..776ff58f969d 100644 >> --- a/kernel/bpf/helpers.c >> +++ b/kernel/bpf/helpers.c >> @@ -155,6 +155,17 @@ const struct bpf_func_proto bpf_ktime_get_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 = true, > > imho should be false, this is normally accessible to userspace code > via syscall, no reason why it should be gpl only for bpf > agreed, no reason for the bpf hook to be gpl_only. Glad to see the v2 of this patch; I wondered what happened to this helper. BP: Thanks Maciej & Ahern for taking time to look at it. I will update the changes suggested shortly.
diff --git a/drivers/media/rc/bpf-lirc.c b/drivers/media/rc/bpf-lirc.c index 5bb144435c16..1cae0cfdcbaf 100644 --- a/drivers/media/rc/bpf-lirc.c +++ b/drivers/media/rc/bpf-lirc.c @@ -103,6 +103,8 @@ lirc_mode2_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) return &bpf_map_peek_elem_proto; case BPF_FUNC_ktime_get_ns: return &bpf_ktime_get_ns_proto; + case BPF_FUNC_ktime_get_real_ns: + return &bpf_ktime_get_real_ns_proto; case BPF_FUNC_ktime_get_boot_ns: return &bpf_ktime_get_boot_ns_proto; case BPF_FUNC_tail_call: diff --git a/include/linux/bpf.h b/include/linux/bpf.h index fc5c901c7542..3179fcc4ef53 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -1756,6 +1756,7 @@ extern const struct bpf_func_proto bpf_get_smp_processor_id_proto; 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_real_ns_proto; extern const struct bpf_func_proto bpf_ktime_get_boot_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; diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index a22812561064..198e69a6508d 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -3586,6 +3586,13 @@ union bpf_attr { * the data in *dst*. This is a wrapper of **copy_from_user**\ (). * Return * 0 on success, or a negative error in case of failure. + * + * u64 bpf_ktime_get_real_ns(void) + * Description + * Return the real time in nanoseconds. + * See: **clock_gettime**\ (**CLOCK_REALTIME**) + * Return + * Current *ktime*. */ #define __BPF_FUNC_MAPPER(FN) \ FN(unspec), \ @@ -3737,6 +3744,7 @@ union bpf_attr { FN(inode_storage_delete), \ FN(d_path), \ FN(copy_from_user), \ + 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 c4811b139caa..af38af5ffc8b 100644 --- a/kernel/bpf/core.c +++ b/kernel/bpf/core.c @@ -2207,6 +2207,7 @@ const struct bpf_func_proto bpf_get_prandom_u32_proto __weak; 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_real_ns_proto __weak; const struct bpf_func_proto bpf_ktime_get_boot_ns_proto __weak; const struct bpf_func_proto bpf_get_current_pid_tgid_proto __weak; diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c index 5cc7425ee476..776ff58f969d 100644 --- a/kernel/bpf/helpers.c +++ b/kernel/bpf/helpers.c @@ -155,6 +155,17 @@ const struct bpf_func_proto bpf_ktime_get_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 = true, + .ret_type = RET_INTEGER, +}; BPF_CALL_0(bpf_ktime_get_boot_ns) { /* NMI safe access to clock boottime */ @@ -655,6 +666,8 @@ bpf_base_func_proto(enum bpf_func_id func_id) return &bpf_tail_call_proto; case BPF_FUNC_ktime_get_ns: return &bpf_ktime_get_ns_proto; + case BPF_FUNC_ktime_get_real_ns: + return &bpf_ktime_get_real_ns_proto; case BPF_FUNC_ktime_get_boot_ns: return &bpf_ktime_get_boot_ns_proto; case BPF_FUNC_ringbuf_output: diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c index 36508f46a8db..18b644fff0be 100644 --- a/kernel/trace/bpf_trace.c +++ b/kernel/trace/bpf_trace.c @@ -1165,6 +1165,8 @@ bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) return &bpf_map_peek_elem_proto; case BPF_FUNC_ktime_get_ns: return &bpf_ktime_get_ns_proto; + case BPF_FUNC_ktime_get_real_ns: + return &bpf_ktime_get_real_ns_proto; case BPF_FUNC_ktime_get_boot_ns: return &bpf_ktime_get_boot_ns_proto; case BPF_FUNC_tail_call: diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h index a22812561064..198e69a6508d 100644 --- a/tools/include/uapi/linux/bpf.h +++ b/tools/include/uapi/linux/bpf.h @@ -3586,6 +3586,13 @@ union bpf_attr { * the data in *dst*. This is a wrapper of **copy_from_user**\ (). * Return * 0 on success, or a negative error in case of failure. + * + * u64 bpf_ktime_get_real_ns(void) + * Description + * Return the real time in nanoseconds. + * See: **clock_gettime**\ (**CLOCK_REALTIME**) + * Return + * Current *ktime*. */ #define __BPF_FUNC_MAPPER(FN) \ FN(unspec), \ @@ -3737,6 +3744,7 @@ union bpf_attr { FN(inode_storage_delete), \ FN(d_path), \ FN(copy_from_user), \ + FN(ktime_get_real_ns), \ /* */ /* integer value in 'imm' field of BPF_CALL instruction selects which helper