From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Borkmann Date: Tue, 06 Aug 2013 20:19:53 +0000 Subject: Re: [PATCH net-next 1/2] net: sctp: convert sctp_checksum_disable module param into sctp sysctl Message-Id: <52015A69.3080204@redhat.com> List-Id: References: <1375816693-7697-1-git-send-email-dborkman@redhat.com> <52014F0E.6040101@gmail.com> In-Reply-To: <52014F0E.6040101@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Vlad Yasevich Cc: davem@davemloft.net, netdev@vger.kernel.org, linux-sctp@vger.kernel.org On 08/06/2013 09:31 PM, Vlad Yasevich wrote: > On 08/06/2013 03:18 PM, Daniel Borkmann wrote: >> Get rid of the last module parameter for SCTP and make this >> configurable via sysctl for SCTP like all the rest of SCTP's >> configuration knobs. >> > > But this isn't like the rest of the sctp knobs. Disabling this violates the base protocol and we don't really want to encourage > people to do this. > > There was a long discussion about it back in 2009 when the original > patch was submitted that proposed a sysctl. Nothing has changed since > then to convince me that this sysctl would be a good idea. Ah, okay. I didn't follow that discussion. Actually both can easily be set or unset as it's just /sys/module/sctp/parameters/no_checksums, but I understand your reasoning that sysctl is something more "official". > -vlad > >> Signed-off-by: Daniel Borkmann >> --- >> Documentation/networking/ip-sysctl.txt | 8 ++++++++ >> include/net/netns/sctp.h | 3 +++ >> include/net/sctp/structs.h | 5 ----- >> net/sctp/input.c | 4 ++-- >> net/sctp/output.c | 5 ++++- >> net/sctp/protocol.c | 5 +++-- >> net/sctp/sysctl.c | 10 +++++++++- >> 7 files changed, 29 insertions(+), 11 deletions(-) >> >> diff --git a/Documentation/networking/ip-sysctl.txt b/Documentation/networking/ip-sysctl.txt >> index 36be26b..6f5b813 100644 >> --- a/Documentation/networking/ip-sysctl.txt >> +++ b/Documentation/networking/ip-sysctl.txt >> @@ -1507,6 +1507,14 @@ sack_timeout - INTEGER >> >> Default: 200 >> >> +checksum_disable - BOOLEAN >> + Disable SCTP checksum computing and verification for debugging purpose. >> + >> + 1: Disable checksumming >> + 0: Enable checksumming >> + >> + Default: 0 >> + >> valid_cookie_life - INTEGER >> The default lifetime of the SCTP cookie (in milliseconds). The cookie >> is used during association establishment. >> diff --git a/include/net/netns/sctp.h b/include/net/netns/sctp.h >> index 3573a81..ebfdf1e 100644 >> --- a/include/net/netns/sctp.h >> +++ b/include/net/netns/sctp.h >> @@ -129,6 +129,9 @@ struct netns_sctp { >> >> /* Threshold for autoclose timeout, in seconds. */ >> unsigned long max_autoclose; >> + >> + /* Flag to disable SCTP checksumming. */ >> + int checksum_disable; >> }; >> >> #endif /* __NETNS_SCTP_H__ */ >> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h >> index d9c93a7..06ebeaa 100644 >> --- a/include/net/sctp/structs.h >> +++ b/include/net/sctp/structs.h >> @@ -141,10 +141,6 @@ extern struct sctp_globals { >> /* This is the sctp port control hash. */ >> int port_hashsize; >> struct sctp_bind_hashbucket *port_hashtable; >> - >> - /* Flag to indicate whether computing and verifying checksum >> - * is disabled. */ >> - bool checksum_disable; >> } sctp_globals; >> >> #define sctp_max_instreams (sctp_globals.max_instreams) >> @@ -156,7 +152,6 @@ extern struct sctp_globals { >> #define sctp_assoc_hashtable (sctp_globals.assoc_hashtable) >> #define sctp_port_hashsize (sctp_globals.port_hashsize) >> #define sctp_port_hashtable (sctp_globals.port_hashtable) >> -#define sctp_checksum_disable (sctp_globals.checksum_disable) >> >> /* SCTP Socket type: UDP or TCP style. */ >> typedef enum { >> diff --git a/net/sctp/input.c b/net/sctp/input.c >> index fa91aff..b9a25e1 100644 >> --- a/net/sctp/input.c >> +++ b/net/sctp/input.c >> @@ -140,8 +140,8 @@ int sctp_rcv(struct sk_buff *skb) >> __skb_pull(skb, skb_transport_offset(skb)); >> if (skb->len < sizeof(struct sctphdr)) >> goto discard_it; >> - if (!sctp_checksum_disable && !skb_csum_unnecessary(skb) && >> - sctp_rcv_checksum(net, skb) < 0) >> + if (!net->sctp.checksum_disable && !skb_csum_unnecessary(skb) && >> + sctp_rcv_checksum(net, skb) < 0) >> goto discard_it; >> >> skb_pull(skb, sizeof(struct sctphdr)); >> diff --git a/net/sctp/output.c b/net/sctp/output.c >> index 5a55c55d..cdb5f49 100644 >> --- a/net/sctp/output.c >> +++ b/net/sctp/output.c >> @@ -395,6 +395,7 @@ int sctp_packet_transmit(struct sctp_packet *packet) >> int padding; /* How much padding do we need? */ >> __u8 has_data = 0; >> struct dst_entry *dst = tp->dst; >> + struct net *net; >> unsigned char *auth = NULL; /* pointer to auth in skb data */ >> __u32 cksum_buf_len = sizeof(struct sctphdr); >> >> @@ -541,7 +542,9 @@ int sctp_packet_transmit(struct sctp_packet *packet) >> * Note: Adler-32 is no longer applicable, as has been replaced >> * by CRC32-C as described in . >> */ >> - if (!sctp_checksum_disable) { >> + net = dev_net(dst->dev); >> + >> + if (!net->sctp.checksum_disable) { >> if (!(dst->dev->features & NETIF_F_SCTP_CSUM)) { >> __u32 crc32 = sctp_start_cksum((__u8 *)sh, cksum_buf_len); >> >> diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c >> index b52ec25..a570a63 100644 >> --- a/net/sctp/protocol.c >> +++ b/net/sctp/protocol.c >> @@ -1193,6 +1193,9 @@ static int __net_init sctp_net_init(struct net *net) >> /* Whether Cookie Preservative is enabled(1) or not(0) */ >> net->sctp.cookie_preserve_enable = 1; >> >> + /* Whether SCTP checksumming is disabled(1) or not(0) */ >> + net->sctp.checksum_disable = 0; >> + >> /* Default sctp sockets to use md5 as their hmac alg */ >> #if defined (CONFIG_SCTP_DEFAULT_COOKIE_HMAC_MD5) >> net->sctp.sctp_hmac_alg = "md5"; >> @@ -1549,6 +1552,4 @@ MODULE_ALIAS("net-pf-" __stringify(PF_INET) "-proto-132"); >> MODULE_ALIAS("net-pf-" __stringify(PF_INET6) "-proto-132"); >> MODULE_AUTHOR("Linux Kernel SCTP developers "); >> MODULE_DESCRIPTION("Support for the SCTP protocol (RFC2960)"); >> -module_param_named(no_checksums, sctp_checksum_disable, bool, 0644); >> -MODULE_PARM_DESC(no_checksums, "Disable checksums computing and verification"); >> MODULE_LICENSE("GPL"); >> diff --git a/net/sctp/sysctl.c b/net/sctp/sysctl.c >> index 1906747..754809a 100644 >> --- a/net/sctp/sysctl.c >> +++ b/net/sctp/sysctl.c >> @@ -296,7 +296,15 @@ static struct ctl_table sctp_net_table[] = { >> .extra1 = &max_autoclose_min, >> .extra2 = &max_autoclose_max, >> }, >> - >> + { >> + .procname = "checksum_disable", >> + .data = &init_net.sctp.checksum_disable, >> + .maxlen = sizeof(int), >> + .mode = 0644, >> + .proc_handler = proc_dointvec_minmax, >> + .extra1 = &zero, >> + .extra2 = &one, >> + }, >> { /* sentinel */ } >> }; >> >> > From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Borkmann Subject: Re: [PATCH net-next 1/2] net: sctp: convert sctp_checksum_disable module param into sctp sysctl Date: Tue, 06 Aug 2013 22:19:53 +0200 Message-ID: <52015A69.3080204@redhat.com> References: <1375816693-7697-1-git-send-email-dborkman@redhat.com> <52014F0E.6040101@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: davem@davemloft.net, netdev@vger.kernel.org, linux-sctp@vger.kernel.org To: Vlad Yasevich Return-path: Received: from mx1.redhat.com ([209.132.183.28]:49202 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756235Ab3HFUUA (ORCPT ); Tue, 6 Aug 2013 16:20:00 -0400 In-Reply-To: <52014F0E.6040101@gmail.com> Sender: netdev-owner@vger.kernel.org List-ID: On 08/06/2013 09:31 PM, Vlad Yasevich wrote: > On 08/06/2013 03:18 PM, Daniel Borkmann wrote: >> Get rid of the last module parameter for SCTP and make this >> configurable via sysctl for SCTP like all the rest of SCTP's >> configuration knobs. >> > > But this isn't like the rest of the sctp knobs. Disabling this violates the base protocol and we don't really want to encourage > people to do this. > > There was a long discussion about it back in 2009 when the original > patch was submitted that proposed a sysctl. Nothing has changed since > then to convince me that this sysctl would be a good idea. Ah, okay. I didn't follow that discussion. Actually both can easily be set or unset as it's just /sys/module/sctp/parameters/no_checksums, but I understand your reasoning that sysctl is something more "official". > -vlad > >> Signed-off-by: Daniel Borkmann >> --- >> Documentation/networking/ip-sysctl.txt | 8 ++++++++ >> include/net/netns/sctp.h | 3 +++ >> include/net/sctp/structs.h | 5 ----- >> net/sctp/input.c | 4 ++-- >> net/sctp/output.c | 5 ++++- >> net/sctp/protocol.c | 5 +++-- >> net/sctp/sysctl.c | 10 +++++++++- >> 7 files changed, 29 insertions(+), 11 deletions(-) >> >> diff --git a/Documentation/networking/ip-sysctl.txt b/Documentation/networking/ip-sysctl.txt >> index 36be26b..6f5b813 100644 >> --- a/Documentation/networking/ip-sysctl.txt >> +++ b/Documentation/networking/ip-sysctl.txt >> @@ -1507,6 +1507,14 @@ sack_timeout - INTEGER >> >> Default: 200 >> >> +checksum_disable - BOOLEAN >> + Disable SCTP checksum computing and verification for debugging purpose. >> + >> + 1: Disable checksumming >> + 0: Enable checksumming >> + >> + Default: 0 >> + >> valid_cookie_life - INTEGER >> The default lifetime of the SCTP cookie (in milliseconds). The cookie >> is used during association establishment. >> diff --git a/include/net/netns/sctp.h b/include/net/netns/sctp.h >> index 3573a81..ebfdf1e 100644 >> --- a/include/net/netns/sctp.h >> +++ b/include/net/netns/sctp.h >> @@ -129,6 +129,9 @@ struct netns_sctp { >> >> /* Threshold for autoclose timeout, in seconds. */ >> unsigned long max_autoclose; >> + >> + /* Flag to disable SCTP checksumming. */ >> + int checksum_disable; >> }; >> >> #endif /* __NETNS_SCTP_H__ */ >> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h >> index d9c93a7..06ebeaa 100644 >> --- a/include/net/sctp/structs.h >> +++ b/include/net/sctp/structs.h >> @@ -141,10 +141,6 @@ extern struct sctp_globals { >> /* This is the sctp port control hash. */ >> int port_hashsize; >> struct sctp_bind_hashbucket *port_hashtable; >> - >> - /* Flag to indicate whether computing and verifying checksum >> - * is disabled. */ >> - bool checksum_disable; >> } sctp_globals; >> >> #define sctp_max_instreams (sctp_globals.max_instreams) >> @@ -156,7 +152,6 @@ extern struct sctp_globals { >> #define sctp_assoc_hashtable (sctp_globals.assoc_hashtable) >> #define sctp_port_hashsize (sctp_globals.port_hashsize) >> #define sctp_port_hashtable (sctp_globals.port_hashtable) >> -#define sctp_checksum_disable (sctp_globals.checksum_disable) >> >> /* SCTP Socket type: UDP or TCP style. */ >> typedef enum { >> diff --git a/net/sctp/input.c b/net/sctp/input.c >> index fa91aff..b9a25e1 100644 >> --- a/net/sctp/input.c >> +++ b/net/sctp/input.c >> @@ -140,8 +140,8 @@ int sctp_rcv(struct sk_buff *skb) >> __skb_pull(skb, skb_transport_offset(skb)); >> if (skb->len < sizeof(struct sctphdr)) >> goto discard_it; >> - if (!sctp_checksum_disable && !skb_csum_unnecessary(skb) && >> - sctp_rcv_checksum(net, skb) < 0) >> + if (!net->sctp.checksum_disable && !skb_csum_unnecessary(skb) && >> + sctp_rcv_checksum(net, skb) < 0) >> goto discard_it; >> >> skb_pull(skb, sizeof(struct sctphdr)); >> diff --git a/net/sctp/output.c b/net/sctp/output.c >> index 5a55c55d..cdb5f49 100644 >> --- a/net/sctp/output.c >> +++ b/net/sctp/output.c >> @@ -395,6 +395,7 @@ int sctp_packet_transmit(struct sctp_packet *packet) >> int padding; /* How much padding do we need? */ >> __u8 has_data = 0; >> struct dst_entry *dst = tp->dst; >> + struct net *net; >> unsigned char *auth = NULL; /* pointer to auth in skb data */ >> __u32 cksum_buf_len = sizeof(struct sctphdr); >> >> @@ -541,7 +542,9 @@ int sctp_packet_transmit(struct sctp_packet *packet) >> * Note: Adler-32 is no longer applicable, as has been replaced >> * by CRC32-C as described in . >> */ >> - if (!sctp_checksum_disable) { >> + net = dev_net(dst->dev); >> + >> + if (!net->sctp.checksum_disable) { >> if (!(dst->dev->features & NETIF_F_SCTP_CSUM)) { >> __u32 crc32 = sctp_start_cksum((__u8 *)sh, cksum_buf_len); >> >> diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c >> index b52ec25..a570a63 100644 >> --- a/net/sctp/protocol.c >> +++ b/net/sctp/protocol.c >> @@ -1193,6 +1193,9 @@ static int __net_init sctp_net_init(struct net *net) >> /* Whether Cookie Preservative is enabled(1) or not(0) */ >> net->sctp.cookie_preserve_enable = 1; >> >> + /* Whether SCTP checksumming is disabled(1) or not(0) */ >> + net->sctp.checksum_disable = 0; >> + >> /* Default sctp sockets to use md5 as their hmac alg */ >> #if defined (CONFIG_SCTP_DEFAULT_COOKIE_HMAC_MD5) >> net->sctp.sctp_hmac_alg = "md5"; >> @@ -1549,6 +1552,4 @@ MODULE_ALIAS("net-pf-" __stringify(PF_INET) "-proto-132"); >> MODULE_ALIAS("net-pf-" __stringify(PF_INET6) "-proto-132"); >> MODULE_AUTHOR("Linux Kernel SCTP developers "); >> MODULE_DESCRIPTION("Support for the SCTP protocol (RFC2960)"); >> -module_param_named(no_checksums, sctp_checksum_disable, bool, 0644); >> -MODULE_PARM_DESC(no_checksums, "Disable checksums computing and verification"); >> MODULE_LICENSE("GPL"); >> diff --git a/net/sctp/sysctl.c b/net/sctp/sysctl.c >> index 1906747..754809a 100644 >> --- a/net/sctp/sysctl.c >> +++ b/net/sctp/sysctl.c >> @@ -296,7 +296,15 @@ static struct ctl_table sctp_net_table[] = { >> .extra1 = &max_autoclose_min, >> .extra2 = &max_autoclose_max, >> }, >> - >> + { >> + .procname = "checksum_disable", >> + .data = &init_net.sctp.checksum_disable, >> + .maxlen = sizeof(int), >> + .mode = 0644, >> + .proc_handler = proc_dointvec_minmax, >> + .extra1 = &zero, >> + .extra2 = &one, >> + }, >> { /* sentinel */ } >> }; >> >> >