From: Vlad Yasevich <vyasevich@gmail.com>
To: Neil Horman <nhorman@tuxdriver.com>, linux-sctp@vger.kernel.org
Cc: Wang Weidong <wangweidong1@huawei.com>,
David Miller <davem@davemloft.net>,
netdev@vger.kernel.org
Subject: Re: [PATCH] sctp: properly latch and use autoclose value from sock to association
Date: Fri, 06 Dec 2013 10:36:26 -0500 [thread overview]
Message-ID: <52A1EEFA.601@gmail.com> (raw)
In-Reply-To: <1386343755-14130-1-git-send-email-nhorman@tuxdriver.com>
On 12/06/2013 10:29 AM, Neil Horman wrote:
> Currently, sctp associations latch a sockets autoclose value to an association
> at association init time, subject to capping constraints from the max_autoclose
> sysctl value. This leads to an odd situation where an application may set a
> socket level autoclose timeout, but sliently sctp will limit the autoclose
> timeout to something less than that.
>
> Fix this by modifying the autoclose setsockopt function to check the limit, cap
> it and warn the user via syslog that the timeout is capped. This will allow
> getsockopt to return valid autoclose timeout values that reflect what subsequent
> associations actually use.
>
> While were at it, also elimintate the assoc->autoclose variable, it duplicates
> whats in the timeout array, which leads to multiple sources for the same
> information, that may differ (as the former isn't subject to any capping). This
> gives us the timeout information in a canonical place and saves some space in
> the association structure as well.
>
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> CC: Wang Weidong <wangweidong1@huawei.com>
> CC: David Miller <davem@davemloft.net>
> CC: Vlad Yasevich <vyasevich@gmail.com>
> CC: netdev@vger.kernel.org
> ---
> include/net/sctp/structs.h | 6 ------
> net/sctp/associola.c | 2 --
> net/sctp/output.c | 3 ++-
> net/sctp/sm_statefuns.c | 12 ++++++------
> net/sctp/socket.c | 7 +++++++
> 5 files changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
> index 2174d8d..0b069a0 100644
> --- a/include/net/sctp/structs.h
> +++ b/include/net/sctp/structs.h
> @@ -1725,12 +1725,6 @@ struct sctp_association {
> /* How many duplicated TSNs have we seen? */
> int numduptsns;
>
> - /* Number of seconds of idle time before an association is closed.
> - * In the association context, this is really used as a boolean
> - * since the real timeout is stored in the timeouts array
> - */
> - __u32 autoclose;
> -
> /* These are to support
> * "SCTP Extensions for Dynamic Reconfiguration of IP Addresses
> * and Enforcement of Flow and Message Limits"
> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> index c9b91cb..0c9d1f1 100644
> --- a/net/sctp/associola.c
> +++ b/net/sctp/associola.c
> @@ -291,8 +291,6 @@ static struct sctp_association *sctp_association_init(struct sctp_association *a
> asoc->peer.ipv6_address = 1;
> INIT_LIST_HEAD(&asoc->asocs);
>
> - asoc->autoclose = sp->autoclose;
> -
> asoc->default_stream = sp->default_stream;
> asoc->default_ppid = sp->default_ppid;
> asoc->default_flags = sp->default_flags;
> diff --git a/net/sctp/output.c b/net/sctp/output.c
> index e650978..c89e57d 100644
> --- a/net/sctp/output.c
> +++ b/net/sctp/output.c
> @@ -580,7 +580,8 @@ int sctp_packet_transmit(struct sctp_packet *packet)
> unsigned long timeout;
>
> /* Restart the AUTOCLOSE timer when sending data. */
> - if (sctp_state(asoc, ESTABLISHED) && asoc->autoclose) {
> + if (sctp_state(asoc, ESTABLISHED) &&
> + asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE]) {
> timer = &asoc->timers[SCTP_EVENT_TIMEOUT_AUTOCLOSE];
> timeout = asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE];
>
> diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
> index dfe3f36..a26065b 100644
> --- a/net/sctp/sm_statefuns.c
> +++ b/net/sctp/sm_statefuns.c
> @@ -820,7 +820,7 @@ sctp_disposition_t sctp_sf_do_5_1D_ce(struct net *net,
> SCTP_INC_STATS(net, SCTP_MIB_PASSIVEESTABS);
> sctp_add_cmd_sf(commands, SCTP_CMD_HB_TIMERS_START, SCTP_NULL());
>
> - if (new_asoc->autoclose)
> + if (new_asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE])
> sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_START,
> SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));
>
> @@ -908,7 +908,7 @@ sctp_disposition_t sctp_sf_do_5_1E_ca(struct net *net,
> SCTP_INC_STATS(net, SCTP_MIB_CURRESTAB);
> SCTP_INC_STATS(net, SCTP_MIB_ACTIVEESTABS);
> sctp_add_cmd_sf(commands, SCTP_CMD_HB_TIMERS_START, SCTP_NULL());
> - if (asoc->autoclose)
> + if (asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE])
> sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_START,
> SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));
>
> @@ -2970,7 +2970,7 @@ sctp_disposition_t sctp_sf_eat_data_6_2(struct net *net,
> if (chunk->chunk_hdr->flags & SCTP_DATA_SACK_IMM)
> force = SCTP_FORCE();
>
> - if (asoc->autoclose) {
> + if (asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE]) {
> sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_RESTART,
> SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));
> }
> @@ -3878,7 +3878,7 @@ sctp_disposition_t sctp_sf_eat_fwd_tsn(struct net *net,
> SCTP_CHUNK(chunk));
>
> /* Count this as receiving DATA. */
> - if (asoc->autoclose) {
> + if (asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE]) {
> sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_RESTART,
> SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));
> }
> @@ -5267,7 +5267,7 @@ sctp_disposition_t sctp_sf_do_9_2_start_shutdown(
> sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_RESTART,
> SCTP_TO(SCTP_EVENT_TIMEOUT_T5_SHUTDOWN_GUARD));
>
> - if (asoc->autoclose)
> + if (asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE])
> sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_STOP,
> SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));
>
> @@ -5346,7 +5346,7 @@ sctp_disposition_t sctp_sf_do_9_2_shutdown_ack(
> sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_RESTART,
> SCTP_TO(SCTP_EVENT_TIMEOUT_T2_SHUTDOWN));
>
> - if (asoc->autoclose)
> + if (asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE])
> sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_STOP,
> SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));
>
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 72046b9..9486ffd 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -2196,6 +2196,7 @@ static int sctp_setsockopt_autoclose(struct sock *sk, char __user *optval,
> unsigned int optlen)
> {
> struct sctp_sock *sp = sctp_sk(sk);
> + struct net *net = sock_net(sk);
>
> /* Applicable to UDP-style socket only */
> if (sctp_style(sk, TCP))
> @@ -2205,6 +2206,12 @@ static int sctp_setsockopt_autoclose(struct sock *sk, char __user *optval,
> if (copy_from_user(&sp->autoclose, optval, optlen))
> return -EFAULT;
>
> + if (sp->autoclose > net->sctp.max_autoclose) {
> + pr_warn("Capping autoclose value %d to defined maximum of %lu\n",
> + sp->autoclose, net->sctp.max_autoclose);
> + sp->autoclose = net->sctp.max_autoclose;
> + }
> +
> return 0;
> }
>
>
Now that we do this, we can do this as well:
diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index 68a27f9..7e86957 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -154,8 +154,7 @@ static struct sctp_association
*sctp_association_init(struct sctp_association *a
asoc->timeouts[SCTP_EVENT_TIMEOUT_HEARTBEAT] = 0;
asoc->timeouts[SCTP_EVENT_TIMEOUT_SACK] = asoc->sackdelay;
- asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE] =
- min_t(unsigned long, sp->autoclose,
net->sctp.max_autoclose) * HZ;
+ asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE] = sp->autoclose * HZ;
-vlad
next prev parent reply other threads:[~2013-12-06 15:36 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-12-03 10:09 [PATCH] sctp: fix a BUG_ON on triggered by setting max_autoclose to Wang Weidong
2013-12-03 15:43 ` Neil Horman
2013-12-03 16:09 ` Vlad Yasevich
2013-12-03 18:18 ` Neil Horman
2013-12-03 21:24 ` Vlad Yasevich
2013-12-04 6:21 ` Wang Weidong
2013-12-04 13:28 ` Neil Horman
2013-12-04 14:45 ` Vlad Yasevich
2013-12-04 18:56 ` Neil Horman
2013-12-05 16:00 ` Vlad Yasevich
2013-12-05 20:43 ` Neil Horman
2013-12-06 15:29 ` [PATCH] sctp: properly latch and use autoclose value from sock to association Neil Horman
2013-12-06 15:36 ` Vlad Yasevich [this message]
2013-12-06 15:48 ` Neil Horman
2013-12-06 17:22 ` [PATCH v2] " Neil Horman
2013-12-06 18:47 ` Vlad Yasevich
2013-12-07 6:29 ` Wang Weidong
2013-12-10 11:48 ` [PATCH v3] " Neil Horman
2013-12-11 3:42 ` David Miller
2013-12-06 15:46 ` [PATCH] " David Laight
2013-12-06 16:04 ` Neil Horman
2013-12-06 16:29 ` Neil Horman
2013-12-10 1:32 ` David Miller
-- strict thread matches above, loose matches on Subject: below --
2013-12-10 3:03 nhorman
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=52A1EEFA.601@gmail.com \
--to=vyasevich@gmail.com \
--cc=davem@davemloft.net \
--cc=linux-sctp@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=nhorman@tuxdriver.com \
--cc=wangweidong1@huawei.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).