[PATCH][RFC] Re: 2.4.15-pre5: /proc/cpuinfo broken

[PATCH][RFC] Re: 2.4.15-pre5: /proc/cpuinfo broken

[Alexander Viro]

On Sat, 17 Nov 2001, Wolfgang Wander wrote:

>   Actually its bash - 2.04.0(1)-release (i386-suse-linux)

2.03.0(1)-release (i386-pc-linux-gnu) (Debian variant) works here.

OK, let's see what can be done.  Situation is the same for all seq_file-based
files.

	a) we can revert to old code.  Which had a lot of problems, quite
a few of them unfixable if we want to have lseek() to calculated position.

	b) we can change i_mode for these files, making them S_IFIFO.
That will stop any silliness (if something would rely on seeking
backwards for something that looks like a pipe, it would break on
real pipes, etc. and that's not too likely to stay unnoticed, to put
it mildly).  And that's trivial to implement.

	c) hunt down and fix the userland that relies on arithmetics
on file position in case of regular files (POSIX prohibits it, SuS allows).

(a) - lots of bad problems, including unsolvable userland races.
(b) - user-visible change, but one that is very unlikely to break
anything.
(c) - yeah, right.  In the middle of 2.4.  Since such stuff exists and is
widely used, it's out of question.  Pity, but there's nothing to do about
that.

Frankly, I'd prefer to try (b) before reverting to (a).  Patch doing that
variant follows.  Linus, your opinion?

diff -urN S15-pre5/fs/proc/inode.c S15-pre5-proc/fs/proc/inode.c
--- S15-pre5/fs/proc/inode.c	Tue Oct  9 21:47:27 2001
+++ S15-pre5-proc/fs/proc/inode.c	Sat Nov 17 12:38:08 2001
@@ -160,14 +160,12 @@
 			inode->i_nlink = de->nlink;
 		if (de->owner)
 			__MOD_INC_USE_COUNT(de->owner);
-		if (S_ISBLK(de->mode)||S_ISCHR(de->mode)||S_ISFIFO(de->mode))
+		if (de->proc_iops)
+			inode->i_op = de->proc_iops;
+		if (de->proc_fops)
+			inode->i_fop = de->proc_fops;
+		else if (S_ISBLK(de->mode)||S_ISCHR(de->mode)||S_ISFIFO(de->mode))
 			init_special_inode(inode,de->mode,kdev_t_to_nr(de->rdev));
-		else {
-			if (de->proc_iops)
-				inode->i_op = de->proc_iops;
-			if (de->proc_fops)
-				inode->i_fop = de->proc_fops;
-		}
 	}
 
 out:
diff -urN S15-pre5/fs/proc/proc_misc.c S15-pre5-proc/fs/proc/proc_misc.c
--- S15-pre5/fs/proc/proc_misc.c	Thu Nov 15 23:43:07 2001
+++ S15-pre5-proc/fs/proc/proc_misc.c	Sat Nov 17 12:38:15 2001
@@ -519,6 +519,14 @@
 
 struct proc_dir_entry *proc_root_kcore;
 
+static void create_seq_entry(char *name, mode_t mode, struct file_operations *f)
+{
+	struct proc_dir_entry *entry;
+	entry = create_proc_entry(name, mode|S_IFIFO, NULL);
+	if (entry)
+		entry->proc_fops = f;
+}
+
 void __init proc_misc_init(void)
 {
 	struct proc_dir_entry *entry;
@@ -568,16 +576,10 @@
 	entry = create_proc_entry("kmsg", S_IRUSR, &proc_root);
 	if (entry)
 		entry->proc_fops = &proc_kmsg_operations;
-	entry = create_proc_entry("mounts", 0, NULL);
-	if (entry)
-		entry->proc_fops = &proc_mounts_operations;
-	entry = create_proc_entry("cpuinfo", 0, NULL);
-	if (entry)
-		entry->proc_fops = &proc_cpuinfo_operations;
+	create_seq_entry("mounts", 0, &proc_mounts_operations);
+	create_seq_entry("cpuinfo", 0, &proc_cpuinfo_operations);
 #ifdef CONFIG_MODULES
-	entry = create_proc_entry("ksyms", 0, NULL);
-	if (entry)
-		entry->proc_fops = &proc_ksyms_operations;
+	create_seq_entry("cpuinfo", 0, &proc_ksyms_operations);
 #endif
 	proc_root_kcore = create_proc_entry("kcore", S_IRUSR, NULL);
 	if (proc_root_kcore) {

Re: [PATCH][RFC] Re: 2.4.15-pre5: /proc/cpuinfo broken

[Alan Cox]

> 	c) hunt down and fix the userland that relies on arithmetics
> on file position in case of regular files (POSIX prohibits it, SuS allows).

You forgot d.

(d) - when someone seeks in the file do the seek, and document that they
lose their guarantees. So they fall back to existing 1.0->2.4 behaviour.
You just run the iterator either on or back from scratch to the seek point.

Alan

Re: [PATCH][RFC] Re: 2.4.15-pre5: /proc/cpuinfo broken

[Alexander Viro]

On Sat, 17 Nov 2001, Alan Cox wrote:

> > 	c) hunt down and fix the userland that relies on arithmetics
> > on file position in case of regular files (POSIX prohibits it, SuS allows).
> 
> You forgot d.
> 
> (d) - when someone seeks in the file do the seek, and document that they
> lose their guarantees. So they fall back to existing 1.0->2.4 behaviour.
> You just run the iterator either on or back from scratch to the seek point.

