On Wed, Nov 29, 2023 at 03:32:39PM +0100, Stefano Brivio wrote:On Mon, 27 Nov 2023 10:33:45 +1100 David Gibson <david(a)gibson.dropbear.id.au> wrote:So, as you note this only verifies that the index is theoretically possible. It doesn't check that it's a valid index in the current size of the connection table, it doesn't check if the connection is already closed and it can't check if it's a stale index for a different connection than the one originally intended, because the table has been compacted. Given all those potential failure modes, I don't see a lot of value in checking if the index is wildly out of bounds, which would require a kernel bug rather more extreme than those other possibilies.In tcp_timer_handler() we use conn_at_idx() to interpret the flow index from the epoll reference. However, this will never be NULL - we always put a valid index into the epoll_ref. Simplify slightly based on this.Sorry, I missed this during review of v1. I have mixed feeling about this, and I don't think 11/11 changes anything in this regard: we have to trust the kernel, as there's no benefit to security in not doing so. At the same time, should we ever get an out-of-bounds index from the epoll event, we can fail gracefully here. Slightly worse, however: if we get a timer event for a connection that's already closed, we'll happily go and try to manipulate its timer (with or without the !conn check).All in all, I think the check is minimally useful, and we should have something more robust than that. So if this patch helps keeping things simple later in the series, I don't see an issue with that, but perhaps we should add back a more sensible set of checks later.Perhaps, yes.The next patches all look good to me.-- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson