Urs Thuermann wrote:
> Patrick McHardy <[EMAIL PROTECTED]> writes:
>
>>>When the module is unloaded it calls can_proto_unregister() which
>>>clears the pointer. Do you see a race condition here?
>>
>>Yes, you do request_module, load the module, get the cp pointer
>>from proto_tab, the module is unloaded again. cp points to
>>stable memory. Using module references would fix this.
>
>
> How would I use the module reference counter? Somehow with
> try_module_get()? I have thought something like
>
> cp = proto_tab[protocol];
> if (!cp ...)
> return ...;
>
> if (!try_module_get(cp->prot->owner))
> return ...;
>
> sk = sk_alloc(...)
>
> module_put(...);
> return ret;
>
> But here I see two problems:
>
> 1. Between the check !cp... and referencing cp->prot->owner the
> module could get unloaded and the reference be invalid. Is there
> some lock I can hold that prevents module unloading? I haven't
> found something like this in include/linux/module.h
No, you need to add your own locking to prevent this, something
list this:
registration/unregistration:
take lock
change proto_tab[]
release lock
lookup:
take lock
cp = proto_tab[]
if (cp && !try_module_get(cp->owner))
cp = NULL
release lock
> 2. If the module gets unloaded after the first check and
> request_module() but before the call to try_module_get() the
> socket() syscall will return with error, although module auto
> loading would normally be successful. How can I prevent that?
Why do you want to prevent it? The admin unloaded the module,
so he apparently doesn't want the operation to succeed.
>>>find_dev_rcv_lists() is called in one place from can_rcv() with RCU
>>>lock held, as you write. The other two calls to find_dev_rcv_lists()
>>>are from can_rx_register/unregister() functions which change the
>>>receive lists. Therefore, we can't only use RCU but need protection
>>>against simultanous writes. We do this with the spin_lock_bh(). The
>>>_bh variant, because can_rcv() runs in interrupt and we need to block
>>>that. I thought this is pretty standard.
>>>
>>>I'll check this again tomorrow, but I have put much time in these
>>>locking issues already, changed it quite a few times and hoped to have
>>>got it right finally.
>>
>>
>>I'm not saying you should use *only* RCU, you need the lock
>>for additions/removal of course, but since the receive path
>>doesn't take that lock and relies on RCU, you need to use
>>the _rcu list walking variant to avoid races with concurrent
>>list changes.
>
>
> I have no objections to add the _rcu suffix for the code changing the
> receive lists, but I don't see why it's necessary. When I do a
> spin_lock_bh() before writing, can't I be sure that there is no
> interrupt routine running in parallel while I hold this spinlock? If
> so, there is no reader in parallel because the can_rcv() function runs
> in a softirq. I'd really like to understand why you think the writers
> should also use the _rcu variant.
I'm saying you need _rcu for the *read side*. All operations changing
the list already use the _rcu variants.
> I'm sorry if I miss something
> obvious here, but could you try to explain it to me?
spin_lock_bh only disables BHs locally, other CPUs can still process
softirqs. And since rcv_lists_lock is only used in process context,
the BH disabling is actually not even necessary.
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
