[PATCH] kernel/sysctl.c: If "count" including the terminating byte '\0' the write system call should retrun success.

[PATCH] kernel/sysctl.c: If "count" including the terminating byte '\0' the write system call should retrun success.

[Sean Fu]

when the input argument "count" including the terminating byte "\0",
The write system call return EINVAL on proc file.
But it return success on regular file.

E.g. Writting two bytes ("1\0") to "/proc/sys/net/ipv4/conf/eth0/rp_filter".
write(fd, "1\0", 2) return EINVAL.

Signed-off-by: Sean Fu <fxinrong@gmail.com>
---
 kernel/sysctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 19b62b5..c2b0594 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2004,7 +2004,7 @@ static int do_proc_dointvec_conv(bool *negp,
unsigned long *lvalp,
        return 0;
 }

-static const char proc_wspace_sep[] = { ' ', '\t', '\n' };
+static const char proc_wspace_sep[] = { ' ', '\t', '\n', '\0' };

 static int __do_proc_dointvec(void *tbl_data, struct ctl_table *table,
                  int write, void __user *buffer,
-- 
2.1.2

Re: [PATCH] kernel/sysctl.c: If "count" including the terminating byte '\0' the write system call should retrun success.

[Eric W. Biederman]

On August 24, 2015 1:56:13 AM PDT, Sean Fu <fxinrong@gmail.com> wrote:
>when the input argument "count" including the terminating byte "\0",
>The write system call return EINVAL on proc file.
>But it return success on regular file.

Nonsense.  It will write the '\0' to a regular file because it is just data.

Integers in proc are more than data.

So I see no justification for this change.


Eric

>E.g. Writting two bytes ("1\0") to
>"/proc/sys/net/ipv4/conf/eth0/rp_filter".
>write(fd, "1\0", 2) return EINVAL.
>
>Signed-off-by: Sean Fu <fxinrong@gmail.com>
>---
> kernel/sysctl.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/kernel/sysctl.c b/kernel/sysctl.c
>index 19b62b5..c2b0594 100644
>--- a/kernel/sysctl.c
>+++ b/kernel/sysctl.c
>@@ -2004,7 +2004,7 @@ static int do_proc_dointvec_conv(bool *negp,
>unsigned long *lvalp,
>        return 0;
> }
>
>-static const char proc_wspace_sep[] = { ' ', '\t', '\n' };
>+static const char proc_wspace_sep[] = { ' ', '\t', '\n', '\0' };
>
> static int __do_proc_dointvec(void *tbl_data, struct ctl_table *table,
>                  int write, void __user *buffer,

Re: [PATCH] kernel/sysctl.c: If "count" including the terminating byte '\0' the write system call should retrun success.

[Sean Fu]

On Mon, Aug 24, 2015 at 8:27 PM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
>
>
> On August 24, 2015 1:56:13 AM PDT, Sean Fu <fxinrong@gmail.com> wrote:
>>when the input argument "count" including the terminating byte "\0",
>>The write system call return EINVAL on proc file.
>>But it return success on regular file.
>
> Nonsense.  It will write the '\0' to a regular file because it is just data.
>
> Integers in proc are more than data.
>
> So I see no justification for this change.
In fact, "write(fd, "1\0", 2)" on Integers proc file return success on
2.6 kernel. I already tested it on 2.6.6.60 kernel.

So, The latest behavior of "write(fd, "1\0", 2)" is different from old
kernel(2.6).
This maybe impact the compatibility of some user space program.
>
>
> Eric
>
>>E.g. Writting two bytes ("1\0") to
>>"/proc/sys/net/ipv4/conf/eth0/rp_filter".
>>write(fd, "1\0", 2) return EINVAL.
>>
>>Signed-off-by: Sean Fu <fxinrong@gmail.com>
>>---
>> kernel/sysctl.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>>diff --git a/kernel/sysctl.c b/kernel/sysctl.c
>>index 19b62b5..c2b0594 100644
>>--- a/kernel/sysctl.c
>>+++ b/kernel/sysctl.c
>>@@ -2004,7 +2004,7 @@ static int do_proc_dointvec_conv(bool *negp,
>>unsigned long *lvalp,
>>        return 0;
>> }
>>
>>-static const char proc_wspace_sep[] = { ' ', '\t', '\n' };
>>+static const char proc_wspace_sep[] = { ' ', '\t', '\n', '\0' };
>>
>> static int __do_proc_dointvec(void *tbl_data, struct ctl_table *table,
>>                  int write, void __user *buffer,
>

Re: [PATCH] kernel/sysctl.c: If "count" including the terminating byte '\0' the write system call should retrun success.

[Steven Rostedt]

On Mon, 24 Aug 2015 16:56:13 +0800
Sean Fu <fxinrong@gmail.com> wrote:

> when the input argument "count" including the terminating byte "\0",
> The write system call return EINVAL on proc file.
> But it return success on regular file.
> 
> E.g. Writting two bytes ("1\0") to "/proc/sys/net/ipv4/conf/eth0/rp_filter".
> write(fd, "1\0", 2) return EINVAL.

And what would do that? What tool broke because of this?

 echo 1 > /proc/sys/net/ipv4/conf/eth0/rp_filter

works just fine. strlen("string") would not include the nul character.
The only thing I could think of would be a sizeof(str), but then that
would include someone hardcoding an integer in a string, like:

	char val[] = "1"

	write(fd, val, sizeof(val));

Again, what tool does that?

If there is a tool out in the wild that use to work on 2.6 (and was
running on 2.6 then, and not something that was created after that
change), then we can consider this fix.

-- Steve

Re: [PATCH] kernel/sysctl.c: If "count" including the terminating byte '\0' the write system call should retrun success.

[Andrew Morton]

On Mon, 24 Aug 2015 23:33:58 +0800 Sean Fu <fxinrong@gmail.com> wrote:

> On Mon, Aug 24, 2015 at 8:27 PM, Eric W. Biederman
> <ebiederm@xmission.com> wrote:
> >
> >
> > On August 24, 2015 1:56:13 AM PDT, Sean Fu <fxinrong@gmail.com> wrote:
> >>when the input argument "count" including the terminating byte "\0",
> >>The write system call return EINVAL on proc file.
> >>But it return success on regular file.
> >
> > Nonsense.  It will write the '\0' to a regular file because it is just data.
> >
> > Integers in proc are more than data.
> >
> > So I see no justification for this change.
> In fact, "write(fd, "1\0", 2)" on Integers proc file return success on
> 2.6 kernel. I already tested it on 2.6.6.60 kernel.
> 
> So, The latest behavior of "write(fd, "1\0", 2)" is different from old
> kernel(2.6).
> This maybe impact the compatibility of some user space program.

2.6 was a long time ago.  If this behaviour change has happened in the
last 1-2 kernel releases then there would be a case to consider making
changes.  But if the kernel has been this way for two years then it's
too late to bother switching back to the old (and strange) behaviour.

Re: [PATCH] kernel/sysctl.c: If "count" including the terminating byte '\0' the write system call should retrun success.

[Heinrich Schuchardt]

This seems to be the relevant patch:

https://lkml.org/lkml/2010/5/5/104
Amerigo Wang <amwang@redhat.com>  2010-05-05 02:26:45
00b7c3395aec3df43de5bd02a3c5a099ca51169f
+static const char proc_wspace_sep[] = { ' ', '\t', '\n' };

So since 2010 we have the current behavior.

Best regards

Heinrich Schuchardt

On 24.08.2015 22:44, Andrew Morton wrote:
> On Mon, 24 Aug 2015 23:33:58 +0800 Sean Fu <fxinrong@gmail.com> wrote:
> 
>> On Mon, Aug 24, 2015 at 8:27 PM, Eric W. Biederman
>> <ebiederm@xmission.com> wrote:
>>>
>>>
>>> On August 24, 2015 1:56:13 AM PDT, Sean Fu <fxinrong@gmail.com> wrote:
>>>> when the input argument "count" including the terminating byte "\0",
>>>> The write system call return EINVAL on proc file.
>>>> But it return success on regular file.
>>>
>>> Nonsense.  It will write the '\0' to a regular file because it is just data.
>>>
>>> Integers in proc are more than data.
>>>
>>> So I see no justification for this change.
>> In fact, "write(fd, "1\0", 2)" on Integers proc file return success on
>> 2.6 kernel. I already tested it on 2.6.6.60 kernel.
>>
>> So, The latest behavior of "write(fd, "1\0", 2)" is different from old
>> kernel(2.6).
>> This maybe impact the compatibility of some user space program.
> 
> 2.6 was a long time ago.  If this behaviour change has happened in the
> last 1-2 kernel releases then there would be a case to consider making
> changes.  But if the kernel has been this way for two years then it's
> too late to bother switching back to the old (and strange) behaviour.
> 
> 

Re: [PATCH] kernel/sysctl.c: If "count" including the terminating byte '\0' the write system call should retrun success.

[Sean Fu]

An application from HuaWei which works fine on 2.6 encounters this
issue on 3.0 or later kernel.

Test code:

#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <unistd.h>
#include <stdio.h>
#include <string.h>
#include <errno.h>

#define MAXLEN (100)

int main(int argc, char** argv)
{
        int fd = 0;
        int len = 0;
        char pathname[MAXLEN] = {0};
        char buf[2] = {0};
        int ret = 0xF;
        int value = 0xF;

        if (argc < 2)
        {
                printf("Input param error, less 2 param: %s, %s.\n",
argv[0], argv[1]);
                return 1;
        }

        printf("Input: %s, %s \n", argv[0], argv[1]);

        ret = sprintf(pathname,
"/proc/sys/net/ipv4/conf/%s/rp_filter", argv[1]);
        if (ret < 0)
                printf("sprintf error, errno %d, %s\n", errno, strerror(errno));
        printf("file is: %s. \n", pathname);

        fd = open(pathname, O_RDWR, S_IRUSR);
        if (fd <=0 )
        {
                printf("open %s failed, errno=%d, %s.\n", pathname,
errno, strerror(errno));
                return 1;
        }
        printf("open %s ok, fd=%d\n", pathname, fd);

        len = write(fd, "0\0", 2);
        printf("write %d: len=%d, errno=%d, %s\n", fd, len, errno,
strerror(errno));

        close(fd);
        return 0;
}

On Tue, Aug 25, 2015 at 12:59 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Mon, 24 Aug 2015 16:56:13 +0800
> Sean Fu <fxinrong@gmail.com> wrote:
>
>> when the input argument "count" including the terminating byte "\0",
>> The write system call return EINVAL on proc file.
>> But it return success on regular file.
>>
>> E.g. Writting two bytes ("1\0") to "/proc/sys/net/ipv4/conf/eth0/rp_filter".
>> write(fd, "1\0", 2) return EINVAL.
>
> And what would do that? What tool broke because of this?
>
>  echo 1 > /proc/sys/net/ipv4/conf/eth0/rp_filter
>
> works just fine. strlen("string") would not include the nul character.
> The only thing I could think of would be a sizeof(str), but then that
> would include someone hardcoding an integer in a string, like:
>
>         char val[] = "1"
>
>         write(fd, val, sizeof(val));
>
> Again, what tool does that?
>
> If there is a tool out in the wild that use to work on 2.6 (and was
> running on 2.6 then, and not something that was created after that
> change), then we can consider this fix.
>
> -- Steve

Re: [PATCH] kernel/sysctl.c: If "count" including the terminating byte '\0' the write system call should retrun success.

[Eric W. Biederman]

On August 24, 2015 6:57:57 PM MDT, Sean Fu <fxinrong@gmail.com> wrote:
>An application from HuaWei which works fine on 2.6 encounters this
>issue on 3.0 or later kernel.

My sympathies.  Being stuck with a 3rd party application you can barely talk about that has been broken for 5years and no one reported it.

Ordinarily we would fix a regression like this. As it has been 5years the challenge now is how do we tell if there are applications that depend on the current behavior.

Before we can change the behavior back we need a convincing argument that we won't cause a regression in another application by making the change.

I do not see how such an argument can be made.  So you have my sympathies but I do not see how we can help you.

Eric

Re: [PATCH] kernel/sysctl.c: If "count" including the terminating byte '\0' the write system call should retrun success.

[Sean Fu]

Call path is "proc_dointvec-->do_proc_dointvec-->__do_proc_dointvec-->proc_get_long".
file: kernel/sysctl.c
function: proc_get_long
...
1927         if (len < *size && perm_tr_len && !memchr(perm_tr, *p,
perm_tr_len))                   //this line should accept two bytes
"1\0".
1928                 return -EINVAL;
...


The latest upstream kernel is also tested, and it is same as 3.16.7 kernel.

3.16.7 kernel:
sean@linux-dunz:~/work/suse_lab/proc_test> cat /proc/version
Linux version 3.16.7-7-desktop (geeko@buildhost) (gcc version 4.8.3
20140627 [gcc-4_8-branch revision 212064] (SUSE Linux) ) #1 SMP
PREEMPT Wed Dec 17 18:00:44 UTC 2014 (762f27a)
sean@linux-dunz:~/work/suse_lab/proc_test> gcc ./proc_test.c -o proc_test
sean@linux-dunz:~/work/suse_lab/proc_test> sudo ./proc_test enp0s25
Input: ./proc_test, enp0s25
file is: /proc/sys/net/ipv4/conf/enp0s25/rp_filter.
open /proc/sys/net/ipv4/conf/enp0s25/rp_filter ok, fd=3
write 3: len=-1, errno=22, Invalid argument

2.6.16.60 kernel:
linux-8lij:~ # gcc ./proc_test.c -o ./proc_test
linux-8lij:~ # cat /proc/version
Linux version 2.6.16.60-0.83.2-bigsmp (geeko@buildhost) (gcc version
4.1.2 20070115 (SUSE Linux)) #1 SMP Fri Sep 2 13:49:16 UTC 2011
linux-8lij:~ # gcc ./proc_test.c -o ./proc_test
linux-8lij:~ # ./proc_test eth7
Input: ./proc_test, eth7
file is: /proc/sys/net/ipv4/conf/eth7/rp_filter.
open /proc/sys/net/ipv4/conf/eth7/rp_filter ok, fd=3
write 3: len=1, errno=0, Success

On Tue, Aug 25, 2015 at 8:57 AM, Sean Fu <fxinrong@gmail.com> wrote:
> An application from HuaWei which works fine on 2.6 encounters this
> issue on 3.0 or later kernel.
>
> Test code:
>
> #include <sys/types.h>
> #include <sys/stat.h>
> #include <fcntl.h>
> #include <unistd.h>
> #include <stdio.h>
> #include <string.h>
> #include <errno.h>
>
> #define MAXLEN (100)
>
> int main(int argc, char** argv)
> {
>         int fd = 0;
>         int len = 0;
>         char pathname[MAXLEN] = {0};
>         char buf[2] = {0};
>         int ret = 0xF;
>         int value = 0xF;
>
>         if (argc < 2)
>         {
>                 printf("Input param error, less 2 param: %s, %s.\n",
> argv[0], argv[1]);
>                 return 1;
>         }
>
>         printf("Input: %s, %s \n", argv[0], argv[1]);
>
>         ret = sprintf(pathname,
> "/proc/sys/net/ipv4/conf/%s/rp_filter", argv[1]);
>         if (ret < 0)
>                 printf("sprintf error, errno %d, %s\n", errno, strerror(errno));
>         printf("file is: %s. \n", pathname);
>
>         fd = open(pathname, O_RDWR, S_IRUSR);
>         if (fd <=0 )
>         {
>                 printf("open %s failed, errno=%d, %s.\n", pathname,
> errno, strerror(errno));
>                 return 1;
>         }
>         printf("open %s ok, fd=%d\n", pathname, fd);
>
>         len = write(fd, "0\0", 2);
>         printf("write %d: len=%d, errno=%d, %s\n", fd, len, errno,
> strerror(errno));
>
>         close(fd);
>         return 0;
> }
>
> On Tue, Aug 25, 2015 at 12:59 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
>> On Mon, 24 Aug 2015 16:56:13 +0800
>> Sean Fu <fxinrong@gmail.com> wrote:
>>
>>> when the input argument "count" including the terminating byte "\0",
>>> The write system call return EINVAL on proc file.
>>> But it return success on regular file.
>>>
>>> E.g. Writting two bytes ("1\0") to "/proc/sys/net/ipv4/conf/eth0/rp_filter".
>>> write(fd, "1\0", 2) return EINVAL.
>>
>> And what would do that? What tool broke because of this?
>>
>>  echo 1 > /proc/sys/net/ipv4/conf/eth0/rp_filter
>>
>> works just fine. strlen("string") would not include the nul character.
>> The only thing I could think of would be a sizeof(str), but then that
>> would include someone hardcoding an integer in a string, like:
>>
>>         char val[] = "1"
>>
>>         write(fd, val, sizeof(val));
>>
>> Again, what tool does that?
>>
>> If there is a tool out in the wild that use to work on 2.6 (and was
>> running on 2.6 then, and not something that was created after that
>> change), then we can consider this fix.
>>
>> -- Steve

Re: [PATCH] kernel/sysctl.c: If "count" including the terminating byte '\0' the write system call should retrun success.

[Sean Fu]

On Tue, Aug 25, 2015 at 10:24 AM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
>
>
> On August 24, 2015 6:57:57 PM MDT, Sean Fu <fxinrong@gmail.com> wrote:
>>An application from HuaWei which works fine on 2.6 encounters this
>>issue on 3.0 or later kernel.
>
> My sympathies.  Being stuck with a 3rd party application you can barely talk about that has been broken for 5years and no one reported it.
>
> Ordinarily we would fix a regression like this. As it has been 5years the challenge now is how do we tell if there are applications that depend on the current behavior.
>
> Before we can change the behavior back we need a convincing argument that we won't cause a regression in another application by making the change.
>
> I do not see how such an argument can be made.  So you have my sympathies but I do not see how we can help you.
We should consider this patch basing on my following arguments.
1 Different version kernel should keep consistent on this behavior.
2 This writting behavior on proc file should be same with writting on
regular file as possible as we can.
3 This patch does not have any potential compatibility risk with 3rd
party application.
4 Support writting "1...\0" to proc file.

>
> Eric
>

Re: [PATCH] kernel/sysctl.c: If "count" including the terminating byte '\0' the write system call should retrun success.

[Steven Rostedt]

On Tue, 25 Aug 2015 15:50:18 +0800
Sean Fu <fxinrong@gmail.com> wrote:

> On Tue, Aug 25, 2015 at 10:24 AM, Eric W. Biederman
> <ebiederm@xmission.com> wrote:
> >
> >
> > On August 24, 2015 6:57:57 PM MDT, Sean Fu <fxinrong@gmail.com> wrote:
> >>An application from HuaWei which works fine on 2.6 encounters this
> >>issue on 3.0 or later kernel.
> >
> > My sympathies.  Being stuck with a 3rd party application you can barely talk about that has been broken for 5years and no one reported it.
> >
> > Ordinarily we would fix a regression like this. As it has been 5years the challenge now is how do we tell if there are applications that depend on the current behavior.
> >
> > Before we can change the behavior back we need a convincing argument that we won't cause a regression in another application by making the change.
> >
> > I do not see how such an argument can be made.  So you have my sympathies but I do not see how we can help you.
> We should consider this patch basing on my following arguments.
> 1 Different version kernel should keep consistent on this behavior.

The thing is, the above argument is against the patch. The behavior
changed 2 years ago, and nobody noticed. Changing it back only causes
more inconsistent behavior.


> 2 This writting behavior on proc file should be same with writting on
> regular file as possible as we can.

Writing to a proc file causes kernel actions. Writing to a regular file
just saves data. That's not an argument here.

> 3 This patch does not have any potential compatibility risk with 3rd
> party application.

How do you know that?

-- Steve

> 4 Support writting "1...\0" to proc file.
> 
> >
> > Eric
> >

Re: [PATCH] kernel/sysctl.c: If "count" including the terminating byte '\0' the write system call should retrun success.

[Sean Fu]

On Tue, Aug 25, 2015 at 10:15 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Tue, 25 Aug 2015 15:50:18 +0800
> Sean Fu <fxinrong@gmail.com> wrote:
>
>> On Tue, Aug 25, 2015 at 10:24 AM, Eric W. Biederman
>> <ebiederm@xmission.com> wrote:
>> >
>> >
>> > On August 24, 2015 6:57:57 PM MDT, Sean Fu <fxinrong@gmail.com> wrote:
>> >>An application from HuaWei which works fine on 2.6 encounters this
>> >>issue on 3.0 or later kernel.
>> >
>> > My sympathies.  Being stuck with a 3rd party application you can barely talk about that has been broken for 5years and no one reported it.
>> >
>> > Ordinarily we would fix a regression like this. As it has been 5years the challenge now is how do we tell if there are applications that depend on the current behavior.
>> >
>> > Before we can change the behavior back we need a convincing argument that we won't cause a regression in another application by making the change.
>> >
>> > I do not see how such an argument can be made.  So you have my sympathies but I do not see how we can help you.
>> We should consider this patch basing on my following arguments.
>> 1 Different version kernel should keep consistent on this behavior.
>
> The thing is, the above argument is against the patch. The behavior
> changed 2 years ago, and nobody noticed. Changing it back only causes
> more inconsistent behavior.
It is impossible to cause more inconsistent behavior.
it just enhance compatibility(support "xx...x\0").
This patch just modify "proc_wspace_sep" array. and "proc_wspace_sep" is static.
Only "proc_get_long" used this array, "proc_get_long" is also static.
There are only 4 place to call "proc_get_long" in kernel/sysctl.c.
I will prove that these 4 callers have no bad impact later.

>
>
>> 2 This writting behavior on proc file should be same with writting on
>> regular file as possible as we can.
>
> Writing to a proc file causes kernel actions. Writing to a regular file
> just saves data. That's not an argument here.
>
>> 3 This patch does not have any potential compatibility risk with 3rd
>> party application.
>
> How do you know that?
I will prove that all other write usage is not impacted later.

Thanks for all reply.

Sean
>
> -- Steve
>
>> 4 Support writting "1...\0" to proc file.
>>
>> >
>> > Eric
>> >
>

Re: [PATCH] kernel/sysctl.c: If "count" including the terminating byte '\0' the write system call should retrun success.

[Austin S Hemmelgarn]

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

On 2015-08-25 12:44, Sean Fu wrote:
> On Tue, Aug 25, 2015 at 10:15 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
>> On Tue, 25 Aug 2015 15:50:18 +0800
>> Sean Fu <fxinrong@gmail.com> wrote:
>>
>>> On Tue, Aug 25, 2015 at 10:24 AM, Eric W. Biederman
>>> <ebiederm@xmission.com> wrote:
>>>>
>>>>
>>>> On August 24, 2015 6:57:57 PM MDT, Sean Fu <fxinrong@gmail.com> wrote:
>>>>> An application from HuaWei which works fine on 2.6 encounters this
>>>>> issue on 3.0 or later kernel.
>>>>
>>>> My sympathies.  Being stuck with a 3rd party application you can barely talk about that has been broken for 5years and no one reported it.
>>>>
>>>> Ordinarily we would fix a regression like this. As it has been 5years the challenge now is how do we tell if there are applications that depend on the current behavior.
>>>>
>>>> Before we can change the behavior back we need a convincing argument that we won't cause a regression in another application by making the change.
>>>>
>>>> I do not see how such an argument can be made.  So you have my sympathies but I do not see how we can help you.
>>> We should consider this patch basing on my following arguments.
>>> 1 Different version kernel should keep consistent on this behavior.
>>
>> The thing is, the above argument is against the patch. The behavior
>> changed 2 years ago, and nobody noticed. Changing it back only causes
>> more inconsistent behavior.
> It is impossible to cause more inconsistent behavior.
> it just enhance compatibility(support "xx...x\0").
> This patch just modify "proc_wspace_sep" array. and "proc_wspace_sep" is static.
> Only "proc_get_long" used this array, "proc_get_long" is also static.
> There are only 4 place to call "proc_get_long" in kernel/sysctl.c.
> I will prove that these 4 callers have no bad impact later.
Except that it is userspace we're worrying about here, not stuff 
internal to the kernel.
>
>>
>>
>>> 2 This writting behavior on proc file should be same with writting on
>>> regular file as possible as we can.
>>
>> Writing to a proc file causes kernel actions. Writing to a regular file
>> just saves data. That's not an argument here.
>>
>>> 3 This patch does not have any potential compatibility risk with 3rd
>>> party application.
>>
>> How do you know that?
> I will prove that all other write usage is not impacted later.
Except that you can only really do this for programs that you have 
access to, and by definition you can not have access to every program 
ever written that writes to /proc.

If you were going to do this, it would need to be itself controlled by 
another sysctl to toggle the behavior, which would need to default to 
the current behavior.
>
> Thanks for all reply.
>
> Sean
>>
>> -- Steve
>>
>>> 4 Support writting "1...\0" to proc file.
>>>
>>>>
>>>> Eric
>>>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>



[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 3019 bytes --]

Re: [PATCH] kernel/sysctl.c: If "count" including the terminating byte '\0' the write system call should retrun success.

[Steven Rostedt]

On Tue, 25 Aug 2015 13:33:57 -0400
Austin S Hemmelgarn <ahferroin7@gmail.com> wrote:

> >> How do you know that?
> > I will prove that all other write usage is not impacted later.
> Except that you can only really do this for programs that you have 
> access to, and by definition you can not have access to every program 
> ever written that writes to /proc.
> 
> If you were going to do this, it would need to be itself controlled by 
> another sysctl to toggle the behavior, which would need to default to 
> the current behavior.

Defending the patch, I can't imagine any user space code expecting the
current behavior. The current behavior is that if you write "1\0" it
will error out instead of accepting the "1". I can't come up with a
scenario that would require userspace to expect "1\0" to fail. Can you?


-- Steve

Re: [PATCH] kernel/sysctl.c: If "count" including the terminating byte '\0' the write system call should retrun success.

[Heinrich Schuchardt]

On 24.08.2015 10:56, Sean Fu wrote:
> when the input argument "count" including the terminating byte "\0",
> The write system call return EINVAL on proc file.
> But it return success on regular file.
> 
> E.g. Writting two bytes ("1\0") to "/proc/sys/net/ipv4/conf/eth0/rp_filter".
> write(fd, "1\0", 2) return EINVAL.

Reading through kernel/sysctl.c it looks like you are allowing
"1\01" to be used to pass two integers or two longs.
This is not what you describe as target of your patch.

Parameter tr returned from proc_get_long should be checked in
__do_proc_dointvec,
__do_proc_doulongvec_minmax.

Best regards

Heinrich Schuchardt

> 
> Signed-off-by: Sean Fu <fxinrong@gmail.com>
> ---
>  kernel/sysctl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 19b62b5..c2b0594 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -2004,7 +2004,7 @@ static int do_proc_dointvec_conv(bool *negp,
> unsigned long *lvalp,
>         return 0;
>  }
> 
> -static const char proc_wspace_sep[] = { ' ', '\t', '\n' };
> +static const char proc_wspace_sep[] = { ' ', '\t', '\n', '\0' };
> 
>  static int __do_proc_dointvec(void *tbl_data, struct ctl_table *table,
>                   int write, void __user *buffer,
> 

Re: [PATCH] kernel/sysctl.c: If "count" including the terminating byte '\0' the write system call should retrun success.

[Sean Fu]

On Wed, Aug 26, 2015 at 4:39 AM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
>
> On 24.08.2015 10:56, Sean Fu wrote:
>> when the input argument "count" including the terminating byte "\0",
>> The write system call return EINVAL on proc file.
>> But it return success on regular file.
>>
>> E.g. Writting two bytes ("1\0") to "/proc/sys/net/ipv4/conf/eth0/rp_filter".
>> write(fd, "1\0", 2) return EINVAL.
>
> Reading through kernel/sysctl.c it looks like you are allowing
> "1\01" to be used to pass two integers or two longs.
> This is not what you describe as target of your patch.
1st             2nd             3rd             Change?
'0'~'9'         '\0'               non '\0'        No

proc_get_long-->simple_strtoul-->simple_strtoull-->_parse_integer
__do_proc_dointvec
...
vleft = table->maxlen / sizeof(*i);               //vleft = 1 if it is
integer type proc file
...
for (; left && vleft--; i++, first=0) {              //In last loop
left=2, but vleft = 0 cause exit.

>
> Parameter tr returned from proc_get_long should be checked in
> __do_proc_dointvec,
> __do_proc_doulongvec_minmax.
>
> Best regards
>
> Heinrich Schuchardt
>
>>
>> Signed-off-by: Sean Fu <fxinrong@gmail.com>
>> ---
>>  kernel/sysctl.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
>> index 19b62b5..c2b0594 100644
>> --- a/kernel/sysctl.c
>> +++ b/kernel/sysctl.c
>> @@ -2004,7 +2004,7 @@ static int do_proc_dointvec_conv(bool *negp,
>> unsigned long *lvalp,
>>         return 0;
>>  }
>>
>> -static const char proc_wspace_sep[] = { ' ', '\t', '\n' };
>> +static const char proc_wspace_sep[] = { ' ', '\t', '\n', '\0' };
>>
>>  static int __do_proc_dointvec(void *tbl_data, struct ctl_table *table,
>>                   int write, void __user *buffer,
>>
All possibilities are listed.

1 byte data(count = 1)

1st             Change?
'\0'              NO
non '\0'        NO

2 bytes data(count = 2)

1st                     2nd             Change?
'0'~'9'                  '\0'              Yes
'0'~'9'                  non '\0'        No
non number        '\0'               No
non number        non '\0'         No

3 bytes data(count = 3)

1st             2nd             3rd             Change?
'0'~'9'         '0'~'9'            '\0'             Yes
'0'~'9'         '0'~'9'            non '\0'       No
'0'~'9'         non '0'~'9'      '\0'             No
'0'~'9'         non '0'~'9'      non '\0'       No
'0'~'9'         '\0'                '\0'             No
'0'~'9'         '\0'               non '\0'        No
non '0'~'9'   Any             Any            No

More 3 bytes data(count > 3)
Number sequence         the next character      Change?
"x1...xn"                      '\0'                             Yes
"x1...xn"                      non '\0'                       No
Non "x1...xn"               '\0'                              No
Non "x1...xn"               non '\0'                        No

"x1...xn" is a string whose all members are "0"~'9'
Non "x1...xn" means the first character is not "0"~'9'.

"Yes" means the behavior is changed.
"No" means the behavior is Not changed.

Re: [PATCH] kernel/sysctl.c: If "count" including the terminating byte '\0' the write system call should retrun success.

[Sean Fu]

On Wed, Aug 26, 2015 at 3:05 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Tue, 25 Aug 2015 13:33:57 -0400
> Austin S Hemmelgarn <ahferroin7@gmail.com> wrote:
>
>> >> How do you know that?
>> > I will prove that all other write usage is not impacted later.
>> Except that you can only really do this for programs that you have
>> access to, and by definition you can not have access to every program
>> ever written that writes to /proc.
>>
>> If you were going to do this, it would need to be itself controlled by
>> another sysctl to toggle the behavior, which would need to default to
>> the current behavior.
>
> Defending the patch, I can't imagine any user space code expecting the
> current behavior. The current behavior is that if you write "1\0" it
> will error out instead of accepting the "1". I can't come up with a
> scenario that would require userspace to expect "1\0" to fail. Can you?
Thanks
>
>
> -- Steve

Re: [PATCH] kernel/sysctl.c: If "count" including the terminating byte '\0' the write system call should retrun success.

[Steven Rostedt]

On Wed, 26 Aug 2015 23:48:01 +0800
Sean Fu <fxinrong@gmail.com> wrote:


> > Defending the patch, I can't imagine any user space code expecting the
> > current behavior. The current behavior is that if you write "1\0" it
> > will error out instead of accepting the "1". I can't come up with a
> > scenario that would require userspace to expect "1\0" to fail. Can you?
> Thanks

Although, with the current patch, would "1\02" fail? It should.

-- Steve

Re: [PATCH] kernel/sysctl.c: If "count" including the terminating byte '\0' the write system call should retrun success.

[Sean Fu]

On Thu, Aug 27, 2015 at 4:36 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Wed, 26 Aug 2015 23:48:01 +0800
> Sean Fu <fxinrong@gmail.com> wrote:
>
>
>> > Defending the patch, I can't imagine any user space code expecting the
>> > current behavior. The current behavior is that if you write "1\0" it
>> > will error out instead of accepting the "1". I can't come up with a
>> > scenario that would require userspace to expect "1\0" to fail. Can you?
>> Thanks
>
> Although, with the current patch, would "1\02" fail? It should.
Yes, "1\02" is equal to "1\2"(count=2) or "1\2\0"(count=3), So it should fail.

code
len = write(fd, "1\0\2", 3);

strace execute result:
write(3, "1\2\0", 3)                    = -1 EINVAL (Invalid argument)

>
> -- Steve

Re: [PATCH] kernel/sysctl.c: If "count" including the terminating byte '\0' the write system call should retrun success.

[Sean Fu]

On Wed, Aug 26, 2015 at 4:39 AM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
>
> On 24.08.2015 10:56, Sean Fu wrote:
>> when the input argument "count" including the terminating byte "\0",
>> The write system call return EINVAL on proc file.
>> But it return success on regular file.
>>
>> E.g. Writting two bytes ("1\0") to "/proc/sys/net/ipv4/conf/eth0/rp_filter".
>> write(fd, "1\0", 2) return EINVAL.
>
> Reading through kernel/sysctl.c it looks like you are allowing
> "1\01" to be used to pass two integers or two longs.
> This is not what you describe as target of your patch.
"1\01" actually is "1\1", So either of them will fail.
>
> Parameter tr returned from proc_get_long should be checked in
> __do_proc_dointvec,
> __do_proc_doulongvec_minmax.
>
> Best regards
>
> Heinrich Schuchardt
>
>>
>> Signed-off-by: Sean Fu <fxinrong@gmail.com>
>> ---
>>  kernel/sysctl.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
>> index 19b62b5..c2b0594 100644
>> --- a/kernel/sysctl.c
>> +++ b/kernel/sysctl.c
>> @@ -2004,7 +2004,7 @@ static int do_proc_dointvec_conv(bool *negp,
>> unsigned long *lvalp,
>>         return 0;
>>  }
>>
>> -static const char proc_wspace_sep[] = { ' ', '\t', '\n' };
>> +static const char proc_wspace_sep[] = { ' ', '\t', '\n', '\0' };
>>
>>  static int __do_proc_dointvec(void *tbl_data, struct ctl_table *table,
>>                   int write, void __user *buffer,
>>

Re: [PATCH] kernel/sysctl.c: If "count" including the terminating byte '\0' the write system call should retrun success.

[Steven Rostedt]

On Thu, 27 Aug 2015 08:17:29 +0800
Sean Fu <fxinrong@gmail.com> wrote:

> On Thu, Aug 27, 2015 at 4:36 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> > On Wed, 26 Aug 2015 23:48:01 +0800
> > Sean Fu <fxinrong@gmail.com> wrote:
> >
> >
> >> > Defending the patch, I can't imagine any user space code expecting the
> >> > current behavior. The current behavior is that if you write "1\0" it
> >> > will error out instead of accepting the "1". I can't come up with a
> >> > scenario that would require userspace to expect "1\0" to fail. Can you?
> >> Thanks
> >
> > Although, with the current patch, would "1\02" fail? It should.
> Yes, "1\02" is equal to "1\2"(count=2) or "1\2\0"(count=3), So it should fail.

Sorry, I meant "1\0 2"

-- Steve

> 
> code
> len = write(fd, "1\0\2", 3);
> 
> strace execute result:
> write(3, "1\2\0", 3)                    = -1 EINVAL (Invalid argument)
> 
> >
> > -- Steve

Re: [PATCH] kernel/sysctl.c: If "count" including the terminating byte '\0' the write system call should retrun success.

[Sean Fu]

On Thu, Aug 27, 2015 at 10:32 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Thu, 27 Aug 2015 08:17:29 +0800
> Sean Fu <fxinrong@gmail.com> wrote:
>
>> On Thu, Aug 27, 2015 at 4:36 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
>> > On Wed, 26 Aug 2015 23:48:01 +0800
>> > Sean Fu <fxinrong@gmail.com> wrote:
>> >
>> >
>> >> > Defending the patch, I can't imagine any user space code expecting the
>> >> > current behavior. The current behavior is that if you write "1\0" it
>> >> > will error out instead of accepting the "1". I can't come up with a
>> >> > scenario that would require userspace to expect "1\0" to fail. Can you?
>> >> Thanks
>> >
>> > Although, with the current patch, would "1\02" fail? It should.
>> Yes, "1\02" is equal to "1\2"(count=2) or "1\2\0"(count=3), So it should fail.
>
> Sorry, I meant "1\0 2"
In this case, The patch behavior is accepting the "1" and discarding
other bytes.
for (; left && vleft--; i++, first=0) {    //vleft is 1 for integer
type or unsigned long type proc file

>
> -- Steve
>
>>
>> code
>> len = write(fd, "1\0\2", 3);
>>
>> strace execute result:
>> write(3, "1\2\0", 3)                    = -1 EINVAL (Invalid argument)
>>
>> >
>> > -- Steve
>

Re: [PATCH] kernel/sysctl.c: If "count" including the terminating byte '\0' the write system call should retrun success.

[Sean Fu]

On Thu, Aug 27, 2015 at 4:32 PM, Sean Fu <fxinrong@gmail.com> wrote:
> On Thu, Aug 27, 2015 at 10:32 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
>> On Thu, 27 Aug 2015 08:17:29 +0800
>> Sean Fu <fxinrong@gmail.com> wrote:
>>
>>> On Thu, Aug 27, 2015 at 4:36 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
>>> > On Wed, 26 Aug 2015 23:48:01 +0800
>>> > Sean Fu <fxinrong@gmail.com> wrote:
>>> >
>>> >
>>> >> > Defending the patch, I can't imagine any user space code expecting the
>>> >> > current behavior. The current behavior is that if you write "1\0" it
>>> >> > will error out instead of accepting the "1". I can't come up with a
>>> >> > scenario that would require userspace to expect "1\0" to fail. Can you?
>>> >> Thanks
>>> >
>>> > Although, with the current patch, would "1\02" fail? It should.
>>> Yes, "1\02" is equal to "1\2"(count=2) or "1\2\0"(count=3), So it should fail.
>>
>> Sorry, I meant "1\0 2"
> In this case, The patch behavior is accepting the "1" and discarding
> other bytes.
> for (; left && vleft--; i++, first=0) {    //vleft is 1 for integer
> type or unsigned long type proc file
>
>>
>> -- Steve
>>
>>>
>>> code
>>> len = write(fd, "1\0\2", 3);
>>>
>>> strace execute result:
>>> write(3, "1\2\0", 3)                    = -1 EINVAL (Invalid argument)
If vleft > 1, "1\0 2" is treated as invalid paraments and all string
include '\0' will be invalid.

>>>
>>> >
>>> > -- Steve
>>

Re: [PATCH] kernel/sysctl.c: If "count" including the terminating byte '\0' the write system call should retrun success.

[Sean Fu]

On Fri, Aug 28, 2015 at 11:31 AM, Sean Fu <fxinrong@gmail.com> wrote:
> On Thu, Aug 27, 2015 at 4:32 PM, Sean Fu <fxinrong@gmail.com> wrote:
>> On Thu, Aug 27, 2015 at 10:32 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
>>> On Thu, 27 Aug 2015 08:17:29 +0800
>>> Sean Fu <fxinrong@gmail.com> wrote:
>>>> strace execute result:
>>>> write(3, "1\2\0", 3)                    = -1 EINVAL (Invalid argument)
> If vleft > 1, "1\0 2" is treated as invalid paraments and all string
> include '\0' will be invalid.
Hi All experts,
Could you please signed off this patch?
>
>>>>
>>>> >
>>>> > -- Steve
>>>

Re: [PATCH] kernel/sysctl.c: If "count" including the terminating byte '\0' the write system call should retrun success.

[Steven Rostedt]

On Tue, 8 Sep 2015 11:11:38 +0800
Sean Fu <fxinrong@gmail.com> wrote:

> On Fri, Aug 28, 2015 at 11:31 AM, Sean Fu <fxinrong@gmail.com> wrote:
> > On Thu, Aug 27, 2015 at 4:32 PM, Sean Fu <fxinrong@gmail.com> wrote:
> >> On Thu, Aug 27, 2015 at 10:32 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> >>> On Thu, 27 Aug 2015 08:17:29 +0800
> >>> Sean Fu <fxinrong@gmail.com> wrote:
> >>>> strace execute result:
> >>>> write(3, "1\2\0", 3)                    = -1 EINVAL (Invalid argument)
> > If vleft > 1, "1\0 2" is treated as invalid paraments and all string
> > include '\0' will be invalid.
> Hi All experts,
> Could you please signed off this patch?

If anyone should take this, it would be Andrew.

I have no issue with the patch. Eric, you had some issue, but I don't
see a scenario that would depend on the current behavior. That is, what
do you think would break if we put it back to the old behavior?

-- Steve

Re: [PATCH] kernel/sysctl.c: If "count" including the terminating byte '\0' the write system call should retrun success.

[Eric W. Biederman]

Steven Rostedt <rostedt@goodmis.org> writes:

> On Tue, 8 Sep 2015 11:11:38 +0800
> Sean Fu <fxinrong@gmail.com> wrote:
>
>> On Fri, Aug 28, 2015 at 11:31 AM, Sean Fu <fxinrong@gmail.com> wrote:
>> > On Thu, Aug 27, 2015 at 4:32 PM, Sean Fu <fxinrong@gmail.com> wrote:
>> >> On Thu, Aug 27, 2015 at 10:32 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
>> >>> On Thu, 27 Aug 2015 08:17:29 +0800
>> >>> Sean Fu <fxinrong@gmail.com> wrote:
>> >>>> strace execute result:
>> >>>> write(3, "1\2\0", 3)                    = -1 EINVAL (Invalid argument)
>> > If vleft > 1, "1\0 2" is treated as invalid paraments and all string
>> > include '\0' will be invalid.
>> Hi All experts,
>> Could you please signed off this patch?
>
> If anyone should take this, it would be Andrew.
>
> I have no issue with the patch. Eric, you had some issue, but I don't
> see a scenario that would depend on the current behavior. That is, what
> do you think would break if we put it back to the old behavior?

This patch does not implement the old behavior.

The old code does use '\0' as a buffer terminator, and because it does
not check things closely I can see how it could accept a '\0' from
userspace and treat that as an early buffer terminator.

The patch treats '\0' as a number separator and allows things that have
never been allowed before and quite frankly is very scary as it just
invites bugs.

So I do not think we should merge the given patch.  It is just wrong.
One that simply truncates the input buffer at the first '\0' character I
think we can consider, although I am not a fan.

Steve as far as what I think would break.  I don't think the current
behavior should have broken anything and apparently it did.  I don't see
what a change that simply truncates the buffer at the first embedded
'\0' would break, but I don't know how to test that there isn't anything
that it will.  We are way past the point of reasonable expectations
being able to guide us.  4 years should have been more than enough soak
time to have been able to say that the change was good, but apparently
it was not.

My gut feel says that if we are going to change this, at this late date,
we find the one specific proc file that matters and change it just for
that one proc file, and in that change we treat '\0' as a terminator not
as a separator.  I never did see in the conversation which proc file it
is that actually matters.  The principle is that the more precise and
the more localized such a change is the less chance it has of causing a
regression of something else, and the greater the chance we can look at
a specific issue.

Eric

Re: [PATCH] kernel/sysctl.c: If "count" including the terminating byte '\0' the write system call should retrun success.

[Steven Rostedt]

On Tue, 08 Sep 2015 11:19:14 -0500
ebiederm@xmission.com (Eric W. Biederman) wrote:


> This patch does not implement the old behavior.
> 
> The old code does use '\0' as a buffer terminator, and because it does
> not check things closely I can see how it could accept a '\0' from
> userspace and treat that as an early buffer terminator.
> 
> The patch treats '\0' as a number separator and allows things that have
> never been allowed before and quite frankly is very scary as it just
> invites bugs.
> 
> So I do not think we should merge the given patch.  It is just wrong.
> One that simply truncates the input buffer at the first '\0' character I
> think we can consider, although I am not a fan.

I agree, and was thinking this patch did that, but I didn't look close
enough (why I never gave a Reviewed-by to it either).


> 
> Steve as far as what I think would break.  I don't think the current
> behavior should have broken anything and apparently it did.  I don't see
> what a change that simply truncates the buffer at the first embedded
> '\0' would break, but I don't know how to test that there isn't anything
> that it will.  We are way past the point of reasonable expectations
> being able to guide us.  4 years should have been more than enough soak
> time to have been able to say that the change was good, but apparently
> it was not.

Well, to be fair, a lot of people (distros, etc) do not use the most
recent kernels. 4 years may be the first time a tool touches a kernel.

> 
> My gut feel says that if we are going to change this, at this late date,
> we find the one specific proc file that matters and change it just for
> that one proc file, and in that change we treat '\0' as a terminator not
> as a separator.  I never did see in the conversation which proc file it
> is that actually matters.  The principle is that the more precise and
> the more localized such a change is the less chance it has of causing a
> regression of something else, and the greater the chance we can look at
> a specific issue.

Sounds like a reasonable compromise. Sean, can you make a patch that
only affects the one proc file (comment it well in the code), and have
it accept nothing past the '\0'. Even if someone passed in "1 \0 2", it
would only see "1 "

-- Steve

Re: [PATCH] kernel/sysctl.c: If "count" including the terminating byte '\0' the write system call should retrun success.

[Sean Fu]

On Wed, Sep 9, 2015 at 12:36 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Tue, 08 Sep 2015 11:19:14 -0500
> ebiederm@xmission.com (Eric W. Biederman) wrote:
>
>
>> This patch does not implement the old behavior.
>>
>> The old code does use '\0' as a buffer terminator, and because it does
>> not check things closely I can see how it could accept a '\0' from
>> userspace and treat that as an early buffer terminator.
>>
>> The patch treats '\0' as a number separator and allows things that have
>> never been allowed before and quite frankly is very scary as it just
>> invites bugs.
>>
>> So I do not think we should merge the given patch.  It is just wrong.
>> One that simply truncates the input buffer at the first '\0' character I
>> think we can consider, although I am not a fan.
>
> I agree, and was thinking this patch did that, but I didn't look close
> enough (why I never gave a Reviewed-by to it either).
>
>
>>
>> Steve as far as what I think would break.  I don't think the current
>> behavior should have broken anything and apparently it did.  I don't see
>> what a change that simply truncates the buffer at the first embedded
>> '\0' would break, but I don't know how to test that there isn't anything
>> that it will.  We are way past the point of reasonable expectations
>> being able to guide us.  4 years should have been more than enough soak
>> time to have been able to say that the change was good, but apparently
>> it was not.
>
> Well, to be fair, a lot of people (distros, etc) do not use the most
> recent kernels. 4 years may be the first time a tool touches a kernel.
>
>>
>> My gut feel says that if we are going to change this, at this late date,
>> we find the one specific proc file that matters and change it just for
>> that one proc file, and in that change we treat '\0' as a terminator not
>> as a separator.  I never did see in the conversation which proc file it
>> is that actually matters.  The principle is that the more precise and
>> the more localized such a change is the less chance it has of causing a
>> regression of something else, and the greater the chance we can look at
>> a specific issue.
>
> Sounds like a reasonable compromise. Sean, can you make a patch that
> only affects the one proc file (comment it well in the code), and have
> it accept nothing past the '\0'. Even if someone passed in "1 \0 2", it
> would only see "1 "
The current code uses uniform handler (e.g. "proc_dointvec") for all
same type proc file.
So all integer type proc file are affected.
In fact, The behavior of all integer type proc file should be changed.
>
> -- Steve

Re: [PATCH] kernel/sysctl.c: If "count" including the terminating byte '\0' the write system call should retrun success.

[Steven Rostedt]

On Fri, 11 Sep 2015 17:05:31 +0800
Sean Fu <fxinrong@gmail.com> wrote:


> > Sounds like a reasonable compromise. Sean, can you make a patch that
> > only affects the one proc file (comment it well in the code), and have
> > it accept nothing past the '\0'. Even if someone passed in "1 \0 2", it
> > would only see "1 "

> The current code uses uniform handler (e.g. "proc_dointvec") for all
> same type proc file.
> So all integer type proc file are affected.
> In fact, The behavior of all integer type proc file should be changed.


Then at least make it where the \0 is the terminating string. Nothing
after it will be seen by the rest of the code in the kernel.

-- Steve

Re: [PATCH] kernel/sysctl.c: If "count" including the terminating byte '\0' the write system call should retrun success.

[Eric W. Biederman]

Sean Fu <fxinrong@gmail.com> writes:

>> Sounds like a reasonable compromise. Sean, can you make a patch that
>> only affects the one proc file (comment it well in the code), and have
>> it accept nothing past the '\0'. Even if someone passed in "1 \0 2", it
>> would only see "1 "
> The current code uses uniform handler (e.g. "proc_dointvec") for all
> same type proc file.
> So all integer type proc file are affected.

No.  I do not believe the proprietary binary application you are dealing
with writes to all proc files that use the proc_dointvec handler.

> In fact, The behavior of all integer type proc file should be changed.

Not at all.  The only files that we can possibly justify changing today
are the files where an actual regression is being observed.

Because quite frankly 5 years is way too long to wait to report a
regression.  By and large software is reasonable and treats proc
files as text files where '\0' is an invalid character.

Accepting a '\0' is not at all reasonable for a text interface.  The
application that does it is buggy.

Eric

Re: [PATCH] kernel/sysctl.c: If "count" including the terminating byte '\0' the write system call should retrun success.

[Sean Fu]

On Sat, Sep 12, 2015 at 1:01 AM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
> Sean Fu <fxinrong@gmail.com> writes:
>
>>> Sounds like a reasonable compromise. Sean, can you make a patch that
>>> only affects the one proc file (comment it well in the code), and have
>>> it accept nothing past the '\0'. Even if someone passed in "1 \0 2", it
>>> would only see "1 "
>> The current code uses uniform handler (e.g. "proc_dointvec") for all
>> same type proc file.
>> So all integer type proc file are affected.
>
> No.  I do not believe the proprietary binary application you are dealing
> with writes to all proc files that use the proc_dointvec handler.
I means all ctl_table whose .proc_handler is "proc_dointvec" are affected.
>
>> In fact, The behavior of all integer type proc file should be changed.
>
> Not at all.  The only files that we can possibly justify changing today
> are the files where an actual regression is being observed.
>
> Because quite frankly 5 years is way too long to wait to report a
> regression.  By and large software is reasonable and treats proc
> files as text files where '\0' is an invalid character.
5 years is not enough long for distros, specially enterprise distros.
The most of HuaWei machines run our SLES10sp3(2.6.16, SUSE LINUX
ENTERPRISE SERVER).
They use one enterprise version for 5+ years usually.
>
> Accepting a '\0' is not at all reasonable for a text interface.  The
> application that does it is buggy.
It is hard to comprehend that the current kernel can accept  two bytes
"1 ", "1\t", "1\n" except "1\0".
>
> Eric

Re: [PATCH] kernel/sysctl.c: If "count" including the terminating byte '\0' the write system call should retrun success.

[Eric W. Biederman]

Sean Fu <fxinrong@gmail.com> writes:

> On Sat, Sep 12, 2015 at 1:01 AM, Eric W. Biederman
> <ebiederm@xmission.com> wrote:
>> Sean Fu <fxinrong@gmail.com> writes:
>>
>>>> Sounds like a reasonable compromise. Sean, can you make a patch that
>>>> only affects the one proc file (comment it well in the code), and have
>>>> it accept nothing past the '\0'. Even if someone passed in "1 \0 2", it
>>>> would only see "1 "
>>> The current code uses uniform handler (e.g. "proc_dointvec") for all
>>> same type proc file.
>>> So all integer type proc file are affected.
>>
>> No.  I do not believe the proprietary binary application you are dealing
>> with writes to all proc files that use the proc_dointvec handler.
> I means all ctl_table whose .proc_handler is "proc_dointvec" are
> affected.

I mean this only deserves consideration because this is a regression
report.

I mean by limiting this to only the proc files that are written to by
the weird program that broke we can minimize the chances that anything
else will break.

I do not believe that the weird program that broke writes to every proc
file.  Certainly I have not heard that asserted.

>>> In fact, The behavior of all integer type proc file should be changed.
>>
>> Not at all.  The only files that we can possibly justify changing today
>> are the files where an actual regression is being observed.
>>
>> Because quite frankly 5 years is way too long to wait to report a
>> regression.  By and large software is reasonable and treats proc
>> files as text files where '\0' is an invalid character.
> 5 years is not enough long for distros, specially enterprise distros.
> The most of HuaWei machines run our SLES10sp3(2.6.16, SUSE LINUX
> ENTERPRISE SERVER).
> They use one enterprise version for 5+ years usually.

If you want to play by enterprise kernel rules please talk to your
enterprise kernel support people.

>> Accepting a '\0' is not at all reasonable for a text interface.  The
>> application that does it is buggy.
> It is hard to comprehend that the current kernel can accept  two bytes
> "1 ", "1\t", "1\n" except "1\0".

'\0' is not and has never been valid in a text file.
proc files are a text interface.
Expecting '\0' to be accepted is very strange, and apparently there is
only one program in existence that does.

That a trailing '\0' was ever accepted was due to a bug in the code.

Accepting '\0' in general in a text interface is a very dangerous and
buggy pattern so it must be done very carefully or else other
regressions or bugs could be easily introduced.

That no one has complained about this in the 5 years since the change happened
strongly indicates this no one else cares.

A very targeted very narrow regression fix that only handles a trailing
'\0' and that only changes the behavior of proc files that matter is
reasonable.

Or do you volunteer to go out and test every program that has been
written or updated to write to proc in the last 5 years (since the
behavior changed) and verify that none of them in no circumstances
depend upon failing if an trailing '\0' is included?   If you can audit
all of the code written in the last 5 years and verify that the change
will not introduce problems for any other user space program we can talk
about changing all of the proc files that use proc_dointvec.

Eric

Re: [PATCH] kernel/sysctl.c: If "count" including the terminating byte '\0' the write system call should retrun success.

[Steven Rostedt]

On Sun, 13 Sep 2015 20:39:31 +0800
Sean Fu <fxinrong@gmail.com> wrote:


> > Accepting a '\0' is not at all reasonable for a text interface.  The
> > application that does it is buggy.
> It is hard to comprehend that the current kernel can accept  two bytes
> "1 ", "1\t", "1\n" except "1\0".

Um, it does not seem hard to comprehend at all. As this is a string,
and it acts the same as a printf() or strlen.

	strlen("1 ") == 2
	strlen("1\t") == 2
	strlen("1\n") == 2
	strlen("1\0") == 1

Big difference to me.

-- Steve

Re: [PATCH] kernel/sysctl.c: If "count" including the terminating byte '\0' the write system call should retrun success.

[Sean Fu]

According to POSIX standard,  "The write() function shall attempt to
write nbyte bytes from the buffer pointed to by buf to the file
associated with the open file descriptor, fildes.".
So it is not the length of string(strlen).

On Mon, Sep 14, 2015 at 4:05 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Sun, 13 Sep 2015 20:39:31 +0800
> Sean Fu <fxinrong@gmail.com> wrote:
>
>
>> > Accepting a '\0' is not at all reasonable for a text interface.  The
>> > application that does it is buggy.
>> It is hard to comprehend that the current kernel can accept  two bytes
>> "1 ", "1\t", "1\n" except "1\0".
>
> Um, it does not seem hard to comprehend at all. As this is a string,
> and it acts the same as a printf() or strlen.
>
>         strlen("1 ") == 2
>         strlen("1\t") == 2
>         strlen("1\n") == 2
>         strlen("1\0") == 1
>
> Big difference to me.
>
> -- Steve
>

Re: [PATCH] kernel/sysctl.c: If "count" including the terminating byte '\0' the write system call should retrun success.

[Sean Fu]

On Mon, Sep 14, 2015 at 12:44 AM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
> Sean Fu <fxinrong@gmail.com> writes:
>
>> On Sat, Sep 12, 2015 at 1:01 AM, Eric W. Biederman
>> <ebiederm@xmission.com> wrote:
>>> Sean Fu <fxinrong@gmail.com> writes:
> '\0' is not and has never been valid in a text file.
> proc files are a text interface.
> Expecting '\0' to be accepted is very strange, and apparently there is
> only one program in existence that does.
>
> That a trailing '\0' was ever accepted was due to a bug in the code.
>
> Accepting '\0' in general in a text interface is a very dangerous and
> buggy pattern so it must be done very carefully or else other
> regressions or bugs could be easily introduced.
Ok,
Could you please give me more detail about the potential risk from the patch?
What is the different behavior between the patch and old kernel?
It seems like entirely same.

> Eric

Re: [PATCH] kernel/sysctl.c: If "count" including the terminating byte '\0' the write system call should retrun success.

[Steven Rostedt]

On Tue, 15 Sep 2015 17:10:11 +0800
Sean Fu <fxinrong@gmail.com> wrote:

> According to POSIX standard,  "The write() function shall attempt to
> write nbyte bytes from the buffer pointed to by buf to the file
> associated with the open file descriptor, fildes.".
> So it is not the length of string(strlen).

Why do we care about the "write()" function? Yeah, so be it. Send
out as much data as you want. We care about the effects of the proc file
system by the input it receives. You can send in X amounts of data.
That's not broken. The only apps that would send out a string to the
proc filesystem that includes "\0" and beyond is root kits. I'm sorry
but your arguments are starting to go south. I'm starting to think that
the app that broke is a root kit, and maybe it's best to just Nack this.

-- Steve


> 
> On Mon, Sep 14, 2015 at 4:05 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> > On Sun, 13 Sep 2015 20:39:31 +0800
> > Sean Fu <fxinrong@gmail.com> wrote:
> >
> >
> >> > Accepting a '\0' is not at all reasonable for a text interface.  The
> >> > application that does it is buggy.
> >> It is hard to comprehend that the current kernel can accept  two bytes
> >> "1 ", "1\t", "1\n" except "1\0".
> >
> > Um, it does not seem hard to comprehend at all. As this is a string,
> > and it acts the same as a printf() or strlen.
> >
> >         strlen("1 ") == 2
> >         strlen("1\t") == 2
> >         strlen("1\n") == 2
> >         strlen("1\0") == 1
> >
> > Big difference to me.
> >
> > -- Steve
> >

Re: [PATCH] kernel/sysctl.c: If "count" including the terminating byte '\0' the write system call should retrun success.

[Eric W. Biederman]

On September 15, 2015 4:30:56 AM CDT, Sean Fu <fxinrong@gmail.com> wrote:
>On Mon, Sep 14, 2015 at 12:44 AM, Eric W. Biederman
><ebiederm@xmission.com> wrote:
>> Sean Fu <fxinrong@gmail.com> writes:
>>
>>> On Sat, Sep 12, 2015 at 1:01 AM, Eric W. Biederman
>>> <ebiederm@xmission.com> wrote:
>>>> Sean Fu <fxinrong@gmail.com> writes:
>> '\0' is not and has never been valid in a text file.
>> proc files are a text interface.
>> Expecting '\0' to be accepted is very strange, and apparently there
>is
>> only one program in existence that does.
>>
>> That a trailing '\0' was ever accepted was due to a bug in the code.
>>
>> Accepting '\0' in general in a text interface is a very dangerous and
>> buggy pattern so it must be done very carefully or else other
>> regressions or bugs could be easily introduced.
>Ok,
>Could you please give me more detail about the potential risk from the
>patch?

Regressions.  AKA There may now be programs that depend on writing a  '\0' failing.

>What is the different behavior between the patch and old kernel?
>It seems like entirely same.

Not at all.  And clear explanations have already been given.

Eric