proc_file_read bug?

proc_file_read bug?

[Thomas Hood]

I don't understand this part of proc_file_read() in 
fs/proc/generic.c:

                /* This is a hack to allow mangling of file pos independent
                 * of actual bytes read.  Simply place the data at page,
                 * return the bytes, and set `start' to the desired offset
                 * as an unsigned int. - Paul.Russell@rustcorp.com.au
                 */
                n -= copy_to_user(buf, start < page ? page : start, n);
                if (n == 0) {
                        if (retval == 0)
                                retval = -EFAULT;
                        break;
                }

                *ppos += start < page ? (long)start : n; /* Move down the file */
                nbytes -= n;
                buf += n;
                retval += n;

When start >= page, we copy n bytes beginning at start and
increase *ppos by n.  Makes sense.  But what happens when
start < page?  We will copy n bytes starting at page, then
increase *ppos by start!!  What sense does that make?  If
there's cleverness happening here, someone please document it.

For your convenience, I append the whole existing proc_file_read
function below.
--
Thomas

static ssize_t
proc_file_read(struct file * file, char * buf, size_t nbytes, loff_t *ppos)
{
	struct inode * inode = file->f_dentry->d_inode;
	char 	*page;
	ssize_t	retval=0;
	int	eof=0;
	ssize_t	n, count;
	char	*start;
	struct proc_dir_entry * dp;

	dp = (struct proc_dir_entry *) inode->u.generic_ip;
	if (!(page = (char*) __get_free_page(GFP_KERNEL)))
		return -ENOMEM;

	while ((nbytes > 0) && !eof)
	{
		count = MIN(PROC_BLOCK_SIZE, nbytes);

		start = NULL;
		if (dp->get_info) {
			/*
			 * Handle backwards compatibility with the old net
			 * routines.
			 */
			n = dp->get_info(page, &start, *ppos, count);
			if (n < count)
				eof = 1;
		} else if (dp->read_proc) {
			n = dp->read_proc(page, &start, *ppos,
					  count, &eof, dp->data);
		} else
			break;

		if (!start) {
			/*
			 * For proc files that are less than 4k
			 */
			start = page + *ppos;
			n -= *ppos;
			if (n <= 0)
				break;
			if (n > count)
				n = count;
		}
		if (n == 0)
			break;	/* End of file */
		if (n < 0) {
			if (retval == 0)
				retval = n;
			break;
		}
		
		/* This is a hack to allow mangling of file pos independent
 		 * of actual bytes read.  Simply place the data at page,
 		 * return the bytes, and set `start' to the desired offset
 		 * as an unsigned int. - Paul.Russell@rustcorp.com.au
		 */
 		n -= copy_to_user(buf, start < page ? page : start, n);
		if (n == 0) {
			if (retval == 0)
				retval = -EFAULT;
			break;
		}

		*ppos += start < page ? (long)start : n; /* Move down the file */
		nbytes -= n;
		buf += n;
		retval += n;
	}
	free_page((unsigned long) page);
	return retval;
}

Re: proc_file_read bug?

[Andreas Schwab]

Thomas Hood <jdthood@mail.com> writes:

|> I don't understand this part of proc_file_read() in 
|> fs/proc/generic.c:
|> 
|>                 /* This is a hack to allow mangling of file pos independent
|>                  * of actual bytes read.  Simply place the data at page,
|>                  * return the bytes, and set `start' to the desired offset
|>                  * as an unsigned int. - Paul.Russell@rustcorp.com.au
|>                  */
|>                 n -= copy_to_user(buf, start < page ? page : start, n);
|>                 if (n == 0) {
|>                         if (retval == 0)
|>                                 retval = -EFAULT;
|>                         break;
|>                 }
|> 
|>                 *ppos += start < page ? (long)start : n; /* Move down the file */
|>                 nbytes -= n;
|>                 buf += n;
|>                 retval += n;
|> 
|> When start >= page, we copy n bytes beginning at start and
|> increase *ppos by n.  Makes sense.  But what happens when
|> start < page?  We will copy n bytes starting at page, then
|> increase *ppos by start!!  What sense does that make?  If
|> there's cleverness happening here, someone please document it.

It is documented, RTFC.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE GmbH, Deutschherrnstr. 15-19, D-90429 Nürnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: proc_file_read bug?

[Thomas Hood]

On Fri, 2002-01-25 at 09:29, Andreas Schwab wrote:
> |>                 /* This is a hack to allow mangling of file pos independent
> |>                  * of actual bytes read.  Simply place the data at page,
> |>                  * return the bytes, and set `start' to the desired offset
> |>                  * as an unsigned int. - Paul.Russell@rustcorp.com.au
> |>                  */

> It is documented, RTFC.

Comment or Code?  The comment is somewhat ambiguous and incorrect.

Reading the code, I take it that "start" is either a pointer into
the buffer where the string of n data bytes starts, or else
(when it is assigned a value less than the beginning of the buffer)
it is a special value by which the file offset is to be adjusted,
instead of n.  Thus the comment might be clarified:
    /* 
     * This is a hack to allow adjusting the file offset
     * by a number different from the number of bytes read.
     * Simply place the data at page, return the number of
     * bytes read, and set "start" to the (signed long) amount
     * by which the file offset is to be increased or
     * decreased
     */

My question then is: why would one want to adjust the file
offset other than by +n?

--
Thomas

Re: proc_file_read bug?

[Thomas Hood]

I discovered that this question is fully answered in
chapter 4 of Linux Device Drivers, 2nd Edition, by
Alessandro Rubini & Jonathan Corbet.  Excellent book.

This "hack" seems to me rather unfortunate.  It makes
a single argument serve two completely different and
not mutually exclusive purposes.  It means that when I
want to override the file offset increment I can't set the
data start position within the buffer, and vice versa.
Overloading "start" in this way also sets an arbitrary
and randomly varying upper limit on the overriding file
offset increment: it can't be equal to or greater than
the address at which the data buffer happens to start.
(It was the seeming irrationality of this that led me
to wonder earlier whether or not the code contained a bug.)

It might have been better to add a new argument to the
read function for the purpose of returning offset increase
overrides.