From: Vlad Yasevich <vyasevich@gmail•com>
To: Marcelo Ricardo Leitner <marcelo.leitner@gmail•com>
Cc: Xin Long <lucien.xin@gmail•com>,
network dev <netdev@vger•kernel.org>,
linux-sctp@vger•kernel.org, daniel@iogearbox•net,
davem@davemloft•net
Subject: Re: [PATCH net 1/3] sctp: fix the transport dead race check by using atomic_add_unless on refcnt
Date: Fri, 22 Jan 2016 13:54:09 -0500 [thread overview]
Message-ID: <56A27AD1.9040502@gmail.com> (raw)
In-Reply-To: <20160122171812.GI3452@mrl.redhat.com>
On 01/22/2016 12:18 PM, Marcelo Ricardo Leitner wrote:
> On Fri, Jan 22, 2016 at 11:50:20AM -0500, Vlad Yasevich wrote:
>> On 01/21/2016 12:49 PM, Xin Long wrote:
>>> Now when __sctp_lookup_association is running in BH, it will try to
>>> check if t->dead is set, but meanwhile other CPUs may be freeing this
>>> transport and this assoc and if it happens that
>>> __sctp_lookup_association checked t->dead a bit too early, it may think
>>> that the association is still good while it was already freed.
>>>
>>> So we fix this race by using atomic_add_unless in sctp_transport_hold.
>>> After we get one transport from hashtable, we will hold it only when
>>> this transport's refcnt is not 0, so that we can make sure t->asoc
>>> cannot be freed before we hold the asoc again.
>>
>> atomic_add_unless() uses atomic_read() to check the value. Since there
>> don't appear to be any barriers, what guarantees that the value
>> read will not have been modified in another thread under a proper lock?
>>
>
> atomic_read() is used only as a starting point. If it got changed in
> between, the new current value (return of atomic_cmpxchg) will be used
> then.
>
>>>
>>> Note that sctp association is not freed using RCU so we can't use
>>> atomic_add_unless() with it as it may just be too late for that either.
>>>
>>> Fixes: 4f0087812648 ("sctp: apply rhashtable api to send/recv path")
>>> Reported-by: Vlad Yasevich <vyasevich@gmail•com>
>>> Signed-off-by: Xin Long <lucien.xin@gmail•com>
>>> ---
>>> include/net/sctp/structs.h | 2 +-
>>> net/sctp/input.c | 17 +++++++++++------
>>> net/sctp/transport.c | 4 ++--
>>> 3 files changed, 14 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
>>> index 20e7212..344da04 100644
>>> --- a/include/net/sctp/structs.h
>>> +++ b/include/net/sctp/structs.h
>>> @@ -955,7 +955,7 @@ void sctp_transport_route(struct sctp_transport *, union sctp_addr *,
>>> void sctp_transport_pmtu(struct sctp_transport *, struct sock *sk);
>>> void sctp_transport_free(struct sctp_transport *);
>>> void sctp_transport_reset_timers(struct sctp_transport *);
>>> -void sctp_transport_hold(struct sctp_transport *);
>>> +int sctp_transport_hold(struct sctp_transport *);
>>> void sctp_transport_put(struct sctp_transport *);
>>> void sctp_transport_update_rto(struct sctp_transport *, __u32);
>>> void sctp_transport_raise_cwnd(struct sctp_transport *, __u32, __u32);
>>> diff --git a/net/sctp/input.c b/net/sctp/input.c
>>> index bf61dfb..49d2cc7 100644
>>> --- a/net/sctp/input.c
>>> +++ b/net/sctp/input.c
>>> @@ -935,15 +935,22 @@ static struct sctp_association *__sctp_lookup_association(
>>> struct sctp_transport **pt)
>>> {
>>> struct sctp_transport *t;
>>> + struct sctp_association *asoc = NULL;
>>>
>>> + rcu_read_lock();
>>> t = sctp_addrs_lookup_transport(net, local, peer);
>>> - if (!t || t->dead)
>>> - return NULL;
>>> + if (!t || !sctp_transport_hold(t))
>>> + goto out;
>>>
>>> - sctp_association_hold(t->asoc);
>>> + asoc = t->asoc;
>>> + sctp_association_hold(asoc);
>>
>> I don't think you can modify the reference count on a transport, let alone
>> the association outside of a lock.
>
> The transport memory is not freed, as it's protected by rcu_read_lock(),
> so we are safe to use it yet.
> atomic_ operations include an embedded lock instruction protecting the
> counter itself, there shouldn't be a need to use another lock around it.
>
> And in the code above, as we could grab a hold on the transport, means
> the association was not freed yet because transports hold a ref on
> assoc. That's why the dance: hold(transport) hold(assoc) put(transport)
>
OK, I see how that holds together, but I think there might be hole wrt icmp
handling. Some icmp processes assume transport can't disappear on them, but in
this case that last put(transport) may result in a call to sctp_transport_destroy()
and that might be bad. I am looking at it now.
Thanks
-vlad
> Marcelo
>
>>
>> -vlad
>>
>>> *pt = t;
>>>
>>> - return t->asoc;
>>> + sctp_transport_put(t);
>>> +
>>> +out:
>>> + rcu_read_unlock();
>>> + return asoc;
>>> }
>>>
>>> /* Look up an association. protected by RCU read lock */
>>> @@ -955,9 +962,7 @@ struct sctp_association *sctp_lookup_association(struct net *net,
>>> {
>>> struct sctp_association *asoc;
>>>
>>> - rcu_read_lock();
>>> asoc = __sctp_lookup_association(net, laddr, paddr, transportp);
>>> - rcu_read_unlock();
>>>
>>> return asoc;
>>> }
>>> diff --git a/net/sctp/transport.c b/net/sctp/transport.c
>>> index aab9e3f..69f3799 100644
>>> --- a/net/sctp/transport.c
>>> +++ b/net/sctp/transport.c
>>> @@ -296,9 +296,9 @@ void sctp_transport_route(struct sctp_transport *transport,
>>> }
>>>
>>> /* Hold a reference to a transport. */
>>> -void sctp_transport_hold(struct sctp_transport *transport)
>>> +int sctp_transport_hold(struct sctp_transport *transport)
>>> {
>>> - atomic_inc(&transport->refcnt);
>>> + return atomic_add_unless(&transport->refcnt, 1, 0);
>>> }
>>>
>>> /* Release a reference to a transport and clean up
>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
>> the body of a message to majordomo@vger•kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
next prev parent reply other threads:[~2016-01-22 18:54 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-01-21 17:49 [PATCH net 0/3] fix the transport dead race check by using atomic_add_unless on refcnt Xin Long
2016-01-21 17:49 ` [PATCH net 1/3] sctp: " Xin Long
2016-01-21 17:53 ` Marcelo Ricardo Leitner
2016-01-22 16:50 ` Vlad Yasevich
2016-01-22 17:18 ` Marcelo Ricardo Leitner
2016-01-22 18:54 ` Vlad Yasevich [this message]
2016-01-25 18:44 ` David Miller
2016-01-21 17:49 ` [PATCH net 2/3] sctp: hold transport before we access t->asoc in sctp proc Xin Long
2016-01-21 17:53 ` Marcelo Ricardo Leitner
2016-01-21 19:27 ` Eric Dumazet
2016-01-21 19:37 ` Marcelo Ricardo Leitner
2016-01-21 19:57 ` Eric Dumazet
2016-01-21 20:08 ` Marcelo Ricardo Leitner
2016-01-21 17:49 ` [PATCH net 3/3] sctp: remove the dead field of sctp_transport Xin Long
2016-01-21 17:54 ` Marcelo Ricardo Leitner
2016-01-21 17:58 ` [PATCH net 0/3] fix the transport dead race check by using atomic_add_unless on refcnt Xin Long
2016-01-28 23:59 ` David Miller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=56A27AD1.9040502@gmail.com \
--to=vyasevich@gmail$(echo .)com \
--cc=daniel@iogearbox$(echo .)net \
--cc=davem@davemloft$(echo .)net \
--cc=linux-sctp@vger$(echo .)kernel.org \
--cc=lucien.xin@gmail$(echo .)com \
--cc=marcelo.leitner@gmail$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox