Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1248289 > unrolled thread

[PATCH net-next 3/4] bpf: add support for persistent maps/progs

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH net-next 3/4] bpf: add support for persistent maps/progs Daniel Borkmann <daniel@iogearbox.net> - 2015-10-16 03:20 +0200
    Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs Hannes Frederic Sowa <hannes@stressinduktion.org> - 2015-10-16 12:30 +0200
      Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs Daniel Borkmann <daniel@iogearbox.net> - 2015-10-16 15:40 +0200
        Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs Hannes Frederic Sowa <hannes@stressinduktion.org> - 2015-10-16 18:40 +0200
          Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs Daniel Borkmann <daniel@iogearbox.net> - 2015-10-16 19:30 +0200
            Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs Alexei Starovoitov <ast@plumgrid.com> - 2015-10-16 19:40 +0200
      Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs Alexei Starovoitov <ast@plumgrid.com> - 2015-10-16 18:20 +0200
        Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs Hannes Frederic Sowa <hannes@stressinduktion.org> - 2015-10-16 18:50 +0200
          Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs Thomas Graf <tgraf@suug.ch> - 2015-10-16 19:40 +0200
          Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs Alexei Starovoitov <ast@plumgrid.com> - 2015-10-16 19:40 +0200
    Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs Hannes Frederic Sowa <hannes@stressinduktion.org> - 2015-10-16 19:30 +0200
      Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs Alexei Starovoitov <ast@plumgrid.com> - 2015-10-16 19:50 +0200
        Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs Daniel Borkmann <daniel@iogearbox.net> - 2015-10-16 20:00 +0200
          Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs ebiederm@xmission.com (Eric W. Biederman) - 2015-10-16 21:00 +0200
            Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs Alexei Starovoitov <ast@plumgrid.com> - 2015-10-16 21:30 +0200
              Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs Daniel Borkmann <daniel@iogearbox.net> - 2015-10-16 22:00 +0200
              Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs ebiederm@xmission.com (Eric W. Biederman) - 2015-10-16 22:10 +0200
                Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs Alexei Starovoitov <ast@plumgrid.com> - 2015-10-16 23:00 +0200
                  Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs ebiederm@xmission.com (Eric W. Biederman) - 2015-10-17 02:00 +0200
                    Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs Alexei Starovoitov <ast@plumgrid.com> - 2015-10-17 04:50 +0200
                      Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs Daniel Borkmann <daniel@iogearbox.net> - 2015-10-17 14:30 +0200
                        Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs Alexei Starovoitov <ast@plumgrid.com> - 2015-10-18 06:40 +0200
                          Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs Daniel Borkmann <daniel@iogearbox.net> - 2015-10-18 17:10 +0200
                            Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs Daniel Borkmann <daniel@iogearbox.net> - 2015-10-18 18:50 +0200
                              Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs Alexei Starovoitov <ast@plumgrid.com> - 2015-10-18 23:00 +0200
                                Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs Hannes Frederic Sowa <hannes@stressinduktion.org> - 2015-10-19 09:40 +0200
                                  Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs Daniel Borkmann <daniel@iogearbox.net> - 2015-10-19 12:00 +0200
                                    Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs Daniel Borkmann <daniel@iogearbox.net> - 2015-10-19 16:30 +0200
                                      Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs Alexei Starovoitov <ast@plumgrid.com> - 2015-10-19 18:30 +0200
                                        Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs Daniel Borkmann <daniel@iogearbox.net> - 2015-10-19 19:40 +0200
                                          Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs Alexei Starovoitov <ast@plumgrid.com> - 2015-10-19 20:20 +0200
                                            Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs Hannes Frederic Sowa <hannes@stressinduktion.org> - 2015-10-19 20:50 +0200
                                              Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs Alexei Starovoitov <ast@plumgrid.com> - 2015-10-19 21:40 +0200
                                                Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs Hannes Frederic Sowa <hannes@stressinduktion.org> - 2015-10-19 22:10 +0200
                                                  Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs Alexei Starovoitov <ast@plumgrid.com> - 2015-10-19 22:50 +0200
                                                    Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs Daniel Borkmann <daniel@iogearbox.net> - 2015-10-20 00:20 +0200
                                                      Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs Alexei Starovoitov <ast@plumgrid.com> - 2015-10-20 02:40 +0200
                                                        Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs Daniel Borkmann <daniel@iogearbox.net> - 2015-10-20 10:50 +0200
                                                          Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs Alexei Starovoitov <ast@plumgrid.com> - 2015-10-20 20:00 +0200
                                                            Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs ebiederm@xmission.com (Eric W. Biederman) - 2015-10-20 21:10 +0200
                                                              Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs Daniel Borkmann <daniel@iogearbox.net> - 2015-10-21 17:20 +0200
                                                                Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs Thomas Graf <tgraf@suug.ch> - 2015-10-21 20:40 +0200
                                                                  Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs Alexei Starovoitov <ast@plumgrid.com> - 2015-10-22 00:50 +0200
                                                                    Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs Daniel Borkmann <daniel@iogearbox.net> - 2015-10-22 15:30 +0200
                                                                Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs ebiederm@xmission.com (Eric W. Biederman) - 2015-10-22 21:50 +0200
                                                                  Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs Daniel Borkmann <daniel@iogearbox.net> - 2015-10-23 15:50 +0200
                                                        Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs Hannes Frederic Sowa <hannes@stressinduktion.org> - 2015-10-20 11:50 +0200
                                                    Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs Hannes Frederic Sowa <hannes@stressinduktion.org> - 2015-10-20 01:10 +0200
                                                      Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs Alexei Starovoitov <ast@plumgrid.com> - 2015-10-20 03:10 +0200
                                                        Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs Hannes Frederic Sowa <hannes@stressinduktion.org> - 2015-10-20 12:10 +0200
                                                          Re: [PATCH net-next 3/4] bpf: add support for persistent maps/progs Alexei Starovoitov <ast@plumgrid.com> - 2015-10-20 20:50 +0200

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

#1249324

On 10/17/2015 04:43 AM, Alexei Starovoitov wrote:
> On 10/16/15 4:44 PM, Eric W. Biederman wrote:
>> Alexei Starovoitov <ast@plumgrid.com> writes:
>>
>>> We can argue about api for 2nd, whether it's mount with fd=1234 string
>>> or else, but for the first mount style doesn't make sense.
>>
>> Why does mount not make sense?  It is exactly what you are looking for
>> so why does it not make sense?
>
> hmm, how do you get a new fd back after mounting it?
> Note, open cannot be overloaded, so we end up with BPF_NEW_FD anyway,
> but now it's more convoluted and empty mounts are everywhere.

That would be my understanding as well, but as Alexei already said,
these are two different issues, it would be step 2 (let me get back
to that further below). But in any case, I don't really like dumping
key/value somewhere as files. You have binary blobs as both, and
lets say your application has a lookup-key (for whatever reason) of
several cachelines it all ends up getting rather messy than making
it really useful for non-bpf(2) aware cmdline tools to deal with.

Anyway, another idea I've been brainstorming with Hannes today a
bit is about the following:

We register two major numbers, one for eBPF maps (X), one for eBPF
progs (Y). A user can either via cmdline call something like ...
mknod /dev/bpf/maps/map_pkts c X Z to create a special character
device, or alternatively out of an application through mknod(2)
syscall (f.e. tc when setting up maps/progs internally from the obj
file for a classifer).

Then, we still have 2 eBPF commands for bpf(2) syscall to add, say
(for example) BPF_BIND_DEV and BPF_FETCH_DEV. The application that
created a map (or prog) already has the map fd and after mknod(2) it
can open(2) the special file to get the special file fd. Then it can
call something like bpf(BPF_BIND_DEV, &attr, sizeof(attr))) where
attr looks like:

   union bpf_attr attr = {
     .bpf_fd    = bpf_fd,
     .dev_fd    = dev_fd,
   };

The bpf(2) syscall can check whether dev_fd belongs to an eBPF special
file and it can then copy over file->private_data from the bpf_fd
to the dev_fd's underlying file, where the private_data, as we know,
from the bpf_fd already points to a proper bpf_map/bpf_prog structure.
The map/prog would then get ref'ed and lives onwards in the char device's
lifetime. No special hashtable, gc, etc needed. The char device has fops
that we can define by ourself, and unlinking would drop the ref from
its private_data.

Now to the other part: BPF_FETCH_DEV would work similar. The application
opens the device, and fills bpf_attr as follows again:

   union bpf_attr attr = {
     .bpf_fd    = 0,
     .dev_fd    = dev_fd,
   };

This would allow us to look up the map/prog from the dev_fd's file->
private_data, and installs a new fd via bpf_{map,prog}_new_fd() that
is returned from bpf(2) for bpf-related access. The remaining fops
from the char device could still be reserved for possibilities like
debugging in future.

Now in future (2nd step), could either be to use Eric's idea and then do
something like mount -t bpffs ... -o /dev/bpf/maps/map_pkts to dump
attributes or other properties to some location for inspection from such
a special file, or we could use kobjects for that attached to the device
if the fops from the cdev should not be sufficient.

So closing the loop to the special files where there were concerns:

This won't forbid to have a future shell-style access possibility, and
it would also not end up as a nightmare on what you mentioned with the
S_ISSOCK-like bit in the other email.

The pinning mechanism would not require an extra file system to be mounted
somewhere, and yet the user can define himself an arbitrary hierarchy
where he puts the special files as this facility already exists. An
approach like this looks overall cleaner to me, and most likely be
realizable in fewer lines of code as well.

Thoughts?

Cheers,
Daniel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1250013

On 10/17/15 5:28 AM, Daniel Borkmann wrote:
>
> Anyway, another idea I've been brainstorming with Hannes today a
> bit is about the following:
>
> We register two major numbers, one for eBPF maps (X), one for eBPF
> progs (Y). A user can either via cmdline call something like ...
> mknod /dev/bpf/maps/map_pkts c X Z to create a special character
> device, or alternatively out of an application through mknod(2)
> syscall (f.e. tc when setting up maps/progs internally from the obj
> file for a classifer).
>
> Then, we still have 2 eBPF commands for bpf(2) syscall to add, say
> (for example) BPF_BIND_DEV and BPF_FETCH_DEV. The application that
> created a map (or prog) already has the map fd and after mknod(2) it
> can open(2) the special file to get the special file fd. Then it can
> call something like bpf(BPF_BIND_DEV, &attr, sizeof(attr))) where
> attr looks like:
>
>    union bpf_attr attr = {
>      .bpf_fd    = bpf_fd,
>      .dev_fd    = dev_fd,
>    };
>
> The bpf(2) syscall can check whether dev_fd belongs to an eBPF special
> file and it can then copy over file->private_data from the bpf_fd
> to the dev_fd's underlying file, where the private_data, as we know,
> from the bpf_fd already points to a proper bpf_map/bpf_prog structure.
> The map/prog would then get ref'ed and lives onwards in the char device's
> lifetime. No special hashtable, gc, etc needed. The char device has fops
> that we can define by ourself, and unlinking would drop the ref from
> its private_data.
>
> Now to the other part: BPF_FETCH_DEV would work similar. The application
> opens the device, and fills bpf_attr as follows again:
>
>    union bpf_attr attr = {
>      .bpf_fd    = 0,
>      .dev_fd    = dev_fd,
>    };
>
> This would allow us to look up the map/prog from the dev_fd's file->
> private_data, and installs a new fd via bpf_{map,prog}_new_fd() that
> is returned from bpf(2) for bpf-related access. The remaining fops
> from the char device could still be reserved for possibilities like
> debugging in future.
>
> Now in future (2nd step), could either be to use Eric's idea and then do
> something like mount -t bpffs ... -o /dev/bpf/maps/map_pkts to dump
> attributes or other properties to some location for inspection from such
> a special file, or we could use kobjects for that attached to the device
> if the fops from the cdev should not be sufficient.
>
> So closing the loop to the special files where there were concerns:
>
> This won't forbid to have a future shell-style access possibility, and
> it would also not end up as a nightmare on what you mentioned with the
> S_ISSOCK-like bit in the other email.
>
> The pinning mechanism would not require an extra file system to be mounted
> somewhere, and yet the user can define himself an arbitrary hierarchy
> where he puts the special files as this facility already exists. An
> approach like this looks overall cleaner to me, and most likely be
> realizable in fewer lines of code as well.
>
> Thoughts?

that indeed sounds cleaner, less lines of code, no fs, etc, but
I don't see how it will work yet.
For chardev with our own ops we can be triggered on open and close
of that chardev, so replacing private_data will be cleared when
user process does close(dev_fd) ? There is no fops for unlink either,
it's fs only property ?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1250143

