On 15/09/21(Wed) 01:23, Vitaliy Makkoveev wrote: > We have weird `rt_refcnt' check in rtfree():
> > 497 rtfree(struct rtentry *rt) > 498 { > ... > 504 refcnt = (int)atomic_dec_int_nv(&rt->rt_refcnt); > 505 if (refcnt <= 0) { > 506 KASSERT(!ISSET(rt->rt_flags, RTF_UP)); > 507 KASSERT(!RT_ROOT(rt)); > 508 atomic_dec_int(&rttrash); > 509 if (refcnt < 0) { > 510 printf("rtfree: %p not freed (neg refs)\n", rt); > 511 return; > 512 } > > We underflow `rt_refcnt' when we missed to get reference to this `rt' or > we did extra release. This is the bug which should be exposed. But > according current code this `rt' is just leaked. Also it's easy to miss > this error condition because we only print error message. Yes, all of this is intentional. If you screw up reference counting a leak is better than a crash that you can't debug. At least people can report there is a leak. > I propose to put "rt->rt_refcnt > 0" assertion before we decrement > `rt_refcnt'. This makes reference counting errors more notable when they > are. Also I changed `rt_refcnt' definition to unsigned integer. Such assert isn't safe because the value can be dereferenced by another thread. You can only assert for the value of a private variable or if you're under a critical section. > I didn't find any "rtfree: ... not freed (neg refs)" report, so it > looks like we can't hit this assertion, but I like to commit this > diff after release. But nothing stops to test it and provide feedback :) We did hit it many many times in the past. Please do not change this. > Index: sys/net/route.c > =================================================================== > RCS file: /cvs/src/sys/net/route.c,v > retrieving revision 1.399 > diff -u -p -r1.399 route.c > --- sys/net/route.c 25 May 2021 22:45:09 -0000 1.399 > +++ sys/net/route.c 14 Sep 2021 21:47:11 -0000 > @@ -496,20 +496,15 @@ rtref(struct rtentry *rt) > void > rtfree(struct rtentry *rt) > { > - int refcnt; > - > if (rt == NULL) > return; > > - refcnt = (int)atomic_dec_int_nv(&rt->rt_refcnt); > - if (refcnt <= 0) { > + KASSERT(rt->rt_refcnt > 0); > + > + if (atomic_dec_int_nv(&rt->rt_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; > - } > > KERNEL_LOCK(); > rt_timer_remove_all(rt); > Index: sys/net/route.h > =================================================================== > RCS file: /cvs/src/sys/net/route.h,v > retrieving revision 1.185 > diff -u -p -r1.185 route.h > --- sys/net/route.h 17 Mar 2021 09:05:42 -0000 1.185 > +++ sys/net/route.h 14 Sep 2021 21:47:11 -0000 > @@ -113,7 +113,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 */ > + unsigned int rt_refcnt; /* # held references */ > int rt_plen; /* prefix length */ > uint16_t rt_labelid; /* route label ID */ > uint8_t rt_priority; /* routing priority to use */ >