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

[Vlad Yasevich]

On 09/09/2015 05:06 PM, Marcelo Ricardo Leitner wrote:
> Em 09-09-2015 17:30, Vlad Yasevich escreveu:
>> On 09/09/2015 04:03 PM, Marcelo Ricardo Leitner wrote:
>>> Consider sctp module is unloaded and is being requested because an user
>>> is creating a sctp socket.
>>>
>>> During initialization, sctp will add the new protocol type and then
>>> initialize pernet subsys:
>>>
>>>  status = sctp_v4_protosw_init();
>>>  if (status)
>>>  goto err_protosw_init;
>>>
>>>  status = sctp_v6_protosw_init();
>>>  if (status)
>>>  goto err_v6_protosw_init;
>>>
>>>  status = register_pernet_subsys(_net_ops);
>>>
>>> The problem is that after those calls to sctp_v{4,6}_protosw_init(), it
>>> is possible for userspace to create SCTP sockets like if the module is
>>> already fully loaded. If that happens, one of the possible effects is
>>> that we will have readers for net->sctp.local_addr_list list earlier
>>> than expected and sctp_net_init() does not take precautions while
>>> dealing with that list, leading to a potential panic but not limited to
>>> that, as sctp_sock_init() will copy a bunch of blank/partially
>>> initialized values from net->sctp.
>>>
>>> The race happens like this:
>>>
>>>   CPU 0   |  CPU 1
>>>socket()   |
>>> __sock_create | socket()
>>>  inet_create  |  __sock_create
>>>   list_for_each_entry_rcu(|
>>>  answer, [sock->type], |
>>>  list) {  |   inet_create
>>>/* no hits */  |
>>>   if (unlikely(err)) {|
>>>...|
>>>request_module()   |
>>>/* socket creation is blocked  |
>>> * the module is fully loaded  |
>>> */|
>>> sctp_init |
>>>  sctp_v4_protosw_init |
>>>   inet_register_protosw   |
>>>list_add_rcu(>list, |
>>> last_perm);   |
>>>   |  list_for_each_entry_rcu(
>>>   | answer, [sock->type],
>>>  sctp_v6_protosw_init | list) {
>>>   | /* hit, so assumes protocol
>>>   |  * is already loaded
>>>   |  */
>>>   |  /* socket creation continues
>>>   |   * before netns is initialized
>>>   |   */
>>>  register_pernet_subsys   |
>>>
>>> Inverting the initialization order between register_pernet_subsys() and
>>> sctp_v4_protosw_init() is not possible because register_pernet_subsys()
>>> will create a control sctp socket, so the protocol must be already
>>> visible by then. Deferring the socket creation to a work-queue is not
>>> good specially because we loose the ability to handle its errors.
>>>
>>> So the fix then is to invert the initialization order inside
>>> register_pernet_subsys() so that the control socket is created by last
>>> and also block socket creation if netns initialization wasn't yet
>>> performed.
>>>
>>
>> not sure how much I like that...  Wouldn't it be better
>> to pull the control socket initialization stuff out into its
>> own function that does something like
>>
>> for_each_net_rcu()
>> init_control_socket(net, ...)
>>
>>
>> Or may be even pull the control socket creation
>> stuff completely into its own per-net ops operations structure
>> and initialize it after the the protosw stuff has been done.
>>
>> -vlad
> 
> I'm afraid error handling won't be easy then.
> 
> But still, the control socket is not really the problem, because we don't 
> care (much?) if
> it contains zeroed values and the panic happens only if you call connect() on 
> it. I moved
> it solely because of the protection on sctp_init_sock().
> 
> The real problem is new sockets created by an user application while module 
> is still
> loading, because even if them don't trigger the panic, they may not be fully 
> functional
> due to improper values loaded. Can't see other good ways to protect 
> sctp_init_sock() from
> that early call (as in, prior to netns initialization).

Right, I understand what the problem really is.  Like you said, the simple fix 
is to
reorder the sctp defaults initialization with protosw registration.  However, 
that's
not possible because control socket is created in the sctp defaults 
initialization code
and needs protosw to be registered (chicken and egg issue).

What I am saying is that it is kind of strange to create control socket during 
protocol
default initialization.  The control socket has nothing  really to do with 
defaults.  So,
we could pull it out of the defaults 

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

[Marcelo Ricardo Leitner]

Em 09-09-2015 21:16, David Miller escreveu:

From: Marcelo Ricardo Leitner 
Date: Wed,  9 Sep 2015 17:03:01 -0300


So the fix then is to invert the initialization order inside
register_pernet_subsys() so that the control socket is created by last
and also block socket creation if netns initialization wasn't yet
performed.


If we really need to we could make ->create() fail with -EAFNOSUPPORT
if kern==1 until the protocol is fully setup.

Or, instead of failing, we could make such ->create() calls block
until the control sock init is complete or fails.


I guess I should have written that paragraph in another order, perhaps like:
So the fix then is to deny any sctp socket creation until netns 
initialization is sufficiently done. And due to that, we have to 
initialize the control socket as last step in netns initialization, as 
now it can't be created earlier anymore.


Is it clearer on the intention?

And my emphasis on userspace sockets was to highlight that a random user 
could trigger it, but yes both users are affected by the issue.


Strictly speaking, we would have to block ->create() not until the 
control socket init is done but until the protocol is fully loaded. Such 
condition, with this patch, is after net->sctp.auto_asconf_splist is 
initialized. But for blocking until instead of just denying, we would 
need some other mechanism.


It would be better from the (sctp) user point of view but then such 
solution may better belong to another layer instead and protect all 
protocols at once. (I checked and couldn't find other protocols at risk 
like sctp)



We have actually several visibility issues wrt. control sockets on
protocol init, in general.

For example, such control sockets can briefly be hashed and visible
to socket dumps and packet input.

A lot of really tricky issues involved here.


Agreed, but does these still apply after explaining that paragraph/the 
solution? I had no intention on visiting these issues with this patch, 
they are left unchanged, but I can if a better solution for the original 
issue calls for it.


  Marcelo

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH net] sctp: fix race on protocol/netns initialization

[David Laight]

From: Marcelo Ricardo Leitner
> Sent: 10 September 2015 13:54
> Em 09-09-2015 21:16, David Miller escreveu:
> > From: Marcelo Ricardo Leitner 
> > Date: Wed,  9 Sep 2015 17:03:01 -0300
> >
> >> So the fix then is to invert the initialization order inside
> >> register_pernet_subsys() so that the control socket is created by last
> >> and also block socket creation if netns initialization wasn't yet
> >> performed.
> >
> > If we really need to we could make ->create() fail with -EAFNOSUPPORT
> > if kern==1 until the protocol is fully setup.
> >
> > Or, instead of failing, we could make such ->create() calls block
> > until the control sock init is complete or fails.
> 
> I guess I should have written that paragraph in another order, perhaps like:
> So the fix then is to deny any sctp socket creation until netns
> initialization is sufficiently done. And due to that, we have to
> initialize the control socket as last step in netns initialization, as
> now it can't be created earlier anymore.
> 
> Is it clearer on the intention?
> 
> And my emphasis on userspace sockets was to highlight that a random user
> could trigger it, but yes both users are affected by the issue.
> 
> Strictly speaking, we would have to block ->create() not until the
> control socket init is done but until the protocol is fully loaded. Such
> condition, with this patch, is after net->sctp.auto_asconf_splist is
> initialized. But for blocking until instead of just denying, we would
> need some other mechanism.
> 
> It would be better from the (sctp) user point of view but then such
> solution may better belong to another layer instead and protect all
> protocols at once. (I checked and couldn't find other protocols at risk
> like sctp)

Given that the first ->create() blocks while the protocol code loads
it really wouldn't be right to error a subsequent ->create() because
the load hasn't completed.

I hold a semaphore across sock_create_kern() because of issues with sctp.
(Current kernels are nowhere near as bad as really old ones though.)

David

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH net] sctp: fix race on protocol/netns initialization

[David Laight]

From: Marcelo Ricardo 
> Sent: 10 September 2015 15:36
...
> > Given that the first ->create() blocks while the protocol code loads
> > it really wouldn't be right to error a subsequent ->create() because
> > the load hasn't completed.
> 
> Can't say I don't agree with you, but at the same time, there are other
> temporary errors that can happen and that the user should just retry.
> This would be just another condition in a trade off for avoiding complexity.

We do retry, but the delay messes up out test scripts :-(

> > I hold a semaphore across sock_create_kern() because of issues with sctp.
> > (Current kernels are nowhere near as bad as really old ones though.)
> 
> Oh, that's not good to hear. I'll experiment with that later, try to
> catch some bugs. :)

I mean REALLY old - like 2.6.12 (FC3).
I'm pretty sure I've seen oops as well as create failing.

We don't create enough sockets for the semaphore to be a problem.

OTOH I've a current problem with a customer using RHEL5.8 (basically 2.6.18).
They might manage to move to RHEL6 (2.6.32) - but that could take a year or two.
RH might be pulling some of the SCTP fixes, but I doubt they get priority.

David

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Marcelo Ricardo Leitner]

