[PATCH v2 0/1] lockd: Change nsm_use_hostnames from bool to u32

[PATCH v2 0/1] lockd: Change nsm_use_hostnames from bool to u32

[Jia He]

nsm_use_hostnames is a module parameter and it will be exported to sysctl
procfs. This is to let user sometimes change it from userspace. But the
minimal unit for sysctl procfs read/write it sizeof(int).
In big endian system, the converting from/to  bool to/from int will cause
error for proc items.

This patch changes the type definition of nsm_use_hostnames.

The test case I used:
/***************************************************************/
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/sysctl.h>

bool __read_mostly nsm_use_hostnames;
module_param(nsm_use_hostnames, bool, 0644);

static struct ctl_table my_sysctl[] = {
        {
                .procname       = "nsm_use_hostnames",
                .data           = &nsm_use_hostnames,
                .maxlen         = sizeof(int),
                .mode           = 0644,
                .proc_handler   = &proc_dointvec,
        },
        {}
};

static struct ctl_table my_root[] = {
        {
        .procname       = "mysysctl",
        .mode           = 0555,
        .child          = my_sysctl,
        },
        {}
};

static struct ctl_table_header * my_ctl_header;

static int __init sysctl_exam_init(void)
{
        my_ctl_header = register_sysctl_table(&my_root);
        if (my_ctl_header == NULL)
                printk("error regiester sysctl");

        return 0;
}

static void __exit sysctl_exam_exit(void)
{
        unregister_sysctl_table(my_ctl_header);
}

module_init(sysctl_exam_init);
module_exit(sysctl_exam_exit);
MODULE_LICENSE("GPL");
/****************************************************************/

[root@bigendian my]# insmod -f /root/my/hello.ko nsm_use_hostnames=1   
[root@bigendian my]# cat /proc/sys/mysysctl/nsm_use_hostnames 
16777216

After I change the bool to int:
[root@bigendian my]# insmod -f /root/my/hello.ko nsm_use_hostnames=1
[root@bigendian my]# cat /proc/sys/mysysctl/nsm_use_hostnames
1

In little endian system, there is no such issue.

Jia He (1):
  lockd: Change nsm_use_hostnames from bool to u32

 fs/lockd/mon.c              | 2 +-
 fs/lockd/svc.c              | 2 +-
 include/linux/lockd/lockd.h | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

-- 
2.5.5

[PATCH v2 1/1] lockd: Change nsm_use_hostnames from bool to u32

[Jia He]

nsm_use_hostnames is a module paramter and it will be exported to sysctl
procfs. This is to let user sometimes change it from userspace. But the
minimal unit for sysctl procfs read/write it sizeof(int).
In big endian system, the converting from/to  bool to/from int will cause
error for proc items.

This patch changes the type definition of nsm_use_hostnames.

V2: Changes extern type in lockd.h
Signed-off-by: Jia He <hejianet@gmail.com>
---
 fs/lockd/mon.c              | 2 +-
 fs/lockd/svc.c              | 2 +-
 include/linux/lockd/lockd.h | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
index 19166d4..3e7ff4d 100644
--- a/fs/lockd/mon.c
+++ b/fs/lockd/mon.c
@@ -57,7 +57,7 @@ static				DEFINE_SPINLOCK(nsm_lock);
  * Local NSM state
  */
 u32	__read_mostly		nsm_local_state;
