proc_dointvec() and write

proc_dointvec() and write

[J. R. Okajima]

The commit 00b7c3395aec3df43de5bd02a3c5a099ca51169f
"sysctl: refactor integer handling proc code"
modified the behaviour of writing to /proc.

Before the commit, write("1\n") to /proc/sys/kernel/printk succeeded. But
now it returns EINVAL. Is this intended change? If not, how about this
patch?


J. R. Okajima

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 4c93486..5058e12 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2262,6 +2272,8 @@ static int __do_proc_dointvec(void *tbl_data, struct ctl_table *table,
 		if (write) {
 			left -= proc_skip_spaces(&kbuf);
 
+			if (!left)
+				break;
 			err = proc_get_long(&kbuf, &left, &lval, &neg,
 					     proc_wspace_sep,
 					     sizeof(proc_wspace_sep), NULL);
@@ -2288,7 +2306,7 @@ static int __do_proc_dointvec(void *tbl_data, struct ctl_table *table,
 
 	if (!write && !first && left && !err)
 		err = proc_put_char(&buffer, &left, '\n');
-	if (write && !err)
+	if (write && !err && left)
 		left -= proc_skip_spaces(&kbuf);
 free:
 	if (write) {

Re: proc_dointvec() and write

[Cong Wang]

On 05/24/10 22:32, J. R. Okajima wrote:
> The commit 00b7c3395aec3df43de5bd02a3c5a099ca51169f
> "sysctl: refactor integer handling proc code"
> modified the behaviour of writing to /proc.
>
> Before the commit, write("1\n") to /proc/sys/kernel/printk succeeded. But
> now it returns EINVAL. Is this intended change? If not, how about this
> patch?
>


Hmm, odd, I tested other proc files, I see no problem.
I am wondering why /proc/sys/kernel/printk is special here.

I will be back later.

>
> J. R. Okajima
>
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 4c93486..5058e12 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -2262,6 +2272,8 @@ static int __do_proc_dointvec(void *tbl_data, struct ctl_table *table,
>   		if (write) {
>   			left -= proc_skip_spaces(&kbuf);
>
> +			if (!left)
> +				break;
>   			err = proc_get_long(&kbuf,&left,&lval,&neg,
>   					     proc_wspace_sep,
>   					     sizeof(proc_wspace_sep), NULL);
> @@ -2288,7 +2306,7 @@ static int __do_proc_dointvec(void *tbl_data, struct ctl_table *table,
>
>   	if (!write&&  !first&&  left&&  !err)
>   		err = proc_put_char(&buffer,&left, '\n');
> -	if (write&&  !err)
> +	if (write&&  !err&&  left)
>   		left -= proc_skip_spaces(&kbuf);
>   free:
>   	if (write) {
>

Re: proc_dointvec() and write

[J. R. Okajima]

Cong Wang:
> Hmm, odd, I tested other proc files, I see no problem.
> I am wondering why /proc/sys/kernel/printk is special here.

The condition is,
- the entry has multiple values, .maxlen = N * sizeof(foo)
- the entry is writable, .mode = 0644 (or something)

In sysctl.c, only "printk" and "acct" match.


J. R. Okajima

Re: proc_dointvec() and write

[Cong Wang]

On 05/25/10 12:57, J. R. Okajima wrote:
> Cong Wang:
>> Hmm, odd, I tested other proc files, I see no problem.
>> I am wondering why /proc/sys/kernel/printk is special here.
>
> The condition is,
> - the entry has multiple values, .maxlen = N * sizeof(foo)
> - the entry is writable, .mode = 0644 (or something)
>
> In sysctl.c, only "printk" and "acct" match.

Hmm, right, I missed this case during testing.

Your patch looks correct for me. Would you mind to send your
patch again with your Signed-off-by?

Thank you!

[PATCH] proc_dointvec, write a single value

[J. R. Okajima]

The commit 00b7c3395aec3df43de5bd02a3c5a099ca51169f
"sysctl: refactor integer handling proc code"
modified the behaviour of writing to /proc.
Before the commit, write("1\n") to /proc/sys/kernel/printk succeeded. But
now it returns EINVAL.

This commit supports writing a single value to a multi-valued entry.

Signed-off-by: J. R. Okajima <hooanon05@yahoo.co.jp>
---
 kernel/sysctl.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 4c93486..78ac3fb 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2262,6 +2262,8 @@ static int __do_proc_dointvec(void *tbl_data, struct ctl_table *table,
 		if (write) {
 			left -= proc_skip_spaces(&kbuf);
 
+			if (!left)
+				break;
 			err = proc_get_long(&kbuf, &left, &lval, &neg,
 					     proc_wspace_sep,
 					     sizeof(proc_wspace_sep), NULL);
@@ -2288,7 +2290,7 @@ static int __do_proc_dointvec(void *tbl_data, struct ctl_table *table,
 
 	if (!write && !first && left && !err)
 		err = proc_put_char(&buffer, &left, '\n');
-	if (write && !err)
+	if (write && !err && left)
 		left -= proc_skip_spaces(&kbuf);
 free:
 	if (write) {
-- 
1.6.6.1

Re: [PATCH] proc_dointvec, write a single value

[Cong Wang]

On 05/25/10 14:49, J. R. Okajima wrote:
> The commit 00b7c3395aec3df43de5bd02a3c5a099ca51169f
> "sysctl: refactor integer handling proc code"
> modified the behaviour of writing to /proc.
> Before the commit, write("1\n") to /proc/sys/kernel/printk succeeded. But
> now it returns EINVAL.
>
> This commit supports writing a single value to a multi-valued entry.
>
> Signed-off-by: J. R. Okajima<hooanon05@yahoo.co.jp>


Reviewed-and-tested-by: WANG Cong <amwang@redhat.com>

Thanks a lot!


> ---
>   kernel/sysctl.c |    4 +++-
>   1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 4c93486..78ac3fb 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -2262,6 +2262,8 @@ static int __do_proc_dointvec(void *tbl_data, struct ctl_table *table,
>   		if (write) {
>   			left -= proc_skip_spaces(&kbuf);
>
> +			if (!left)
> +				break;
>   			err = proc_get_long(&kbuf,&left,&lval,&neg,
>   					     proc_wspace_sep,
>   					     sizeof(proc_wspace_sep), NULL);
> @@ -2288,7 +2290,7 @@ static int __do_proc_dointvec(void *tbl_data, struct ctl_table *table,
>
>   	if (!write&&  !first&&  left&&  !err)
>   		err = proc_put_char(&buffer,&left, '\n');
> -	if (write&&  !err)
> +	if (write&&  !err&&  left)
>   		left -= proc_skip_spaces(&kbuf);
>   free:
>   	if (write) {

Re: [PATCH] proc_dointvec, write a single value

[David Miller]

From: Cong Wang <amwang@redhat.com>
Date: Tue, 25 May 2010 20:23:47 +0800

> On 05/25/10 14:49, J. R. Okajima wrote:
>> The commit 00b7c3395aec3df43de5bd02a3c5a099ca51169f
>> "sysctl: refactor integer handling proc code"
>> modified the behaviour of writing to /proc.
>> Before the commit, write("1\n") to /proc/sys/kernel/printk
>> succeeded. But
>> now it returns EINVAL.
>>
>> This commit supports writing a single value to a multi-valued entry.
>>
>> Signed-off-by: J. R. Okajima<hooanon05@yahoo.co.jp>
> 
> Reviewed-and-tested-by: WANG Cong <amwang@redhat.com>

Since the regression causing change came in via the net tree
I'll integrate this fix too, applied, thanks!

Re: [PATCH] proc_dointvec, write a single value

[Eric W. Biederman]

David Miller <davem@davemloft.net> writes:

> From: Cong Wang <amwang@redhat.com>
> Date: Tue, 25 May 2010 20:23:47 +0800
>
>> On 05/25/10 14:49, J. R. Okajima wrote:
>>> The commit 00b7c3395aec3df43de5bd02a3c5a099ca51169f
>>> "sysctl: refactor integer handling proc code"
>>> modified the behaviour of writing to /proc.
>>> Before the commit, write("1\n") to /proc/sys/kernel/printk
>>> succeeded. But
>>> now it returns EINVAL.
>>>
>>> This commit supports writing a single value to a multi-valued entry.
>>>
>>> Signed-off-by: J. R. Okajima<hooanon05@yahoo.co.jp>
>> 
>> Reviewed-and-tested-by: WANG Cong <amwang@redhat.com>
>
> Since the regression causing change came in via the net tree
> I'll integrate this fix too, applied, thanks!

Thanks, guys for fixing this.

I hate to see my review comments show up like this in practice.
I had really hoped those unnecessary changes to proc would have
been dropped from that patchset.  Oh well.

Eric