Date: Sat, 11 Jan 2025 11:51:07 -0500 From: Mark Johnston <markj@freebsd.org> To: Paul Vixie <paul@redbarn.org> Cc: freebsd-net@freebsd.org Subject: Re: per-FIB socket binding Message-ID: <Z4Khe2IfKp6QVH_n@nuc> In-Reply-To: <3330519.aeNJFYEL58@localhost> References: <7772475.EvYhyI6sBW@dhcp-151.access.rits.tisf.net> <38589000.XM6RcZxFsP@dhcp-151.access.rits.tisf.net> <Z3v9R1YnDX9AWMUm@nuc> <3330519.aeNJFYEL58@localhost>
next in thread | previous in thread | raw e-mail | index | archive | help
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 > >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?Z4Khe2IfKp6QVH_n>