Re: [PATCH v5 2/3] media: rc: introduce BPF_PROG_LIRC_MODE2

[Sean Young]

On Mon, Jun 04, 2018 at 07:47:30PM +0200, Matthias Reichl wrote:
> Hi Sean,
> 
> I finally found the time to test your patch series and noticed
> 2 issues - comments are inline
> 
> On Sun, May 27, 2018 at 12:24:09PM +0100, Sean Young wrote:
> > diff --git a/drivers/media/rc/Kconfig b/drivers/media/rc/Kconfig
> > index eb2c3b6eca7f..d5b35a6ba899 100644
> > --- a/drivers/media/rc/Kconfig
> > +++ b/drivers/media/rc/Kconfig
> > @@ -25,6 +25,19 @@ config LIRC
> >passes raw IR to and from userspace, which is needed for
> >IR transmitting (aka "blasting") and for the lirc daemon.
> >  
> > +config BPF_LIRC_MODE2
> > +   bool "Support for eBPF programs attached to lirc devices"
> > +   depends on BPF_SYSCALL
> > +   depends on RC_CORE=y
> 
> Requiring rc-core to be built into the kernel could become
> problematic in the future for people using media_build.
> 
> Currently the whole media tree (including rc-core) can be built
> as modules so DVB and IR drivers can be replaced by newer versions.
> But with rc-core in the kernel things could easily break if internal
> data structures are changed.
> 
> Maybe we should add a small layer with a stable API/ABI between
> bpf-lirc and rc-core to decouple them? Or would it be possible
> to build rc-core with bpf support as a module?

Unfortunately bpf cannot be built as a module.

