Re: IPFilter issue in -current

[Darren Reed <darrenr%netbsd.org@localhost>]

On 23/12/2012 8:17 PM, Geoff Adams wrote:> On Dec 22, 2012, at 11:05 PM, Darren 
Reed <darrenr%netbsd.org@localhost> wrote:
>
>> ok, now that I've studied the patches...
>>
>> The change to ip_nat.c (adding a MUTEX_DESTROY()) is a no-op.
>>
>> The reason for this is that when ipf_nat_insert() fails (as is
>> with the case for which you are adding code), it is assumed that
>> the "nat" pointer can be free'd without any further action. So
>> calling MUTEX_DESTROY() there serves no purpose as nothing is
>> going to touch that lock again.
>
> But it's still allocated as a lock. Running the kernel with
> LOCKDEBUG enabled picks this up, and dutifully panics when the
> pointer containing an allocated (but inactive, in this case)
> lock is freed.

And if there were a path for testing with ipftest, it'd crash
out too (ipftest checks to ensure all locks have been cleaned
up before exiting cleanly) so that change does belong.

>> The change to ip_state.c that moves MUTEX_EXIT(&is->is_lock)
>> I'm also unsure of. The code below that MUTEX_EXIT() is always
>> going to be running solo - nothing else will be using that
>> ipstate_t structure as is_ref == 0.  The line that has
>> "is->is_ref = 0" should be replaced with an ASSERT around
>> that statement. That leaves us with how is fr_srctrack
>> guarded. When ipf_state_insert() is called, a read lock
>> on ipf_state is held and ipf_stinsert is held across the
>> call to ipf_ht_node_add(). When ipf_state_del() is called,
>> a write lock on ipf_state must be held. This should mean
>> that there are no modification operations on the rbtree
>> that happen in a manner that does not guarantee the caller
>> exclusivity.
>
> I moved that based on these comments in ipf_ht_node_add and ipf_ht_node_del:
>
> /* NOTE: THIS FUNCTION MUST BE CALLED WITH AN EXCLUSIVE LOCK THAT PREVENTS  */
> /*       ipf_ht_node_del FROM RUNNING CONCURRENTLY ON THE SAME htp.         */
>
> The call to ipf_ht_node_add on line 1078 in ip_state.c seems
> to be protected by is->is_lock, and so it seemed that that was
> the correct lock to use to guarantee exclusivity with ipf_ht_node_del,
> which moving the lock exit down a few lines accomplishes.
> I didn't notice the locks on ipf_state being held a couple function calls 
> above.
>
> So, with a read lock on ipf_state, multiple calls to ipf_ht_node_add
> could still occur simultaneously, right? Would that be a potential
> problem? Or would that never occur in the same rb tree?

On the add side, a mutex is used, ipf_stinsert, that is free'd just
after the ipf_ht_node_add() call fails.

...
>
> That really doesn't seem to help much, though. Besides the lack
> of source lines (and I wasn't able to get any using kernel gdb, either).
> I'm a little surprised why stack frames don't appear for the rb functions;
> even though they're implemented as macros, the macros define proper
> functions. Perhaps they get inlined away.

Yes. Often "-g -O0" is required to debug #defines and other things that
generate inlined code.

> Do you have unit tests for the RB tree code that stress test it?

Not yet and I can see that I'm going to need to do that next.

On 23/12/2012 8:20 PM, David Laight wrote:
> On Sun, Dec 23, 2012 at 06:05:34PM +1100, Darren Reed wrote:
>> ok, now that I've studied the patches...
>>
>> The change to ip_nat.c (adding a MUTEX_DESTROY()) is a no-op.
>>
>> The reason for this is that when ipf_nat_insert() fails (as is
>> with the case for which you are adding code), it is assumed that
>> the "nat" pointer can be free'd without any further action. So
>> calling MUTEX_DESTROY() there serves no purpose as nothing is
>> going to touch that lock again.
> 
> I had a feeling that, on some OS at least (Solaris?), a mutex
> contained a pointer to dynamically allocated memeory making it
> necessary to actually 'destroy' it?

IIRC, all that it does on debug kernels is put a special value
in the structure saying it is no longer a valid lock.


Darren