On 10/18/2015 04:20 AM, Alexei Starovoitov wrote:
...
> that indeed sounds cleaner, less lines of code, no fs, etc, but
> I don't see how it will work yet.

I'll have some code ready very soon to show the concept. Will post it here
tonight, stay tuned. ;)

Cheers,
Daniel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1250170

On 10/18/2015 05:03 PM, Daniel Borkmann wrote:
> On 10/18/2015 04:20 AM, Alexei Starovoitov wrote:
> ...
>> that indeed sounds cleaner, less lines of code, no fs, etc, but
>> I don't see how it will work yet.
>
> I'll have some code ready very soon to show the concept. Will post it here
> tonight, stay tuned. ;)

Okay, I have pushed some rough working proof of concept here:

   https://git.breakpoint.cc/cgit/dborkman/net-next.git/log/?h=ebpf-fds-final5

So the idea eventually had to be slightly modified after giving this further
thoughts and is the following:

We have 3 commands (BPF_DEV_CREATE, BPF_DEV_DESTROY, BPF_DEV_CONNECT), and
related to that a bpf_attr extension with only a single __u32 fd member in it.

Now, when we have an existing map/prog fd, we can do bpf_dev_create(fd) from
the application, and the kernel will create automatically a device, assigning
major/minor, etc.

You'll automatically have a sysfs entry under a new "bpf" class, for example:

   # ls -la /sys/class/bpf/
   lrwxrwxrwx.  1 root root 0 Oct 18 18:24 bpf_map0 -> ../../devices/virtual/bpf/bpf_map0
   lrwxrwxrwx.  1 root root 0 Oct 18 18:24 bpf_prog0 -> ../../devices/virtual/bpf/bpf_prog0

   # cat /sys/class/bpf/bpf_map0/dev
   249:0
   # cat /sys/class/bpf/bpf_prog0/dev
   248:0

And they also appear automatically under:

   # ls -la /dev/bpf/
   crw-------.  1 root root 249, 0 Oct 18 17:38 bpf_map0
   crw-------.  1 root root 248, 0 Oct 18 18:23 bpf_prog0

This means, you can create your own hierarchy somewhere and then symlink to it,
or add further mknod's, f.e.:

   # mknod ./foomap c 249 0
   # ./samples/bpf/devicex map-connect ./foomap
   dev, fd:3 (Success)
   map, fd:4 (Success)
   map, fd:4 read pair:(123,0) (Success)

The nice thing about it is that you can create/unlink as many as you want, but
when you remove the real device from an application via bpf_dev_destroy(fd),
then all links disappear with it. Just like in the case of a normal device driver.

On device creation, the kernel will return the minor number via bpf(2), so you
can access the file easily, f.e. /dev/bpf/bpf_map<minor> resp. /dev/bpf/bpf_prog<minor>,
and then move on with mknod(2) or symlink(2) from there if wished.

Last but not least, we can open the device driver, and then bpf_dev_connect(fd)
will return a new fd you can operate with the bpf(2) syscall to access maps and
stuff. Same here, the remaining device driver ops, we can then still use in future
for something useful.

The example code (top commit) does, to show the concept:

** Map:

  * Create map and place map into special device:
   # ./samples/bpf/devicex map-create /tmp/map-test
   map, fd:3 (Success)
   map, dev minor:2 (Success)
   map, /dev/bpf/bpf_map2 linked to /tmp/map-test (Success)
   map, fd:3 wrote pair:(123,456) (Success)
   map, fd:3 read pair:(123,456) (Success)

  * Retrieve map from special device:
   # ./samples/bpf/devicex map-connect /tmp/map-test
   dev, fd:3 (Success)
   map, fd:4 (Success)
   map, fd:4 read pair:(123,456) (Success)

  * Destroy special device (map is still locally available):
   # ./samples/bpf/devicex map-destroy /tmp/map-test2
   dev, fd:3 (Success)
   map, fd:4 (Success)
   map, dev destroyed:2 (Success)
   map, fd:4 read pair:(123,456) (Success)

** Prog:

  * Create prog and place prog into special device:
   # ./samples/bpf/devicex prog-create /tmp/prog-test
   prog, fd:3 (Success)
   prog, dev minor:0 (Success)
   prog, /dev/bpf/bpf_prog0 linked to /tmp/prog-test (Success)
   sock, fd:4 (Success)
   sock, prog attached:0 (Success)

  * Retrieve prog from special device, attach to sock:
   # ./samples/bpf/devicex prog-connect /tmp/prog-test
   dev, fd:3 (Success)
   prog, fd:4 (Success)
   sock, fd:3 (Success)
   sock, prog attached:0 (Success)

  * Destroy special device (prog is still locally available):
   # ./samples/bpf/devicex prog-destroy /tmp/prog-test
   dev, fd:3 (Success)
   prog, fd:4 (Success)
   prog, dev destroyed:0 (Success)

The actual code needed (2nd commit from above link), would be roughly along the
lines of what is shown below ... the code is overall a bit smaller than the fs.

This model seems much cleaner and more flexible to me than the file system. So,
I could polish this stuff further up a bit and do further tests/reviews on Monday
for a real submission. Does that sound like a plan?

Thanks,
Daniel

Code:

  include/linux/bpf.h      |  20 +++
  include/uapi/linux/bpf.h |  45 +-----
  kernel/bpf/Makefile      |   4 +-
  kernel/bpf/core.c        |   2 +-
  kernel/bpf/device.c      | 407 +++++++++++++++++++++++++++++++++++++++++++++++
  kernel/bpf/syscall.c     |  52 ++++--
  6 files changed, 482 insertions(+), 48 deletions(-)
  create mode 100644 kernel/bpf/device.c

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 0ae6f77..52d57ed 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -8,8 +8,12 @@
  #define _LINUX_BPF_H 1

  #include <uapi/linux/bpf.h>
+
  #include <linux/workqueue.h>
  #include <linux/file.h>
+#include <linux/cdev.h>
+
+#define BPF_F_HAS_DEV	(1 << 0)

  struct bpf_map;

@@ -37,7 +41,11 @@ struct bpf_map {
  	u32 value_size;
  	u32 max_entries;
  	u32 pages;
+	int minor;
+	unsigned long flags;
+	struct mutex m_lock;
  	struct user_struct *user;
+	struct cdev cdev;
  	const struct bpf_map_ops *ops;
  	struct work_struct work;
  };
