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 <[email protected]>
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
On Fri, Jun 12, 2020 at 9:02 AM Lorenz Bauer <[email protected]> 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 <[email protected]>
> 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.
On Fri, 12 Jun 2020 at 23:36, Alexei Starovoitov
<[email protected]> wrote:
>
> On Fri, Jun 12, 2020 at 9:02 AM Lorenz Bauer <[email protected]> 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 <[email protected]>
> > 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
http://www.cloudflare.com
On Mon, Jun 15, 2020 at 7:43 AM Lorenz Bauer <[email protected]> wrote:
>
> On Fri, 12 Jun 2020 at 23:36, Alexei Starovoitov
> <[email protected]> wrote:
> >
> > On Fri, Jun 12, 2020 at 9:02 AM Lorenz Bauer <[email protected]> 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 <[email protected]>
> > > 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.
On Tue, 16 Jun 2020 at 04:55, Alexei Starovoitov
<[email protected]> wrote:
>
> On Mon, Jun 15, 2020 at 7:43 AM Lorenz Bauer <[email protected]> wrote:
> >
> > On Fri, 12 Jun 2020 at 23:36, Alexei Starovoitov
> > <[email protected]> wrote:
> > >
> > > On Fri, Jun 12, 2020 at 9:02 AM Lorenz Bauer <[email protected]> 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 <[email protected]>
> > > > 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
http://www.cloudflare.com
On Tue, Jun 16, 2020 at 1:30 AM Lorenz Bauer <[email protected]> wrote:
>
> On Tue, 16 Jun 2020 at 04:55, Alexei Starovoitov
> <[email protected]> wrote:
> >
> > On Mon, Jun 15, 2020 at 7:43 AM Lorenz Bauer <[email protected]> wrote:
> > >
> > > On Fri, 12 Jun 2020 at 23:36, Alexei Starovoitov
> > > <[email protected]> wrote:
> > > >
> > > > On Fri, Jun 12, 2020 at 9:02 AM Lorenz Bauer <[email protected]> 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 <[email protected]>
> > > > > 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?
Alexei Starovoitov wrote:
> On Tue, Jun 16, 2020 at 1:30 AM Lorenz Bauer <[email protected]> wrote:
> >
> > On Tue, 16 Jun 2020 at 04:55, Alexei Starovoitov
> > <[email protected]> wrote:
> > >
> > > On Mon, Jun 15, 2020 at 7:43 AM Lorenz Bauer <[email protected]> wrote:
> > > >
> > > > On Fri, 12 Jun 2020 at 23:36, Alexei Starovoitov
> > > > <[email protected]> wrote:
> > > > >
> > > > > On Fri, Jun 12, 2020 at 9:02 AM Lorenz Bauer <[email protected]> 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 <[email protected]>
> > > > > > 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?
On Wed, 17 Jun 2020 at 07:49, John Fastabend <[email protected]> wrote:
>
> Alexei Starovoitov wrote:
> > On Tue, Jun 16, 2020 at 1:30 AM Lorenz Bauer <[email protected]> wrote:
> > >
> > > On Tue, 16 Jun 2020 at 04:55, Alexei Starovoitov
> > > <[email protected]> wrote:
> > > >
> > > > On Mon, Jun 15, 2020 at 7:43 AM Lorenz Bauer <[email protected]> wrote:
> > > > >
> > > > > On Fri, 12 Jun 2020 at 23:36, Alexei Starovoitov
> > > > > <[email protected]> wrote:
> > > > > >
> > > > > > On Fri, Jun 12, 2020 at 9:02 AM Lorenz Bauer <[email protected]> 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 <[email protected]>
> > > > > > > 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
http://www.cloudflare.com