Re: [PATCH bpf-next v2 3/4] libbpf: add low level TC-BPF API
On Tue, Apr 20, 2021 at 02:30:44AM IST, Daniel Borkmann wrote:
> On 4/19/21 2:18 PM, Kumar Kartikeya Dwivedi wrote:
> > This adds functions that wrap the netlink API used for adding,
> > manipulating, and removing traffic control filters. These functions
> > operate directly on the loaded prog's fd, and return a handle to the
> > filter using an out parameter named id.
> >
> > The basic featureset is covered to allow for attaching, manipulation of
> > properties, and removal of filters. Some additional features like
> > TCA_BPF_POLICE and TCA_RATE for tc_cls have been omitted. These can
> > added on top later by extending the bpf_tc_cls_opts struct.
> >
> > Support for binding actions directly to a classifier by passing them in
> > during filter creation has also been omitted for now. These actions have
> > an auto clean up property because their lifetime is bound to the filter
> > they are attached to. This can be added later, but was omitted for now
> > as direct action mode is a better alternative to it, which is enabled by
> > default.
> >
> > An API summary:
> >
> > bpf_tc_act_{attach, change, replace} may be used to attach, change, and
>
> typo on bpf_tc_act_{...} ?
>
Oops, yes. Should be bpf_tc_cls_...
> > replace SCHED_CLS bpf classifier. The protocol field can be set as 0, in
> > which case it is subsitituted as ETH_P_ALL by default.
>
> Do you have an actual user that needs anything other than ETH_P_ALL? Why is it
> even needed? Why not stick to just ETH_P_ALL?
>
Mostly because it was little to no effort to expose this. Though if you feel
strongly about it I can drop the protocol option, and just bake in ETH_P_ALL. It
can always be added later ofcourse, if the need arises in the future.
> > The behavior of the three functions is as follows:
> >
> > attach = create filter if it does not exist, fail otherwise
> > change = change properties of the classifier of existing filter
> > replace = create filter, and replace any existing filter
>
> This touches on tc oddities quite a bit. Why do we need to expose them? Can't
> we
> simplify/abstract this e.g. i) create or update instance, ii) delete instance,
> iii) get instance ? What concrete use case do you have that you need those
> three
> above?
>
'change' is relevant for modifying classifier specific options, and given it's
a lot less useful now as per the current state of this patch, I am fine with
removing it. This is also where the distinction becomes visible to the user, so
removing it should hide the filter/classifier separation.
> > bpf_tc_cls_detach may be used to detach existing SCHED_CLS
> > filter. The bpf_tc_cls_attach_id object filled in during attach,
> > change, or replace must be passed in to the detach functions for them to
> > remove the filter and its attached classififer correctly.
> >
> > bpf_tc_cls_get_info is a helper that can be used to obtain attributes
> > for the filter and classififer. The opts structure may be used to
> > choose the granularity of search, such that info for a specific filter
> > corresponding to the same loaded bpf program can be obtained. By
> > default, the first match is returned to the user.
> >
> > Examples:
> >
> > struct bpf_tc_cls_attach_id id = {};
> > struct bpf_object *obj;
> > struct bpf_program *p;
> > int fd, r;
> >
> > obj = bpf_object_open("foo.o");
> > if (IS_ERR_OR_NULL(obj))
> > return PTR_ERR(obj);
> >
> > p = bpf_object__find_program_by_title(obj, "classifier");
> > if (IS_ERR_OR_NULL(p))
> > return PTR_ERR(p);
> >
> > if (bpf_object__load(obj) < 0)
> > return -1;
> >
> > fd = bpf_program__fd(p);
> >
> > r = bpf_tc_cls_attach(fd, if_nametoindex("lo"),
> > BPF_TC_CLSACT_INGRESS,
> > NULL, );
> > if (r < 0)
> > return r;
> >
> > ... which is roughly equivalent to (after clsact qdisc setup):
> ># tc filter add dev lo ingress bpf obj foo.o sec classifier da
> >
> > ... as direct action mode is always enabled.
> >
> > If a user wishes to modify existing options on an attached classifier,
> > bpf_tc_cls_change API may be used.
> >
> > Only parameters class_id can be modified, the rest are filled in to
> > identify the correct filter. protocol can be left out if it was not
> > chosen explicitly (defaulting to ETH_P_ALL).
> >
> > Example:
> >
> > /* Optional parameters necessary to
[PATCH bpf-next v2 4/4] libbpf: add selftests for TC-BPF API
This adds some basic tests for the low level bpf_tc_cls_* API.
Reviewed-by: Toke Høiland-Jørgensen
Signed-off-by: Kumar Kartikeya Dwivedi
---
.../selftests/bpf/prog_tests/test_tc_bpf.c| 112 ++
.../selftests/bpf/progs/test_tc_bpf_kern.c| 12 ++
2 files changed, 124 insertions(+)
create mode 100644 tools/testing/selftests/bpf/prog_tests/test_tc_bpf.c
create mode 100644 tools/testing/selftests/bpf/progs/test_tc_bpf_kern.c
diff --git a/tools/testing/selftests/bpf/prog_tests/test_tc_bpf.c
b/tools/testing/selftests/bpf/prog_tests/test_tc_bpf.c
new file mode 100644
index ..945f3a1a72f8
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/test_tc_bpf.c
@@ -0,0 +1,112 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define LO_IFINDEX 1
+
+static int test_tc_cls_internal(int fd, __u32 parent_id)
+{
+ DECLARE_LIBBPF_OPTS(bpf_tc_cls_opts, opts, .handle = 1, .priority = 10,
+ .class_id = TC_H_MAKE(1UL << 16, 1),
+ .chain_index = 5);
+ struct bpf_tc_cls_attach_id id = {};
+ struct bpf_tc_cls_info info = {};
+ int ret;
+
+ ret = bpf_tc_cls_attach(fd, LO_IFINDEX, parent_id, , );
+ if (CHECK_FAIL(ret < 0))
+ return ret;
+
+ ret = bpf_tc_cls_get_info(fd, LO_IFINDEX, parent_id, NULL, );
+ if (CHECK_FAIL(ret < 0))
+ goto end;
+
+ ret = -1;
+
+ if (CHECK_FAIL(info.id.handle != id.handle) ||
+ CHECK_FAIL(info.id.chain_index != id.chain_index) ||
+ CHECK_FAIL(info.id.priority != id.priority) ||
+ CHECK_FAIL(info.id.handle != 1) ||
+ CHECK_FAIL(info.id.priority != 10) ||
+ CHECK_FAIL(info.class_id != TC_H_MAKE(1UL << 16, 1)) ||
+ CHECK_FAIL(info.id.chain_index != 5))
+ goto end;
+
+ ret = bpf_tc_cls_replace(fd, LO_IFINDEX, parent_id, , );
+ if (CHECK_FAIL(ret < 0))
+ return ret;
+
+ if (CHECK_FAIL(info.id.handle != 1) ||
+ CHECK_FAIL(info.id.priority != 10) ||
+ CHECK_FAIL(info.class_id != TC_H_MAKE(1UL << 16, 1)))
+ goto end;
+
+ /* Demonstrate changing attributes */
+ opts.class_id = TC_H_MAKE(1UL << 16, 2);
+
+ ret = bpf_tc_cls_change(fd, LO_IFINDEX, parent_id, , );
+ if (CHECK_FAIL(ret < 0))
+ goto end;
+
+ ret = bpf_tc_cls_get_info(fd, LO_IFINDEX, parent_id, NULL, );
+ if (CHECK_FAIL(ret < 0))
+ goto end;
+
+ if (CHECK_FAIL(info.class_id != TC_H_MAKE(1UL << 16, 2)))
+ goto end;
+ if (CHECK_FAIL((info.bpf_flags & TCA_BPF_FLAG_ACT_DIRECT) != 1))
+ goto end;
+
+end:
+ ret = bpf_tc_cls_detach(LO_IFINDEX, parent_id, );
+ CHECK_FAIL(ret < 0);
+ return ret;
+}
+
+void test_test_tc_bpf(void)
+{
+ const char *file = "./test_tc_bpf_kern.o";
+ struct bpf_program *clsp;
+ struct bpf_object *obj;
+ int cls_fd, ret;
+
+ obj = bpf_object__open(file);
+ if (CHECK_FAIL(IS_ERR_OR_NULL(obj)))
+ return;
+
+ clsp = bpf_object__find_program_by_title(obj, "classifier");
+ if (CHECK_FAIL(IS_ERR_OR_NULL(clsp)))
+ goto end;
+
+ ret = bpf_object__load(obj);
+ if (CHECK_FAIL(ret < 0))
+ goto end;
+
+ cls_fd = bpf_program__fd(clsp);
+
+ system("tc qdisc del dev lo clsact");
+
+ ret = test_tc_cls_internal(cls_fd, BPF_TC_CLSACT_INGRESS);
+ if (CHECK_FAIL(ret < 0))
+ goto end;
+
+ if (CHECK_FAIL(system("tc qdisc del dev lo clsact")))
+ goto end;
+
+ ret = test_tc_cls_internal(cls_fd, BPF_TC_CLSACT_EGRESS);
+ if (CHECK_FAIL(ret < 0))
+ goto end;
+
+ CHECK_FAIL(system("tc qdisc del dev lo clsact"));
+
+end:
+ bpf_object__close(obj);
+}
diff --git a/tools/testing/selftests/bpf/progs/test_tc_bpf_kern.c
b/tools/testing/selftests/bpf/progs/test_tc_bpf_kern.c
new file mode 100644
index ..3dd40e21af8e
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_tc_bpf_kern.c
@@ -0,0 +1,12 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include
+#include
+
+// Dummy prog to test TC-BPF API
+
+SEC("classifier")
+int cls(struct __sk_buff *skb)
+{
+ return 0;
+}
--
2.30.2
[PATCH bpf-next v2 3/4] libbpf: add low level TC-BPF API
This adds functions that wrap the netlink API used for adding,
manipulating, and removing traffic control filters. These functions
operate directly on the loaded prog's fd, and return a handle to the
filter using an out parameter named id.
The basic featureset is covered to allow for attaching, manipulation of
properties, and removal of filters. Some additional features like
TCA_BPF_POLICE and TCA_RATE for tc_cls have been omitted. These can
added on top later by extending the bpf_tc_cls_opts struct.
Support for binding actions directly to a classifier by passing them in
during filter creation has also been omitted for now. These actions have
an auto clean up property because their lifetime is bound to the filter
they are attached to. This can be added later, but was omitted for now
as direct action mode is a better alternative to it, which is enabled by
default.
An API summary:
bpf_tc_act_{attach, change, replace} may be used to attach, change, and
replace SCHED_CLS bpf classifier. The protocol field can be set as 0, in
which case it is subsitituted as ETH_P_ALL by default.
The behavior of the three functions is as follows:
attach = create filter if it does not exist, fail otherwise
change = change properties of the classifier of existing filter
replace = create filter, and replace any existing filter
bpf_tc_cls_detach may be used to detach existing SCHED_CLS
filter. The bpf_tc_cls_attach_id object filled in during attach,
change, or replace must be passed in to the detach functions for them to
remove the filter and its attached classififer correctly.
bpf_tc_cls_get_info is a helper that can be used to obtain attributes
for the filter and classififer. The opts structure may be used to
choose the granularity of search, such that info for a specific filter
corresponding to the same loaded bpf program can be obtained. By
default, the first match is returned to the user.
Examples:
struct bpf_tc_cls_attach_id id = {};
struct bpf_object *obj;
struct bpf_program *p;
int fd, r;
obj = bpf_object_open("foo.o");
if (IS_ERR_OR_NULL(obj))
return PTR_ERR(obj);
p = bpf_object__find_program_by_title(obj, "classifier");
if (IS_ERR_OR_NULL(p))
return PTR_ERR(p);
if (bpf_object__load(obj) < 0)
return -1;
fd = bpf_program__fd(p);
r = bpf_tc_cls_attach(fd, if_nametoindex("lo"),
BPF_TC_CLSACT_INGRESS,
NULL, );
if (r < 0)
return r;
... which is roughly equivalent to (after clsact qdisc setup):
# tc filter add dev lo ingress bpf obj foo.o sec classifier da
... as direct action mode is always enabled.
If a user wishes to modify existing options on an attached classifier,
bpf_tc_cls_change API may be used.
Only parameters class_id can be modified, the rest are filled in to
identify the correct filter. protocol can be left out if it was not
chosen explicitly (defaulting to ETH_P_ALL).
Example:
/* Optional parameters necessary to select the right filter */
DECLARE_LIBBPF_OPTS(bpf_tc_cls_opts, opts,
.handle = id.handle,
.priority = id.priority,
.chain_index = id.chain_index)
opts.class_id = TC_H_MAKE(1UL << 16, 12);
r = bpf_tc_cls_change(fd, if_nametoindex("lo"),
BPF_TC_CLSACT_INGRESS,
, );
if (r < 0)
return r;
struct bpf_tc_cls_info info = {};
r = bpf_tc_cls_get_info(fd, if_nametoindex("lo"),
BPF_TC_CLSACT_INGRESS,
, );
if (r < 0)
return r;
assert(info.class_id == TC_H_MAKE(1UL << 16, 12));
This would be roughly equivalent to doing:
# tc filter change dev lo egress prio handle bpf obj foo.o sec \
classifier classid 1:12
... except a new bpf program will be loaded and replace existing one.
Reviewed-by: Toke Høiland-Jørgensen
Signed-off-by: Kumar Kartikeya Dwivedi
---
tools/lib/bpf/libbpf.h | 52 ++
tools/lib/bpf/libbpf.map | 5 +
tools/lib/bpf/netlink.c | 377 ++-
3 files changed, 428 insertions(+), 6 deletions(-)
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index bec4e6a6e31d..2f4a2036cb74 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -16,6 +16,9 @@
#include
#include // for size_t
#include
+#include
+#include
+#include
#include "libbpf_common.h"
@@ -775,6 +778,55 @@ LIBBPF_API int bpf_linker__add_file(struct bpf_linker
*linker, const char *filen
LIBBPF_API int bpf_linker__finalize(struct bpf_linker *linker);
LIBBPF_API void bpf_linker__free(struct bpf_linker *linker);
+/* Conven
[PATCH bpf-next v2 2/4] libbpf: add helpers for preparing netlink attributes
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
nlattr_end_nested, which sets its length properly. NLA_F_NESTED is
enforeced using nlattr_begin_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 nlattr_add
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
Signed-off-by: Kumar Kartikeya Dwivedi
---
tools/lib/bpf/netlink.c | 37 ++-
tools/lib/bpf/nlattr.h | 48 +
2 files changed, 64 insertions(+), 21 deletions(-)
diff --git a/tools/lib/bpf/netlink.c b/tools/lib/bpf/netlink.c
index d2cb28e9ef52..c79e30484e81 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 *))
- + NLMSG_ALIGN(req.nh.nlmsg_len));
- nla->nla_type = NLA_F_NESTED | IFLA_XDP;
- nla->nla_len = NLA_HDRLEN;
+ nla = nlattr_begin_nested(, 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, , sizeof(fd));
- nla->nla_len += nla_xdp->nla_len;
+ ret = nlattr_add(, sizeof(req), IFLA_XDP_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, , sizeof(flags));
- nla->nla_len += nla_xdp->nla_len;
+ ret = nlattr_add(, sizeof(req), IFLA_XDP_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, _fd, sizeof(old_fd));
- nla->nla_len += nla_xdp->nla_len;
+ ret = nlattr_add(, sizeof(req), IFLA_XDP_EXPECTED_FD,
, sizeof(flags));
+ if (ret < 0)
+ goto cleanup;
}
- req.nh.nlmsg_len += NLA_ALIGN(nla->nla_len);
+ nlattr_end_nested(, nla);
if (send(sock, , req.nh.nlmsg_len, 0) < 0) {
ret = -errno;
diff --git a/tools/lib/bpf/nlattr.h b/tools/lib/bpf/nlattr.h
index 6cc3ac91690f..1c94cdb6e89d 100644
--- a/tools/lib/bpf/nlattr.h
+++ b/tools/lib/bpf/nlattr.h
@@ -10,7 +10,10 @@
#define __LIBBPF_NLATTR_H
#include
+#include
+#include
#include
+
/* avoid multiple definition of netlink features */
#define __LINUX_NETLINK_H
@@ -103,4 +106,49 @@ int libbpf_nla_parse_nested(struct nlattr *tb[], int
maxtype,
int libbpf_nla_dump_errormsg(struct nlmsghdr *nlh);
+static inline struct nlattr *nla_data(struct nlattr *nla)
+{
+ return (struct nlattr *)((char *)nla + NLA_HDRLEN);
+}
+
+static inline struct nlattr *nh_tail(struct nlmsghdr *nh)
+{
+ return (struct nlattr *)((char *)nh + NLMSG_ALIGN(nh->nlmsg_len));
+}
+
+static inline int nlattr_add(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 = nh_ta
[PATCH bpf-next v2 1/4] tools: pkt_cls.h: sync with kernel sources
Update the header file so we can use the new defines in subsequent
patches. Also make sure they remain up to date.
Reviewed-by: Toke Høiland-Jørgensen
Signed-off-by: Kumar Kartikeya Dwivedi
---
tools/include/uapi/linux/pkt_cls.h | 174 -
tools/lib/bpf/Makefile | 3 +
2 files changed, 173 insertions(+), 4 deletions(-)
diff --git a/tools/include/uapi/linux/pkt_cls.h
b/tools/include/uapi/linux/pkt_cls.h
index 12153771396a..025c40fef93d 100644
--- a/tools/include/uapi/linux/pkt_cls.h
+++ b/tools/include/uapi/linux/pkt_cls.h
@@ -16,9 +16,36 @@ enum {
TCA_ACT_STATS,
TCA_ACT_PAD,
TCA_ACT_COOKIE,
+ TCA_ACT_FLAGS,
+ TCA_ACT_HW_STATS,
+ TCA_ACT_USED_HW_STATS,
__TCA_ACT_MAX
};
+#define TCA_ACT_FLAGS_NO_PERCPU_STATS 1 /* Don't use percpu allocator for
+* actions stats.
+*/
+
+/* tca HW stats type
+ * When user does not pass the attribute, he does not care.
+ * It is the same as if he would pass the attribute with
+ * all supported bits set.
+ * In case no bits are set, user is not interested in getting any HW
statistics.
+ */
+#define TCA_ACT_HW_STATS_IMMEDIATE (1 << 0) /* Means that in dump, user
+* gets the current HW stats
+* state from the device
+* queried at the dump time.
+*/
+#define TCA_ACT_HW_STATS_DELAYED (1 << 1) /* Means that in dump, user gets
+ * HW stats that might be out of date
+ * for some time, maybe couple of
+ * seconds. This is the case when
+ * driver polls stats updates
+ * periodically or when it gets async
+ * stats update from the device.
+ */
+
#define TCA_ACT_MAX __TCA_ACT_MAX
#define TCA_OLD_COMPAT (TCA_ACT_MAX+1)
#define TCA_ACT_MAX_PRIO 32
@@ -63,12 +90,53 @@ enum {
#define TC_ACT_GOTO_CHAIN __TC_ACT_EXT(2)
#define TC_ACT_EXT_OPCODE_MAX TC_ACT_GOTO_CHAIN
+/* These macros are put here for binary compatibility with userspace apps that
+ * make use of them. For kernel code and new userspace apps, use the TCA_ID_*
+ * versions.
+ */
+#define TCA_ACT_GACT 5
+#define TCA_ACT_IPT 6
+#define TCA_ACT_PEDIT 7
+#define TCA_ACT_MIRRED 8
+#define TCA_ACT_NAT 9
+#define TCA_ACT_XT 10
+#define TCA_ACT_SKBEDIT 11
+#define TCA_ACT_VLAN 12
+#define TCA_ACT_BPF 13
+#define TCA_ACT_CONNMARK 14
+#define TCA_ACT_SKBMOD 15
+#define TCA_ACT_CSUM 16
+#define TCA_ACT_TUNNEL_KEY 17
+#define TCA_ACT_SIMP 22
+#define TCA_ACT_IFE 25
+#define TCA_ACT_SAMPLE 26
+
/* Action type identifiers*/
-enum {
- TCA_ID_UNSPEC=0,
- TCA_ID_POLICE=1,
+enum tca_id {
+ TCA_ID_UNSPEC = 0,
+ TCA_ID_POLICE = 1,
+ TCA_ID_GACT = TCA_ACT_GACT,
+ TCA_ID_IPT = TCA_ACT_IPT,
+ TCA_ID_PEDIT = TCA_ACT_PEDIT,
+ TCA_ID_MIRRED = TCA_ACT_MIRRED,
+ TCA_ID_NAT = TCA_ACT_NAT,
+ TCA_ID_XT = TCA_ACT_XT,
+ TCA_ID_SKBEDIT = TCA_ACT_SKBEDIT,
+ TCA_ID_VLAN = TCA_ACT_VLAN,
+ TCA_ID_BPF = TCA_ACT_BPF,
+ TCA_ID_CONNMARK = TCA_ACT_CONNMARK,
+ TCA_ID_SKBMOD = TCA_ACT_SKBMOD,
+ TCA_ID_CSUM = TCA_ACT_CSUM,
+ TCA_ID_TUNNEL_KEY = TCA_ACT_TUNNEL_KEY,
+ TCA_ID_SIMP = TCA_ACT_SIMP,
+ TCA_ID_IFE = TCA_ACT_IFE,
+ TCA_ID_SAMPLE = TCA_ACT_SAMPLE,
+ TCA_ID_CTINFO,
+ TCA_ID_MPLS,
+ TCA_ID_CT,
+ TCA_ID_GATE,
/* other actions go here */
- __TCA_ID_MAX=255
+ __TCA_ID_MAX = 255
};
#define TCA_ID_MAX __TCA_ID_MAX
@@ -120,6 +188,10 @@ enum {
TCA_POLICE_RESULT,
TCA_POLICE_TM,
TCA_POLICE_PAD,
+ TCA_POLICE_RATE64,
+ TCA_POLICE_PEAKRATE64,
+ TCA_POLICE_PKTRATE64,
+ TCA_POLICE_PKTBURST64,
__TCA_POLICE_MAX
#define TCA_POLICE_RESULT TCA_POLICE_RESULT
};
@@ -333,12 +405,19 @@ enum {
/* Basic filter */
+struct tc_basic_pcnt {
+ __u64 rcnt;
+ __u64 rhit;
+};
+
enum {
TCA_BASIC_UNSPEC,
TCA_BASIC_CLASSID,
TCA_BASIC_EMATCHES,
TCA_BASIC_ACT,
TCA_BASIC_POLICE,
+ TCA_BASIC_PCNT,
+ TCA_BASIC_PAD,
__TCA_BASIC_MAX
};
@@ -485,17 +564,54 @@ enum {
TCA_FLOWER_IN_HW_COUNT,
+ TCA_FLOWER_KEY_PORT_SRC_MIN,/* be16 */
+ TCA_FLOWER_KEY_PORT_SRC_MAX,/* be16 */
+ TCA_FLOWER_KEY_PORT_DST_MIN,/* be16 */
+ TCA_FLOWER_KEY_PORT_DST_MAX,/* be16 */
+
+ TCA_FLOWER_KEY_CT_STATE,/* u16 */
+ TCA_FLOWER_KEY_CT_STATE_MASK, /* u16 */
+ TCA_FLOWER_KEY_CT_ZONE,
[PATCH bpf-next v2 0/4] Add TC-BPF API
This is the second version of the TC-BPF series. It adds a simple API that uses netlink to attach the tc filter and its bpf classifier. Currently, a user needs to shell out to the tc command line to be able to create filters and attach SCHED_CLS programs as classifiers. With the help of this API, it will be possible to use libbpf for doing all parts of bpf program setup and attach. Direct action is now the default, and currently no way to disable it has been provided. This also means that SCHED_ACT programs are a lot less useful, so support for them has been removed for now. In the future, if someone comes up with a convincing use case where the direct action mode doesn't serve their needs, a simple extension that allows disabling direct action mode and passing the list of actions that would be bound to the classifier can be added. In an effort to keep discussion focused, this series doesn't have the high level TC-BPF API. It was clear that there is a need for a bpf_link API in the kernel, hence that will be submitted as a separate patchset. The individual commit messages contain more details, and also a brief summary of the API. Changelog: -- v1 -> v2 v1: https://lore.kernel.org/bpf/20210325120020.236504-1-mem...@gmail.com * netlink helpers have been renamed to object_action style. * attach_id now only contains attributes that are not explicitly set. Only the bare minimum info is kept in it. * protocol is now an optional and defaults to ETH_P_ALL. * direct-action mode is default, and cannot be unset for now. * skip_sw and skip_hw options have also been removed. * bpf_tc_cls_info struct now also returns the bpf program tag and id, as available in the netlink response. This came up as a requirement during discussion with people wanting to use this functionality. * support for attaching SCHED_ACT programs has been dropped, as it isn't useful without any support for binding loaded actions to a classifier. * the distinction between dev and block API has been dropped, there is now a single set of functions and user has to pass the special ifindex value to indicate operation on a shared filter block on their own. * The high level API returning a bpf_link is gone. This was already non- functional for pinning and typical ownership semantics. Instead, a separate patchset will be sent adding a bpf_link API for attaching SCHED_CLS progs to the kernel, and its corresponding libbpf API. * The clsact qdisc is now setup automatically in a best-effort fashion whenever user passes in the clsact ingress or egress parent id. This is done with exclusive mode, such that if an ingress or clsact qdisc is already set up, we skip the setup and move on with filter creation. * Other minor changes that came up during the course of discussion and rework. Kumar Kartikeya Dwivedi (4): tools: pkt_cls.h: sync with kernel sources libbpf: add helpers for preparing netlink attributes libbpf: add low level TC-BPF API libbpf: add selftests for TC-BPF API tools/include/uapi/linux/pkt_cls.h| 174 +++- tools/lib/bpf/Makefile| 3 + tools/lib/bpf/libbpf.h| 52 +++ tools/lib/bpf/libbpf.map | 5 + tools/lib/bpf/netlink.c | 414 -- tools/lib/bpf/nlattr.h| 48 ++ .../selftests/bpf/prog_tests/test_tc_bpf.c| 112 + .../selftests/bpf/progs/test_tc_bpf_kern.c| 12 + 8 files changed, 789 insertions(+), 31 deletions(-) create mode 100644 tools/testing/selftests/bpf/prog_tests/test_tc_bpf.c create mode 100644 tools/testing/selftests/bpf/progs/test_tc_bpf_kern.c -- 2.30.2
Re: [PATCH bpf-next 3/5] libbpf: add low level TC-BPF API
On Mon, Apr 05, 2021 at 10:51:09PM IST, Andrii Nakryiko wrote: > > [...] > > if _block variant is just a special ifindex value, then it should be > fine for users to know such a detail (we can leave a comment > mentioning this specifically), especially given it's not a very > popular thing. Almost doubling amount of APIs just for this doesn't > make much sense, IMO. > Ok. > > If we know that we need variant with options, I'd vote for having just > one bpf_tc_attach() API which always takes options. Passing NULL for > opts is simple, no need for two APIs, I think. > Ack. > > Which parts of that id struct is the data that caller might not know > or can't know? Is it handle and chain_index? Or just one of them? > Or?... If there is something that has to be returned back, I'd keep > only that, instead of returning 6+ fields, most of which user should > already know. > The user will know ifindex and parent_id, and perhaps protocol (it would be ETH_P_ALL if they don't supply one by default). Other fields like handle, priority and chain_index can all be kernel assigned, so keeping those still makes sense. I'll change this in v2. -- Kartikeya
Re: [PATCH bpf-next 3/5] libbpf: add low level TC-BPF API
On Sat, Apr 03, 2021 at 12:02:14AM IST, Alexei Starovoitov wrote:
> On Fri, Apr 2, 2021 at 8:27 AM Kumar Kartikeya Dwivedi
> wrote:
> > [...]
>
> All of these things are messy because of tc legacy. bpf tried to follow tc
> style
> with cls and act distinction and it didn't quite work. cls with
> direct-action is the only
> thing that became mainstream while tc style attach wasn't really addressed.
> There were several incidents where tc had tens of thousands of progs attached
> because of this attach/query/index weirdness described above.
> I think the only way to address this properly is to introduce bpf_link style
> of
> attaching to tc. Such bpf_link would support ingress/egress only.
> direction-action will be implied. There won't be any index and query
> will be obvious.
Note that we already have bpf_link support working (without support for pinning
ofcourse) in a limited way. The ifindex, protocol, parent_id, priority, handle,
chain_index tuple uniquely identifies a filter, so we stash this in the bpf_link
and are able to operate on the exact filter during release.
> So I would like to propose to take this patch set a step further from
> what Daniel said:
> int bpf_tc_attach(prog_fd, ifindex, {INGRESS,EGRESS}):
> and make this proposed api to return FD.
> To detach from tc ingress/egress just close(fd).
You mean adding an fd-based TC API to the kernel?
> The user processes will not conflict with each other and will not accidently
> detach bpf program that was attached by another user process.
> Such api will address the existing tc query/attach/detach race race
> conditions.
Hmm, I think we do solve the race condition by returning the id. As long as you
don't misuse the interface and go around deleting filters arbitrarily (i.e. only
detach using the id), programs won't step over each other's filters. Storing the
id from the netlink response received during detach also eliminates any
ambigiuity from probing through get_info after attach. Same goes for actions,
and the same applies to the bpf_link returning API (which stashes id/index).
Do you have any other example that can still be racy given the current API?
The only advantage of fd would be the possibility of pinning it, and extending
lifetime of the filter.
> And libbpf side of support for this api will be trivial. Single bpf
> link_create command
> with ifindex and ingress|egress arguments.
> wdyt?
--
Kartikeya
Re: [PATCH bpf-next 3/5] libbpf: add low level TC-BPF API
On Fri, Apr 02, 2021 at 05:49:29AM IST, Daniel Borkmann wrote:
> On 3/31/21 11:44 AM, Kumar Kartikeya Dwivedi wrote:
> > On Wed, Mar 31, 2021 at 02:55:47AM IST, Daniel Borkmann wrote:
> > > Do we even need the _block variant? I would rather prefer to take the
> > > chance
> > > and make it as simple as possible, and only iff really needed extend with
> > > other APIs, for example:
> >
> > The block variant can be dropped, I'll use the TC_BLOCK/TC_DEV alternative
> > which
> > sets parent_id/ifindex properly.
> >
> > >bpf_tc_attach(prog_fd, ifindex, {INGRESS,EGRESS});
> > >
> > > Internally, this will create the sch_clsact qdisc & cls_bpf filter
> > > instance
> > > iff not present yet, and attach to a default prio 1 handle 1, and
> > > _always_ in
> > > direct-action mode. This is /as simple as it gets/ and we don't need to
> > > bother
> > > users with more complex tc/cls_bpf internals unless desired. For example,
> > > extended APIs could add prio/parent so that multi-prog can be attached to
> > > a
> > > single cls_bpf instance, but even that could be a second step, imho.
> >
> > I am not opposed to clsact qdisc setup if INGRESS/EGRESS is supplied (not
> > sure
> > how others feel about it).
>
> What speaks against it? It would be 100% clear from API side where the prog is
> being attached. Same as with tc cmdline where you specify 'ingress'/'egress'.
>
Ok, I will add the qdisc setup in the next revision.
> > We could make direct_action mode default, and similarly choose prio
>
> To be honest, I wouldn't even support a mode from the lib/API side where
> direct_action
> is not set. It should always be forced to true. Everything else is rather
> broken
> setup-wise, imho, since it won't scale. We added direct_action a bit later to
> the
> kernel than original cls_bpf, but if I would do it again today, I'd make it
> the
> only available option. I don't see a reasonable use-case where you have it to
> false.
>
I'm all for doing that, but in some sense that also speaks against SCHED_ACT
support. Currently, you can load SCHED_ACT programs using this series, but not
really bind them to classifier. I left that option open to a future patch, it
would just reuse the existing tc_act_add_action helper (also why I kept it in
its own function). Maybe we need to reconsider that, if direct action is the
only recommended way going forward (to discourage people from using SCHED_ACT),
or just add opts to do all the setup in low level API, instead of leaving it
incomplete.
> > as 1 by default instead of letting the kernel do it. Then you can just pass
> > in
> > NULL for bpf_tc_cls_opts and be close to what you're proposing. For
> > protocol we
> > can choose ETH_P_ALL by default too if the user doesn't set it.
>
> Same here with ETH_P_ALL, I'm not sure anyone uses anything other than
> ETH_P_ALL,
> so yes, that should be default.
Ack.
>
> > With these modifications, the equivalent would look like
> > bpf_tc_cls_attach(prog_fd, TC_DEV(ifindex, INGRESS), NULL, );
>
> Few things compared to bpf_tc_attach(prog_fd, ifindex, {INGRESS,EGRESS}):
>
> 1) nit, but why even 'cls' in the name. I think we shouldn't expose such
> old-days
>tc semantics to a user. Just bpf_tc_attach() is cleaner/simpler to
> understand.
Since it would make it clear this is for SCHED_CLS progs, likewise bpf_tc_act_*
is for SCHED_ACT progs. Not opposed to changing the name.
> 2) What's the 'TC_DEV(ifindex, INGRESS)' macro doing exactly? Looks
> unnecessary,
>why not regular args to the API?
It is very easy to support BLOCK (I know it's not really popular here, but I
think if supporting it just requires adding a macro, then we can go ahead). So
the user can use TC_BLOCK(block_idx) instead of remembering ifindex is to be set
to TCM_IFINDEX_MAGIC_BLOCK and parent_id to actual block index. It will just
expand to:
#define TC_BLOCK(block_idx) TCM_IFINDEX_MAGIC_BLOCK, (block_idx)
TC_DEV macro can be dropped, since user can directly pass ifindex and parent_id.
> 3) Exposed bpf_tc_attach() API could internally call a bpf_tc_attach_opts()
> API
>with preset defaults, and the latter could have all the custom bits if the
> user
>needs to go beyond the simple API, so from your bpf_tc_cls_attach() I'd
> also
>drop the NULL.
Ok, this is probably better (but maybe we can do this for the high-level
bpf_program__attach that returns a bpf_link * instead of introducing yet
another function).
> 4) For the simple API I'd likely also drop the id (you could have a query API
> if
>needed).
>
This would be fine, because it's n
Re: [PATCH bpf-next 3/5] libbpf: add low level TC-BPF API
On Wed, Mar 31, 2021 at 02:55:47AM IST, Daniel Borkmann wrote:
> Do we even need the _block variant? I would rather prefer to take the chance
> and make it as simple as possible, and only iff really needed extend with
> other APIs, for example:
The block variant can be dropped, I'll use the TC_BLOCK/TC_DEV alternative which
sets parent_id/ifindex properly.
>
> bpf_tc_attach(prog_fd, ifindex, {INGRESS,EGRESS});
>
> Internally, this will create the sch_clsact qdisc & cls_bpf filter instance
> iff not present yet, and attach to a default prio 1 handle 1, and _always_ in
> direct-action mode. This is /as simple as it gets/ and we don't need to bother
> users with more complex tc/cls_bpf internals unless desired. For example,
> extended APIs could add prio/parent so that multi-prog can be attached to a
> single cls_bpf instance, but even that could be a second step, imho.
>
I am not opposed to clsact qdisc setup if INGRESS/EGRESS is supplied (not sure
how others feel about it).
We could make direct_action mode default, and similarly choose prio
as 1 by default instead of letting the kernel do it. Then you can just pass in
NULL for bpf_tc_cls_opts and be close to what you're proposing. For protocol we
can choose ETH_P_ALL by default too if the user doesn't set it.
With these modifications, the equivalent would look like
bpf_tc_cls_attach(prog_fd, TC_DEV(ifindex, INGRESS), NULL, );
So as long as the user doesn't care about other details, they can just pass opts
as NULL.
WDYT?
> Thanks,
> Daniel
--
Kartikeya
Re: [PATCH bpf-next 3/5] libbpf: add low level TC-BPF API
On Wed, Mar 31, 2021 at 02:41:40AM IST, Toke Høiland-Jørgensen wrote:
> Andrii Nakryiko writes:
>
> > On Sun, Mar 28, 2021 at 1:11 AM Kumar Kartikeya Dwivedi
> > wrote:
> >>
> >> On Sun, Mar 28, 2021 at 10:12:40AM IST, Andrii Nakryiko wrote:
> >> > Is there some succinct but complete enough documentation/tutorial/etc
> >> > that I can reasonably read to understand kernel APIs provided by TC
> >> > (w.r.t. BPF, of course). I'm trying to wrap my head around this and
> >> > whether API makes sense or not. Please share links, if you have some.
> >> >
> >>
> >> Hi Andrii,
> >>
> >> Unfortunately for the kernel API part, I couldn't find any when I was
> >> working
> >> on this. So I had to read the iproute2 tc code (tc_filter.c, f_bpf.c,
> >> m_action.c, m_bpf.c) and the kernel side bits (cls_api.c, cls_bpf.c,
> >> act_api.c,
> >> act_bpf.c) to grok anything I didn't understand. There's also similar code
> >> in
> >> libnl (lib/route/{act,cls}.c).
> >>
> >> Other than that, these resources were useful (perhaps you already went
> >> through
> >> some/all of them):
> >>
> >> https://docs.cilium.io/en/latest/bpf/#tc-traffic-control
> >> https://qmonnet.github.io/whirl-offload/2020/04/11/tc-bpf-direct-action/
> >> tc(8), and tc-bpf(8) man pages
> >>
> >> I hope this is helpful!
> >
> > Thanks! I'll take a look. Sorry, I'm a bit behind with all the stuff,
> > trying to catch up.
> >
> > I was just wondering if it would be more natural instead of having
> > _dev _block variants and having to specify __u32 ifindex, __u32
> > parent_id, __u32 protocol, to have some struct specifying TC
> > "destination"? Maybe not, but I thought I'd bring this up early. So
> > you'd have just bpf_tc_cls_attach(), and you'd so something like
> >
> > bpf_tc_cls_attach(prog_fd, TC_DEV(ifindex, parent_id, protocol))
> >
> > or
> >
> > bpf_tc_cls_attach(prog_fd, TC_BLOCK(block_idx, protocol))
> >
> > ? Or it's taking it too far?
>
> Hmm, that's not a bad idea, actually. An earlier version of the series
> did have only a single set of functions, but with way too many
> arguments, which is why we ended up agreeing to split them. But
> encapsulating the destination in a separate struct and combining it with
> some helper macros might just make this work! I like it! Kumar, WDYT?
>
SGTM.
> -Toke
>
--
Kartikeya
Re: [PATCH v2 1/1] net: sched: bump refcount for new action in ACT replace mode
On Wed, Mar 31, 2021 at 10:10:45AM IST, Cong Wang wrote: > On Mon, Mar 29, 2021 at 3:55 PM Kumar Kartikeya Dwivedi > wrote: > > diff --git a/net/sched/act_api.c b/net/sched/act_api.c > > index b919826939e0..43cceb924976 100644 > > --- a/net/sched/act_api.c > > +++ b/net/sched/act_api.c > > @@ -1042,6 +1042,9 @@ struct tc_action *tcf_action_init_1(struct net *net, > > struct tcf_proto *tp, > > if (err != ACT_P_CREATED) > > module_put(a_o->owner); > > > > + if (!bind && ovr && err == ACT_P_CREATED) > > + refcount_set(>tcfa_refcnt, 2); > > + > > Hmm, if we set the refcnt to 2 here, how could tcf_action_destroy() > destroy them when we rollback from a failure in the middle of the loop > in tcf_action_init()? > You are right, it wouldn't. I sent a new version with a fix. PTAL. > Thanks. -- Kartikeya
[PATCH net-next v3] net: sched: bump refcount for new action in ACT replace mode
Currently, action creation using ACT API in replace mode is buggy. When
invoking for non-existent action index 42,
tc action replace action bpf obj foo.o sec index 42
kernel creates the action, fills up the netlink response, and then just
deletes the action while notifying userspace of success.
tc action show action bpf
doesn't list the action.
This happens due to the following sequence when ovr = 1 (replace mode)
is enabled:
tcf_idr_check_alloc is used to atomically check and either obtain
reference for existing action at index, or reserve the index slot using
a dummy entry (ERR_PTR(-EBUSY)).
This is necessary as pointers to these actions will be held after
dropping the idrinfo lock, so bumping the reference count is necessary
as we need to insert the actions, and notify userspace by dumping their
attributes. Finally, we drop the reference we took using the
tcf_action_put_many call in tcf_action_add. However, for the case where
a new action is created due to free index, its refcount remains one.
This when paired with the put_many call leads to the kernel setting up
the action, notifying userspace of its creation, and then tearing it
down. For existing actions, the refcount is still held so they remain
unaffected.
Fortunately due to rtnl_lock serialization requirement, such an action
with refcount == 1 will not be concurrently deleted by anything else, at
best CLS API can move its refcount up and down by binding to it after it
has been published from tcf_idr_insert_many. Since refcount is atleast
one until put_many call, CLS API cannot delete it. Also __tcf_action_put
release path already ensures deterministic outcome (either new action
will be created or existing action will be reused in case CLS API tries
to bind to action concurrently) due to idr lock serialization.
We fix this by making refcount of newly created actions as 2 in ACT API
replace mode. A relaxed store will suffice as visibility is ensured only
after the tcf_idr_insert_many call.
We also remember the new actions that we took an additional reference on,
and relinquish the temporal reference during rollback on failure.
Note that in case of creation or overwriting using CLS API only (i.e.
bind = 1), overwriting existing action object is not allowed, and any
such request is silently ignored (without error).
The refcount bump that occurs in tcf_idr_check_alloc call there for
existing action will pair with tcf_exts_destroy call made from the owner
module for the same action. In case of action creation, there is no
existing action, so no tcf_exts_destroy callback happens.
This means no code changes for CLS API.
Fixes: cae422f379f3 ("net: sched: use reference counting action init")
Signed-off-by: Kumar Kartikeya Dwivedi
---
Changelog:
v2 -> v3
Cleanup new action on rollback after raising refcount (Cong)
v1 -> v2
Remove erroneous tcf_action_put_many call in tcf_exts_validate (Vlad)
Isolate refcount bump to ACT API in replace mode
---
include/net/act_api.h | 2 +-
net/sched/act_api.c | 24 ++--
net/sched/cls_api.c | 2 +-
3 files changed, 24 insertions(+), 4 deletions(-)
diff --git a/include/net/act_api.h b/include/net/act_api.h
index 2bf3092ae7ec..a01ff5fa641e 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -194,7 +194,7 @@ struct tc_action *tcf_action_init_1(struct net *net, struct
tcf_proto *tp,
struct nlattr *nla, struct nlattr *est,
char *name, int ovr, int bind,
struct tc_action_ops *ops, bool rtnl_held,
- struct netlink_ext_ack *extack);
+ struct netlink_ext_ack *extack, bool *ref);
int tcf_action_dump(struct sk_buff *skb, struct tc_action *actions[], int bind,
int ref, bool terse);
int tcf_action_dump_old(struct sk_buff *skb, struct tc_action *a, int, int);
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index b919826939e0..718bc197b9a7 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -993,7 +993,7 @@ struct tc_action *tcf_action_init_1(struct net *net, struct
tcf_proto *tp,
struct nlattr *nla, struct nlattr *est,
char *name, int ovr, int bind,
struct tc_action_ops *a_o, bool rtnl_held,
- struct netlink_ext_ack *extack)
+ struct netlink_ext_ack *extack, bool *ref)
{
struct nla_bitfield32 flags = { 0, 0 };
u8 hw_stats = TCA_ACT_HW_STATS_ANY;
@@ -1042,6 +1042,13 @@ struct tc_action *tcf_action_init_1(struct net *net,
struct tcf_proto *tp,
if (err != ACT_P_CREATED)
module_put(a_o->owner);
+ if (!bind && ovr && err == ACT_P_CREATED) {
+ if (ref)
+ *ref = t
Re: [PATCH 1/1] net: sched: extend lifetime of new action in replace mode
On Mon, Mar 29, 2021 at 02:35:12PM IST, Vlad Buslov wrote: > it seems that there are two ways actions are overwritten/deleted: > > 1. Directly through action API, which is still serialized by rtnl lock. > > 2. Classifier API, which doesn't use rtnl lock anymore and can execute > concurrently. > > Actions created by path 2 also have their bind count incremented which > prevents them from being deleted by path 1 and cls API can only deleted > them together with classifier that points to them. > > [...] > So, what happens here is actions were 'deleted' concurrently (their > tcfa_refcnt decremented by 1)? tcf_action_put_many() will decrement > refcnt again, it will reach 0, actions get actually deleted and > tcf_exts_validate() returns with non-error code, but exts->actions > pointing to freed memory? Doesn't look like the patches fixes the > described issue, unless I'm missing something. > Thanks for the review and comments. You are absolutely right. This patch was totally broken. Your feedback however was quite helpful in understanding the code. I sent a v2, please lmk if it's correct (also with a hopefully thorough description of the problem & solution). -- Kartikeya
[PATCH v2 1/1] net: sched: bump refcount for new action in ACT replace mode
Currently, action creation using ACT API in replace mode is buggy.
When invoking for non-existent action index 42,
tc action replace action bpf obj foo.o sec index 42
kernel creates the action, fills up the netlink response, and then just
deletes the action after notifying userspace.
tc action show action bpf
doesn't list the action.
This happens due to the following sequence when ovr = 1 (replace mode)
is enabled:
tcf_idr_check_alloc is used to atomically check and either obtain
reference for existing action at index, or reserve the index slot using
a dummy entry (ERR_PTR(-EBUSY)).
This is necessary as pointers to these actions will be held after
dropping the idrinfo lock, so bumping the reference count is necessary
as we need to insert the actions, and notify userspace by dumping their
attributes. Finally, we drop the reference we took using the
tcf_action_put_many call in tcf_action_add. However, for the case where
a new action is created due to free index, its refcount remains one.
This when paired with the put_many call leads to the kernel setting up
the action, notifying userspace of its creation, and then tearing it
down. For existing actions, the refcount is still held so they remain
unaffected.
Fortunately due to rtnl_lock serialization requirement, such an action
with refcount == 1 will not be concurrently deleted by anything else, at
best CLS API can move its refcount up and down by binding to it after it
has been published from tcf_idr_insert_many. Since refcount is atleast
one until put_many call, CLS API cannot delete it. Also __tcf_action_put
release path already ensures deterministic outcome (either new action
will be created or existing action will be reused in case CLS API tries
to bind to action concurrently) due to idr lock serialization.
We fix this by making refcount of newly created actions as 2 in ACT API
replace mode. A relaxed store will suffice as visibility is ensured only
after the tcf_idr_insert_many call.
Note that in case of creation or overwriting using CLS API only (i.e.
bind = 1), overwriting existing action object is not allowed, and any
such request is silently ignored (without error).
The refcount bump that occurs in tcf_idr_check_alloc call there for
existing action will pair with tcf_exts_destroy call made from the
owner module for the same action. In case of action creation, there
is no existing action, so no tcf_exts_destroy callback happens.
This means no code changes for CLS API.
Fixes: cae422f379f3 ("net: sched: use reference counting action init")
Signed-off-by: Kumar Kartikeya Dwivedi
---
Changelog:
v1 -> v2
Remove erroneous tcf_action_put_many call in tcf_exts_validate (Vlad)
Isolate refcount bump to ACT API in replace mode
---
net/sched/act_api.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index b919826939e0..43cceb924976 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -1042,6 +1042,9 @@ struct tc_action *tcf_action_init_1(struct net *net,
struct tcf_proto *tp,
if (err != ACT_P_CREATED)
module_put(a_o->owner);
+ if (!bind && ovr && err == ACT_P_CREATED)
+ refcount_set(>tcfa_refcnt, 2);
+
return a;
err_out:
--
2.30.2
Re: [PATCH bpf-next 5/5] libbpf: add selftests for TC-BPF API
On Mon, Mar 29, 2021 at 06:56:02AM IST, Alexei Starovoitov wrote: > This is up to you. I'm trying to understand the motivation for *_block() apis. > I'm not taking a stance for/against them. The block APIs simply attach to a different shared filter block, so in that sense they just forward to the bpf_tc_cls_*_dev API internally, where parent_id is substituted as block_index, and ifindex is set to a special value (to indicate operation on a block), but is still a distinct attach point, and both APIs cannot be mixed (i.e. manipulation of filter attached using block API is not possible using dev API). e.g. # tc qdisc add dev ingress block 1 # tc qdisc add dev ingress block 1 Now you can attach a filter to the shared block, e.g. # tc filter add block 1 bpf /home/kkd/foo.o sec cls direct-action and it will attach the identical filter with the bpf prog classifier to both qdiscs in one go, instead of having to duplicate filter creation for each qdisc. You can add arbitrarily many qdiscs to such a filter block, easing filter management, and saving on resources. So for the API, it made sense to separate this into its own function as it is a different attach point, both for the low level API and their higher level wrappers. This does increase the symbol count, but maintenance wise it is zero-cost since it simply forwards to the dev functions. As for the tests, I'll add them for the block API in v2, when I get around to sending it (i.e. after the review is over). > [...] -- Kartikeya
Re: [PATCH bpf-next 3/5] libbpf: add low level TC-BPF API
On Sun, Mar 28, 2021 at 10:12:40AM IST, Andrii Nakryiko wrote:
> Is there some succinct but complete enough documentation/tutorial/etc
> that I can reasonably read to understand kernel APIs provided by TC
> (w.r.t. BPF, of course). I'm trying to wrap my head around this and
> whether API makes sense or not. Please share links, if you have some.
>
Hi Andrii,
Unfortunately for the kernel API part, I couldn't find any when I was working
on this. So I had to read the iproute2 tc code (tc_filter.c, f_bpf.c,
m_action.c, m_bpf.c) and the kernel side bits (cls_api.c, cls_bpf.c, act_api.c,
act_bpf.c) to grok anything I didn't understand. There's also similar code in
libnl (lib/route/{act,cls}.c).
Other than that, these resources were useful (perhaps you already went through
some/all of them):
https://docs.cilium.io/en/latest/bpf/#tc-traffic-control
https://qmonnet.github.io/whirl-offload/2020/04/11/tc-bpf-direct-action/
tc(8), and tc-bpf(8) man pages
I hope this is helpful!
--
Kartikeya
[PATCH 1/1] net: sched: extend lifetime of new action in replace mode
When creating an action in replace mode, in tcf_action_add, the refcount
of existing actions is rightly raised during tcf_idr_check_alloc call,
but for new actions a dummy placeholder entry is created. This is then
replaced with the actual action during tcf_idr_insert_many, but between
this and the tcf_action_put_many call made in replace mode, we don't
hold a reference to the newly created action while it has been
published. This means that it can disappear under our feet, and that
newly created actions are destroyed right after their creation as their
refcount drops to 0 in replace mode.
This leads to commands like tc action replace reporting success in
creation of the action and just removing them right after adding the
action, leading to confusion in userspace.
Fix this by raising the refcount of a newly created action and then
pairing that increment with a tcf_action_put call to release the
reference held during insertion. This is only needed in replace mode.
We use a relaxed store as insert will ensure its visibility.
Fixes: cae422f379f3 ("net: sched: use reference counting action init")
Signed-off-by: Kumar Kartikeya Dwivedi
---
include/net/act_api.h | 1 +
net/sched/act_api.c | 5 -
net/sched/cls_api.c | 3 +++
3 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/include/net/act_api.h b/include/net/act_api.h
index 2bf3092ae7ec..8b375b46cc04 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -181,6 +181,7 @@ int tcf_register_action(struct tc_action_ops *a, struct
pernet_operations *ops);
int tcf_unregister_action(struct tc_action_ops *a,
struct pernet_operations *ops);
int tcf_action_destroy(struct tc_action *actions[], int bind);
+void tcf_action_put_many(struct tc_action *actions[]);
int tcf_action_exec(struct sk_buff *skb, struct tc_action **actions,
int nr_actions, struct tcf_result *res);
int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index b919826939e0..7e26c18cdb15 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -778,7 +778,7 @@ static int tcf_action_put(struct tc_action *p)
}
/* Put all actions in this array, skip those NULL's. */
-static void tcf_action_put_many(struct tc_action *actions[])
+void tcf_action_put_many(struct tc_action *actions[])
{
int i;
@@ -1042,6 +1042,9 @@ struct tc_action *tcf_action_init_1(struct net *net,
struct tcf_proto *tp,
if (err != ACT_P_CREATED)
module_put(a_o->owner);
+ if (err == ACT_P_CREATED && ovr)
+ refcount_set(>tcfa_refcnt, 2);
+
return a;
err_out:
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index d3db70865d66..666077c23147 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -3074,6 +3074,9 @@ int tcf_exts_validate(struct net *net, struct tcf_proto
*tp, struct nlattr **tb,
exts->nr_actions = err;
}
}
+
+ if (ovr)
+ tcf_action_put_many(exts->actions);
#else
if ((exts->action && tb[exts->action]) ||
(exts->police && tb[exts->police])) {
--
2.30.2
Re: [PATCH bpf-next 1/5] tools pkt_cls.h: sync with kernel sources
On Sat, Mar 27, 2021 at 04:55:51AM IST, Andrii Nakryiko wrote: > On Thu, Mar 25, 2021 at 5:01 AM Kumar Kartikeya Dwivedi > wrote: > > > > Update the header file so we can use the new defines in subsequent > > patches. > > > > Reviewed-by: Toke Høiland-Jørgensen > > Signed-off-by: Kumar Kartikeya Dwivedi > > --- > > tools/include/uapi/linux/pkt_cls.h | 174 - > > If libbpf is going to rely on this UAPI header, we probably need to > add this header to the list of headers that are checked for being up > to date. See Makefile, roughly at line 140. > Ok, will do in v2. > > 1 file changed, 170 insertions(+), 4 deletions(-) > > > > [...] -- Kartikeya
[PATCH bpf-next 5/5] libbpf: add selftests for TC-BPF API
This adds some basic tests for the low level bpf_tc_* API and its
bpf_program__attach_tc_* wrapper on top.
Reviewed-by: Toke Høiland-Jørgensen
Signed-off-by: Kumar Kartikeya Dwivedi
---
.../selftests/bpf/prog_tests/test_tc_bpf.c| 261 ++
.../selftests/bpf/progs/test_tc_bpf_kern.c| 18 ++
2 files changed, 279 insertions(+)
create mode 100644 tools/testing/selftests/bpf/prog_tests/test_tc_bpf.c
create mode 100644 tools/testing/selftests/bpf/progs/test_tc_bpf_kern.c
diff --git a/tools/testing/selftests/bpf/prog_tests/test_tc_bpf.c
b/tools/testing/selftests/bpf/prog_tests/test_tc_bpf.c
new file mode 100644
index ..8bab56b4dea0
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/test_tc_bpf.c
@@ -0,0 +1,261 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define LO_IFINDEX 1
+
+static int test_tc_cls_internal(int fd, __u32 parent_id)
+{
+ struct bpf_tc_cls_attach_id id = {};
+ struct bpf_tc_cls_info info = {};
+ int ret;
+ DECLARE_LIBBPF_OPTS(bpf_tc_cls_opts, opts, .handle = 1, .priority = 10,
+ .class_id = TC_H_MAKE(1UL << 16, 1),
+ .chain_index = 5);
+
+ ret = bpf_tc_cls_attach_dev(fd, LO_IFINDEX, parent_id, ETH_P_IP, ,
+ );
+ if (CHECK_FAIL(ret < 0))
+ return ret;
+
+ ret = bpf_tc_cls_get_info_dev(fd, LO_IFINDEX, parent_id, ETH_P_IP, NULL,
+ );
+ if (CHECK_FAIL(ret < 0))
+ goto end;
+
+ ret = -1;
+
+ if (CHECK_FAIL(info.id.ifindex != id.ifindex) ||
+ CHECK_FAIL(info.id.parent_id != id.parent_id) ||
+ CHECK_FAIL(info.id.handle != id.handle) ||
+ CHECK_FAIL(info.id.protocol != id.protocol) ||
+ CHECK_FAIL(info.id.chain_index != id.chain_index) ||
+ CHECK_FAIL(info.id.priority != id.priority) ||
+ CHECK_FAIL(info.id.ifindex != LO_IFINDEX) ||
+ CHECK_FAIL(info.id.parent_id != parent_id) ||
+ CHECK_FAIL(info.id.handle != 1) ||
+ CHECK_FAIL(info.id.priority != 10) ||
+ CHECK_FAIL(info.id.protocol != ETH_P_IP) ||
+ CHECK_FAIL(info.class_id != TC_H_MAKE(1UL << 16, 1)) ||
+ CHECK_FAIL(info.id.chain_index != 5))
+ goto end;
+
+ opts.direct_action = true;
+ ret = bpf_tc_cls_replace_dev(fd, id.ifindex, id.parent_id, id.protocol,
+, );
+ if (CHECK_FAIL(ret < 0))
+ return ret;
+
+end:;
+ ret = bpf_tc_cls_detach_dev();
+ CHECK_FAIL(ret < 0);
+ return ret;
+}
+
+static int test_tc_cls(struct bpf_program *prog, __u32 parent_id)
+{
+ struct bpf_tc_cls_info info = {};
+ struct bpf_link *link;
+ int ret;
+ DECLARE_LIBBPF_OPTS(bpf_tc_cls_opts, opts, .priority = 10, .handle = 1,
+ .class_id = TC_H_MAKE(1UL << 16, 1));
+
+ link = bpf_program__attach_tc_cls_dev(prog, LO_IFINDEX, parent_id,
+ ETH_P_ALL, );
+ if (CHECK_FAIL(IS_ERR_OR_NULL(link)))
+ return PTR_ERR(link);
+
+ ret = bpf_tc_cls_get_info_dev(bpf_program__fd(prog), LO_IFINDEX,
+ parent_id, ETH_P_ALL, NULL, );
+ if (CHECK_FAIL(ret < 0))
+ goto end;
+
+ ret = -1;
+
+ if (CHECK_FAIL(info.id.ifindex != LO_IFINDEX) ||
+ CHECK_FAIL(info.id.handle != 1) ||
+ CHECK_FAIL(info.id.priority != 10) ||
+ CHECK_FAIL(info.id.protocol != ETH_P_ALL) ||
+ CHECK_FAIL(info.class_id != TC_H_MAKE(1UL << 16, 1)))
+ goto end;
+
+ /* Demonstrate changing attributes (e.g. to direct action) */
+ opts.class_id = TC_H_MAKE(1UL << 16, 2);
+ opts.direct_action = true;
+
+ /* Disconnect as we drop to the lower level API, which invalidates the
+* link.
+*/
+ bpf_link__disconnect(link);
+
+ ret = bpf_tc_cls_change_dev(bpf_program__fd(prog), info.id.ifindex,
+ info.id.parent_id, info.id.protocol, ,
+ );
+ if (CHECK_FAIL(ret < 0))
+ goto end;
+
+ ret = bpf_tc_cls_get_info_dev(bpf_program__fd(prog), info.id.ifindex,
+ info.id.parent_id, info.id.protocol, NULL,
+ );
+ if (CHECK_FAIL(ret < 0))
+ goto end;
+
+ ret = -1;
+
+ if (CHECK_FAIL(info.class_id != TC_H_MAKE(1UL << 16, 2)))
+ goto end;
+ if (CHECK_FAIL((info.bpf_flags & TCA_BPF_FLAG_ACT_DIRECT) != 1))
+ goto end;
+
+ ret = bpf_tc_cls_detach_dev();
+ if (CHECK_FAIL(ret < 0))
+ goto end;
+
[PATCH bpf-next 4/5] libbpf: add high level TC-BPF API
A high level API is provided using the aforementioned routines internally,
and these return a bpf_link object to the user. These are limited to just
attach for now, and can be extended to change/replace if the use case
arises in the future. It is also possible to call bpf_link__disconnect
on the link and switch to managing the filter/action manually if the
need arises. In most cases, the higher level API should suffice.
Example:
struct bpf_tc_cls_info info = {};
struct bpf_object *obj;
struct bpf_program *p;
struct bpf_link *link;
__u32 index;
int fd, r;
obj = bpf_object_open("foo.o");
if (IS_ERR_OR_NULL(obj))
return PTR_ERR(obj);
p = bpf_object__find_program_by_title(obj, "classifier");
if (IS_ERR_OR_NULL(p))
return PTR_ERR(p);
DECLARE_LIBBPF_OPTS(bpf_tc_cls_opts, opts, .handle = 1);
link = bpf_program__attach_tc_cls_dev(p, if_nametoindex("lo"),
BPF_TC_CLSACT_INGRESS,
ETH_P_IP, );
if (IS_ERR_OR_NULL(link))
return PTR_ERR(link);
/* We want to take ownership of the filter, so we disconnect the
* link and detach it on our own
*/
bpf_link__disconnect(link);
r = bpf_tc_cls_get_info_dev(bpf_program__fd(fd),
if_nametoindex("lo"),
BPF_TC_CLSACT_INGRESS,
ETH_P_IP, , );
if (r < 0)
return r;
/* We get the attach_id in the info struct, pass it to detach */
bpf_tc_cls_detach_dev();
bpf_link__destroy(link);
Example:
struct bpf_object *obj;
struct bpf_program *p;
struct bpf_link *link;
__u32 index;
int fd, r;
obj = bpf_object_open("foo.o");
if (IS_ERR_OR_NULL(obj))
return PTR_ERR(obj);
p = bpf_object__find_program_by_title(obj, "action");
if (IS_ERR_OR_NULL(p))
return PTR_ERR(p);
/* A simple example that attaches a SCHED_ACT prog */
link = bpf_program__attach_tc_act(p, NULL);
if (IS_ERR_OR_NULL(link))
return PTR_ERR(link);
bpf_link__destroy(link);
Reviewed-by: Toke Høiland-Jørgensen
Signed-off-by: Kumar Kartikeya Dwivedi
---
tools/lib/bpf/libbpf.c | 110 ++-
tools/lib/bpf/libbpf.h | 15 ++
tools/lib/bpf/libbpf.map | 3 ++
3 files changed, 127 insertions(+), 1 deletion(-)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 058b643cbcb1..cc5c200a661d 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -33,6 +33,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -6847,7 +6848,7 @@ static int bpf_object__collect_relos(struct bpf_object
*obj)
for (i = 0; i < obj->nr_programs; i++) {
struct bpf_program *p = >programs[i];
-
+
if (!p->nr_reloc)
continue;
@@ -9443,6 +9444,10 @@ int bpf_prog_load_xattr(const struct bpf_prog_load_attr
*attr,
struct bpf_link {
int (*detach)(struct bpf_link *link);
int (*destroy)(struct bpf_link *link);
+ union {
+ struct bpf_tc_cls_attach_id *tc_cls_id;
+ __u32 tc_act_index;
+ };
char *pin_path; /* NULL, if not pinned */
int fd; /* hook FD, -1 if not applicable */
bool disconnected;
@@ -10199,6 +10204,109 @@ struct bpf_link *bpf_map__attach_struct_ops(struct
bpf_map *map)
return link;
}
+static int bpf_link__detach_tc_cls(struct bpf_link *link)
+{
+ return bpf_tc_cls_detach_dev(link->tc_cls_id);
+}
+
+static int bpf_link__destroy_tc_cls(struct bpf_link *link)
+{
+ zfree(>tc_cls_id);
+ return 0;
+}
+
+struct bpf_link *bpf_program__attach_tc_cls_dev(struct bpf_program *prog,
+ __u32 ifindex, __u32 parent_id,
+ __u32 protocol,
+ const struct bpf_tc_cls_opts
*opts)
+{
+ struct bpf_tc_cls_attach_id *id = NULL;
+ struct bpf_link *link = NULL;
+ char errmsg[STRERR_BUFSIZE];
+ int prog_fd, err;
+
+ prog_fd = bpf_program__fd(prog);
+ if (prog_fd < 0) {
+ pr_warn("prog '%s': can't attach before loaded\n", prog->name);
+ return ERR_PTR(-EINVAL);
+ }
+
+ link = calloc(1, sizeof(*link));
+ if (!link)
+ return ERR_PTR(-ENOMEM);
+ link->detach = _link__detach_tc_cls;
+ link->destroy = _link__destroy_tc_cls;
+ link->fd = -1;
+
+
[PATCH bpf-next 3/5] libbpf: add low level TC-BPF API
t fd, r;
obj = bpf_object_open("foo.o");
if (IS_ERR_OR_NULL(obj))
return PTR_ERR(obj);
p = bpf_object__find_program_by_title(obj, "action");
if (IS_ERR_OR_NULL(p))
return PTR_ERR(p);
if (bpf_object__load(obj) < 0)
return -1;
fd = bpf_program__fd(p);
r = bpf_tc_act_attach(fd, NULL, );
if (r < 0)
return r;
if (bpf_tc_act_detach(index))
return -1;
... which is equivalent to the following sequence:
tc action add action bpf obj /home/kkd/foo.o sec action
tc action del action bpf index
Reviewed-by: Toke Høiland-Jørgensen
Signed-off-by: Kumar Kartikeya Dwivedi
---
tools/lib/bpf/libbpf.h | 118 +++
tools/lib/bpf/libbpf.map | 14 +
tools/lib/bpf/netlink.c | 715 ++-
3 files changed, 841 insertions(+), 6 deletions(-)
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index a1a424b9b8ff..63baef6045b1 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -16,6 +16,9 @@
#include
#include // for size_t
#include
+#include
+#include
+#include
#include "libbpf_common.h"
@@ -773,6 +776,121 @@ LIBBPF_API int bpf_linker__add_file(struct bpf_linker
*linker, const char *filen
LIBBPF_API int bpf_linker__finalize(struct bpf_linker *linker);
LIBBPF_API void bpf_linker__free(struct bpf_linker *linker);
+/*
+ * Requirements:
+ * If choosing hw offload mode (skip_sw = true), ifindex during prog load
must be set.
+ */
+
+/* Convenience macros for the clsact attach hooks */
+#define BPF_TC_CLSACT_INGRESS TC_H_MAKE(TC_H_CLSACT, TC_H_MIN_INGRESS)
+#define BPF_TC_CLSACT_EGRESS TC_H_MAKE(TC_H_CLSACT, TC_H_MIN_EGRESS)
+
+struct bpf_tc_cls_opts {
+ size_t sz;
+ __u32 chain_index;
+ __u32 handle;
+ __u32 priority;
+ __u32 class_id;
+ bool direct_action;
+ bool skip_sw;
+ bool skip_hw;
+ size_t :0;
+};
+
+#define bpf_tc_cls_opts__last_field skip_hw
+
+/* Acts as a handle for an attached filter */
+struct bpf_tc_cls_attach_id {
+ __u32 ifindex;
+ union {
+ __u32 block_index;
+ __u32 parent_id;
+ };
+ __u32 protocol;
+ __u32 chain_index;
+ __u32 handle;
+ __u32 priority;
+};
+
+struct bpf_tc_cls_info {
+ struct bpf_tc_cls_attach_id id;
+ __u32 class_id;
+ __u32 bpf_flags;
+ __u32 bpf_flags_gen;
+};
+
+/* id is out parameter that will be written to, it must not be NULL */
+LIBBPF_API int bpf_tc_cls_attach_dev(int fd, __u32 ifindex, __u32 parent_id,
+__u32 protocol,
+const struct bpf_tc_cls_opts *opts,
+struct bpf_tc_cls_attach_id *id);
+LIBBPF_API int bpf_tc_cls_change_dev(int fd, __u32 ifindex, __u32 parent_id,
+__u32 protocol,
+const struct bpf_tc_cls_opts *opts,
+struct bpf_tc_cls_attach_id *id);
+/* This replaces an existing filter with the same attributes, so the arguments
+ * can be filled in from an existing attach_id when replacing, and otherwise be
+ * used like bpf_tc_cls_attach_dev.
+ */
+LIBBPF_API int bpf_tc_cls_replace_dev(int fd, __u32 ifindex, __u32 parent_id,
+ __u32 protocol,
+ const struct bpf_tc_cls_opts *opts,
+ struct bpf_tc_cls_attach_id *id);
+LIBBPF_API int bpf_tc_cls_detach_dev(const struct bpf_tc_cls_attach_id *id);
+LIBBPF_API int bpf_tc_cls_get_info_dev(int fd, __u32 ifindex, __u32 parent_id,
+ __u32 protocol,
+ const struct bpf_tc_cls_opts *opts,
+ struct bpf_tc_cls_info *info);
+
+/* id is out parameter that will be written to, it must not be NULL */
+LIBBPF_API int bpf_tc_cls_attach_block(int fd, __u32 block_index,
+ __u32 protocol,
+ const struct bpf_tc_cls_opts *opts,
+ struct bpf_tc_cls_attach_id *id);
+LIBBPF_API int bpf_tc_cls_change_block(int fd, __u32 block_index,
+ __u32 protocol,
+ const struct bpf_tc_cls_opts *opts,
+ struct bpf_tc_cls_attach_id *id);
+/* This replaces an existing filter with the same attributes, so the arguments
+ * can be filled in from an existing attach_id when replacing, and otherwise be
+ * used like bpf_tc_cls_attach_block.
+ */
+LIBBPF_API int bpf_tc_cls_replace_block(int fd, __u32 block_index,
+ __u32 protocol,
+
[PATCH bpf-next 2/5] libbpf: add helpers for preparing netlink attributes
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
Signed-off-by: Kumar Kartikeya Dwivedi
---
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 *))
- + NLMSG_ALIGN(req.nh.nlmsg_len));
- nla->nla_type = NLA_F_NESTED | IFLA_XDP;
- nla->nla_len = NLA_HDRLEN;
+ nla = begin_nlattr_nested(, 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, , sizeof(fd));
- nla->nla_len += nla_xdp->nla_len;
+ ret = add_nlattr(, sizeof(req), IFLA_XDP_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, , sizeof(flags));
- nla->nla_len += nla_xdp->nla_len;
+ ret = add_nlattr(, sizeof(req), IFLA_XDP_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, _fd, sizeof(old_fd));
- nla->nla_len += nla_xdp->nla_len;
+ ret = add_nlattr(, sizeof(req), IFLA_XDP_EXPECTED_FD,
, sizeof(flags));
+ if (ret < 0)
+ goto cleanup;
}
- req.nh.nlmsg_len += NLA_ALIGN(nla->nla_len);
+ end_nlattr_nested(, nla);
if (send(sock, , 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
+#include
+#include
#include
+
/* 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;
[PATCH bpf-next 1/5] tools pkt_cls.h: sync with kernel sources
Update the header file so we can use the new defines in subsequent
patches.
Reviewed-by: Toke Høiland-Jørgensen
Signed-off-by: Kumar Kartikeya Dwivedi
---
tools/include/uapi/linux/pkt_cls.h | 174 -
1 file changed, 170 insertions(+), 4 deletions(-)
diff --git a/tools/include/uapi/linux/pkt_cls.h
b/tools/include/uapi/linux/pkt_cls.h
index 12153771396a..025c40fef93d 100644
--- a/tools/include/uapi/linux/pkt_cls.h
+++ b/tools/include/uapi/linux/pkt_cls.h
@@ -16,9 +16,36 @@ enum {
TCA_ACT_STATS,
TCA_ACT_PAD,
TCA_ACT_COOKIE,
+ TCA_ACT_FLAGS,
+ TCA_ACT_HW_STATS,
+ TCA_ACT_USED_HW_STATS,
__TCA_ACT_MAX
};
+#define TCA_ACT_FLAGS_NO_PERCPU_STATS 1 /* Don't use percpu allocator for
+* actions stats.
+*/
+
+/* tca HW stats type
+ * When user does not pass the attribute, he does not care.
+ * It is the same as if he would pass the attribute with
+ * all supported bits set.
+ * In case no bits are set, user is not interested in getting any HW
statistics.
+ */
+#define TCA_ACT_HW_STATS_IMMEDIATE (1 << 0) /* Means that in dump, user
+* gets the current HW stats
+* state from the device
+* queried at the dump time.
+*/
+#define TCA_ACT_HW_STATS_DELAYED (1 << 1) /* Means that in dump, user gets
+ * HW stats that might be out of date
+ * for some time, maybe couple of
+ * seconds. This is the case when
+ * driver polls stats updates
+ * periodically or when it gets async
+ * stats update from the device.
+ */
+
#define TCA_ACT_MAX __TCA_ACT_MAX
#define TCA_OLD_COMPAT (TCA_ACT_MAX+1)
#define TCA_ACT_MAX_PRIO 32
@@ -63,12 +90,53 @@ enum {
#define TC_ACT_GOTO_CHAIN __TC_ACT_EXT(2)
#define TC_ACT_EXT_OPCODE_MAX TC_ACT_GOTO_CHAIN
+/* These macros are put here for binary compatibility with userspace apps that
+ * make use of them. For kernel code and new userspace apps, use the TCA_ID_*
+ * versions.
+ */
+#define TCA_ACT_GACT 5
+#define TCA_ACT_IPT 6
+#define TCA_ACT_PEDIT 7
+#define TCA_ACT_MIRRED 8
+#define TCA_ACT_NAT 9
+#define TCA_ACT_XT 10
+#define TCA_ACT_SKBEDIT 11
+#define TCA_ACT_VLAN 12
+#define TCA_ACT_BPF 13
+#define TCA_ACT_CONNMARK 14
+#define TCA_ACT_SKBMOD 15
+#define TCA_ACT_CSUM 16
+#define TCA_ACT_TUNNEL_KEY 17
+#define TCA_ACT_SIMP 22
+#define TCA_ACT_IFE 25
+#define TCA_ACT_SAMPLE 26
+
/* Action type identifiers*/
-enum {
- TCA_ID_UNSPEC=0,
- TCA_ID_POLICE=1,
+enum tca_id {
+ TCA_ID_UNSPEC = 0,
+ TCA_ID_POLICE = 1,
+ TCA_ID_GACT = TCA_ACT_GACT,
+ TCA_ID_IPT = TCA_ACT_IPT,
+ TCA_ID_PEDIT = TCA_ACT_PEDIT,
+ TCA_ID_MIRRED = TCA_ACT_MIRRED,
+ TCA_ID_NAT = TCA_ACT_NAT,
+ TCA_ID_XT = TCA_ACT_XT,
+ TCA_ID_SKBEDIT = TCA_ACT_SKBEDIT,
+ TCA_ID_VLAN = TCA_ACT_VLAN,
+ TCA_ID_BPF = TCA_ACT_BPF,
+ TCA_ID_CONNMARK = TCA_ACT_CONNMARK,
+ TCA_ID_SKBMOD = TCA_ACT_SKBMOD,
+ TCA_ID_CSUM = TCA_ACT_CSUM,
+ TCA_ID_TUNNEL_KEY = TCA_ACT_TUNNEL_KEY,
+ TCA_ID_SIMP = TCA_ACT_SIMP,
+ TCA_ID_IFE = TCA_ACT_IFE,
+ TCA_ID_SAMPLE = TCA_ACT_SAMPLE,
+ TCA_ID_CTINFO,
+ TCA_ID_MPLS,
+ TCA_ID_CT,
+ TCA_ID_GATE,
/* other actions go here */
- __TCA_ID_MAX=255
+ __TCA_ID_MAX = 255
};
#define TCA_ID_MAX __TCA_ID_MAX
@@ -120,6 +188,10 @@ enum {
TCA_POLICE_RESULT,
TCA_POLICE_TM,
TCA_POLICE_PAD,
+ TCA_POLICE_RATE64,
+ TCA_POLICE_PEAKRATE64,
+ TCA_POLICE_PKTRATE64,
+ TCA_POLICE_PKTBURST64,
__TCA_POLICE_MAX
#define TCA_POLICE_RESULT TCA_POLICE_RESULT
};
@@ -333,12 +405,19 @@ enum {
/* Basic filter */
+struct tc_basic_pcnt {
+ __u64 rcnt;
+ __u64 rhit;
+};
+
enum {
TCA_BASIC_UNSPEC,
TCA_BASIC_CLASSID,
TCA_BASIC_EMATCHES,
TCA_BASIC_ACT,
TCA_BASIC_POLICE,
+ TCA_BASIC_PCNT,
+ TCA_BASIC_PAD,
__TCA_BASIC_MAX
};
@@ -485,17 +564,54 @@ enum {
TCA_FLOWER_IN_HW_COUNT,
+ TCA_FLOWER_KEY_PORT_SRC_MIN,/* be16 */
+ TCA_FLOWER_KEY_PORT_SRC_MAX,/* be16 */
+ TCA_FLOWER_KEY_PORT_DST_MIN,/* be16 */
+ TCA_FLOWER_KEY_PORT_DST_MAX,/* be16 */
+
+ TCA_FLOWER_KEY_CT_STATE,/* u16 */
+ TCA_FLOWER_KEY_CT_STATE_MASK, /* u16 */
+ TCA_FLOWER_KEY_CT_ZONE, /* u16 */
+ TCA_FLOWER_KEY_CT_ZONE_MASK,/* u16 */
+ TCA_FLOWER_KEY_CT_MARK,
[PATCH bpf-next 0/5] libbpf: Add TC-BPF API
This series adds support to libbpf for attaching SCHED_CLS and SCHED_ACT bpf programs to their respective tc attach points. Currently, a user needs to shell out to the tc command line for add, change, replace, and del operations, which is not ideal. Some of the features that have been omitted for the CLS API: * TCA_BPF_POLICE Support for adding police actions to filter has been omitted for now. * TCA_RATE Support for packet rate estimator has been omitted for now. * Attaching actions directly to the classifier This allows the attached actions to be bound to classifier and get auto detached when it is deleted. It translates to 'bind' refcount in the kernel internally. They run after a successful classification from the SCHED_CLS prog. Support for this can be added later, but has been omitted for now, primarily because direct-action mode provides a better alternative. A high level TC-BPF API is also provided, and currently only supports attach and destroy operations. These functions return a pointer to a bpf_link object. When falling back to the low level API, the link must be disconnected to take over its ownership. It can be released using bpf_link__destroy, which will also cause the filter/action to be detached if not disconnected. The individual commits contain a general API summary and examples. Kumar Kartikeya Dwivedi (5): tools pkt_cls.h: sync with kernel sources libbpf: add helpers for preparing netlink attributes libbpf: add low level TC-BPF API libbpf: add high level TC-BPF API libbpf: add selftests for TC-BPF API tools/include/uapi/linux/pkt_cls.h| 174 +++- tools/lib/bpf/libbpf.c| 110 ++- tools/lib/bpf/libbpf.h| 133 tools/lib/bpf/libbpf.map | 17 + tools/lib/bpf/netlink.c | 752 +- tools/lib/bpf/nlattr.h| 43 + .../selftests/bpf/prog_tests/test_tc_bpf.c| 261 ++ .../selftests/bpf/progs/test_tc_bpf_kern.c| 18 + 8 files changed, 1476 insertions(+), 32 deletions(-) create mode 100644 tools/testing/selftests/bpf/prog_tests/test_tc_bpf.c create mode 100644 tools/testing/selftests/bpf/progs/test_tc_bpf_kern.c -- 2.30.2
[PATCH bpf-next v2] libbpf: use SOCK_CLOEXEC when opening the netlink socket
Otherwise, there exists a small window between the opening and closing
of the socket fd where it may leak into processes launched by some other
thread.
Fixes: 949abbe88436 ("libbpf: add function to setup XDP")
Signed-off-by: Kumar Kartikeya Dwivedi
---
Changelog:
v1 -> v2
Tag the bpf-next tree (Toke)
Add a Fixes: tag (Toke)
---
tools/lib/bpf/netlink.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/lib/bpf/netlink.c b/tools/lib/bpf/netlink.c
index 4dd73de00..d2cb28e9e 100644
--- a/tools/lib/bpf/netlink.c
+++ b/tools/lib/bpf/netlink.c
@@ -40,7 +40,7 @@ static int libbpf_netlink_open(__u32 *nl_pid)
memset(, 0, sizeof(sa));
sa.nl_family = AF_NETLINK;
- sock = socket(AF_NETLINK, SOCK_RAW, NETLINK_ROUTE);
+ sock = socket(AF_NETLINK, SOCK_RAW | SOCK_CLOEXEC, NETLINK_ROUTE);
if (sock < 0)
return -errno;
--
2.30.2
[PATCH] libbpf: use SOCK_CLOEXEC when opening the netlink socket
Otherwise, there exists a small window between the opening and closing of the socket fd where it may leak into processes launched by some other thread. Signed-off-by: Kumar Kartikeya Dwivedi --- tools/lib/bpf/netlink.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/lib/bpf/netlink.c b/tools/lib/bpf/netlink.c index 4dd73de00..d2cb28e9e 100644 --- a/tools/lib/bpf/netlink.c +++ b/tools/lib/bpf/netlink.c @@ -40,7 +40,7 @@ static int libbpf_netlink_open(__u32 *nl_pid) memset(, 0, sizeof(sa)); sa.nl_family = AF_NETLINK; - sock = socket(AF_NETLINK, SOCK_RAW, NETLINK_ROUTE); + sock = socket(AF_NETLINK, SOCK_RAW | SOCK_CLOEXEC, NETLINK_ROUTE); if (sock < 0) return -errno; -- 2.30.2
[PATCH] staging/greybus: eliminate use of NAME_SIZE for strings
Instead, depend on the size of the destination buffer for easier
refactoring.
Signed-off-by: Kumar Kartikeya Dwivedi
---
Hopefully, this is more thorough. The only cases left now are where the
destination string is represented by a pointer, otherwise all call sites with a
fixed sized buffer have been changed.
---
drivers/staging/greybus/audio_module.c | 4 ++--
drivers/staging/greybus/audio_topology.c | 12 ++--
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/greybus/audio_module.c
b/drivers/staging/greybus/audio_module.c
index 0f9fdc077..12c376c47 100644
--- a/drivers/staging/greybus/audio_module.c
+++ b/drivers/staging/greybus/audio_module.c
@@ -260,7 +260,7 @@ static int gb_audio_probe(struct gb_bundle *bundle,
INIT_LIST_HEAD(>widget_ctl_list);
INIT_LIST_HEAD(>jack_list);
gbmodule->dev = dev;
- snprintf(gbmodule->name, NAME_SIZE, "%s.%s", dev->driver->name,
+ snprintf(gbmodule->name, sizeof(gbmodule->name), "%s.%s",
dev->driver->name,
dev_name(dev));
greybus_set_drvdata(bundle, gbmodule);
@@ -342,7 +342,7 @@ static int gb_audio_probe(struct gb_bundle *bundle,
/* inform above layer for uevent */
dev_dbg(dev, "Inform set_event:%d to above layer\n", 1);
/* prepare for the audio manager */
- strscpy(desc.name, gbmodule->name, GB_AUDIO_MANAGER_MODULE_NAME_LEN);
+ strscpy(desc.name, gbmodule->name, sizeof(desc.name));
desc.vid = 2; /* todo */
desc.pid = 3; /* todo */
desc.intf_id = gbmodule->dev_id;
diff --git a/drivers/staging/greybus/audio_topology.c
b/drivers/staging/greybus/audio_topology.c
index e816e4db5..1fc7727ab 100644
--- a/drivers/staging/greybus/audio_topology.c
+++ b/drivers/staging/greybus/audio_topology.c
@@ -200,7 +200,7 @@ static int gbcodec_mixer_ctl_info(struct snd_kcontrol
*kcontrol,
return -EINVAL;
name = gbaudio_map_controlid(module, data->ctl_id,
uinfo->value.enumerated.item);
- strscpy(uinfo->value.enumerated.name, name, NAME_SIZE);
+ strscpy(uinfo->value.enumerated.name, name,
sizeof(uinfo->value.enumerated.name));
break;
default:
dev_err(comp->dev, "Invalid type: %d for %s:kcontrol\n",
@@ -363,7 +363,7 @@ static int gbcodec_mixer_dapm_ctl_info(struct snd_kcontrol
*kcontrol,
platform_min = le32_to_cpu(info->value.integer.min);
if (platform_max == 1 &&
- !strnstr(kcontrol->id.name, " Volume", NAME_SIZE))
+ !strnstr(kcontrol->id.name, " Volume", sizeof(kcontrol->id.name)))
uinfo->type = SNDRV_CTL_ELEM_TYPE_BOOLEAN;
else
uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
@@ -1047,8 +1047,8 @@ static int gbaudio_tplg_create_widget(struct
gbaudio_module_info *module,
}
/* Prefix dev_id to widget control_name */
- strscpy(temp_name, w->name, NAME_SIZE);
- snprintf(w->name, NAME_SIZE, "GB %d %s", module->dev_id, temp_name);
+ strscpy(temp_name, w->name, sizeof(temp_name));
+ snprintf(w->name, sizeof(w->name), "GB %d %s", module->dev_id,
temp_name);
switch (w->type) {
case snd_soc_dapm_spk:
@@ -1169,8 +1169,8 @@ static int gbaudio_tplg_process_kcontrols(struct
gbaudio_module_info *module,
}
control->id = curr->id;
/* Prefix dev_id to widget_name */
- strscpy(temp_name, curr->name, NAME_SIZE);
- snprintf(curr->name, NAME_SIZE, "GB %d %s", module->dev_id,
+ strscpy(temp_name, curr->name, sizeof(temp_name));
+ snprintf(curr->name, sizeof(curr->name), "GB %d %s",
module->dev_id,
temp_name);
control->name = curr->name;
if (curr->info.type == GB_AUDIO_CTL_ELEM_TYPE_ENUMERATED) {
--
2.29.2
Re: [PATCH 02/13] staging: greybus: Switch from strlcpy to strscpy
On Tue, Feb 16, 2021 at 08:24:59PM IST, Alex Elder wrote: > This is a good change. But while you're at it, I would > appreciate if you would convert a few spots to use > sizeof(dest) rather than a fixed constant. I will > point them out below. > > If this is the *only* request for a change on your > series, please tell me that and I can sign off on So far, this has been the only request. > this without you implementing my suggestion. But > if you post a version 2, please do what I describe. > I will incorporate these if I end up sending a v2. Alternatively, would a separate patch with your suggestions applied on top of this be acceptable, if I don't? -- Kartikeya
[PATCH v3] staging: emxx_udc: Make incorrectly defined global static
The global gpio_desc pointer and int vbus_irq were defined in the header, instead put the definitions in the translation unit and make them static as there's only a single consumer, and these symbols shouldn't pollute the global namespace. This fixes the following sparse warnings for this driver: drivers/staging/emxx_udc/emxx_udc.c: note: in included file: drivers/staging/emxx_udc/emxx_udc.h:23:18: warning: symbol 'vbus_gpio' was not declared. Should it be static? drivers/staging/emxx_udc/emxx_udc.h:24:5: warning: symbol 'vbus_irq' was not declared. Should it be static? Signed-off-by: Kumar Kartikeya Dwivedi --- Changes in v1: Switch to variable with static linkage instead of extern Changes in v2: Resend a versioned patch Changes in v3: Include version changelog below the marker --- drivers/staging/emxx_udc/emxx_udc.c | 3 +++ drivers/staging/emxx_udc/emxx_udc.h | 2 -- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/staging/emxx_udc/emxx_udc.c b/drivers/staging/emxx_udc/emxx_udc.c index a30b4f5b1..3536c03ff 100644 --- a/drivers/staging/emxx_udc/emxx_udc.c +++ b/drivers/staging/emxx_udc/emxx_udc.c @@ -34,6 +34,9 @@ #defineDRIVER_DESC "EMXX UDC driver" #defineDMA_ADDR_INVALID(~(dma_addr_t)0) +static struct gpio_desc *vbus_gpio; +static int vbus_irq; + static const char driver_name[] = "emxx_udc"; static const char driver_desc[] = DRIVER_DESC; diff --git a/drivers/staging/emxx_udc/emxx_udc.h b/drivers/staging/emxx_udc/emxx_udc.h index bca614d69..c9e37a1b8 100644 --- a/drivers/staging/emxx_udc/emxx_udc.h +++ b/drivers/staging/emxx_udc/emxx_udc.h @@ -20,8 +20,6 @@ /* below hacked up for staging integration */ #define GPIO_VBUS 0 /* GPIO_P153 on KZM9D */ #define INT_VBUS 0 /* IRQ for GPIO_P153 */ -struct gpio_desc *vbus_gpio; -int vbus_irq; /* Board dependence(Wait) */ -- 2.29.2
[PATCH v2] staging: emxx_udc: Make incorrectly defined global static
The global gpio_desc pointer and int vbus_irq were defined in the header, instead put the definitions in the translation unit and make them static as there's only a single consumer, and these symbols shouldn't pollute the global namespace. This fixes the following sparse warnings for this driver: drivers/staging/emxx_udc/emxx_udc.c: note: in included file: drivers/staging/emxx_udc/emxx_udc.h:23:18: warning: symbol 'vbus_gpio' was not declared. Should it be static? drivers/staging/emxx_udc/emxx_udc.h:24:5: warning: symbol 'vbus_irq' was not declared. Should it be static? Signed-off-by: Kumar Kartikeya Dwivedi --- drivers/staging/emxx_udc/emxx_udc.c | 3 +++ drivers/staging/emxx_udc/emxx_udc.h | 2 -- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/staging/emxx_udc/emxx_udc.c b/drivers/staging/emxx_udc/emxx_udc.c index a30b4f5b1..3536c03ff 100644 --- a/drivers/staging/emxx_udc/emxx_udc.c +++ b/drivers/staging/emxx_udc/emxx_udc.c @@ -34,6 +34,9 @@ #defineDRIVER_DESC "EMXX UDC driver" #defineDMA_ADDR_INVALID(~(dma_addr_t)0) +static struct gpio_desc *vbus_gpio; +static int vbus_irq; + static const char driver_name[] = "emxx_udc"; static const char driver_desc[] = DRIVER_DESC; diff --git a/drivers/staging/emxx_udc/emxx_udc.h b/drivers/staging/emxx_udc/emxx_udc.h index bca614d69..c9e37a1b8 100644 --- a/drivers/staging/emxx_udc/emxx_udc.h +++ b/drivers/staging/emxx_udc/emxx_udc.h @@ -20,8 +20,6 @@ /* below hacked up for staging integration */ #define GPIO_VBUS 0 /* GPIO_P153 on KZM9D */ #define INT_VBUS 0 /* IRQ for GPIO_P153 */ -struct gpio_desc *vbus_gpio; -int vbus_irq; /* Board dependence(Wait) */ -- 2.29.2
Re: [PATCH] staging: emxx_udc: Fix incorrectly defined global
On Sun, Feb 07, 2021 at 12:04:41PM IST, Stephen Rothwell wrote: > > Given that drivers/staging/emxx_udc/emxx_udc.h is only included by > drivers/staging/emxx_udc/emxx_udc.c, shouldn't these variables just be > declared static in emxx_udc.c and removed from emxx_udc.h? > Either would be correct. I went this way because it was originally trying to (incorrectly) define a global variable instead. I guess they can be static now and when more users are added, the linkage can be adjusted as needed. Here's another version of the patch: -- >From 5835ad916ceeba620c85bc32f52ecd69f21f8dab Mon Sep 17 00:00:00 2001 From: Kumar Kartikeya Dwivedi Date: Sun, 7 Feb 2021 12:53:39 +0530 Subject: [PATCH] staging: emxx_udc: Make incorrectly defined global static The global gpio_desc pointer and int vbus_irq were defined in the header, instead put the definitions in the translation unit and make them static as there's only a single consumer in emxx_udc.c. This fixes the following sparse warnings for this driver: drivers/staging/emxx_udc/emxx_udc.c: note: in included file: drivers/staging/emxx_udc/emxx_udc.h:23:18: warning: symbol 'vbus_gpio' was not declared. Should it be static? drivers/staging/emxx_udc/emxx_udc.h:24:5: warning: symbol 'vbus_irq' was not declared. Should it be static? Signed-off-by: Kumar Kartikeya Dwivedi --- drivers/staging/emxx_udc/emxx_udc.c | 3 +++ drivers/staging/emxx_udc/emxx_udc.h | 2 -- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/staging/emxx_udc/emxx_udc.c b/drivers/staging/emxx_udc/emxx_udc.c index a30b4f5b1..3536c03ff 100644 --- a/drivers/staging/emxx_udc/emxx_udc.c +++ b/drivers/staging/emxx_udc/emxx_udc.c @@ -34,6 +34,9 @@ #defineDRIVER_DESC "EMXX UDC driver" #defineDMA_ADDR_INVALID(~(dma_addr_t)0) +static struct gpio_desc *vbus_gpio; +static int vbus_irq; + static const char driver_name[] = "emxx_udc"; static const char driver_desc[] = DRIVER_DESC; diff --git a/drivers/staging/emxx_udc/emxx_udc.h b/drivers/staging/emxx_udc/emxx_udc.h index bca614d69..c9e37a1b8 100644 --- a/drivers/staging/emxx_udc/emxx_udc.h +++ b/drivers/staging/emxx_udc/emxx_udc.h @@ -20,8 +20,6 @@ /* below hacked up for staging integration */ #define GPIO_VBUS 0 /* GPIO_P153 on KZM9D */ #define INT_VBUS 0 /* IRQ for GPIO_P153 */ -struct gpio_desc *vbus_gpio; -int vbus_irq; /* Board dependence(Wait) */ -- 2.29.2 -- Kartikeya
[PATCH] staging: emxx_udc: Fix incorrectly defined global
The global gpio_desc pointer and int were defined in the header, instead put the definitions in the translation unit and add an extern declaration for consumers of the header (currently only one, which is perhaps why the linker didn't complain about symbol collisions). This fixes sparse related warnings for this driver: drivers/staging/emxx_udc/emxx_udc.c: note: in included file: drivers/staging/emxx_udc/emxx_udc.h:23:18: warning: symbol 'vbus_gpio' was not declared. Should it be static? drivers/staging/emxx_udc/emxx_udc.h:24:5: warning: symbol 'vbus_irq' was not declared. Should it be static? Signed-off-by: Kumar Kartikeya Dwivedi --- drivers/staging/emxx_udc/emxx_udc.c | 3 +++ drivers/staging/emxx_udc/emxx_udc.h | 4 ++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/staging/emxx_udc/emxx_udc.c b/drivers/staging/emxx_udc/emxx_udc.c index a30b4f5b1..6983c3e31 100644 --- a/drivers/staging/emxx_udc/emxx_udc.c +++ b/drivers/staging/emxx_udc/emxx_udc.c @@ -34,6 +34,9 @@ #defineDRIVER_DESC "EMXX UDC driver" #defineDMA_ADDR_INVALID(~(dma_addr_t)0) +struct gpio_desc *vbus_gpio; +int vbus_irq; + static const char driver_name[] = "emxx_udc"; static const char driver_desc[] = DRIVER_DESC; diff --git a/drivers/staging/emxx_udc/emxx_udc.h b/drivers/staging/emxx_udc/emxx_udc.h index bca614d69..b3c4ccbe5 100644 --- a/drivers/staging/emxx_udc/emxx_udc.h +++ b/drivers/staging/emxx_udc/emxx_udc.h @@ -20,8 +20,8 @@ /* below hacked up for staging integration */ #define GPIO_VBUS 0 /* GPIO_P153 on KZM9D */ #define INT_VBUS 0 /* IRQ for GPIO_P153 */ -struct gpio_desc *vbus_gpio; -int vbus_irq; +extern struct gpio_desc *vbus_gpio; +extern int vbus_irq; /* Board dependence(Wait) */ -- 2.29.2
[PATCH 04/13] staging: most: Switch from strlcpy to strscpy
strlcpy is marked as deprecated in Documentation/process/deprecated.rst,
and there is no functional difference when the caller expects truncation
(when not checking the return value). strscpy is relatively better as it
also avoids scanning the whole source string.
This silences the related checkpatch warnings from:
5dbdb2d87c29 ("checkpatch: prefer strscpy to strlcpy")
Signed-off-by: Kumar Kartikeya Dwivedi
---
drivers/staging/most/sound/sound.c | 2 +-
drivers/staging/most/video/video.c | 6 +++---
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/most/sound/sound.c
b/drivers/staging/most/sound/sound.c
index 3a1a59058..c4287994b 100644
--- a/drivers/staging/most/sound/sound.c
+++ b/drivers/staging/most/sound/sound.c
@@ -525,7 +525,7 @@ static int audio_probe_channel(struct most_interface
*iface, int channel_id,
pr_err("Incompatible channel type\n");
return -EINVAL;
}
- strlcpy(arg_list_cpy, arg_list, STRING_SIZE);
+ strscpy(arg_list_cpy, arg_list, STRING_SIZE);
ret = split_arg_list(arg_list_cpy, _num, _res);
if (ret < 0)
return ret;
diff --git a/drivers/staging/most/video/video.c
b/drivers/staging/most/video/video.c
index 829df899b..90933d78c 100644
--- a/drivers/staging/most/video/video.c
+++ b/drivers/staging/most/video/video.c
@@ -245,8 +245,8 @@ static int vidioc_querycap(struct file *file, void *priv,
struct comp_fh *fh = priv;
struct most_video_dev *mdev = fh->mdev;
- strlcpy(cap->driver, "v4l2_component", sizeof(cap->driver));
- strlcpy(cap->card, "MOST", sizeof(cap->card));
+ strscpy(cap->driver, "v4l2_component", sizeof(cap->driver));
+ strscpy(cap->card, "MOST", sizeof(cap->card));
snprintf(cap->bus_info, sizeof(cap->bus_info),
"%s", mdev->iface->description);
return 0;
@@ -483,7 +483,7 @@ static int comp_probe_channel(struct most_interface *iface,
int channel_idx,
mdev->v4l2_dev.release = comp_v4l2_dev_release;
/* Create the v4l2_device */
- strlcpy(mdev->v4l2_dev.name, name, sizeof(mdev->v4l2_dev.name));
+ strscpy(mdev->v4l2_dev.name, name, sizeof(mdev->v4l2_dev.name));
ret = v4l2_device_register(NULL, >v4l2_dev);
if (ret) {
pr_err("v4l2_device_register() failed\n");
--
2.29.2
[PATCH 06/13] staging: octeon: Switch from strlcpy to strscpy
strlcpy is marked as deprecated in Documentation/process/deprecated.rst,
and there is no functional difference when the caller expects truncation
(when not checking the return value). strscpy is relatively better as it
also avoids scanning the whole source string.
This silences the related checkpatch warnings from:
5dbdb2d87c29 ("checkpatch: prefer strscpy to strlcpy")
Signed-off-by: Kumar Kartikeya Dwivedi
---
drivers/staging/octeon/ethernet-mdio.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/octeon/ethernet-mdio.c
b/drivers/staging/octeon/ethernet-mdio.c
index 0bf545849..1bb91a904 100644
--- a/drivers/staging/octeon/ethernet-mdio.c
+++ b/drivers/staging/octeon/ethernet-mdio.c
@@ -21,9 +21,9 @@
static void cvm_oct_get_drvinfo(struct net_device *dev,
struct ethtool_drvinfo *info)
{
- strlcpy(info->driver, KBUILD_MODNAME, sizeof(info->driver));
- strlcpy(info->version, UTS_RELEASE, sizeof(info->version));
- strlcpy(info->bus_info, "Builtin", sizeof(info->bus_info));
+ strscpy(info->driver, KBUILD_MODNAME, sizeof(info->driver));
+ strscpy(info->version, UTS_RELEASE, sizeof(info->version));
+ strscpy(info->bus_info, "Builtin", sizeof(info->bus_info));
}
static int cvm_oct_nway_reset(struct net_device *dev)
--
2.29.2
[PATCH 13/13] staging: wimax: Switch from strlcpy to strscpy
strlcpy is marked as deprecated in Documentation/process/deprecated.rst,
and there is no functional difference when the caller expects truncation
(when not checking the return value). strscpy is relatively better as it
also avoids scanning the whole source string.
This silences the related checkpatch warnings from:
5dbdb2d87c29 ("checkpatch: prefer strscpy to strlcpy")
Signed-off-by: Kumar Kartikeya Dwivedi
---
drivers/staging/wimax/i2400m/netdev.c | 6 +++---
drivers/staging/wimax/i2400m/usb.c| 4 ++--
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/staging/wimax/i2400m/netdev.c
b/drivers/staging/wimax/i2400m/netdev.c
index 8339d600e..cd06eaf75 100644
--- a/drivers/staging/wimax/i2400m/netdev.c
+++ b/drivers/staging/wimax/i2400m/netdev.c
@@ -561,11 +561,11 @@ static void i2400m_get_drvinfo(struct net_device *net_dev,
{
struct i2400m *i2400m = net_dev_to_i2400m(net_dev);
- strlcpy(info->driver, KBUILD_MODNAME, sizeof(info->driver));
- strlcpy(info->fw_version, i2400m->fw_name ? : "",
+ strscpy(info->driver, KBUILD_MODNAME, sizeof(info->driver));
+ strscpy(info->fw_version, i2400m->fw_name ? : "",
sizeof(info->fw_version));
if (net_dev->dev.parent)
- strlcpy(info->bus_info, dev_name(net_dev->dev.parent),
+ strscpy(info->bus_info, dev_name(net_dev->dev.parent),
sizeof(info->bus_info));
}
diff --git a/drivers/staging/wimax/i2400m/usb.c
b/drivers/staging/wimax/i2400m/usb.c
index f250d03ce..481b1ccde 100644
--- a/drivers/staging/wimax/i2400m/usb.c
+++ b/drivers/staging/wimax/i2400m/usb.c
@@ -333,8 +333,8 @@ static void i2400mu_get_drvinfo(struct net_device *net_dev,
struct i2400mu *i2400mu = container_of(i2400m, struct i2400mu, i2400m);
struct usb_device *udev = i2400mu->usb_dev;
- strlcpy(info->driver, KBUILD_MODNAME, sizeof(info->driver));
- strlcpy(info->fw_version, i2400m->fw_name ? : "",
+ strscpy(info->driver, KBUILD_MODNAME, sizeof(info->driver));
+ strscpy(info->fw_version, i2400m->fw_name ? : "",
sizeof(info->fw_version));
usb_make_path(udev, info->bus_info, sizeof(info->bus_info));
}
--
2.29.2
[PATCH 05/13] staging: nvec: Switch from strlcpy to strscpy
strlcpy is marked as deprecated in Documentation/process/deprecated.rst,
and there is no functional difference when the caller expects truncation
(when not checking the return value). strscpy is relatively better as it
also avoids scanning the whole source string.
This silences the related checkpatch warnings from:
5dbdb2d87c29 ("checkpatch: prefer strscpy to strlcpy")
Signed-off-by: Kumar Kartikeya Dwivedi
---
drivers/staging/nvec/nvec_ps2.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/nvec/nvec_ps2.c b/drivers/staging/nvec/nvec_ps2.c
index 45db29262..157009015 100644
--- a/drivers/staging/nvec/nvec_ps2.c
+++ b/drivers/staging/nvec/nvec_ps2.c
@@ -112,8 +112,8 @@ static int nvec_mouse_probe(struct platform_device *pdev)
ser_dev->start = ps2_startstreaming;
ser_dev->stop = ps2_stopstreaming;
- strlcpy(ser_dev->name, "nvec mouse", sizeof(ser_dev->name));
- strlcpy(ser_dev->phys, "nvec", sizeof(ser_dev->phys));
+ strscpy(ser_dev->name, "nvec mouse", sizeof(ser_dev->name));
+ strscpy(ser_dev->phys, "nvec", sizeof(ser_dev->phys));
ps2_dev.ser_dev = ser_dev;
ps2_dev.notifier.notifier_call = nvec_ps2_notifier;
--
2.29.2
[PATCH 01/13] staging: comedi: Switch from strlcpy to strscpy
strlcpy is marked as deprecated in Documentation/process/deprecated.rst,
and there is no functional difference when the caller expects truncation
(when not checking the return value). strscpy is relatively better as it
also avoids scanning the whole source string.
This silences the related checkpatch warnings from:
5dbdb2d87c29 ("checkpatch: prefer strscpy to strlcpy")
Signed-off-by: Kumar Kartikeya Dwivedi
---
drivers/staging/comedi/comedi_fops.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/comedi/comedi_fops.c
b/drivers/staging/comedi/comedi_fops.c
index 80d74cce2..df77b6bf5 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -939,8 +939,8 @@ static int do_devinfo_ioctl(struct comedi_device *dev,
/* fill devinfo structure */
devinfo.version_code = COMEDI_VERSION_CODE;
devinfo.n_subdevs = dev->n_subdevices;
- strlcpy(devinfo.driver_name, dev->driver->driver_name, COMEDI_NAMELEN);
- strlcpy(devinfo.board_name, dev->board_name, COMEDI_NAMELEN);
+ strscpy(devinfo.driver_name, dev->driver->driver_name, COMEDI_NAMELEN);
+ strscpy(devinfo.board_name, dev->board_name, COMEDI_NAMELEN);
s = comedi_file_read_subdevice(file);
if (s)
--
2.29.2
[PATCH 02/13] staging: greybus: Switch from strlcpy to strscpy
strlcpy is marked as deprecated in Documentation/process/deprecated.rst,
and there is no functional difference when the caller expects truncation
(when not checking the return value). strscpy is relatively better as it
also avoids scanning the whole source string.
This silences the related checkpatch warnings from:
5dbdb2d87c29 ("checkpatch: prefer strscpy to strlcpy")
Signed-off-by: Kumar Kartikeya Dwivedi
---
drivers/staging/greybus/audio_helper.c | 2 +-
drivers/staging/greybus/audio_module.c | 2 +-
drivers/staging/greybus/audio_topology.c | 6 +++---
drivers/staging/greybus/power_supply.c | 2 +-
drivers/staging/greybus/spilib.c | 4 ++--
5 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/greybus/audio_helper.c
b/drivers/staging/greybus/audio_helper.c
index 3011b8abc..1ed4772d2 100644
--- a/drivers/staging/greybus/audio_helper.c
+++ b/drivers/staging/greybus/audio_helper.c
@@ -166,7 +166,7 @@ static int gbaudio_remove_controls(struct snd_card *card,
struct device *dev,
snprintf(id.name, sizeof(id.name), "%s %s", prefix,
control->name);
else
- strlcpy(id.name, control->name, sizeof(id.name));
+ strscpy(id.name, control->name, sizeof(id.name));
id.numid = 0;
id.iface = control->iface;
id.device = control->device;
diff --git a/drivers/staging/greybus/audio_module.c
b/drivers/staging/greybus/audio_module.c
index a243d60f0..0f9fdc077 100644
--- a/drivers/staging/greybus/audio_module.c
+++ b/drivers/staging/greybus/audio_module.c
@@ -342,7 +342,7 @@ static int gb_audio_probe(struct gb_bundle *bundle,
/* inform above layer for uevent */
dev_dbg(dev, "Inform set_event:%d to above layer\n", 1);
/* prepare for the audio manager */
- strlcpy(desc.name, gbmodule->name, GB_AUDIO_MANAGER_MODULE_NAME_LEN);
+ strscpy(desc.name, gbmodule->name, GB_AUDIO_MANAGER_MODULE_NAME_LEN);
desc.vid = 2; /* todo */
desc.pid = 3; /* todo */
desc.intf_id = gbmodule->dev_id;
diff --git a/drivers/staging/greybus/audio_topology.c
b/drivers/staging/greybus/audio_topology.c
index 662e3e8b4..e816e4db5 100644
--- a/drivers/staging/greybus/audio_topology.c
+++ b/drivers/staging/greybus/audio_topology.c
@@ -200,7 +200,7 @@ static int gbcodec_mixer_ctl_info(struct snd_kcontrol
*kcontrol,
return -EINVAL;
name = gbaudio_map_controlid(module, data->ctl_id,
uinfo->value.enumerated.item);
- strlcpy(uinfo->value.enumerated.name, name, NAME_SIZE);
+ strscpy(uinfo->value.enumerated.name, name, NAME_SIZE);
break;
default:
dev_err(comp->dev, "Invalid type: %d for %s:kcontrol\n",
@@ -1047,7 +1047,7 @@ static int gbaudio_tplg_create_widget(struct
gbaudio_module_info *module,
}
/* Prefix dev_id to widget control_name */
- strlcpy(temp_name, w->name, NAME_SIZE);
+ strscpy(temp_name, w->name, NAME_SIZE);
snprintf(w->name, NAME_SIZE, "GB %d %s", module->dev_id, temp_name);
switch (w->type) {
@@ -1169,7 +1169,7 @@ static int gbaudio_tplg_process_kcontrols(struct
gbaudio_module_info *module,
}
control->id = curr->id;
/* Prefix dev_id to widget_name */
- strlcpy(temp_name, curr->name, NAME_SIZE);
+ strscpy(temp_name, curr->name, NAME_SIZE);
snprintf(curr->name, NAME_SIZE, "GB %d %s", module->dev_id,
temp_name);
control->name = curr->name;
diff --git a/drivers/staging/greybus/power_supply.c
b/drivers/staging/greybus/power_supply.c
index ec96f2888..75cb170e0 100644
--- a/drivers/staging/greybus/power_supply.c
+++ b/drivers/staging/greybus/power_supply.c
@@ -449,7 +449,7 @@ static int __gb_power_supply_set_name(char *init_name, char
*name, size_t len)
if (!strlen(init_name))
init_name = "gb_power_supply";
- strlcpy(name, init_name, len);
+ strscpy(name, init_name, len);
while ((ret < len) && (psy = power_supply_get_by_name(name))) {
power_supply_put(psy);
diff --git a/drivers/staging/greybus/spilib.c b/drivers/staging/greybus/spilib.c
index fc27c52de..672d540d3 100644
--- a/drivers/staging/greybus/spilib.c
+++ b/drivers/staging/greybus/spilib.c
@@ -455,10 +455,10 @@ static int gb_spi_setup_device(struct gb_spilib *spi, u8
cs)
dev_type = response.device_type;
if (dev_type == GB_SPI_SPI_DEV)
- strlcpy(spi_board.modalias, "spidev",
+ strscpy(spi_board.modalias, "spidev"
[PATCH 12/13] staging: sm750fb: Switch from strlcpy to strscpy
strlcpy is marked as deprecated in Documentation/process/deprecated.rst,
and there is no functional difference when the caller expects truncation
(when not checking the return value). strscpy is relatively better as it
also avoids scanning the whole source string.
This silences the related checkpatch warnings from:
5dbdb2d87c29 ("checkpatch: prefer strscpy to strlcpy")
Signed-off-by: Kumar Kartikeya Dwivedi
---
drivers/staging/sm750fb/sm750.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/sm750.c
index 029f0d09e..c237a8f8e 100644
--- a/drivers/staging/sm750fb/sm750.c
+++ b/drivers/staging/sm750fb/sm750.c
@@ -814,7 +814,7 @@ static int lynxfb_set_fbinfo(struct fb_info *info, int
index)
fix->ywrapstep = crtc->ywrapstep;
fix->accel = FB_ACCEL_SMI;
- strlcpy(fix->id, fixId[index], sizeof(fix->id));
+ strscpy(fix->id, fixId[index], sizeof(fix->id));
fix->smem_start = crtc->oScreen + sm750_dev->vidmem_start;
pr_info("fix->smem_start = %lx\n", fix->smem_start);
--
2.29.2
[PATCH 07/13] staging: olpc_dcon: Switch from strlcpy to strscpy
strlcpy is marked as deprecated in Documentation/process/deprecated.rst,
and there is no functional difference when the caller expects truncation
(when not checking the return value). strscpy is relatively better as it
also avoids scanning the whole source string.
This silences the related checkpatch warnings from:
5dbdb2d87c29 ("checkpatch: prefer strscpy to strlcpy")
Signed-off-by: Kumar Kartikeya Dwivedi
---
drivers/staging/olpc_dcon/olpc_dcon.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/olpc_dcon/olpc_dcon.c
b/drivers/staging/olpc_dcon/olpc_dcon.c
index e7281212d..6d8e9a481 100644
--- a/drivers/staging/olpc_dcon/olpc_dcon.c
+++ b/drivers/staging/olpc_dcon/olpc_dcon.c
@@ -576,7 +576,7 @@ static struct notifier_block dcon_panic_nb = {
static int dcon_detect(struct i2c_client *client, struct i2c_board_info *info)
{
- strlcpy(info->type, "olpc_dcon", I2C_NAME_SIZE);
+ strscpy(info->type, "olpc_dcon", I2C_NAME_SIZE);
return 0;
}
--
2.29.2
[PATCH 03/13] staging: fsl-dpaa2: Switch from strlcpy to strscpy
strlcpy is marked as deprecated in Documentation/process/deprecated.rst,
and there is no functional difference when the caller expects truncation
(when not checking the return value). strscpy is relatively better as it
also avoids scanning the whole source string.
This silences the related checkpatch warnings from:
5dbdb2d87c29 ("checkpatch: prefer strscpy to strlcpy")
Signed-off-by: Kumar Kartikeya Dwivedi
---
drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c
b/drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c
index d7f4ed1df..0af2e9914 100644
--- a/drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c
+++ b/drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c
@@ -38,19 +38,19 @@ static void dpaa2_switch_get_drvinfo(struct net_device
*netdev,
u16 version_major, version_minor;
int err;
- strlcpy(drvinfo->driver, KBUILD_MODNAME, sizeof(drvinfo->driver));
+ strscpy(drvinfo->driver, KBUILD_MODNAME, sizeof(drvinfo->driver));
err = dpsw_get_api_version(port_priv->ethsw_data->mc_io, 0,
_major,
_minor);
if (err)
- strlcpy(drvinfo->fw_version, "N/A",
+ strscpy(drvinfo->fw_version, "N/A",
sizeof(drvinfo->fw_version));
else
snprintf(drvinfo->fw_version, sizeof(drvinfo->fw_version),
"%u.%u", version_major, version_minor);
- strlcpy(drvinfo->bus_info, dev_name(netdev->dev.parent->parent),
+ strscpy(drvinfo->bus_info, dev_name(netdev->dev.parent->parent),
sizeof(drvinfo->bus_info));
}
--
2.29.2
[PATCH 08/13] staging: rtl8188eu: Switch from strlcpy to strscpy
strlcpy is marked as deprecated in Documentation/process/deprecated.rst,
and there is no functional difference when the caller expects truncation
(when not checking the return value). strscpy is relatively better as it
also avoids scanning the whole source string.
This silences the related checkpatch warnings from:
5dbdb2d87c29 ("checkpatch: prefer strscpy to strlcpy")
Signed-off-by: Kumar Kartikeya Dwivedi
---
drivers/staging/rtl8188eu/os_dep/ioctl_linux.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
b/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
index 6f42f13a7..bf22f130d 100644
--- a/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
+++ b/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
@@ -1865,7 +1865,7 @@ static int rtw_wx_set_enc_ext(struct net_device *dev,
goto exit;
}
- strlcpy((char *)param->u.crypt.alg, alg_name, IEEE_CRYPT_ALG_NAME_LEN);
+ strscpy((char *)param->u.crypt.alg, alg_name, IEEE_CRYPT_ALG_NAME_LEN);
if (pext->ext_flags & IW_ENCODE_EXT_SET_TX_KEY)
param->u.crypt.set_tx = 1;
--
2.29.2
[PATCH 11/13] staging: rtl8712: Switch from strlcpy to strscpy
strlcpy is marked as deprecated in Documentation/process/deprecated.rst,
and there is no functional difference when the caller expects truncation
(when not checking the return value). strscpy is relatively better as it
also avoids scanning the whole source string.
This silences the related checkpatch warnings from:
5dbdb2d87c29 ("checkpatch: prefer strscpy to strlcpy")
Signed-off-by: Kumar Kartikeya Dwivedi
---
drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
index cbaa7a489..81de5a9e6 100644
--- a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
+++ b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
@@ -1784,7 +1784,7 @@ static int r871x_wx_set_enc_ext(struct net_device *dev,
return -ENOMEM;
param->cmd = IEEE_CMD_SET_ENCRYPTION;
eth_broadcast_addr(param->sta_addr);
- strlcpy((char *)param->u.crypt.alg, alg_name, IEEE_CRYPT_ALG_NAME_LEN);
+ strscpy((char *)param->u.crypt.alg, alg_name, IEEE_CRYPT_ALG_NAME_LEN);
if (pext->ext_flags & IW_ENCODE_EXT_GROUP_KEY)
param->u.crypt.set_tx = 0;
if (pext->ext_flags & IW_ENCODE_EXT_SET_TX_KEY)
--
2.29.2
[PATCH 09/13] staging: rtl8192e: Switch from strlcpy to strscpy
strlcpy is marked as deprecated in Documentation/process/deprecated.rst,
and there is no functional difference when the caller expects truncation
(when not checking the return value). strscpy is relatively better as it
also avoids scanning the whole source string.
This silences the related checkpatch warnings from:
5dbdb2d87c29 ("checkpatch: prefer strscpy to strlcpy")
Signed-off-by: Kumar Kartikeya Dwivedi
---
drivers/staging/rtl8192e/rtl8192e/rtl_ethtool.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/rtl8192e/rtl8192e/rtl_ethtool.c
b/drivers/staging/rtl8192e/rtl8192e/rtl_ethtool.c
index 6ae7a67e7..f4f7b74c8 100644
--- a/drivers/staging/rtl8192e/rtl8192e/rtl_ethtool.c
+++ b/drivers/staging/rtl8192e/rtl8192e/rtl_ethtool.c
@@ -18,9 +18,9 @@ static void _rtl92e_ethtool_get_drvinfo(struct net_device
*dev,
{
struct r8192_priv *priv = rtllib_priv(dev);
- strlcpy(info->driver, DRV_NAME, sizeof(info->driver));
- strlcpy(info->version, DRV_VERSION, sizeof(info->version));
- strlcpy(info->bus_info, pci_name(priv->pdev), sizeof(info->bus_info));
+ strscpy(info->driver, DRV_NAME, sizeof(info->driver));
+ strscpy(info->version, DRV_VERSION, sizeof(info->version));
+ strscpy(info->bus_info, pci_name(priv->pdev), sizeof(info->bus_info));
}
static u32 _rtl92e_ethtool_get_link(struct net_device *dev)
--
2.29.2
[PATCH 10/13] staging: rtl8192u: Switch from strlcpy to strscpy
strlcpy is marked as deprecated in Documentation/process/deprecated.rst,
and there is no functional difference when the caller expects truncation
(when not checking the return value). strscpy is relatively better as it
also avoids scanning the whole source string.
This silences the related checkpatch warnings from:
5dbdb2d87c29 ("checkpatch: prefer strscpy to strlcpy")
Signed-off-by: Kumar Kartikeya Dwivedi
---
drivers/staging/rtl8192u/ieee80211/ieee80211_softmac_wx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac_wx.c
b/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac_wx.c
index f434a26cd..afa92ddfa 100644
--- a/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac_wx.c
+++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac_wx.c
@@ -484,7 +484,7 @@ int ieee80211_wx_get_name(struct ieee80211_device *ieee,
struct iw_request_info *info,
union iwreq_data *wrqu, char *extra)
{
- strlcpy(wrqu->name, "802.11", IFNAMSIZ);
+ strscpy(wrqu->name, "802.11", IFNAMSIZ);
if (ieee->modulation & IEEE80211_CCK_MODULATION) {
strlcat(wrqu->name, "b", IFNAMSIZ);
if (ieee->modulation & IEEE80211_OFDM_MODULATION)
--
2.29.2
[PATCH 00/13] Convert all users of strlcpy to strscpy
This series converts all existing users of strlcpy in drivers/staging to use
strscpy instead.
strlcpy is marked as deprecated in Documentation/process/deprecated.rst, and
there is no functional difference when the caller expects truncation (when not
checking the return value). strscpy is relatively better as it also avoids
scanning the whole source string.
This silences the related checkpatch warnings from:
5dbdb2d87c29 ("checkpatch: prefer strscpy to strlcpy")
The conversions were performed in two steps:
1. The following coccinelle script was used to identify and replace call sites
that expect truncation.
$ spatch --sp-file strscpy.cocci --include-headers --dir --in-place
drivers/staging
@strscpy@
expression dest, src, size;
@@
-strlcpy(dest, src, size);
+strscpy(dest, src, size);
2. Each individual automated conversion was rechecked manually for correctness.
Kumar Kartikeya Dwivedi (13):
staging: comedi: Switch from strlcpy to strscpy
staging: greybus: Switch from strlcpy to strscpy
staging: fsl-dpaa2: Switch from strlcpy to strscpy
staging: most: Switch from strlcpy to strscpy
staging: nvec: Switch from strlcpy to strscpy
staging: octeon: Switch from strlcpy to strscpy
staging: olpc_dcon: Switch from strlcpy to strscpy
staging: rtl8188eu: Switch from strlcpy to strscpy
staging: rtl8192e: Switch from strlcpy to strscpy
staging: rtl8192u: Switch from strlcpy to strscpy
staging: rtl8712: Switch from strlcpy to strscpy
staging: sm750fb: Switch from strlcpy to strscpy
staging: wimax: Switch from strlcpy to strscpy
drivers/staging/comedi/comedi_fops.c | 4 ++--
drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c | 6 +++---
drivers/staging/greybus/audio_helper.c| 2 +-
drivers/staging/greybus/audio_module.c| 2 +-
drivers/staging/greybus/audio_topology.c | 6 +++---
drivers/staging/greybus/power_supply.c| 2 +-
drivers/staging/greybus/spilib.c | 4 ++--
drivers/staging/most/sound/sound.c| 2 +-
drivers/staging/most/video/video.c| 6 +++---
drivers/staging/nvec/nvec_ps2.c | 4 ++--
drivers/staging/octeon/ethernet-mdio.c| 6 +++---
drivers/staging/olpc_dcon/olpc_dcon.c | 2 +-
drivers/staging/rtl8188eu/os_dep/ioctl_linux.c| 2 +-
drivers/staging/rtl8192e/rtl8192e/rtl_ethtool.c | 6 +++---
drivers/staging/rtl8192u/ieee80211/ieee80211_softmac_wx.c | 2 +-
drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 2 +-
drivers/staging/sm750fb/sm750.c | 2 +-
drivers/staging/wimax/i2400m/netdev.c | 6 +++---
drivers/staging/wimax/i2400m/usb.c| 4 ++--
19 files changed, 35 insertions(+), 35 deletions(-)
--
2.29.2
[PATCH v2] staging: qlge/qlge_ethtool.c: Switch from strlcpy to strscpy
strlcpy is marked as deprecated in Documentation/process/deprecated.rst,
and there is no functional difference when the caller expects truncation
(when not checking the return value). strscpy is relatively better as it
also avoids scanning the whole source string.
This silences the related checkpatch warnings from:
5dbdb2d87c29 ("checkpatch: prefer strscpy to strlcpy")
Signed-off-by: Kumar Kartikeya Dwivedi
---
drivers/staging/qlge/qlge_ethtool.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/qlge/qlge_ethtool.c
b/drivers/staging/qlge/qlge_ethtool.c
index a28f0254c..635d3338f 100644
--- a/drivers/staging/qlge/qlge_ethtool.c
+++ b/drivers/staging/qlge/qlge_ethtool.c
@@ -417,15 +417,15 @@ static void ql_get_drvinfo(struct net_device *ndev,
{
struct ql_adapter *qdev = netdev_priv(ndev);
- strlcpy(drvinfo->driver, qlge_driver_name, sizeof(drvinfo->driver));
- strlcpy(drvinfo->version, qlge_driver_version,
+ strscpy(drvinfo->driver, qlge_driver_name, sizeof(drvinfo->driver));
+ strscpy(drvinfo->version, qlge_driver_version,
sizeof(drvinfo->version));
snprintf(drvinfo->fw_version, sizeof(drvinfo->fw_version),
"v%d.%d.%d",
(qdev->fw_rev_id & 0x00ff) >> 16,
(qdev->fw_rev_id & 0xff00) >> 8,
(qdev->fw_rev_id & 0x00ff));
- strlcpy(drvinfo->bus_info, pci_name(qdev->pdev),
+ strscpy(drvinfo->bus_info, pci_name(qdev->pdev),
sizeof(drvinfo->bus_info));
}
--
2.29.2
[PATCH] staging: qlge/qlge_ethtool.c: strlcpy -> strscpy
Fixes checkpatch warnings for usage of strlcpy.
Signed-off-by: Kumar Kartikeya Dwivedi
---
drivers/staging/qlge/qlge_ethtool.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/qlge/qlge_ethtool.c
b/drivers/staging/qlge/qlge_ethtool.c
index a28f0254c..635d3338f 100644
--- a/drivers/staging/qlge/qlge_ethtool.c
+++ b/drivers/staging/qlge/qlge_ethtool.c
@@ -417,15 +417,15 @@ static void ql_get_drvinfo(struct net_device *ndev,
{
struct ql_adapter *qdev = netdev_priv(ndev);
- strlcpy(drvinfo->driver, qlge_driver_name, sizeof(drvinfo->driver));
- strlcpy(drvinfo->version, qlge_driver_version,
+ strscpy(drvinfo->driver, qlge_driver_name, sizeof(drvinfo->driver));
+ strscpy(drvinfo->version, qlge_driver_version,
sizeof(drvinfo->version));
snprintf(drvinfo->fw_version, sizeof(drvinfo->fw_version),
"v%d.%d.%d",
(qdev->fw_rev_id & 0x00ff) >> 16,
(qdev->fw_rev_id & 0xff00) >> 8,
(qdev->fw_rev_id & 0x00ff));
- strlcpy(drvinfo->bus_info, pci_name(qdev->pdev),
+ strscpy(drvinfo->bus_info, pci_name(qdev->pdev),
sizeof(drvinfo->bus_info));
}
--
2.29.2
