From: Theo Buehler <tb@theobuehler.org>
Subject: Re: bgpd: rtr fix startup behaviour with empty cache
To: tech@openbsd.org
Date: Mon, 15 Jan 2024 12:38:25 +0100

On Mon, Jan 15, 2024 at 12:15:15PM +0100, Claudio Jeker wrote:
> During startup (and therefor version negotiation) the cache may respond
> with a NO_DATA_AVAILABLE error. So we need to set the version based on
> this error PDU's version (the code then triggers a RTR_EVNT_NO_DATA
> which moves the state to RTR_STATE_ESTABLISHED). I moved the version
> handling back into rtr_parse_header() so all of that is in one place.
> 
> When the no data event happens the session_id is still unset. So
> rtr_parse_notify() needs to set the session_id when not yet set to avoid
> an unnecessary error condition right after.
> 
> Last but not least add the needed call to rtr_send_reset_query() in the
> RTR_EVNT_TIMER_RETRY case. When the retry timer fires because of the no
> data condition the router should probe the cache again to see if there is
> data available.

This all makes sense.

ok tb

> 
> -- 
> :wq Claudio
> 
> Index: rtr_proto.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rtr_proto.c,v
> diff -u -p -r1.31 rtr_proto.c
> --- rtr_proto.c	11 Jan 2024 15:38:05 -0000	1.31
> +++ rtr_proto.c	15 Jan 2024 11:08:28 -0000
> @@ -419,6 +419,7 @@ rtr_parse_header(struct rtr_session *rs,
>  {
>  	struct rtr_header rh;
>  	size_t len;
> +	uint16_t errcode;
>  
>  	if (ibuf_get(hdr, &rh, sizeof(rh)) == -1)
>  		fatal("%s: ibuf_get", __func__);
> @@ -443,7 +444,14 @@ rtr_parse_header(struct rtr_session *rs,
>  			rtr_fsm(rs, RTR_EVNT_NEGOTIATION_DONE);
>  			break;
>  		case ERROR_REPORT:
> -			/* version handled in rtr_parse_error() */
> +			errcode = ntohs(rh.session_id);
> +			if (errcode == UNSUPP_PROTOCOL_VERS ||
> +			    errcode == NO_DATA_AVAILABLE) {
> +				if (rh.version < rs->version) {
> +					rs->prev_version = rs->version;
> +					rs->version = rh.version;
> +				}
> +			}
>  			break;
>  		case SERIAL_NOTIFY:
>  			/* ignore SERIAL_NOTIFY */
> @@ -537,6 +545,10 @@ rtr_parse_notify(struct rtr_session *rs,
>  	if (ibuf_get(pdu, &notify, sizeof(notify)) == -1)
>  		goto badlen;
>  
> +	/* set session_id if not yet happened */
> +	if (rs->session_id == -1)
> +		rs->session_id = ntohs(notify.hdr.session_id);
> +
>  	if (rtr_check_session_id(rs, rs->session_id, &notify.hdr, pdu) == -1)
>  		return -1;
>  
> @@ -960,10 +972,6 @@ rtr_parse_error(struct rtr_session *rs, 
>  		rtr_fsm(rs, RTR_EVNT_NO_DATA);
>  		rv = 0;
>  	} else if (errcode == UNSUPP_PROTOCOL_VERS) {
> -		if (rh.version < rs->version) {
> -			rs->prev_version = rs->version;
> -			rs->version = rh.version;
> -		}
>  		rtr_fsm(rs, RTR_EVNT_UNSUPP_PROTO_VERSION);
>  		rv = 0;
>  	} else
> @@ -1126,6 +1134,11 @@ rtr_fsm(struct rtr_session *rs, enum rtr
>  			timer_set(&rs->timers, Timer_Rtr_Retry, rs->retry);
>  			rtr_imsg_compose(IMSG_SOCKET_CONN, rs->id, 0, NULL, 0);
>  			break;
> +		case RTR_STATE_ESTABLISHED:
> +			if (rs->session_id == -1)
> +				rtr_send_reset_query(rs);
> +			else
> +				rtr_send_serial_query(rs);
>  		default:
>  			break;
>  		}
>