Umm...  In principle doable, but then we are losing anything resembling
sane behaviour on seek to remembered position.  OTOH, it's weak anyway
and saying that it's FIFO and trying to squeeze something from lseek()
is not too attractive.

I'll do that variant - it will be local to file/seq_file.c.  I'll give it
some beating and send it - hopefully in an hour or two.

[PATCH][CFT] seq_file and lseek()

[Alexander Viro]

On Sat, 17 Nov 2001, Alexander Viro wrote:

> On Sat, 17 Nov 2001, Alan Cox wrote:
> 
> > (d) - when someone seeks in the file do the seek, and document that they
> > lose their guarantees. So they fall back to existing 1.0->2.4 behaviour.
> > You just run the iterator either on or back from scratch to the seek point.
> 
> Umm...  In principle doable, but then we are losing anything resembling
> sane behaviour on seek to remembered position.  OTOH, it's weak anyway
> and saying that it's FIFO and trying to squeeze something from lseek()
> is not too attractive.
> 
> I'll do that variant - it will be local to file/seq_file.c.  I'll give it
> some beating and send it - hopefully in an hour or two.

OK, it seems to be working.  Now what used to be ->f_pos is m->index and
->f_pos is counted in bytes.  IOW, read() works exactly as it used to,
but lseek() anywhere other than current position walks through the
thing doing show() and counting bytes.

It's pretty straightforward and seems to be working here.  Help with
testing and review would be very welcome.

diff -urN S15-pre5/fs/seq_file.c S15-pre5-proc/fs/seq_file.c
--- S15-pre5/fs/seq_file.c	Thu Nov 15 23:43:07 2001
+++ S15-pre5-proc/fs/seq_file.c	Sat Nov 17 18:25:42 2001
@@ -73,13 +73,13 @@
 		buf += n;
 		copied += n;
 		if (!m->count)
-			(*ppos)++;
+			m->index++;
 		if (!size)
 			goto Done;
 	}
 	/* we need at least one record in buffer */
 	while (1) {
-		pos = *ppos;
+		pos = m->index;
 		p = m->op->start(m, &pos);
 		err = PTR_ERR(p);
 		if (!p || IS_ERR(p))
@@ -125,10 +125,12 @@
 		m->from = n;
 	else
 		pos++;
-	*ppos = pos;
+	m->index = pos;
 Done:
 	if (!copied)
 		copied = err;
+	else
+		*ppos += copied;
 	up(&m->sem);
 	return copied;
 Enomem:
@@ -139,6 +141,54 @@
 	goto Done;
 }
 
