search

mirage / qubes-mirage-firewall

A Mirage firewall VM for QubesOS
208 stars 28 forks source link

My_nat.free_udp_port: avoid looping forever, use last_resort_port earlier #159

Closed hannesm closed 1 year ago

hannesm commented 1 year ago

This avoids using the entire CPU. See #158 for the issue description (especially my comment https://github.com/mirage/qubes-mirage-firewall/issues/158#issuecomment-1307610459).

palainp commented 1 year ago

Thanks for that PR. After some times setting up correctly my mirage-fw I can confirm that it doesn't burn the CPU anymore. LGTM (together with the mirage-nat update that also helps in solving this issue)!

hannesm commented 1 year ago

I would like to verify first that the mirage-nat change (and 3.0.1 release) fixes the issue. This change here is "purely cosmetical" and shouldn't be needed. Though it as well helps to avoid busy loops (since the free_udp_port will be bounded).

While working on this, another issue is how DNS replies are handled with a single global mailbox (Lwt_mvar)

This means there can be concurrent requests with unclear ordering, and also the Lwt_mvar.put (in uplink) can block if there's nobody reading the mailbox (which only my_dns is). So that code should be revised that DNS replies are matching the requests, and there's no blocking behaviour if more replies are received than requests that were sent. Especially evil since there's a timeout which cancels the Lwt_mvar.take...

palainp commented 1 year ago

Thanks @hannesm for your amazing work on that points! I can confirm that the PR with mirage-nat alone (without this PR) permit to not overload CPU, but I'll feel better to have a bounded loop :)

hannesm commented 1 year ago

this has been part of https://github.com/mirage/qubes-mirage-firewall/pull/163 and the 0.8.3 release