On 02/26, Martin Lau wrote:
> On Mon, Feb 25, 2019 at 03:14:38PM -0800, Stanislav Fomichev wrote:
> [ ... ]
>
> > >
> > > To ensure it is only called from BPF_CGROUP_INET_EGRESS, the
> > > attr->expected_attach_type must be specified as BPF_CGROUP_INET_EGRESS
> > > during load time if the prog uses this new helper.
> > > The newly added prog->enforce_expected_attach_type bit will also be set
> > > if this new helper is used. This bit is for backward compatibility reason
> > > because currently prog->expected_attach_type has been ignored in
> > > BPF_PROG_TYPE_CGROUP_SKB. During attach time,
> > > prog->expected_attach_type is only enforced if the
> > > prog->enforce_expected_attach_type bit is set.
> > > i.e. prog->expected_attach_type is only enforced if this new helper
> > > is used by the prog.
> > >
> [ ... ]
>
> > > @@ -1725,6 +1733,10 @@ static int bpf_prog_attach_check_attach_type(const
> > > struct bpf_prog *prog,
> > > case BPF_PROG_TYPE_CGROUP_SOCK:
> > > case BPF_PROG_TYPE_CGROUP_SOCK_ADDR:
> > > return attach_type == prog->expected_attach_type ? 0 : -EINVAL;
> > > + case BPF_PROG_TYPE_CGROUP_SKB:
> > > + return prog->enforce_expected_attach_type &&
> > > + prog->expected_attach_type != attach_type ?
> > > + -EINVAL : 0;
> > > default:
> > > return 0;
> > > }
> [ ... ]
>
> > > diff --git a/net/core/filter.c b/net/core/filter.c
> > > index 97916eedfe69..ca57ef25279c 100644
> > > --- a/net/core/filter.c
> > > +++ b/net/core/filter.c
> > > @@ -5426,6 +5426,24 @@ static const struct bpf_func_proto
> > > bpf_tcp_sock_proto = {
> > > .arg1_type = ARG_PTR_TO_SOCK_COMMON,
> > > };
> > >
> > > +BPF_CALL_1(bpf_tcp_enter_cwr, struct tcp_sock *, tp)
> > > +{
> > > + struct sock *sk = (struct sock *)tp;
> > > +
> > > + if (sk->sk_state == TCP_ESTABLISHED) {
> > > + tcp_enter_cwr(sk);
> > > + return 0;
> > > + }
> > > +
> > > + return -EINVAL;
> > > +}
> > > +
> > > +static const struct bpf_func_proto bpf_tcp_enter_cwr_proto = {
> > > + .func = bpf_tcp_enter_cwr,
> > > + .gpl_only = false,
> > > + .ret_type = RET_INTEGER,
> > > + .arg1_type = ARG_PTR_TO_TCP_SOCK,
> > > +};
> > > #endif /* CONFIG_INET */
> > >
> > > bool bpf_helper_changes_pkt_data(void *func)
> > > @@ -5585,6 +5603,13 @@ cg_skb_func_proto(enum bpf_func_id func_id, struct
> > > bpf_prog *prog)
> > > #ifdef CONFIG_INET
> > > case BPF_FUNC_tcp_sock:
> > > return &bpf_tcp_sock_proto;
> >
> > [...]
> > > + case BPF_FUNC_tcp_enter_cwr:
> > > + if (prog->expected_attach_type == BPF_CGROUP_INET_EGRESS) {
> > > + prog->enforce_expected_attach_type = 1;
> > > + return &bpf_tcp_enter_cwr_proto;
> > Instead of this back and forth with enforce_expected_attach_type, can we
> > just do here:
> >
> > if (prog->expected_attach_type == BPF_CGROUP_INET_EGRESS)
> > return &bpf_tcp_enter_cwr_proto;
> > else
> > return null;
> >
> > Wouldn't it have the same effect?
> The attr->expected_attach_type is currently ignored (i.e. not checked)
> during the bpf load time.
But nothing stops you form checking prog->expected_attach_type in
the cg_skb_func_proto, right? That is done at the time of loading.
So depending on prog->expected_attach_type just return null or non-null
and the verifier will take care of the rest. Then, at attach time just
make sure we are attaching it to the expected_attach_type.
We also should not have any existing use cases for
BPF_FUNC_tcp_enter_cwr I suppose.
In other words, why something like below won't work? Am I missing something?
---
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index b155cd17c1bd..86dc7cd00f34 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1678,6 +1678,10 @@ static int bpf_prog_attach_check_attach_type(const
struct bpf_prog *prog,
case BPF_PROG_TYPE_CGROUP_SOCK:
case BPF_PROG_TYPE_CGROUP_SOCK_ADDR:
return attach_type == prog->expected_attach_type ? 0 : -EINVAL;
+ case BPF_PROG_TYPE_CGROUP_SKB:
+ if (prog->expected_attach_type)
+ return attach_type == prog->expected_attach_type ? 0 :
-EINVAL;
+ return 0;
default:
return 0;
}
diff --git a/net/core/filter.c b/net/core/filter.c
index 7559d6835ecb..56f70468fc7a 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -5396,6 +5396,11 @@ cg_skb_func_proto(enum bpf_func_id func_id, const struct
bpf_prog *prog)
switch (func_id) {
case BPF_FUNC_get_local_storage:
return &bpf_get_local_storage_proto;
+ case BPF_FUNC_tcp_enter_cwr:
+ if (prog->expected_attach_type == BPF_CGROUP_INET_EGRESS)
+ return &bpf_tcp_enter_cwr_proto;
+ else
+ return NULL;
default:
return sk_filter_func_proto(func_id, prog);
}
>
> How to avoid breaking backward compatibility without selectively
> enforcing prog->expected_attach_type during attach time?
