[PATCH bpf v2 0/6] Fix attach / detach uapi for sockmap and flow_dissector

[PATCH bpf v2 0/6] Fix attach / detach uapi for sockmap and flow_dissector

[Lorenz Bauer]

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

Lorenz Bauer (6):
  bpf: flow_dissector: check value of unused flags to BPF_PROG_ATTACH
  bpf: flow_dissector: check value of unused flags to BPF_PROG_DETACH
  bpf: sockmap: check value of unused args to BPF_PROG_ATTACH
  bpf: sockmap: require attach_bpf_fd when detaching a program
  selftests: bpf: pass program and target_fd in flow_dissector_reattach
  selftests: bpf: pass program to bpf_prog_detach in flow_dissector

 include/linux/bpf-netns.h                     |  5 +-
 include/linux/bpf.h                           | 13 ++++-
 include/linux/skmsg.h                         | 13 +++++
 kernel/bpf/net_namespace.c                    | 22 ++++++--
 kernel/bpf/syscall.c                          |  6 +--
 net/core/sock_map.c                           | 53 +++++++++++++++++--
 .../selftests/bpf/prog_tests/flow_dissector.c |  4 +-
 .../bpf/prog_tests/flow_dissector_reattach.c  | 12 ++---
 8 files changed, 103 insertions(+), 25 deletions(-)

-- 
2.25.1

[PATCH bpf v2 1/6] bpf: flow_dissector: check value of unused flags to BPF_PROG_ATTACH

[Lorenz Bauer]

Using BPF_PROG_ATTACH on a flow dissector program supports neither
target_fd, attach_flags or replace_bpf_fd but accepts any value.

Enforce that all of them are zero. This is fine for replace_bpf_fd
since its presence is indicated by BPF_F_REPLACE. It's more
problematic for target_fd, since zero is a valid fd. Should we
want to use the flag later on we'd have to add an exception for
fd 0. The alternative is to force a value like -1. This requires
more changes to tests. There is also precedent for using 0,
since bpf_iter uses this for target_fd as well.

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
Fixes: b27f7bb590ba ("flow_dissector: Move out netns_bpf prog callbacks")
---
 kernel/bpf/net_namespace.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/bpf/net_namespace.c b/kernel/bpf/net_namespace.c
index 3e89c7ad42cb..bf18eabeaea2 100644
--- a/kernel/bpf/net_namespace.c
+++ b/kernel/bpf/net_namespace.c
@@ -217,6 +217,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;
-- 
2.25.1

Re: [PATCH bpf v2 1/6] bpf: flow_dissector: check value of unused flags to BPF_PROG_ATTACH

[Jiri Benc]

On Mon, 29 Jun 2020 10:56:25 +0100, Lorenz Bauer wrote:
> Using BPF_PROG_ATTACH on a flow dissector program supports neither
> target_fd, attach_flags or replace_bpf_fd but accepts any value.
> 
> Enforce that all of them are zero. This is fine for replace_bpf_fd
> since its presence is indicated by BPF_F_REPLACE. It's more
> problematic for target_fd, since zero is a valid fd. Should we
> want to use the flag later on we'd have to add an exception for
> fd 0. The alternative is to force a value like -1. This requires
> more changes to tests. There is also precedent for using 0,
> since bpf_iter uses this for target_fd as well.
> 
> Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> Fixes: b27f7bb590ba ("flow_dissector: Move out netns_bpf prog callbacks")
> ---
>  kernel/bpf/net_namespace.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/kernel/bpf/net_namespace.c b/kernel/bpf/net_namespace.c
> index 3e89c7ad42cb..bf18eabeaea2 100644
> --- a/kernel/bpf/net_namespace.c
> +++ b/kernel/bpf/net_namespace.c
> @@ -217,6 +217,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;

I'm debugging failing test_flow_dissector.sh selftest and I wonder how
this patch works.

The test_flow_dissector.sh selftest at line 28 runs:

bpftool prog -d attach pinned /sys/fs/bpf/flow/flow_dissector flow_dissector

which invokes this code:

