diff mbox series

[bpf-next,2/5] libbpf: add helpers for preparing netlink attributes

Message ID 20210325120020.236504-3-memxor@gmail.com (mailing list archive)
State Changes Requested
Delegated to: BPF
Headers show
Series libbpf: Add TC-BPF API | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for bpf-next
netdev/subject_prefix success Link
netdev/cc_maintainers success CCed 13 of 13 maintainers
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 5 this patch: 5
netdev/verify_fixes success Link
netdev/checkpatch warning CHECK: Alignment should match open parenthesis CHECK: Please don't use multiple blank lines WARNING: line length of 100 exceeds 80 columns WARNING: line length of 83 exceeds 80 columns WARNING: line length of 94 exceeds 80 columns
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success Link

Commit Message

Kumar Kartikeya Dwivedi March 25, 2021, noon UTC
This change introduces a few helpers to wrap open coded attribute
preparation in netlink.c.

Every nested attribute's closure must happen using the helper
end_nlattr_nested, which sets its length properly. NLA_F_NESTED is
enforeced using begin_nlattr_nested helper. Other simple attributes
can be added directly.

The maxsz parameter corresponds to the size of the request structure
which is being filled in, so for instance with req being:

struct {
	struct nlmsghdr nh;
	struct tcmsg t;
	char buf[4096];
} req;

Then, maxsz should be sizeof(req).

This change also converts the open coded attribute preparation with the
helpers. Note that the only failure the internal call to add_nlattr
could result in the nested helper would be -EMSGSIZE, hence that is what
we return to our caller.

Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 tools/lib/bpf/netlink.c | 37 +++++++++++++++--------------------
 tools/lib/bpf/nlattr.h  | 43 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 21 deletions(-)

Comments