+static int traverse(struct seq_file *m, loff_t offset)
+{
+	loff_t pos = 0;
+	int error = 0;
+	void *p;
+
+	m->index = 0;
+	m->count = m->from = 0;
+	if (!offset)
+		return 0;
+	if (!m->buf) {
+		m->buf = kmalloc(m->size = PAGE_SIZE, GFP_KERNEL);
+		if (!m->buf)
+			return -ENOMEM;
+	}
+	p = m->op->start(m, &m->index);
+	while (p) {
+		error = PTR_ERR(p);
+		if (IS_ERR(p))
+			break;
+		error = m->op->show(m, p);
+		if (error)
+			break;
+		if (m->count == m->size)
+			goto Eoverflow;
+		if (pos + m->count > offset) {
+			m->from = offset - pos;
+			m->count -= m->from;
+			break;
+		}
+		pos += m->count;
+		m->count = 0;
+		if (pos == offset) {
+			m->index++;
+			break;
+		}
+		p = m->op->next(m, p, &m->index);
+	}
+	m->op->stop(m, p);
+	return error;
+
+Eoverflow:
+	m->op->stop(m, p);
+	kfree(m->buf);
+	m->buf = kmalloc(m->size <<= 1, GFP_KERNEL);
+	return !m->buf ? -ENOMEM : -EAGAIN;
+}
+
 /**
  *	seq_lseek -	->llseek() method for sequential files.
  *	@file, @offset, @origin: see file_operations method
@@ -157,11 +207,19 @@
 		case 0:
 			if (offset < 0)
 				break;
+			retval = offset;
 			if (offset != file->f_pos) {
-				file->f_pos = offset;
-				m->count = 0;
+				while ((retval=traverse(m, offset)) == -EAGAIN)
+					;
+				if (retval) {
+					/* with extreme perjudice... */
+					file->f_pos = 0;
+					m->index = 0;
+					m->count = 0;
+				} else {
+					retval = file->f_pos = offset;
+				}
 			}
-			retval = offset;
 	}
 	up(&m->sem);
 	return retval;

Re: [PATCH][RFC] Re: 2.4.15-pre5: /proc/cpuinfo broken

[Linus Torvalds]

On Sat, 17 Nov 2001, Alexander Viro wrote:
>
> Frankly, I'd prefer to try (b) before reverting to (a).  Patch doing that
> variant follows.  Linus, your opinion?

(d) make seq_file have my originally suggested "subposition" code.

Ie make the X low bits of "pos" be the position in the record, with the
high bits of "pos" being the current "record index" kind of thing.

That makes lseek() happy.

		Linus

Re: [PATCH][RFC] Re: 2.4.15-pre5: /proc/cpuinfo broken

[Alexander Viro]

On Sat, 17 Nov 2001, Linus Torvalds wrote:

> 
> On Sat, 17 Nov 2001, Alexander Viro wrote:
> >
> > Frankly, I'd prefer to try (b) before reverting to (a).  Patch doing that
> > variant follows.  Linus, your opinion?
> 
> (d) make seq_file have my originally suggested "subposition" code.
> 
> Ie make the X low bits of "pos" be the position in the record, with the
> high bits of "pos" being the current "record index" kind of thing.
> 
> That makes lseek() happy.

It will not help.  lseek() in question is relative and crosses the
record boundary.  I.e. we have

	n = read(fd, buf, ...);
	/* process k bytes */
	lseek(fd, k-n, SEEK_CUR);

and that will break just as the current variant does.  It's not about
seek to remembered position - it's a relative seek to calculated offset.
Calculated from number of bytes returned by read().

Re: [PATCH][RFC] Re: 2.4.15-pre5: /proc/cpuinfo broken

[H. Peter Anvin]

Followup to:  <Pine.GSO.4.21.0111171359410.11475-100000@weyl.math.psu.edu>
By author:    Alexander Viro <viro@math.psu.edu>
In newsgroup: linux.dev.kernel
> 
> On Sat, 17 Nov 2001, Linus Torvalds wrote:
> 
> > 
> > On Sat, 17 Nov 2001, Alexander Viro wrote:
> > >
> > > Frankly, I'd prefer to try (b) before reverting to (a).  Patch doing that
> > > variant follows.  Linus, your opinion?
> > 
> > (d) make seq_file have my originally suggested "subposition" code.
> > 
> > Ie make the X low bits of "pos" be the position in the record, with the
> > high bits of "pos" being the current "record index" kind of thing.
> > 
> > That makes lseek() happy.
> 
> It will not help.  lseek() in question is relative and crosses the
> record boundary.  I.e. we have
> 
> 	n = read(fd, buf, ...);
> 	/* process k bytes */
> 	lseek(fd, k-n, SEEK_CUR);
> 
> and that will break just as the current variant does.  It's not about
> seek to remembered position - it's a relative seek to calculated offset.
> Calculated from number of bytes returned by read().
> 

We may really want to consider if we want /proc entries to be
S_IFREG().  The closest equivalent I can think of is really a
character device node (S_IFCHR) more so that S_IFIFO.

	  -hpa

-- 
<hpa@transmeta.com> at work, <hpa@zytor.com> in private!
"Unix gives you enough rope to shoot yourself in the foot."
http://www.zytor.com/~hpa/puzzle.txt	<amsp@zytor.com>