static int parse_attach_detach_args(int argc, char **argv, int *progfd,
                                    enum bpf_attach_type *attach_type,
                                    int *mapfd)
{
	[...]
        if (*attach_type == BPF_FLOW_DISSECTOR) {
                *mapfd = -1;
                return 0;
        }
	[...]
}

The mapfd is later used as attr->target_fd:

static int do_attach(int argc, char **argv)
{
	[...]
        err = bpf_prog_attach(progfd, mapfd, attach_type, 0);
	[...]
}

and rejected in the kernel by the line added by this patch. Seems that
setting flow dissector using bpftool does not work since this patch was
applied? What am I missing?

 Jiri

Re: [PATCH bpf v2 1/6] bpf: flow_dissector: check value of unused flags to BPF_PROG_ATTACH

[Lorenz Bauer]

On Wed, 4 Nov 2020 at 18:08, Jiri Benc <jbenc@redhat.com> wrote:
>
> On Mon, 29 Jun 2020 10:56:25 +0100, Lorenz Bauer wrote:
> > Using BPF_PROG_ATTACH on a flow dissector program supports neither
> > target_fd, attach_flags or replace_bpf_fd but accepts any value.
> >
> > Enforce that all of them are zero. This is fine for replace_bpf_fd
> > since its presence is indicated by BPF_F_REPLACE. It's more
> > problematic for target_fd, since zero is a valid fd. Should we
> > want to use the flag later on we'd have to add an exception for
> > fd 0. The alternative is to force a value like -1. This requires
> > more changes to tests. There is also precedent for using 0,
> > since bpf_iter uses this for target_fd as well.
> >
> > Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> > Fixes: b27f7bb590ba ("flow_dissector: Move out netns_bpf prog callbacks")
> > ---
> >  kernel/bpf/net_namespace.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/kernel/bpf/net_namespace.c b/kernel/bpf/net_namespace.c
> > index 3e89c7ad42cb..bf18eabeaea2 100644
> > --- a/kernel/bpf/net_namespace.c
> > +++ b/kernel/bpf/net_namespace.c
> > @@ -217,6 +217,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;
>
> I'm debugging failing test_flow_dissector.sh selftest and I wonder how
> this patch works.
>
> The test_flow_dissector.sh selftest at line 28 runs:
>
> bpftool prog -d attach pinned /sys/fs/bpf/flow/flow_dissector flow_dissector
>
> which invokes this code:
>
> static int parse_attach_detach_args(int argc, char **argv, int *progfd,
>                                     enum bpf_attach_type *attach_type,
>                                     int *mapfd)
> {
>         [...]
>         if (*attach_type == BPF_FLOW_DISSECTOR) {
>                 *mapfd = -1;
>                 return 0;
>         }
>         [...]
> }

Hi Jiri,

Thanks for the bug report. The commit indeed breaks attaching the flow
dissector using bpftool as you point out. I think I didn't catch this
because I don't have bpftool in my path, so that test was actually
skipped... The other parts of the test use a custom loader, which
passes 0 for the arguments and are not affected.

I had a cursory look at bpftool packaging in Debian, Ubuntu and
Fedora, it seems they all package bpftool in a kernel version
dependent package. So the most straightforward fix is probably to
change bpftool to use *mapfd = 0 and then land that via the bpf tree.

What do you think?

Lorenz

-- 
Lorenz Bauer  |  Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK

www.cloudflare.com

Re: [PATCH bpf v2 1/6] bpf: flow_dissector: check value of unused flags to BPF_PROG_ATTACH

[Jiri Benc]

On Thu, 5 Nov 2020 11:00:45 +0000, Lorenz Bauer wrote:
> I had a cursory look at bpftool packaging in Debian, Ubuntu and
> Fedora, it seems they all package bpftool in a kernel version
> dependent package. So the most straightforward fix is probably to
> change bpftool to use *mapfd = 0 and then land that via the bpf tree.
> 
> What do you think?

Apparently nobody is using bpftool for this (otherwise someone would
notice), so I'd say go ahead.

Thanks!

 Jiri

