| Message ID | 20230706100243.318109-1-liuxin350@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] bpf, sockops: Enhance the return capability of sockops | expand |
On 07/06, Xin Liu wrote: > Since commit 2585cd62f098 ("bpf: Only reply field should be writeable"), > sockops is not allowd to modify the replylong field except replylong[0]. > The reason is that the replylong[1] to replylong[3] field is not used > at that time. > > But in actual use, we can call `BPF_CGROUP_RUN_PROG_SOCK_OPS` in the > kernel modules and expect sockops to return some useful data. > > The design comment about bpf_sock_ops::replylong in > include/uapi/linux/bpf.h is described as follows: > > ``` > struct bpf_sock_ops { > __u32 op; > union { > __u32 args[4]; /* Optionally passed to bpf program */ > __u32 reply; /* Returned by bpf program */ > __u32 replylong[4]; /* Optioznally returned by bpf prog */ > }; > ... > ``` > > It seems to contradict the purpose for which the field was originally > designed. Let's remove this restriction. > > Fixes: 2585cd62f098 ("bpf: Only reply field should be writeable") The commit you reference explicitly says that there is no reason to allow replylong[1..3] because there is no use for them. Has something changed since it was added? Any reason to expose those fields?
On Thu, Jul 06, 2023 at 06:02:43PM +0800, Xin Liu wrote: > Since commit 2585cd62f098 ("bpf: Only reply field should be writeable"), > sockops is not allowd to modify the replylong field except replylong[0]. nit: allowd -> allowed
diff --git a/net/core/filter.c b/net/core/filter.c index 06ba0e56e369..4662d2d3a0af 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -9063,7 +9063,7 @@ static bool sock_ops_is_valid_access(int off, int size, if (type == BPF_WRITE) { switch (off) { - case offsetof(struct bpf_sock_ops, reply): + case bpf_ctx_range_till(struct bpf_sock_ops, reply, replylong[3]): case offsetof(struct bpf_sock_ops, sk_txhash): if (size != size_default) return false;
Since commit 2585cd62f098 ("bpf: Only reply field should be writeable"), sockops is not allowd to modify the replylong field except replylong[0]. The reason is that the replylong[1] to replylong[3] field is not used at that time. But in actual use, we can call `BPF_CGROUP_RUN_PROG_SOCK_OPS` in the kernel modules and expect sockops to return some useful data. The design comment about bpf_sock_ops::replylong in include/uapi/linux/bpf.h is described as follows: ``` struct bpf_sock_ops { __u32 op; union { __u32 args[4]; /* Optionally passed to bpf program */ __u32 reply; /* Returned by bpf program */ __u32 replylong[4]; /* Optioznally returned by bpf prog */ }; ... ``` It seems to contradict the purpose for which the field was originally designed. Let's remove this restriction. Fixes: 2585cd62f098 ("bpf: Only reply field should be writeable") Signed-off-by: Xin Liu <liuxin350@huawei.com> --- net/core/filter.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)