Download raw body.
make route rt_locks atomic
On Tue, Jan 14, 2025 at 12:16:59AM +0100, Alexander Bluhm wrote:
> Hi,
>
> rt_locks is accessed lockless. Setting the bits has to be done
> atomically. Loads should be marked as atomic to avoid wrong compiler
> optimizations.
>
> The correlation between rt_locks and rt_mtu is very weak. When
> RTV_MTU is set, stop modifying rt_mtu. But is is not really necessary
> to stop updating rt_mtu immediately when another thread modifies
> rt_locks. The events in the network stack changing those values
> are not synchronized anyway. So I came to the conclusion that
> memory barriers are not necessary.
>
> In route message output, I have left setting the flags in exclusive
> net lock as they are modified together with other route attributes.
>
> ok?
I'm probably late to the party but again, how can rt_locks be atomic when
the value rt_locks protects is changed not in an atomic transaction with
the rt_locks value?
This is not truly atomic and while what you do is probably OK it is not
correct.
> bluhm
>
> Index: net/route.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/net/route.c,v
> diff -u -p -r1.438 route.c
> --- net/route.c 3 Jan 2025 21:27:40 -0000 1.438
> +++ net/route.c 13 Jan 2025 22:50:54 -0000
> @@ -557,7 +557,7 @@ rt_setgwroute(struct rtentry *rt, const
> * If the MTU of next hop is 0, this will reset the MTU of the
> * route to run PMTUD again from scratch.
> */
> - if (!ISSET(rt->rt_locks, RTV_MTU)) {
> + if (!ISSET(atomic_load_int(&rt->rt_locks), RTV_MTU)) {
> u_int mtu, nhmtu;
>
> mtu = atomic_load_int(&rt->rt_mtu);
> Index: net/route.h
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/net/route.h,v
> diff -u -p -r1.211 route.h
> --- net/route.h 3 Jan 2025 21:27:40 -0000 1.211
> +++ net/route.h 13 Jan 2025 22:50:54 -0000
> @@ -38,6 +38,7 @@
> /*
> * Locks used to protect struct members in this file:
> * I immutable after creation
> + * a atomic operations
> * N net lock
> * X exclusive net lock, or shared net lock + kernel lock
> * R art (rtable) lock
> @@ -60,7 +61,7 @@
> struct rt_kmetrics {
> u_int64_t rmx_pksent; /* packets sent using this route */
> int64_t rmx_expire; /* lifetime for route, e.g. redirect */
> - u_int rmx_locks; /* Kernel must leave these values */
> + u_int rmx_locks; /* [a] Kernel must leave these values */
> u_int rmx_mtu; /* [a] MTU for this path */
> };
> #endif
> Index: net/rtsock.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/net/rtsock.c,v
> diff -u -p -r1.377 rtsock.c
> --- net/rtsock.c 9 Jan 2025 18:20:29 -0000 1.377
> +++ net/rtsock.c 13 Jan 2025 22:50:54 -0000
> @@ -1198,8 +1198,9 @@ change:
> }
> if_group_routechange(info->rti_info[RTAX_DST],
> info->rti_info[RTAX_NETMASK]);
> - rt->rt_locks &= ~(rtm->rtm_inits);
> - rt->rt_locks |= (rtm->rtm_inits & rtm->rtm_rmx.rmx_locks);
> + atomic_clearbits_int(&rt->rt_locks, rtm->rtm_inits);
> + atomic_setbits_int(&rt->rt_locks,
> + rtm->rtm_inits & rtm->rtm_rmx.rmx_locks);
> NET_UNLOCK();
> break;
> case RTM_GET:
> @@ -1344,7 +1345,7 @@ route_cleargateway(struct rtentry *rt, v
> struct rtentry *nhrt = arg;
>
> if (ISSET(rt->rt_flags, RTF_GATEWAY) && rt->rt_gwroute == nhrt &&
> - !ISSET(rt->rt_locks, RTV_MTU))
> + !ISSET(atomic_load_int(&rt->rt_locks), RTV_MTU))
> atomic_store_int(&rt->rt_mtu, 0);
>
> return (0);
> @@ -1420,7 +1421,7 @@ rtm_getmetrics(const struct rtentry *rt,
> }
>
> bzero(out, sizeof(*out));
> - out->rmx_locks = in->rmx_locks;
> + out->rmx_locks = atomic_load_int(&in->rmx_locks);
> out->rmx_mtu = atomic_load_int(&in->rmx_mtu);
> out->rmx_expire = expire;
> out->rmx_pksent = in->rmx_pksent;
> Index: netinet/ip_icmp.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_icmp.c,v
> diff -u -p -r1.198 ip_icmp.c
> --- netinet/ip_icmp.c 3 Jan 2025 21:27:40 -0000 1.198
> +++ netinet/ip_icmp.c 13 Jan 2025 22:50:54 -0000
> @@ -1054,9 +1054,9 @@ icmp_mtudisc(struct icmp *icp, u_int rta
> * on a route. We should be using a separate flag
> * for the kernel to indicate this.
> */
> - if ((rt->rt_locks & RTV_MTU) == 0) {
> + if (!ISSET(atomic_load_int(&rt->rt_locks), RTV_MTU)) {
> if (mtu < 296 || mtu > ifp->if_mtu)
> - rt->rt_locks |= RTV_MTU;
> + atomic_setbits_int(&rt->rt_locks, RTV_MTU);
> else if (rtmtu > mtu || rtmtu == 0)
> atomic_cas_uint(&rt->rt_mtu, rtmtu, mtu);
> }
> @@ -1090,7 +1090,7 @@ icmp_mtudisc_timeout(struct rtentry *rt,
> (*ctlfunc)(PRC_MTUINC, sintosa(&sin),
> rtableid, NULL);
> } else {
> - if ((rt->rt_locks & RTV_MTU) == 0)
> + if (!ISSET(atomic_load_int(&rt->rt_locks), RTV_MTU))
> atomic_store_int(&rt->rt_mtu, 0);
> }
>
> Index: netinet/ip_output.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_output.c,v
> diff -u -p -r1.402 ip_output.c
> --- netinet/ip_output.c 3 Jan 2025 21:27:40 -0000 1.402
> +++ netinet/ip_output.c 13 Jan 2025 22:50:54 -0000
> @@ -384,7 +384,7 @@ sendit:
> * the route's MTU is locked.
> */
> if ((flags & IP_MTUDISC) && ro && ro->ro_rt &&
> - (ro->ro_rt->rt_locks & RTV_MTU) == 0)
> + (!ISSET(atomic_load_int(&ro->ro_rt->rt_locks), RTV_MTU)))
> ip->ip_off |= htons(IP_DF);
>
> #ifdef IPSEC
> @@ -471,7 +471,7 @@ sendit:
> */
> if (rtisvalid(ro->ro_rt) &&
> ISSET(ro->ro_rt->rt_flags, RTF_HOST) &&
> - !(ro->ro_rt->rt_locks & RTV_MTU)) {
> + !ISSET(atomic_load_int(&ro->ro_rt->rt_locks), RTV_MTU)) {
> u_int rtmtu;
>
> rtmtu = atomic_load_int(&ro->ro_rt->rt_mtu);
> Index: netinet/tcp_subr.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/tcp_subr.c,v
> diff -u -p -r1.204 tcp_subr.c
> --- netinet/tcp_subr.c 3 Jan 2025 17:23:51 -0000 1.204
> +++ netinet/tcp_subr.c 13 Jan 2025 22:50:54 -0000
> @@ -868,7 +868,8 @@ tcp_mtudisc(struct inpcb *inp, int errno
>
> rt = in_pcbrtentry(inp);
> if (rt != NULL) {
> - unsigned int orig_mtulock = (rt->rt_locks & RTV_MTU);
> + unsigned int orig_mtulock =
> + (atomic_load_int(&rt->rt_locks) & RTV_MTU);
>
> /*
> * If this was not a host route, remove and realloc.
> @@ -878,7 +879,7 @@ tcp_mtudisc(struct inpcb *inp, int errno
> if ((rt = in_pcbrtentry(inp)) == NULL)
> return;
> }
> - if (orig_mtulock < (rt->rt_locks & RTV_MTU))
> + if (orig_mtulock < (atomic_load_int(&rt->rt_locks) & RTV_MTU))
> change = 1;
> }
> tcp_mss(tp, -1);
> Index: netinet/tcp_timer.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/tcp_timer.c,v
> diff -u -p -r1.80 tcp_timer.c
> --- netinet/tcp_timer.c 5 Jan 2025 12:18:48 -0000 1.80
> +++ netinet/tcp_timer.c 13 Jan 2025 22:50:54 -0000
> @@ -282,7 +282,7 @@ tcp_timer_rexmt(void *arg)
> rt = in_pcbrtentry(inp);
> /* Check if path MTU discovery is disabled already */
> if (rt && (rt->rt_flags & RTF_HOST) &&
> - (rt->rt_locks & RTV_MTU))
> + ISSET(atomic_load_int(&rt->rt_locks), RTV_MTU))
> goto leave;
>
> rt = NULL;
> @@ -303,8 +303,8 @@ tcp_timer_rexmt(void *arg)
> }
> if (rt != NULL) {
> /* Disable path MTU discovery */
> - if ((rt->rt_locks & RTV_MTU) == 0) {
> - rt->rt_locks |= RTV_MTU;
> + if (!ISSET(atomic_load_int(&rt->rt_locks), RTV_MTU)) {
> + atomic_setbits_int(&rt->rt_locks, RTV_MTU);
> in_rtchange(inp, 0);
> }
>
> Index: netinet6/icmp6.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/icmp6.c,v
> diff -u -p -r1.256 icmp6.c
> --- netinet6/icmp6.c 3 Jan 2025 21:27:40 -0000 1.256
> +++ netinet6/icmp6.c 13 Jan 2025 22:50:54 -0000
> @@ -1016,7 +1016,7 @@ icmp6_mtudisc_update(struct ip6ctlparam
> rt = icmp6_mtudisc_clone(&sin6, m->m_pkthdr.ph_rtableid, 0);
>
> if (rt != NULL && ISSET(rt->rt_flags, RTF_HOST) &&
> - !(rt->rt_locks & RTV_MTU)) {
> + !ISSET(atomic_load_int(&rt->rt_locks), RTV_MTU)) {
> u_int rtmtu;
>
> rtmtu = atomic_load_int(&rt->rt_mtu);
> @@ -1851,7 +1851,7 @@ icmp6_mtudisc_timeout(struct rtentry *rt
> if ((rt->rt_flags & (RTF_DYNAMIC|RTF_HOST)) == (RTF_DYNAMIC|RTF_HOST)) {
> rtdeletemsg(rt, ifp, rtableid);
> } else {
> - if (!(rt->rt_locks & RTV_MTU))
> + if (!ISSET(atomic_load_int(&rt->rt_locks), RTV_MTU))
> atomic_store_int(&rt->rt_mtu, 0);
> }
>
> Index: netinet6/ip6_output.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/ip6_output.c,v
> diff -u -p -r1.294 ip6_output.c
> --- netinet6/ip6_output.c 3 Jan 2025 21:27:40 -0000 1.294
> +++ netinet6/ip6_output.c 13 Jan 2025 22:50:54 -0000
> @@ -1047,7 +1047,7 @@ ip6_getpmtu(struct rtentry *rt, struct i
> * field isn't locked).
> */
> mtu = ifp->if_mtu;
> - if (!(rt->rt_locks & RTV_MTU))
> + if (!ISSET(atomic_load_int(&rt->rt_locks), RTV_MTU))
> atomic_cas_uint(&rt->rt_mtu, rtmtu, mtu);
> }
> } else {
>
--
:wq Claudio
make route rt_locks atomic