[PATCH net-next] bpf: add show_fdinfo handler for maps

[PATCH net-next] bpf: add show_fdinfo handler for maps

[Daniel Borkmann]

Add a handler for show_fdinfo() to be used by the anon-inodes
backend for eBPF maps, and dump the map specification there. Not
only useful for admins, but also it provides a minimal way to
compare specs from ELF vs pinned object.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
 kernel/bpf/syscall.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 0d3313d..6d1407b 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -93,6 +93,23 @@ void bpf_map_put(struct bpf_map *map)
 	}
 }
 
+#ifdef CONFIG_PROC_FS
+static void bpf_map_show_fdinfo(struct seq_file *m, struct file *filp)
+{
+	const struct bpf_map *map = filp->private_data;
+
+	seq_printf(m,
+		   "map_type:\t%u\n"
+		   "key_size:\t%u\n"
+		   "value_size:\t%u\n"
+		   "max_entries:\t%u\n",
+		   map->map_type,
+		   map->key_size,
+		   map->value_size,
+		   map->max_entries);
+}
+#endif
+
 static int bpf_map_release(struct inode *inode, struct file *filp)
 {
 	struct bpf_map *map = filp->private_data;
@@ -108,7 +125,10 @@ static int bpf_map_release(struct inode *inode, struct file *filp)
 }
 
 static const struct file_operations bpf_map_fops = {
-	.release = bpf_map_release,
+#ifdef CONFIG_PROC_FS
+	.show_fdinfo	= bpf_map_show_fdinfo,
+#endif
+	.release	= bpf_map_release,
 };
 
 int bpf_map_new_fd(struct bpf_map *map)
-- 
1.9.3

Re: [PATCH net-next] bpf: add show_fdinfo handler for maps

[Alexei Starovoitov]

On Thu, Nov 19, 2015 at 11:56:22AM +0100, Daniel Borkmann wrote:
> Add a handler for show_fdinfo() to be used by the anon-inodes
> backend for eBPF maps, and dump the map specification there. Not
> only useful for admins, but also it provides a minimal way to
> compare specs from ELF vs pinned object.
> 
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>

Acked-by: Alexei Starovoitov <ast@kernel.org>

Re: [PATCH net-next] bpf: add show_fdinfo handler for maps

[Hannes Frederic Sowa]

On Thu, Nov 19, 2015, at 11:56, Daniel Borkmann wrote:
> Add a handler for show_fdinfo() to be used by the anon-inodes
> backend for eBPF maps, and dump the map specification there. Not
> only useful for admins, but also it provides a minimal way to
> compare specs from ELF vs pinned object.
> 
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>

Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>

Does it make sense to include bpf_htab->count in case of a hashmap?

Thanks,
Hannes

Re: [PATCH net-next] bpf: add show_fdinfo handler for maps

[Alexei Starovoitov]

On Thu, Nov 19, 2015 at 07:19:24PM +0100, Hannes Frederic Sowa wrote:
> On Thu, Nov 19, 2015, at 11:56, Daniel Borkmann wrote:
> > Add a handler for show_fdinfo() to be used by the anon-inodes
> > backend for eBPF maps, and dump the map specification there. Not
> > only useful for admins, but also it provides a minimal way to
> > compare specs from ELF vs pinned object.
> > 
> > Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> 
> Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> 
> Does it make sense to include bpf_htab->count in case of a hashmap?

no. user space should not rely on such things. It can only be misused.

Re: [PATCH net-next] bpf: add show_fdinfo handler for maps

[Hannes Frederic Sowa]

On Thu, Nov 19, 2015, at 19:32, Alexei Starovoitov wrote:
> On Thu, Nov 19, 2015 at 07:19:24PM +0100, Hannes Frederic Sowa wrote:
> > On Thu, Nov 19, 2015, at 11:56, Daniel Borkmann wrote:
> > > Add a handler for show_fdinfo() to be used by the anon-inodes
> > > backend for eBPF maps, and dump the map specification there. Not
> > > only useful for admins, but also it provides a minimal way to
> > > compare specs from ELF vs pinned object.
> > > 
> > > Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> > 
> > Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> > 
> > Does it make sense to include bpf_htab->count in case of a hashmap?
> 
> no. user space should not rely on such things. It can only be misused.

Sorry, I don't get it. How can it be misused? As an admin it would
certainly be interesting to know the pressure on the map? Do you expect
kmsg messages from the eBPF program?

Thanks,
Hannes

Re: [PATCH net-next] bpf: add show_fdinfo handler for maps

[Alexei Starovoitov]

On Thu, Nov 19, 2015 at 09:12:30PM +0100, Hannes Frederic Sowa wrote:
> On Thu, Nov 19, 2015, at 19:32, Alexei Starovoitov wrote:
> > On Thu, Nov 19, 2015 at 07:19:24PM +0100, Hannes Frederic Sowa wrote:
> > > On Thu, Nov 19, 2015, at 11:56, Daniel Borkmann wrote:
> > > > Add a handler for show_fdinfo() to be used by the anon-inodes
> > > > backend for eBPF maps, and dump the map specification there. Not
> > > > only useful for admins, but also it provides a minimal way to
> > > > compare specs from ELF vs pinned object.
> > > > 
> > > > Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> > > 
> > > Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> > > 
> > > Does it make sense to include bpf_htab->count in case of a hashmap?
> > 
> > no. user space should not rely on such things. It can only be misused.
> 
> Sorry, I don't get it. How can it be misused? As an admin it would
> certainly be interesting to know the pressure on the map? Do you expect
> kmsg messages from the eBPF program?

If user space can be see both 'count' and 'max_entries', it can be very
tempting to start assuming 'full' and 'empty' state of the map which will
lead to race conditions and bad design.
bpf programs and maps are inherently multi-thread and concurrent.
If userapp wants to do the counting of elements it needs to do so on its own
and shoot itself in the foot eventually.
For the same reason I don't want to see BPF_MAP_GET_COUNT command.

Re: [PATCH net-next] bpf: add show_fdinfo handler for maps

[Hannes Frederic Sowa]

Hi Alexei,

On Fri, Nov 20, 2015, at 04:30, Alexei Starovoitov wrote:
> On Thu, Nov 19, 2015 at 09:12:30PM +0100, Hannes Frederic Sowa wrote:
> > On Thu, Nov 19, 2015, at 19:32, Alexei Starovoitov wrote:
> > > On Thu, Nov 19, 2015 at 07:19:24PM +0100, Hannes Frederic Sowa wrote:
> > > > On Thu, Nov 19, 2015, at 11:56, Daniel Borkmann wrote:
> > > > > Add a handler for show_fdinfo() to be used by the anon-inodes
> > > > > backend for eBPF maps, and dump the map specification there. Not
> > > > > only useful for admins, but also it provides a minimal way to
> > > > > compare specs from ELF vs pinned object.
> > > > > 
> > > > > Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> > > > 
> > > > Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> > > > 
> > > > Does it make sense to include bpf_htab->count in case of a hashmap?
> > > 
> > > no. user space should not rely on such things. It can only be misused.
> > 
> > Sorry, I don't get it. How can it be misused? As an admin it would
> > certainly be interesting to know the pressure on the map? Do you expect
> > kmsg messages from the eBPF program?
> 
> If user space can be see both 'count' and 'max_entries', it can be very
> tempting to start assuming 'full' and 'empty' state of the map which will
> lead to race conditions and bad design.
> bpf programs and maps are inherently multi-thread and concurrent.
> If userapp wants to do the counting of elements it needs to do so on its
> own
> and shoot itself in the foot eventually.
> For the same reason I don't want to see BPF_MAP_GET_COUNT command.

Hmmm... I don't understand your argument. This is the same with memory
management in general and we still report memory statistics to user
space. I really would find it helpful to have a feeling if a map is
nearly full or nearly empty.

We can also count collisions or the load in the buckets, but some
evidence what is going on would be nice, wouldn't it?

Re: [PATCH net-next] bpf: add show_fdinfo handler for maps

[Alexei Starovoitov]

On Fri, Nov 20, 2015 at 11:30:13AM +0100, Hannes Frederic Sowa wrote:
> Hi Alexei,
> 
> > If user space can be see both 'count' and 'max_entries', it can be very
> > tempting to start assuming 'full' and 'empty' state of the map which will
> > lead to race conditions and bad design.
> > bpf programs and maps are inherently multi-thread and concurrent.
> > If userapp wants to do the counting of elements it needs to do so on its
> > own
> > and shoot itself in the foot eventually.
> > For the same reason I don't want to see BPF_MAP_GET_COUNT command.
> 
> Hmmm... I don't understand your argument. This is the same with memory
> management in general and we still report memory statistics to user
> space. I really would find it helpful to have a feeling if a map is
> nearly full or nearly empty.

