diff mbox series

[bpf-next,v3,5/6] libbpf: Add bpf_get_link_xdp_info() function to get more XDP information

Message ID 157325766011.27401.5278664694085166014.stgit@toke.dk
State Changes Requested
Delegated to: BPF Maintainers
Headers show
Series libbpf: Fix pinning and error message bugs and add new getters | expand

Commit Message

Toke Høiland-Jørgensen Nov. 9, 2019, 12:01 a.m. UTC 
From: Toke Høiland-Jørgensen <toke@redhat.com>

Currently, libbpf only provides a function to get a single ID for the XDP
program attached to the interface. However, it can be useful to get the
full set of program IDs attached, along with the attachment mode, in one
go. Add a new getter function to support this, using an extendible
structure to carry the information. Express the old bpf_get_link_id()
function in terms of the new function.

Acked-by: David S. Miller <davem@davemloft.net>
Acked-by: Song Liu <songliubraving@fb.com>
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 tools/lib/bpf/libbpf.h   |   10 ++++++
 tools/lib/bpf/libbpf.map |    1 +
 tools/lib/bpf/netlink.c  |   82 ++++++++++++++++++++++++++++++----------------
 3 files changed, 65 insertions(+), 28 deletions(-)

Comments

Andrii Nakryiko Nov. 9, 2019, 12:51 a.m. UTC | #1 
On Fri, Nov 8, 2019 at 4:01 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> From: Toke Høiland-Jørgensen <toke@redhat.com>
>
> Currently, libbpf only provides a function to get a single ID for the XDP
> program attached to the interface. However, it can be useful to get the
> full set of program IDs attached, along with the attachment mode, in one
> go. Add a new getter function to support this, using an extendible
> structure to carry the information. Express the old bpf_get_link_id()
> function in terms of the new function.
>
> Acked-by: David S. Miller <davem@davemloft.net>
> Acked-by: Song Liu <songliubraving@fb.com>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---
>  tools/lib/bpf/libbpf.h   |   10 ++++++
>  tools/lib/bpf/libbpf.map |    1 +
>  tools/lib/bpf/netlink.c  |   82 ++++++++++++++++++++++++++++++----------------
>  3 files changed, 65 insertions(+), 28 deletions(-)
>

[...]

>
> -int bpf_get_link_xdp_id(int ifindex, __u32 *prog_id, __u32 flags)
> +int bpf_get_link_xdp_info(int ifindex, struct xdp_link_info *info,
> +                         size_t info_size, __u32 flags)
>  {
>         struct xdp_id_md xdp_id = {};
>         int sock, ret;
>         __u32 nl_pid;
>         __u32 mask;
>
> -       if (flags & ~XDP_FLAGS_MASK)
> +       if (flags & ~XDP_FLAGS_MASK || info_size < sizeof(*info))
>                 return -EINVAL;

Well, now it's backwards-incompatible: older program passes smaller
(but previously perfectly valid) sizeof(struct xdp_link_info) to newer
version of libbpf. This has to go both ways: smaller struct should be
supported as long as program doesn't request (using flags) something,
that can't be put into allowed space.

I know it's PITA to support this, but that's what we have to do for
forward/backward compatibility.

>
>         /* Check whether the single {HW,DRV,SKB} mode is set */
> @@ -274,14 +272,42 @@ int bpf_get_link_xdp_id(int ifindex, __u32 *prog_id, __u32 flags)
>         xdp_id.ifindex = ifindex;
>         xdp_id.flags = flags;
>
> -       ret = libbpf_nl_get_link(sock, nl_pid, get_xdp_id, &xdp_id);
> -       if (!ret)
> -               *prog_id = xdp_id.id;
> +       ret = libbpf_nl_get_link(sock, nl_pid, get_xdp_info, &xdp_id);
> +       if (!ret) {
> +               memset(info, 0, info_size);
> +               memcpy(info, &xdp_id.info, min(info_size, sizeof(xdp_id.info)));

nit: memset above should start at info + min(info_size, sizeof(xdp_id.info))

> +       }
>
>         close(sock);
>         return ret;
>  }
>

