procfs proc_write_file bug

procfs proc_write_file bug

[Petr Grillinger]

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

Hello,

I needed recently to write a large file into the /proc FS,
there is however a bug that makes this impossible. I am working
with 2.4.25 kernel, but the bug is present even in the 2.6 line.

The problem is that the proc_file_write function (in fs/proc/generic.c)
does not pass the file position (loff_r *ppos) to the registered
handling function - it cannot. The prototype for this function
(defined in include/linux/proc_fs.h) does not include the needed
argument.

I have made a patch that solves this problem by adding another hook
for an alternative write handler. This does not break old code using
the procfs write function and allows to write large files properly.
It is however not "very nice".

Do you have some ideas how to solve this problem?

(patch is attached)


	Petr Grillinger


[-- Attachment #2: linux-2.4.25-pg.patch --]
[-- Type: text/plain, Size: 3282 bytes --]

diff -uNrp linux-2.4.25-rtai/fs/proc/generic.c linux-2.4.25-pg/fs/proc/generic.c
--- linux-2.4.25-rtai/fs/proc/generic.c	2003-11-28 19:26:21.000000000 +0100
+++ linux-2.4.25-pg/fs/proc/generic.c	2004-11-19 14:20:40.000000000 +0100
@@ -120,20 +120,38 @@ proc_file_read(struct file * file, char 
 	return retval;
 }
 
-static ssize_t
-proc_file_write(struct file * file, const char * buffer,
-		size_t count, loff_t *ppos)
-{
-	struct inode *inode = file->f_dentry->d_inode;
-	struct proc_dir_entry * dp;
+
+/* 
+ * Added support for files larger than one PAGE 
+ * 2004/11/19, Petr Grillinger, pgrillin@kiv.zcu.cz 
+ *
+ * The old 'write_proc' entry doesn't allow to write files larger than one
+ * PAGE. A new 'write_proc2' entry can be used to implement an alternative
+ * handler - the new version handler gets an additional argument (ppos) which
+ * is a pointer to the current file position. The handler must update this
+ * position after each successful write operation - if there are more data from
+ * the user-space, it will be called again.
+ *
+ * The old interface is available with no changes. The new interface can be
+ * used by assigning a function address to the write_proc2 field of
+ * proc_dir_entry.  If a new interface function is available it is used instead
+ * of the old one.
+ *
+ * The handler should return the number of written bytes or a negative value
+ * for error conditions.
+ */
+static ssize_t proc_file_write(struct file * file, const char * buffer, size_t
+        count, loff_t *ppos) { struct inode *inode = file->f_dentry->d_inode;
+    struct proc_dir_entry * dp;
 	
 	dp = (struct proc_dir_entry *) inode->u.generic_ip;
-
-	if (!dp->write_proc)
-		return -EIO;
-
-	/* FIXME: does this routine need ppos?  probably... */
-	return dp->write_proc(file, buffer, count, dp->data);
+   
+    if (dp->write_proc2)  /* new interface */
+        return dp->write_proc2(file, buffer, ppos, count, dp->data);
+    if (dp->write_proc)  /* old interface */
+        return dp->write_proc(file, buffer, count, dp->data);
+ 
+    return -EIO;
 }
 
 
diff -uNrp linux-2.4.25-rtai/include/linux/proc_fs.h linux-2.4.25-pg/include/linux/proc_fs.h
--- linux-2.4.25-rtai/include/linux/proc_fs.h	2003-11-28 19:26:21.000000000 +0100
+++ linux-2.4.25-pg/include/linux/proc_fs.h	2004-11-19 14:10:56.000000000 +0100
@@ -48,6 +48,13 @@ typedef	int (read_proc_t)(char *page, ch
 			  int count, int *eof, void *data);
 typedef	int (write_proc_t)(struct file *file, const char *buffer,
 			   unsigned long count, void *data);
+/* A write function must have the ppos pointer to be able to access
+ * larger files than one page. Not including it was a bug which 
+ * cannot be corrected without breaking old code. That is why there
+ * is a new write_proc2 entry in the proc_dir_entry structure that
+ * uses this corrected prototype. */
+typedef	int (write_proc2_t)(struct file *file, const char *buffer,
+			   loff_t *ppos, unsigned long count, void *data);
 typedef int (get_info_t)(char *, char **, off_t, int);
 
 struct proc_dir_entry {
@@ -67,6 +74,7 @@ struct proc_dir_entry {
 	void *data;
 	read_proc_t *read_proc;
 	write_proc_t *write_proc;
+	write_proc2_t *write_proc2;
 	atomic_t count;		/* use count */
 	int deleted;		/* delete flag */
 	kdev_t	rdev;

Re: procfs proc_write_file bug

[Greg KH]

On Fri, Nov 19, 2004 at 03:12:07PM +0100, Petr Grillinger wrote:
> 
> I needed recently to write a large file into the /proc FS,

Why do you need to do this?  Wouldn't it be simpler to create your own
fs if you want to accept large ammounts of data?  /proc isn't meant for
something like this.

Oh, and you forgot to use tabs in your code :)

thanks,

greg k-h

Re: procfs proc_write_file bug

[Petr Grillinger]

Sorry about the tabs, I forgot to change my editor settings.
I use procfs to export information about my module - in text form.
I also use it to export the binary image of some configuration
data (about 16KB). To be able to configure the module by writing
into /procfs (binary/text) would be nice, because then the interface
is all in one place.

But still thanks for the idea, it didn't occur to me that I could
solve it by creating my own FS. Maybe next time :-)

Petr Grillinger

Greg KH wrote:
> On Fri, Nov 19, 2004 at 03:12:07PM +0100, Petr Grillinger wrote:
> 
>>I needed recently to write a large file into the /proc FS,
> 
> 
> Why do you need to do this?  Wouldn't it be simpler to create your own
> fs if you want to accept large ammounts of data?  /proc isn't meant for
> something like this.
> 
> Oh, and you forgot to use tabs in your code :)
> 
> thanks,
> 
> greg k-h
> -
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: procfs proc_write_file bug

[Greg KH]

On Fri, Nov 19, 2004 at 05:48:14PM +0100, Petr Grillinger wrote:
> Sorry about the tabs, I forgot to change my editor settings.
> I use procfs to export information about my module - in text form.

That's what sysfs is for.

> I also use it to export the binary image of some configuration
> data (about 16KB).

sysfs's binary files work well for this.

thanks,

greg k-h

Re: procfs proc_write_file bug

[Petr Grillinger]

Ok, thanks for the info but it doesn't solve my problem. I am
limited to 2.4 kernels because I need a stable RTAI/RTNET machine.
2.6 kernel support for these two isn't mature yet.

But then, maybe I could port sysfs into a 2.4 kernel. Do you know
of some reason why this should be difficult?

	Petr Grillinger

Greg KH wrote:
> On Fri, Nov 19, 2004 at 05:48:14PM +0100, Petr Grillinger wrote:
> 
>>Sorry about the tabs, I forgot to change my editor settings.
>>I use procfs to export information about my module - in text form.
> 
> 
> That's what sysfs is for.
> 
>>I also use it to export the binary image of some configuration
>>data (about 16KB).
> 
> 
> sysfs's binary files work well for this.
> 
> thanks,
> 
> greg k-h
> -

Re: procfs proc_write_file bug

[Greg KH]

On Fri, Nov 19, 2004 at 07:21:01PM +0100, Petr Grillinger wrote:
> Ok, thanks for the info but it doesn't solve my problem. I am
> limited to 2.4 kernels because I need a stable RTAI/RTNET machine.
> 2.6 kernel support for these two isn't mature yet.
> 
> But then, maybe I could port sysfs into a 2.4 kernel. Do you know
> of some reason why this should be difficult?

It would be easier to write your own filesystem for 2.4.  Take a look at
pcihpfs as an example of how to do this.

good luck,

greg k-h

Re: procfs proc_write_file bug

[Bryan Henderson]

>But still thanks for the idea, it didn't occur to me that I could
>solve it by creating my own FS. Maybe next time :-)