memory is not the same, since it's a shared resource and knowledge
about consumption by the process gives no insight whether next malloc()
will succeed or not.

> We can also count collisions or the load in the buckets, but some
> evidence what is going on would be nice, wouldn't it?

reporting collisions may be ok, since it's probably hard to exploit
such stats, but security may become a concern in some use cases,
so may not be such a good idea at the end.

In general when user space passed kernel some numbers (like type,
element size, max_entries) it's ok to report it back via fdinfo.
Anything else I'd rather keep private.
debugging in general should be done by debugging tools.
Like I often use kprobe+bpf to debug networking+bpf :)
Unfortunately kprobe+bpf doesn't work to debug itself, but then
regular tracing and kprobe comes to rescue.

Re: [PATCH net-next] bpf: add show_fdinfo handler for maps

[Hannes Frederic Sowa]

On Sun, Nov 22, 2015, at 00:18, Alexei Starovoitov wrote:
> On Fri, Nov 20, 2015 at 11:30:13AM +0100, Hannes Frederic Sowa wrote:
> > Hi Alexei,
> > 
> > > If user space can be see both 'count' and 'max_entries', it can be very
> > > tempting to start assuming 'full' and 'empty' state of the map which will
> > > lead to race conditions and bad design.
> > > bpf programs and maps are inherently multi-thread and concurrent.
> > > If userapp wants to do the counting of elements it needs to do so on its
> > > own
> > > and shoot itself in the foot eventually.
> > > For the same reason I don't want to see BPF_MAP_GET_COUNT command.
> > 
> > Hmmm... I don't understand your argument. This is the same with memory
> > management in general and we still report memory statistics to user
> > space. I really would find it helpful to have a feeling if a map is
> > nearly full or nearly empty.
> 
> memory is not the same, since it's a shared resource and knowledge
> about consumption by the process gives no insight whether next malloc()
> will succeed or not.
> 
> > We can also count collisions or the load in the buckets, but some
> > evidence what is going on would be nice, wouldn't it?
> 
> reporting collisions may be ok, since it's probably hard to exploit
> such stats, but security may become a concern in some use cases,
> so may not be such a good idea at the end.

Actually, that is the reason why I mentioned it, so *the admin* can see
something is going on. Do you want to protect ebpf from root? Skynet? ;)

> In general when user space passed kernel some numbers (like type,
> element size, max_entries) it's ok to report it back via fdinfo.
> Anything else I'd rather keep private.
> debugging in general should be done by debugging tools.
> Like I often use kprobe+bpf to debug networking+bpf :)
> Unfortunately kprobe+bpf doesn't work to debug itself, but then
> regular tracing and kprobe comes to rescue.

In my opinion the kernel never should hide any information of the admin
if they are accessible easily. Sampling the number of failed updates to
a map or printing it via procfs/ebpffs seems to be just a matter of how
difficult it should be done. The map has a lock, so the number is fairly
accurate. Sampling and plotting size of hash maps without having kprobes
installed would be a nice thing, because it reduces complexity and this
is nice to have.

Bye,
Hannes

Re: [PATCH net-next] bpf: add show_fdinfo handler for maps

[Alexei Starovoitov]

On Mon, Nov 23, 2015 at 05:11:58PM +0100, Hannes Frederic Sowa wrote:
> 
> Actually, that is the reason why I mentioned it, so *the admin* can see
> something is going on. Do you want to protect ebpf from root? Skynet? ;)

correct. To me both root and non-root are users in the first place and
they both shouldn't be allowed to misuse it.

> In my opinion the kernel never should hide any information of the admin
> if they are accessible easily. Sampling the number of failed updates to
> a map or printing it via procfs/ebpffs seems to be just a matter of how
> difficult it should be done. The map has a lock, so the number is fairly

map_lookup is actually lockless. It's a critical path and should be
as fast as possible. No extra stats just for debugging.

> accurate. Sampling and plotting size of hash maps without having kprobes
> installed would be a nice thing, because it reduces complexity and this
> is nice to have.

doing 'cat' from procfs is, of course, easier to use, but it's an extra
code that permenanetly lives in memory, whereas kprobe+bpf is a run-time
debugging.

