Error returns not handled correctly by sysfs.c:subsys_attr_store()

Error returns not handled correctly by sysfs.c:subsys_attr_store()

[Andrew Patterson]

The buf in fs/sysfs.c:subsys_attr_store() does not seem to be updated
correctly when returning a negative value (indicating that an error
condition has occurred) is returned.  If a negative value is returned,
the next subsequent call to subsys_attr_store will have the contents of
buf appended to the previous call.  Example: I have modified the
sd.c:sd_store_allow_restart to always print out the contents of the buf
and return an error using the following patch:

--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -183,6 +183,9 @@ static ssize_t sd_store_allow_restart(struct class_device *c
        struct scsi_disk *sdkp = to_scsi_disk(cdev);
        struct scsi_device *sdp = sdkp->device;
 
+       printk(KERN_ERR "buf_ptr = 0x%p, buf = %s, count = %u\n", buf, buf, coun
+       return -EINVAL;
+
        if (!capable(CAP_SYS_ADMIN))
                return -EACCES;

I get the following output when writing invalid values to the
allow_restart sysfs file:

# echo x > /sys/class/scsi_disk/4:0:0:0/allow_restart
bash: echo: write error: Invalid argument
# echo y > /sys/class/scsi_disk/4:0:0:0/allow_restart
bash: echo: write error: Invalid argument
# echo z > /sys/class/scsi_disk/4:0:0:0/allow_restart
bash: echo: write error: Invalid argument

And the console output shows:

buf_ptr = 0xe00001004bdb0000, buf = x
, count = 2
buf_ptr = 0xe00001004bdb0000, buf = x
, count = 2
buf_ptr = 0xe00001004bdb0000, buf = x
y
, count = 4
buf_ptr = 0xe00001004bdb0000, buf = x
y
, count = 4
buf_ptr = 0xe00001004caf0000, buf = x
y
z
, count = 6
buf_ptr = 0xe00001004caf0000, buf = x
y
z
, count = 6

and the same append problem occurs when using another sysfs file:

# echo xyzzy > /sys/class/scsi_disk/4:0:1:0/allow_restart
bash: echo: write error: Invalid argument

buf_ptr = 0xe00001004caf0000, buf = x
y
z
xyzzy
, count = 12

I found this problem in 2.6.24-rc3 and and an earlier version of 2.6.23.

This seems to work correctly on 2.6.18 (at least with the RHEL5 kernel I
did some testing with), i.e. the contents of buf from the previous
failed called are thrown away/overwritten. I looked through sysfs.c to
see if I could find anything obvious but could not see anything.
Perhaps this is handled at a higher level. 

-- 
Andrew Patterson
Hewlett-Packard

Re: Error returns not handled correctly by sysfs.c:subsys_attr_store()

[Andrew Morton]

On Wed, 21 Nov 2007 15:16:59 -0700 Andrew Patterson <andrew.patterson@hp.com> wrote:

> The buf in fs/sysfs.c:subsys_attr_store() does not seem to be updated
> correctly when returning a negative value (indicating that an error
> condition has occurred) is returned.  If a negative value is returned,
> the next subsequent call to subsys_attr_store will have the contents of
> buf appended to the previous call.

subsys_attr_store() gets deleted by
http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/gregkh-01-driver/kset-kill-subsys-attr.patch

So maybe we will soon accidentally fix whatever-this-is?  Or maybe we will
faithfully maintain it.

Re: Error returns not handled correctly by sysfs.c:subsys_attr_store()

[Greg KH]

On Mon, Nov 26, 2007 at 08:31:16PM -0800, Andrew Morton wrote:
> On Wed, 21 Nov 2007 15:16:59 -0700 Andrew Patterson <andrew.patterson@hp.com> wrote:
> 
> > The buf in fs/sysfs.c:subsys_attr_store() does not seem to be updated
> > correctly when returning a negative value (indicating that an error
> > condition has occurred) is returned.  If a negative value is returned,
> > the next subsequent call to subsys_attr_store will have the contents of
> > buf appended to the previous call.
> 
> subsys_attr_store() gets deleted by
> http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/gregkh-01-driver/kset-kill-subsys-attr.patch
> 
> So maybe we will soon accidentally fix whatever-this-is?  Or maybe we will
> faithfully maintain it.

Yes, subsys attributes go away, but this is showing a bug in the sysfs
core with attributes, not in the "middle" layers of attributes.

I bounced the original bug report to Tejun, who has been changing the
logic around this area to see if he sees anything that might be
different now.

Tejun?

thanks,

greg k-h

Re: Error returns not handled correctly by sysfs.c:subsys_attr_store()

[Tejun Heo]

Greg KH wrote:
> On Mon, Nov 26, 2007 at 08:31:16PM -0800, Andrew Morton wrote:
>> On Wed, 21 Nov 2007 15:16:59 -0700 Andrew Patterson <andrew.patterson@hp.com> wrote:
>>
>>> The buf in fs/sysfs.c:subsys_attr_store() does not seem to be updated
>>> correctly when returning a negative value (indicating that an error
>>> condition has occurred) is returned.  If a negative value is returned,
>>> the next subsequent call to subsys_attr_store will have the contents of
>>> buf appended to the previous call.
>> subsys_attr_store() gets deleted by
>> http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/gregkh-01-driver/kset-kill-subsys-attr.patch
>>
>> So maybe we will soon accidentally fix whatever-this-is?  Or maybe we will
>> faithfully maintain it.
> 
> Yes, subsys attributes go away, but this is showing a bug in the sysfs
> core with attributes, not in the "middle" layers of attributes.
> 
> I bounced the original bug report to Tejun, who has been changing the
> logic around this area to see if he sees anything that might be
> different now.
> 
> Tejun?

(groaning buried under ATA bugs) Will take a look soon.

Thanks.

-- 
tejun

Re: Error returns not handled correctly by sysfs.c:subsys_attr_store()

[Tejun Heo]

Greg KH wrote:
> On Mon, Nov 26, 2007 at 08:31:16PM -0800, Andrew Morton wrote:
>> On Wed, 21 Nov 2007 15:16:59 -0700 Andrew Patterson <andrew.patterson@hp.com> wrote:
>>
>>> The buf in fs/sysfs.c:subsys_attr_store() does not seem to be updated
>>> correctly when returning a negative value (indicating that an error
>>> condition has occurred) is returned.  If a negative value is returned,
>>> the next subsequent call to subsys_attr_store will have the contents of
>>> buf appended to the previous call.
>> subsys_attr_store() gets deleted by
>> http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/gregkh-01-driver/kset-kill-subsys-attr.patch
>>
>> So maybe we will soon accidentally fix whatever-this-is?  Or maybe we will
>> faithfully maintain it.
> 
> Yes, subsys attributes go away, but this is showing a bug in the sysfs
> core with attributes, not in the "middle" layers of attributes.
> 
> I bounced the original bug report to Tejun, who has been changing the
> logic around this area to see if he sees anything that might be
> different now.
> 
> Tejun?

Weird, the problem is not reproducible here.

# echo a > allow_restart
-bash: echo: write error: Invalid argument
[  437.518024] buf_ptr = 0xffff810005e20000, buf = x
[  437.518027] , count = 2
# echo b > allow_restart
-bash: echo: write error: Invalid argument
[  438.972973] buf_ptr = 0xffff81001be6f000, buf = y
[  438.972976] , count = 2
# echo c > allow_restart
-bash: echo: write error: Invalid argument
[  440.539747] buf_ptr = 0xffff81001d4ba000, buf = z
[  440.539750] , count = 2

Which is expected.  On each open, sysfs_buffer is allocated with kzalloc
and the buffer is freed on close, so I don't see how it can happen.
Behavior for multiple write can be considered peculiar in that ppos is
essentially ignored and each write is passed just like brand new write
to ->store method but this too is the expected behavior.

# (echo a; echo b; echo c) > allow_restart
[  765.257132] buf_ptr = 0xffff81001be4f000, buf = a
[  765.257135] , count = 2
[  765.285474] buf_ptr = 0xffff81001be4f000, buf = b
[  765.285484] , count = 2
[  765.314002] buf_ptr = 0xffff81001be4f000, buf = c
[  765.314004] , count = 2
-bash: echo: write error: Invalid argument
-bash: echo: write error: Invalid argument
-bash: echo: write error: Invalid argument

Andrew Petterson, can you please build 2.6.24-rc3 from clean source tree
and retry?

-- 
tejun

Re: Error returns not handled correctly by sysfs.c:subsys_attr_store()

[Andrew Patterson]

On Wed, 2007-11-28 at 16:42 +0900, Tejun Heo wrote:
> Greg KH wrote:
> > On Mon, Nov 26, 2007 at 08:31:16PM -0800, Andrew Morton wrote:
> >> On Wed, 21 Nov 2007 15:16:59 -0700 Andrew Patterson <andrew.patterson@hp.com> wrote:
> >>
> >>> The buf in fs/sysfs.c:subsys_attr_store() does not seem to be updated
> >>> correctly when returning a negative value (indicating that an error
> >>> condition has occurred) is returned.  If a negative value is returned,
> >>> the next subsequent call to subsys_attr_store will have the contents of
> >>> buf appended to the previous call.
> >> subsys_attr_store() gets deleted by
> >> http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/gregkh-01-driver/kset-kill-subsys-attr.patch
> >>
> >> So maybe we will soon accidentally fix whatever-this-is?  Or maybe we will
> >> faithfully maintain it.
> > 
> > Yes, subsys attributes go away, but this is showing a bug in the sysfs
> > core with attributes, not in the "middle" layers of attributes.
> > 
> > I bounced the original bug report to Tejun, who has been changing the
> > logic around this area to see if he sees anything that might be
> > different now.
> > 
> > Tejun?
> 
> Weird, the problem is not reproducible here.
> 
> # echo a > allow_restart
> -bash: echo: write error: Invalid argument
> [  437.518024] buf_ptr = 0xffff810005e20000, buf = x
> [  437.518027] , count = 2
> # echo b > allow_restart
> -bash: echo: write error: Invalid argument
> [  438.972973] buf_ptr = 0xffff81001be6f000, buf = y
> [  438.972976] , count = 2
> # echo c > allow_restart
> -bash: echo: write error: Invalid argument
> [  440.539747] buf_ptr = 0xffff81001d4ba000, buf = z
> [  440.539750] , count = 2
> 
> Which is expected.  On each open, sysfs_buffer is allocated with kzalloc
> and the buffer is freed on close, so I don't see how it can happen.
> Behavior for multiple write can be considered peculiar in that ppos is
> essentially ignored and each write is passed just like brand new write
> to ->store method but this too is the expected behavior.
> 
> # (echo a; echo b; echo c) > allow_restart
> [  765.257132] buf_ptr = 0xffff81001be4f000, buf = a
> [  765.257135] , count = 2
> [  765.285474] buf_ptr = 0xffff81001be4f000, buf = b
> [  765.285484] , count = 2
> [  765.314002] buf_ptr = 0xffff81001be4f000, buf = c
> [  765.314004] , count = 2
> -bash: echo: write error: Invalid argument
> -bash: echo: write error: Invalid argument
> -bash: echo: write error: Invalid argument
> 
> Andrew Petterson, can you please build 2.6.24-rc3 from clean source tree
> and retry?
> 

I tried with clean 2.6.24-rc3 and get the same bad behavior.  This is on
an ia64 box, so maybe that is an issue. I can try on an x86 box as well.
Oh, one other thing.  I tried a "uname -r" to make sure I had the
correct kernel booted and got:

# uname -r
2.6.24-rc3
x
y
z
#

Andrew
-- 
Andrew Patterson
Hewlett-Packard

Re: Error returns not handled correctly by sysfs.c:subsys_attr_store()

[Greg KH]

On Wed, Nov 28, 2007 at 12:31:40PM -0700, Andrew Patterson wrote:
> On Wed, 2007-11-28 at 16:42 +0900, Tejun Heo wrote:
> > Greg KH wrote:
> > > On Mon, Nov 26, 2007 at 08:31:16PM -0800, Andrew Morton wrote:
> > >> On Wed, 21 Nov 2007 15:16:59 -0700 Andrew Patterson <andrew.patterson@hp.com> wrote:
> > >>
> > >>> The buf in fs/sysfs.c:subsys_attr_store() does not seem to be updated
> > >>> correctly when returning a negative value (indicating that an error
> > >>> condition has occurred) is returned.  If a negative value is returned,
> > >>> the next subsequent call to subsys_attr_store will have the contents of
> > >>> buf appended to the previous call.
> > >> subsys_attr_store() gets deleted by
> > >> http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/gregkh-01-driver/kset-kill-subsys-attr.patch
> > >>
> > >> So maybe we will soon accidentally fix whatever-this-is?  Or maybe we will
> > >> faithfully maintain it.
> > > 
> > > Yes, subsys attributes go away, but this is showing a bug in the sysfs
> > > core with attributes, not in the "middle" layers of attributes.
> > > 
> > > I bounced the original bug report to Tejun, who has been changing the
> > > logic around this area to see if he sees anything that might be
> > > different now.
> > > 
> > > Tejun?
> > 
> > Weird, the problem is not reproducible here.
> > 
> > # echo a > allow_restart
> > -bash: echo: write error: Invalid argument
> > [  437.518024] buf_ptr = 0xffff810005e20000, buf = x
> > [  437.518027] , count = 2
> > # echo b > allow_restart
> > -bash: echo: write error: Invalid argument
> > [  438.972973] buf_ptr = 0xffff81001be6f000, buf = y
> > [  438.972976] , count = 2
> > # echo c > allow_restart
> > -bash: echo: write error: Invalid argument
> > [  440.539747] buf_ptr = 0xffff81001d4ba000, buf = z
> > [  440.539750] , count = 2
> > 
> > Which is expected.  On each open, sysfs_buffer is allocated with kzalloc
> > and the buffer is freed on close, so I don't see how it can happen.
> > Behavior for multiple write can be considered peculiar in that ppos is
> > essentially ignored and each write is passed just like brand new write
> > to ->store method but this too is the expected behavior.
> > 
> > # (echo a; echo b; echo c) > allow_restart
> > [  765.257132] buf_ptr = 0xffff81001be4f000, buf = a
> > [  765.257135] , count = 2
> > [  765.285474] buf_ptr = 0xffff81001be4f000, buf = b
> > [  765.285484] , count = 2
> > [  765.314002] buf_ptr = 0xffff81001be4f000, buf = c
> > [  765.314004] , count = 2
> > -bash: echo: write error: Invalid argument
> > -bash: echo: write error: Invalid argument
> > -bash: echo: write error: Invalid argument
> > 
> > Andrew Petterson, can you please build 2.6.24-rc3 from clean source tree
> > and retry?
> > 
> 
> I tried with clean 2.6.24-rc3 and get the same bad behavior.  This is on
> an ia64 box, so maybe that is an issue. I can try on an x86 box as well.

Please do so.

> Oh, one other thing.  I tried a "uname -r" to make sure I had the
> correct kernel booted and got:
> 
> # uname -r
> 2.6.24-rc3
> x
> y
> z

Heh, that's not good, try a clean tree :)

thanks,

greg k-h

Re: Error returns not handled correctly by sysfs.c:subsys_attr_store()

[Tejun Heo]

Andrew Patterson wrote:
> I tried with clean 2.6.24-rc3 and get the same bad behavior.  This is on
> an ia64 box, so maybe that is an issue. I can try on an x86 box as well.
> Oh, one other thing.  I tried a "uname -r" to make sure I had the
> correct kernel booted and got:
> 
> # uname -r
> 2.6.24-rc3
> x
> y
> z
> #

Yeah, please try it on another machine from clean tree.  sysfs code is
definitely not endian dependent and is 64 bit clean.  Heck, all my test
machines run 64 bit these days.  I would be surprised if it's something
architecture dependent but please try on a different machine with
different userland with kernel built from fresh source tree.

Thanks.

-- 
tejun

Re: Error returns not handled correctly by sysfs.c:subsys_attr_store()

[Andrew Patterson]

On Thu, 2007-11-29 at 10:07 +0900, Tejun Heo wrote:
> Andrew Patterson wrote:
> > I tried with clean 2.6.24-rc3 and get the same bad behavior.  This is on
> > an ia64 box, so maybe that is an issue. I can try on an x86 box as well.
> > Oh, one other thing.  I tried a "uname -r" to make sure I had the
> > correct kernel booted and got:
> > 
> > # uname -r
> > 2.6.24-rc3
> > x
> > y
> > z
> > #
> 
> Yeah, please try it on another machine from clean tree.  sysfs code is
> definitely not endian dependent and is 64 bit clean.  Heck, all my test
> machines run 64 bit these days.  I would be surprised if it's something
> architecture dependent but please try on a different machine with
> different userland with kernel built from fresh source tree.
> 
> Thanks.

I tried this on a AMD system running an i386 kernel. I get the same bad
behavior.  This is from a 2.6.24-rc3 kernel downloaded from kernel.org.
I ran "make mrproper" followed by "make oldconfig" and accepted all the
defaults for the config.

There is one slight change with this experiment.  Other nodes are not
getting corrupted, i.e., uname -r is getting the correct value.

-- 
Andrew Patterson
Hewlett-Packard Company

Re: Error returns not handled correctly by sysfs.c:subsys_attr_store()

[Greg KH]

On Mon, Dec 03, 2007 at 02:15:58PM -0700, Andrew Patterson wrote:
> On Thu, 2007-11-29 at 10:07 +0900, Tejun Heo wrote:
> > Andrew Patterson wrote:
> > > I tried with clean 2.6.24-rc3 and get the same bad behavior.  This is on
> > > an ia64 box, so maybe that is an issue. I can try on an x86 box as well.
> > > Oh, one other thing.  I tried a "uname -r" to make sure I had the
> > > correct kernel booted and got:
> > > 
> > > # uname -r
> > > 2.6.24-rc3
> > > x
> > > y
> > > z
> > > #
> > 
> > Yeah, please try it on another machine from clean tree.  sysfs code is
> > definitely not endian dependent and is 64 bit clean.  Heck, all my test
> > machines run 64 bit these days.  I would be surprised if it's something
> > architecture dependent but please try on a different machine with
> > different userland with kernel built from fresh source tree.
> > 
> > Thanks.
> 
> I tried this on a AMD system running an i386 kernel. I get the same bad
> behavior.  This is from a 2.6.24-rc3 kernel downloaded from kernel.org.
> I ran "make mrproper" followed by "make oldconfig" and accepted all the
> defaults for the config.
> 
> There is one slight change with this experiment.  Other nodes are not
> getting corrupted, i.e., uname -r is getting the correct value.

Are you still seeing this on 2.6.24-rc6?

I still can not duplicate this here :(

thanks,

greg k-h

Re: Error returns not handled correctly by sysfs.c:subsys_attr_store()

[Andrew Patterson]

On Mon, 2007-12-03 at 14:15 -0700, Andrew Patterson wrote:
> On Thu, 2007-11-29 at 10:07 +0900, Tejun Heo wrote:
> > Andrew Patterson wrote:
> > > I tried with clean 2.6.24-rc3 and get the same bad behavior.  This is on
> > > an ia64 box, so maybe that is an issue. I can try on an x86 box as well.
> > > Oh, one other thing.  I tried a "uname -r" to make sure I had the
> > > correct kernel booted and got:
> > > 
> > > # uname -r
> > > 2.6.24-rc3
> > > x
> > > y
> > > z
> > > #
> > 
> > Yeah, please try it on another machine from clean tree.  sysfs code is
> > definitely not endian dependent and is 64 bit clean.  Heck, all my test
> > machines run 64 bit these days.  I would be surprised if it's something
> > architecture dependent but please try on a different machine with
> > different userland with kernel built from fresh source tree.
> > 
> > Thanks.
> 
> I tried this on a AMD system running an i386 kernel. I get the same bad
> behavior.  This is from a 2.6.24-rc3 kernel downloaded from kernel.org.
> I ran "make mrproper" followed by "make oldconfig" and accepted all the
> defaults for the config.
> 
> There is one slight change with this experiment.  Other nodes are not
> getting corrupted, i.e., uname -r is getting the correct value.

It looks like this is a shell issue.  After looking through the sysfs
code, I realized that this problem seems to be driven from user-land.
So I performed some experiments:

     1. Wrote a simple program that just used write(2) to write to the
        sysfs entry. This works fine.
     2. Used /bin/echo instead of the built-in echo command.  This too
        works fine.
     3. Tried several shells.  Zsh and Bash both fail.  Csh works fine.

I then ran strace on the following shell-script:

#!/bin/bash

echo x > allow_restart
echo y > allow_restart
echo z > allow_restart

and got:

# strace -e trace=write ~/tmp/tester.sh 
write(1, "x\n", 2)                      = -1 EINVAL (Invalid argument)
write(1, "x\n", 2)                      = -1 EINVAL (Invalid argument)
write(2, "/home/andrew/tmp/tester.sh: line"..., 72/home/andrew/tmp/tester.sh: line 4: echo: write error: Invalid argument
) = 72
write(1, "x\ny\n", 4)                   = -1 EINVAL (Invalid argument)
write(1, "x\ny\n", 4)                   = -1 EINVAL (Invalid argument)
write(2, "/home/andrew/tmp/tester.sh: line"..., 72/home/andrew/tmp/tester.sh: line 5: echo: write error: Invalid argument
) = 72
write(1, "x\ny\nz\n", 6)                = -1 EINVAL (Invalid argument)
write(1, "x\ny\nz\n", 6)                = -1 EINVAL (Invalid argument)
write(2, "/home/andrew/tmp/tester.sh: line"..., 72/home/andrew/tmp/tester.sh: line 6: echo: write error: Invalid argument
) = 72
write(1, "x\ny\nz\n", 6x
y
z
)                = 6
Process 3800 detached

As you can see, subsequent echo commands have their arguments appended
to the previous command. So it seems that the shell is not resetting the
buffer between failed echo commands.  

-- 
Andrew Patterson
Hewlett-Packard Company

Re: Error returns not handled correctly by sysfs.c:subsys_attr_store()

[Tejun Heo]

Hello,

Andrew Patterson wrote:
> It looks like this is a shell issue.  After looking through the sysfs
> code, I realized that this problem seems to be driven from user-land.
> So I performed some experiments:
> 
>      1. Wrote a simple program that just used write(2) to write to the
>         sysfs entry. This works fine.
>      2. Used /bin/echo instead of the built-in echo command.  This too
>         works fine.
>      3. Tried several shells.  Zsh and Bash both fail.  Csh works fine.
> 
> I then ran strace on the following shell-script:
> 
> #!/bin/bash
> 
> echo x > allow_restart
> echo y > allow_restart
> echo z > allow_restart
> 
> and got:
> 
> # strace -e trace=write ~/tmp/tester.sh 
> write(1, "x\n", 2)                      = -1 EINVAL (Invalid argument)
> write(1, "x\n", 2)                      = -1 EINVAL (Invalid argument)
> write(2, "/home/andrew/tmp/tester.sh: line"..., 72/home/andrew/tmp/tester.sh: line 4: echo: write error: Invalid argument
> ) = 72
> write(1, "x\ny\n", 4)                   = -1 EINVAL (Invalid argument)
> write(1, "x\ny\n", 4)                   = -1 EINVAL (Invalid argument)
> write(2, "/home/andrew/tmp/tester.sh: line"..., 72/home/andrew/tmp/tester.sh: line 5: echo: write error: Invalid argument
> ) = 72
> write(1, "x\ny\nz\n", 6)                = -1 EINVAL (Invalid argument)
> write(1, "x\ny\nz\n", 6)                = -1 EINVAL (Invalid argument)
> write(2, "/home/andrew/tmp/tester.sh: line"..., 72/home/andrew/tmp/tester.sh: line 6: echo: write error: Invalid argument
> ) = 72
> write(1, "x\ny\nz\n", 6x
> y
> z
> )                = 6
> Process 3800 detached

Eeeeeeeekkkk.... That's scary.  Which distro are you using and what does
'bash --version' say?

-- 
tejun

Re: Error returns not handled correctly by sysfs.c:subsys_attr_store()

[Andrew Patterson]

On Fri, 2008-01-04 at 09:07 +0900, Tejun Heo wrote:
> Hello,
> 
> Andrew Patterson wrote:
> > It looks like this is a shell issue.  After looking through the sysfs
> > code, I realized that this problem seems to be driven from user-land.
> > So I performed some experiments:
> > 
> >      1. Wrote a simple program that just used write(2) to write to the
> >         sysfs entry. This works fine.
> >      2. Used /bin/echo instead of the built-in echo command.  This too
> >         works fine.
> >      3. Tried several shells.  Zsh and Bash both fail.  Csh works fine.
> > 
> > I then ran strace on the following shell-script:
> > 
> > #!/bin/bash
> > 
> > echo x > allow_restart
> > echo y > allow_restart
> > echo z > allow_restart
> > 
> > and got:
> > 
> > # strace -e trace=write ~/tmp/tester.sh 
> > write(1, "x\n", 2)                      = -1 EINVAL (Invalid argument)
> > write(1, "x\n", 2)                      = -1 EINVAL (Invalid argument)
> > write(2, "/home/andrew/tmp/tester.sh: line"..., 72/home/andrew/tmp/tester.sh: line 4: echo: write error: Invalid argument
> > ) = 72
> > write(1, "x\ny\n", 4)                   = -1 EINVAL (Invalid argument)
> > write(1, "x\ny\n", 4)                   = -1 EINVAL (Invalid argument)
> > write(2, "/home/andrew/tmp/tester.sh: line"..., 72/home/andrew/tmp/tester.sh: line 5: echo: write error: Invalid argument
> > ) = 72
> > write(1, "x\ny\nz\n", 6)                = -1 EINVAL (Invalid argument)
> > write(1, "x\ny\nz\n", 6)                = -1 EINVAL (Invalid argument)
> > write(2, "/home/andrew/tmp/tester.sh: line"..., 72/home/andrew/tmp/tester.sh: line 6: echo: write error: Invalid argument
> > ) = 72
> > write(1, "x\ny\nz\n", 6x
> > y
> > z
> > )                = 6
> > Process 3800 detached
> 
> Eeeeeeeekkkk.... That's scary.  Which distro are you using and what does
> 'bash --version' say?

IA64 Debian lenny.  

# bash --version
GNU bash, version 3.1.17(1)-release (ia64-unknown-linux-gnu)

# zsh --version 
zsh 4.3.4 (ia64-unknown-linux-gnu)

# csh --version
tcsh 6.14.00 (Astron) 2005-03-25 (ia64-unknown-linux) options
wide,nls,dl,al,kan,rh,nd,color,filec

I suppose I should try this an ia32 box again, and perhaps with some
other distros.  I am not sure what the kernel can do about this, but it
might be nice to report it to the shell maintainers.

-- 
Andrew Patterson
Hewlett-Packard Company

Re: Error returns not handled correctly by sysfs.c:subsys_attr_store()

[Andrew Patterson]

On Thu, 2008-01-03 at 17:17 -0700, Andrew Patterson wrote:
> On Fri, 2008-01-04 at 09:07 +0900, Tejun Heo wrote:
> > Hello,
> > 
> > Andrew Patterson wrote:
> > > It looks like this is a shell issue.  After looking through the sysfs
> > > code, I realized that this problem seems to be driven from user-land.
> > > So I performed some experiments:
> > > 
> > >      1. Wrote a simple program that just used write(2) to write to the
> > >         sysfs entry. This works fine.
> > >      2. Used /bin/echo instead of the built-in echo command.  This too
> > >         works fine.
> > >      3. Tried several shells.  Zsh and Bash both fail.  Csh works fine.
> > > 
> > > I then ran strace on the following shell-script:
> > > 
> > > #!/bin/bash
> > > 
> > > echo x > allow_restart
> > > echo y > allow_restart
> > > echo z > allow_restart
> > > 
> > > and got:
> > > 
> > > # strace -e trace=write ~/tmp/tester.sh 
> > > write(1, "x\n", 2)                      = -1 EINVAL (Invalid argument)
> > > write(1, "x\n", 2)                      = -1 EINVAL (Invalid argument)
> > > write(2, "/home/andrew/tmp/tester.sh: line"..., 72/home/andrew/tmp/tester.sh: line 4: echo: write error: Invalid argument
> > > ) = 72
> > > write(1, "x\ny\n", 4)                   = -1 EINVAL (Invalid argument)
> > > write(1, "x\ny\n", 4)                   = -1 EINVAL (Invalid argument)
> > > write(2, "/home/andrew/tmp/tester.sh: line"..., 72/home/andrew/tmp/tester.sh: line 5: echo: write error: Invalid argument
> > > ) = 72
> > > write(1, "x\ny\nz\n", 6)                = -1 EINVAL (Invalid argument)
> > > write(1, "x\ny\nz\n", 6)                = -1 EINVAL (Invalid argument)
> > > write(2, "/home/andrew/tmp/tester.sh: line"..., 72/home/andrew/tmp/tester.sh: line 6: echo: write error: Invalid argument
> > > ) = 72
> > > write(1, "x\ny\nz\n", 6x
> > > y
> > > z
> > > )                = 6
> > > Process 3800 detached
> > 
> > Eeeeeeeekkkk.... That's scary.  Which distro are you using and what does
> > 'bash --version' say?
> 
> IA64 Debian lenny.  
> 
> # bash --version
> GNU bash, version 3.1.17(1)-release (ia64-unknown-linux-gnu)
> 
> # zsh --version 
> zsh 4.3.4 (ia64-unknown-linux-gnu)
> 
> # csh --version
> tcsh 6.14.00 (Astron) 2005-03-25 (ia64-unknown-linux) options
> wide,nls,dl,al,kan,rh,nd,color,filec
> 
> I suppose I should try this an ia32 box again, and perhaps with some
> other distros.  I am not sure what the kernel can do about this, but it
> might be nice to report it to the shell maintainers.

Some further tests:

AMD running Debian lenny with i686 kernel -- fails.  
Bash version = 3.1.17(1)

Intel running Ubuntu/gutsy with i686 kernel -- fails.
Bash version = 3.2.25(1)

Itanium running SLES10 with ia64 kernel -- succeeds.
Bash version = 3.1.17(1)

BTW, I found a way to reproduce this without modifying the kernel.
The /sys/class/scsi_host/*/state sysfs store routine returns EINVAL if
an invalid state is written. So just echo 2 bad values to the the state
sysfs entry while running strace.

Andrew

-- 
Andrew Patterson
Hewlett-Packard Company

Re: Error returns not handled correctly by sysfs.c:subsys_attr_store()

[Andrey Borzenkov]

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

On Friday 04 January 2008, Andrew Patterson wrote:
> On Thu, 2008-01-03 at 17:17 -0700, Andrew Patterson wrote:
> > On Fri, 2008-01-04 at 09:07 +0900, Tejun Heo wrote:
> > > Hello,
> > > 
> > > Andrew Patterson wrote:
> > > > It looks like this is a shell issue.  After looking through the sysfs
> > > > code, I realized that this problem seems to be driven from user-land.
> > > > So I performed some experiments:
> > > > 
> > > >      1. Wrote a simple program that just used write(2) to write to the
> > > >         sysfs entry. This works fine.
> > > >      2. Used /bin/echo instead of the built-in echo command.  This too
> > > >         works fine.
> > > >      3. Tried several shells.  Zsh and Bash both fail.  Csh works fine.
> > > > 
> > > > I then ran strace on the following shell-script:
> > > > 
> > > > #!/bin/bash
> > > > 
> > > > echo x > allow_restart
> > > > echo y > allow_restart
> > > > echo z > allow_restart
> > > > 
> > > > and got:
> > > > 
> > > > # strace -e trace=write ~/tmp/tester.sh 
> > > > write(1, "x\n", 2)                      = -1 EINVAL (Invalid argument)
> > > > write(1, "x\n", 2)                      = -1 EINVAL (Invalid argument)
> > > > write(2, "/home/andrew/tmp/tester.sh: line"..., 
72/home/andrew/tmp/tester.sh: line 4: echo: write error: Invalid argument
> > > > ) = 72
> > > > write(1, "x\ny\n", 4)                   = -1 EINVAL (Invalid argument)
> > > > write(1, "x\ny\n", 4)                   = -1 EINVAL (Invalid argument)
> > > > write(2, "/home/andrew/tmp/tester.sh: line"..., 
72/home/andrew/tmp/tester.sh: line 5: echo: write error: Invalid argument
> > > > ) = 72
> > > > write(1, "x\ny\nz\n", 6)                = -1 EINVAL (Invalid argument)
> > > > write(1, "x\ny\nz\n", 6)                = -1 EINVAL (Invalid argument)
> > > > write(2, "/home/andrew/tmp/tester.sh: line"..., 
72/home/andrew/tmp/tester.sh: line 6: echo: write error: Invalid argument
> > > > ) = 72
> > > > write(1, "x\ny\nz\n", 6x
> > > > y
> > > > z
> > > > )                = 6
> > > > Process 3800 detached
> > > 
> > > Eeeeeeeekkkk.... That's scary.  Which distro are you using and what does
> > > 'bash --version' say?
> > 
> > IA64 Debian lenny.  
> > 
> > # bash --version
> > GNU bash, version 3.1.17(1)-release (ia64-unknown-linux-gnu)
> > 
> > # zsh --version 
> > zsh 4.3.4 (ia64-unknown-linux-gnu)
> > 
> > # csh --version
> > tcsh 6.14.00 (Astron) 2005-03-25 (ia64-unknown-linux) options
> > wide,nls,dl,al,kan,rh,nd,color,filec
> > 
> > I suppose I should try this an ia32 box again, and perhaps with some
> > other distros.  I am not sure what the kernel can do about this, but it
> > might be nice to report it to the shell maintainers.
> 
> Some further tests:
> 
> AMD running Debian lenny with i686 kernel -- fails.  
> Bash version = 3.1.17(1)
> 
> Intel running Ubuntu/gutsy with i686 kernel -- fails.
> Bash version = 3.2.25(1)
> 
> Itanium running SLES10 with ia64 kernel -- succeeds.
> Bash version = 3.1.17(1)
> 
> BTW, I found a way to reproduce this without modifying the kernel.
> The /sys/class/scsi_host/*/state sysfs store routine returns EINVAL if
> an invalid state is written. So just echo 2 bad values to the the state
> sysfs entry while running strace.
> 

I can't reproduce it using zsh either 4.3.4 as shipped by Mandriva or zsh CVS 
head. In both cases it echoes correct argument. Nor do I see double writes's in 
strace.

{pts/0}% sudo strace -e trace=write /tmp/foo # zsh 4.3.4
write(1, "x\n", 2)                      = -1 EINVAL (Invalid argument)
write(2, "/tmp/foo:echo:3: write error: \320\235"..., 72/tmp/foo:echo:3: write 
error: Недопустимый аргумент
) = 72
write(1, "y\n", 2)                      = -1 EINVAL (Invalid argument)
write(2, "/tmp/foo:echo:4: write error: \320\235"..., 72/tmp/foo:echo:4: write 
error: Недопустимый аргумент
) = 72
write(1, "z\n", 2)                      = -1 EINVAL (Invalid argument)
write(2, "/tmp/foo:echo:5: write error: \320\235"..., 72/tmp/foo:echo:5: write 
error: Недопустимый аргумент
) = 72
{pts/0}% sudo strace -e trace=write /tmp/foo # zsh CVS head
write(1, "x\n", 2)                      = -1 EINVAL (Invalid argument)
write(2, "/tmp/foo:echo:3: write error: \320\235"..., 72/tmp/foo:echo:3: write 
error: Недопустимый аргумент
) = 72
write(1, "y\n", 2)                      = -1 EINVAL (Invalid argument)
write(2, "/tmp/foo:echo:4: write error: \320\235"..., 72/tmp/foo:echo:4: write 
error: Недопустимый аргумент
) = 72
write(1, "z\n", 2)                      = -1 EINVAL (Invalid argument)
write(2, "/tmp/foo:echo:5: write error: \320\235"..., 72/tmp/foo:echo:5: write 
error: Недопустимый аргумент
) = 72

{pts/0}% cat /tmp/foo
#!/home/bor/pkg/bin/zsh -f

echo x > state
echo y > state
echo z > state

where state is /sys/power/state


{pts/1}% zsh --version
zsh 4.3.4 (i586-mandriva-linux-gnu)
{pts/1}% ~/pkg/bin/zsh --version
zsh 4.3.4-dev-6 (i686-pc-linux-gnu)

-andrey

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: Error returns not handled correctly by sysfs.c:subsys_attr_store()

[Andrew Patterson]

On Thu, 2008-01-03 at 17:17 -0700, Andrew Patterson wrote:
> On Fri, 2008-01-04 at 09:07 +0900, Tejun Heo wrote:
> > Hello,
> > 
> > Andrew Patterson wrote:
> > > It looks like this is a shell issue.  After looking through the sysfs
> > > code, I realized that this problem seems to be driven from user-land.
> > > So I performed some experiments:
> > > 
> > >      1. Wrote a simple program that just used write(2) to write to the
> > >         sysfs entry. This works fine.
> > >      2. Used /bin/echo instead of the built-in echo command.  This too
> > >         works fine.
> > >      3. Tried several shells.  Zsh and Bash both fail.  Csh works fine.
> > > 
> > > I then ran strace on the following shell-script:
> > > 
> > > #!/bin/bash
> > > 
> > > echo x > allow_restart
> > > echo y > allow_restart
> > > echo z > allow_restart
> > > 
> > > and got:
> > > 
> > > # strace -e trace=write ~/tmp/tester.sh 
> > > write(1, "x\n", 2)                      = -1 EINVAL (Invalid argument)
> > > write(1, "x\n", 2)                      = -1 EINVAL (Invalid argument)
> > > write(2, "/home/andrew/tmp/tester.sh: line"..., 72/home/andrew/tmp/tester.sh: line 4: echo: write error: Invalid argument
> > > ) = 72
> > > write(1, "x\ny\n", 4)                   = -1 EINVAL (Invalid argument)
> > > write(1, "x\ny\n", 4)                   = -1 EINVAL (Invalid argument)
> > > write(2, "/home/andrew/tmp/tester.sh: line"..., 72/home/andrew/tmp/tester.sh: line 5: echo: write error: Invalid argument
> > > ) = 72
> > > write(1, "x\ny\nz\n", 6)                = -1 EINVAL (Invalid argument)
> > > write(1, "x\ny\nz\n", 6)                = -1 EINVAL (Invalid argument)
> > > write(2, "/home/andrew/tmp/tester.sh: line"..., 72/home/andrew/tmp/tester.sh: line 6: echo: write error: Invalid argument
> > > ) = 72
> > > write(1, "x\ny\nz\n", 6x
> > > y
> > > z
> > > )                = 6
> > > Process 3800 detached
> > 
> > Eeeeeeeekkkk.... That's scary.  Which distro are you using and what does
> > 'bash --version' say?
> 
> IA64 Debian lenny.  
> 
> # bash --version
> GNU bash, version 3.1.17(1)-release (ia64-unknown-linux-gnu)
> 
> # zsh --version 
> zsh 4.3.4 (ia64-unknown-linux-gnu)
> 
> # csh --version
> tcsh 6.14.00 (Astron) 2005-03-25 (ia64-unknown-linux) options
> wide,nls,dl,al,kan,rh,nd,color,filec
> 
> I suppose I should try this an ia32 box again, and perhaps with some
> other distros.  I am not sure what the kernel can do about this, but it
> might be nice to report it to the shell maintainers.
> 

This looks like it might be the culprit. 

http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=459643

The fact that it works on SLES10 lends further evidence to glibc being
the problem.

-- 
Andrew Patterson
Hewlett-Packard Company