On Mon, Mar 18, 2019 at 4:18 AM Simon <[email protected]> wrote:
>
> Thx a lot again for your time and your detailed explanation.
>
> About the workaround you proposed, I didn't get where I should repeat __u16
> udp_len = bpf_ntohs(udp->len);.. I tried several spot but didn't succeed to
> make it works.
This is a tough issue. I spent a couple of hours trying various source
workaround and did not succeed.
To illustrate my experiment, the following is what I tried to do to
move the code udp_len calculation and its usage closer to each other
to avoid register spill/refills.
```
-bash-4.4$ diff ulb.c ulb.c.org
61,62c61,62
< static inline int ipv4_l4_csum(struct udphdr *data_start, __u32 data_size,
< __u64 *csum, struct iphdr *iph, void
*data_end) {
---
> static inline void ipv4_l4_csum(void *data_start, __u32 data_size,
> __u64 *csum, struct iphdr *iph) {
70,87c70,71
<
< data_start = iph + 1;
< if (data_start + 1 > data_end)
< return XDP_DROP;
< data_size = bpf_ntohs(data_start->len);
< if (data_size < 8)
< return -1;
< if (data_size > 512)
< return -1;
< data_size = data_size & 0x1ff;
< data_start = (void *)data_start + data_size;
< if ((void *) data_start <= data_end) {
< *csum = bpf_csum_diff(0, 0, (void *)data_start - data_size,
data_size, *csum);
< *csum = csum_fold_helper(*csum);
< return 0;
< }
<
< return -1;
---
> *csum = bpf_csum_diff(0, 0, data_start, data_size, *csum);
> *csum = csum_fold_helper(*csum);
140a125,132
> __u16 udp_len = bpf_ntohs(udp->len);
> if (udp_len < 8)
> return XDP_DROP;
> if (udp_len > 512) // TODO use a more approriate max value
> return XDP_DROP;
> udp_len = udp_len & 0x1ff;
> if ((void *) udp + udp_len > data_end)
> return XDP_DROP;
200,203c192
< __u16 udp_len = 0;
< int ret = ipv4_l4_csum(udp, udp_len, &cs, iph, data_end) ;
< if (ret == -1)
< return XDP_DROP;
---
> ipv4_l4_csum(udp, udp_len, &cs, iph) ;
-bash-4.4$
```
But my attempt above not working. The main reason is the compiler is
always trying to
use the original assignment register
data_start = iph + 1; /* data_start is in r8 */
/* r1 = r8, and below r1 is used for data_start */
if (data_start + 1 > data_end)
return XDP_DROP;
...
*csum = bpf_csum_diff(0, 0, (void *)data_start - data_size,
data_size, *csum); /* data_start in r8 */
The data_start is refined in r1 while r8 is used in the final argument passing.
I think I need to look at kernel verifier side and compiler side.
> By the way repeat it, means probably shifting byte again ... so I pretty sure
> I didn't get well what you mean :/ ... Did you succeed to make it works with
> this workaround ?
>
> This is maybe a stupid question, but it seems to me that I'm not doing so
> exotic code here. I mean calculate a checksum with XDP and I fall in not so
> easy "issues". Is there something I do which is totally wrong or uncommon ?
You are not. The compiler is doing optimization which make verifier
fail. It is possible an early compiler with less optimizations may
work.
>
> I will take a look at SCALAR_VALUE issue later on.But if you or anybody has
> cycles and want to look into this issue,feel free to do so.
>
> I didn't have enough skill to do that, I'm not even sure to understand what
> "spill/reload" means ?
Something like:
udp_len = ...
...
... = udp_len
The generated code:
store_to_stack for udp_len;
...
get udp_len value from stack
use udp_len
>
-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#1613): https://lists.iovisor.org/g/iovisor-dev/message/1613
Mute This Topic: https://lists.iovisor.org/mt/30315706/21656
Mute #verifier: https://lists.iovisor.org/mk?hashtag=verifier&subid=2590197
Group Owner: [email protected]
Unsubscribe: https://lists.iovisor.org/g/iovisor-dev/unsub
[[email protected]]
-=-=-=-=-=-=-=-=-=-=-=-
