Skip to contentSkip to conversation

proc_file_read bug?

DISCUSSION Thomas Hood 2 participants 3 messages Jan 25, 2002

lore
Share & tools
lore
#0 lore raw

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. - [email protected]
                 */
                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. - [email protected]
		 */
 		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;
}
#1 lore raw

Thomas Hood <[email protected]> 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. - [email protected]
|>                  */
|>                 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.

Signature · 4 lines
Andreas Schwab, SuSE Labs, [email protected]
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."
#2 lore raw
Quoted 7 lines · Andreas Schwab
|>                 /* 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. - [email protected]
|>                  */

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