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 */
>
