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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <e2d7436a0812160331h63152b26kd4aad6b449ed7f8@mail.gmail.com>
Date:	Tue, 16 Dec 2008 12:31:39 +0100
From:	"Michał Mirosław" <mirqus@...il.com>
To:	"Gerrit Renker" <gerrit@....abdn.ac.uk>
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

2008/12/16 Gerrit Renker <gerrit@....abdn.ac.uk>:
> | 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.

The read-lock is just a memory barrier here (not a read memory barrier
as I wrote
before) and it's not needed here. If I read the code correctly you are
testing a single
pointer to be NULL and don't really care about ordering wrt module
initialization.

You can actually annotate ccids[] as read_mostly (it's changed only on module
load/unload) and protect it with RCU instead of the home-grown rwlock
you are using.

Best Regards,
Michał Mirosław