netdev - Re: [PATCH 2/7] CAN: Add PF

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	20 Sep 2007 13:30:52 +0200
From:	Urs Thuermann <urs@...ogud.escape.de>
To:	Patrick McHardy <kaber@...sh.net>
Cc:	netdev@...r.kernel.org, David Miller <davem@...emloft.net>,
	Thomas Gleixner <tglx@...utronix.de>,
	Oliver Hartkopp <oliver@...tkopp.net>,
	Oliver Hartkopp <oliver.hartkopp@...kswagen.de>
Subject: Re: [PATCH 2/7] CAN: Add PF_CAN core module

Patrick McHardy <kaber@...sh.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
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html