> > +   depends on LIRC
> > +   help
> > +  Allow attaching eBPF programs to a lirc device using the bpf(2)
> > +  syscall command BPF_PROG_ATTACH. This is supported for raw IR
> > +  receivers.
> > +
> > +  These eBPF programs can be used to decode IR into scancodes, for
> > +  IR protocols not supported by the kernel decoders.
> > +
> >  menuconfig RC_DECODERS
> > bool "Remote controller decoders"
> > depends on RC_CORE
> > [...]
> > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > index 388d4feda348..3c104113d040 100644
> > --- a/kernel/bpf/syscall.c
> > +++ b/kernel/bpf/syscall.c
> > @@ -11,6 +11,7 @@
> >   */
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -1578,6 +1579,8 @@ static int bpf_prog_attach(const union bpf_attr *attr)
> > case BPF_SK_SKB_STREAM_PARSER:
> > case BPF_SK_SKB_STREAM_VERDICT:
> > return sockmap_get_from_fd(attr, BPF_PROG_TYPE_SK_SKB, true);
> > +   case BPF_LIRC_MODE2:
> > +   return lirc_prog_attach(attr);
> > default:
> > return -EINVAL;
> > }
> > @@ -1648,6 +1651,8 @@ static int bpf_prog_detach(const union bpf_attr *attr)
> > case BPF_SK_SKB_STREAM_PARSER:
> > case BPF_SK_SKB_STREAM_VERDICT:
> > return sockmap_get_from_fd(attr, BPF_PROG_TYPE_SK_SKB, false);
> > +   case BPF_LIRC_MODE2:
> > +   return lirc_prog_detach(attr);
> > default:
> > return -EINVAL;
> > }
> > @@ -1695,6 +1700,8 @@ static int bpf_prog_query(const union bpf_attr *attr,
> > case BPF_CGROUP_SOCK_OPS:
> > case BPF_CGROUP_DEVICE:
> > break;
> > +   case BPF_LIRC_MODE2:
> > +   return lirc_prog_query(attr, uattr);
> 
> When testing this patch series I was wondering why I always got
> -EINVAL when trying to query the registered programs.
> 
> Closer inspection revealed that bpf_prog_attach/detach/query and
> calls to them in the bpf syscall are in "#ifdef CONFIG_CGROUP_BPF"
> blocks - and as I built the kernel without CONFIG_CGROUP_BPF
> BPF_PROG_ATTACH/DETACH/QUERY weren't handled in the syscall switch
> and I got -EINVAL from the bpf syscall function.
> 
> I haven't checked in detail yet, but it looks to me like
> bpf_prog_attach/detach/query could always be built (or when
> either cgroup bpf or lirc bpf are enabled) and the #ifdefs moved
> inside the switch(). So lirc bpf could be used without cgroup bpf.
> Or am I missing something?

You are right, this features depends on CONFIG_CGROUP_BPF right now. This
also affects the BPF_SK_MSG_VERDICT, BPF_SK_SKB_STREAM_VERDICT and
BPF_SK_SKB_STREAM_PARSER type bpf attachments, and as far as I know
these shouldn't depend on CONFIG_CGROUP_BPF either.


Sean

Re: [PATCH v5 2/3] media: rc: introduce BPF_PROG_LIRC_MODE2

[Sean Young]

On Mon, Jun 04, 2018 at 07:47:30PM +0200, Matthias Reichl wrote:
> Hi Sean,
> 
> I finally found the time to test your patch series and noticed
> 2 issues - comments are inline
> 
> On Sun, May 27, 2018 at 12:24:09PM +0100, Sean Young wrote:
> > diff --git a/drivers/media/rc/Kconfig b/drivers/media/rc/Kconfig
> > index eb2c3b6eca7f..d5b35a6ba899 100644
> > --- a/drivers/media/rc/Kconfig
> > +++ b/drivers/media/rc/Kconfig
> > @@ -25,6 +25,19 @@ config LIRC
> >passes raw IR to and from userspace, which is needed for
> >IR transmitting (aka "blasting") and for the lirc daemon.
> >  
> > +config BPF_LIRC_MODE2
> > +   bool "Support for eBPF programs attached to lirc devices"
> > +   depends on BPF_SYSCALL
> > +   depends on RC_CORE=y
> 
> Requiring rc-core to be built into the kernel could become
> problematic in the future for people using media_build.
> 
> Currently the whole media tree (including rc-core) can be built
> as modules so DVB and IR drivers can be replaced by newer versions.
> But with rc-core in the kernel things could easily break if internal
> data structures are changed.
> 
> Maybe we should add a small layer with a stable API/ABI between
> bpf-lirc and rc-core to decouple them? Or would it be possible
> to build rc-core with bpf support as a module?

Unfortunately bpf cannot be built as a module.

> > +   depends on LIRC
> > +   help
> > +  Allow attaching eBPF programs to a lirc device using the bpf(2)
> > +  syscall command BPF_PROG_ATTACH. This is supported for raw IR
> > +  receivers.
> > +
> > +  These eBPF programs can be used to decode IR into scancodes, for
> > +  IR protocols not supported by the kernel decoders.
> > +
> >  menuconfig RC_DECODERS
> > bool "Remote controller decoders"
> > depends on RC_CORE
> > [...]
> > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > index 388d4feda348..3c104113d040 100644
> > --- a/kernel/bpf/syscall.c
> > +++ b/kernel/bpf/syscall.c
> > @@ -11,6 +11,7 @@
> >   */
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -1578,6 +1579,8 @@ static int bpf_prog_attach(const union bpf_attr *attr)
> > case BPF_SK_SKB_STREAM_PARSER:
> > case BPF_SK_SKB_STREAM_VERDICT:
> > return sockmap_get_from_fd(attr, BPF_PROG_TYPE_SK_SKB, true);
> > +   case BPF_LIRC_MODE2:
> > +   return lirc_prog_attach(attr);
> > default:
> > return -EINVAL;
> > }
> > @@ -1648,6 +1651,8 @@ static int bpf_prog_detach(const union bpf_attr *attr)
> > case BPF_SK_SKB_STREAM_PARSER:
> > case BPF_SK_SKB_STREAM_VERDICT:
> > return sockmap_get_from_fd(attr, BPF_PROG_TYPE_SK_SKB, false);
> > +   case BPF_LIRC_MODE2:
> > +   return lirc_prog_detach(attr);
> > default:
> > return -EINVAL;
> > }
> > @@ -1695,6 +1700,8 @@ static int bpf_prog_query(const union bpf_attr *attr,
> > case BPF_CGROUP_SOCK_OPS:
> > case BPF_CGROUP_DEVICE:
> > break;
> > +   case BPF_LIRC_MODE2:
> > +   return lirc_prog_query(attr, uattr);
> 
> When testing this patch series I was wondering why I always got
> -EINVAL when trying to query the registered programs.
> 
> Closer inspection revealed that bpf_prog_attach/detach/query and
> calls to them in the bpf syscall are in "#ifdef CONFIG_CGROUP_BPF"
> blocks - and as I built the kernel without CONFIG_CGROUP_BPF
> BPF_PROG_ATTACH/DETACH/QUERY weren't handled in the syscall switch
> and I got -EINVAL from the bpf syscall function.
> 
> I haven't checked in detail yet, but it looks to me like
> bpf_prog_attach/detach/query could always be built (or when
> either cgroup bpf or lirc bpf are enabled) and the #ifdefs moved
> inside the switch(). So lirc bpf could be used without cgroup bpf.
> Or am I missing something?

You are right, this features depends on CONFIG_CGROUP_BPF right now. This
also affects the BPF_SK_MSG_VERDICT, BPF_SK_SKB_STREAM_VERDICT and
BPF_SK_SKB_STREAM_PARSER type bpf attachments, and as far as I know
these shouldn't depend on CONFIG_CGROUP_BPF either.


Sean

[PATCH v5 2/3] media: rc: introduce BPF_PROG_LIRC_MODE2

[Sean Young]

Add support for BPF_PROG_LIRC_MODE2. This type of BPF program can call
rc_keydown() to reported decoded IR scancodes, or rc_repeat() to report
that the last key should be repeated.

The bpf program can be attached to using the bpf(BPF_PROG_ATTACH) syscall;
the target_fd must be the /dev/lircN device.

Acked-by: Yonghong Song 
Signed-off-by: Sean Young 
---
 drivers/media/rc/Kconfig|  13 ++
 drivers/media/rc/Makefile   |   1 +
 drivers/media/rc/bpf-lirc.c | 313 
 drivers/media/rc/lirc_dev.c |  30 +++
 drivers/media/rc/rc-core-priv.h |  21 +++
 drivers/media/rc/rc-ir-raw.c|  12 +-
 include/linux/bpf_lirc.h|  29 +++
 include/linux/bpf_types.h   |   3 +
 include/uapi/linux/bpf.h|  53 +-
 kernel/bpf/syscall.c|   7 +
 10 files changed, 479 insertions(+), 3 deletions(-)
 create mode 100644 drivers/media/rc/bpf-lirc.c
 create mode 100644 include/linux/bpf_lirc.h

diff --git a/drivers/media/rc/Kconfig b/drivers/media/rc/Kconfig
index eb2c3b6eca7f..d5b35a6ba899 100644
--- a/drivers/media/rc/Kconfig
+++ b/drivers/media/rc/Kconfig
@@ -25,6 +25,19 @@ config LIRC
   passes raw IR to and from userspace, which is needed for
   IR transmitting (aka "blasting") and for the lirc daemon.
 
+config BPF_LIRC_MODE2
+   bool "Support for eBPF programs attached to lirc devices"
+   depends on BPF_SYSCALL
+   depends on RC_CORE=y
+   depends on LIRC
+   help
+  Allow attaching eBPF programs to a lirc device using the bpf(2)
+  syscall command BPF_PROG_ATTACH. This is supported for raw IR
+  receivers.
+
+  These eBPF programs can be used to decode IR into scancodes, for
+  IR protocols not supported by the kernel decoders.
+
 menuconfig RC_DECODERS
bool "Remote controller decoders"
depends on RC_CORE
diff --git a/drivers/media/rc/Makefile b/drivers/media/rc/Makefile
index 2e1c87066f6c..e0340d043fe8 100644
--- a/drivers/media/rc/Makefile
+++ b/drivers/media/rc/Makefile
@@ -5,6 +5,7 @@ obj-y += keymaps/
 obj-$(CONFIG_RC_CORE) += rc-core.o
 rc-core-y := rc-main.o rc-ir-raw.o
 rc-core-$(CONFIG_LIRC) += lirc_dev.o
+rc-core-$(CONFIG_BPF_LIRC_MODE2) += bpf-lirc.o
 obj-$(CONFIG_IR_NEC_DECODER) += ir-nec-decoder.o
 obj-$(CONFIG_IR_RC5_DECODER) += ir-rc5-decoder.o
 obj-$(CONFIG_IR_RC6_DECODER) += ir-rc6-decoder.o
diff --git a/drivers/media/rc/bpf-lirc.c b/drivers/media/rc/bpf-lirc.c
new file mode 100644
index ..40826bba06b6
--- /dev/null
+++ b/drivers/media/rc/bpf-lirc.c
@@ -0,0 +1,313 @@
+// SPDX-License-Identifier: GPL-2.0
+// bpf-lirc.c - handles bpf
+//
+// Copyright (C) 2018 Sean Young 
+
+#include 
+#include 
+#include 
+#include "rc-core-priv.h"
+
+/*
+ * BPF interface for raw IR
+ */
+const struct bpf_prog_ops lirc_mode2_prog_ops = {
+};
+
+BPF_CALL_1(bpf_rc_repeat, u32*, sample)
+{
+   struct ir_raw_event_ctrl *ctrl;
+
+   ctrl = container_of(sample, struct ir_raw_event_ctrl, bpf_sample);
+
+   rc_repeat(ctrl->dev);
+
+   return 0;
+}
+
+static const struct bpf_func_proto rc_repeat_proto = {
+   .func  = bpf_rc_repeat,
+   .gpl_only  = true, /* rc_repeat is EXPORT_SYMBOL_GPL */
+   .ret_type  = RET_INTEGER,
+   .arg1_type = ARG_PTR_TO_CTX,
+};
+
+/*
+ * Currently rc-core does not support 64-bit scancodes, but there are many
+ * known protocols with more than 32 bits. So, define the interface as u64
+ * as a future-proof.
+ */
+BPF_CALL_4(bpf_rc_keydown, u32*, sample, u32, protocol, u64, scancode,
+  u32, toggle)
+{
+   struct ir_raw_event_ctrl *ctrl;
+
+   ctrl = container_of(sample, struct ir_raw_event_ctrl, bpf_sample);
+
+   rc_keydown(ctrl->dev, protocol, scancode, toggle != 0);
+
+   return 0;
+}
+
+static const struct bpf_func_proto rc_keydown_proto = {
+   .func  = bpf_rc_keydown,
+   .gpl_only  = true, /* rc_keydown is EXPORT_SYMBOL_GPL */
+   .ret_type  = RET_INTEGER,
+   .arg1_type = ARG_PTR_TO_CTX,
+   .arg2_type = ARG_ANYTHING,
+   .arg3_type = ARG_ANYTHING,
+   .arg4_type = ARG_ANYTHING,
+};
+
+static const struct bpf_func_proto *
+lirc_mode2_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
+{
+   switch (func_id) {
+   case BPF_FUNC_rc_repeat:
+   return _repeat_proto;
+   case BPF_FUNC_rc_keydown:
+   return _keydown_proto;
+   case BPF_FUNC_map_lookup_elem:
+   return _map_lookup_elem_proto;
+   case BPF_FUNC_map_update_elem:
+   return _map_update_elem_proto;
+   case BPF_FUNC_map_delete_elem:
+   return _map_delete_elem_proto;
+   case BPF_FUNC_ktime_get_ns:
+   return _ktime_get_ns_proto;
+   case BPF_FUNC_tail_call:
+   return _tail_call_proto;
+   case BPF_FUNC_get_prandom_u32:
+   return _get_prandom_u32_proto;
+   case 

[PATCH v5 2/3] media: rc: introduce BPF_PROG_LIRC_MODE2

[Sean Young]

Add support for BPF_PROG_LIRC_MODE2. This type of BPF program can call
rc_keydown() to reported decoded IR scancodes, or rc_repeat() to report
that the last key should be repeated.

The bpf program can be attached to using the bpf(BPF_PROG_ATTACH) syscall;
the target_fd must be the /dev/lircN device.

Acked-by: Yonghong Song 
Signed-off-by: Sean Young 
---
 drivers/media/rc/Kconfig|  13 ++
 drivers/media/rc/Makefile   |   1 +
 drivers/media/rc/bpf-lirc.c | 313 
 drivers/media/rc/lirc_dev.c |  30 +++
 drivers/media/rc/rc-core-priv.h |  21 +++
 drivers/media/rc/rc-ir-raw.c|  12 +-
 include/linux/bpf_lirc.h|  29 +++
 include/linux/bpf_types.h   |   3 +
 include/uapi/linux/bpf.h|  53 +-
 kernel/bpf/syscall.c|   7 +
 10 files changed, 479 insertions(+), 3 deletions(-)
 create mode 100644 drivers/media/rc/bpf-lirc.c
 create mode 100644 include/linux/bpf_lirc.h

diff --git a/drivers/media/rc/Kconfig b/drivers/media/rc/Kconfig
index eb2c3b6eca7f..d5b35a6ba899 100644
--- a/drivers/media/rc/Kconfig
+++ b/drivers/media/rc/Kconfig
@@ -25,6 +25,19 @@ config LIRC
   passes raw IR to and from userspace, which is needed for
   IR transmitting (aka "blasting") and for the lirc daemon.
 
+config BPF_LIRC_MODE2
+   bool "Support for eBPF programs attached to lirc devices"
+   depends on BPF_SYSCALL
+   depends on RC_CORE=y
+   depends on LIRC
+   help
+  Allow attaching eBPF programs to a lirc device using the bpf(2)
+  syscall command BPF_PROG_ATTACH. This is supported for raw IR
+  receivers.
+
+  These eBPF programs can be used to decode IR into scancodes, for
+  IR protocols not supported by the kernel decoders.
+
 menuconfig RC_DECODERS
bool "Remote controller decoders"
depends on RC_CORE
diff --git a/drivers/media/rc/Makefile b/drivers/media/rc/Makefile
index 2e1c87066f6c..e0340d043fe8 100644
--- a/drivers/media/rc/Makefile
+++ b/drivers/media/rc/Makefile
@@ -5,6 +5,7 @@ obj-y += keymaps/
 obj-$(CONFIG_RC_CORE) += rc-core.o
 rc-core-y := rc-main.o rc-ir-raw.o
 rc-core-$(CONFIG_LIRC) += lirc_dev.o
+rc-core-$(CONFIG_BPF_LIRC_MODE2) += bpf-lirc.o
 obj-$(CONFIG_IR_NEC_DECODER) += ir-nec-decoder.o
 obj-$(CONFIG_IR_RC5_DECODER) += ir-rc5-decoder.o
 obj-$(CONFIG_IR_RC6_DECODER) += ir-rc6-decoder.o
diff --git a/drivers/media/rc/bpf-lirc.c b/drivers/media/rc/bpf-lirc.c
new file mode 100644
index ..40826bba06b6
--- /dev/null
+++ b/drivers/media/rc/bpf-lirc.c
@@ -0,0 +1,313 @@
+// SPDX-License-Identifier: GPL-2.0
+// bpf-lirc.c - handles bpf
+//
+// Copyright (C) 2018 Sean Young 
+
+#include 
+#include 
+#include 
+#include "rc-core-priv.h"
+
+/*
+ * BPF interface for raw IR
+ */
+const struct bpf_prog_ops lirc_mode2_prog_ops = {
+};
+
+BPF_CALL_1(bpf_rc_repeat, u32*, sample)
+{
+   struct ir_raw_event_ctrl *ctrl;
+
+   ctrl = container_of(sample, struct ir_raw_event_ctrl, bpf_sample);
+
+   rc_repeat(ctrl->dev);
+
+   return 0;
+}
+
+static const struct bpf_func_proto rc_repeat_proto = {
+   .func  = bpf_rc_repeat,
+   .gpl_only  = true, /* rc_repeat is EXPORT_SYMBOL_GPL */
+   .ret_type  = RET_INTEGER,
+   .arg1_type = ARG_PTR_TO_CTX,
+};
+
+/*
+ * Currently rc-core does not support 64-bit scancodes, but there are many
+ * known protocols with more than 32 bits. So, define the interface as u64
+ * as a future-proof.
+ */
+BPF_CALL_4(bpf_rc_keydown, u32*, sample, u32, protocol, u64, scancode,
+  u32, toggle)
+{
+   struct ir_raw_event_ctrl *ctrl;
+
+   ctrl = container_of(sample, struct ir_raw_event_ctrl, bpf_sample);
+
+   rc_keydown(ctrl->dev, protocol, scancode, toggle != 0);
+
+   return 0;
+}
+
+static const struct bpf_func_proto rc_keydown_proto = {
+   .func  = bpf_rc_keydown,
+   .gpl_only  = true, /* rc_keydown is EXPORT_SYMBOL_GPL */
+   .ret_type  = RET_INTEGER,
+   .arg1_type = ARG_PTR_TO_CTX,
+   .arg2_type = ARG_ANYTHING,
+   .arg3_type = ARG_ANYTHING,
+   .arg4_type = ARG_ANYTHING,
+};
+
+static const struct bpf_func_proto *
+lirc_mode2_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
+{
+   switch (func_id) {
+   case BPF_FUNC_rc_repeat:
+   return _repeat_proto;
+   case BPF_FUNC_rc_keydown:
+   return _keydown_proto;
+   case BPF_FUNC_map_lookup_elem:
+   return _map_lookup_elem_proto;
+   case BPF_FUNC_map_update_elem:
+   return _map_update_elem_proto;
+   case BPF_FUNC_map_delete_elem:
+   return _map_delete_elem_proto;
+   case BPF_FUNC_ktime_get_ns:
+   return _ktime_get_ns_proto;
+   case BPF_FUNC_tail_call:
+   return _tail_call_proto;
+   case BPF_FUNC_get_prandom_u32:
+   return _get_prandom_u32_proto;
+   case BPF_FUNC_trace_printk:
+   if