@@ -127,10 +135,14 @@ struct bpf_prog_type_list {
  struct bpf_prog_aux {
  	atomic_t refcnt;
  	u32 used_map_cnt;
+	int minor;
+	unsigned long flags;
+	struct mutex p_lock;
  	const struct bpf_verifier_ops *ops;
  	struct bpf_map **used_maps;
  	struct bpf_prog *prog;
  	struct user_struct *user;
+	struct cdev cdev;
  	union {
  		struct work_struct work;
  		struct rcu_head	rcu;
@@ -167,11 +179,19 @@ struct bpf_prog *bpf_prog_get(u32 ufd);
  void bpf_prog_put(struct bpf_prog *prog);
  void bpf_prog_put_rcu(struct bpf_prog *prog);

+struct bpf_map *bpf_map_get(u32 ufd);
  struct bpf_map *__bpf_map_get(struct fd f);
  void bpf_map_put(struct bpf_map *map);

  extern int sysctl_unprivileged_bpf_disabled;

+int __bpf_dev_create(__u32 ufd);
+int __bpf_dev_destroy(__u32 ufd);
+int __bpf_dev_connect(__u32 ufd);
+
+int bpf_map_new_fd(struct bpf_map *map);
+int bpf_prog_new_fd(struct bpf_prog *prog);
+
  /* verify correctness of eBPF program */
  int bpf_check(struct bpf_prog **fp, union bpf_attr *attr);
  #else
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 564f1f0..55e5aad 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -63,50 +63,17 @@ struct bpf_insn {
  	__s32	imm;		/* signed immediate constant */
  };

-/* BPF syscall commands */
+/* BPF syscall commands, see bpf(2) man-page for details. */
  enum bpf_cmd {
-	/* create a map with given type and attributes
-	 * fd = bpf(BPF_MAP_CREATE, union bpf_attr *, u32 size)
-	 * returns fd or negative error
-	 * map is deleted when fd is closed
-	 */
  	BPF_MAP_CREATE,
-
-	/* lookup key in a given map
-	 * err = bpf(BPF_MAP_LOOKUP_ELEM, union bpf_attr *attr, u32 size)
-	 * Using attr->map_fd, attr->key, attr->value
-	 * returns zero and stores found elem into value
-	 * or negative error
-	 */
  	BPF_MAP_LOOKUP_ELEM,
-
-	/* create or update key/value pair in a given map
-	 * err = bpf(BPF_MAP_UPDATE_ELEM, union bpf_attr *attr, u32 size)
-	 * Using attr->map_fd, attr->key, attr->value, attr->flags
-	 * returns zero or negative error
-	 */
  	BPF_MAP_UPDATE_ELEM,
-
-	/* find and delete elem by key in a given map
-	 * err = bpf(BPF_MAP_DELETE_ELEM, union bpf_attr *attr, u32 size)
-	 * Using attr->map_fd, attr->key
-	 * returns zero or negative error
-	 */
  	BPF_MAP_DELETE_ELEM,
-
-	/* lookup key in a given map and return next key
-	 * err = bpf(BPF_MAP_GET_NEXT_KEY, union bpf_attr *attr, u32 size)
-	 * Using attr->map_fd, attr->key, attr->next_key
-	 * returns zero and stores next key or negative error
-	 */
  	BPF_MAP_GET_NEXT_KEY,
-
-	/* verify and load eBPF program
-	 * prog_fd = bpf(BPF_PROG_LOAD, union bpf_attr *attr, u32 size)
-	 * Using attr->prog_type, attr->insns, attr->license
-	 * returns fd or negative error
-	 */
  	BPF_PROG_LOAD,
+	BPF_DEV_CREATE,
+	BPF_DEV_DESTROY,
+	BPF_DEV_CONNECT,
  };

  enum bpf_map_type {
@@ -160,6 +127,10 @@ union bpf_attr {
  		__aligned_u64	log_buf;	/* user supplied buffer */
  		__u32		kern_version;	/* checked when prog_type=kprobe */
  	};
+
+	struct { /* anonymous struct used by BPF_DEV_* commands */
+		__u32		fd;
+	};
  } __attribute__((aligned(8)));

  /* integer value in 'imm' field of BPF_CALL instruction selects which helper
diff --git a/kernel/bpf/Makefile b/kernel/bpf/Makefile
index e6983be..f871ca6 100644
--- a/kernel/bpf/Makefile
+++ b/kernel/bpf/Makefile
@@ -1,2 +1,4 @@
  obj-y := core.o
-obj-$(CONFIG_BPF_SYSCALL) += syscall.o verifier.o hashtab.o arraymap.o helpers.o
+
+obj-$(CONFIG_BPF_SYSCALL) += syscall.o verifier.o device.o helpers.o
+obj-$(CONFIG_BPF_SYSCALL) += hashtab.o arraymap.o
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 8086471..260058d 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -92,6 +92,7 @@ struct bpf_prog *bpf_prog_alloc(unsigned int size, gfp_t gfp_extra_flags)

  	fp->pages = size / PAGE_SIZE;
  	fp->aux = aux;
+	aux->prog = fp;

  	return fp;
  }
@@ -726,7 +727,6 @@ void bpf_prog_free(struct bpf_prog *fp)
  	struct bpf_prog_aux *aux = fp->aux;

  	INIT_WORK(&aux->work, bpf_prog_free_deferred);
-	aux->prog = fp;
  	schedule_work(&aux->work);
  }
  EXPORT_SYMBOL_GPL(bpf_prog_free);
diff --git a/kernel/bpf/device.c b/kernel/bpf/device.c
new file mode 100644
index 0000000..e99fc82
--- /dev/null
+++ b/kernel/bpf/device.c
@@ -0,0 +1,407 @@
+/*
+ * Special file backend for persistent eBPF maps and programs, used by
+ * bpf() system call.
+ *
+ * (C) 2015 Daniel Borkmann <daniel@iogearbox.net>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/fs.h>
+#include <linux/filter.h>
+#include <linux/bpf.h>
+#include <linux/idr.h>
+#include <linux/mutex.h>
+#include <linux/cdev.h>
+
+#define BPF_MAX_DEVS	(1UL << MINORBITS)
+
+enum bpf_type {
+	BPF_TYPE_PROG,
+	BPF_TYPE_MAP,
+};
+
+static struct class *bpf_class;
+
+static dev_t bpf_map_devt;
+static DEFINE_IDR(bpf_map_idr);
+static DEFINE_MUTEX(bpf_map_idr_lock);
+
+static dev_t bpf_prog_devt;
+static DEFINE_IDR(bpf_prog_idr);
+static DEFINE_MUTEX(bpf_prog_idr_lock);
+
+static int bpf_map_get_minor(struct bpf_map *map)
+{
+	int minor;
+
+	mutex_lock(&bpf_map_idr_lock);
+	minor = idr_alloc(&bpf_map_idr, map, 0, BPF_MAX_DEVS, GFP_KERNEL);
+	mutex_unlock(&bpf_map_idr_lock);
+
+	return minor;
+}
+
+static void bpf_map_put_minor(const struct bpf_map *map)
+{
+	mutex_lock(&bpf_map_idr_lock);
+	idr_remove(&bpf_map_idr, map->minor);
+	mutex_unlock(&bpf_map_idr_lock);
+}
+
+static int bpf_prog_get_minor(struct bpf_prog *prog)
+{
+	int minor;
+
+	mutex_lock(&bpf_prog_idr_lock);
+	minor = idr_alloc(&bpf_prog_idr, prog, 0, BPF_MAX_DEVS, GFP_KERNEL);
+	mutex_unlock(&bpf_prog_idr_lock);
+
+	return minor;
+}
+
+static void bpf_prog_put_minor(const struct bpf_prog *prog)
+{
+	mutex_lock(&bpf_prog_idr_lock);
+	idr_remove(&bpf_prog_idr, prog->aux->minor);
+	mutex_unlock(&bpf_prog_idr_lock);
+}
+
+static int bpf_map_open(struct inode *inode, struct file *filep)
+{
+	filep->private_data = container_of(inode->i_cdev,
+					   struct bpf_map, cdev);
+	return 0;
+}
+
+static const struct file_operations bpf_dev_map_fops = {
+	.owner		= THIS_MODULE,
+	.open		= bpf_map_open,
+	.llseek		= noop_llseek,
+};
+
+static int bpf_prog_open(struct inode *inode, struct file *filep)
+{
+	filep->private_data = container_of(inode->i_cdev,
+					   struct bpf_prog_aux, cdev)->prog;
+	return 0;
+}
+
+static const struct file_operations bpf_dev_prog_fops = {
+	.owner		= THIS_MODULE,
+	.open		= bpf_prog_open,
+	.llseek		= noop_llseek,
+};
+
+static char *bpf_devnode(struct device *dev, umode_t *mode)
+{
+	return kasprintf(GFP_KERNEL, "bpf/%s", dev_name(dev));
+}
+
+static int bpf_map_make_dev(struct bpf_map *map)
+{
+	struct device *dev;
+	dev_t devt;
+	int ret;
+
+	mutex_lock(&map->m_lock);
+	if (map->flags & BPF_F_HAS_DEV) {
+		ret = map->minor;
+		goto out;
+	}
+
+	cdev_init(&map->cdev, &bpf_dev_map_fops);
+	map->cdev.owner = map->cdev.ops->owner;
+	map->minor = bpf_map_get_minor(map);
+
+	devt = MKDEV(MAJOR(bpf_map_devt), map->minor);
+	ret = cdev_add(&map->cdev, devt, 1);
+	if (ret)
+		goto unwind;
+
+	dev = device_create(bpf_class, NULL, devt, NULL, "bpf_map%d",
+			    map->minor);
+	if (IS_ERR(dev)) {
+		ret = PTR_ERR(dev);
+		goto unwind_cdev;
+	}
+
+	map->flags |= BPF_F_HAS_DEV;
+	ret = map->minor;
+out:
+	mutex_unlock(&map->m_lock);
+	return ret;
+unwind_cdev:
+	cdev_del(&map->cdev);
+unwind:
+	bpf_map_put_minor(map);
+	goto out;
+}
+
+static int bpf_map_destroy_dev(struct bpf_map *map)
+{
+	bool drop_ref = false;
+	dev_t devt;
+	int ret;
+
+	mutex_lock(&map->m_lock);
+	if (!(map->flags & BPF_F_HAS_DEV)) {
+		ret = -ENOENT;
+		goto out;
+	}
+
+	devt = MKDEV(MAJOR(bpf_map_devt), map->minor);
+	ret = map->minor;
+
+	cdev_del(&map->cdev);
+	device_destroy(bpf_class, devt);
+	bpf_map_put_minor(map);
+
+	map->flags &= ~BPF_F_HAS_DEV;
+	drop_ref = true;
+out:
+	mutex_unlock(&map->m_lock);
+
+	if (drop_ref)
+		bpf_map_put(map);
+	return ret;
+}
+
+static int bpf_prog_make_dev(struct bpf_prog *prog)
+{
+	struct bpf_prog_aux *aux = prog->aux;
+	struct device *dev;
+	dev_t devt;
+	int ret;
+
+	mutex_lock(&aux->p_lock);
+	if (aux->flags & BPF_F_HAS_DEV) {
+		ret = aux->minor;
+		goto out;
+	}
+
+	cdev_init(&aux->cdev, &bpf_dev_prog_fops);
+	aux->cdev.owner = aux->cdev.ops->owner;
+	aux->minor = bpf_prog_get_minor(prog);
+
+	devt = MKDEV(MAJOR(bpf_prog_devt), aux->minor);
+	ret = cdev_add(&aux->cdev, devt, 1);
+	if (ret)
+		goto unwind;
+
+	dev = device_create(bpf_class, NULL, devt, NULL, "bpf_prog%d",
+			    aux->minor);
+	if (IS_ERR(dev)) {
+		ret = PTR_ERR(dev);
+		goto unwind_cdev;
+	}
+
+	aux->flags |= BPF_F_HAS_DEV;
+	ret = aux->minor;
+out:
+	mutex_unlock(&aux->p_lock);
+	return ret;
+unwind_cdev:
+	cdev_del(&aux->cdev);
+unwind:
+	bpf_prog_put_minor(prog);
+	goto out;
+}
+
+static int bpf_prog_destroy_dev(struct bpf_prog *prog)
+{
+	struct bpf_prog_aux *aux = prog->aux;
+	bool drop_ref = false;
+	dev_t devt;
+	int ret;
+
+	mutex_lock(&aux->p_lock);
+	if (!(aux->flags & BPF_F_HAS_DEV)) {
+		ret = -ENOENT;
+		goto out;
+	}
+
+	devt = MKDEV(MAJOR(bpf_prog_devt), aux->minor);
+	ret = aux->minor;
+
+	cdev_del(&aux->cdev);
+	device_destroy(bpf_class, devt);
+	bpf_prog_put_minor(prog);
+
+	aux->flags &= ~BPF_F_HAS_DEV;
+	drop_ref = true;
+out:
+	mutex_unlock(&aux->p_lock);
+
+	if (drop_ref)
+		bpf_prog_put(prog);
+	return ret;
+}
+
+static void bpf_any_get(void *raw, enum bpf_type type)
+{
+	switch (type) {
+	case BPF_TYPE_PROG:
+		atomic_inc(&((struct bpf_prog *)raw)->aux->refcnt);
+		break;
+	case BPF_TYPE_MAP:
+		atomic_inc(&((struct bpf_map *)raw)->refcnt);
+		break;
+	}
+}
+
+void bpf_any_put(void *raw, enum bpf_type type)
+{
+	switch (type) {
+	case BPF_TYPE_PROG:
+		bpf_prog_put(raw);
+		break;
+	case BPF_TYPE_MAP:
+		bpf_map_put(raw);
+		break;
+	}
+}
+
+static void *__bpf_dev_get(struct fd f, enum bpf_type *type)
+{
+	if (!f.file)
+		return ERR_PTR(-EBADF);
+	if (f.file->f_op != &bpf_dev_map_fops &&
+	    f.file->f_op != &bpf_dev_prog_fops) {
+		fdput(f);
+		return ERR_PTR(-EINVAL);
+	}
+
+	*type = f.file->f_op == &bpf_dev_map_fops ?
+		BPF_TYPE_MAP : BPF_TYPE_PROG;
+	return f.file->private_data;
+}
+
+static void *bpf_dev_get(u32 ufd, enum bpf_type *type)
+{
+	struct fd f = fdget(ufd);
+	void *raw;
+
+	raw = __bpf_dev_get(f, type);
+	if (IS_ERR(raw))
+		return raw;
+
+	bpf_any_get(raw, *type);
+	fdput(f);
+
+	return raw;
+}
+
+int __bpf_dev_create(__u32 ufd)
+{
+	enum bpf_type type;
+	void *raw;
+	int ret;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	type = BPF_TYPE_MAP;
+	raw = bpf_map_get(ufd);
+	if (IS_ERR(raw)) {
+		type = BPF_TYPE_PROG;
+		raw = bpf_prog_get(ufd);
+		if (IS_ERR(raw))
+			return PTR_ERR(raw);
+	}
+
+	switch (type) {
+	case BPF_TYPE_MAP:
+		ret = bpf_map_make_dev(raw);
+		break;
+	case BPF_TYPE_PROG:
+		ret = bpf_prog_make_dev(raw);
+		break;
+	}
+
+	if (ret < 0)
+		bpf_any_put(raw, type);
+
+	return ret;
+}
+
+int __bpf_dev_destroy(__u32 ufd)
+{
+	enum bpf_type type;
+	void *raw;
+	int ret;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	type = BPF_TYPE_MAP;
+	raw = bpf_map_get(ufd);
+	if (IS_ERR(raw)) {
+		type = BPF_TYPE_PROG;
+		raw = bpf_prog_get(ufd);
+		if (IS_ERR(raw))
+			return PTR_ERR(raw);
+	}
+
+	switch (type) {
+	case BPF_TYPE_MAP:
+		ret = bpf_map_destroy_dev(raw);
+		break;
+	case BPF_TYPE_PROG:
+		ret = bpf_prog_destroy_dev(raw);
+		break;
+	}
+
+	bpf_any_put(raw, type);
+	return ret;
+}
+
+int __bpf_dev_connect(__u32 ufd)
+{
+	enum bpf_type type;
+	void *raw;
+	int ret;
+
+	raw = bpf_dev_get(ufd, &type);
+	if (IS_ERR(raw))
+		return PTR_ERR(raw);
+
+	switch (type) {
+	case BPF_TYPE_MAP:
+		ret = bpf_map_new_fd(raw);
+		break;
+	case BPF_TYPE_PROG:
+		ret = bpf_prog_new_fd(raw);
+		break;
+	}
+	if (ret < 0)
+		bpf_any_put(raw, type);
+
+	return ret;
+}
+
+static int __init bpf_dev_init(void)
+{
+	int ret;
+
+	ret = alloc_chrdev_region(&bpf_map_devt, 0, BPF_MAX_DEVS,
+				  "bpf_map");
+	if (ret)
+		return ret;
+
+	ret = alloc_chrdev_region(&bpf_prog_devt, 0, BPF_MAX_DEVS,
+				  "bpf_prog");
+	if (ret)
+		unregister_chrdev_region(bpf_map_devt, BPF_MAX_DEVS);
+
+	bpf_class = class_create(THIS_MODULE, "bpf");
+	bpf_class->devnode = bpf_devnode;
+
+	return ret;
+}
+late_initcall(bpf_dev_init);
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index c629fe6..458b2f9 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -14,6 +14,7 @@
  #include <linux/slab.h>
  #include <linux/anon_inodes.h>
  #include <linux/file.h>
+#include <linux/mutex.h>
  #include <linux/license.h>
  #include <linux/filter.h>
  #include <linux/version.h>
@@ -111,7 +112,7 @@ static const struct file_operations bpf_map_fops = {
  	.release = bpf_map_release,
  };

-static int bpf_map_new_fd(struct bpf_map *map)
+int bpf_map_new_fd(struct bpf_map *map)
  {
  	return anon_inode_getfd("bpf-map", &bpf_map_fops, map,
  				O_RDWR | O_CLOEXEC);
@@ -141,6 +142,7 @@ static int map_create(union bpf_attr *attr)
  	if (IS_ERR(map))
  		return PTR_ERR(map);

+	mutex_init(&map->m_lock);
  	atomic_set(&map->refcnt, 1);

  	err = bpf_map_charge_memlock(map);
@@ -174,7 +176,7 @@ struct bpf_map *__bpf_map_get(struct fd f)
  	return f.file->private_data;
  }

-static struct bpf_map *bpf_map_get(u32 ufd)
+struct bpf_map *bpf_map_get(u32 ufd)
  {
  	struct fd f = fdget(ufd);
  	struct bpf_map *map;
@@ -525,18 +527,14 @@ static void __prog_put_common(struct rcu_head *rcu)
  /* version of bpf_prog_put() that is called after a grace period */
  void bpf_prog_put_rcu(struct bpf_prog *prog)
  {
-	if (atomic_dec_and_test(&prog->aux->refcnt)) {
-		prog->aux->prog = prog;
+	if (atomic_dec_and_test(&prog->aux->refcnt))
  		call_rcu(&prog->aux->rcu, __prog_put_common);
-	}
  }

  void bpf_prog_put(struct bpf_prog *prog)
  {
-	if (atomic_dec_and_test(&prog->aux->refcnt)) {
-		prog->aux->prog = prog;
+	if (atomic_dec_and_test(&prog->aux->refcnt))
  		__prog_put_common(&prog->aux->rcu);
-	}
  }
  EXPORT_SYMBOL_GPL(bpf_prog_put);

@@ -552,7 +550,7 @@ static const struct file_operations bpf_prog_fops = {
          .release = bpf_prog_release,
  };

-static int bpf_prog_new_fd(struct bpf_prog *prog)
+int bpf_prog_new_fd(struct bpf_prog *prog)
  {
  	return anon_inode_getfd("bpf-prog", &bpf_prog_fops, prog,
  				O_RDWR | O_CLOEXEC);
@@ -641,6 +639,7 @@ static int bpf_prog_load(union bpf_attr *attr)
  	prog->orig_prog = NULL;
  	prog->jited = 0;

+	mutex_init(&prog->aux->p_lock);
  	atomic_set(&prog->aux->refcnt, 1);
  	prog->gpl_compatible = is_gpl ? 1 : 0;

@@ -678,6 +677,32 @@ free_prog_nouncharge:
  	return err;
  }

+#define BPF_DEV_LAST_FIELD fd
+
+static int bpf_dev_create(const union bpf_attr *attr)
+{
+	if (CHECK_ATTR(BPF_DEV))
+		return -EINVAL;
+
+	return __bpf_dev_create(attr->fd);
+}
+
+static int bpf_dev_destroy(const union bpf_attr *attr)
+{
+	if (CHECK_ATTR(BPF_DEV))
+		return -EINVAL;
+
+	return __bpf_dev_destroy(attr->fd);
+}
+
+static int bpf_dev_connect(const union bpf_attr *attr)
+{
+	if (CHECK_ATTR(BPF_DEV))
+		return -EINVAL;
+
+	return __bpf_dev_connect(attr->fd);
+}
+
  SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *, uattr, unsigned int, size)
  {
  	union bpf_attr attr = {};
@@ -738,6 +763,15 @@ SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *, uattr, unsigned int, siz
  	case BPF_PROG_LOAD:
  		err = bpf_prog_load(&attr);
  		break;
+	case BPF_DEV_CREATE:
+		err = bpf_dev_create(&attr);
+		break;
+	case BPF_DEV_DESTROY:
+		err = bpf_dev_destroy(&attr);
+		break;
+	case BPF_DEV_CONNECT:
+		err = bpf_dev_connect(&attr);
+		break;
  	default:
  		err = -EINVAL;
  		break;
-- 
cgit

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1250234

On 10/18/15 9:49 AM, Daniel Borkmann wrote:
> Okay, I have pushed some rough working proof of concept here:
>
> https://git.breakpoint.cc/cgit/dborkman/net-next.git/log/?h=ebpf-fds-final5
>
> So the idea eventually had to be slightly modified after giving this
> further
> thoughts and is the following:
>
> We have 3 commands (BPF_DEV_CREATE, BPF_DEV_DESTROY, BPF_DEV_CONNECT), and
> related to that a bpf_attr extension with only a single __u32 fd member
> in it.
...
> The nice thing about it is that you can create/unlink as many as you
> want, but
> when you remove the real device from an application via
> bpf_dev_destroy(fd),
> then all links disappear with it. Just like in the case of a normal
> device driver.

interesting idea!
What happens if user app creates a dev via bpf_dev_create(), exits and
then admin does rm of that dev ?
Looks like map/prog will leak ?
So the only proper way to delete such cdevs is via bpf_dev_destroy ?

> On device creation, the kernel will return the minor number via bpf(2),
> so you
> can access the file easily, f.e. /dev/bpf/bpf_map<minor> resp.
> /dev/bpf/bpf_prog<minor>,
> and then move on with mknod(2) or symlink(2) from there if wished.

what if admin mknod in that dir with some arbitrary minor ?
mknod will succeed, but it won't hold anything?
looks like bpf_dev_connect will handle it gracefully.
So these cdevs should only be created and destroyed via bpf syscall
and only sensible operations on them is open() to get fd and pass
to bpf_dev_connect and symlink. Anything else admin should be
careful not to do. Right?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1250407

Hi,

On Sun, Oct 18, 2015, at 22:59, Alexei Starovoitov wrote:
> On 10/18/15 9:49 AM, Daniel Borkmann wrote:
> > Okay, I have pushed some rough working proof of concept here:
> >
> > https://git.breakpoint.cc/cgit/dborkman/net-next.git/log/?h=ebpf-fds-final5
> >
> > So the idea eventually had to be slightly modified after giving this
> > further
> > thoughts and is the following:
> >
> > We have 3 commands (BPF_DEV_CREATE, BPF_DEV_DESTROY, BPF_DEV_CONNECT), and
> > related to that a bpf_attr extension with only a single __u32 fd member
> > in it.
> ...
> > The nice thing about it is that you can create/unlink as many as you
> > want, but
> > when you remove the real device from an application via
> > bpf_dev_destroy(fd),
> > then all links disappear with it. Just like in the case of a normal
> > device driver.
> 
> interesting idea!
> What happens if user app creates a dev via bpf_dev_create(), exits and
> then admin does rm of that dev ?
> Looks like map/prog will leak ?
> So the only proper way to delete such cdevs is via bpf_dev_destroy ?

The mknod is not the holder but rather the kobject which should be
represented in sysfs will be. So you can still get the map major:minor
by looking up the /dev file in the correspdonding sysfs directory or I
think we should provide a 'unbind' file, which will drop the kobject if
the user writes a '1' to it.

> 
> > On device creation, the kernel will return the minor number via bpf(2),
> > so you
> > can access the file easily, f.e. /dev/bpf/bpf_map<minor> resp.
> > /dev/bpf/bpf_prog<minor>,
> > and then move on with mknod(2) or symlink(2) from there if wished.
> 
> what if admin mknod in that dir with some arbitrary minor ?

Basically, -EIO. :)

> mknod will succeed, but it won't hold anything?

That is right now true for basically all mknod operations, which udev
creates.

> looks like bpf_dev_connect will handle it gracefully.
> So these cdevs should only be created and destroyed via bpf syscall
> and only sensible operations on them is open() to get fd and pass
> to bpf_dev_connect and symlink. Anything else admin should be
> careful not to do. Right?

Besides maybe some statistics and other stuff in sysfs directory, no,
that is all.

Bye,
Hannes

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1250535

On 10/19/2015 09:36 AM, Hannes Frederic Sowa wrote:
> Hi,
>
> On Sun, Oct 18, 2015, at 22:59, Alexei Starovoitov wrote:
>> On 10/18/15 9:49 AM, Daniel Borkmann wrote:
>>> Okay, I have pushed some rough working proof of concept here:
>>>
>>> https://git.breakpoint.cc/cgit/dborkman/net-next.git/log/?h=ebpf-fds-final5
>>>
>>> So the idea eventually had to be slightly modified after giving this
>>> further
>>> thoughts and is the following:
>>>
>>> We have 3 commands (BPF_DEV_CREATE, BPF_DEV_DESTROY, BPF_DEV_CONNECT), and
>>> related to that a bpf_attr extension with only a single __u32 fd member
>>> in it.
>> ...
>>> The nice thing about it is that you can create/unlink as many as you
>>> want, but
>>> when you remove the real device from an application via
>>> bpf_dev_destroy(fd),
>>> then all links disappear with it. Just like in the case of a normal
>>> device driver.
>>
>> interesting idea!
>> What happens if user app creates a dev via bpf_dev_create(), exits and
>> then admin does rm of that dev ?
>> Looks like map/prog will leak ?
>> So the only proper way to delete such cdevs is via bpf_dev_destroy ?
>
> The mknod is not the holder but rather the kobject which should be
> represented in sysfs will be. So you can still get the map major:minor
> by looking up the /dev file in the correspdonding sysfs directory or I
> think we should provide a 'unbind' file, which will drop the kobject if
> the user writes a '1' to it.

I agree, this could still be done.

>>> On device creation, the kernel will return the minor number via bpf(2),
>>> so you
>>> can access the file easily, f.e. /dev/bpf/bpf_map<minor> resp.
>>> /dev/bpf/bpf_prog<minor>,
>>> and then move on with mknod(2) or symlink(2) from there if wished.
>>
>> what if admin mknod in that dir with some arbitrary minor ?
>
> Basically, -EIO. :)
>
>> mknod will succeed, but it won't hold anything?
>
> That is right now true for basically all mknod operations, which udev
> creates.
>
>> looks like bpf_dev_connect will handle it gracefully.
>> So these cdevs should only be created and destroyed via bpf syscall
>> and only sensible operations on them is open() to get fd and pass
>> to bpf_dev_connect and symlink. Anything else admin should be
>> careful not to do. Right?
>
> Besides maybe some statistics and other stuff in sysfs directory, no,
> that is all.
>
> Bye,
> Hannes
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1250747

On 10/19/2015 11:51 AM, Daniel Borkmann wrote:
> On 10/19/2015 09:36 AM, Hannes Frederic Sowa wrote:
>> On Sun, Oct 18, 2015, at 22:59, Alexei Starovoitov wrote:
>>> On 10/18/15 9:49 AM, Daniel Borkmann wrote:
>>>> Okay, I have pushed some rough working proof of concept here:
>>>>
>>>> https://git.breakpoint.cc/cgit/dborkman/net-next.git/log/?h=ebpf-fds-final5
>>>>
>>>> So the idea eventually had to be slightly modified after giving this
>>>> further
>>>> thoughts and is the following:
>>>>
>>>> We have 3 commands (BPF_DEV_CREATE, BPF_DEV_DESTROY, BPF_DEV_CONNECT), and
>>>> related to that a bpf_attr extension with only a single __u32 fd member
>>>> in it.
>>> ...
>>>> The nice thing about it is that you can create/unlink as many as you
>>>> want, but
>>>> when you remove the real device from an application via
>>>> bpf_dev_destroy(fd),
>>>> then all links disappear with it. Just like in the case of a normal
>>>> device driver.
>>>
>>> interesting idea!
>>> What happens if user app creates a dev via bpf_dev_create(), exits and
>>> then admin does rm of that dev ?
>>> Looks like map/prog will leak ?
>>> So the only proper way to delete such cdevs is via bpf_dev_destroy ?
>>
>> The mknod is not the holder but rather the kobject which should be
>> represented in sysfs will be. So you can still get the map major:minor
>> by looking up the /dev file in the correspdonding sysfs directory or I
>> think we should provide a 'unbind' file, which will drop the kobject if
>> the user writes a '1' to it.
>
> I agree, this could still be done.
>
>>>> On device creation, the kernel will return the minor number via bpf(2),
>>>> so you
>>>> can access the file easily, f.e. /dev/bpf/bpf_map<minor> resp.
>>>> /dev/bpf/bpf_prog<minor>,
>>>> and then move on with mknod(2) or symlink(2) from there if wished.
>>>
>>> what if admin mknod in that dir with some arbitrary minor ?
>>
>> Basically, -EIO. :)

