Re: [PATCH 2/7] CAN: Add PF_CAN core module

Urs Thuermann
2007-09-20T11:35:44Z

Patrick McHardy <kaber@trash.net> writes:

> 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

Ah, ok. Thanks for that hint. I will add it that way.

> > 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.

Well, unloading a module doesn't usually cause to operation to fail
when auto loading is enabled. It only wouldn't succeed when the
unload happens in the small window between test/request-module and
call to try_module_get(). This looks ugly to me. But the lock you
described above would also solve this.

> 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.

Well, I finally (hopefully) got it and I have changed the code
accordingly. Thanks for your explanation.

I will post our updated code again, probably today. The issues still
left are

* module parameter for loopback, but we want to keep that.
* configure option for allowing normal users access to raw and bcm CAN
sockets. I'll check how easily an (embedded) system can be set up
to run relevant/all processes with the CAP_NEW_RAW capability. I
would like to kill that configure option.
* seq_files for proc fs. On my TODO list.

urs