Re: [pve-devel] [PATCH proxmox-firewall 1/1] service: flush firewall rules on force disable

[Stefan Hanreich <s.hanreich@proxmox.com>]

On 7/4/24 12:49, Fabian Grünbichler wrote:
> so if I understand this correctly, it should handle the following case:
> 
> proxmox-firewall runs and sets up NFT rules
> user disables NFT
> pve-firewall runs and sets up legacy rules and force disable file
> proxmox-firewall runs and disables NFT rules
> 
> as opposed to the following sequence
> 
> proxmox-firewall runs and sets up NFT rules
> user disables NFT
> proxmox-firewall runs and disables NFT rules
> pve-firewall runs and sets up legacy rules and force disable file
> 
> which is already handled..

correct

> I don't see why the first cast should "almost never happen", just because the
> loops have a different period - it all comes down to alignment of the periods
> and timing of the user action?
> 
> in other words, you have a sequence
> 
> 0:    N
> 5:    N
> 5+X:  L
> 10:   N
> 15:   N
> 15+X: L
> 20:   N
> 25:   N
> 25+X: L
> 
> where the gap between N and N is 5 seconds, and the gap between N and L and L
> and N together is also 5 seconds. on average (assuming random alignment of the
> periods), there's an X=2.5s window (out of 10) that the user action must hit to
> trigger the issue (in the gap between L and N, since X can be between 0 and
> 5s)?
> 
> FWIW, the change itself looks good to me, but the commit message might need
> some adaptation ;)

You're correct, the reasoning in the commit message is wrong or at least
badly worded and I'll try to improve upon that in a v2. I guess it might
even be unnecessary to mention how often / rarely this can happen and
focus more on describing the race condition itself.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel