On 9/5/16 10:09 AM, Daniel Borkmann wrote:
On 09/05/2016 04:09 PM, Daniel Mack wrote:
On 09/05/2016 03:56 PM, Daniel Borkmann wrote:
On 09/05/2016 02:54 PM, Daniel Mack wrote:
On 08/30/2016 01:00 AM, Daniel Borkmann wrote:
On 08/26/2016 09:58 PM, Daniel Mack wrote:
enum bpf_map_type {
@@ -147,6 +149,13 @@ union bpf_attr {
__aligned_u64 pathname;
__u32 bpf_fd;
};
+
+ struct { /* anonymous struct used by BPF_PROG_ATTACH/DETACH
commands */
+ __u32 target_fd; /* container object to attach
to */
+ __u32 attach_bpf_fd; /* eBPF program to attach */
+ __u32 attach_type; /* BPF_ATTACH_TYPE_* */
+ __u64 attach_flags;
Could we just do ...
__u32 dst_fd;
__u32 src_fd;
__u32 attach_type;
... and leave flags out, since unused anyway? Also see below.
I'd really like to keep the flags, even if they're unused right now.
This only adds 8 bytes during the syscall operation, so it doesn't
harm.
However, we cannot change the userspace API after the fact, and who
knows what this (rather generic) interface will be used for later on.
With the below suggestion added, then flags doesn't need to be
added currently as it can be done safely at a later point in time
with respecting old binaries. See also the syscall handling code
in kernel/bpf/syscall.c +825 and the CHECK_ATTR() macro. The
underlying idea of this was taken from perf_event_open() syscall
back then, see [1] for a summary.
[1] https://lkml.org/lkml/2014/8/26/116
Yes, I know that's possible, and I like the idea, but I don't think any
new interface should come without flags really, as flags are something
that will most certainly be needed at some point anyway. I didn't have
them in my first shot, but Alexei pointed out that they should be added,
and I agree.
Also, this optimization wouldn't make the transported struct payload any
smaller anyway, because the member of that union used by BPF_PROG_LOAD
is still by far the biggest.
I really don't think it's worth sparing 8 bytes here and then do the
binary compat dance after flags are added, for no real gain.
Sure, but there's not much of a dance needed, see for example how map_flags
were added some time ago. So, iff there's really no foreseeable use-case in
sight and since we have this flexibility in place already, then I don't
quite
follow why it's needed, if there's zero pain to add it later on. I would
understand it of course, if it cannot be handled later on anymore.
I agree with Daniel B. Since flags are completely unused right now,
there is no plan to use it for anything in the coming months and
even worse they make annoying hole in the struct, let's not
add them. We can safely do that later. CHECK_ATTR() allows us to
do it easily. It's not like syscall where flags are must have,
since we cannot add it later. Here it's done trivially.