Em 10-09-2015 10:24, Vlad Yasevich escreveu:

On 09/09/2015 05:06 PM, Marcelo Ricardo Leitner wrote:

Em 09-09-2015 17:30, Vlad Yasevich escreveu:

On 09/09/2015 04:03 PM, Marcelo Ricardo Leitner wrote:

Consider sctp module is unloaded and is being requested because an user
is creating a sctp socket.

During initialization, sctp will add the new protocol type and then
initialize pernet subsys:

  status = sctp_v4_protosw_init();
  if (status)
  goto err_protosw_init;

  status = sctp_v6_protosw_init();
  if (status)
  goto err_v6_protosw_init;

  status = register_pernet_subsys(_net_ops);

The problem is that after those calls to sctp_v{4,6}_protosw_init(), it
is possible for userspace to create SCTP sockets like if the module is
already fully loaded. If that happens, one of the possible effects is
that we will have readers for net->sctp.local_addr_list list earlier
than expected and sctp_net_init() does not take precautions while
dealing with that list, leading to a potential panic but not limited to
that, as sctp_sock_init() will copy a bunch of blank/partially
initialized values from net->sctp.

The race happens like this:

   CPU 0   |  CPU 1
socket()   |
 __sock_create | socket()
  inet_create  |  __sock_create
   list_for_each_entry_rcu(|
  answer, [sock->type], |
  list) {  |   inet_create
/* no hits */  |
   if (unlikely(err)) {|
...|
request_module()   |
/* socket creation is blocked  |
 * the module is fully loaded  |
 */|
 sctp_init |
  sctp_v4_protosw_init |
   inet_register_protosw   |
list_add_rcu(>list, |
 last_perm);   |
   |  list_for_each_entry_rcu(
   | answer, [sock->type],
  sctp_v6_protosw_init | list) {
   | /* hit, so assumes protocol
   |  * is already loaded
   |  */
   |  /* socket creation continues
   |   * before netns is initialized
   |   */
  register_pernet_subsys   |

Inverting the initialization order between register_pernet_subsys() and
sctp_v4_protosw_init() is not possible because register_pernet_subsys()
will create a control sctp socket, so the protocol must be already
visible by then. Deferring the socket creation to a work-queue is not
good specially because we loose the ability to handle its errors.

So the fix then is to invert the initialization order inside
register_pernet_subsys() so that the control socket is created by last
and also block socket creation if netns initialization wasn't yet
performed.



not sure how much I like that...  Wouldn't it be better
to pull the control socket initialization stuff out into its
own function that does something like

for_each_net_rcu()
 init_control_socket(net, ...)


Or may be even pull the control socket creation
stuff completely into its own per-net ops operations structure
and initialize it after the the protosw stuff has been done.

-vlad


I'm afraid error handling won't be easy then.

But still, the control socket is not really the problem, because we don't care 
(much?) if
it contains zeroed values and the panic happens only if you call connect() on 
it. I moved
it solely because of the protection on sctp_init_sock().

The real problem is new sockets created by an user application while module is 
still
loading, because even if them don't trigger the panic, they may not be fully 
functional
due to improper values loaded. Can't see other good ways to protect 
sctp_init_sock() from
that early call (as in, prior to netns initialization).


Right, I understand what the problem really is.  Like you said, the simple fix 
is to
reorder the sctp defaults initialization with protosw registration.  However, 
that's
not possible because control socket is created in the sctp defaults 
initialization code
and needs protosw to be registered (chicken and egg issue).


Yes, same page then, cool.


What I am saying is that it is kind of strange to create control socket during 
protocol
default initialization.  The control socket has nothing  really to do with 
defaults.  So,
we could pull it out of the defaults initialization (sctp_net_init()) code and 
into its
own initialization path.


I don't really see sctp_net_init() as a pure defaults initialization 
routine. It's the callback for new netns's, so it should initialize 

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

[Marcelo Ricardo Leitner]

Em 10-09-2015 10:02, David Laight escreveu:

From: Marcelo Ricardo Leitner

Sent: 10 September 2015 13:54
Em 09-09-2015 21:16, David Miller escreveu:

From: Marcelo Ricardo Leitner 
Date: Wed,  9 Sep 2015 17:03:01 -0300


So the fix then is to invert the initialization order inside
register_pernet_subsys() so that the control socket is created by last
and also block socket creation if netns initialization wasn't yet
performed.


If we really need to we could make ->create() fail with -EAFNOSUPPORT
if kern==1 until the protocol is fully setup.

Or, instead of failing, we could make such ->create() calls block
until the control sock init is complete or fails.


I guess I should have written that paragraph in another order, perhaps like:
So the fix then is to deny any sctp socket creation until netns
initialization is sufficiently done. And due to that, we have to
initialize the control socket as last step in netns initialization, as
now it can't be created earlier anymore.

Is it clearer on the intention?

And my emphasis on userspace sockets was to highlight that a random user
could trigger it, but yes both users are affected by the issue.

Strictly speaking, we would have to block ->create() not until the
control socket init is done but until the protocol is fully loaded. Such
condition, with this patch, is after net->sctp.auto_asconf_splist is
initialized. But for blocking until instead of just denying, we would
need some other mechanism.

It would be better from the (sctp) user point of view but then such
solution may better belong to another layer instead and protect all
protocols at once. (I checked and couldn't find other protocols at risk
like sctp)


Given that the first ->create() blocks while the protocol code loads
it really wouldn't be right to error a subsequent ->create() because
the load hasn't completed.


Can't say I don't agree with you, but at the same time, there are other 
temporary errors that can happen and that the user should just retry. 
This would be just another condition in a trade off for avoiding complexity.



I hold a semaphore across sock_create_kern() because of issues with sctp.
(Current kernels are nowhere near as bad as really old ones though.)


Oh, that's not good to hear. I'll experiment with that later, try to 
catch some bugs. :)


  Marcelo

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Vlad Yasevich]

On 09/10/2015 10:22 AM, Marcelo Ricardo Leitner wrote:
> Em 10-09-2015 10:24, Vlad Yasevich escreveu:
>> On 09/09/2015 05:06 PM, Marcelo Ricardo Leitner wrote:
>>> Em 09-09-2015 17:30, Vlad Yasevich escreveu:
 On 09/09/2015 04:03 PM, Marcelo Ricardo Leitner wrote:
> Consider sctp module is unloaded and is being requested because an user
> is creating a sctp socket.
>
> During initialization, sctp will add the new protocol type and then
> initialize pernet subsys:
>
>   status = sctp_v4_protosw_init();
>   if (status)
>   goto err_protosw_init;
>
>   status = sctp_v6_protosw_init();
>   if (status)
>   goto err_v6_protosw_init;
>
>   status = register_pernet_subsys(_net_ops);
>
> The problem is that after those calls to sctp_v{4,6}_protosw_init(), it
> is possible for userspace to create SCTP sockets like if the module is
> already fully loaded. If that happens, one of the possible effects is
> that we will have readers for net->sctp.local_addr_list list earlier
> than expected and sctp_net_init() does not take precautions while
> dealing with that list, leading to a potential panic but not limited to
> that, as sctp_sock_init() will copy a bunch of blank/partially
> initialized values from net->sctp.
>
> The race happens like this:
>
>CPU 0   |  CPU 1
> socket()   |
>  __sock_create | socket()
>   inet_create  |  __sock_create
>list_for_each_entry_rcu(|
>   answer, [sock->type], |
>   list) {  |   inet_create
> /* no hits */  |
>if (unlikely(err)) {|
> ...|
> request_module()   |
> /* socket creation is blocked  |
>  * the module is fully loaded  |
>  */|
>  sctp_init |
>   sctp_v4_protosw_init |
>inet_register_protosw   |
> list_add_rcu(>list, |
>  last_perm);   |
>|  list_for_each_entry_rcu(
>| answer, [sock->type],
>   sctp_v6_protosw_init | list) {
>| /* hit, so assumes protocol
>|  * is already loaded
>|  */
>|  /* socket creation continues
>|   * before netns is initialized
>|   */
>   register_pernet_subsys   |
>
> Inverting the initialization order between register_pernet_subsys() and
> sctp_v4_protosw_init() is not possible because register_pernet_subsys()
> will create a control sctp socket, so the protocol must be already
> visible by then. Deferring the socket creation to a work-queue is not
> good specially because we loose the ability to handle its errors.
>
> So the fix then is to invert the initialization order inside
> register_pernet_subsys() so that the control socket is created by last
> and also block socket creation if netns initialization wasn't yet
> performed.
>

 not sure how much I like that...  Wouldn't it be better
 to pull the control socket initialization stuff out into its
 own function that does something like

 for_each_net_rcu()
  init_control_socket(net, ...)


 Or may be even pull the control socket creation
 stuff completely into its own per-net ops operations structure
 and initialize it after the the protosw stuff has been done.

 -vlad
>>>
>>> I'm afraid error handling won't be easy then.
>>>
>>> But still, the control socket is not really the problem, because we don't 
>>> care (much?) if
>>> it contains zeroed values and the panic happens only if you call connect() 
>>> on it. I moved
>>> it solely because of the protection on sctp_init_sock().
>>>
>>> The real problem is new sockets created by an user application while module 
>>> is still
>>> loading, because even if them don't trigger the panic, they may not be 
>>> fully functional
>>> due to improper values loaded. Can't see other good ways to protect 
>>> sctp_init_sock() from
>>> that early call (as in, prior to netns initialization).
>>
>> Right, I understand what the problem really is.  Like you said, the simple 
>> fix is to
>> reorder the sctp defaults initialization with protosw registration.  
>> However, that's
>> not possible because 

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

[Marcelo Ricardo Leitner]

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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Marcelo Ricardo Leitner]

On Thu, Sep 10, 2015 at 01:24:54PM -0300, Marcelo Ricardo Leitner wrote:
> 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.

I was testing with it, something is not good. I finished your patch and
testing with a flooder like:
 # for j in {1..5}; do for i in {1234..1280}; do \
   sctp_darn -H 192.168.122.147 -P $j$i -l & done & done

The system didn't crash, but I got:
[1] 13507
[2] 13508
[3] 13510
[4] 13513
[5] 13517
[mrl@localhost ~]$ sctp_darn: failed to create socket:  Socket type not
supported.
sctp_darn: failed to create socket:  Socket type not supported.
sctp_darn: failed to create socket:  Socket type not supported.
sctp_darn: failed to create socket:  Socket type not supported.
sctp_darn: failed to create socket:  Socket type not supported.
sctp_darn: failed to create socket:  Socket type not supported.
sctp_darn: failed to create socket:  Socket type not supported.
sctp_darn: failed to create socket:  Socket type not supported.
sctp_darn: failed to create socket:  Socket type not supported.
sctp_darn: failed to create socket:  Socket type not supported.
sctp_darn: failed to create socket:  Socket type not supported.
sctp_darn: failed to create socket:  Socket type not supported.
sctp_darn: failed to create socket:  Socket type not supported.
sctp_darn listening...
sctp_darn listening...
sctp_darn listening...
sctp_darn listening...
...
sctp_darn listening...
sctp_darn listening...
sctp_darn listening...
sctp_darn listening...
sctp_darn: failed to create socket:  Socket type not supported.
sctp_darn listening...
sctp_darn listening...

And with this applied:
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -289,6 +289,7 @@ lookup_protocol:
if (unlikely(err)) {
if (try_loading_module < 2) {
rcu_read_unlock();
+   printk("%p loading proto module\n", sock);
/*
 * Be more specific, e.g. net-pf-2-proto-132-type-1
 * (net-pf-PF_INET-proto-IPPROTO_SCTP-type-SOCK_STREAM)
@@ -303,6 +304,8 @@ lookup_protocol:
else
request_module("net-pf-%d-proto-%d",
   PF_INET, protocol);
+
+ 

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

[Marcelo Ricardo Leitner]

On Thu, Sep 10, 2015 at 03:35:20PM -0300, Marcelo Ricardo Leitner wrote:
> On Thu, Sep 10, 2015 at 01:24:54PM -0300, Marcelo Ricardo Leitner wrote:
> > 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.
> 
> I was testing with it, something is not good. I finished your patch and
> testing with a flooder like:

This is the patch I used. Mostly just fixed a few typos and added error 
handling.

diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
index 4345790ad326..8930046eaa1b 100644
--- a/net/sctp/protocol.c
+++ b/net/sctp/protocol.c
@@ -1178,7 +1178,7 @@ static void sctp_v4_del_protocol(void)
unregister_inetaddr_notifier(_inetaddr_notifier);
 }
 
-static int __net_init sctp_net_init(struct net *net)
+static int __net_init sctp_defaults_init(struct net *net)
 {
int status;
 
@@ -1271,12 +1271,6 @@ static int __net_init sctp_net_init(struct net *net)
 
sctp_dbg_objcnt_init(net);
 
-   /* Initialize the control inode/socket for handling OOTB packets.  */
-   if ((status = sctp_ctl_sock_init(net))) {
-   pr_err("Failed to initialize the SCTP control sock\n");
-   goto err_ctl_sock_init;
-   }
-
/* Initialize the local address list. */
INIT_LIST_HEAD(>sctp.local_addr_list);
spin_lock_init(>sctp.local_addr_lock);
@@ -1292,9 +1286,6 @@ static int __net_init sctp_net_init(struct net *net)
 
return 0;
 
-err_ctl_sock_init:
-   sctp_dbg_objcnt_exit(net);
-   sctp_proc_exit(net);
 err_init_proc:
cleanup_sctp_mibs(net);
 err_init_mibs:
@@ -1303,15 +1294,12 @@ err_sysctl_register:
return status;
 }
 
-static void __net_exit sctp_net_exit(struct net *net)
+static void __net_exit sctp_defaults_exit(struct net *net)
 {
/* Free the local address list */
sctp_free_addr_wq(net);
sctp_free_local_addr_list(net);
 
-   /* Free the control endpoint.  */
-   inet_ctl_sock_destroy(net->sctp.ctl_sock);
-
sctp_dbg_objcnt_exit(net);
 
sctp_proc_exit(net);
@@ -1319,9 +1307,32 @@ static void __net_exit sctp_net_exit(struct net *net)

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

[Vlad Yasevich]

On 09/10/2015 02:35 PM, Marcelo Ricardo Leitner wrote:
> On Thu, Sep 10, 2015 at 01:24:54PM -0300, Marcelo Ricardo Leitner wrote:
>> 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.
> 
> I was testing with it, something is not good. I finished your patch and
> testing with a flooder like:
>  # for j in {1..5}; do for i in {1234..1280}; do \
>sctp_darn -H 192.168.122.147 -P $j$i -l & done & done
> 
... snip...
> 
> It seems that request_module will not serialize it as we wanted and we
> would be putting unexpected pressure on it, yet it fixes the original
> issue.

So, wouldn't the same issue exist when running the above with DCCP sockets?

> Maybe we can place a semaphore at inet_create(), protecting the
> request_module()s so only one socket can do it at a time and, after it
> is released, whoever was blocked on it re-checks if the module isn't
> already loaded before attempting again. It makes the loading of
> different modules slower, though, but I'm not sure if that's really a
> problem. Not many modules are loaded at the same time like that. What do
> you think? 

I think this is a different issue.  The fact that we keep trying to probe
the same module is silly.  May be a per proto semaphore so that SCTP doesn't
block DCCP for example.

-vlad

> 
>   Marcelo
> 

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Marcelo Ricardo Leitner]

Em 10-09-2015 16:14, Vlad Yasevich escreveu:

On 09/10/2015 02:35 PM, Marcelo Ricardo Leitner wrote:

On Thu, Sep 10, 2015 at 01:24:54PM -0300, Marcelo Ricardo Leitner wrote:

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.


I was testing with it, something is not good. I finished your patch and
testing with a flooder like:
  # for j in {1..5}; do for i in {1234..1280}; do \
sctp_darn -H 192.168.122.147 -P $j$i -l & done & done


... snip...


It seems that request_module will not serialize it as we wanted and we
would be putting unexpected pressure on it, yet it fixes the original
issue.


So, wouldn't the same issue exist when running the above with DCCP sockets?


Pretty much, yes.


Maybe we can place a semaphore at inet_create(), protecting the
request_module()s so only one socket can do it at a time and, after it
is released, whoever was blocked on it re-checks if the module isn't
already loaded before attempting again. It makes the loading of
different modules slower, though, but I'm not sure if that's really a
problem. Not many modules are loaded at the same time like that. What do
you think?


I think this is a different issue.  The fact that we keep trying to probe


Agreed. I'll post this one as v2 while we continue with the 
request_module part.



the same module is silly.  May be a per proto semaphore so that SCTP doesn't
block DCCP for example.


Can be, yes. It just has to be dynamic, otherwise we would have to have 
like 256 semaphores that are left unused for most of the system's 
lifetime. I'll see what I can do here.


Thanks,
Marcelo

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[David Miller]

From: Marcelo Ricardo Leitner 
Date: Wed,  9 Sep 2015 17:03:01 -0300

> So the fix then is to invert the initialization order inside
> register_pernet_subsys() so that the control socket is created by last
> and also block socket creation if netns initialization wasn't yet
> performed.

If we really need to we could make ->create() fail with -EAFNOSUPPORT
if kern==1 until the protocol is fully setup.

Or, instead of failing, we could make such ->create() calls block
until the control sock init is complete or fails.

We have actually several visibility issues wrt. control sockets on
protocol init, in general.

For example, such control sockets can briefly be hashed and visible
to socket dumps and packet input.

A lot of really tricky issues involved here.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Vlad Yasevich]

On 09/09/2015 04:03 PM, Marcelo Ricardo Leitner wrote:
> Consider sctp module is unloaded and is being requested because an user
> is creating a sctp socket.
> 
> During initialization, sctp will add the new protocol type and then
> initialize pernet subsys:
> 
> status = sctp_v4_protosw_init();
> if (status)
> goto err_protosw_init;
> 
> status = sctp_v6_protosw_init();
> if (status)
> goto err_v6_protosw_init;
> 
> status = register_pernet_subsys(_net_ops);
> 
> The problem is that after those calls to sctp_v{4,6}_protosw_init(), it
> is possible for userspace to create SCTP sockets like if the module is
> already fully loaded. If that happens, one of the possible effects is
> that we will have readers for net->sctp.local_addr_list list earlier
> than expected and sctp_net_init() does not take precautions while
> dealing with that list, leading to a potential panic but not limited to
> that, as sctp_sock_init() will copy a bunch of blank/partially
> initialized values from net->sctp.
> 
> The race happens like this:
> 
>  CPU 0   |  CPU 1
>   socket()   |
>__sock_create | socket()
> inet_create  |  __sock_create
>  list_for_each_entry_rcu(|
> answer, [sock->type], |
> list) {  |   inet_create
>   /* no hits */  |
>  if (unlikely(err)) {|
>   ...|
>   request_module()   |
>   /* socket creation is blocked  |
>* the module is fully loaded  |
>*/|
>sctp_init |
> sctp_v4_protosw_init |
>  inet_register_protosw   |
>   list_add_rcu(>list, |
>last_perm);   |
>  |  list_for_each_entry_rcu(
>  | answer, [sock->type],
> sctp_v6_protosw_init | list) {
>  | /* hit, so assumes protocol
>  |  * is already loaded
>  |  */
>  |  /* socket creation continues
>  |   * before netns is initialized
>  |   */
> register_pernet_subsys   |
> 
> Inverting the initialization order between register_pernet_subsys() and
> sctp_v4_protosw_init() is not possible because register_pernet_subsys()
> will create a control sctp socket, so the protocol must be already
> visible by then. Deferring the socket creation to a work-queue is not
> good specially because we loose the ability to handle its errors.
> 
> So the fix then is to invert the initialization order inside
> register_pernet_subsys() so that the control socket is created by last
> and also block socket creation if netns initialization wasn't yet
> performed.
> 

not sure how much I like that...  Wouldn't it be better
to pull the control socket initialization stuff out into its
own function that does something like

for_each_net_rcu()
init_control_socket(net, ...)


Or may be even pull the control socket creation
stuff completely into its own per-net ops operations structure
and initialize it after the the protosw stuff has been done.

-vlad

> Fixes: 4db67e808640 ("sctp: Make the address lists per network namespace")
> Signed-off-by: Marcelo Ricardo Leitner 
> ---
>  net/sctp/protocol.c | 18 +++---
>  net/sctp/socket.c   |  4 
>  2 files changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
> index 
> 4345790ad3266c353eeac5398593c2a9ce4effda..d8f78165768a75f93f4ce4120dd5475b6a623aaf
>  100644
> --- a/net/sctp/protocol.c
> +++ b/net/sctp/protocol.c
> @@ -1271,12 +1271,6 @@ static int __net_init sctp_net_init(struct net *net)
>  
>   sctp_dbg_objcnt_init(net);
>  
> - /* Initialize the control inode/socket for handling OOTB packets.  */
> - if ((status = sctp_ctl_sock_init(net))) {
> - pr_err("Failed to initialize the SCTP control sock\n");
> - goto err_ctl_sock_init;
> - }
> -
>   /* Initialize the local address list. */
>   INIT_LIST_HEAD(>sctp.local_addr_list);
>   spin_lock_init(>sctp.local_addr_lock);
> @@ -1284,11 +1278,21 @@ static int __net_init sctp_net_init(struct net *net)
>  
>   /* Initialize the address event list */
>   INIT_LIST_HEAD(>sctp.addr_waitq);
> - INIT_LIST_HEAD(>sctp.auto_asconf_splist);
>   spin_lock_init(>sctp.addr_wq_lock);
>   net->sctp.addr_wq_timer.expires = 0;
>   setup_timer(>sctp.addr_wq_timer, sctp_addr_wq_timeout_handler,
>   (unsigned long)net);
> + /* sctp_init_sock() will use this 

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

[Marcelo Ricardo Leitner]

Em 09-09-2015 17:30, Vlad Yasevich escreveu:

On 09/09/2015 04:03 PM, Marcelo Ricardo Leitner wrote:

Consider sctp module is unloaded and is being requested because an user
is creating a sctp socket.

During initialization, sctp will add the new protocol type and then
initialize pernet subsys:

 status = sctp_v4_protosw_init();
 if (status)
 goto err_protosw_init;

 status = sctp_v6_protosw_init();
 if (status)
 goto err_v6_protosw_init;

 status = register_pernet_subsys(_net_ops);

The problem is that after those calls to sctp_v{4,6}_protosw_init(), it
is possible for userspace to create SCTP sockets like if the module is
already fully loaded. If that happens, one of the possible effects is
that we will have readers for net->sctp.local_addr_list list earlier
than expected and sctp_net_init() does not take precautions while
dealing with that list, leading to a potential panic but not limited to
that, as sctp_sock_init() will copy a bunch of blank/partially
initialized values from net->sctp.

The race happens like this:

  CPU 0   |  CPU 1
   socket()   |
__sock_create | socket()
 inet_create  |  __sock_create
  list_for_each_entry_rcu(|
 answer, [sock->type], |
 list) {  |   inet_create
   /* no hits */  |
  if (unlikely(err)) {|
   ...|
   request_module()   |
   /* socket creation is blocked  |
* the module is fully loaded  |
*/|
sctp_init |
 sctp_v4_protosw_init |
  inet_register_protosw   |
   list_add_rcu(>list, |
last_perm);   |
  |  list_for_each_entry_rcu(
  | answer, [sock->type],
 sctp_v6_protosw_init | list) {
  | /* hit, so assumes protocol
  |  * is already loaded
  |  */
  |  /* socket creation continues
  |   * before netns is initialized
  |   */
 register_pernet_subsys   |

Inverting the initialization order between register_pernet_subsys() and
sctp_v4_protosw_init() is not possible because register_pernet_subsys()
will create a control sctp socket, so the protocol must be already
visible by then. Deferring the socket creation to a work-queue is not
good specially because we loose the ability to handle its errors.

So the fix then is to invert the initialization order inside
register_pernet_subsys() so that the control socket is created by last
and also block socket creation if netns initialization wasn't yet
performed.



not sure how much I like that...  Wouldn't it be better
to pull the control socket initialization stuff out into its
own function that does something like

for_each_net_rcu()
init_control_socket(net, ...)


Or may be even pull the control socket creation
stuff completely into its own per-net ops operations structure
and initialize it after the the protosw stuff has been done.

-vlad


I'm afraid error handling won't be easy then.

But still, the control socket is not really the problem, because we 
don't care (much?) if it contains zeroed values and the panic happens 
only if you call connect() on it. I moved it solely because of the 
protection on sctp_init_sock().


The real problem is new sockets created by an user application while 
module is still loading, because even if them don't trigger the panic, 
they may not be fully functional due to improper values loaded. Can't 
see other good ways to protect sctp_init_sock() from that early call (as 
in, prior to netns initialization).


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.


  Marcelo


Fixes: 4db67e808640 ("sctp: Make the address lists per network namespace")
Signed-off-by: Marcelo Ricardo Leitner 
---
  net/sctp/protocol.c | 18 +++---
  net/sctp/socket.c   |  4 
  2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
index 
4345790ad3266c353eeac5398593c2a9ce4effda..d8f78165768a75f93f4ce4120dd5475b6a623aaf
 100644
--- a/net/sctp/protocol.c
+++ b/net/sctp/protocol.c
@@ -1271,12 +1271,6 @@ static int __net_init sctp_net_init(struct net *net)

sctp_dbg_objcnt_init(net);

-   /* Initialize the control inode/socket for handling OOTB packets.  */
-   if ((status =