On Monday, June 9, 2025 5:19:53 PM UTC Mark Johnston wrote:
> On Mon, Jun 09, 2025 at 04:42:55AM +0000, Paul Vixie wrote:
> > ...
> I've made a number of inline comments.
tyvm! this is precisely the kind of review i'd hoped for. see inline.
> > - so->so_fibnum = head->so_fibnum;
> > + if ((so->so_fibnum = head->so_fibnum) == 0)
> > + so->so_fibnum = fibnum;
an accept()'ed socket [which this is] inherits a fibnum from its listen()
socket which inherits from its creating process. the former can be overridden
with setsockopt(SO_FIB) and the latter by setfib(). here, we are making this
inherited or crafted [whichever it was] fibnum a fallback, so that in the case
of a TCP SYN we can preserve the interface's fibnum [in the mbuf header] as
the accept() socket's fibnum. i anticipate further complexity in the future,
such that there may someday be more than than one fallback priority. so:
> The double assignment looks odd to me. It'd be neater to write:
>
> so->so_fibnum == head->so_fibnum != 0 ? head->so_fibnum : fibnum;
it would not be future proof. as written, the extra memory write will be
coalesced in the cache's write buffer, so any performance loss would only
occur on processors made before ~Y2K. if we're to neaten it, i'd say:
if (head->so_fibnum != 0)
so->so_fibnum = head->so_fibnum;
else
so->so_fibnum = fibnum;
...so as to support future more-levels fallbacking. i dislike the read before
write at least as much as you dislike the write after write, because the "if"
will emit some kind of conditional branch in the machine code [as would your
proposed trinary conditional), which then subjects us to branch prediction and
occasional pipeline stalls. write coalescence is simply faster on processors
made after ~Y2K. but i'll adapt as shown, optimizing for clarity and future-
proofing. i'll also make the corresponding change to tcp_input.c.
> > -ifa_ifwithaddr(const struct sockaddr *addr)
> > +ifa_ifwithaddr_getfib(const struct sockaddr *addr, uint16_t *fibnum)
> > @@ -1834,17 +1834,23 @@ ifa_ifwithaddr(const struct sockaddr *addr)
> > done:
> > + if (fibnum != NULL && ifp != NULL)
> > + *fibnum = ifp->if_fib;
>
> Rather than complicating this function to add an additional return
> value, why not let the caller get the fib number directly? They can
> just get ifa->ifa_ifp->if_fib.
i wasn't willing to retrieve it outside the NET_EPOCH lock, so i changed the
call chain to pull it inside a NET_EPOCH_ASSERT() block.
> Similarly, I think adding this extra helper is unnecessary. Callers
> which want the FIB number can use ifa_ifwithaddr() and extract it
> directly. That's how in6_pcbbind_avail() handles fetching ifaddr flags,
> for instance.
i did not do a general audit to see if other potential race conditions already
exist. if you're sure this is a norm we should be living with, i'll make the
change you propose.
> > --- a/sys/netinet/in_pcb.c
> > +++ b/sys/netinet/in_pcb.c
> > @@ -646,7 +646,8 @@ in_pcballoc(struct socket *so, struct inpcbinfo
> > *pcbinfo)>
> > inp->inp_pcbinfo = pcbinfo;
> > inp->inp_socket = so;
> > inp->inp_cred = crhold(so->so_cred);
> >
> > - inp->inp_inc.inc_fibnum = so->so_fibnum;
> > + /* The FIB number is in both inp_inc and inp_socket; synchronize. */
> > + inp->inp_inc.inc_fibnum = inp->inp_socket->so_fibnum;
>
> so == inp->inp_socket, so this part of the change is redundant.
nice. i'm loathe to reference "so" after its value has been copied. i realize
that C doesn't have a borrow checker but i've been treating pointer copies as
if they were destructive, and in many cases adding something like "so = NULL"
after the copy to ensure that they are. i'll remove the comment but the code
is correct.
i'd like to see the so_cred dereference also made from inp->inp_socket if that
wouldn't break anything.
> > @@ -741,6 +743,8 @@ in_pcbbind(struct inpcb *inp, struct sockaddr_in *sin,
> > int flags,>
> > }
> > if (anonport)
> >
> > inp->inp_flags |= INP_ANONPORT;
> >
> > + /* The FIB number is in both inp_inc and inp_socket; synchronize. */
> > + sosetfib(inp->inp_socket, inp->inp_inc.inc_fibnum);
>
> Why is there no such call in the IPv6 case?
an oversight -- thanks.
> > static int
> > in_pcbbind_avail(struct inpcb *inp, const struct in_addr laddr,
> > - ifa_ifwithaddr_check((const struct sockaddr *)&sin) ==
0)
> > + ifa_ifwithaddr_check_getfib((const struct sockaddr
*)&sin,
> > + fibnum) == 0)
>
> This excludes the IN_MULTICAST() case--is that intentional?
i don't know. the multicast source address isn't checked against the interface
table. if that's an oversight i'll fix that and subsequently retrieve the
interface FIB. in the spirit of minimalism i only altered the case where an
interface is known to possess the local address.
> > + lookupflags, fibnum, cred);
>
> Indentation on continuing lines should be by four spaces.
will fix.
thanks for your investment of time in this project.
--
Paul Vixie