-bool	__read_mostly		nsm_use_hostnames;
+u32	__read_mostly		nsm_use_hostnames;
 
 static inline struct sockaddr *nsm_addr(const struct nsm_handle *nsm)
 {
diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
index fc4084e..308033d 100644
--- a/fs/lockd/svc.c
+++ b/fs/lockd/svc.c
@@ -658,7 +658,7 @@ module_param_call(nlm_udpport, param_set_port, param_get_int,
 		  &nlm_udpport, 0644);
 module_param_call(nlm_tcpport, param_set_port, param_get_int,
 		  &nlm_tcpport, 0644);
-module_param(nsm_use_hostnames, bool, 0644);
+module_param(nsm_use_hostnames, u32, 0644);
 module_param(nlm_max_connections, uint, 0644);
 
 static int lockd_init_net(struct net *net)
diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
index c153738..db52152 100644
--- a/include/linux/lockd/lockd.h
+++ b/include/linux/lockd/lockd.h
@@ -196,7 +196,7 @@ extern struct svc_procedure	nlmsvc_procedures4[];
 #endif
 extern int			nlmsvc_grace_period;
 extern unsigned long		nlmsvc_timeout;
-extern bool			nsm_use_hostnames;
+extern u32			nsm_use_hostnames;
 extern u32			nsm_local_state;
 
 /*
-- 
2.5.5

Re: [PATCH v2 1/1] lockd: Change nsm_use_hostnames from bool to u32

[Pan Xinhui]

hi, jia
	nice catch!
However I think we should fix it totally.
This is because do_proc_dointvec_conv() try to get a int value from a bool *.

something like below might help. pls. ignore the code style and this is tested :)

diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
index fc4084e..7eeaee4 100644
--- a/fs/lockd/svc.c
+++ b/fs/lockd/svc.c
@@ -519,6 +519,8 @@ EXPORT_SYMBOL_GPL(lockd_down);
   * Sysctl parameters (same as module parameters, different interface).
   */
  
+int proc_dou8vec(struct ctl_table *table, int write,
+                                    void __user *buffer, size_t *lenp, loff_t *ppos);
  static struct ctl_table nlm_sysctls[] = {
         {
                 .procname       = "nlm_grace_period",
@@ -561,7 +563,7 @@ static struct ctl_table nlm_sysctls[] = {
                 .data           = &nsm_use_hostnames,
                 .maxlen         = sizeof(int),
                 .mode           = 0644,
-               .proc_handler   = proc_dointvec,
+               .proc_handler   = proc_dou8vec,
         },
         {
                 .procname       = "nsm_local_state",
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 706309f..6307737 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2112,6 +2112,30 @@ static int proc_put_char(void __user **buf, size_t *size, char c)
         return 0;
  }
  
+
+static int do_proc_dou8vec_conv(bool *negp, unsigned long *lvalp,
+                                u8 *valp,
+                                int write, void *data)
+{
+       if (write) {
+               if (*negp) {
+                       *valp = -*lvalp;
+               } else {
+                       *valp = *lvalp;
+               }
+       } else {
+               int val = *valp;
+               if (val < 0) {
+                       *negp = true;
+                       *lvalp = -(unsigned long)val;
+               } else {
+                       *negp = false;
+                       *lvalp = (unsigned long)val;
+               }
+       }
+       return 0;
+}
+
  static int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp,
                                  int *valp,
                                  int write, void *data)
@@ -2296,6 +2320,14 @@ int proc_douintvec(struct ctl_table *table, int write,
                                 do_proc_douintvec_conv, NULL);
  }
  
+int proc_dou8vec(struct ctl_table *table, int write,
+                    void __user *buffer, size_t *lenp, loff_t *ppos)
+{
+       return do_proc_dointvec(table, write, buffer, lenp, ppos,
+                               do_proc_dou8vec_conv, NULL);
+}
+
+

在 2016/12/11 23:36, Jia He 写道:
> nsm_use_hostnames is a module paramter and it will be exported to sysctl
> procfs. This is to let user sometimes change it from userspace. But the
> minimal unit for sysctl procfs read/write it sizeof(int).
> In big endian system, the converting from/to  bool to/from int will cause
> error for proc items.
>
> This patch changes the type definition of nsm_use_hostnames.
>
> V2: Changes extern type in lockd.h
> Signed-off-by: Jia He <hejianet@gmail.com>
> ---
>  fs/lockd/mon.c              | 2 +-
>  fs/lockd/svc.c              | 2 +-
>  include/linux/lockd/lockd.h | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
> index 19166d4..3e7ff4d 100644
> --- a/fs/lockd/mon.c
> +++ b/fs/lockd/mon.c
> @@ -57,7 +57,7 @@ static				DEFINE_SPINLOCK(nsm_lock);
>   * Local NSM state
>   */
>  u32	__read_mostly		nsm_local_state;
> -bool	__read_mostly		nsm_use_hostnames;
> +u32	__read_mostly		nsm_use_hostnames;
>
>  static inline struct sockaddr *nsm_addr(const struct nsm_handle *nsm)
>  {
> diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
> index fc4084e..308033d 100644
> --- a/fs/lockd/svc.c
> +++ b/fs/lockd/svc.c
> @@ -658,7 +658,7 @@ module_param_call(nlm_udpport, param_set_port, param_get_int,
>  		  &nlm_udpport, 0644);
>  module_param_call(nlm_tcpport, param_set_port, param_get_int,
>  		  &nlm_tcpport, 0644);
> -module_param(nsm_use_hostnames, bool, 0644);
> +module_param(nsm_use_hostnames, u32, 0644);
>  module_param(nlm_max_connections, uint, 0644);
>
>  static int lockd_init_net(struct net *net)
> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
> index c153738..db52152 100644
> --- a/include/linux/lockd/lockd.h
> +++ b/include/linux/lockd/lockd.h
> @@ -196,7 +196,7 @@ extern struct svc_procedure	nlmsvc_procedures4[];
>  #endif
>  extern int			nlmsvc_grace_period;
>  extern unsigned long		nlmsvc_timeout;
> -extern bool			nsm_use_hostnames;
> +extern u32			nsm_use_hostnames;
>  extern u32			nsm_local_state;
>
>  /*
>

Re: [PATCH v2 1/1] lockd: Change nsm_use_hostnames from bool to u32

[Pan Xinhui]

在 2016/12/12 01:43, Pan Xinhui 写道:
>
> hi, jia
>     nice catch!
> However I think we should fix it totally.
> This is because do_proc_dointvec_conv() try to get a int value from a bool *.
>
> something like below might help. pls. ignore the code style and this is tested :)
									_untested_.

Re: [PATCH v2 1/1] lockd: Change nsm_use_hostnames from bool to u32

[hejianet]

Hi Xinhui
Thanks, it really works.
Will send out V3 soon afterwards

B.R.
Jia

On 12/12/16 1:43 AM, Pan Xinhui wrote:
>
> hi, jia
>     nice catch!
> However I think we should fix it totally.
> This is because do_proc_dointvec_conv() try to get a int value from a bool *.
>
> something like below might help. pls. ignore the code style and this is tested :)
>
> diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
> index fc4084e..7eeaee4 100644
> --- a/fs/lockd/svc.c
> +++ b/fs/lockd/svc.c
> @@ -519,6 +519,8 @@ EXPORT_SYMBOL_GPL(lockd_down);
>   * Sysctl parameters (same as module parameters, different interface).
>   */
>
> +int proc_dou8vec(struct ctl_table *table, int write,
> +                                    void __user *buffer, size_t *lenp, loff_t 
> *ppos);
>  static struct ctl_table nlm_sysctls[] = {
>         {
>                 .procname       = "nlm_grace_period",
> @@ -561,7 +563,7 @@ static struct ctl_table nlm_sysctls[] = {
>                 .data           = &nsm_use_hostnames,
>                 .maxlen         = sizeof(int),
>                 .mode           = 0644,
> -               .proc_handler   = proc_dointvec,
> +               .proc_handler   = proc_dou8vec,
>         },
>         {
>                 .procname       = "nsm_local_state",
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 706309f..6307737 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -2112,6 +2112,30 @@ static int proc_put_char(void __user **buf, size_t 
> *size, char c)
>         return 0;
>  }
>
> +
> +static int do_proc_dou8vec_conv(bool *negp, unsigned long *lvalp,
> +                                u8 *valp,
> +                                int write, void *data)
> +{
> +       if (write) {
> +               if (*negp) {
> +                       *valp = -*lvalp;
> +               } else {
> +                       *valp = *lvalp;
> +               }
> +       } else {
> +               int val = *valp;
> +               if (val < 0) {
> +                       *negp = true;
> +                       *lvalp = -(unsigned long)val;
> +               } else {
> +                       *negp = false;
> +                       *lvalp = (unsigned long)val;
> +               }
> +       }
> +       return 0;
> +}
> +
>  static int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp,
>                                  int *valp,
>                                  int write, void *data)
> @@ -2296,6 +2320,14 @@ int proc_douintvec(struct ctl_table *table, int write,
>                                 do_proc_douintvec_conv, NULL);
>  }
>
> +int proc_dou8vec(struct ctl_table *table, int write,
> +                    void __user *buffer, size_t *lenp, loff_t *ppos)
> +{
> +       return do_proc_dointvec(table, write, buffer, lenp, ppos,
> +                               do_proc_dou8vec_conv, NULL);
> +}
> +
> +
>
> 在 2016/12/11 23:36, Jia He 写道:
>> nsm_use_hostnames is a module paramter and it will be exported to sysctl
>> procfs. This is to let user sometimes change it from userspace. But the
>> minimal unit for sysctl procfs read/write it sizeof(int).
>> In big endian system, the converting from/to  bool to/from int will cause
>> error for proc items.
>>
>> This patch changes the type definition of nsm_use_hostnames.
>>
>> V2: Changes extern type in lockd.h
>> Signed-off-by: Jia He <hejianet@gmail.com>
>> ---
>>  fs/lockd/mon.c              | 2 +-
>>  fs/lockd/svc.c              | 2 +-
>>  include/linux/lockd/lockd.h | 2 +-
>>  3 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
>> index 19166d4..3e7ff4d 100644
>> --- a/fs/lockd/mon.c
>> +++ b/fs/lockd/mon.c
>> @@ -57,7 +57,7 @@ static DEFINE_SPINLOCK(nsm_lock);
>>   * Local NSM state
>>   */
>>  u32    __read_mostly        nsm_local_state;
>> -bool    __read_mostly        nsm_use_hostnames;
>> +u32    __read_mostly        nsm_use_hostnames;
>>
>>  static inline struct sockaddr *nsm_addr(const struct nsm_handle *nsm)
>>  {
>> diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
>> index fc4084e..308033d 100644
>> --- a/fs/lockd/svc.c
>> +++ b/fs/lockd/svc.c
>> @@ -658,7 +658,7 @@ module_param_call(nlm_udpport, param_set_port, 
>> param_get_int,
>>            &nlm_udpport, 0644);
>>  module_param_call(nlm_tcpport, param_set_port, param_get_int,
>>            &nlm_tcpport, 0644);
>> -module_param(nsm_use_hostnames, bool, 0644);
>> +module_param(nsm_use_hostnames, u32, 0644);
>>  module_param(nlm_max_connections, uint, 0644);
>>
>>  static int lockd_init_net(struct net *net)
>> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
>> index c153738..db52152 100644
>> --- a/include/linux/lockd/lockd.h
>> +++ b/include/linux/lockd/lockd.h
>> @@ -196,7 +196,7 @@ extern struct svc_procedure nlmsvc_procedures4[];
>>  #endif
>>  extern int            nlmsvc_grace_period;
>>  extern unsigned long        nlmsvc_timeout;
>> -extern bool            nsm_use_hostnames;
>> +extern u32            nsm_use_hostnames;
>>  extern u32            nsm_local_state;
>>
>>  /*
>>
>
>

Re: [PATCH v2 1/1] lockd: Change nsm_use_hostnames from bool to u32

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3303 bytes --]

Hi Jia,

[auto build test ERROR on nfsd/nfsd-next]
[also build test ERROR on v4.9 next-20161209]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Jia-He/lockd-Change-nsm_use_hostnames-from-bool-to-u32/20161212-001614
base:   git://linux-nfs.org/~bfields/linux.git nfsd-next
config: x86_64-kexec (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   In file included from include/linux/module.h:18:0,
                    from fs/lockd/svc.c:15:
>> include/linux/moduleparam.h:146:27: error: expected ')' before '&' token
     param_check_##type(name, &(value));       \
                              ^
   include/linux/moduleparam.h:126:2: note: in expansion of macro 'module_param_named'
     module_param_named(name, name, type, perm)
     ^~~~~~~~~~~~~~~~~~
   fs/lockd/svc.c:661:1: note: in expansion of macro 'module_param'
    module_param(nsm_use_hostnames, u32, 0644);
    ^~~~~~~~~~~~
>> include/linux/moduleparam.h:147:25: error: 'param_ops_u32' undeclared here (not in a function)
     module_param_cb(name, &param_ops_##type, &value, perm);     \
                            ^
   include/linux/moduleparam.h:224:39: note: in definition of macro '__module_param_call'
     = { __param_str_##name, THIS_MODULE, ops,   \
                                          ^~~
   include/linux/moduleparam.h:147:2: note: in expansion of macro 'module_param_cb'
     module_param_cb(name, &param_ops_##type, &value, perm);     \
     ^~~~~~~~~~~~~~~
   include/linux/moduleparam.h:126:2: note: in expansion of macro 'module_param_named'
     module_param_named(name, name, type, perm)
     ^~~~~~~~~~~~~~~~~~
   fs/lockd/svc.c:661:1: note: in expansion of macro 'module_param'
    module_param(nsm_use_hostnames, u32, 0644);
    ^~~~~~~~~~~~

vim +146 include/linux/moduleparam.h

546970bc Rusty Russell 2010-08-11  140   *
546970bc Rusty Russell 2010-08-11  141   * Usually it's a good idea to have variable names and user-exposed names the
546970bc Rusty Russell 2010-08-11  142   * same, but that's harder if the variable must be non-static or is inside a
546970bc Rusty Russell 2010-08-11  143   * structure.  This allows exposure under a different name.
546970bc Rusty Russell 2010-08-11  144   */
546970bc Rusty Russell 2010-08-11  145  #define module_param_named(name, value, type, perm)			   \
546970bc Rusty Russell 2010-08-11 @146  	param_check_##type(name, &(value));				   \
546970bc Rusty Russell 2010-08-11 @147  	module_param_cb(name, &param_ops_##type, &value, perm);		   \
546970bc Rusty Russell 2010-08-11  148  	__MODULE_PARM_TYPE(name, #type)
546970bc Rusty Russell 2010-08-11  149  
546970bc Rusty Russell 2010-08-11  150  /**

:::::: The code at line 146 was first introduced by commit
:::::: 546970bc6afc7fb37447fbac09b82c7884662c21 param: add kerneldoc to moduleparam.h

:::::: TO: Rusty Russell <rusty@rustcorp.com.au>
:::::: CC: Rusty Russell <rusty@rustcorp.com.au>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 24265 bytes --]

Re: [PATCH v2 0/1] lockd: Change nsm_use_hostnames from bool to u32

[Pan Xinhui]

在 2016/12/11 23:36, Jia He 写道:
> nsm_use_hostnames is a module parameter and it will be exported to sysctl
> procfs. This is to let user sometimes change it from userspace. But the
> minimal unit for sysctl procfs read/write it sizeof(int).
> In big endian system, the converting from/to  bool to/from int will cause
> error for proc items.
hi, Jia

not only in BE system. :)
Current code is just touching a wrong pointer.

some tests based on yours

u8 __read_mostly nsm_use_hostnames[4]={1,2,3,4}; // an arrary of u8, and [0] passed to ctl_table as data

static struct ctl_table my_sysctl[] = {
         {
                 .procname       = "nsm_use_hostnames",
                 .data           = &nsm_use_hostnames[0],//u8,
                 .maxlen         = sizeof(int),
                 .mode           = 0644,
                 .proc_handler   = &proc_dointvec,
         },
         {}
};

then run your tests and result will be

root@ltcalpine2-lp13:~/linux/bench# cat /proc/sys/mysysctl/nsm_use_hostnames
67305985( This is 0x4030201, expected be 0x1)

So your fix patch work around it. But I suggest we can support u8/u16, not only int/double int.

thanks
xinhui
  
>
> This patch changes the type definition of nsm_use_hostnames.
>
> The test case I used:
> /***************************************************************/
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/sysctl.h>
>
> bool __read_mostly nsm_use_hostnames;
> module_param(nsm_use_hostnames, bool, 0644);
>
> static struct ctl_table my_sysctl[] = {
>         {
>                 .procname       = "nsm_use_hostnames",
>                 .data           = &nsm_use_hostnames,
>                 .maxlen         = sizeof(int),
>                 .mode           = 0644,
>                 .proc_handler   = &proc_dointvec,
>         },
>         {}
> };
>
> static struct ctl_table my_root[] = {
>         {
>         .procname       = "mysysctl",
>         .mode           = 0555,
>         .child          = my_sysctl,
>         },
>         {}
> };
>
> static struct ctl_table_header * my_ctl_header;
>
> static int __init sysctl_exam_init(void)
> {
>         my_ctl_header = register_sysctl_table(&my_root);
>         if (my_ctl_header == NULL)
>                 printk("error regiester sysctl");
>
>         return 0;
> }
>
> static void __exit sysctl_exam_exit(void)
> {
>         unregister_sysctl_table(my_ctl_header);
> }
>
> module_init(sysctl_exam_init);
> module_exit(sysctl_exam_exit);
> MODULE_LICENSE("GPL");
> /****************************************************************/
>
> [root@bigendian my]# insmod -f /root/my/hello.ko nsm_use_hostnames=1
> [root@bigendian my]# cat /proc/sys/mysysctl/nsm_use_hostnames
> 16777216
>
> After I change the bool to int:
> [root@bigendian my]# insmod -f /root/my/hello.ko nsm_use_hostnames=1
> [root@bigendian my]# cat /proc/sys/mysysctl/nsm_use_hostnames
> 1
>
> In little endian system, there is no such issue.
>
> Jia He (1):
>   lockd: Change nsm_use_hostnames from bool to u32
>
>  fs/lockd/mon.c              | 2 +-
>  fs/lockd/svc.c              | 2 +-
>  include/linux/lockd/lockd.h | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
>