I would resist the idea of creating a separate filesystem, because of the 
added complexity for the user of having to mount it in addition to a the 
extra (and duplicate) coding effort.

Before sysfs, proc filled that role.

What you're seeing in proc_file_write() isn't a bug but a limitation of 
this simplified interface.  You can do multipage writes by using the more 
general interface where you supply your own file operation methods.  (set 
the proc_fops field in struct proc_dir_entry).

--
Bryan Henderson                          IBM Almaden Research Center
San Jose CA                              Filesystems

Re: procfs proc_write_file bug

[Greg KH]

On Fri, Nov 19, 2004 at 03:09:12PM -0800, Bryan Henderson wrote:
> >But still thanks for the idea, it didn't occur to me that I could
> >solve it by creating my own FS. Maybe next time :-)
> 
> I would resist the idea of creating a separate filesystem, because of the 
> added complexity for the user of having to mount it in addition to a the 
> extra (and duplicate) coding effort.

It's dirt simple to write your own filesystem these days, thanks to
libfs.  Look at the pcihpfs code in 2.4, very tiny.

And as for mounting it, bah, that's what installers are for, it's not a
big deal at all :)

thanks,

greg k-h

Re: procfs proc_write_file bug

[Petr Grillinger]

Bryan Henderson wrote:
>>But still thanks for the idea, it didn't occur to me that I could
>>solve it by creating my own FS. Maybe next time :-)
> 
> 
> I would resist the idea of creating a separate filesystem, because of the 
> added complexity for the user of having to mount it in addition to a the 
> extra (and duplicate) coding effort.
> 
> Before sysfs, proc filled that role.
> 
> What you're seeing in proc_file_write() isn't a bug but a limitation of 
> this simplified interface.  You can do multipage writes by using the more 
> general interface where you supply your own file operation methods.  (set 
> the proc_fops field in struct proc_dir_entry).
> 
> --
> Bryan Henderson                          IBM Almaden Research Center
> San Jose CA                              Filesystems

That's it. Exactly what I needed - no need to change the kernel and very
simple, thank you.

Regarding the proc_file_write, it may not be an implementation bug but
then, the FIXME comment, telling that the programmer wasn't sure at all
when creating the interface, is confusing. I think, it should be
replaced by your advice :-)

	Petr Grillinger