netdev - Re: [PATCH 2/5] dccp: Auto-load (when supported) CCID plugins for negotiation

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 for Android: free password hash cracker in your pocket

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

Date:	Tue, 16 Dec 2008 06:55:41 +0100
From:	Gerrit Renker <gerrit@....abdn.ac.uk>
To:	Micha? Miros?aw <mirqus@...il.com>
Cc:	davem@...emloft.net, dccp@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH 2/5] dccp: Auto-load (when supported) CCID plugins for
	negotiation

| > | > +static int ccid_request_module(u8 id)
| > | > +{
| > | > +       if (!in_atomic()) {
| > | > +               ccids_read_lock();
| > | > +               if (ccids[id] == NULL) {
| > | > +                       ccids_read_unlock();
| > | > +                       return request_module("net-dccp-ccid-%d", id);
| > | > +               }
| > | > +               ccids_read_unlock();
| > | > +       }
| > | > +       return 0;
| > | > +}
| > |
| > | Just a random thought: does this lock really do anything useful here?
| > |
| > Reading the (shared) 'ccids' array is the solution chosen to check whether
| > the module for CCID with number 'id' is already loaded.
| >
| > It would be bad to call request_module() each time a new DCCP socket is
| > opened. Using the 'ccids' array may not be the only way to check whether
| > a given module (whose name depends on the value of 'id') is loaded.
| >
| > But if this solution is chosen, then it requires to protect the read
| > access to 'ccids', which is shared among all DCCP sockets.
| 
| Since the lock is dropped after checking ccids[id] then there's
| a window where multiple request_module()s can be called if multiple
| applications create a DCCP socket at a same time. The code below
| should do the same without a lock (ccids is a static array,
| so ccids[N] is always at the same place).
| 
| static int ccid_request_module(u8 id)
| {
|        if (!in_atomic()) {
|                rmb();
|                if (ccids[id] == NULL)
|                        return request_module("net-dccp-ccid-%d", id);
|        }
|        return 0;
| }
| 
Sorry Michael, but this is really just a "random thought". What you are
in effect saying is that reader/writer locks can be replaced with just a
read memory barrier.

Please have a more detailed look at net/dccp/ccid.c. I also checked how
other subsystems handle comparable situations of module loading: the 
implementation details differ, but the principle is the same: there are
mutexes, semaphores, and spinlocks in use to protect those shared
structures that are related to the loaded module.

Hence your suggestion does not improve the code. I maintain that it is
correct. And it has proven to work in the test tree for more than one
year, including tests with up to 100 parallel (iperf) connections.
--
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