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@gibson.dropbear.id.au> 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