Andrii Nakryiko March 26, 2021, 11:52 p.m. UTC | #1
On Thu, Mar 25, 2021 at 5:01 AM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> This change introduces a few helpers to wrap open coded attribute
> preparation in netlink.c.
>
> Every nested attribute's closure must happen using the helper
> end_nlattr_nested, which sets its length properly. NLA_F_NESTED is
> enforeced using begin_nlattr_nested helper. Other simple attributes
> can be added directly.
>
> The maxsz parameter corresponds to the size of the request structure
> which is being filled in, so for instance with req being:
>
> struct {
>         struct nlmsghdr nh;
>         struct tcmsg t;
>         char buf[4096];
> } req;
>
> Then, maxsz should be sizeof(req).
>
> This change also converts the open coded attribute preparation with the
> helpers. Note that the only failure the internal call to add_nlattr
> could result in the nested helper would be -EMSGSIZE, hence that is what
> we return to our caller.
>
> Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---
>  tools/lib/bpf/netlink.c | 37 +++++++++++++++--------------------
>  tools/lib/bpf/nlattr.h  | 43 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 59 insertions(+), 21 deletions(-)
>
> diff --git a/tools/lib/bpf/netlink.c b/tools/lib/bpf/netlink.c
> index 4dd73de00b6f..f448c29de76d 100644
> --- a/tools/lib/bpf/netlink.c
> +++ b/tools/lib/bpf/netlink.c
> @@ -135,7 +135,7 @@ static int __bpf_set_link_xdp_fd_replace(int ifindex, int fd, int old_fd,
>                                          __u32 flags)
>  {
>         int sock, seq = 0, ret;
> -       struct nlattr *nla, *nla_xdp;
> +       struct nlattr *nla;
>         struct {
>                 struct nlmsghdr  nh;
>                 struct ifinfomsg ifinfo;
> @@ -157,36 +157,31 @@ static int __bpf_set_link_xdp_fd_replace(int ifindex, int fd, int old_fd,
>         req.ifinfo.ifi_index = ifindex;
>
>         /* started nested attribute for XDP */
> -       nla = (struct nlattr *)(((char *)&req)
> -                               + NLMSG_ALIGN(req.nh.nlmsg_len));
> -       nla->nla_type = NLA_F_NESTED | IFLA_XDP;
> -       nla->nla_len = NLA_HDRLEN;
> +       nla = begin_nlattr_nested(&req.nh, sizeof(req), IFLA_XDP);
> +       if (!nla) {
> +               ret = -EMSGSIZE;
> +               goto cleanup;
> +       }
>
>         /* add XDP fd */
> -       nla_xdp = (struct nlattr *)((char *)nla + nla->nla_len);
> -       nla_xdp->nla_type = IFLA_XDP_FD;
> -       nla_xdp->nla_len = NLA_HDRLEN + sizeof(int);
> -       memcpy((char *)nla_xdp + NLA_HDRLEN, &fd, sizeof(fd));
> -       nla->nla_len += nla_xdp->nla_len;
> +       ret = add_nlattr(&req.nh, sizeof(req), IFLA_XDP_FD, &fd, sizeof(fd));
> +       if (ret < 0)
> +               goto cleanup;
>
>         /* if user passed in any flags, add those too */
>         if (flags) {
> -               nla_xdp = (struct nlattr *)((char *)nla + nla->nla_len);
> -               nla_xdp->nla_type = IFLA_XDP_FLAGS;
> -               nla_xdp->nla_len = NLA_HDRLEN + sizeof(flags);
> -               memcpy((char *)nla_xdp + NLA_HDRLEN, &flags, sizeof(flags));
> -               nla->nla_len += nla_xdp->nla_len;
> +               ret = add_nlattr(&req.nh, sizeof(req), IFLA_XDP_FLAGS, &flags, sizeof(flags));
> +               if (ret < 0)
> +                       goto cleanup;
>         }
>
>         if (flags & XDP_FLAGS_REPLACE) {
> -               nla_xdp = (struct nlattr *)((char *)nla + nla->nla_len);
> -               nla_xdp->nla_type = IFLA_XDP_EXPECTED_FD;
> -               nla_xdp->nla_len = NLA_HDRLEN + sizeof(old_fd);
> -               memcpy((char *)nla_xdp + NLA_HDRLEN, &old_fd, sizeof(old_fd));
> -               nla->nla_len += nla_xdp->nla_len;
> +               ret = add_nlattr(&req.nh, sizeof(req), IFLA_XDP_EXPECTED_FD, &flags, sizeof(flags));
> +               if (ret < 0)
> +                       goto cleanup;
>         }
>
> -       req.nh.nlmsg_len += NLA_ALIGN(nla->nla_len);
> +       end_nlattr_nested(&req.nh, nla);
>
>         if (send(sock, &req, req.nh.nlmsg_len, 0) < 0) {
>                 ret = -errno;
> diff --git a/tools/lib/bpf/nlattr.h b/tools/lib/bpf/nlattr.h
> index 6cc3ac91690f..463a53bf3022 100644
> --- a/tools/lib/bpf/nlattr.h
> +++ b/tools/lib/bpf/nlattr.h
> @@ -10,7 +10,10 @@
>  #define __LIBBPF_NLATTR_H
>
>  #include <stdint.h>
> +#include <string.h>
> +#include <errno.h>
>  #include <linux/netlink.h>
> +
>  /* avoid multiple definition of netlink features */
>  #define __LINUX_NETLINK_H
>
> @@ -103,4 +106,44 @@ int libbpf_nla_parse_nested(struct nlattr *tb[], int maxtype,
>
>  int libbpf_nla_dump_errormsg(struct nlmsghdr *nlh);
>
> +
> +/* Helpers for preparing/consuming attributes */
> +
> +#define NLA_DATA(nla) ((struct nlattr *)((char *)(nla) + NLA_HDRLEN))

`((char *)nh + NLMSG_ALIGN(nh->nlmsg_len))` seems to be another
popular one (three occurrences in this file), maybe extract that one
as well?

And can you please use functions, not macros? This way you can specify
what types you expect, as one of the benefits.

> +
> +static inline int add_nlattr(struct nlmsghdr *nh, size_t maxsz, int type,
> +                            const void *data, int len)
> +{
> +       struct nlattr *nla;
> +
> +       if (NLMSG_ALIGN(nh->nlmsg_len) + NLA_ALIGN(NLA_HDRLEN + len) > maxsz)
> +               return -EMSGSIZE;
> +       if ((!data && len) || (data && !len))
> +               return -EINVAL;
> +
> +       nla = (struct nlattr *)((char *)nh + NLMSG_ALIGN(nh->nlmsg_len));
> +       nla->nla_type = type;
> +       nla->nla_len = NLA_HDRLEN + len;
> +       if (data)
> +               memcpy((char *)nla + NLA_HDRLEN, data, len);
> +       nh->nlmsg_len = NLMSG_ALIGN(nh->nlmsg_len) + NLA_ALIGN(nla->nla_len);
> +       return 0;
> +}
> +
> +static inline struct nlattr *begin_nlattr_nested(struct nlmsghdr *nh, size_t maxsz,
> +                                              int type)
> +{
> +       struct nlattr *tail;
> +
> +       tail = (struct nlattr *)((char *)nh + NLMSG_ALIGN(nh->nlmsg_len));
> +       if (add_nlattr(nh, maxsz, type | NLA_F_NESTED, NULL, 0))
> +               return NULL;
> +       return tail;
> +}
> +
> +static inline void end_nlattr_nested(struct nlmsghdr *nh, struct nlattr *tail)

I don't know much about their use (yet, I feel like I'm about to learn
:( ), but would nlattr_add, nlattr_begin_nested/nlattr_start_nested,
nlattr_end_nested make sense and be a bit more in line with overall
object_action naming pattern?

> +{
> +       tail->nla_len = ((char *)nh + NLMSG_ALIGN(nh->nlmsg_len)) - (char *)(tail);
> +}
> +
>  #endif /* __LIBBPF_NLATTR_H */
> --
> 2.30.2
>
diff mbox series

Patch

diff --git a/tools/lib/bpf/netlink.c b/tools/lib/bpf/netlink.c
index 4dd73de00b6f..f448c29de76d 100644
--- a/tools/lib/bpf/netlink.c
+++ b/tools/lib/bpf/netlink.c
@@ -135,7 +135,7 @@  static int __bpf_set_link_xdp_fd_replace(int ifindex, int fd, int old_fd,
 					 __u32 flags)
 {
 	int sock, seq = 0, ret;
-	struct nlattr *nla, *nla_xdp;
+	struct nlattr *nla;
 	struct {
 		struct nlmsghdr  nh;
 		struct ifinfomsg ifinfo;
@@ -157,36 +157,31 @@  static int __bpf_set_link_xdp_fd_replace(int ifindex, int fd, int old_fd,
 	req.ifinfo.ifi_index = ifindex;
 
 	/* started nested attribute for XDP */
-	nla = (struct nlattr *)(((char *)&req)
-				+ NLMSG_ALIGN(req.nh.nlmsg_len));
-	nla->nla_type = NLA_F_NESTED | IFLA_XDP;
-	nla->nla_len = NLA_HDRLEN;
+	nla = begin_nlattr_nested(&req.nh, sizeof(req), IFLA_XDP);
+	if (!nla) {
+		ret = -EMSGSIZE;
+		goto cleanup;
+	}
 
 	/* add XDP fd */
-	nla_xdp = (struct nlattr *)((char *)nla + nla->nla_len);
-	nla_xdp->nla_type = IFLA_XDP_FD;
-	nla_xdp->nla_len = NLA_HDRLEN + sizeof(int);
-	memcpy((char *)nla_xdp + NLA_HDRLEN, &fd, sizeof(fd));
-	nla->nla_len += nla_xdp->nla_len;
+	ret = add_nlattr(&req.nh, sizeof(req), IFLA_XDP_FD, &fd, sizeof(fd));
+	if (ret < 0)
+		goto cleanup;
 
 	/* if user passed in any flags, add those too */
 	if (flags) {
-		nla_xdp = (struct nlattr *)((char *)nla + nla->nla_len);
-		nla_xdp->nla_type = IFLA_XDP_FLAGS;
-		nla_xdp->nla_len = NLA_HDRLEN + sizeof(flags);
-		memcpy((char *)nla_xdp + NLA_HDRLEN, &flags, sizeof(flags));
-		nla->nla_len += nla_xdp->nla_len;
+		ret = add_nlattr(&req.nh, sizeof(req), IFLA_XDP_FLAGS, &flags, sizeof(flags));
+		if (ret < 0)
+			goto cleanup;
 	}
 
 	if (flags & XDP_FLAGS_REPLACE) {
-		nla_xdp = (struct nlattr *)((char *)nla + nla->nla_len);
-		nla_xdp->nla_type = IFLA_XDP_EXPECTED_FD;
-		nla_xdp->nla_len = NLA_HDRLEN + sizeof(old_fd);
-		memcpy((char *)nla_xdp + NLA_HDRLEN, &old_fd, sizeof(old_fd));
-		nla->nla_len += nla_xdp->nla_len;
+		ret = add_nlattr(&req.nh, sizeof(req), IFLA_XDP_EXPECTED_FD, &flags, sizeof(flags));
+		if (ret < 0)
+			goto cleanup;
 	}
 
-	req.nh.nlmsg_len += NLA_ALIGN(nla->nla_len);
+	end_nlattr_nested(&req.nh, nla);
 
 	if (send(sock, &req, req.nh.nlmsg_len, 0) < 0) {
 		ret = -errno;
diff --git a/tools/lib/bpf/nlattr.h b/tools/lib/bpf/nlattr.h
index 6cc3ac91690f..463a53bf3022 100644
--- a/tools/lib/bpf/nlattr.h
+++ b/tools/lib/bpf/nlattr.h
@@ -10,7 +10,10 @@ 
 #define __LIBBPF_NLATTR_H
 
 #include <stdint.h>
+#include <string.h>
+#include <errno.h>
 #include <linux/netlink.h>
+
 /* avoid multiple definition of netlink features */
 #define __LINUX_NETLINK_H
 
@@ -103,4 +106,44 @@  int libbpf_nla_parse_nested(struct nlattr *tb[], int maxtype,
 
 int libbpf_nla_dump_errormsg(struct nlmsghdr *nlh);
 
+
+/* Helpers for preparing/consuming attributes */
+
+#define NLA_DATA(nla) ((struct nlattr *)((char *)(nla) + NLA_HDRLEN))
+
+static inline int add_nlattr(struct nlmsghdr *nh, size_t maxsz, int type,
+			     const void *data, int len)
+{
+	struct nlattr *nla;
+
+	if (NLMSG_ALIGN(nh->nlmsg_len) + NLA_ALIGN(NLA_HDRLEN + len) > maxsz)
+		return -EMSGSIZE;
+	if ((!data && len) || (data && !len))
+		return -EINVAL;
+
+	nla = (struct nlattr *)((char *)nh + NLMSG_ALIGN(nh->nlmsg_len));
+	nla->nla_type = type;
+	nla->nla_len = NLA_HDRLEN + len;
+	if (data)
+		memcpy((char *)nla + NLA_HDRLEN, data, len);
+	nh->nlmsg_len = NLMSG_ALIGN(nh->nlmsg_len) + NLA_ALIGN(nla->nla_len);
+	return 0;
+}
+
+static inline struct nlattr *begin_nlattr_nested(struct nlmsghdr *nh, size_t maxsz,
+					       int type)
+{
+	struct nlattr *tail;
+
+	tail = (struct nlattr *)((char *)nh + NLMSG_ALIGN(nh->nlmsg_len));
+	if (add_nlattr(nh, maxsz, type | NLA_F_NESTED, NULL, 0))
+		return NULL;
+	return tail;
+}
+
+static inline void end_nlattr_nested(struct nlmsghdr *nh, struct nlattr *tail)
+{
+	tail->nla_len = ((char *)nh + NLMSG_ALIGN(nh->nlmsg_len)) - (char *)(tail);
+}
+
 #endif /* __LIBBPF_NLATTR_H */