If an admin does a mknod that has the major of a map or prog cdev, but a
not yet used minor, then connecting to that fails. And at the time when a
real device has been created with that assigned minor, then connecting to
it succeeds.

It's nothing different than with other devices in the system, f.e. ...

   # ls -la /dev/urandom
   crw-rw-rw-. 1 root root 1, 9 Oct 19 15:18 /dev/urandom
   # mknod ./foobar c 1 9

... will make random driver available under ./foobar as well.

If your question is rather on what happens when an admin does an ``mknod
/dev/bpf/bpf_map9 c 249 11'' and the device created has a minor of 9 and
/dev/bpf/bpf_map9 already exists in the system, then udev won't auto-create
or overwrite the node pointing to the major:minor there. The device itself
is being created nevertheless and visible under /sys/class/bpf/, but I think
this is a non-issue and nothing different from any other device drivers.

As Hannes said, under /sys/class/bpf/ an admin can see all held nodes, so
visibility is there for free at all times. The device management (creation/
deletion) itself and the mknod's pointing to it are simply decoupled.

This whole approach looks sound to me, also integrates nicely into the
existing Linux facilities, and works on top of every fs supporting special
files. Much cleaner than an extra file-system that would be required by a
syscall in order to make the syscall work.

>>> mknod will succeed, but it won't hold anything?
>>
>> That is right now true for basically all mknod operations, which udev
>> creates.
>>
>>> looks like bpf_dev_connect will handle it gracefully.
>>> So these cdevs should only be created and destroyed via bpf syscall
>>> and only sensible operations on them is open() to get fd and pass
>>> to bpf_dev_connect and symlink. Anything else admin should be
>>> careful not to do. Right?
>>
>> Besides maybe some statistics and other stuff in sysfs directory, no,
>> that is all.
>>
>> Bye,
>> Hannes
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1250894

