Re: [PATCH RFC-v5 bpf-next 02/12] net: Add BPF_XDP_EGRESS as a bpf_attach_type

["Toke Høiland-Jørgensen" <toke@redhat.com>]

David Ahern <dsahern@gmail.com> writes:

> On 4/16/20 8:01 AM, Toke Høiland-Jørgensen wrote:
>> David Ahern <dsahern@kernel.org> writes:
>> 
>>> From: David Ahern <dahern@digitalocean.com>
>>>
>>> Add new bpf_attach_type, BPF_XDP_EGRESS, for BPF programs attached
>>> at the XDP layer, but the egress path.
>>>
>>> Since egress path will not have ingress_ifindex and rx_queue_index
>>> set, update xdp_is_valid_access to block access to these entries in
>>> the xdp context when a program is attached to egress path.
>>>
>>> Update dev_change_xdp_fd to verify expected_attach_type for a program
>>> is BPF_XDP_EGRESS if egress argument is set.
>>>
>>> The next patch adds support for the egress ifindex.
>>>
>>> Signed-off-by: Prashant Bhole <prashantbhole.linux@gmail.com>
>>> Signed-off-by: David Ahern <dahern@digitalocean.com>
>>> ---
>>>  include/uapi/linux/bpf.h       | 1 +
>>>  net/core/dev.c                 | 6 ++++++
>>>  net/core/filter.c              | 8 ++++++++
>>>  tools/include/uapi/linux/bpf.h | 1 +
>>>  4 files changed, 16 insertions(+)
>>>
>>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>>> index 2e29a671d67e..a9d384998e8b 100644
>>> --- a/include/uapi/linux/bpf.h
>>> +++ b/include/uapi/linux/bpf.h
>>> @@ -215,6 +215,7 @@ enum bpf_attach_type {
>>>  	BPF_TRACE_FEXIT,
>>>  	BPF_MODIFY_RETURN,
>>>  	BPF_LSM_MAC,
>>> +	BPF_XDP_EGRESS,
>>>  	__MAX_BPF_ATTACH_TYPE
>>>  };
>>>  
>>> diff --git a/net/core/dev.c b/net/core/dev.c
>>> index 06e0872ecdae..e763b6cea8ff 100644
>>> --- a/net/core/dev.c
>>> +++ b/net/core/dev.c
>>> @@ -8731,6 +8731,12 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
>>>  		if (IS_ERR(prog))
>>>  			return PTR_ERR(prog);
>>>  
>>> +		if (egress && prog->expected_attach_type != BPF_XDP_EGRESS) {
>>> +			NL_SET_ERR_MSG(extack, "XDP program in Tx path must use BPF_XDP_EGRESS attach type");
>>> +			bpf_prog_put(prog);
>>> +			return -EINVAL;
>>> +		}
>>> +
>>>  		if (!offload && bpf_prog_is_dev_bound(prog->aux)) {
>>>  			NL_SET_ERR_MSG(extack, "using device-bound program without HW_MODE flag is not supported");
>>>  			bpf_prog_put(prog);
>>> diff --git a/net/core/filter.c b/net/core/filter.c
>>> index 7628b947dbc3..c4e0e044722f 100644
>>> --- a/net/core/filter.c
>>> +++ b/net/core/filter.c
>>> @@ -6935,6 +6935,14 @@ static bool xdp_is_valid_access(int off, int size,
>>>  				const struct bpf_prog *prog,
>>>  				struct bpf_insn_access_aux *info)
>>>  {
>>> +	if (prog->expected_attach_type == BPF_XDP_EGRESS) {
>>> +		switch (off) {
>>> +		case offsetof(struct xdp_md, ingress_ifindex):
>>> +		case offsetof(struct xdp_md, rx_queue_index):
>>> +			return false;
>>> +		}
>>> +	}
>>> +
>> 
>> How will this be handled for freplace programs - will they also
>> "inherit" the expected_attach_type of the programs they attach to?
>> 
>
> not sure I understand your point. This is not the first program type to
> have an expected_attach_type; it should work the same way others do -
> e.g., cgroup program types.

When attaching an freplace prog, the verifier will update the verifier
ops it uses with those of the target XDP program (in
check_attach_btf_id()). This is what makes the freplace program get
verified as if it were itself an XDP program.

However the freplace() program itself cannot use expected_attach_type
(see check in bpf_tracing_prog_attach()). So I don't think this check
based on prog->expected_attach_type is not going to work for freplace()
programs?

-Toke