On Mon, Jun 27, 2022 at 03:21:56PM +0200, Alexander Bluhm wrote:
> Hi,
> 
> Use ref count API for routes.
> 
> ok?
> 

Some time ago, I posted the diff which removes the logic around
negative `refcnt' within rtfree(). This diff was stopped by mpi@, he
said we could underflow rt->rt_refcnt and this logic is required to
prevent panics. I don't know, is it true, but I didn't find any report
about "rtfree: ... not freed (neg refs)" in the dmesg(8) output.

I think the logic around negative `refcnt' is much worse then panic,
because we trying hide the problem instead of catch and fix it.

ok mvs@ unless someone else denies this diff.

> bluhm
> 
> Index: net/route.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/net/route.c,v
> retrieving revision 1.410
> diff -u -p -r1.410 route.c
> --- net/route.c       5 May 2022 13:57:40 -0000       1.410
> +++ net/route.c       27 Jun 2022 13:03:16 -0000
> @@ -488,40 +488,34 @@ rt_putgwroute(struct rtentry *rt)
>  void
>  rtref(struct rtentry *rt)
>  {
> -     atomic_inc_int(&rt->rt_refcnt);
> +     refcnt_take(&rt->rt_refcnt);
>  }
>  
>  void
>  rtfree(struct rtentry *rt)
>  {
> -     int              refcnt;
> -
>       if (rt == NULL)
>               return;
>  
> -     refcnt = (int)atomic_dec_int_nv(&rt->rt_refcnt);
> -     if (refcnt <= 0) {
> -             KASSERT(!ISSET(rt->rt_flags, RTF_UP));
> -             KASSERT(!RT_ROOT(rt));
> -             atomic_dec_int(&rttrash);
> -             if (refcnt < 0) {
> -                     printf("rtfree: %p not freed (neg refs)\n", rt);
> -                     return;
> -             }
> +     if (refcnt_rele(&rt->rt_refcnt) == 0)
> +             return;
>  
> -             KERNEL_LOCK();
> -             rt_timer_remove_all(rt);
> -             ifafree(rt->rt_ifa);
> -             rtlabel_unref(rt->rt_labelid);
> +     KASSERT(!ISSET(rt->rt_flags, RTF_UP));
> +     KASSERT(!RT_ROOT(rt));
> +     atomic_dec_int(&rttrash);
> +
> +     KERNEL_LOCK();
> +     rt_timer_remove_all(rt);
> +     ifafree(rt->rt_ifa);
> +     rtlabel_unref(rt->rt_labelid);
>  #ifdef MPLS
> -             rt_mpls_clear(rt);
> +     rt_mpls_clear(rt);
>  #endif
> -             free(rt->rt_gateway, M_RTABLE, ROUNDUP(rt->rt_gateway->sa_len));
> -             free(rt_key(rt), M_RTABLE, rt_key(rt)->sa_len);
> -             KERNEL_UNLOCK();
> +     free(rt->rt_gateway, M_RTABLE, ROUNDUP(rt->rt_gateway->sa_len));
> +     free(rt_key(rt), M_RTABLE, rt_key(rt)->sa_len);
> +     KERNEL_UNLOCK();
>  
> -             pool_put(&rtentry_pool, rt);
> -     }
> +     pool_put(&rtentry_pool, rt);
>  }
>  
>  void
> @@ -877,7 +871,7 @@ rtrequest(int req, struct rt_addrinfo *i
>                       return (ENOBUFS);
>               }
>  
> -             rt->rt_refcnt = 1;
> +             refcnt_init(&rt->rt_refcnt);
>               rt->rt_flags = info->rti_flags | RTF_UP;
>               rt->rt_priority = prio; /* init routing priority */
>               LIST_INIT(&rt->rt_timer);
> @@ -1864,8 +1858,8 @@ db_show_rtentry(struct rtentry *rt, void
>  {
>       db_printf("rtentry=%p", rt);
>  
> -     db_printf(" flags=0x%x refcnt=%d use=%llu expire=%lld rtableid=%u\n",
> -         rt->rt_flags, rt->rt_refcnt, rt->rt_use, rt->rt_expire, id);
> +     db_printf(" flags=0x%x refcnt=%u use=%llu expire=%lld rtableid=%u\n",
> +         rt->rt_flags, rt->rt_refcnt.r_refs, rt->rt_use, rt->rt_expire, id);
>  
>       db_printf(" key="); db_print_sa(rt_key(rt));
>       db_printf(" plen=%d", rt_plen(rt));
> Index: net/route.h
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/net/route.h,v
> retrieving revision 1.194
> diff -u -p -r1.194 route.h
> --- net/route.h       5 May 2022 13:57:40 -0000       1.194
> +++ net/route.h       27 Jun 2022 13:03:16 -0000
> @@ -119,7 +119,7 @@ struct rtentry {
>       struct rt_kmetrics rt_rmx;      /* metrics used by rx'ing protocols */
>       unsigned int     rt_ifidx;      /* the answer: interface to use */
>       unsigned int     rt_flags;      /* up/down?, host/net */
> -     int              rt_refcnt;     /* # held references */
> +     struct refcnt    rt_refcnt;     /* # held references */
>       int              rt_plen;       /* prefix length */
>       uint16_t         rt_labelid;    /* route label ID */
>       uint8_t          rt_priority;   /* routing priority to use */
> Index: net/rtable.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/net/rtable.c,v
> retrieving revision 1.77
> diff -u -p -r1.77 rtable.c
> --- net/rtable.c      19 Apr 2022 15:44:56 -0000      1.77
> +++ net/rtable.c      27 Jun 2022 13:03:16 -0000
> @@ -680,7 +680,7 @@ rtable_delete(unsigned int rtableid, str
>               npaths++;
>  
>       if (npaths > 1) {
> -             KASSERT(rt->rt_refcnt >= 1);
> +             KASSERT(refcnt_read(&rt->rt_refcnt) >= 1);
>               SRPL_REMOVE_LOCKED(&rt_rc, &an->an_rtlist, rt, rtentry,
>                   rt_next);
>  
> @@ -694,7 +694,7 @@ rtable_delete(unsigned int rtableid, str
>       if (art_delete(ar, an, addr, plen) == NULL)
>               panic("art_delete failed to find node %p", an);
>  
> -     KASSERT(rt->rt_refcnt >= 1);
> +     KASSERT(refcnt_read(&rt->rt_refcnt) >= 1);
>       SRPL_REMOVE_LOCKED(&rt_rc, &an->an_rtlist, rt, rtentry, rt_next);
>       art_put(an);
>  
> Index: net/rtsock.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/net/rtsock.c,v
> retrieving revision 1.330
> diff -u -p -r1.330 rtsock.c
> --- net/rtsock.c      26 Jun 2022 16:07:00 -0000      1.330
> +++ net/rtsock.c      27 Jun 2022 13:03:16 -0000
> @@ -1992,7 +1992,7 @@ sysctl_dumpentry(struct rtentry *rt, voi
>               rtm->rtm_priority = rt->rt_priority & RTP_MASK;
>               rtm_getmetrics(&rt->rt_rmx, &rtm->rtm_rmx);
>               /* Do not account the routing table's reference. */
> -             rtm->rtm_rmx.rmx_refcnt = rt->rt_refcnt - 1;
> +             rtm->rtm_rmx.rmx_refcnt = refcnt_read(&rt->rt_refcnt) - 1;
>               rtm->rtm_index = rt->rt_ifidx;
>               rtm->rtm_addrs = info.rti_addrs;
>               rtm->rtm_tableid = id;
> 

Reply via email to