On 10/19/15 7:23 AM, Daniel Borkmann wrote:
>>> The mknod is not the holder but rather the kobject which should be
>>> represented in sysfs will be. So you can still get the map major:minor
>>> by looking up the /dev file in the correspdonding sysfs directory or I
>>> think we should provide a 'unbind' file, which will drop the kobject if
>>> the user writes a '1' to it.
>>
>> I agree, this could still be done.

imo doing 'rm' is way cleaner then dealing with 'unbind' file.

> As Hannes said, under /sys/class/bpf/ an admin can see all held nodes, so
> visibility is there for free at all times. The device management (creation/
> deletion) itself and the mknod's pointing to it are simply decoupled.
>
> This whole approach looks sound to me, also integrates nicely into the
> existing Linux facilities, and works on top of every fs supporting special
> files. Much cleaner than an extra file-system that would be required by a
> syscall in order to make the syscall work.

thanks for the explanations. I think I got a complete picture now on
how such cdev will be used and I don't like it.
There is nothing in linux or any unix that creates thousands of cdevs
on the fly, but here user apps will create/destroy them back and forth
and they would need to do it quickly. Whole sysfs/kobj baggage is
completely unnecessary here. The kernel will consume more memory for
no real reason other than cdev are used to keep prog/maps around.
imo fs is cleaner and we can tailor it to be similar to cdev style.
For example we can make bpffs automount in /sys/kernel/bpf/ as standard
location and have one directory structure for all mounts (like tracefs).
Then within it have idr mechanism to crate bpf_progX and bpf_mapY
special files via BPF_PIN_FD bpf syscall with single FD argument.
At this point fs and cdev approach from user point of view look
exactly the same, but overhead of fs is significantly lower,
normal 'rm' works just fine and much faster.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1250953

On 10/19/2015 06:22 PM, Alexei Starovoitov wrote:
> On 10/19/15 7:23 AM, Daniel Borkmann wrote:
>>>> The mknod is not the holder but rather the kobject which should be
>>>> represented in sysfs will be. So you can still get the map major:minor
>>>> by looking up the /dev file in the correspdonding sysfs directory or I
>>>> think we should provide a 'unbind' file, which will drop the kobject if
>>>> the user writes a '1' to it.
>>>
>>> I agree, this could still be done.
>
> imo doing 'rm' is way cleaner then dealing with 'unbind' file.

Hmm, not sure, maybe this was misunderstood. It's not about files, but
rather devices. Devices are decoupled.

This unbind file is optional and could live under /sys/class/bpf/bpf_{map,
prog}<X>/unbind for a device release. It's not strictly necessary for this
to work, though, the management is, as explained, via bpf() syscall.

>> As Hannes said, under /sys/class/bpf/ an admin can see all held nodes, so
>> visibility is there for free at all times. The device management (creation/
>> deletion) itself and the mknod's pointing to it are simply decoupled.
>>
>> This whole approach looks sound to me, also integrates nicely into the
>> existing Linux facilities, and works on top of every fs supporting special
>> files. Much cleaner than an extra file-system that would be required by a
>> syscall in order to make the syscall work.
>
> thanks for the explanations. I think I got a complete picture now on
> how such cdev will be used and I don't like it.
> There is nothing in linux or any unix that creates thousands of cdevs
> on the fly, but here user apps will create/destroy them back and forth
> and they would need to do it quickly. Whole sysfs/kobj baggage is

Well, you are talking about thousand maps and even root can create about
5 maps and then will get an -EPERM. ;) Until an admin will figure out over
couple of corners that ulimit -l needs to be adjusted ... ;)

But more serious, can you elaborate what you mean?

An eBPF program or map loading/destruction is *not* by any means to be
considered fast-path. We currently hold a global mutex during loading.
So, how can that be considered fast-path? Similarly, socket creation/
destruction is also not fast-path, etc. Do you expect that applications
would create/destroy these devices within milliseconds? I'd argue that
something would be seriously wrong with that application, then. Such
persistent maps are to be considered rather mid-long living objects in
the system. The fast-path surely is the data-path of them.

> completely unnecessary here. The kernel will consume more memory for
> no real reason other than cdev are used to keep prog/maps around.

I don't consider this a big issue, and well worth the trade-off. You'll
have an infrastructure that integrates *nicely* into the *existing* kernel
model *and* tooling with the proposed patch. This is a HUGE plus. The
UAPI of this is simple and minimal. And to me, these are in-fact special
files, not regular ones.

> imo fs is cleaner and we can tailor it to be similar to cdev style.

Really, IMHO I think this is over-designed, and much much more hacky. We
design a whole new file system that works *exactly* like cdevs, takes
likely more than twice the code and complexity to realize but just to
save a few bytes ...? I don't understand that.

Cheers,
Daniel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1250986

On 10/19/15 10:37 AM, Daniel Borkmann wrote:
> An eBPF program or map loading/destruction is *not* by any means to be
> considered fast-path. We currently hold a global mutex during loading.
> So, how can that be considered fast-path? Similarly, socket creation/
> destruction is also not fast-path, etc. Do you expect that applications
> would create/destroy these devices within milliseconds? I'd argue that
> something would be seriously wrong with that application, then. Such
> persistent maps are to be considered rather mid-long living objects in
> the system. The fast-path surely is the data-path of them.

consider seccomp that loads several programs for every tab, then
container use case where we're loading two for each as well.
Who knows what use cases will come up in the future.
It's obviously not a fast path that is being hit million times a second,
but why add overhead when it's unnecessary?

>> completely unnecessary here. The kernel will consume more memory for
>> no real reason other than cdev are used to keep prog/maps around.
>
> I don't consider this a big issue, and well worth the trade-off. You'll
> have an infrastructure that integrates *nicely* into the *existing* kernel
> model *and* tooling with the proposed patch. This is a HUGE plus. The
> UAPI of this is simple and minimal. And to me, these are in-fact special
> files, not regular ones.

Seriously? Syscall to create/destory cdevs is a nice api?
Not by any means. We can argue in circles, but it doesn't fit.
Using cdev to hold maps is a hack.
Telling kernel that fake device is created only to hold a map?
This fake device doesn't have any of the device properties.
Look at fops->open. Surely it's a smart trick, but it's not behaving
like device.

uapi for fs adds only two commands, whereas cdev needs three.

>> imo fs is cleaner and we can tailor it to be similar to cdev style.
>
> Really, IMHO I think this is over-designed, and much much more hacky. We
> design a whole new file system that works *exactly* like cdevs, takes
> likely more than twice the code and complexity to realize but just to
> save a few bytes ...? I don't understand that.

Let's argue with facts. Your fs patch 758 lines vs cdev 483.
In fs the cost is single alloc_inode(), whereas in cdev the struct cdev
and mutex will add to all maps and progs (even when they're not going
to be pinned), plus kobj allocation, plus gigantic struct device, plus
sysfs inodes, etc. Way more than few bytes of difference.

where do you see 'over-design' in fs? It's a straightforward code and
most of it is boilerplate like all other fs.
Kill rmdir/mkdir ops, term bit and options and the fs diff will shrink
by another 200+ lines.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1251000

Hi,

On Mon, Oct 19, 2015, at 20:15, Alexei Starovoitov wrote:
> On 10/19/15 10:37 AM, Daniel Borkmann wrote:
> > An eBPF program or map loading/destruction is *not* by any means to be
> > considered fast-path. We currently hold a global mutex during loading.
> > So, how can that be considered fast-path? Similarly, socket creation/
> > destruction is also not fast-path, etc. Do you expect that applications
> > would create/destroy these devices within milliseconds? I'd argue that
> > something would be seriously wrong with that application, then. Such
> > persistent maps are to be considered rather mid-long living objects in
> > the system. The fast-path surely is the data-path of them.
> 
> consider seccomp that loads several programs for every tab, then
> container use case where we're loading two for each as well.
> Who knows what use cases will come up in the future.
> It's obviously not a fast path that is being hit million times a second,
> but why add overhead when it's unnecessary?

But the browser using seccomp does not need persistent maps at all. It
would merely create temporary maps without the overhead which are
automatically released when the program finished. In no way should they
be persistent but automatically garbage collected as soon as the
supervisor process and its children die.

> >> completely unnecessary here. The kernel will consume more memory for
> >> no real reason other than cdev are used to keep prog/maps around.
> >
> > I don't consider this a big issue, and well worth the trade-off. You'll
> > have an infrastructure that integrates *nicely* into the *existing* kernel
> > model *and* tooling with the proposed patch. This is a HUGE plus. The
> > UAPI of this is simple and minimal. And to me, these are in-fact special
> > files, not regular ones.
> 
> Seriously? Syscall to create/destory cdevs is a nice api?

It is pretty normal to do. They don't use syscall but ioctl on a special
pseudo device node (lvm2/device-mapper, tun, etc.). Same thing, very
overloaded syscall. Also I don't see a reason to not make the creation
possible via sysfs, which would be even nicer but not orthogonal to the
current creation of maps, which is nowadays set in stone by uapi.

> Not by any means. We can argue in circles, but it doesn't fit.
> Using cdev to hold maps is a hack.
> Telling kernel that fake device is created only to hold a map?
> This fake device doesn't have any of the device properties.
> Look at fops->open. Surely it's a smart trick, but it's not behaving
> like device.
> 
> uapi for fs adds only two commands, whereas cdev needs three.
> 
> >> imo fs is cleaner and we can tailor it to be similar to cdev style.
> >
> > Really, IMHO I think this is over-designed, and much much more hacky. We
> > design a whole new file system that works *exactly* like cdevs, takes
> > likely more than twice the code and complexity to realize but just to
> > save a few bytes ...? I don't understand that.
> 
> Let's argue with facts. Your fs patch 758 lines vs cdev 483.
> In fs the cost is single alloc_inode(), whereas in cdev the struct cdev
> and mutex will add to all maps and progs (even when they're not going
> to be pinned), plus kobj allocation, plus gigantic struct device, plus
> sysfs inodes, etc. Way more than few bytes of difference.

sysfs already has infrastructure to ensure supportability and
debugability. Dependency graphs can be created to see which programs
depend on which bpf_progs. kobjects are singeltons in sysfs
representation. If you have multiple ebpf filesystems, even maybe
referencing the same hashtable with gigabytes of data multiple times,
there needs to be some way to help administrators to check resource
usage, statistics, tweak and tune the rhashtable. All this needs to be
handled as well in the future. It doesn't really fit the filesystem
model, but representing a kobject seems to be a good fit to me.

Policy can be done by user space with help of udev. Selinux policies can
easily being extended to allow specific domains access. Namespace
"passthrough" is defined for devices already.

> where do you see 'over-design' in fs? It's a straightforward code and
> most of it is boilerplate like all other fs.
> Kill rmdir/mkdir ops, term bit and options and the fs diff will shrink
> by another 200+ lines.

I don't think that only a filesystem will do it in the foreseeable
future. You want to have tools like lsof reporting which map and which
program has references to each other. Those file nodes will need more
metadata attached in future anyway, so currently just comparing lines of
codes seems not to be a good way for arguing.

With filesystems suddenly a lot of other questions arise: selinux
support, acls etc.

Bye,
Hannes
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1251033

On 10/19/15 11:46 AM, Hannes Frederic Sowa wrote:
> Hi,
>
> On Mon, Oct 19, 2015, at 20:15, Alexei Starovoitov wrote:
>> On 10/19/15 10:37 AM, Daniel Borkmann wrote:
>>> An eBPF program or map loading/destruction is *not* by any means to be
>>> considered fast-path. We currently hold a global mutex during loading.
>>> So, how can that be considered fast-path? Similarly, socket creation/
>>> destruction is also not fast-path, etc. Do you expect that applications
>>> would create/destroy these devices within milliseconds? I'd argue that
>>> something would be seriously wrong with that application, then. Such
>>> persistent maps are to be considered rather mid-long living objects in
>>> the system. The fast-path surely is the data-path of them.
>>
>> consider seccomp that loads several programs for every tab, then
>> container use case where we're loading two for each as well.
>> Who knows what use cases will come up in the future.
>> It's obviously not a fast path that is being hit million times a second,
>> but why add overhead when it's unnecessary?
>
> But the browser using seccomp does not need persistent maps at all. It

