Re: [PATCH net-next 1/2] net: sctp: convert sctp_checksum_disable module param into sctp sysctl

[Daniel Borkmann <dborkman@redhat.com>]

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 <dborkman@redhat.com>
>> ---
>>   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 <draft-ietf-tsvwg-sctpcsum-02.txt>.
>>        */
>> -    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 <linux-sctp@vger.kernel.org>");
>>   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 */ }
>>   };
>>
>>
>