netdev - Re: [PATCH 1/2] Revert "genetlink: fix family dump race"

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] [day] [month] [year] [list]

Message-ID: <CALnjE+rjaidk+KEw4VFA_C2LoXir7VBxRuaA0xajqGd+uUt1qw@mail.gmail.com>
Date:	Thu, 22 Aug 2013 14:27:26 -0700
From:	Pravin Shelar <pshelar@...ira.com>
To:	Johannes Berg <johannes@...solutions.net>
Cc:	David Miller <davem@...emloft.net>,
	netdev <netdev@...r.kernel.org>, Thomas Graf <tgraf@...g.ch>
Subject: Re: [PATCH 1/2] Revert "genetlink: fix family dump race"

On Thu, Aug 22, 2013 at 1:36 PM, Johannes Berg
<johannes@...solutions.net> wrote:
> On Thu, 2013-08-22 at 13:26 -0700, David Miller wrote:
>
>> How to deal with the problem we were attempting to solve is still
>> under discussion.  It seems that even if we go to RCU we must also
>> address to module reference count issue.
>
> Yes, that's a separate issue. However, I think we should also check in
> more detail the dumpit locking issue Pravin pointed out - before his
> changes there was indeed the genl lock used as the cb_mutex. As I've
> said over in the other thread, I'm not sure that change was actually
> useful - it sounded like he confused kernel sockets and userland sockets
> here? (Or maybe I am?!)
>
> OTOH, we've already fixed the race conditions that resulted from his
> patch, at least in nl80211. You might remember the issue Linus ran into
> with the attrbuf, it's looking like that issue was because he changed
> generic netlink to no longer use the genl_lock as the cb_mutex.
>
>> I think the existing locking is very messy, and RCU looks a lot
>> cleaner and has potential for future improvements to the scalability
>> of dumps.
>
> I agree, though we're not all that interested in generic netlink family
> scalability I think, we have less than a dozen families, so this
> shouldn't really be an issue.
>
>> So I'd like to propose that we combine Johannes's RCU conversion
>> with some variant of the module reference count fix.
>>
>> Can you guys work together and come up with something I can apply?
>
> Sure, that in itself isn't really a problem, but if we don't take
> Pravin's patch to "revert" the cb_mutex change in his parallel_ops
> changes, then we definitely need to audit all generic netlink dumpit
> implementations in all users to see if they have similar races to
> nl80211. I originally thought that it was an nl80211 problem, but I'm
> now convinced that it wasn't. Still the new code in nl80211 is probably
> nicer, and we can probably make it parallel_ops now due to these changes
> but I'm not convinced we can audit all genl families.
>

I have sent fixes for genl-locking.

> If it wasn't that so much time has already passed since the parallel_ops
> changes I'd almost suggest reverting those altogether and addressing the
> locking properly ...
>
I think genl-parallel ops patch decouple genl-locking from netlink
lock which simplifies it. This also allows existing genl-family
gradually converted to parallel-ops. Once all are moved to parallel
ops, we can get rid of genl_mutex completely.

Passing mutex after one module to another is messy and confusing.
After genl-parallel op changes this is done only by rtnl module.
I will try to get rid of this cb_mutex parameter so that each layer
can have independent locks. I need to check rtnl module for that.
--
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