today it doesn't, but if it was a light weight feature, it could
have been used much more broadly.
Currently we're using user agent for networking and it's a pain.

>>>> completely unnecessary here. The kernel will consume more memory for
>>>> no real reason other than cdev are used to keep prog/maps around.
>>>
>>> I don't consider this a big issue, and well worth the trade-off. You'll
>>> have an infrastructure that integrates *nicely* into the *existing* kernel
>>> model *and* tooling with the proposed patch. This is a HUGE plus. The
>>> UAPI of this is simple and minimal. And to me, these are in-fact special
>>> files, not regular ones.
>>
>> Seriously? Syscall to create/destory cdevs is a nice api?
>
> It is pretty normal to do. They don't use syscall but ioctl on a special
> pseudo device node (lvm2/device-mapper, tun, etc.). Same thing, very
> overloaded syscall. Also I don't see a reason to not make the creation
> possible via sysfs, which would be even nicer but not orthogonal to the
> current creation of maps, which is nowadays set in stone by uapi.

Isn't it your point going against cdev? ioctls on devs are overloaded,
whereas here we have clean bpf syscall with no extra baggage.
Going old-school to device model goes against that clean syscall
approach.

> sysfs already has infrastructure to ensure supportability and
> debugability. Dependency graphs can be created to see which programs
> depend on which bpf_progs. kobjects are singeltons in sysfs
> representation. If you have multiple ebpf filesystems, even maybe
> referencing the same hashtable with gigabytes of data multiple times,
> there needs to be some way to help administrators to check resource
> usage, statistics, tweak and tune the rhashtable. All this needs to be
> handled as well in the future. It doesn't really fit the filesystem
> model, but representing a kobject seems to be a good fit to me.

quite the opposite. The way cdev patch is done now, there is no way
to extend it to create a hierarchy without breaking users,
whereas fs style with initial Daniel's patch can be extended.
Doing 'resource stats' via sysfs requires bpf to add to sysfs, which
is not this cdev approach.

> Policy can be done by user space with help of udev. Selinux policies can
> easily being extended to allow specific domains access. Namespace
> "passthrough" is defined for devices already.

that's an interesting point, but isn't it better done with fs?
you can go much more fine-grained with directory permissions in fs.
With different users/namespaces having their own hierarchies and mounts
whereas cdev dumps everything into one spot.

>> where do you see 'over-design' in fs? It's a straightforward code and
>> most of it is boilerplate like all other fs.
>> Kill rmdir/mkdir ops, term bit and options and the fs diff will shrink
>> by another 200+ lines.
>
> I don't think that only a filesystem will do it in the foreseeable
> future. You want to have tools like lsof reporting which map and which
> program has references to each other. Those file nodes will need more
> metadata attached in future anyway, so currently just comparing lines of
> codes seems not to be a good way for arguing.

my point was that both have roughly the same number, but the lines of
code will grow in either approach and when cdev starts as a hack I can
only see more hacks added in the future, whereas fs gives us
full flexibility to do any type of file access and user visible
representation.
Also I don't buy the point of reinventing sysfs. bpffs is not doing
sysfs. I don't want to see _every_ bpf object in sysfs. It's way too
much overhead. Classic doesn't have sysfs and everyone have been
using it just fine.
bpffs is solving the need to 'pin_fd' when user process exits.
That's it. Let's solve that and not go into any of this sysfs stuff.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1251045

Hi Alexei,

On Mon, Oct 19, 2015, at 21:34, Alexei Starovoitov wrote:
> On 10/19/15 11:46 AM, Hannes Frederic Sowa wrote:
> > Hi,
> >
> > On Mon, Oct 19, 2015, at 20:15, Alexei Starovoitov wrote:
> >> On 10/19/15 10:37 AM, Daniel Borkmann wrote:
> >>> An eBPF program or map loading/destruction is *not* by any means to be
> >>> considered fast-path. We currently hold a global mutex during loading.
> >>> So, how can that be considered fast-path? Similarly, socket creation/
> >>> destruction is also not fast-path, etc. Do you expect that applications
> >>> would create/destroy these devices within milliseconds? I'd argue that
> >>> something would be seriously wrong with that application, then. Such
> >>> persistent maps are to be considered rather mid-long living objects in
> >>> the system. The fast-path surely is the data-path of them.
> >>
> >> consider seccomp that loads several programs for every tab, then
> >> container use case where we're loading two for each as well.
> >> Who knows what use cases will come up in the future.
> >> It's obviously not a fast path that is being hit million times a second,
> >> but why add overhead when it's unnecessary?
> >
> > But the browser using seccomp does not need persistent maps at all. It
> 
> today it doesn't, but if it was a light weight feature, it could
> have been used much more broadly.
> Currently we're using user agent for networking and it's a pain.

I doubt it will stay a lightweight feature as it should not be in the
responsibility of user space to provide those debug facilities.

> >>>> completely unnecessary here. The kernel will consume more memory for
> >>>> no real reason other than cdev are used to keep prog/maps around.
> >>>
> >>> I don't consider this a big issue, and well worth the trade-off. You'll
> >>> have an infrastructure that integrates *nicely* into the *existing* kernel
> >>> model *and* tooling with the proposed patch. This is a HUGE plus. The
> >>> UAPI of this is simple and minimal. And to me, these are in-fact special
> >>> files, not regular ones.
> >>
> >> Seriously? Syscall to create/destory cdevs is a nice api?
> >
> > It is pretty normal to do. They don't use syscall but ioctl on a special
> > pseudo device node (lvm2/device-mapper, tun, etc.). Same thing, very
> > overloaded syscall. Also I don't see a reason to not make the creation
> > possible via sysfs, which would be even nicer but not orthogonal to the
> > current creation of maps, which is nowadays set in stone by uapi.
> 
> Isn't it your point going against cdev? ioctls on devs are overloaded,
> whereas here we have clean bpf syscall with no extra baggage.
> Going old-school to device model goes against that clean syscall
> approach.

The bpf syscall is still used to create the pseudo nodes. If they should
be persistent they just get registered in the sysfs class hierarchy.

> > sysfs already has infrastructure to ensure supportability and
> > debugability. Dependency graphs can be created to see which programs
> > depend on which bpf_progs. kobjects are singeltons in sysfs
> > representation. If you have multiple ebpf filesystems, even maybe
> > referencing the same hashtable with gigabytes of data multiple times,
> > there needs to be some way to help administrators to check resource
> > usage, statistics, tweak and tune the rhashtable. All this needs to be
> > handled as well in the future. It doesn't really fit the filesystem
> > model, but representing a kobject seems to be a good fit to me.
> 
> quite the opposite. The way cdev patch is done now, there is no way
> to extend it to create a hierarchy without breaking users,
> whereas fs style with initial Daniel's patch can be extended.
> Doing 'resource stats' via sysfs requires bpf to add to sysfs, which
> is not this cdev approach.

This is not yet part of the patch, but I think this would be added.
Daniel?

> > Policy can be done by user space with help of udev. Selinux policies can
> > easily being extended to allow specific domains access. Namespace
> > "passthrough" is defined for devices already.
> 
> that's an interesting point, but isn't it better done with fs?
> you can go much more fine-grained with directory permissions in fs.
> With different users/namespaces having their own hierarchies and mounts
> whereas cdev dumps everything into one spot.

Policy can already be defined in terms of cgroups:
<http://lxr.free-electrons.com/source/Documentation/cgroups/devices.txt>

I don't think there are broad differences. But in case a namespaces uses
huge number of maps with tons of data, the admin in the initial
namespace might want to debug that without searching all mountpoints and
find dependencies between processes etc. IMHO sysfs approach can be
better extended here.
 
> >> where do you see 'over-design' in fs? It's a straightforward code and
> >> most of it is boilerplate like all other fs.
> >> Kill rmdir/mkdir ops, term bit and options and the fs diff will shrink
> >> by another 200+ lines.
> >
> > I don't think that only a filesystem will do it in the foreseeable
> > future. You want to have tools like lsof reporting which map and which
> > program has references to each other. Those file nodes will need more
> > metadata attached in future anyway, so currently just comparing lines of
> > codes seems not to be a good way for arguing.
> 
> my point was that both have roughly the same number, but the lines of
> code will grow in either approach and when cdev starts as a hack I can
> only see more hacks added in the future, whereas fs gives us
> full flexibility to do any type of file access and user visible
> representation.
> Also I don't buy the point of reinventing sysfs. bpffs is not doing
> sysfs. I don't want to see _every_ bpf object in sysfs. It's way too
> much overhead. Classic doesn't have sysfs and everyone have been
> using it just fine.

But classic bpf does not have persistence for maps and data. ;) There is
a 1:1 relationship between socket and bpf_prog for example.

> bpffs is solving the need to 'pin_fd' when user process exits.
> That's it. Let's solve that and not go into any of this sysfs stuff.

But how can the filesystem be extended in terms of tunables and
information? File attributes? Wouldn't it need the same infrastructure
otherwise as sysfs? Some third-party lookup filesystem or ioctl? This
char dev approach also pins maps and progs while giving more policy in
hand of central user space programs we are currently using (udev,
systemd, whatever, etc.).

Bye,
Hannes
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1251062

On 10/19/15 1:03 PM, Hannes Frederic Sowa wrote:
>
> I doubt it will stay a lightweight feature as it should not be in the
> responsibility of user space to provide those debug facilities.

It feels we're talking past each other.
I want to solve 'persistent map' problem.
debugging of maps/progs, hierarchy, etc are all nice to have,
but different issues.
In case of persistent maps I imagine unprivileged process would want
to use it eventually as well, so this requirement already kills cdev
approach for me, since I don't think we ever let unprivileged apps
create cdev with syscall.

> The bpf syscall is still used to create the pseudo nodes. If they should
> be persistent they just get registered in the sysfs class hierarchy.

nope. they should not. sysfs is debugging/tunning facility.
There is absolutely no need for bpf to plug into sysfs.

>> Doing 'resource stats' via sysfs requires bpf to add to sysfs, which
>> is not this cdev approach.
>
> This is not yet part of the patch, but I think this would be added.
> Daniel?

please don't. I'm strongly against adding unnecessary bloat.

> I don't think there are broad differences. But in case a namespaces uses
> huge number of maps with tons of data, the admin in the initial
> namespace might want to debug that without searching all mountpoints and
> find dependencies between processes etc. IMHO sysfs approach can be
> better extended here.

sure, then we can force all bpffs to have the same hierarchy and mounted
in /sys/kernel/bpf location. That would be the same.

It feels you're pushing for cdev only because of that potential
debugging need. Did you actually face that need? I didn't and
don't like to add 'nice to have' feature until real need comes.

>> Also I don't buy the point of reinventing sysfs. bpffs is not doing
>> sysfs. I don't want to see _every_ bpf object in sysfs. It's way too
>> much overhead. Classic doesn't have sysfs and everyone have been
>> using it just fine.
>
> But classic bpf does not have persistence for maps and data. ;) There is
> a 1:1 relationship between socket and bpf_prog for example.

single task in seccomp can have a chain of bpf progs, so hierarchy
is already there.

> But how can the filesystem be extended in terms of tunables and
> information? File attributes? Wouldn't it need the same infrastructure
> otherwise as sysfs? Some third-party lookup filesystem or ioctl? This
> char dev approach also pins maps and progs while giving more policy in
> hand of central user space programs we are currently using (udev,
> systemd, whatever, etc.).

tunables for bpf maps? There are no such things today.
I think you're implying that we can add rhashtable type of map, so
admin can tune thresholds ? Ouch. I think if we add it, its parameters
will be specified by the user that is creating the map only. There will
be no tunables exposed to sysfs and there should be no way of creating
maps via sysfs.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1251131

On 10/19/2015 10:48 PM, Alexei Starovoitov wrote:
> On 10/19/15 1:03 PM, Hannes Frederic Sowa wrote:
>>
>> I doubt it will stay a lightweight feature as it should not be in the
>> responsibility of user space to provide those debug facilities.
>
> It feels we're talking past each other.
> I want to solve 'persistent map' problem.
> debugging of maps/progs, hierarchy, etc are all nice to have,
> but different issues.

