zijianzhang@ wrote:
> From: Zijian Zhang <zijianzhang@xxxxxxxxxxxxx>
>
> Found in the test_txmsg_pull in test_sockmap,
> ```
> txmsg_cork = 512;
> opt->iov_length = 3;
> opt->iov_count = 1;
> opt->rate = 512;
> ```
> The first sendmsg will send an sk_msg with size 3, and bpf_msg_pull_data
> will be invoked the first time. sk_msg_reset_curr will reset the copybreak
> from 3 to 0, then the second sendmsg will write into copybreak starting at
> 0 which overwrites the first sendmsg. The same problem happens in push and
> pop test. Thus, fix sk_msg_reset_curr to restore the correct copybreak.
>
> Fixes: bb9aefde5bba ("bpf: sockmap, updating the sg structure should also update curr")
> Signed-off-by: Zijian Zhang <zijianzhang@xxxxxxxxxxxxx>
Hi Zijian, question on below.
> ---
> net/core/filter.c | 20 +++++++++-----------
> 1 file changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 8e1a8a8d8d55..b725d3a2fdb8 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -2619,18 +2619,16 @@ BPF_CALL_2(bpf_msg_cork_bytes, struct sk_msg *, msg, u32, bytes)
>
I find push_data a bit easier to think through so allow me to walk
through a push example.
If we setup so that curr=0 and copybreak=3 then call
push_data(skmsg, 2, 2);
When we get to the sk_msg_reset_curr we should have a layout,
msg->sg.data[0] = length(2) equal to original [0,2]
msg->sg.data[1] = length(2)
msg->sg.data[2] = legnth(1) equal to original [3]
The current before the reset curr will be,
curr = 1
copybreak = 3
> static void sk_msg_reset_curr(struct sk_msg *msg)
> {
> - u32 i = msg->sg.start;
> - u32 len = 0;
> -
with above context i = 0
> - do {
> - len += sk_msg_elem(msg, i)->length;
> - sk_msg_iter_var_next(i);
> - if (len >= msg->sg.size)
> - break;
> - } while (i != msg->sg.end);
When we exit loop,
i = 3
len = 5
msg->sg.curr = 3
msg->sg.copybreak = 0
So we zero the copy break and set curr = 3. The next send
should happen over sg.curr=3? What did I miss?
> + if (!msg->sg.size) {
> + msg->sg.curr = msg->sg.start;
> + msg->sg.copybreak = 0;
> + } else {
> + u32 i = msg->sg.end;
>
> - msg->sg.curr = i;
> - msg->sg.copybreak = 0;
> + sk_msg_iter_var_prev(i);
With this curr will always point to the end-1 but I'm not sure this can
handle the case where we have done sk_msg_alloc() so we have start/end
setup. And then on a copy fault for example we might have curr pointing
somewhere in the middle of that. I think I will need to construct the
example, but I believe this is originally why the 'i' is discovered
by sg walk vs simpler end.
> + msg->sg.curr = i;
> + msg->sg.copybreak = msg->sg.data[i].length;
This does seem more accurate then simply zero'ing out the copybreak
which is a good thing.
> + }
> }
>
> static const struct bpf_func_proto bpf_msg_cork_bytes_proto = {
> --
> 2.20.1
>
