Re: Fileassoc locking

[Elad Efrat <elad%NetBSD.org@localhost>]

On Sat, Dec 26, 2009 at 8:45 PM, Mindaugas Rasiukevicius
<rmind%netbsd.org@localhost> wrote:
> Elad Efrat <elad%NetBSD.org@localhost> wrote:
>> > - fileassoc_table_lookup() or its uses are missing locking (reference does
>> >  not provide exclusion).  Thus, fileassoc_table does not need
>> > ref-counting, unless for workarounds in fileassoc_table_run(), see below.
>> >  Use of RW-lock would fit in this case.
>>
>> Why do we need to hold the table exclusively in fileassoc_table_lookup()?
>
> See below.
>
>> > - fileassoc_file_lookup() is wrong: fileassoc_table should be locked when
>> >  performing lookup, it does not need ref-counting.  Also, no need to hold
>> >  a reference on each fileassoc_file entry, reference must be held and kept
>> >  for caller on entry which was found.
>>
>> Hmm... Why should the table be locked? If we hold a reference, and
>> modifiers of the table will not modify it until they hold it
>> exclusively, why should we lock it...? Also, I'm holding a reference
>> to the file because we use it in the comparisons (of the file handle
>> etc.) -- this is to prevent it from being modified while in use.
>
> And here you duplicate basically the same what rwlock or mutex provides
> for you (i.e. exclusion, just hardcoded).  There is no reason for that.
>
> Same point with other mentioned cases, so I will skip them.

Well, this is my understanding as well. However, the reply I received
was that rwlocks are expensive and one should use mutexes and
condition variables instead. As I don't pretend to be fluent with any
of our locking/synchronization mechanisms, I simply accepted that as
true, and changed my use of rwlocks to that of mutexes and condition
variables.

Let's try to separate the question: Do you see anything wrong with the
locking protocol now in use? I.e. if the same thing was done using
rwlocks or what have you, would it be "okay"? (Assume that the purpose
is to provide locking in fileassoc, rather than provide the *best*
locking in fileassoc.)

(Perhaps a man-page like memoryallocators(9) is warranted for kernel
locking/synchronization mechanisms... Maybe there is one that I'm not
aware of. :)

>> As for returning the entry with a reference -- I'll do that once the
>> rest of the locking is "done" because it'll require changes to users
>> of fileassoc(9)...
>
> What needs fileassoc_release()?

Probably code in Veriexec and PaX that uses fileassoc(9) to retrieve
and manipulate its private data rather than just check that it's
there. I've started looking, but can't say I have it laid out yet.

>> > - Why fileassoc_file_add() and fileassoc_file_delete() are waiting for
>> > zero references?  It does not look to me that fileassoc_file requires
>> > condvar, e.g. last reference holder can free it.   Also,
>> > fileassoc_file_add() checks for duplicates - I think caller (of API)
>> > should do that.
>>
>> <...>
>>
>> As for your suggestion that the KPI user should check for duplicates
>> -- note that to the KPI user, a "file" is really a combination of
>> "file" and "assoc," which is properly detected in fileassoc_add()
>> where EEXIST is returned if you're trying to add a combination that
>> already exists. Otherwise, the "file" part is used internally, so the
>> KPI user can't (and shouldn't) check whether it exists or not.
>
> Why not?  Caller can use fileassoc_lookup().  And if it does, then
> fileassoc_add() performs unnecessary lookup.

Okay, because this is another issue unrelated to locking, let's
discuss it in a different thread once we're done with the locking bit.
:) (Generally, though, I accept what you're saying and don't mind
changing it, but let's focus on locking first please...)

Thanks,

-e.