Reference count race window

[Gumpula, Suresh]

Thanks a lot for suggestions/comments  Alfred/Julian/Poul Henning.   I will instrument as per Alfred suggestion first and will see if misuse of crfree is the root cause of corruption than a race window.

By the way , what are all the ways  we have in freebsd to debug memory corruptions.  I am aware of   options DEBUG_REDZONE( overflow detection) ,  DEBUG_MEMGAURD( for a malloc type),  MALLOC_DEBUG_MAXZONES  and some INVARIANTS in 
Kern_malloc.c  which caches the  malloc_type that most recently freed .   And any more corruption debugging methods we already have ?

Thanks
Suresh

-----Original Message-----
From: Alfred Perlstein [mailto:bright at mu.org] 
Sent: Thursday, January 02, 2014 9:53 PM
To: Gumpula, Suresh; Julian Elischer; freebsd-hackers at freebsd.org
Subject: Re: Reference count race window


On 1/2/14, 6:38 PM, Gumpula, Suresh wrote:
> Hi Alfred,
>        I agree that there could have been an extra/invalid  crfree() which decremented the count  and  looks valid  crhold(acquire) from socket code  panic'ed in my case. As per your suggestion if we
>   replace  the assert with if condition in release,  we will end up panicing  when the actual  crfree() happens.  But we may not be knowing who crfree'ed() in the first and invalid place.  Am I correct?   I will try your suggestion.
>
> Can you please bit more explain your array trick ?

I think the simplest thing to do would be to replace crfree with a macro to pass __FILE__ and __LINE__ down into the actual crfree (or you can use builtin_return_address to get the stack address of the caller instead.

Then just either have an array of {file,line} tuples or instead just return addresses in the struct.

Just extend struct ucred and add this:

#define MAX_PREV 10
const char *files[MAX_PREV];
int lines[MAX_PREV];

Then hack your own version of refcount_release(), call it
refcount_release2() but have it take a pointer to an integer that it will write the value of the old refcount into.

Then you can use that return value like so:

if (old_refcount < MAX_PREV) {
   cred->files[old_refcount] = pointer_to_file_name;
   cred->lines[old_refcount] = line_number; }

Then add the assert.  or better yet, turn on INVARIANTS (or maybe try each option in turn as INVARIANTS might hide the bug).

When you crash you can then see the last few callers who did free inside the struct.

Since the refcount "old_refcount" is atomically manipulated you should see the last few frees that send you negative.

-Alfred

>
> Thanks
> Suresh
>
>
>
> -----Original Message-----
> From: owner-freebsd-hackers at freebsd.org 
> [mailto:owner-freebsd-hackers at freebsd.org] On Behalf Of Alfred 
> Perlstein
> Sent: Thursday, January 02, 2014 8:21 PM
> To: Gumpula, Suresh; Julian Elischer; freebsd-hackers at freebsd.org
> Subject: Re: Reference count race window
>
>
> On 1/2/14, 3:53 PM, Gumpula, Suresh wrote:
>>>> Without changing the return-value semantics of refcount_acquire, we 
>>>> have introduced a panic if we detected a race as below.
>>>> static __inline void
>>>> refcount_acquire(volatile u_int *count) {
>>>>            u_int old;
>>>>
>>>>            old = atomic_fetchadd_int(count, 1);
>>>>            if (old == 0) {
>>>>              panic("refcount_acquire race condition detected!\n");
>>>>            }
>>>>>>> so what is the stacktrace of the panic?
>> It's from the socket code calling crhold.   It's a non debug build( NO INVARIANTS )
>>
>> #4  0xffffffff80331d34 in panic (fmt=0xffffffff805c1e60 
>> "refcount_acquire race condition detected!\n") at
>> ../../../../sys/kern/kern_shutdown.c:1009
>> #5  0xffffffff80326662 in refcount_acquire (count=<optimized out>) at
>> ../../../../sys/sys/refcount.h:65
>> #6  crhold (cr=<optimized out>) at
>> ../../../../sys/kern/kern_prot.c:1814
>> #7  0xffffffff803aa0d9 in socreate (dom=<optimized out>, 
>> aso=0xffffff80345c1b00, type=<optimized out>, proto=0, 
>> cred=0xffffff0017d7aa00, td=0xffffff000b294410) at
>> ../../../../sys/kern/uipc_socket.c:441
>> #8  0xffffffff803b2e5c in socket (td=0xffffff000b294410,
>> uap=0xffffff80345c1be0) at ../../../../sys/kern/uipc_syscalls.c:201
>> #9  0xffffffff80539ecb in syscall (frame=0xffffff80345c1c80) at
>> ../../../../sys/amd64/amd64/trap.c:1260
>>
> If it's a non-debug build then how do you know that someone isn't incorrectly lowering the refcount?
>
> Please try some invariants or at least manually turn on the one KASSERT I mentioned.
>
> Another trick would be to add a an array of char*+int for the last few places that decremented, you can use the returned refcount as an index to that array to track who may be doing the extra frees.
>
> -Alfred
>
> _______________________________________________
> freebsd-hackers at freebsd.org mailing list 
> http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
> To unsubscribe, send any mail to "freebsd-hackers-unsubscribe at freebsd.org"
>