Re: [PATCH bpf v2 1/6] bpf: flow_dissector: check value of unused flags to BPF_PROG_ATTACH

[Stanislav Fomichev]

On Thu, Nov 5, 2020 at 3:08 AM Jiri Benc <jbenc@redhat.com> wrote:
>
> On Thu, 5 Nov 2020 11:00:45 +0000, Lorenz Bauer wrote:
> > I had a cursory look at bpftool packaging in Debian, Ubuntu and
> > Fedora, it seems they all package bpftool in a kernel version
> > dependent package. So the most straightforward fix is probably to
> > change bpftool to use *mapfd = 0 and then land that via the bpf tree.
> >
> > What do you think?
>
> Apparently nobody is using bpftool for this (otherwise someone would
> notice), so I'd say go ahead.
Might be a good time to switch selftests to use bpftool to attach the
flow dissector.
I think right now we have a custom binary to do it (mainly because
bpftool wasn't available in the selftests when the flow dissector was
added).

[PATCH bpf v2 2/6] bpf: flow_dissector: check value of unused flags to BPF_PROG_DETACH

[Lorenz Bauer]

Using BPF_PROG_DETACH on a flow dissector program supports neither
attach_flags nor attach_bpf_fd. Yet no value is enforced for them.

Enforce that attach_flags are zero, and require the current program
to be passed via attach_bpf_fd. This allows us to remove the check
for CAP_SYS_ADMIN, since userspace can now no longer remove
arbitrary flow dissector programs.

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
Fixes: b27f7bb590ba ("flow_dissector: Move out netns_bpf prog callbacks")
---
 include/linux/bpf-netns.h  |  5 +++--
 kernel/bpf/net_namespace.c | 19 +++++++++++++++----
 kernel/bpf/syscall.c       |  4 +---
 3 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/include/linux/bpf-netns.h b/include/linux/bpf-netns.h
index 4052d649f36d..47d5b0c708c9 100644
--- a/include/linux/bpf-netns.h
+++ b/include/linux/bpf-netns.h
@@ -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;
 }