Re: [PATCH net-next] bpf: add show_fdinfo handler for maps

[John Fastabend]

On 15-11-23 10:03 AM, Alexei Starovoitov wrote:
> On Mon, Nov 23, 2015 at 05:11:58PM +0100, Hannes Frederic Sowa wrote:
>>
>> Actually, that is the reason why I mentioned it, so *the admin* can see
>> something is going on. Do you want to protect ebpf from root? Skynet? ;)
> 
> correct. To me both root and non-root are users in the first place and
> they both shouldn't be allowed to misuse it.
> 
>> In my opinion the kernel never should hide any information of the admin
>> if they are accessible easily. Sampling the number of failed updates to
>> a map or printing it via procfs/ebpffs seems to be just a matter of how
>> difficult it should be done. The map has a lock, so the number is fairly
> 
> map_lookup is actually lockless. It's a critical path and should be
> as fast as possible. No extra stats just for debugging.
> 
>> accurate. Sampling and plotting size of hash maps without having kprobes
>> installed would be a nice thing, because it reduces complexity and this
>> is nice to have.
> 
> doing 'cat' from procfs is, of course, easier to use, but it's an extra
> code that permenanetly lives in memory, whereas kprobe+bpf is a run-time
> debugging.

Hopefully not jumping in off-base here (I've read most the thread), but
what I've been doing is loading programs with debug ebpf code in them
to keep a statistics map(s) and then I read that from  userspace for
stats. It works pretty well and lets me compile out the debug code when
I want and also doesn't need kprobe at all. Also I can implement
sampling so that the debug code only runs every .01% or something like
that so it can be used in "real" systems. My "real" systems are just a
couple node test setup but it seems to be ok ;)

.John

> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH net-next] bpf: add show_fdinfo handler for maps

[Hannes Frederic Sowa]

On Mon, Nov 23, 2015, at 20:09, John Fastabend wrote:
> On 15-11-23 10:03 AM, Alexei Starovoitov wrote:
> > On Mon, Nov 23, 2015 at 05:11:58PM +0100, Hannes Frederic Sowa wrote:
> >>
> >> Actually, that is the reason why I mentioned it, so *the admin* can see
> >> something is going on. Do you want to protect ebpf from root? Skynet? ;)
> > 
> > correct. To me both root and non-root are users in the first place and
> > they both shouldn't be allowed to misuse it.
> > 
> >> In my opinion the kernel never should hide any information of the admin
> >> if they are accessible easily. Sampling the number of failed updates to
> >> a map or printing it via procfs/ebpffs seems to be just a matter of how
> >> difficult it should be done. The map has a lock, so the number is fairly
> > 
> > map_lookup is actually lockless. It's a critical path and should be
> > as fast as possible. No extra stats just for debugging.
> > 
> >> accurate. Sampling and plotting size of hash maps without having kprobes
> >> installed would be a nice thing, because it reduces complexity and this
> >> is nice to have.
> > 
> > doing 'cat' from procfs is, of course, easier to use, but it's an extra
> > code that permenanetly lives in memory, whereas kprobe+bpf is a run-time
> > debugging.
> 
> Hopefully not jumping in off-base here (I've read most the thread), but
> what I've been doing is loading programs with debug ebpf code in them
> to keep a statistics map(s) and then I read that from  userspace for
> stats. It works pretty well and lets me compile out the debug code when
> I want and also doesn't need kprobe at all. Also I can implement
> sampling so that the debug code only runs every .01% or something like
> that so it can be used in "real" systems. My "real" systems are just a
> couple node test setup but it seems to be ok ;)

Ok, I am fine to wait until there is user demand.

Anyway, all you refer to is code you have under your control. I am
worried about bpf code that is not under my control.

Thanks,
Hannes

Re: [PATCH net-next] bpf: add show_fdinfo handler for maps

[John Fastabend]

On 15-11-23 11:12 AM, Hannes Frederic Sowa wrote:
> On Mon, Nov 23, 2015, at 20:09, John Fastabend wrote:
>> On 15-11-23 10:03 AM, Alexei Starovoitov wrote:
>>> On Mon, Nov 23, 2015 at 05:11:58PM +0100, Hannes Frederic Sowa wrote:
>>>>
>>>> Actually, that is the reason why I mentioned it, so *the admin* can see
>>>> something is going on. Do you want to protect ebpf from root? Skynet? ;)
>>>
>>> correct. To me both root and non-root are users in the first place and
>>> they both shouldn't be allowed to misuse it.
>>>
>>>> In my opinion the kernel never should hide any information of the admin
>>>> if they are accessible easily. Sampling the number of failed updates to
>>>> a map or printing it via procfs/ebpffs seems to be just a matter of how
>>>> difficult it should be done. The map has a lock, so the number is fairly
>>>
>>> map_lookup is actually lockless. It's a critical path and should be
>>> as fast as possible. No extra stats just for debugging.
>>>
>>>> accurate. Sampling and plotting size of hash maps without having kprobes
>>>> installed would be a nice thing, because it reduces complexity and this
>>>> is nice to have.
>>>
>>> doing 'cat' from procfs is, of course, easier to use, but it's an extra
>>> code that permenanetly lives in memory, whereas kprobe+bpf is a run-time
>>> debugging.
>>
>> Hopefully not jumping in off-base here (I've read most the thread), but
>> what I've been doing is loading programs with debug ebpf code in them
>> to keep a statistics map(s) and then I read that from  userspace for
>> stats. It works pretty well and lets me compile out the debug code when
>> I want and also doesn't need kprobe at all. Also I can implement
>> sampling so that the debug code only runs every .01% or something like
>> that so it can be used in "real" systems. My "real" systems are just a
>> couple node test setup but it seems to be ok ;)
> 
> Ok, I am fine to wait until there is user demand.
> 
> Anyway, all you refer to is code you have under your control. I am
> worried about bpf code that is not under my control.
> 

Right, I've not gotten this far. To date everything I've been looking at
is owned by the admin. So probably some more use cases there I haven't
looked at.

> Thanks,
> Hannes
> 

Re: [PATCH net-next] bpf: add show_fdinfo handler for maps

[Daniel Borkmann]

On 11/19/2015 07:19 PM, Hannes Frederic Sowa wrote:
> On Thu, Nov 19, 2015, at 11:56, Daniel Borkmann wrote:
>> Add a handler for show_fdinfo() to be used by the anon-inodes
>> backend for eBPF maps, and dump the map specification there. Not
>> only useful for admins, but also it provides a minimal way to
>> compare specs from ELF vs pinned object.
>>
>> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
>
> Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
>
> Does it make sense to include bpf_htab->count in case of a hashmap?

It would give a sort-of only volatile view, e.g. like the map's
refcount. It would be possible to show it, but only as kind of
informative to an admin, have no strong opinion on it, though.

I think this goes rather into the direction of better debugging
interfaces for eBPF, probably makes sense think about it wrt a
general debugging/introspection framework and better dump it
there ...

Re: [PATCH net-next] bpf: add show_fdinfo handler for maps

[Alexei Starovoitov]

On Thu, Nov 19, 2015 at 07:36:10PM +0100, Daniel Borkmann wrote:
> On 11/19/2015 07:19 PM, Hannes Frederic Sowa wrote:
> >On Thu, Nov 19, 2015, at 11:56, Daniel Borkmann wrote:
> >>Add a handler for show_fdinfo() to be used by the anon-inodes
> >>backend for eBPF maps, and dump the map specification there. Not
> >>only useful for admins, but also it provides a minimal way to
> >>compare specs from ELF vs pinned object.
> >>
> >>Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> >
> >Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> >
> >Does it make sense to include bpf_htab->count in case of a hashmap?
> 
> It would give a sort-of only volatile view, e.g. like the map's
> refcount. It would be possible to show it, but only as kind of
> informative to an admin, have no strong opinion on it, though.
> 
> I think this goes rather into the direction of better debugging
> interfaces for eBPF, probably makes sense think about it wrt a
> general debugging/introspection framework and better dump it
> there ...

refcnt to userspace?! nack.
I'm strongly against such 'debugging' that can only be misused
if exposed to user space. Regardless whether it's admin or not.

Re: [PATCH net-next] bpf: add show_fdinfo handler for maps

[David Miller]

From: Daniel Borkmann <daniel@iogearbox.net>
Date: Thu, 19 Nov 2015 11:56:22 +0100

> Add a handler for show_fdinfo() to be used by the anon-inodes
> backend for eBPF maps, and dump the map specification there. Not
> only useful for admins, but also it provides a minimal way to
> compare specs from ELF vs pinned object.
> 
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>

Applied, thanks.