lowRISC / opentitan

OpenTitan: Open source silicon root of trust
https://www.opentitan.org
Apache License 2.0
2.58k stars 777 forks source link

[dv/dpi] TCP server enters infinite loop if ring buffer is full #24616

Open duk-37 opened 2 months ago

duk-37 commented 2 months ago

Description

The rptr and wptr variables here are missing volatile, so GCC will optimize the while loop in tcp_server_put_byte to a branch and an infinite loop when compiling with -O2. as you can see in this godbolt link.

rswarbrick commented 1 month ago

Oh dear! Thanks for the report and for analysing what exactly is going on here. If I understand correctly, the tcp_buffer_put_byte function expects some other thread to update rptr and, because it isn't volatile, we get the infinite loop you describe.

Presumably a trivial way to get this effect would be to pass buf is as volatile struct tcp_buf *buf or similar. It's "more volatile" than just the pointer variables (because we are allowing the outside world to change buf->buf as well), but the meaning might be more obvious (and doesn't require us to add annotations to the struct itself).

Would you be happy to file a PR with a fix?

duk-37 commented 1 month ago

Oh dear! Thanks for the report and for analysing what exactly is going on here. If I understand correctly, the tcp_buffer_put_byte function expects some other thread to update rptr and, because it isn't volatile, we get the infinite loop you describe.

Yep!

Presumably a trivial way to get this effect would be to pass buf is as volatile struct tcp_buf *buf or similar. It's "more volatile" than just the pointer variables (because we are allowing the outside world to change buf->buf as well), but the meaning might be more obvious (and doesn't require us to add annotations to the struct itself).

Would you be happy to file a PR with a fix?

Sure, I can file one later today.