diff --git a/kernel/bpf/net_namespace.c b/kernel/bpf/net_namespace.c
index bf18eabeaea2..7216215990ed 100644
--- a/kernel/bpf/net_namespace.c
+++ b/kernel/bpf/net_namespace.c
@@ -286,7 +286,8 @@ static void netns_bpf_run_array_detach(struct net *net,
 
 /* 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;
 
@@ -295,7 +296,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;
@@ -303,19 +304,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;
 }
 
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 8da159936bab..c0ec572f056c 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2897,9 +2897,7 @@ static int bpf_prog_detach(const union bpf_attr *attr)
 	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:
-- 
2.25.1

[PATCH bpf v2 3/6] bpf: sockmap: check value of unused args to BPF_PROG_ATTACH

[Lorenz Bauer]

Using BPF_PROG_ATTACH on a sockmap program currently understands no
flags or replace_bpf_fd, but accepts any value. Return EINVAL instead.

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
Fixes: 604326b41a6f ("bpf, sockmap: convert to generic sk_msg interface")
---
 net/core/sock_map.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/core/sock_map.c b/net/core/sock_map.c
index 4c1123c749bb..db45c1453d39 100644
--- a/net/core/sock_map.c
+++ b/net/core/sock_map.c
@@ -70,6 +70,9 @@ 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))
-- 
2.25.1

[PATCH bpf v2 4/6] bpf: sockmap: require attach_bpf_fd when detaching a program

[Lorenz Bauer]

The sockmap code currently ignores the value of attach_bpf_fd when
detaching a program. This is contrary to the usual behaviour of
checking that attach_bpf_fd represents the currently attached
program.

Ensure that attach_bpf_fd is indeed the currently attached
program. It turns out that all sockmap selftests already do this,
which indicates that this is unlikely to cause breakage.

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
Fixes: 604326b41a6f ("bpf, sockmap: convert to generic sk_msg interface")
---
 include/linux/bpf.h   | 13 +++++++++--
 include/linux/skmsg.h | 13 +++++++++++
 kernel/bpf/syscall.c  |  2 +-
 net/core/sock_map.c   | 50 ++++++++++++++++++++++++++++++++++++++-----
 4 files changed, 70 insertions(+), 8 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 5d0506f46f24..6c3160fbae0b 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1555,13 +1555,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;
 }
@@ -1571,6 +1574,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)
diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h
index 08674cd14d5a..1e9ed840b9fc 100644
--- a/include/linux/skmsg.h
+++ b/include/linux/skmsg.h
@@ -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);
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index c0ec572f056c..77340045b071 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2893,7 +2893,7 @@ 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:
diff --git a/net/core/sock_map.c b/net/core/sock_map.c
index db45c1453d39..119f52a99dc1 100644
--- a/net/core/sock_map.c
+++ b/net/core/sock_map.c
@@ -77,7 +77,42 @@ int sock_map_get_from_fd(const union bpf_attr *attr, struct bpf_prog *prog)
 	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;
 }
@@ -1212,27 +1247,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;
 }
 
-- 
2.25.1

Re: [PATCH bpf v2 4/6] bpf: sockmap: require attach_bpf_fd when detaching a program

[Martin KaFai Lau]

On Mon, Jun 29, 2020 at 10:56:28AM +0100, Lorenz Bauer wrote:
> The sockmap code currently ignores the value of attach_bpf_fd when
> detaching a program. This is contrary to the usual behaviour of
> checking that attach_bpf_fd represents the currently attached
> program.
> 
> Ensure that attach_bpf_fd is indeed the currently attached
> program. It turns out that all sockmap selftests already do this,
> which indicates that this is unlikely to cause breakage.
Please update the tools/testing/selftests/bpf/test_maps.c.
It is currently broken:

#> ./test_maps
Failed empty parser prog detach

[PATCH bpf v2 5/6] selftests: bpf: pass program and target_fd in flow_dissector_reattach

[Lorenz Bauer]

Pass 0 as target_fd when attaching and detaching flow dissector.
Additionally, pass the expected program when detaching.

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
Fixes: 1f043f87bb59 ("selftests/bpf: Add tests for attaching bpf_link to netns")
---
 .../bpf/prog_tests/flow_dissector_reattach.c         | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/flow_dissector_reattach.c b/tools/testing/selftests/bpf/prog_tests/flow_dissector_reattach.c
index 15cb554a66d8..d70adbc7309a 100644
--- a/tools/testing/selftests/bpf/prog_tests/flow_dissector_reattach.c
+++ b/tools/testing/selftests/bpf/prog_tests/flow_dissector_reattach.c
@@ -116,7 +116,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));
@@ -152,7 +152,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;
@@ -168,7 +168,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));
@@ -188,7 +188,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));
@@ -211,7 +211,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));
@@ -231,7 +231,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;
-- 
2.25.1

[PATCH bpf v2 6/6] selftests: bpf: pass program to bpf_prog_detach in flow_dissector

[Lorenz Bauer]

Calling bpf_prog_detach is incorrect, since it takes target_fd as
its argument. The intention here is to pass it as attach_bpf_fd,
so use bpf_prog_detach2 and pass zero for target_fd.

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
Fixes: 06716e04a043 ("selftests/bpf: Extend test_flow_dissector to cover link creation")
---
 tools/testing/selftests/bpf/prog_tests/flow_dissector.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/flow_dissector.c b/tools/testing/selftests/bpf/prog_tests/flow_dissector.c
index ea14e3ece812..f11f187990e9 100644
--- a/tools/testing/selftests/bpf/prog_tests/flow_dissector.c
+++ b/tools/testing/selftests/bpf/prog_tests/flow_dissector.c
@@ -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)
-- 
2.25.1

Re: [PATCH bpf v2 0/6] Fix attach / detach uapi for sockmap and flow_dissector

[Yonghong Song]

On 6/29/20 2:56 AM, Lorenz Bauer wrote:
> 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

Since bpf_iter is mentioned here, I would like to provide a little
context on how target_fd in link_create is treated there.

Currently, target_fd is always 0 as it is not used. This is
just easier if we want to use it in the future.

In the future, bpf_iter can maintain that target_fd must be 0
or it may not so. For example, it can add a flag value in
link_create such that when flag is set it will take whatever
value in target_fd and use it. Or it may just take a non-0
target_fd as an indication of the flag is set. I have not
finalized patches yet. I intend to do the latter, i.e.,
taking a non-0 target_fd. But we will see once my bpf_iter
patches for map elements are out.

There is another example where 0 and non-0 prog_fd make a difference.
The attach_prog_fd field when doing prog_load.
When attach_prog_fd is 0, it means attaching to vmlinux through
attach_btf_id. If attach_prog_fd is not 0, it means attaching to
another bpf program (replace). So user space (libbpf) may
already need to pay attention to this.

> 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
> 
> Lorenz Bauer (6):
>    bpf: flow_dissector: check value of unused flags to BPF_PROG_ATTACH
>    bpf: flow_dissector: check value of unused flags to BPF_PROG_DETACH
>    bpf: sockmap: check value of unused args to BPF_PROG_ATTACH
>    bpf: sockmap: require attach_bpf_fd when detaching a program
>    selftests: bpf: pass program and target_fd in flow_dissector_reattach
>    selftests: bpf: pass program to bpf_prog_detach in flow_dissector
> 
>   include/linux/bpf-netns.h                     |  5 +-
>   include/linux/bpf.h                           | 13 ++++-
>   include/linux/skmsg.h                         | 13 +++++
>   kernel/bpf/net_namespace.c                    | 22 ++++++--
>   kernel/bpf/syscall.c                          |  6 +--
>   net/core/sock_map.c                           | 53 +++++++++++++++++--
>   .../selftests/bpf/prog_tests/flow_dissector.c |  4 +-
>   .../bpf/prog_tests/flow_dissector_reattach.c  | 12 ++---
>   8 files changed, 103 insertions(+), 25 deletions(-)
> 

Re: [PATCH bpf v2 0/6] Fix attach / detach uapi for sockmap and flow_dissector

[Lorenz Bauer]

On Tue, 30 Jun 2020 at 06:48, Yonghong Song <yhs@fb.com> wrote:
>
> Since bpf_iter is mentioned here, I would like to provide a little
> context on how target_fd in link_create is treated there.

Thanks!

> Currently, target_fd is always 0 as it is not used. This is
> just easier if we want to use it in the future.
>
> In the future, bpf_iter can maintain that target_fd must be 0
> or it may not so. For example, it can add a flag value in
> link_create such that when flag is set it will take whatever
> value in target_fd and use it. Or it may just take a non-0
> target_fd as an indication of the flag is set. I have not
> finalized patches yet. I intend to do the latter, i.e.,
> taking a non-0 target_fd. But we will see once my bpf_iter
> patches for map elements are out.

I had a piece of code for sockmap which did something like this:

    prog = bpf_prog_get(attr->attach_bpf_fd)
    if (IS_ERR(prog))
        if (!attr->attach_bpf_fd)
            // fall back to old behaviour
        else
            return PTR_ERR(prog)
    else if (prog->type != TYPE)
        return -EINVAL

The benefit is that it continues to work if a binary is invoked with
stdin closed, which could lead to a BPF program with fd 0.

Could this work for bpf_iter as well?

>
> There is another example where 0 and non-0 prog_fd make a difference.
> The attach_prog_fd field when doing prog_load.
> When attach_prog_fd is 0, it means attaching to vmlinux through
> attach_btf_id. If attach_prog_fd is not 0, it means attaching to
> another bpf program (replace). So user space (libbpf) may
> already need to pay attention to this.

That is unfortunate. What was the reason to use 0 instead of -1 to
attach to vmlinux?

>
> > 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
> >
> > Lorenz Bauer (6):
> >    bpf: flow_dissector: check value of unused flags to BPF_PROG_ATTACH
> >    bpf: flow_dissector: check value of unused flags to BPF_PROG_DETACH
> >    bpf: sockmap: check value of unused args to BPF_PROG_ATTACH
> >    bpf: sockmap: require attach_bpf_fd when detaching a program
> >    selftests: bpf: pass program and target_fd in flow_dissector_reattach
> >    selftests: bpf: pass program to bpf_prog_detach in flow_dissector
> >
> >   include/linux/bpf-netns.h                     |  5 +-
> >   include/linux/bpf.h                           | 13 ++++-
> >   include/linux/skmsg.h                         | 13 +++++
> >   kernel/bpf/net_namespace.c                    | 22 ++++++--
> >   kernel/bpf/syscall.c                          |  6 +--
> >   net/core/sock_map.c                           | 53 +++++++++++++++++--
> >   .../selftests/bpf/prog_tests/flow_dissector.c |  4 +-
> >   .../bpf/prog_tests/flow_dissector_reattach.c  | 12 ++---
> >   8 files changed, 103 insertions(+), 25 deletions(-)
> >



-- 
Lorenz Bauer  |  Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK

www.cloudflare.com

Re: [PATCH bpf v2 0/6] Fix attach / detach uapi for sockmap and flow_dissector

[Yonghong Song]

On 6/30/20 1:39 AM, Lorenz Bauer wrote:
> On Tue, 30 Jun 2020 at 06:48, Yonghong Song <yhs@fb.com> wrote:
>>
>> Since bpf_iter is mentioned here, I would like to provide a little
>> context on how target_fd in link_create is treated there.
> 
> Thanks!
> 
>> Currently, target_fd is always 0 as it is not used. This is
>> just easier if we want to use it in the future.
>>
>> In the future, bpf_iter can maintain that target_fd must be 0
>> or it may not so. For example, it can add a flag value in
>> link_create such that when flag is set it will take whatever
>> value in target_fd and use it. Or it may just take a non-0
>> target_fd as an indication of the flag is set. I have not
>> finalized patches yet. I intend to do the latter, i.e.,
>> taking a non-0 target_fd. But we will see once my bpf_iter
>> patches for map elements are out.
> 
> I had a piece of code for sockmap which did something like this:
> 
>      prog = bpf_prog_get(attr->attach_bpf_fd)
>      if (IS_ERR(prog))
>          if (!attr->attach_bpf_fd)
>              // fall back to old behaviour
>          else
>              return PTR_ERR(prog)
>      else if (prog->type != TYPE)
>          return -EINVAL
> 
> The benefit is that it continues to work if a binary is invoked with
> stdin closed, which could lead to a BPF program with fd 0.

For bpf_iter, there is no legacy. So I will have something like
     // somecondition could be new attr->flags, or some kernel internal 
checking
     if (somecondition) {
       /* not accepting fd 0 */
       if (attr->attach_bpf_fd == 0)
         return -EINVAL;
       prog = bpf_prog_get(attr->attach_bpf_fd)
       if (IS_ERR(prog))
         return PTR_ERR(prog)
     } else if (attr->attach_bpf_fd != 0)
       return -EINVAL;
