2002-01-31 23:58:47

by Dave Hansen

Subject: Revealing unload_lock to everyone

This came up in a conversation about ieee1394_core.c. In 2.5.3, the BKL
is used to protect a module from being unloaded. The code looks like this:

lock_kernel();
read_lock(&ieee1394_chardevs_lock);
file_ops = ieee1394_chardevs[blocknum].file_ops;
module = ieee1394_chardevs[blocknum].module;
read_unlock(&ieee1394_chardevs_lock);
...
INCREF(module);
unlock_kernel();


The question is, how can we keep the module from being unloaded between
the file_ops assignment, and the INCREF. Do we have a general purpose
way, other than the BKL, to keep a module from being unloaded? There is
unload_lock, but it is static to module.c. We can always make it
global, but is there a better solution?

--
Dave Hansen
[email protected]

2002-02-01 01:03:27

by Keith Owens

Subject: Re: Revealing unload_lock to everyone

On Thu, 31 Jan 2002 15:58:17 -0800,
Dave Hansen <[email protected]> wrote:
>This came up in a conversation about ieee1394_core.c. In 2.5.3, the BKL
>is used to protect a module from being unloaded. The code looks like this:
>
> lock_kernel();
> read_lock(&ieee1394_chardevs_lock);
> file_ops = ieee1394_chardevs[blocknum].file_ops;
> module = ieee1394_chardevs[blocknum].module;
> read_unlock(&ieee1394_chardevs_lock);
> ...
> INCREF(module);
> unlock_kernel();
>
>
>The question is, how can we keep the module from being unloaded between
>the file_ops assignment, and the INCREF. Do we have a general purpose
>way, other than the BKL, to keep a module from being unloaded? There is
>unload_lock, but it is static to module.c. We can always make it
>global, but is there a better solution?

try_inc_mod_count().

2002-02-01 15:35:43

by Dave Hansen

Subject: Re: Revealing unload_lock to everyone

Horst von Brand wrote:
> Dave Hansen <[email protected]> said:
>
>>This came up in a conversation about ieee1394_core.c. In 2.5.3, the BKL
>>is used to protect a module from being unloaded. The code looks like
this:
>>
>> lock_kernel();
>> read_lock(&ieee1394_chardevs_lock);
>> file_ops = ieee1394_chardevs[blocknum].file_ops;
>> module = ieee1394_chardevs[blocknum].module;
>> read_unlock(&ieee1394_chardevs_lock);
>> ...
>> INCREF(module);
>> unlock_kernel();
>>
>>
>>The question is, how can we keep the module from being unloaded between
>>the file_ops assignment, and the INCREF. Do we have a general purpose
>>way, other than the BKL, to keep a module from being unloaded? There is
>>unload_lock, but it is static to module.c. We can always make it
>>global, but is there a better solution?
>>
>
> Move the INCREF() up?
>

This is really perverse, but here is why that doesn't work:

module not loaded
INCREF(module); /* this fails, no module loaded*/
interrupt, blah, blah, blah
now module is loaded by insmod or something
module = ieee1394_chardevs[blocknum].module;
module now set, but no refcnt bump has been done because it's newly loaded.
module removed
try to set something which went with the module
*BAM*

So, instead, we used try_mod_inc_count() instead of the local INCREF()
#define and return failure if try_mod_inc_count() fails. Thanks to
Keith Ownens for pointing me to try_mod_inc_count().

--
Dave Hansen
[email protected]