Re: per-FIB socket binding

On Sat, Jan 11, 2025 at 06:25:22AM +0000, Paul Vixie wrote:
> On Monday, January 6, 2025 3:56:55 PM UTC Mark Johnston wrote:
> > On Fri, Dec 27, 2024 at 08:48:48AM +0000, Paul Vixie wrote:
> > > ...
> 
> > I think the patch is probably a good idea, and the trick of only
> > inheriting the packet's FIB if the socket's is non-zero would avoid
> > breaking compatibility for most cases.
> 
> note, i often feel that integers aren't booleans and shouldn't be treated as 
> booleans. however, i can't easily look at
> 
> 	x = (y != 0) ? y : z;
> 
> when i know it could be written instead
> 
> 	x = y || z;
> 
> so if this is forbidden by today's freebsd kernel rules, please educate me. i 
> know that GCC and CLang will optimize down to the same instruction sequence 
> for either, but i prefer the shorter form since in this rare case it is 
> clearer.

We are not super consistent about it, but style(9) does prescribe
explicit comparisons, i.e., "if (count != 0)" rather than "if (count)".
In any case, I'd add a comment, since that assignment is a bit subtle.

> > One side effect of the patch is that a service listening in FIB 0 that
> > has no route to the source address of a connection attempt from a
> > different FIB would previously not accept such a connection, but now it
> > will.  Perhaps that's drastic enough to warrant a sysctl and/or sockopt
> > to control this behaviour.
> 
> i hope not. the SYN|ACK will always use the FIB from the interface where the 
> SYN arrived (this is in tcp_syncache.c).

This isn't clear to me.  The initial SYN will create a syncache entry in the
  
  if (tp->t_state == TCPS_LISTEN && SOLISTENING(so))

case in tcp_input_with_port().  In this case we define inc.inc_fibnum =
so->so_fibnum, i.e., the FIB of the listening socket.  Then,
syncache_add() copies inc to sc_inc, so sc_inc.inc_fibnum for the
syncache entry comes from the listening socket, and syncache_respond()
sets the SYN|ACK mbuf FIB with M_SETFIB(m, sc->sc_inc.inc_fibnum).  What
am I missing?  (Yes, I should actually do an experiement to check the
behaviour.)

> if the SYN's source address isn't 
> allowed by uRPF or other checking (in PF or IPFW) this will be determined by 
> the FIB in the SYN's PKTHDR not the one we put into the PCB. whereas if there 
> is some user-mode ACL processing (for example in BIND9's "allow-transfer") 
> there is no way to learn the socket's FIB. i think this excludes the case 
> where we would have rejected a connection if only the interface's FIB had not 
> been stamped onto the PCB. as before please educate me if i'm misunderstanding 
> you, and i apologize in advance for the extra work involved in doing so.
> 
> > It would be better to pass the fibnum to solisten_clone() and assign it
> > there.  Otherwise, the value of so_fibnum will be wrong for a short
> > window during which the socket is passed to MAC and other hooks, which
> > might have some surprising effects.
> 
> it shall be done. thanks for your review.
> 
> more reviews would be welcome. i'm working on the bind() case now, so that the 
> socket's FIB if still zero will be made equal to the FIB of the interface 
> where the laddr is found. this still won't handle UDP responses but it will 
> take care of TCP, UDP, and SCTP initiators. and i think i'm going to implement 
> this for SCTP responders even though i can't test it here since it looks 
> simple. (famous last words, i know.)
> 
> -- 
> Paul Vixie
> 
>