or I could have
     if (somecondition) {
       /* accepting any fd */
       prog = bpf_prog_get(attr->attach_bpf_fd)
       if (IS_ERR(prog))
         return PTR_ERR(prog)
     } else if (attr->attach_bpf_fd != 0)
       return -EINVAL;

This "somecondition" is false for the current bpf_iter, so existing
behavior attr->attach_bpf_fd == 0 is still enforced.

> 
> Could this work for bpf_iter as well?
> 
>>
>> There is another example where 0 and non-0 prog_fd make a difference.
>> The attach_prog_fd field when doing prog_load.
>> When attach_prog_fd is 0, it means attaching to vmlinux through
>> attach_btf_id. If attach_prog_fd is not 0, it means attaching to
>> another bpf program (replace). So user space (libbpf) may
>> already need to pay attention to this.
> 
> That is unfortunate. What was the reason to use 0 instead of -1 to
> attach to vmlinux?

attaching to vmlinux happens first and at that time attach_prog_fd
does not exist. Later when replace prog feature is introduced,
attach_prog_fd is added. This field is used to differentiate
between vmlinux func attachment vs. bpf_prog attachment. A little
bit unfortunate, but using 0 is easier as we have check_attr
in the kernel to ensure all kernel-unsupported fields must be 0.
using -1 will break that.

