Re: prison_flag() check in hot path of in_pcblookup()

On Tue, Dec 13, 2022 at 11:54:17PM +0000, Bjoern A. Zeeb wrote:
B> On Tue, 13 Dec 2022, Andrew Gallatin wrote:
B> 
B> > [ I added pjd, since the original patch came from him ]
B> >
B> > Just to make sure I understand, I have a simple yes/no question:
B> >
B> > Can  jails and the host ever share the same (local) port and the same IP?
B> 
B> Can they currently (I tested only for TCP)?
B> 
B> - local binds can overlap like they can with just the base system.
B>    so bind(... {AF_INET, laddr, lport} ... ) works fine (REUSEPORT).
B> 
B> - tcp connect of a 2nd socket to the same {faddr, fport} from the above
B>    bind will fail with 'Address already in use'  [currently]
B>    [I believe that would mean your patch could go in? Where does the error come from [%]?] [*]

My reading of code confirms your tests. The official way to insert a tuple
is in_pcbconnect_setup() which branches into either its own check with
in_pcblookup_hash_locked() and returns EADDRINUSE or runs in_pcb_lport_dest()
which would also use in_pcblookup_hash_locked() to check for existing
entries. Thus, with official KPI it is impossible to insert two completely
identical 4-tuples. Alas, we have in_pcbinshash() exposed outside, as it
is expected to be called after in_pcblookup_hash_locked(), so there is no
100% source code protection from having identical tuples. ... and I'm lost
in researching all possible scenarios that edit the database. We need to
tighten this KPI.

Anyway, given tests that Bjoern did, given lack of official tests in
src/tests/sys and overall agreement on this code looking strange, I'd
suggest to go forward with Drew's suggestion:

https://reviews.freebsd.org/D38015

What's your opinion?

Given that code comes from Drew I'd suggest him to either commandeer the
revision and commit himself, or let me commit & push with --author. Or
I can commit with my name and be fully resposible for the fallout :)

-- 
Gleb Smirnoff