[PATCH 1/3] disallow seeks and appends on sysfs files

[PATCH 1/3] disallow seeks and appends on sysfs files

[Mitch Williams]

This patch causes sysfs to return errors if the caller attempts to append
to or seek on a sysfs file.

Generated from 2.6.11-rc1.

Signed-off-by: Mitch Williams <mitch.a.williams@intel.com>

diff -uprN -X dontdiff linux-2.6.11-clean/fs/sysfs/file.c linux-2.6.11/fs/sysfs/file.c
--- linux-2.6.11-clean/fs/sysfs/file.c	2004-12-24 13:33:50.000000000 -0800
+++ linux-2.6.11/fs/sysfs/file.c	2005-01-21 13:09:21.000000000 -0800
@@ -281,6 +281,11 @@ static int check_perm(struct inode * ino
 	if (!ops)
 		goto Eaccess;

+	/* Is the file is open for append?  Sorry, we don't do that. */
+	if (file->f_flags & O_APPEND) {
+		goto Einval;
+	}
+
 	/* File needs write support.
 	 * The inode's perms must say it's ok,
 	 * and we must have a store method.
@@ -312,6 +302,10 @@ static int check_perm(struct inode * ino
 		file->private_data = buffer;
 	} else
 		error = -ENOMEM;
+
+	/*  Set mode bits to disallow seeking.  */
+	file->f_mode &= ~(FMODE_LSEEK | FMODE_PREAD | FMODE_PWRITE);
+
 	goto Done;

  Einval:
@@ -368,7 +343,7 @@ static int sysfs_release(struct inode *
 struct file_operations sysfs_file_operations = {
 	.read		= sysfs_read_file,
 	.write		= sysfs_write_file,
-	.llseek		= generic_file_llseek,
+	.llseek		= no_llseek,
 	.open		= sysfs_open_file,
 	.release	= sysfs_release,
 };

Re: [PATCH 1/3] disallow seeks and appends on sysfs files

[Greg KH]

On Fri, Jan 21, 2005 at 02:49:39PM -0800, Mitch Williams wrote:
> This patch causes sysfs to return errors if the caller attempts to append
> to or seek on a sysfs file.

And what happens to it today if you do either of these?

Also, isn't this two different things?

> 
> Generated from 2.6.11-rc1.
> 
> Signed-off-by: Mitch Williams <mitch.a.williams@intel.com>
> 
> diff -uprN -X dontdiff linux-2.6.11-clean/fs/sysfs/file.c linux-2.6.11/fs/sysfs/file.c
> --- linux-2.6.11-clean/fs/sysfs/file.c	2004-12-24 13:33:50.000000000 -0800
> +++ linux-2.6.11/fs/sysfs/file.c	2005-01-21 13:09:21.000000000 -0800
> @@ -281,6 +281,11 @@ static int check_perm(struct inode * ino
>  	if (!ops)
>  		goto Eaccess;
> 
> +	/* Is the file is open for append?  Sorry, we don't do that. */
> +	if (file->f_flags & O_APPEND) {
> +		goto Einval;
> +	}

Please, no {} for one line if statements.  Like the one above it :)



> +
>  	/* File needs write support.
>  	 * The inode's perms must say it's ok,
>  	 * and we must have a store method.
> @@ -312,6 +302,10 @@ static int check_perm(struct inode * ino
>  		file->private_data = buffer;
>  	} else
>  		error = -ENOMEM;
> +
> +	/*  Set mode bits to disallow seeking.  */
> +	file->f_mode &= ~(FMODE_LSEEK | FMODE_PREAD | FMODE_PWRITE);
> +
>  	goto Done;
> 
>   Einval:
> @@ -368,7 +343,7 @@ static int sysfs_release(struct inode *
>  struct file_operations sysfs_file_operations = {
>  	.read		= sysfs_read_file,
>  	.write		= sysfs_write_file,
> -	.llseek		= generic_file_llseek,
> +	.llseek		= no_llseek,
>  	.open		= sysfs_open_file,
>  	.release	= sysfs_release,
>  };

-- 

Re: [PATCH 1/3] disallow seeks and appends on sysfs files

[Mitch Williams]

On Sat, 22 Jan 2005, Greg KH wrote:

>
> On Fri, Jan 21, 2005 at 02:49:39PM -0800, Mitch Williams wrote:
> > This patch causes sysfs to return errors if the caller attempts to
> append
> > to or seek on a sysfs file.
>
> And what happens to it today if you do either of these?
>
> Also, isn't this two different things?
>
Appending and seeking are obviously two different operations, but the
result is the same to the sysfs file system.  Because the store method
doesn't have an offset argument, it must assume that all writes are based
from the beginning of the buffer.

So if your sysfs file contains "123" and you do
   echo "45" >> mysysfsfile
instead of the expected "12345", you end up with "45" in the file with no
errors.  Opening the file, seeking, and writing gives the same type of
behavior, with no errors.

This patch just sets a few flags to make sure that errors are returned
when this behavior is seen.  Logically then, the two "features" do the
same thing (set flags), and prevent the same behavior (writing wrong
contents without error).  Hence, I grouped them into one patch.

However, if you want two even simpler patches, I'm willing to comply.  Of
the three patches I sent, this is the most important to me.


>
> Please, no {} for one line if statements.  Like the one above it :)

I'll be glad to fix this and resubmit.  I prefer to not have braces
either, but I've seen a bunch of places in the kernel where people do it,
so I really wasn't sure which was right.  It's not really called out in
the coding style doc either.

Re: [PATCH 1/3] disallow seeks and appends on sysfs files

[Greg KH]

On Mon, Jan 24, 2005 at 10:16:37AM -0800, Mitch Williams wrote:
> 
> 
> On Sat, 22 Jan 2005, Greg KH wrote:
> 
> >
> > On Fri, Jan 21, 2005 at 02:49:39PM -0800, Mitch Williams wrote:
> > > This patch causes sysfs to return errors if the caller attempts to
> > append
> > > to or seek on a sysfs file.
> >
> > And what happens to it today if you do either of these?
> >
> > Also, isn't this two different things?
> >
> Appending and seeking are obviously two different operations, but the
> result is the same to the sysfs file system.  Because the store method
> doesn't have an offset argument, it must assume that all writes are based
> from the beginning of the buffer.
> 
> So if your sysfs file contains "123" and you do
>    echo "45" >> mysysfsfile
> instead of the expected "12345", you end up with "45" in the file with no
> errors.  Opening the file, seeking, and writing gives the same type of
> behavior, with no errors.

Ick, yeah, but users shouldn't be doing that :)

Anyway, ok, I'll accept this kind of patch, to give errors for that.

> However, if you want two even simpler patches, I'm willing to comply.  Of
> the three patches I sent, this is the most important to me.

Yes, could you split it up?

> > Please, no {} for one line if statements.  Like the one above it :)
> 
> I'll be glad to fix this and resubmit.  I prefer to not have braces
> either, but I've seen a bunch of places in the kernel where people do it,
> so I really wasn't sure which was right.  It's not really called out in
> the coding style doc either.

Yes, please fix this and resubmit it.

thanks,

greg k-h