> 
>>
>>> 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
>>>
>>> Lorenz Bauer (6):
>>>     bpf: flow_dissector: check value of unused flags to BPF_PROG_ATTACH
>>>     bpf: flow_dissector: check value of unused flags to BPF_PROG_DETACH
>>>     bpf: sockmap: check value of unused args to BPF_PROG_ATTACH
>>>     bpf: sockmap: require attach_bpf_fd when detaching a program
>>>     selftests: bpf: pass program and target_fd in flow_dissector_reattach
>>>     selftests: bpf: pass program to bpf_prog_detach in flow_dissector
>>>
>>>    include/linux/bpf-netns.h                     |  5 +-
>>>    include/linux/bpf.h                           | 13 ++++-
>>>    include/linux/skmsg.h                         | 13 +++++
>>>    kernel/bpf/net_namespace.c                    | 22 ++++++--
>>>    kernel/bpf/syscall.c                          |  6 +--
>>>    net/core/sock_map.c                           | 53 +++++++++++++++++--
>>>    .../selftests/bpf/prog_tests/flow_dissector.c |  4 +-
>>>    .../bpf/prog_tests/flow_dissector_reattach.c  | 12 ++---
>>>    8 files changed, 103 insertions(+), 25 deletions(-)
>>>
> 
> 
> 

