netdev - Re: [PATCH net] sctp: fix race on protocol/netns initialization

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:	Thu, 10 Sep 2015 13:24:54 -0300
From:	Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
To:	Vlad Yasevich <vyasevich@...il.com>
Cc:	netdev@...r.kernel.org, Neil Horman <nhorman@...driver.com>,
	linux-sctp@...r.kernel.org
Subject: Re: [PATCH net] sctp: fix race on protocol/netns initialization

On Thu, Sep 10, 2015 at 11:50:06AM -0400, Vlad Yasevich wrote:
> On 09/10/2015 10:22 AM, Marcelo Ricardo Leitner wrote:
> > Em 10-09-2015 10:24, Vlad Yasevich escreveu:
...
> >> Then you can order sctp_net_init() such that it happens first, then protosw registration
> >> happens, then control socket initialization happens, then inet protocol registration
> >> happens.
> >>
> >> This way, we are always guaranteed that by the time user calls socket(), protocol
> >> defaults are fully initialized.
> > 
> > Okay, that works for module loading stage, but then how would we handle new netns's? We
> > have to create the control socket per netns and AFAICT sctp_net_init() is the only hook
> > called when a new netns is being created.
> > 
> > Then if we move it a workqueue that is scheduled by sctp_net_init(), we loose the ability
> > to handle its errors by propagating through sctp_net_init() return value, not good.
> 
> Here is kind of what I had in mind.  It's incomplete and completely untested (not even
> compiled), but good enough to describe the idea:
...

Ohh, ok now I get it, thanks. If having two pernet_subsys for a given
module is fine, that works for me. It's clearer and has no moment of
temporary failure.

I can finish this patch if everybody agrees with it.

> >>> I used the list pointer because that's null as that memory is entirely zeroed when alloced
> >>> and, after initialization, it's never null again. Works like a lock/condition without
> >>> using an extra field.
> >>>
> >>
> >> I understand this a well.  What I don't particularly like is that we are re-using
> >> a list without really stating why it's now done this way.  Additionally, it's not really
> >> the last that happens so it's seems kind of hacky...  If we need to add new
> >> per-net initializers, we now need to make sure that the code is put in the right
> >> place.  I'd just really like to have a cleaner solution...
> > 
> > Ok, got you. We could add a dedicated flag/bit for that then, if reusing the list is not
> > clear enough. Or, as we are discussing on the other part of thread, we could make it block
> > and wait for the initialization, probably using some wait_queue. I'm still thinking on
> > something this way, likely something more below than sctp then.
> > 
> 
> I think if we don the above, the second process calling socket() will either find the
> the protosw or will try to load the module also.  I think either is ok after
> request_module returns we'll look at the protosw and will find find things.

Seems so, yes. Nice.

  Marcelo

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