Merge branch 'fix-sockmap-flow_dissector-uapi'

Lorenz Bauer says:

====================
Both sockmap and flow_dissector ingnore various arguments passed to
BPF_PROG_ATTACH and BPF_PROG_DETACH. We can fix the attach case by
checking that the unused arguments are zero. I considered requiring
target_fd to be -1 instead of 0, but this leads to a lot of churn
in selftests. There is also precedent in that bpf_iter already
expects 0 for a similar field. I think that we can come up with a
work around for fd 0 should we need to in the future.

The detach case is more problematic: both cgroups and lirc2 verify
that attach_bpf_fd matches the currently attached program. This
way you need access to the program fd to be able to remove it.
Neither sockmap nor flow_dissector do this. flow_dissector even
has a check for CAP_NET_ADMIN because of this. The patch set
addresses this by implementing the desired behaviour.

There is a possibility for user space breakage: any callers that
don't provide the correct fd will fail with ENOENT. For sockmap
the risk is low: even the selftests assume that sockmap works
the way I described. For flow_dissector the story is less
straightforward, and the selftests use a variety of arguments.

I've includes fixes tags for the oldest commits that allow an easy
backport, however the behaviour dates back to when sockmap and
flow_dissector were introduced. What is the best way to handle these?

This set is based on top of Jakub's work "bpf, netns: Prepare
for multi-prog attachment" available at
https://lore.kernel.org/bpf/87k0zwmhtb.fsf@cloudflare.com/T/

Since v1:
- Adjust selftests
- Implement detach behaviour
====================

Signed-off-by: Alexei Starovoitov <ast@kernel.org>
This commit is contained in:
Alexei Starovoitov 2020-06-30 10:46:39 -07:00
commit 084af57c51
8 changed files with 103 additions and 25 deletions

View File

@ -33,7 +33,7 @@ int netns_bpf_prog_query(const union bpf_attr *attr,
union bpf_attr __user *uattr);
int netns_bpf_prog_attach(const union bpf_attr *attr,
struct bpf_prog *prog);
int netns_bpf_prog_detach(const union bpf_attr *attr);
int netns_bpf_prog_detach(const union bpf_attr *attr, enum bpf_prog_type ptype);
int netns_bpf_link_create(const union bpf_attr *attr,
struct bpf_prog *prog);
#else
@ -49,7 +49,8 @@ static inline int netns_bpf_prog_attach(const union bpf_attr *attr,
return -EOPNOTSUPP;
}
static inline int netns_bpf_prog_detach(const union bpf_attr *attr)
static inline int netns_bpf_prog_detach(const union bpf_attr *attr,
enum bpf_prog_type ptype)
{
return -EOPNOTSUPP;
}

View File

@ -1543,13 +1543,16 @@ static inline void bpf_map_offload_map_free(struct bpf_map *map)
#endif /* CONFIG_NET && CONFIG_BPF_SYSCALL */
#if defined(CONFIG_BPF_STREAM_PARSER)
int sock_map_prog_update(struct bpf_map *map, struct bpf_prog *prog, u32 which);
int sock_map_prog_update(struct bpf_map *map, struct bpf_prog *prog,
struct bpf_prog *old, u32 which);
int sock_map_get_from_fd(const union bpf_attr *attr, struct bpf_prog *prog);
int sock_map_prog_detach(const union bpf_attr *attr, enum bpf_prog_type ptype);
void sock_map_unhash(struct sock *sk);
void sock_map_close(struct sock *sk, long timeout);
#else
static inline int sock_map_prog_update(struct bpf_map *map,
struct bpf_prog *prog, u32 which)
struct bpf_prog *prog,
struct bpf_prog *old, u32 which)
{
return -EOPNOTSUPP;
}
@ -1559,6 +1562,12 @@ static inline int sock_map_get_from_fd(const union bpf_attr *attr,
{
return -EINVAL;
}
static inline int sock_map_prog_detach(const union bpf_attr *attr,
enum bpf_prog_type ptype)
{
return -EOPNOTSUPP;
}
#endif /* CONFIG_BPF_STREAM_PARSER */
#if defined(CONFIG_INET) && defined(CONFIG_BPF_SYSCALL)

View File

@ -430,6 +430,19 @@ static inline void psock_set_prog(struct bpf_prog **pprog,
bpf_prog_put(prog);
}
static inline int psock_replace_prog(struct bpf_prog **pprog,
struct bpf_prog *prog,
struct bpf_prog *old)
{
if (cmpxchg(pprog, old, prog) != old)
return -ENOENT;
if (old)
bpf_prog_put(old);
return 0;
}
static inline void psock_progs_drop(struct sk_psock_progs *progs)
{
psock_set_prog(&progs->msg_parser, NULL);

View File

@ -209,6 +209,9 @@ int netns_bpf_prog_attach(const union bpf_attr *attr, struct bpf_prog *prog)
struct net *net;
int ret;
if (attr->target_fd || attr->attach_flags || attr->replace_bpf_fd)
return -EINVAL;
type = to_netns_bpf_attach_type(attr->attach_type);
if (type < 0)
return -EINVAL;
@ -266,7 +269,8 @@ int netns_bpf_prog_attach(const union bpf_attr *attr, struct bpf_prog *prog)
/* Must be called with netns_bpf_mutex held. */
static int __netns_bpf_prog_detach(struct net *net,
enum netns_bpf_attach_type type)
enum netns_bpf_attach_type type,
struct bpf_prog *old)
{
struct bpf_prog *attached;
@ -275,7 +279,7 @@ static int __netns_bpf_prog_detach(struct net *net,
return -EINVAL;
attached = net->bpf.progs[type];
if (!attached)
if (!attached || attached != old)
return -ENOENT;
netns_bpf_run_array_detach(net, type);
net->bpf.progs[type] = NULL;
@ -283,19 +287,29 @@ static int __netns_bpf_prog_detach(struct net *net,
return 0;
}
int netns_bpf_prog_detach(const union bpf_attr *attr)
int netns_bpf_prog_detach(const union bpf_attr *attr, enum bpf_prog_type ptype)
{
enum netns_bpf_attach_type type;
struct bpf_prog *prog;
int ret;
if (attr->target_fd)
return -EINVAL;
type = to_netns_bpf_attach_type(attr->attach_type);
if (type < 0)
return -EINVAL;
prog = bpf_prog_get_type(attr->attach_bpf_fd, ptype);
if (IS_ERR(prog))
return PTR_ERR(prog);
mutex_lock(&netns_bpf_mutex);
ret = __netns_bpf_prog_detach(current->nsproxy->net_ns, type);
ret = __netns_bpf_prog_detach(current->nsproxy->net_ns, type, prog);
mutex_unlock(&netns_bpf_mutex);
bpf_prog_put(prog);
return ret;
}

View File

@ -2893,13 +2893,11 @@ static int bpf_prog_detach(const union bpf_attr *attr)
switch (ptype) {
case BPF_PROG_TYPE_SK_MSG:
case BPF_PROG_TYPE_SK_SKB:
return sock_map_get_from_fd(attr, NULL);
return sock_map_prog_detach(attr, ptype);
case BPF_PROG_TYPE_LIRC_MODE2:
return lirc_prog_detach(attr);
case BPF_PROG_TYPE_FLOW_DISSECTOR:
if (!capable(CAP_NET_ADMIN))
return -EPERM;
return netns_bpf_prog_detach(attr);
return netns_bpf_prog_detach(attr, ptype);
case BPF_PROG_TYPE_CGROUP_DEVICE:
case BPF_PROG_TYPE_CGROUP_SKB:
case BPF_PROG_TYPE_CGROUP_SOCK:

View File

@ -70,11 +70,49 @@ int sock_map_get_from_fd(const union bpf_attr *attr, struct bpf_prog *prog)
struct fd f;
int ret;
if (attr->attach_flags || attr->replace_bpf_fd)
return -EINVAL;
f = fdget(ufd);
map = __bpf_map_get(f);
if (IS_ERR(map))
return PTR_ERR(map);
ret = sock_map_prog_update(map, prog, attr->attach_type);
ret = sock_map_prog_update(map, prog, NULL, attr->attach_type);
fdput(f);
return ret;
}
int sock_map_prog_detach(const union bpf_attr *attr, enum bpf_prog_type ptype)
{
u32 ufd = attr->target_fd;
struct bpf_prog *prog;
struct bpf_map *map;
struct fd f;
int ret;
if (attr->attach_flags || attr->replace_bpf_fd)
return -EINVAL;
f = fdget(ufd);
map = __bpf_map_get(f);
if (IS_ERR(map))
return PTR_ERR(map);
prog = bpf_prog_get(attr->attach_bpf_fd);
if (IS_ERR(prog)) {
ret = PTR_ERR(prog);
goto put_map;
}
if (prog->type != ptype) {
ret = -EINVAL;
goto put_prog;
}
ret = sock_map_prog_update(map, NULL, prog, attr->attach_type);
put_prog:
bpf_prog_put(prog);
put_map:
fdput(f);
return ret;
}
@ -1203,27 +1241,32 @@ static struct sk_psock_progs *sock_map_progs(struct bpf_map *map)
}
int sock_map_prog_update(struct bpf_map *map, struct bpf_prog *prog,
u32 which)
struct bpf_prog *old, u32 which)
{
struct sk_psock_progs *progs = sock_map_progs(map);
struct bpf_prog **pprog;
if (!progs)
return -EOPNOTSUPP;
switch (which) {
case BPF_SK_MSG_VERDICT:
psock_set_prog(&progs->msg_parser, prog);
pprog = &progs->msg_parser;
break;
case BPF_SK_SKB_STREAM_PARSER:
psock_set_prog(&progs->skb_parser, prog);
pprog = &progs->skb_parser;
break;
case BPF_SK_SKB_STREAM_VERDICT:
psock_set_prog(&progs->skb_verdict, prog);
pprog = &progs->skb_verdict;
break;
default:
return -EOPNOTSUPP;
}
if (old)
return psock_replace_prog(pprog, prog, old);
psock_set_prog(pprog, prog);
return 0;
}

View File

@ -527,8 +527,8 @@ static void test_skb_less_prog_attach(struct bpf_flow *skel, int tap_fd)
run_tests_skb_less(tap_fd, skel->maps.last_dissection);
err = bpf_prog_detach(prog_fd, BPF_FLOW_DISSECTOR);
CHECK(err, "bpf_prog_detach", "err %d errno %d\n", err, errno);
err = bpf_prog_detach2(prog_fd, 0, BPF_FLOW_DISSECTOR);
CHECK(err, "bpf_prog_detach2", "err %d errno %d\n", err, errno);
}
static void test_skb_less_link_create(struct bpf_flow *skel, int tap_fd)

View File

@ -113,7 +113,7 @@ static void test_prog_attach_prog_attach(int netns, int prog1, int prog2)
CHECK_FAIL(query_attached_prog_id(netns) != query_prog_id(prog2));
out_detach:
err = bpf_prog_detach(0, BPF_FLOW_DISSECTOR);
err = bpf_prog_detach2(prog2, 0, BPF_FLOW_DISSECTOR);
if (CHECK_FAIL(err))
perror("bpf_prog_detach");
CHECK_FAIL(prog_is_attached(netns));
@ -149,7 +149,7 @@ static void test_prog_attach_link_create(int netns, int prog1, int prog2)
DECLARE_LIBBPF_OPTS(bpf_link_create_opts, opts);
int err, link;
err = bpf_prog_attach(prog1, -1, BPF_FLOW_DISSECTOR, 0);
err = bpf_prog_attach(prog1, 0, BPF_FLOW_DISSECTOR, 0);
if (CHECK_FAIL(err)) {
perror("bpf_prog_attach(prog1)");
return;
@ -165,7 +165,7 @@ static void test_prog_attach_link_create(int netns, int prog1, int prog2)
close(link);
CHECK_FAIL(query_attached_prog_id(netns) != query_prog_id(prog1));
err = bpf_prog_detach(-1, BPF_FLOW_DISSECTOR);
err = bpf_prog_detach2(prog1, 0, BPF_FLOW_DISSECTOR);
if (CHECK_FAIL(err))
perror("bpf_prog_detach");
CHECK_FAIL(prog_is_attached(netns));
@ -185,7 +185,7 @@ static void test_link_create_prog_attach(int netns, int prog1, int prog2)
/* Expect failure attaching prog when link exists */
errno = 0;
err = bpf_prog_attach(prog2, -1, BPF_FLOW_DISSECTOR, 0);
err = bpf_prog_attach(prog2, 0, BPF_FLOW_DISSECTOR, 0);
if (CHECK_FAIL(!err || errno != EEXIST))
perror("bpf_prog_attach(prog2) expected EEXIST");
CHECK_FAIL(query_attached_prog_id(netns) != query_prog_id(prog1));
@ -208,7 +208,7 @@ static void test_link_create_prog_detach(int netns, int prog1, int prog2)
/* Expect failure detaching prog when link exists */
errno = 0;
err = bpf_prog_detach(-1, BPF_FLOW_DISSECTOR);
err = bpf_prog_detach2(prog1, 0, BPF_FLOW_DISSECTOR);
if (CHECK_FAIL(!err || errno != EINVAL))
perror("bpf_prog_detach expected EINVAL");
CHECK_FAIL(query_attached_prog_id(netns) != query_prog_id(prog1));
@ -228,7 +228,7 @@ static void test_prog_attach_detach_query(int netns, int prog1, int prog2)
}
CHECK_FAIL(query_attached_prog_id(netns) != query_prog_id(prog1));
err = bpf_prog_detach(0, BPF_FLOW_DISSECTOR);
err = bpf_prog_detach2(prog1, 0, BPF_FLOW_DISSECTOR);
if (CHECK_FAIL(err)) {
perror("bpf_prog_detach");
return;