Re: Routing socket issue?

[Roy Marples <roy%marples.name@localhost>]

Hi Frank :)

On 31/01/2021 07:58, Frank Kardel wrote:
> > For example I fail to see how RTM_LOSING helps that because it won't change
> > how ntpd would configure itself.
> Well if I read the comment right I am inclined to differ here:
> In in_pcs.c we find:
> /*
>   * Check for alternatives when higher level complains
>   * about service problems.  For now, invalidate cached
>   * routing information.  If the route was created dynamically
>   * (by a redirect), time to try a default gateway again.
>   */
> in_losing(struct inpcb *inp)
>
> and the call is in tcp_time.c:
>      /*
>       * If losing, let the lower level know and try for
>       * a better route.  Also, if we backed off this far,
>       * our srtt estimate is probably bogus.  Clobber it
>       * so we'll take the next rtt measurement as our srtt;
>       * move the current srtt into rttvar to keep the current
>       * retransmit times until then.
>       */
>
> As ntpd acts after a grace period the routing engine may have corrected this 
> situation and routing may indeed change.
> ntpd's interactions with peers can take up to 1024s so it is good to attempt in 
> a best effort way to keep the internal
> local address/socket state close to the current state.
> It is likely though that there have been routing messages like 
> RTM_CHANGE/ADD/DELETE before that and RTM_LOSING is not providing
> additional information at the point.

Right, RTM_LOSING is just informational.
If any routing does change then we get RTM_CHANGE/ADD/DELETE etc.

> > As NTP doesn't bring interfaces up or down, RFM_IFANNOUNCE is useless as well.
> > If the interface does vanish, any addresses on it will be reported via 
> > RTM_DELADDR.
> > RTM_IFINFO is also questionable as commentary in the code is that it only 
> > cares about addresses.
> Well I read
> ntp_io.c
>                          /*
>                           * we are keen on new and deleted addresses and
>                           * if an interface goes up and down or routing
>                           * changes
>                           */
> not as being interested in addresses only.
>
> Also keep in mind that at this point routing messages are processed in a loop 
> and the action here
>      timer_interfacetimeout(current_time + UPDATE_GRACE);
> just sets the variable for the next interface+local address update run. This is 
> very cheap. The grace period
> will batch multiple routing message together. An explicit routing message flush 
> is from my point of view
> code clutter here. as the socket is effectively drained in the loop at the cost 
> of examining the msg_type and setting
> a variable. Not much gained here.

OK, we'll keep RTM_IFINFO but drop RTM_IFANNOUNCE.
The point is trying to eliminate the overflow message entirely.

> > I mean, if you want to argue against any of that then I would suggest why even 
> > bother filtering or looking at overflow at all?
> > Shrink the code - any activity on the routing socket, drain it ignoring all 
> > error, start the interface update timer.
> That would be an option but we should react only on known events. There may be 
> one or two events that could be removed from
> the list after examination as other messages can cover for them. Keep in mind 
> the this is a portable code section and the
> code tries to be on the fail safe, robust side for the goal of address/routing 
> tracking so adjusting it to a particular implementation
> may break on other os implementations.

Well, Dragonfly (prior to my patches there) and by extension FreeBSD (not 
checked to see if that changed) both emit RMT_DELADDR before RTM_IFANNOUNCE (ie 
wrong order) so when they do overflow you never see RTM_IFANNOUNCE to say the 
interface vanished. Hence there is zero point is listening for it for ntp.

> > > As for the message: IMHO it does not need to be logged at all (DPRINTF/maybe 
> > > LOGDEBUG at most) because the overflow should and does just trigger ntpd to 
> > > reevaluate the interface/routing configuration.
> > >
> > > This information is not important at all for normal operation as the effects 
> > > are correctly mitigated.

I changed it to LOG_DEBUG as well as removing RTM_LOSING and RTM_IFANNOUNCE as 
discussed above.

> > Great.
> >
> > > BTW: does the current code revert to (fail safe) periodic interface scanning 
> > > if the routing socket is being disabled (happens when an unexpected error 
> > > code is returned from read(2))?
> >
> > No.
> >
> > The socket is non blocking so the only error to ignore here would be EINTR.
> > Any other errors are due to bad programming IMO.
> Could be bad programming, but I prefer the ntpd being forgiving against hiccups 
> by reverting to periodic scanning when we
> disable to routing socket. That is a fail safe strategy and would also warrant a 
> log message as it is an unusual event.

EINTR is now ignored.
I'll find time to restore periodic scanning later.

Roy