netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
To: Xin Long <lucien.xin@gmail.com>
Cc: Vlad Yasevich <vyasevich@gmail.com>,
	network dev <netdev@vger.kernel.org>,
	linux-sctp@vger.kernel.org, Vlad Yasevich <vyasevic@redhat.com>,
	davem <davem@davemloft.net>
Subject: Re: [PATCH net] sctp: sctp should release assoc when sctp_make_abort_user return NULL in sctp_close
Date: Mon, 21 Dec 2015 11:45:41 -0200	[thread overview]
Message-ID: <56780285.3040007@gmail.com> (raw)
In-Reply-To: <CADvbK_fWOErVv6EM5R5jstZSXscqK_-K+wvTLdb4DYv6+MTuSg@mail.gmail.com>

Em 21-12-2015 07:56, Xin Long escreveu:
> On Sat, Dec 19, 2015 at 12:23 AM, Marcelo Ricardo Leitner
> <marcelo.leitner@gmail.com> wrote:
>> On Fri, Dec 18, 2015 at 09:08:46AM -0500, Vlad Yasevich wrote:
>>> On 12/17/2015 02:33 PM, Vlad Yasevich wrote:
>>>> On 12/17/2015 02:01 PM, Marcelo Ricardo Leitner wrote:
>> ...
>>>>> There is a check on sctp_cmd_delete_tcb() that avoids calling that on temp assocs on
>>>>> listening sockets, but that condition is false due to the check on sk_shutdown so it will
>>>>> call those two functions anyway.
>>>>
>>>> The condition I am a bit concerned about is one thread waiting in sctp_wait_for_sndbuf
>>>> while another does an abort.
>>>>
>>>> I think this is OK though.  I need to look a bit more...
>>>
>>> I think the only time this ends up biting us is if SO_SNDTIMEO was used and we ran out
>>> of send buffer.  It looks to me like schedule_timeout() will wait until timer expired and
>>> depending on the timer value, you could wait quite a while.
>>>
>>> With this path, since you don't transition state, the asoc->wait wait queue is never
>>> notified and it could be hanging around for quite a while.
>
> do you think it makes sense if we have this condition judgment there ?
>          if (waitqueue_active(&asoc->wait))
>              wake_up_interruptible(&asoc->wait);

No, because later if there is something else like this that we need to 
handle on this situation, we will have to update both places and we may 
forget to update one of them. It's better to just skip the packet 
transmission/CMD_REPLY if chunk is NULL and let rest execute, as Vlad 
suggested.

It will also be better for troubleshooting, as it may generate debug 
msgs about the state transition.

   Marcelo

      reply	other threads:[~2015-12-21 13:45 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-12-17 14:30 [PATCH net] sctp: sctp should release assoc when sctp_make_abort_user return NULL in sctp_close Xin Long
2015-12-17 18:29 ` Vlad Yasevich
2015-12-17 19:01   ` Marcelo Ricardo Leitner
2015-12-17 19:33     ` Vlad Yasevich
2015-12-18 14:08       ` Vlad Yasevich
2015-12-18 16:23         ` Marcelo Ricardo Leitner
2015-12-21  9:56           ` Xin Long
2015-12-21 13:45             ` Marcelo Ricardo Leitner [this message]

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=56780285.3040007@gmail.com \
    --to=marcelo.leitner@gmail.com \
    --cc=davem@davemloft.net \
    --cc=linux-sctp@vger.kernel.org \
    --cc=lucien.xin@gmail.com \
    --cc=netdev@vger.kernel.org \
    --cc=vyasevic@redhat.com \
    --cc=vyasevich@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
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;
as well as URLs for NNTP newsgroup(s).