* [PATCH bpf 1/2] flow_dissector: reject invalid attach_flags
@ 2020-06-12 16:01 Lorenz Bauer
2020-06-12 16:01 ` [PATCH bpf 2/2] bpf: sockmap: " Lorenz Bauer
2020-06-12 22:35 ` [PATCH bpf 1/2] flow_dissector: " Alexei Starovoitov
0 siblings, 2 replies; 9+ messages in thread
From: Lorenz Bauer @ 2020-06-12 16:01 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Jakub Sitnicki
Cc: kernel-team, Lorenz Bauer, netdev, bpf, linux-kernel
Using BPF_PROG_ATTACH on a flow dissector program supports neither flags
nor target_fd but accepts any value. Return EINVAL if either are non-zero.
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 78cf061f8179..56133e78ae4f 100644
--- a/kernel/bpf/net_namespace.c
+++ b/kernel/bpf/net_namespace.c
@@ -192,6 +192,9 @@ int netns_bpf_prog_attach(const union bpf_attr *attr, struct bpf_prog *prog)
struct net *net;
int ret;
+ if (attr->attach_flags || attr->target_fd)
+ return -EINVAL;
+
type = to_netns_bpf_attach_type(attr->attach_type);
if (type < 0)
return -EINVAL;
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH bpf 2/2] bpf: sockmap: reject invalid attach_flags
2020-06-12 16:01 [PATCH bpf 1/2] flow_dissector: reject invalid attach_flags Lorenz Bauer
@ 2020-06-12 16:01 ` Lorenz Bauer
2020-06-12 22:35 ` [PATCH bpf 1/2] flow_dissector: " Alexei Starovoitov
1 sibling, 0 replies; 9+ messages in thread
From: Lorenz Bauer @ 2020-06-12 16:01 UTC (permalink / raw)
To: John Fastabend, Daniel Borkmann, Jakub Sitnicki, Lorenz Bauer,
David S. Miller, Jakub Kicinski, Alexei Starovoitov
Cc: kernel-team, netdev, bpf, linux-kernel
Using BPF_PROG_ATTACH on a sockmap program currently understands no
flags, but accepts any value. Return EINVAL if any flags are specified.
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 00a26cf2cfe9..6f0894909138 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)
+ return -EINVAL;
+
f = fdget(ufd);
map = __bpf_map_get(f);
if (IS_ERR(map))
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH bpf 1/2] flow_dissector: reject invalid attach_flags
2020-06-12 16:01 [PATCH bpf 1/2] flow_dissector: reject invalid attach_flags Lorenz Bauer
2020-06-12 16:01 ` [PATCH bpf 2/2] bpf: sockmap: " Lorenz Bauer
@ 2020-06-12 22:35 ` Alexei Starovoitov
2020-06-15 14:43 ` Lorenz Bauer
1 sibling, 1 reply; 9+ messages in thread
From: Alexei Starovoitov @ 2020-06-12 22:35 UTC (permalink / raw)
To: Lorenz Bauer
Cc: Alexei Starovoitov, Daniel Borkmann, Jakub Sitnicki, kernel-team,
Network Development, bpf, LKML
On Fri, Jun 12, 2020 at 9:02 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> Using BPF_PROG_ATTACH on a flow dissector program supports neither flags
> nor target_fd but accepts any value. Return EINVAL if either are non-zero.
>
> 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 78cf061f8179..56133e78ae4f 100644
> --- a/kernel/bpf/net_namespace.c
> +++ b/kernel/bpf/net_namespace.c
> @@ -192,6 +192,9 @@ int netns_bpf_prog_attach(const union bpf_attr *attr, struct bpf_prog *prog)
> struct net *net;
> int ret;
>
> + if (attr->attach_flags || attr->target_fd)
> + return -EINVAL;
> +
In theory it makes sense, but how did you test it?
test_progs -t flow
fails 5 tests.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf 1/2] flow_dissector: reject invalid attach_flags
2020-06-12 22:35 ` [PATCH bpf 1/2] flow_dissector: " Alexei Starovoitov
@ 2020-06-15 14:43 ` Lorenz Bauer
2020-06-16 3:55 ` Alexei Starovoitov
0 siblings, 1 reply; 9+ messages in thread
From: Lorenz Bauer @ 2020-06-15 14:43 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Alexei Starovoitov, Daniel Borkmann, Jakub Sitnicki, kernel-team,
Network Development, bpf, LKML
On Fri, 12 Jun 2020 at 23:36, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Fri, Jun 12, 2020 at 9:02 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
> >
> > Using BPF_PROG_ATTACH on a flow dissector program supports neither flags
> > nor target_fd but accepts any value. Return EINVAL if either are non-zero.
> >
> > 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 78cf061f8179..56133e78ae4f 100644
> > --- a/kernel/bpf/net_namespace.c
> > +++ b/kernel/bpf/net_namespace.c
> > @@ -192,6 +192,9 @@ int netns_bpf_prog_attach(const union bpf_attr *attr, struct bpf_prog *prog)
> > struct net *net;
> > int ret;
> >
> > + if (attr->attach_flags || attr->target_fd)
> > + return -EINVAL;
> > +
>
> In theory it makes sense, but how did you test it?
Not properly it seems, sorry!
> test_progs -t flow
> fails 5 tests.
I spent today digging through this, and the issue is actually more annoying than
I thought. BPF_PROG_DETACH for sockmap and flow_dissector ignores
attach_bpf_fd. The cgroup and lirc2 attach point use this to make sure that the
program being detached is actually what user space expects. We actually have
tests that set attach_bpf_fd for these to attach points, which tells
me that this is
an easy mistake to make.
Unfortunately I can't come up with a good fix that seems backportable:
- Making sockmap and flow_dissector have the same semantics as cgroup
and lirc2 requires a bunch of changes (probably a new function for sockmap)
- Returning EINVAL from BPF_PROG_DETACH if attach_bpf_fd is specified
leads to a lot of churn in selftests
Is it worth just landing these fixes on bpf or bpf-next without
backporting them?
--
Lorenz Bauer | Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK
www.cloudflare.com
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf 1/2] flow_dissector: reject invalid attach_flags
2020-06-15 14:43 ` Lorenz Bauer
@ 2020-06-16 3:55 ` Alexei Starovoitov
2020-06-16 8:30 ` Lorenz Bauer
0 siblings, 1 reply; 9+ messages in thread
From: Alexei Starovoitov @ 2020-06-16 3:55 UTC (permalink / raw)
To: Lorenz Bauer
Cc: Alexei Starovoitov, Daniel Borkmann, Jakub Sitnicki, kernel-team,
Network Development, bpf, LKML
On Mon, Jun 15, 2020 at 7:43 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> On Fri, 12 Jun 2020 at 23:36, Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Fri, Jun 12, 2020 at 9:02 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
> > >
> > > Using BPF_PROG_ATTACH on a flow dissector program supports neither flags
> > > nor target_fd but accepts any value. Return EINVAL if either are non-zero.
> > >
> > > 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 78cf061f8179..56133e78ae4f 100644
> > > --- a/kernel/bpf/net_namespace.c
> > > +++ b/kernel/bpf/net_namespace.c
> > > @@ -192,6 +192,9 @@ int netns_bpf_prog_attach(const union bpf_attr *attr, struct bpf_prog *prog)
> > > struct net *net;
> > > int ret;
> > >
> > > + if (attr->attach_flags || attr->target_fd)
> > > + return -EINVAL;
> > > +
> >
> > In theory it makes sense, but how did you test it?
>
> Not properly it seems, sorry!
>
> > test_progs -t flow
> > fails 5 tests.
>
> I spent today digging through this, and the issue is actually more annoying than
> I thought. BPF_PROG_DETACH for sockmap and flow_dissector ignores
> attach_bpf_fd. The cgroup and lirc2 attach point use this to make sure that the
> program being detached is actually what user space expects. We actually have
> tests that set attach_bpf_fd for these to attach points, which tells
> me that this is
> an easy mistake to make.
>
> Unfortunately I can't come up with a good fix that seems backportable:
> - Making sockmap and flow_dissector have the same semantics as cgroup
> and lirc2 requires a bunch of changes (probably a new function for sockmap)
making flow dissector pass prog_fd as cg and lirc is certainly my preference.
Especially since tests are passing fd user code is likely doing the same,
so breakage is unlikely. Also it wasn't done that long ago, so
we can backport far enough.
It will remove cap_net_admin ugly check in bpf_prog_detach()
which is the only exception now in cap model.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf 1/2] flow_dissector: reject invalid attach_flags
2020-06-16 3:55 ` Alexei Starovoitov
@ 2020-06-16 8:30 ` Lorenz Bauer
2020-06-16 20:37 ` Alexei Starovoitov
0 siblings, 1 reply; 9+ messages in thread
From: Lorenz Bauer @ 2020-06-16 8:30 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Alexei Starovoitov, Daniel Borkmann, Jakub Sitnicki, kernel-team,
Network Development, bpf, LKML
On Tue, 16 Jun 2020 at 04:55, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Mon, Jun 15, 2020 at 7:43 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
> >
> > On Fri, 12 Jun 2020 at 23:36, Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > On Fri, Jun 12, 2020 at 9:02 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
> > > >
> > > > Using BPF_PROG_ATTACH on a flow dissector program supports neither flags
> > > > nor target_fd but accepts any value. Return EINVAL if either are non-zero.
> > > >
> > > > 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 78cf061f8179..56133e78ae4f 100644
> > > > --- a/kernel/bpf/net_namespace.c
> > > > +++ b/kernel/bpf/net_namespace.c
> > > > @@ -192,6 +192,9 @@ int netns_bpf_prog_attach(const union bpf_attr *attr, struct bpf_prog *prog)
> > > > struct net *net;
> > > > int ret;
> > > >
> > > > + if (attr->attach_flags || attr->target_fd)
> > > > + return -EINVAL;
> > > > +
> > >
> > > In theory it makes sense, but how did you test it?
> >
> > Not properly it seems, sorry!
> >
> > > test_progs -t flow
> > > fails 5 tests.
> >
> > I spent today digging through this, and the issue is actually more annoying than
> > I thought. BPF_PROG_DETACH for sockmap and flow_dissector ignores
> > attach_bpf_fd. The cgroup and lirc2 attach point use this to make sure that the
> > program being detached is actually what user space expects. We actually have
> > tests that set attach_bpf_fd for these to attach points, which tells
> > me that this is
> > an easy mistake to make.
> >
> > Unfortunately I can't come up with a good fix that seems backportable:
> > - Making sockmap and flow_dissector have the same semantics as cgroup
> > and lirc2 requires a bunch of changes (probably a new function for sockmap)
>
> making flow dissector pass prog_fd as cg and lirc is certainly my preference.
> Especially since tests are passing fd user code is likely doing the same,
> so breakage is unlikely. Also it wasn't done that long ago, so
> we can backport far enough.
> It will remove cap_net_admin ugly check in bpf_prog_detach()
> which is the only exception now in cap model.
SGTM. What about sockmap though? The code for that has been around for ages.
--
Lorenz Bauer | Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK
www.cloudflare.com
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf 1/2] flow_dissector: reject invalid attach_flags
2020-06-16 8:30 ` Lorenz Bauer
@ 2020-06-16 20:37 ` Alexei Starovoitov
2020-06-17 6:49 ` John Fastabend
0 siblings, 1 reply; 9+ messages in thread
From: Alexei Starovoitov @ 2020-06-16 20:37 UTC (permalink / raw)
To: Lorenz Bauer
Cc: Alexei Starovoitov, Daniel Borkmann, Jakub Sitnicki, kernel-team,
Network Development, bpf, LKML, John Fastabend
On Tue, Jun 16, 2020 at 1:30 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> On Tue, 16 Jun 2020 at 04:55, Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Mon, Jun 15, 2020 at 7:43 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
> > >
> > > On Fri, 12 Jun 2020 at 23:36, Alexei Starovoitov
> > > <alexei.starovoitov@gmail.com> wrote:
> > > >
> > > > On Fri, Jun 12, 2020 at 9:02 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
> > > > >
> > > > > Using BPF_PROG_ATTACH on a flow dissector program supports neither flags
> > > > > nor target_fd but accepts any value. Return EINVAL if either are non-zero.
> > > > >
> > > > > 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 78cf061f8179..56133e78ae4f 100644
> > > > > --- a/kernel/bpf/net_namespace.c
> > > > > +++ b/kernel/bpf/net_namespace.c
> > > > > @@ -192,6 +192,9 @@ int netns_bpf_prog_attach(const union bpf_attr *attr, struct bpf_prog *prog)
> > > > > struct net *net;
> > > > > int ret;
> > > > >
> > > > > + if (attr->attach_flags || attr->target_fd)
> > > > > + return -EINVAL;
> > > > > +
> > > >
> > > > In theory it makes sense, but how did you test it?
> > >
> > > Not properly it seems, sorry!
> > >
> > > > test_progs -t flow
> > > > fails 5 tests.
> > >
> > > I spent today digging through this, and the issue is actually more annoying than
> > > I thought. BPF_PROG_DETACH for sockmap and flow_dissector ignores
> > > attach_bpf_fd. The cgroup and lirc2 attach point use this to make sure that the
> > > program being detached is actually what user space expects. We actually have
> > > tests that set attach_bpf_fd for these to attach points, which tells
> > > me that this is
> > > an easy mistake to make.
> > >
> > > Unfortunately I can't come up with a good fix that seems backportable:
> > > - Making sockmap and flow_dissector have the same semantics as cgroup
> > > and lirc2 requires a bunch of changes (probably a new function for sockmap)
> >
> > making flow dissector pass prog_fd as cg and lirc is certainly my preference.
> > Especially since tests are passing fd user code is likely doing the same,
> > so breakage is unlikely. Also it wasn't done that long ago, so
> > we can backport far enough.
> > It will remove cap_net_admin ugly check in bpf_prog_detach()
> > which is the only exception now in cap model.
>
> SGTM. What about sockmap though? The code for that has been around for ages.
you mean the second patch that enforces sock_map_get_from_fd doesn't
use attach_flags?
I think it didn't break anything, so enforcing is fine.
or the detach part that doesn't use prog_fd ?
I'm not sure what's the best here.
At least from cap perspective it's fine because map_fd is there.
John, wdyt?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf 1/2] flow_dissector: reject invalid attach_flags
2020-06-16 20:37 ` Alexei Starovoitov
@ 2020-06-17 6:49 ` John Fastabend
2020-06-23 17:07 ` Lorenz Bauer
0 siblings, 1 reply; 9+ messages in thread
From: John Fastabend @ 2020-06-17 6:49 UTC (permalink / raw)
To: Alexei Starovoitov, Lorenz Bauer
Cc: Alexei Starovoitov, Daniel Borkmann, Jakub Sitnicki, kernel-team,
Network Development, bpf, LKML, John Fastabend
Alexei Starovoitov wrote:
> On Tue, Jun 16, 2020 at 1:30 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
> >
> > On Tue, 16 Jun 2020 at 04:55, Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > On Mon, Jun 15, 2020 at 7:43 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
> > > >
> > > > On Fri, 12 Jun 2020 at 23:36, Alexei Starovoitov
> > > > <alexei.starovoitov@gmail.com> wrote:
> > > > >
> > > > > On Fri, Jun 12, 2020 at 9:02 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
> > > > > >
> > > > > > Using BPF_PROG_ATTACH on a flow dissector program supports neither flags
> > > > > > nor target_fd but accepts any value. Return EINVAL if either are non-zero.
> > > > > >
> > > > > > 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 78cf061f8179..56133e78ae4f 100644
> > > > > > --- a/kernel/bpf/net_namespace.c
> > > > > > +++ b/kernel/bpf/net_namespace.c
> > > > > > @@ -192,6 +192,9 @@ int netns_bpf_prog_attach(const union bpf_attr *attr, struct bpf_prog *prog)
> > > > > > struct net *net;
> > > > > > int ret;
> > > > > >
> > > > > > + if (attr->attach_flags || attr->target_fd)
> > > > > > + return -EINVAL;
> > > > > > +
> > > > >
> > > > > In theory it makes sense, but how did you test it?
> > > >
> > > > Not properly it seems, sorry!
> > > >
> > > > > test_progs -t flow
> > > > > fails 5 tests.
> > > >
> > > > I spent today digging through this, and the issue is actually more annoying than
> > > > I thought. BPF_PROG_DETACH for sockmap and flow_dissector ignores
> > > > attach_bpf_fd. The cgroup and lirc2 attach point use this to make sure that the
> > > > program being detached is actually what user space expects. We actually have
> > > > tests that set attach_bpf_fd for these to attach points, which tells
> > > > me that this is
> > > > an easy mistake to make.
In sockmap case I didn't manage to think what multiple programs of the same type
on the same map would look like so we can just remove whatever program is there.
Is there a problem with this or is it that we just want the sanity check.
> > > >
> > > > Unfortunately I can't come up with a good fix that seems backportable:
> > > > - Making sockmap and flow_dissector have the same semantics as cgroup
> > > > and lirc2 requires a bunch of changes (probably a new function for sockmap)
> > >
> > > making flow dissector pass prog_fd as cg and lirc is certainly my preference.
> > > Especially since tests are passing fd user code is likely doing the same,
> > > so breakage is unlikely. Also it wasn't done that long ago, so
> > > we can backport far enough.
> > > It will remove cap_net_admin ugly check in bpf_prog_detach()
> > > which is the only exception now in cap model.
> >
> > SGTM. What about sockmap though? The code for that has been around for ages.
>
> you mean the second patch that enforces sock_map_get_from_fd doesn't
> use attach_flags?
> I think it didn't break anything, so enforcing is fine.
I'm ok with enforcing it.
>
> or the detach part that doesn't use prog_fd ?
> I'm not sure what's the best here.
> At least from cap perspective it's fine because map_fd is there.
>
> John, wdyt?
I think we can keep the current detach without the prog_fd as-is. And
then add logic so that if the prog_fd is included we check it?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf 1/2] flow_dissector: reject invalid attach_flags
2020-06-17 6:49 ` John Fastabend
@ 2020-06-23 17:07 ` Lorenz Bauer
0 siblings, 0 replies; 9+ messages in thread
From: Lorenz Bauer @ 2020-06-23 17:07 UTC (permalink / raw)
To: John Fastabend
Cc: Alexei Starovoitov, Alexei Starovoitov, Daniel Borkmann,
Jakub Sitnicki, kernel-team, Network Development, bpf, LKML
On Wed, 17 Jun 2020 at 07:49, John Fastabend <john.fastabend@gmail.com> wrote:
>
> Alexei Starovoitov wrote:
> > On Tue, Jun 16, 2020 at 1:30 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
> > >
> > > On Tue, 16 Jun 2020 at 04:55, Alexei Starovoitov
> > > <alexei.starovoitov@gmail.com> wrote:
> > > >
> > > > On Mon, Jun 15, 2020 at 7:43 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
> > > > >
> > > > > On Fri, 12 Jun 2020 at 23:36, Alexei Starovoitov
> > > > > <alexei.starovoitov@gmail.com> wrote:
> > > > > >
> > > > > > On Fri, Jun 12, 2020 at 9:02 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
> > > > > > >
> > > > > > > Using BPF_PROG_ATTACH on a flow dissector program supports neither flags
> > > > > > > nor target_fd but accepts any value. Return EINVAL if either are non-zero.
> > > > > > >
> > > > > > > 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 78cf061f8179..56133e78ae4f 100644
> > > > > > > --- a/kernel/bpf/net_namespace.c
> > > > > > > +++ b/kernel/bpf/net_namespace.c
> > > > > > > @@ -192,6 +192,9 @@ int netns_bpf_prog_attach(const union bpf_attr *attr, struct bpf_prog *prog)
> > > > > > > struct net *net;
> > > > > > > int ret;
> > > > > > >
> > > > > > > + if (attr->attach_flags || attr->target_fd)
> > > > > > > + return -EINVAL;
> > > > > > > +
> > > > > >
> > > > > > In theory it makes sense, but how did you test it?
> > > > >
> > > > > Not properly it seems, sorry!
> > > > >
> > > > > > test_progs -t flow
> > > > > > fails 5 tests.
> > > > >
> > > > > I spent today digging through this, and the issue is actually more annoying than
> > > > > I thought. BPF_PROG_DETACH for sockmap and flow_dissector ignores
> > > > > attach_bpf_fd. The cgroup and lirc2 attach point use this to make sure that the
> > > > > program being detached is actually what user space expects. We actually have
> > > > > tests that set attach_bpf_fd for these to attach points, which tells
> > > > > me that this is
> > > > > an easy mistake to make.
>
> In sockmap case I didn't manage to think what multiple programs of the same type
> on the same map would look like so we can just remove whatever program is there.
> Is there a problem with this or is it that we just want the sanity check.
>
> > > > >
> > > > > Unfortunately I can't come up with a good fix that seems backportable:
> > > > > - Making sockmap and flow_dissector have the same semantics as cgroup
> > > > > and lirc2 requires a bunch of changes (probably a new function for sockmap)
> > > >
> > > > making flow dissector pass prog_fd as cg and lirc is certainly my preference.
> > > > Especially since tests are passing fd user code is likely doing the same,
> > > > so breakage is unlikely. Also it wasn't done that long ago, so
> > > > we can backport far enough.
> > > > It will remove cap_net_admin ugly check in bpf_prog_detach()
> > > > which is the only exception now in cap model.
> > >
> > > SGTM. What about sockmap though? The code for that has been around for ages.
> >
> > you mean the second patch that enforces sock_map_get_from_fd doesn't
> > use attach_flags?
> > I think it didn't break anything, so enforcing is fine.
>
> I'm ok with enforcing it.
>
> >
> > or the detach part that doesn't use prog_fd ?
> > I'm not sure what's the best here.
> > At least from cap perspective it's fine because map_fd is there.
> >
> > John, wdyt?
>
> I think we can keep the current detach without the prog_fd as-is. And
> then add logic so that if the prog_fd is included we check it?
Do you know of users that rely on this? FWIW all of the selftests actually
pass attach_bpf_fd when detaching from sockmap (on a recent bpf-next at least).
It'd be nice if I could make sockmap require this to be present, just
so that it's consistent with flow_dissector and other BPF_PROG_DETACH
users.
OTOH I'm not sure if this is backport material after all.
Lorenz
--
Lorenz Bauer | Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK
www.cloudflare.com
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-06-23 17:07 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-12 16:01 [PATCH bpf 1/2] flow_dissector: reject invalid attach_flags Lorenz Bauer
2020-06-12 16:01 ` [PATCH bpf 2/2] bpf: sockmap: " Lorenz Bauer
2020-06-12 22:35 ` [PATCH bpf 1/2] flow_dissector: " Alexei Starovoitov
2020-06-15 14:43 ` Lorenz Bauer
2020-06-16 3:55 ` Alexei Starovoitov
2020-06-16 8:30 ` Lorenz Bauer
2020-06-16 20:37 ` Alexei Starovoitov
2020-06-17 6:49 ` John Fastabend
2020-06-23 17:07 ` Lorenz Bauer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).