[...]
Toke Høiland-Jørgensen Nov. 9, 2019, 11:20 a.m. UTC | #2 
Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

> On Fri, Nov 8, 2019 at 4:01 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> From: Toke Høiland-Jørgensen <toke@redhat.com>
>>
>> Currently, libbpf only provides a function to get a single ID for the XDP
>> program attached to the interface. However, it can be useful to get the
>> full set of program IDs attached, along with the attachment mode, in one
>> go. Add a new getter function to support this, using an extendible
>> structure to carry the information. Express the old bpf_get_link_id()
>> function in terms of the new function.
>>
>> Acked-by: David S. Miller <davem@davemloft.net>
>> Acked-by: Song Liu <songliubraving@fb.com>
>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>> ---
>>  tools/lib/bpf/libbpf.h   |   10 ++++++
>>  tools/lib/bpf/libbpf.map |    1 +
>>  tools/lib/bpf/netlink.c  |   82 ++++++++++++++++++++++++++++++----------------
>>  3 files changed, 65 insertions(+), 28 deletions(-)
>>
>
> [...]
>
>>
>> -int bpf_get_link_xdp_id(int ifindex, __u32 *prog_id, __u32 flags)
>> +int bpf_get_link_xdp_info(int ifindex, struct xdp_link_info *info,
>> +                         size_t info_size, __u32 flags)
>>  {
>>         struct xdp_id_md xdp_id = {};
>>         int sock, ret;
>>         __u32 nl_pid;
>>         __u32 mask;
>>
>> -       if (flags & ~XDP_FLAGS_MASK)
>> +       if (flags & ~XDP_FLAGS_MASK || info_size < sizeof(*info))
>>                 return -EINVAL;
>
> Well, now it's backwards-incompatible: older program passes smaller
> (but previously perfectly valid) sizeof(struct xdp_link_info) to newer
> version of libbpf. This has to go both ways: smaller struct should be
> supported as long as program doesn't request (using flags) something,
> that can't be put into allowed space.

But there's nothing to be backwards-compatible with? I get that *when*
we extend the size of xdp_link_info, we should still accept the old,
smaller size. But in this case that cannot happen as we're only just
introducing this now?

-Toke
Andrii Nakryiko Nov. 9, 2019, 7:10 p.m. UTC | #3 
On Sat, Nov 9, 2019 at 3:20 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>
> > On Fri, Nov 8, 2019 at 4:01 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> >>
> >> From: Toke Høiland-Jørgensen <toke@redhat.com>
> >>
> >> Currently, libbpf only provides a function to get a single ID for the XDP
> >> program attached to the interface. However, it can be useful to get the
> >> full set of program IDs attached, along with the attachment mode, in one
> >> go. Add a new getter function to support this, using an extendible
> >> structure to carry the information. Express the old bpf_get_link_id()
> >> function in terms of the new function.
> >>
> >> Acked-by: David S. Miller <davem@davemloft.net>
> >> Acked-by: Song Liu <songliubraving@fb.com>
> >> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> >> ---
> >>  tools/lib/bpf/libbpf.h   |   10 ++++++
> >>  tools/lib/bpf/libbpf.map |    1 +
> >>  tools/lib/bpf/netlink.c  |   82 ++++++++++++++++++++++++++++++----------------
> >>  3 files changed, 65 insertions(+), 28 deletions(-)
> >>
> >
> > [...]
> >
> >>
> >> -int bpf_get_link_xdp_id(int ifindex, __u32 *prog_id, __u32 flags)
> >> +int bpf_get_link_xdp_info(int ifindex, struct xdp_link_info *info,
> >> +                         size_t info_size, __u32 flags)
> >>  {
> >>         struct xdp_id_md xdp_id = {};
> >>         int sock, ret;
> >>         __u32 nl_pid;
> >>         __u32 mask;
> >>
> >> -       if (flags & ~XDP_FLAGS_MASK)
> >> +       if (flags & ~XDP_FLAGS_MASK || info_size < sizeof(*info))
> >>                 return -EINVAL;
> >
> > Well, now it's backwards-incompatible: older program passes smaller
> > (but previously perfectly valid) sizeof(struct xdp_link_info) to newer
> > version of libbpf. This has to go both ways: smaller struct should be
> > supported as long as program doesn't request (using flags) something,
> > that can't be put into allowed space.
>
> But there's nothing to be backwards-compatible with? I get that *when*
> we extend the size of xdp_link_info, we should still accept the old,
> smaller size. But in this case that cannot happen as we're only just
> introducing this now?

This seems like a shifting burden to next person that will have to
extend this, but ok, fine by me.

>
> -Toke
Toke Høiland-Jørgensen Nov. 9, 2019, 8:18 p.m. UTC | #4 
Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

> On Sat, Nov 9, 2019 at 3:20 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>>
>> > On Fri, Nov 8, 2019 at 4:01 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>> >>
>> >> From: Toke Høiland-Jørgensen <toke@redhat.com>
>> >>
>> >> Currently, libbpf only provides a function to get a single ID for the XDP
>> >> program attached to the interface. However, it can be useful to get the
>> >> full set of program IDs attached, along with the attachment mode, in one
>> >> go. Add a new getter function to support this, using an extendible
>> >> structure to carry the information. Express the old bpf_get_link_id()
>> >> function in terms of the new function.
>> >>
>> >> Acked-by: David S. Miller <davem@davemloft.net>
>> >> Acked-by: Song Liu <songliubraving@fb.com>
>> >> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>> >> ---
>> >>  tools/lib/bpf/libbpf.h   |   10 ++++++
>> >>  tools/lib/bpf/libbpf.map |    1 +
>> >>  tools/lib/bpf/netlink.c  |   82 ++++++++++++++++++++++++++++++----------------
>> >>  3 files changed, 65 insertions(+), 28 deletions(-)
>> >>
>> >
>> > [...]
>> >
>> >>
>> >> -int bpf_get_link_xdp_id(int ifindex, __u32 *prog_id, __u32 flags)
>> >> +int bpf_get_link_xdp_info(int ifindex, struct xdp_link_info *info,
>> >> +                         size_t info_size, __u32 flags)
>> >>  {
>> >>         struct xdp_id_md xdp_id = {};
>> >>         int sock, ret;
>> >>         __u32 nl_pid;
>> >>         __u32 mask;
>> >>
>> >> -       if (flags & ~XDP_FLAGS_MASK)
>> >> +       if (flags & ~XDP_FLAGS_MASK || info_size < sizeof(*info))
>> >>                 return -EINVAL;
>> >
>> > Well, now it's backwards-incompatible: older program passes smaller
>> > (but previously perfectly valid) sizeof(struct xdp_link_info) to newer
>> > version of libbpf. This has to go both ways: smaller struct should be
>> > supported as long as program doesn't request (using flags) something,
>> > that can't be put into allowed space.
>>
>> But there's nothing to be backwards-compatible with? I get that *when*
>> we extend the size of xdp_link_info, we should still accept the old,
>> smaller size. But in this case that cannot happen as we're only just
>> introducing this now?
>
> This seems like a shifting burden to next person that will have to
> extend this, but ok, fine by me.

Well, there's a good chance that this could be myself ;)

However, in this case, since it's just a getter, and we're already doing
size checks on how much data we memcpy back, I suppose that we don't
actually need any minimum size at all, do we (well, apart from a check
for 0)? We can just always copy whatever size the caller passes in, and
they'll just get whatever portion of the struct that happens to be?

-Toke
diff mbox series

Patch

diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index 6ddc0419337b..f0947cc949d2 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -427,8 +427,18 @@  LIBBPF_API int bpf_prog_load_xattr(const struct bpf_prog_load_attr *attr,
 LIBBPF_API int bpf_prog_load(const char *file, enum bpf_prog_type type,
 			     struct bpf_object **pobj, int *prog_fd);
 
+struct xdp_link_info {
+	__u32 prog_id;
+	__u32 drv_prog_id;
+	__u32 hw_prog_id;
+	__u32 skb_prog_id;
+	__u8 attach_mode;
+};
+
 LIBBPF_API int bpf_set_link_xdp_fd(int ifindex, int fd, __u32 flags);
 LIBBPF_API int bpf_get_link_xdp_id(int ifindex, __u32 *prog_id, __u32 flags);
+LIBBPF_API int bpf_get_link_xdp_info(int ifindex, struct xdp_link_info *info,
+				     size_t info_size, __u32 flags);
 
 struct perf_buffer;
 
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index 86173cbb159d..d1a782a3a58d 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -193,6 +193,7 @@  LIBBPF_0.0.5 {
 
 LIBBPF_0.0.6 {
 	global:
+		bpf_get_link_xdp_info;
 		bpf_map__get_pin_path;
 		bpf_map__is_pinned;
 		bpf_map__set_pin_path;
diff --git a/tools/lib/bpf/netlink.c b/tools/lib/bpf/netlink.c
index a261df9cb488..ec206c9e1b3a 100644
--- a/tools/lib/bpf/netlink.c
+++ b/tools/lib/bpf/netlink.c
@@ -25,7 +25,7 @@  typedef int (*__dump_nlmsg_t)(struct nlmsghdr *nlmsg, libbpf_dump_nlmsg_t,
 struct xdp_id_md {
 	int ifindex;
 	__u32 flags;
-	__u32 id;
+	struct xdp_link_info info;
 };
 
 int libbpf_netlink_open(__u32 *nl_pid)
@@ -203,26 +203,11 @@  static int __dump_link_nlmsg(struct nlmsghdr *nlh,
 	return dump_link_nlmsg(cookie, ifi, tb);
 }
 
-static unsigned char get_xdp_id_attr(unsigned char mode, __u32 flags)
-{
-	if (mode != XDP_ATTACHED_MULTI)
-		return IFLA_XDP_PROG_ID;
-	if (flags & XDP_FLAGS_DRV_MODE)
-		return IFLA_XDP_DRV_PROG_ID;
-	if (flags & XDP_FLAGS_HW_MODE)
-		return IFLA_XDP_HW_PROG_ID;
-	if (flags & XDP_FLAGS_SKB_MODE)
-		return IFLA_XDP_SKB_PROG_ID;
-
-	return IFLA_XDP_UNSPEC;
-}
-
-static int get_xdp_id(void *cookie, void *msg, struct nlattr **tb)
+static int get_xdp_info(void *cookie, void *msg, struct nlattr **tb)
 {
 	struct nlattr *xdp_tb[IFLA_XDP_MAX + 1];
 	struct xdp_id_md *xdp_id = cookie;
 	struct ifinfomsg *ifinfo = msg;
-	unsigned char mode, xdp_attr;
 	int ret;
 
 	if (xdp_id->ifindex && xdp_id->ifindex != ifinfo->ifi_index)
@@ -238,27 +223,40 @@  static int get_xdp_id(void *cookie, void *msg, struct nlattr **tb)
 	if (!xdp_tb[IFLA_XDP_ATTACHED])
 		return 0;
 
-	mode = libbpf_nla_getattr_u8(xdp_tb[IFLA_XDP_ATTACHED]);
-	if (mode == XDP_ATTACHED_NONE)
-		return 0;
+	xdp_id->info.attach_mode = libbpf_nla_getattr_u8(
+		xdp_tb[IFLA_XDP_ATTACHED]);
 
-	xdp_attr = get_xdp_id_attr(mode, xdp_id->flags);
-	if (!xdp_attr || !xdp_tb[xdp_attr])
+	if (xdp_id->info.attach_mode == XDP_ATTACHED_NONE)
 		return 0;
 
-	xdp_id->id = libbpf_nla_getattr_u32(xdp_tb[xdp_attr]);
+	if (xdp_tb[IFLA_XDP_PROG_ID])
+		xdp_id->info.prog_id = libbpf_nla_getattr_u32(
+			xdp_tb[IFLA_XDP_PROG_ID]);
+
+	if (xdp_tb[IFLA_XDP_SKB_PROG_ID])
+		xdp_id->info.skb_prog_id = libbpf_nla_getattr_u32(
+			xdp_tb[IFLA_XDP_SKB_PROG_ID]);
+
+	if (xdp_tb[IFLA_XDP_DRV_PROG_ID])
+		xdp_id->info.drv_prog_id = libbpf_nla_getattr_u32(
+			xdp_tb[IFLA_XDP_DRV_PROG_ID]);
+
+	if (xdp_tb[IFLA_XDP_HW_PROG_ID])
+		xdp_id->info.hw_prog_id = libbpf_nla_getattr_u32(
+			xdp_tb[IFLA_XDP_HW_PROG_ID]);
 
 	return 0;
 }
 
-int bpf_get_link_xdp_id(int ifindex, __u32 *prog_id, __u32 flags)
+int bpf_get_link_xdp_info(int ifindex, struct xdp_link_info *info,
+			  size_t info_size, __u32 flags)
 {
 	struct xdp_id_md xdp_id = {};
 	int sock, ret;
 	__u32 nl_pid;
 	__u32 mask;
 
-	if (flags & ~XDP_FLAGS_MASK)
+	if (flags & ~XDP_FLAGS_MASK || info_size < sizeof(*info))
 		return -EINVAL;
 
 	/* Check whether the single {HW,DRV,SKB} mode is set */
@@ -274,14 +272,42 @@  int bpf_get_link_xdp_id(int ifindex, __u32 *prog_id, __u32 flags)
 	xdp_id.ifindex = ifindex;
 	xdp_id.flags = flags;
 
-	ret = libbpf_nl_get_link(sock, nl_pid, get_xdp_id, &xdp_id);
-	if (!ret)
-		*prog_id = xdp_id.id;
+	ret = libbpf_nl_get_link(sock, nl_pid, get_xdp_info, &xdp_id);
+	if (!ret) {
+		memset(info, 0, info_size);
+		memcpy(info, &xdp_id.info, min(info_size, sizeof(xdp_id.info)));
+	}
 
 	close(sock);
 	return ret;
 }
 
+static __u32 get_xdp_id(struct xdp_link_info *info, __u32 flags)
+{
+	if (info->attach_mode != XDP_ATTACHED_MULTI)
+		return info->prog_id;
+	if (flags & XDP_FLAGS_DRV_MODE)
+		return info->drv_prog_id;
+	if (flags & XDP_FLAGS_HW_MODE)
+		return info->hw_prog_id;
+	if (flags & XDP_FLAGS_SKB_MODE)
+		return info->skb_prog_id;
+
+	return 0;
+}
+
+int bpf_get_link_xdp_id(int ifindex, __u32 *prog_id, __u32 flags)
+{
+	struct xdp_link_info info;
+	int ret;
+
+	ret = bpf_get_link_xdp_info(ifindex, &info, sizeof(info), flags);
+	if (!ret)
+		*prog_id = get_xdp_id(&info, flags);
+
+	return ret;
+}
+
 int libbpf_nl_get_link(int sock, unsigned int nl_pid,
 		       libbpf_dump_nlmsg_t dump_link_nlmsg, void *cookie)
 {