Ok, so you are saying that this file system will have *only* regular
files that are to be considered nodes for eBPF maps and progs. Nothing
else will ever be added to it? Yet, eBPF map and prog nodes are *not*
*regular files* to me, this seems odd.

As soon as you are starting to add additional folders that contain
files dumping additional meta data, etc, you basically end up on a
bit of similar concept to sysfs, no?

> In case of persistent maps I imagine unprivileged process would want
> to use it eventually as well, so this requirement already kills cdev
> approach for me, since I don't think we ever let unprivileged apps
> create cdev with syscall.

Hmm, I see. So far the discussion was only about having this for privileged
users (also in this fs patch). F.e. privileged system daemons could setup
and distribute progs/maps to consumers, etc (f.e. seccomp and tc case).

When we start lifting this, eBPF maps by its own will become a real kernel
IPC facility for unprivileged Linux applications (independently whether
they are connected to an actual eBPF program). Those kernel IPC facilities
that are anchored in the file system like named pipes and Unix domain sockets
are indicated as such as special files, no?

>> The bpf syscall is still used to create the pseudo nodes. If they should
>> be persistent they just get registered in the sysfs class hierarchy.
>
> nope. they should not. sysfs is debugging/tunning facility.
> There is absolutely no need for bpf to plug into sysfs.
>
>>> Doing 'resource stats' via sysfs requires bpf to add to sysfs, which
>>> is not this cdev approach.
>>
>> This is not yet part of the patch, but I think this would be added.
>> Daniel?
>
> please don't. I'm strongly against adding unnecessary bloat.
>
>> I don't think there are broad differences. But in case a namespaces uses
>> huge number of maps with tons of data, the admin in the initial
>> namespace might want to debug that without searching all mountpoints and
>> find dependencies between processes etc. IMHO sysfs approach can be
>> better extended here.
>
> sure, then we can force all bpffs to have the same hierarchy and mounted
> in /sys/kernel/bpf location. That would be the same.

That would imply to have a mount_single() file system (like f.e. tracefs and
securityfs), right?

So you'd loose having various mounts in different namespaces. And if you
allow various mount points, how would that /facilitate/ to an admin to
identify all eBPF objects/resources currently present in the system?

Or to an application developer finding possible mount points for his own
application so that bpf(2) syscall to create these nodes succeeds? Would
you make mounting also unprivileged? What if various distros have these
mount points at different locations? What should unprivileged applications
do to know that they can use these locations for themselves?

Also, since they are only regular files, one can only try and find these
objects based on their naming schemes, which seems to get a bit odd in case
this file system also carries (perhaps future?) other regular files that
are not eBPF map and program-special.

> It feels you're pushing for cdev only because of that potential
> debugging need. Did you actually face that need? I didn't and
> don't like to add 'nice to have' feature until real need comes.

I think this discussion arose, because the question of how flexible we are
in future to extend this facility. Nothing more.

I still don't consider the cdev approach as a hack. Despite a bit "old
school" as you say, the semantics seem to be better defined, imho. It
integrates better into existing facilities.

Cheers,
Daniel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1251172

On 10/19/15 3:17 PM, Daniel Borkmann wrote:
> On 10/19/2015 10:48 PM, Alexei Starovoitov wrote:
>> On 10/19/15 1:03 PM, Hannes Frederic Sowa wrote:
>>>
>>> I doubt it will stay a lightweight feature as it should not be in the
>>> responsibility of user space to provide those debug facilities.
>>
>> It feels we're talking past each other.
>> I want to solve 'persistent map' problem.
>> debugging of maps/progs, hierarchy, etc are all nice to have,
>> but different issues.
>
> Ok, so you are saying that this file system will have *only* regular
> files that are to be considered nodes for eBPF maps and progs. Nothing
> else will ever be added to it? Yet, eBPF map and prog nodes are *not*
> *regular files* to me, this seems odd.
>
> As soon as you are starting to add additional folders that contain
> files dumping additional meta data, etc, you basically end up on a
> bit of similar concept to sysfs, no?

as we discussed in this thread and earlier during plumbers I think
it would be good to expose key/values somehow in this fs.
'how' is a big question.
But regardless which path we take, sysfs is too rigid.
For the sake of argument say we do every key as a new file in bpffs.
It's not very scalable, but comparing to sysfs it's better
(resource wise).
If we decide to add bpf syscall command to expose map details we
can provide pretty printer to it in a form of printk-like string
or via some schema passed to syscall, so that keys(file names) can
look properly in bpffs.
The above and other ideas all possible in bpffs, but not possible
in sysfs.

>> In case of persistent maps I imagine unprivileged process would want
>> to use it eventually as well, so this requirement already kills cdev
>> approach for me, since I don't think we ever let unprivileged apps
>> create cdev with syscall.
>
> Hmm, I see. So far the discussion was only about having this for privileged
> users (also in this fs patch). F.e. privileged system daemons could setup
> and distribute progs/maps to consumers, etc (f.e. seccomp and tc case).

It completely makes sense to restrict it to admin today, but design
should not prevent relaxing it in the future.

> When we start lifting this, eBPF maps by its own will become a real kernel
> IPC facility for unprivileged Linux applications (independently whether
> they are connected to an actual eBPF program). Those kernel IPC facilities
> that are anchored in the file system like named pipes and Unix domain
> sockets
> are indicated as such as special files, no?

not everything in unix is a model that should be followed.
af_unix with name[0]!=0 is a bad api that wasn't thought through.
Thankfully Linux improved it with abstract names that don't use
special files.
bpf maps obviously is not an IPC (either pinned or not).

>> sure, then we can force all bpffs to have the same hierarchy and mounted
>> in /sys/kernel/bpf location. That would be the same.
>
> That would imply to have a mount_single() file system (like f.e. tracefs
> and
> securityfs), right?

Probably. I'm not sure whether it should be single fs or we allow
multiple mount points. There are pro and con for both.

> So you'd loose having various mounts in different namespaces. And if you
> allow various mount points, how would that /facilitate/ to an admin to
> identify all eBPF objects/resources currently present in the system?

if it's single mount point there are no issues, but would be nice
to separate users and namespaces somehow.

> Or to an application developer finding possible mount points for his own
> application so that bpf(2) syscall to create these nodes succeeds? Would
> you make mounting also unprivileged? What if various distros have these
> mount points at different locations? What should unprivileged applications
> do to know that they can use these locations for themselves?

mounting is root only of course and having standard location answers
all of these questions.

> Also, since they are only regular files, one can only try and find these
> objects based on their naming schemes, which seems to get a bit odd in case
> this file system also carries (perhaps future?) other regular files that
> are not eBPF map and program-special.

not sure what you meant. If names are given by kernel, there are no
problem finding them. If by user, we'd file attributes like the way
you did with xattr.

>> It feels you're pushing for cdev only because of that potential
>> debugging need. Did you actually face that need? I didn't and
>> don't like to add 'nice to have' feature until real need comes.
>
> I think this discussion arose, because the question of how flexible we are
> in future to extend this facility. Nothing more.

Exactly and cdev style pushes us into the corner of traditional
cdev with ioctl which I don't think is flexible enough.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1251454

On 10/20/2015 02:30 AM, Alexei Starovoitov wrote:
> On 10/19/15 3:17 PM, Daniel Borkmann wrote:
>> On 10/19/2015 10:48 PM, Alexei Starovoitov wrote:
>>> On 10/19/15 1:03 PM, Hannes Frederic Sowa wrote:
>>>>
>>>> I doubt it will stay a lightweight feature as it should not be in the
>>>> responsibility of user space to provide those debug facilities.
>>>
>>> It feels we're talking past each other.
>>> I want to solve 'persistent map' problem.
>>> debugging of maps/progs, hierarchy, etc are all nice to have,
>>> but different issues.
>>
>> Ok, so you are saying that this file system will have *only* regular
>> files that are to be considered nodes for eBPF maps and progs. Nothing
>> else will ever be added to it? Yet, eBPF map and prog nodes are *not*
>> *regular files* to me, this seems odd.
>>
>> As soon as you are starting to add additional folders that contain
>> files dumping additional meta data, etc, you basically end up on a
>> bit of similar concept to sysfs, no?
>
> as we discussed in this thread and earlier during plumbers I think
> it would be good to expose key/values somehow in this fs.
> 'how' is a big question.

Yes, it is a big question, and probably best left to the domain-specific
application itself, which can already dump the map nowadays via bpf(2)
syscall. You can add bindings to various languages to make it available
elsewhere as well.

Or, you have a user space 'bpf' tool that can connect to any map that is
being exposed with whatever model, and have modular pretty printers in
user space somewhere located as shared objects, they could get auto-loaded
in the background. Maps could get an annotation attached as an attribute
during creation that is being exposed somewhere, so it can be mapped to
a pretty printer shared object. This would better be solved in user space
entirely, in my opinion, why should the kernel add complexity for this
when this is so much user-space application specific anyway?

As we all agreed, looking into key/values via shell is a rare event and
not needed most of the times. It comes with it's own problems (f.e. think
of dumping a possible rhashtable map with key/values as files). But even
iff we'd want to stick this into files by all means, fusefs can do this
specific job entirely in user space _plus_ fetching these shared objects
for pretty printers etc, all we need for this is to add this annotation/
mapping attribute somewhere to bpf_maps and that's all it takes.

This question is no doubt independant of the fd pinning mechanism, but as
I said, I don't think sticking this into the kernel is a good idea. Why
would that be the kernel's job?

In the other email, you are mentioning fdinfo. fdinfo can be done for any
map/prog already today by just adding the right .show_fdinfo() callback to
bpf_map_fops and bpf_prog_fops, so we let the anon-inodes that we already
use today to do this job for free and such debugging info can be inspected
through procfs already. This is common practice, f.e. look at timerfd,
signalfd and others.

> But regardless which path we take, sysfs is too rigid.
> For the sake of argument say we do every key as a new file in bpffs.
> It's not very scalable, but comparing to sysfs it's better
> (resource wise).

I doubt this is scaleable at all, no matter if its sysfs or a own custom
fs. How should that work. You have a map with possibly thousands or millions
of entries. Are these files to be generated on the fly like in procfs as
soon as you enter that directory? Or as a one-time snapshot (but then
the user mights want to create various snapshots)? There might be new
map elements as building blocks in the future such as pipes, ring buffers
etc. How are they being dumped as files?

> If we decide to add bpf syscall command to expose map details we
> can provide pretty printer to it in a form of printk-like string
> or via some schema passed to syscall, so that keys(file names) can
> look properly in bpffs.
> The above and other ideas all possible in bpffs, but not possible
> in sysfs.

So, you'd need schemes for keys /and/ values. They could both be complicated
structures and even within these structures, you might need further domain
specific knowledge to dump stuff properly. Perhaps to make sense of it, a
member of that structure needs further processing, etc. That's why I think,
as mentioned above, it's better done in user space through modular shared
objects helpers. A default set of these .so files for pretty printing common
stuff could be shipped already and for more complex applications, they can
ship through distros their own .so files along with the application. And
developers can hack their own modules together locally, too.

So, neither sysfs nor bpffs would need anything here. Why force this into
the kernel? Even more so, if it's just rarely used anyway?

>>> In case of persistent maps I imagine unprivileged process would want
>>> to use it eventually as well, so this requirement already kills cdev
>>> approach for me, since I don't think we ever let unprivileged apps
>>> create cdev with syscall.
>>
>> Hmm, I see. So far the discussion was only about having this for privileged
>> users (also in this fs patch). F.e. privileged system daemons could setup
>> and distribute progs/maps to consumers, etc (f.e. seccomp and tc case).
>
> It completely makes sense to restrict it to admin today, but design
> should not prevent relaxing it in the future.
>
>> When we start lifting this, eBPF maps by its own will become a real kernel
>> IPC facility for unprivileged Linux applications (independently whether
>> they are connected to an actual eBPF program). Those kernel IPC facilities
>> that are anchored in the file system like named pipes and Unix domain
>> sockets
>> are indicated as such as special files, no?
>
> not everything in unix is a model that should be followed.
> af_unix with name[0]!=0 is a bad api that wasn't thought through.
> Thankfully Linux improved it with abstract names that don't use
> special files.
> bpf maps obviously is not an IPC (either pinned or not).

So, if this pinning facility is unprivileged and available for *all*
applications, then applications can in-fact use eBPF maps (w/o any
other aides such as Unix domain sockets to transfer fds) among themselves
to exchange state via bpf(2) syscall. It doesn't need a corresponding
program.

