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 <[email protected]>
---
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
On August 24, 2015 1:56:13 AM PDT, Sean Fu <[email protected]> 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 <[email protected]>
>---
> 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,
On Mon, Aug 24, 2015 at 8:27 PM, Eric W. Biederman
<[email protected]> wrote:
>
>
> On August 24, 2015 1:56:13 AM PDT, Sean Fu <[email protected]> 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 <[email protected]>
>>---
>> 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,
>
On Mon, 24 Aug 2015 16:56:13 +0800
Sean Fu <[email protected]> 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
On Mon, 24 Aug 2015 23:33:58 +0800 Sean Fu <[email protected]> wrote:
> On Mon, Aug 24, 2015 at 8:27 PM, Eric W. Biederman
> <[email protected]> wrote:
> >
> >
> > On August 24, 2015 1:56:13 AM PDT, Sean Fu <[email protected]> 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.
This seems to be the relevant patch:
https://lkml.org/lkml/2010/5/5/104
Amerigo Wang <[email protected]> 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 <[email protected]> wrote:
>
>> On Mon, Aug 24, 2015 at 8:27 PM, Eric W. Biederman
>> <[email protected]> wrote:
>>>
>>>
>>> On August 24, 2015 1:56:13 AM PDT, Sean Fu <[email protected]> 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.
>
>
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 <[email protected]> wrote:
> On Mon, 24 Aug 2015 16:56:13 +0800
> Sean Fu <[email protected]> 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
On August 24, 2015 6:57:57 PM MDT, Sean Fu <[email protected]> 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
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 <[email protected]> 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 <[email protected]> wrote:
>> On Mon, 24 Aug 2015 16:56:13 +0800
>> Sean Fu <[email protected]> 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
On Tue, Aug 25, 2015 at 10:24 AM, Eric W. Biederman
<[email protected]> wrote:
>
>
> On August 24, 2015 6:57:57 PM MDT, Sean Fu <[email protected]> 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
>
On Tue, 25 Aug 2015 15:50:18 +0800
Sean Fu <[email protected]> wrote:
> On Tue, Aug 25, 2015 at 10:24 AM, Eric W. Biederman
> <[email protected]> wrote:
> >
> >
> > On August 24, 2015 6:57:57 PM MDT, Sean Fu <[email protected]> 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
> >
On Tue, Aug 25, 2015 at 10:15 PM, Steven Rostedt <[email protected]> wrote:
> On Tue, 25 Aug 2015 15:50:18 +0800
> Sean Fu <[email protected]> wrote:
>
>> On Tue, Aug 25, 2015 at 10:24 AM, Eric W. Biederman
>> <[email protected]> wrote:
>> >
>> >
>> > On August 24, 2015 6:57:57 PM MDT, Sean Fu <[email protected]> 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
>> >
>
On 2015-08-25 12:44, Sean Fu wrote:
> On Tue, Aug 25, 2015 at 10:15 PM, Steven Rostedt <[email protected]> wrote:
>> On Tue, 25 Aug 2015 15:50:18 +0800
>> Sean Fu <[email protected]> wrote:
>>
>>> On Tue, Aug 25, 2015 at 10:24 AM, Eric W. Biederman
>>> <[email protected]> wrote:
>>>>
>>>>
>>>> On August 24, 2015 6:57:57 PM MDT, Sean Fu <[email protected]> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
On Tue, 25 Aug 2015 13:33:57 -0400
Austin S Hemmelgarn <[email protected]> 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
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 <[email protected]>
> ---
> 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,
>
On Wed, Aug 26, 2015 at 4:39 AM, Heinrich Schuchardt <[email protected]> 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 <[email protected]>
>> ---
>> 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.
On Wed, Aug 26, 2015 at 3:05 AM, Steven Rostedt <[email protected]> wrote:
> On Tue, 25 Aug 2015 13:33:57 -0400
> Austin S Hemmelgarn <[email protected]> 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
On Wed, 26 Aug 2015 23:48:01 +0800
Sean Fu <[email protected]> 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
On Thu, Aug 27, 2015 at 4:36 AM, Steven Rostedt <[email protected]> wrote:
> On Wed, 26 Aug 2015 23:48:01 +0800
> Sean Fu <[email protected]> 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
On Wed, Aug 26, 2015 at 4:39 AM, Heinrich Schuchardt <[email protected]> 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 <[email protected]>
>> ---
>> 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,
>>
On Thu, 27 Aug 2015 08:17:29 +0800
Sean Fu <[email protected]> wrote:
> On Thu, Aug 27, 2015 at 4:36 AM, Steven Rostedt <[email protected]> wrote:
> > On Wed, 26 Aug 2015 23:48:01 +0800
> > Sean Fu <[email protected]> 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
On Thu, Aug 27, 2015 at 10:32 AM, Steven Rostedt <[email protected]> wrote:
> On Thu, 27 Aug 2015 08:17:29 +0800
> Sean Fu <[email protected]> wrote:
>
>> On Thu, Aug 27, 2015 at 4:36 AM, Steven Rostedt <[email protected]> wrote:
>> > On Wed, 26 Aug 2015 23:48:01 +0800
>> > Sean Fu <[email protected]> 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
>
On Thu, Aug 27, 2015 at 4:32 PM, Sean Fu <[email protected]> wrote:
> On Thu, Aug 27, 2015 at 10:32 AM, Steven Rostedt <[email protected]> wrote:
>> On Thu, 27 Aug 2015 08:17:29 +0800
>> Sean Fu <[email protected]> wrote:
>>
>>> On Thu, Aug 27, 2015 at 4:36 AM, Steven Rostedt <[email protected]> wrote:
>>> > On Wed, 26 Aug 2015 23:48:01 +0800
>>> > Sean Fu <[email protected]> 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
>>