On Mon, Nov 13, 2017 at 11:40 PM, Xin Long <lucien.xin@xxxxxxxxx> wrote:
> On Mon, Nov 13, 2017 at 11:23 PM, Neil Horman <nhorman@xxxxxxxxxxxxx> wrote:
>> On Mon, Nov 13, 2017 at 01:47:58PM +0800, Xin Long wrote:
>>> Commit dfcb9f4f99f1 ("sctp: deny peeloff operation on asocs with threads
>>> sleeping on it") fixed the race between peeloff and wait sndbuf by
>>> checking waitqueue_active(&asoc->wait) in sctp_do_peeloff().
>>>
>>> But it actually doesn't work as even if waitqueue_active returns false
>>> the waiting sndbuf thread may still not yet hold sk lock.
>>>
>>> This patch is to fix this by adding wait_buf flag in asoc, and setting it
>>> before going the waiting loop, clearing it until the waiting loop breaks,
>>> and checking it in sctp_do_peeloff instead.
>>>
>>> Fixes: dfcb9f4f99f1 ("sctp: deny peeloff operation on asocs with threads sleeping on it")
>>> Suggested-by: Marcelo Ricardo Leitner <marcelo.leitner@xxxxxxxxx>
>>> Signed-off-by: Xin Long <lucien.xin@xxxxxxxxx>
>>> ---
>>> include/net/sctp/structs.h | 1 +
>>> net/sctp/socket.c | 4 +++-
>>> 2 files changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
>>> index 0477945..446350e 100644
>>> --- a/include/net/sctp/structs.h
>>> +++ b/include/net/sctp/structs.h
>>> @@ -1883,6 +1883,7 @@ struct sctp_association {
>>>
>>> __u8 need_ecne:1, /* Need to send an ECNE Chunk? */
>>> temp:1, /* Is it a temporary association? */
>>> + wait_buf:1,
>>> force_delay:1,
>>> prsctp_enable:1,
>>> reconf_enable:1;
>>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
>>> index 6f45d17..1b2c78c 100644
>>> --- a/net/sctp/socket.c
>>> +++ b/net/sctp/socket.c
>>> @@ -4946,7 +4946,7 @@ int sctp_do_peeloff(struct sock *sk, sctp_assoc_t id, struct socket **sockp)
>>> /* If there is a thread waiting on more sndbuf space for
>>> * sending on this asoc, it cannot be peeled.
>>> */
>>> - if (waitqueue_active(&asoc->wait))
>>> + if (asoc->wait_buf)
>>> return -EBUSY;
>>>
>>> /* An association cannot be branched off from an already peeled-off
>>> @@ -7835,6 +7835,7 @@ static int sctp_wait_for_sndbuf(struct sctp_association *asoc, long *timeo_p,
>>> /* Increment the association's refcnt. */
>>> sctp_association_hold(asoc);
>>>
>>> + asoc->wait_buf = 1;
>>> /* Wait on the association specific sndbuf space. */
>>> for (;;) {
>>> prepare_to_wait_exclusive(&asoc->wait, &wait,
>>> @@ -7860,6 +7861,7 @@ static int sctp_wait_for_sndbuf(struct sctp_association *asoc, long *timeo_p,
>>> }
>>>
>>> out:
>>> + asoc->wait_buf = 0;
>>> finish_wait(&asoc->wait, &wait);
>>>
>>> /* Release the association's refcnt. */
>>> --
>>> 2.1.0
>>>
>>>
>>
>> This doesn't make much sense to me, as it appears to be prone to aliasing. That
>> is to say:
>>
>> a) If multiple tasks are queued waiting in sctp_wait_for_sndbuf, the first
>> thread to exit that for(;;) loop will clean asoc->wait_buf, even though others
>> may be waiting on it, allowing sctp_do_peeloff to continue when it shouldn't be
> You're right, we talked about this before using waitqueue_active in
> earlier time.
> I didn't remember this somehow. Sorry.
>
>>
>> b) In the case of a single task blocking in sct_wait_for_sendbuf, checking
>> waitqueue_active is equally good, because it returns true, until such time as
>> finish_wait is called anyway.
> waitqueue_active can not work here, because in sctp_wait_for_sndbuf():
> ...
> release_sock(sk);
> current_timeo = schedule_timeout(current_timeo); <-----[a]
> lock_sock(sk);
> If another thread wakes up asoc->wait, it will be removed from
> this wait queue, you check DEFINE_WAIT, the callback autoremove_wake_function
> will do this removal in wake_up().
>
> I guess we need to think about another to fix this.
maybe we can use
DEFINE_WAIT_FUNC(wait, woken_wake_function);
instead of DEFINE_WAIT(wait) here ?
>
>>
>> It really seems to me that waitqueue_active is the right answer here, as it
>> should return true until there are no longer any tasks waiting on sndbuf space
>>
>> Neil
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
