| Message ID | b3f1dd7e21d72dc47beac222d655640ee584c5cd.1447930253.git.daniel@iogearbox.net |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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> -- 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
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 -- 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
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. -- 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
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 ... -- 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
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. -- 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
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 -- 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
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. -- 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
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? -- 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
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. -- 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
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. -- 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
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 -- 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
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. -- 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
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 > -- 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
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 -- 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
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 > -- 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
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)
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(-)