netdev - Re: [PATCH net] sctp: add wait_buf flag in asoc to avoid the peeloff and wait sndbuf 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] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 13 Nov 2017 23:40:59 +0800
From:   Xin Long <lucien.xin@...il.com>
To:     Neil Horman <nhorman@...driver.com>
Cc:     network dev <netdev@...r.kernel.org>, linux-sctp@...r.kernel.org,
        davem <davem@...emloft.net>,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
Subject: Re: [PATCH net] sctp: add wait_buf flag in asoc to avoid the peeloff
 and wait sndbuf race

On Mon, Nov 13, 2017 at 11:23 PM, Neil Horman <nhorman@...driver.com> 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@...il.com>
>> Signed-off-by: Xin Long <lucien.xin@...il.com>
>> ---
>>  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.

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