Re: [PATCH bpf v2 0/6] Fix attach / detach uapi for sockmap and flow_dissector

[Lorenz Bauer]

On Tue, 30 Jun 2020 at 16:08, Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 6/30/20 1:39 AM, Lorenz Bauer wrote:
> > On Tue, 30 Jun 2020 at 06:48, Yonghong Song <yhs@fb.com> wrote:
> >>
> >> Since bpf_iter is mentioned here, I would like to provide a little
> >> context on how target_fd in link_create is treated there.
> >
> > Thanks!
> >
> >> Currently, target_fd is always 0 as it is not used. This is
> >> just easier if we want to use it in the future.
> >>
> >> In the future, bpf_iter can maintain that target_fd must be 0
> >> or it may not so. For example, it can add a flag value in
> >> link_create such that when flag is set it will take whatever
> >> value in target_fd and use it. Or it may just take a non-0
> >> target_fd as an indication of the flag is set. I have not
> >> finalized patches yet. I intend to do the latter, i.e.,
> >> taking a non-0 target_fd. But we will see once my bpf_iter
> >> patches for map elements are out.
> >
> > I had a piece of code for sockmap which did something like this:
> >
> >      prog = bpf_prog_get(attr->attach_bpf_fd)
> >      if (IS_ERR(prog))
> >          if (!attr->attach_bpf_fd)
> >              // fall back to old behaviour
> >          else
> >              return PTR_ERR(prog)
> >      else if (prog->type != TYPE)
> >          return -EINVAL
> >
> > The benefit is that it continues to work if a binary is invoked with
> > stdin closed, which could lead to a BPF program with fd 0.
>
> For bpf_iter, there is no legacy. So I will have something like
>      // somecondition could be new attr->flags, or some kernel internal
> checking
>      if (somecondition) {
>        /* not accepting fd 0 */
>        if (attr->attach_bpf_fd == 0)
>          return -EINVAL;
>        prog = bpf_prog_get(attr->attach_bpf_fd)
>        if (IS_ERR(prog))
>          return PTR_ERR(prog)
>      } else if (attr->attach_bpf_fd != 0)
>        return -EINVAL;
> or I could have
>      if (somecondition) {
>        /* accepting any fd */
>        prog = bpf_prog_get(attr->attach_bpf_fd)
>        if (IS_ERR(prog))
>          return PTR_ERR(prog)
>      } else if (attr->attach_bpf_fd != 0)
>        return -EINVAL;
>
> This "somecondition" is false for the current bpf_iter, so existing
> behavior attr->attach_bpf_fd == 0 is still enforced.
>
> >
> > Could this work for bpf_iter as well?
> >
> >>
> >> There is another example where 0 and non-0 prog_fd make a difference.
> >> The attach_prog_fd field when doing prog_load.
> >> When attach_prog_fd is 0, it means attaching to vmlinux through
> >> attach_btf_id. If attach_prog_fd is not 0, it means attaching to
> >> another bpf program (replace). So user space (libbpf) may
> >> already need to pay attention to this.
> >
> > That is unfortunate. What was the reason to use 0 instead of -1 to
> > attach to vmlinux?
>
> attaching to vmlinux happens first and at that time attach_prog_fd
> does not exist. Later when replace prog feature is introduced,
> attach_prog_fd is added. This field is used to differentiate
> between vmlinux func attachment vs. bpf_prog attachment. A little
> bit unfortunate, but using 0 is easier as we have check_attr
> in the kernel to ensure all kernel-unsupported fields must be 0.
> using -1 will break that.

Ah, that makes sense, thank you!

Best
Lorenz

-- 
Lorenz Bauer  |  Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK

www.cloudflare.com

Re: [PATCH bpf v2 0/6] Fix attach / detach uapi for sockmap and flow_dissector

[Alexei Starovoitov]

On Mon, Jun 29, 2020 at 2:59 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> 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/

Folks, you should have used 'bpf' in the subject of Jakub's patches.
I've dropped Jakub's set from bpf-next and re-applied to bpf tree.
And applied this set on top.
Thanks!

Re: [PATCH bpf v2 0/6] Fix attach / detach uapi for sockmap and flow_dissector

[Jakub Sitnicki]

On Tue, Jun 30, 2020 at 08:00 PM CEST, Alexei Starovoitov wrote:
> On Mon, Jun 29, 2020 at 2:59 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
>>
>> 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/
>
> Folks, you should have used 'bpf' in the subject of Jakub's patches.
> I've dropped Jakub's set from bpf-next and re-applied to bpf tree.
> And applied this set on top.
> Thanks!

The preparatory work for multi-prog for netns programs [0] that landed
recently in bpf-next wasn't fixing anything, so I thought it was -next
material.

I've messed up because I've asked Lorenz to base his patchset on top of
mine, but didn't consider the fact that Lorenz's fixes are targeted for
v5.8 and earlier :-/

So alternatively, we could respin Lorenz patchset on top of 'bpf', if
you want, to untangle this situation.

But if you decide to keep my patchset in 'bpf', then can you please also
apply the today's fixup [1]? Or I can resend with correct subject prefix
tomorrow.

Thanks,
-jkbs

[0] https://lore.kernel.org/bpf/20200625141357.910330-1-jakub@cloudflare.com/
[1] https://lore.kernel.org/bpf/20200630164541.1329993-1-jakub@cloudflare.com/

Re: [PATCH bpf v2 0/6] Fix attach / detach uapi for sockmap and flow_dissector

[Alexei Starovoitov]

On Tue, Jun 30, 2020 at 11:31 AM Jakub Sitnicki <jakub@cloudflare.com> wrote:
>
> But if you decide to keep my patchset in 'bpf', then can you please also
> apply the today's fixup [1]? Or I can resend with correct subject prefix
> tomorrow.

Already did.
I found it in patchworks, but not in my mailbox.
Please cc myself and Daniel in the future.
vger can be unreliable some times.

Re: [PATCH bpf v2 0/6] Fix attach / detach uapi for sockmap and flow_dissector

[Jakub Sitnicki]

On Tue, Jun 30, 2020 at 08:42 PM CEST, Alexei Starovoitov wrote:
> On Tue, Jun 30, 2020 at 11:31 AM Jakub Sitnicki <jakub@cloudflare.com> wrote:
>>
>> But if you decide to keep my patchset in 'bpf', then can you please also
>> apply the today's fixup [1]? Or I can resend with correct subject prefix
>> tomorrow.
>
> Already did.
> I found it in patchworks, but not in my mailbox.
> Please cc myself and Daniel in the future.
> vger can be unreliable some times.

Noted. Will do from now on.