>>> sure, then we can force all bpffs to have the same hierarchy and mounted
>>> in /sys/kernel/bpf location. That would be the same.
>>
>> That would imply to have a mount_single() file system (like f.e. tracefs
>> and
>> securityfs), right?
>
> Probably. I'm not sure whether it should be single fs or we allow
> multiple mount points. There are pro and con for both.
>
>> So you'd loose having various mounts in different namespaces. And if you
>> allow various mount points, how would that /facilitate/ to an admin to
>> identify all eBPF objects/resources currently present in the system?
>
> if it's single mount point there are no issues, but would be nice
> to separate users and namespaces somehow.
>
>> Or to an application developer finding possible mount points for his own
>> application so that bpf(2) syscall to create these nodes succeeds? Would
>> you make mounting also unprivileged? What if various distros have these
>> mount points at different locations? What should unprivileged applications
>> do to know that they can use these locations for themselves?
>
> mounting is root only of course and having standard location answers
> all of these questions.

Okay, sure, but then having a mount_single() and separating users and
namespaces is still not being resolved, as you've noticed.

>> Also, since they are only regular files, one can only try and find these
>> objects based on their naming schemes, which seems to get a bit odd in case
>> this file system also carries (perhaps future?) other regular files that
>> are not eBPF map and program-special.
>
> not sure what you meant. If names are given by kernel, there are no
> problem finding them. If by user, we'd file attributes like the way
> you did with xattr.

So, if you distribute the names through the kernel and dictate a strict
hierarchy, then we'll end up with a similar model that cdevs resolve.

>>> It feels you're pushing for cdev only because of that potential
>>> debugging need. Did you actually face that need? I didn't and
>>> don't like to add 'nice to have' feature until real need comes.
>>
>> I think this discussion arose, because the question of how flexible we are
>> in future to extend this facility. Nothing more.
>
> Exactly and cdev style pushes us into the corner of traditional
> cdev with ioctl which I don't think is flexible enough.

So, as elaborated above in terms of pretty printers and fdinfo examples,
they can all be resolved much more elegantly. So far I still fail to see
the reason why a custom dedicated fs would be better to make the pinning
syscall work, when this can be solved with a better integration into
Linux facilities by using cdevs.

Thanks !
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1251930

On 10/20/15 1:46 AM, Daniel Borkmann wrote:
>> as we discussed in this thread and earlier during plumbers I think
>> it would be good to expose key/values somehow in this fs.
>> 'how' is a big question.
>
> Yes, it is a big question, and probably best left to the domain-specific
> application itself, which can already dump the map nowadays via bpf(2)
> syscall. You can add bindings to various languages to make it available
> elsewhere as well.
>
> Or, you have a user space 'bpf' tool that can connect to any map that is
> being exposed with whatever model, and have modular pretty printers in
> user space somewhere located as shared objects, they could get auto-loaded
> in the background. Maps could get an annotation attached as an attribute
> during creation that is being exposed somewhere, so it can be mapped to
> a pretty printer shared object. This would better be solved in user space
> entirely, in my opinion, why should the kernel add complexity for this
> when this is so much user-space application specific anyway?
>
> As we all agreed, looking into key/values via shell is a rare event and
> not needed most of the times. It comes with it's own problems (f.e. think
> of dumping a possible rhashtable map with key/values as files). But even
> iff we'd want to stick this into files by all means, fusefs can do this
> specific job entirely in user space _plus_ fetching these shared objects
> for pretty printers etc, all we need for this is to add this annotation/
> mapping attribute somewhere to bpf_maps and that's all it takes.
>
> This question is no doubt independant of the fd pinning mechanism, but as
> I said, I don't think sticking this into the kernel is a good idea. Why
> would that be the kernel's job?

agree with all of the concerns above. I said it would be good for
kernel to expose key/values and I still think it would be a useful
feature. Regardless whether kernel does it or not in the future,
the point was 'IF we want kernel to do it then bpf FS is the right way'.

> In the other email, you are mentioning fdinfo. fdinfo can be done for any
> map/prog already today by just adding the right .show_fdinfo() callback to
> bpf_map_fops and bpf_prog_fops, so we let the anon-inodes that we already
> use today to do this job for free and such debugging info can be inspected
> through procfs already. This is common practice, f.e. look at timerfd,
> signalfd and others.

I know. That's exactly what I proposed, but again the point was
that fdinfo of regular FDs should match in style to pinned FDs,
'cat /sys/kernel/bpf/.../map5' should be similar to
'cat /proc/.../fdinfo/5'
and 'cat /sys/kernel/bpf...' you can only cleanly do with bpffs.

>> But regardless which path we take, sysfs is too rigid.
>> For the sake of argument say we do every key as a new file in bpffs.
>> It's not very scalable, but comparing to sysfs it's better
>> (resource wise).
>
> I doubt this is scaleable at all, no matter if its sysfs or a own custom
> fs. How should that work. You have a map with possibly thousands or
> millions
> of entries. Are these files to be generated on the fly like in procfs as
> soon as you enter that directory? Or as a one-time snapshot (but then
> the user mights want to create various snapshots)? There might be new
> map elements as building blocks in the future such as pipes, ring buffers
> etc. How are they being dumped as files?

you're arguing that keys as files are not scalable. sure.
See what I said above "it's not very scalable"
The point is that fs approach is more flexible comparing to cdev.

>> not everything in unix is a model that should be followed.
>> af_unix with name[0]!=0 is a bad api that wasn't thought through.
>> Thankfully Linux improved it with abstract names that don't use
>> special files.
>> bpf maps obviously is not an IPC (either pinned or not).
>
> So, if this pinning facility is unprivileged and available for *all*
> applications, then applications can in-fact use eBPF maps (w/o any
> other aides such as Unix domain sockets to transfer fds) among themselves
> to exchange state via bpf(2) syscall. It doesn't need a corresponding
> program.

Obviously I know that, but it doesn't make it an IPC.
Just because two processes can talk to each other via normal tcpip it
doesn't make tcpip an IPC mechanism.
The point is "just because two processes can communicate with each
other via X (bpf maps) we are not going to optimize (or make
architectural decisions in X) just for this use case". It's a job of
generic IPC and we have enough of them already.

> Okay, sure, but then having a mount_single() and separating users and
> namespaces is still not being resolved, as you've noticed.

yes and that's what I proposed to do:
Tweaking this FS patch to do mount_single() and define directory
structure is the best way forward.

> So, if you distribute the names through the kernel and dictate a strict
> hierarchy, then we'll end up with a similar model that cdevs resolve.

yes. exactly.
but comparing to cdev, it will be:
- cheaper for kernel to keep (memory wise)
- faster to pin FDs
- do normal 'rm' to destroy
- possible to extend to unprivileged users
- possible to add fdinfo (same output for pinned and normal fd)
- possible to expose key/value

I'm puzzled how you can keep arguing in favor of cdev when it's
obviously deficient comparing to fs and fs has no disadvantages.
Looks like we can only resolve it over beer.
How about we setup a public hangout ? Today or tomorrow?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1251969

Alexei Starovoitov <ast@plumgrid.com> writes:

> On 10/20/15 1:46 AM, Daniel Borkmann wrote:
>>> as we discussed in this thread and earlier during plumbers I think
>>> it would be good to expose key/values somehow in this fs.
>>> 'how' is a big question.
>>
>> Yes, it is a big question, and probably best left to the domain-specific
>> application itself, which can already dump the map nowadays via bpf(2)
>> syscall. You can add bindings to various languages to make it available
>> elsewhere as well.
>>
>> Or, you have a user space 'bpf' tool that can connect to any map that is
>> being exposed with whatever model, and have modular pretty printers in
>> user space somewhere located as shared objects, they could get auto-loaded
>> in the background. Maps could get an annotation attached as an attribute
>> during creation that is being exposed somewhere, so it can be mapped to
>> a pretty printer shared object. This would better be solved in user space
>> entirely, in my opinion, why should the kernel add complexity for this
>> when this is so much user-space application specific anyway?
>>
>> As we all agreed, looking into key/values via shell is a rare event and
>> not needed most of the times. It comes with it's own problems (f.e. think
>> of dumping a possible rhashtable map with key/values as files). But even
>> iff we'd want to stick this into files by all means, fusefs can do this
>> specific job entirely in user space _plus_ fetching these shared objects
>> for pretty printers etc, all we need for this is to add this annotation/
>> mapping attribute somewhere to bpf_maps and that's all it takes.
>>
>> This question is no doubt independant of the fd pinning mechanism, but as
>> I said, I don't think sticking this into the kernel is a good idea. Why
>> would that be the kernel's job?
>
> agree with all of the concerns above. I said it would be good for
> kernel to expose key/values and I still think it would be a useful
> feature. Regardless whether kernel does it or not in the future,
> the point was 'IF we want kernel to do it then bpf FS is the right way'.
>
>> In the other email, you are mentioning fdinfo. fdinfo can be done for any
>> map/prog already today by just adding the right .show_fdinfo() callback to
>> bpf_map_fops and bpf_prog_fops, so we let the anon-inodes that we already
>> use today to do this job for free and such debugging info can be inspected
>> through procfs already. This is common practice, f.e. look at timerfd,
>> signalfd and others.
>
> I know. That's exactly what I proposed, but again the point was
> that fdinfo of regular FDs should match in style to pinned FDs,
> 'cat /sys/kernel/bpf/.../map5' should be similar to
> 'cat /proc/.../fdinfo/5'
> and 'cat /sys/kernel/bpf...' you can only cleanly do with bpffs.
>
>>> But regardless which path we take, sysfs is too rigid.
>>> For the sake of argument say we do every key as a new file in bpffs.
>>> It's not very scalable, but comparing to sysfs it's better
>>> (resource wise).
>>
>> I doubt this is scaleable at all, no matter if its sysfs or a own custom
>> fs. How should that work. You have a map with possibly thousands or
>> millions
>> of entries. Are these files to be generated on the fly like in procfs as
>> soon as you enter that directory? Or as a one-time snapshot (but then
>> the user mights want to create various snapshots)? There might be new
>> map elements as building blocks in the future such as pipes, ring buffers
>> etc. How are they being dumped as files?
>
> you're arguing that keys as files are not scalable. sure.
> See what I said above "it's not very scalable"
> The point is that fs approach is more flexible comparing to cdev.
>
>>> not everything in unix is a model that should be followed.
>>> af_unix with name[0]!=0 is a bad api that wasn't thought through.
>>> Thankfully Linux improved it with abstract names that don't use
>>> special files.
>>> bpf maps obviously is not an IPC (either pinned or not).
>>
>> So, if this pinning facility is unprivileged and available for *all*
>> applications, then applications can in-fact use eBPF maps (w/o any
>> other aides such as Unix domain sockets to transfer fds) among themselves
>> to exchange state via bpf(2) syscall. It doesn't need a corresponding
>> program.
>
> Obviously I know that, but it doesn't make it an IPC.
> Just because two processes can talk to each other via normal tcpip it
> doesn't make tcpip an IPC mechanism.
> The point is "just because two processes can communicate with each
> other via X (bpf maps) we are not going to optimize (or make
> architectural decisions in X) just for this use case". It's a job of
> generic IPC and we have enough of them already.
>
>> Okay, sure, but then having a mount_single() and separating users and
>> namespaces is still not being resolved, as you've noticed.
>
> yes and that's what I proposed to do:
> Tweaking this FS patch to do mount_single() and define directory
> structure is the best way forward.
>
>> So, if you distribute the names through the kernel and dictate a strict
>> hierarchy, then we'll end up with a similar model that cdevs resolve.
>
> yes. exactly.
> but comparing to cdev, it will be:
> - cheaper for kernel to keep (memory wise)
> - faster to pin FDs
> - do normal 'rm' to destroy
> - possible to extend to unprivileged users
> - possible to add fdinfo (same output for pinned and normal fd)
> - possible to expose key/value
>
> I'm puzzled how you can keep arguing in favor of cdev when it's
> obviously deficient comparing to fs and fs has no disadvantages.
> Looks like we can only resolve it over beer.
> How about we setup a public hangout ? Today or tomorrow?

Just FYI:  Using a device for this kind of interface is pretty
much a non-starter as that quickly gets you into situations where
things do not work in containers.  If someone gets a version of device
namespaces past GregKH it might be up for discussion to use character
devices.

But really device nodes are a technology that is slowly being changed to
support hotplug.  Nothing you are doing seems to match up well with
devices.  So for an interface that you want ordinary applications to use
character devices are a bad bad fit.

Eric

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

Back to top | Article view | linux.kernel

csiph-web