Re: [PATCH v2 08/11] tcp: Remove unneccessary bounds check in tcp_timer_handler()

On Wed, Nov 29, 2023 at 03:32:39PM +0100, Stefano Brivio wrote:

On Mon, 27 Nov 2023 10:33:45 +1100 David Gibson wrote:

...

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).

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.

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