Migration of kernel interfaces to seq_files breaks pread() consumers

Migration of kernel interfaces to seq_files breaks pread() consumers

[Paul Turner]

(Specifically) Several interfaces under /proc have been migrated to use 
seq_files.  This was previously observed to be a problem with VMware's 
reading of /proc/uptime.  We're now running into the same problem on  
/proc/<pid>/stat; we have many consumers performing preads on this 
interface which break under new kernels.

Reverting these migrations presents other problems and doesn't scale with 
everyones' pet dependencies over an abi that's been
broken :(

Part of the problem in implementing pread in seq_files is that we don't  
know know whether the read was issued by pread(2) or read(2).  It's not 
nice to shoehorn this information down the stack.  I've attached a 
skeleton patch which shows one way we could push it up (although something 
like a second f_pos would be necessary to make it maintain pread 
semantics against reads).

One advantage of this style of approach is that it doesn't break on 
partial record reads.  But it's a little gross at the same time.

Comments?  Ideas? 

Thanks,

- Paul

--
 fs/read_write.c    |   10 ++++++++++
 fs/seq_file.c      |    2 ++
 include/linux/fs.h |    2 ++
 3 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/fs/read_write.c b/fs/read_write.c
index 2fc2980..744094a 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -407,6 +407,16 @@ asmlinkage ssize_t sys_pread64(unsigned int fd, char __user *buf,
 		ret = -ESPIPE;
 		if (file->f_mode & FMODE_PREAD)
 			ret = vfs_read(file, buf, count, &pos);
+		else if (file->f_mode & FMODE_SEQ_FILE) {
+			/*
+			 * We break the pread semantic and actually make it
+			 * seek, this prevents inconsistent record reads across
+			 * boundaries.
+			 */
+			vfs_llseek(file, pos, SEEK_SET);
+			ret = vfs_read(file, buf, count, &pos);
+			file_pos_write(file, pos);
+		}
 		fput_light(file, fput_needed);
 	}
 
diff --git a/fs/seq_file.c b/fs/seq_file.c
index 3f54dbd..f8c5379 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -50,6 +50,8 @@ int seq_open(struct file *file, const struct seq_operations *op)
 
 	/* SEQ files support lseek, but not pread/pwrite */
 	file->f_mode &= ~(FMODE_PREAD | FMODE_PWRITE);
+	file->f_mode |= FMODE_SEQ_FILE;
+
 	return 0;
 }
 EXPORT_SYMBOL(seq_open);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 5f7b912..c3b5916 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -76,6 +76,8 @@ extern int dir_notify_enable;
    behavior for cross-node execution/opening_for_writing of files */
 #define FMODE_EXEC	16
 
+#define FMODE_SEQ_FILE_PREAD	32
+
 #define RW_MASK		1
 #define RWA_MASK	2
 #define READ 0

Re: Migration of kernel interfaces to seq_files breaks pread() consumers

[Andrew Morton]

On Fri, 16 Jan 2009 23:51:35 -0800 (PST) Paul Turner <pjt@google.com> wrote:

> 
> (Specifically) Several interfaces under /proc have been migrated to use 
> seq_files.  This was previously observed to be a problem with VMware's 
> reading of /proc/uptime.  We're now running into the same problem on  
> /proc/<pid>/stat; we have many consumers performing preads on this 
> interface which break under new kernels.
> 
> Reverting these migrations presents other problems and doesn't scale with 
> everyones' pet dependencies over an abi that's been
> broken :(

We changed userspace-visible behaviour and broke real applications. 
This is a serious matter.  So serious in fact that your report has
languished without reply for a week.

Reverting those changes until we have a suitable reimplementation which
doesn't bust userspace is 100% justifiable.

In which kernel versions is this regression present?

What would a revert look like?  Big and ugly or small and simple?  Do
the original commits (which were they?) still revert OK?

> Part of the problem in implementing pread in seq_files is that we don't  
> know know whether the read was issued by pread(2) or read(2).  It's not 
> nice to shoehorn this information down the stack.  I've attached a 
> skeleton patch which shows one way we could push it up (although something 
> like a second f_pos would be necessary to make it maintain pread 
> semantics against reads).
> 
> One advantage of this style of approach is that it doesn't break on 
> partial record reads.  But it's a little gross at the same time.
> 

Yes, that is a bit gross.

Does this patch actually 100% solve the problem, or is it a precursor
to some other fix or what?  It's hard to comment sensibly if it's a
partial thing with no sign how it will be used.

> diff --git a/fs/read_write.c b/fs/read_write.c
> index 2fc2980..744094a 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -407,6 +407,16 @@ asmlinkage ssize_t sys_pread64(unsigned int fd, char __user *buf,
>  		ret = -ESPIPE;
>  		if (file->f_mode & FMODE_PREAD)
>  			ret = vfs_read(file, buf, count, &pos);
> +		else if (file->f_mode & FMODE_SEQ_FILE) {
> +			/*
> +			 * We break the pread semantic and actually make it
> +			 * seek, this prevents inconsistent record reads across
> +			 * boundaries.
> +			 */
> +			vfs_llseek(file, pos, SEEK_SET);
> +			ret = vfs_read(file, buf, count, &pos);
> +			file_pos_write(file, pos);
> +		}

Well yes, that's a userspace-visible wrong change too.

>  		fput_light(file, fput_needed);
>  	}
>  
> diff --git a/fs/seq_file.c b/fs/seq_file.c
> index 3f54dbd..f8c5379 100644
> --- a/fs/seq_file.c
> +++ b/fs/seq_file.c
> @@ -50,6 +50,8 @@ int seq_open(struct file *file, const struct seq_operations *op)
>  
>  	/* SEQ files support lseek, but not pread/pwrite */
>  	file->f_mode &= ~(FMODE_PREAD | FMODE_PWRITE);
> +	file->f_mode |= FMODE_SEQ_FILE;
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL(seq_open);
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 5f7b912..c3b5916 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -76,6 +76,8 @@ extern int dir_notify_enable;
>     behavior for cross-node execution/opening_for_writing of files */
>  #define FMODE_EXEC	16
>  
> +#define FMODE_SEQ_FILE_PREAD	32

-EWONTCOMPILE, btw.

Re: Migration of kernel interfaces to seq_files breaks pread() consumers

[Paul Turner]

On Sat, Jan 24, 2009 at 6:19 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Fri, 16 Jan 2009 23:51:35 -0800 (PST) Paul Turner <pjt@google.com> wrote:
>
>>
>> (Specifically) Several interfaces under /proc have been migrated to use
>> seq_files.  This was previously observed to be a problem with VMware's
>> reading of /proc/uptime.  We're now running into the same problem on
>> /proc/<pid>/stat; we have many consumers performing preads on this
>> interface which break under new kernels.
>>
>> Reverting these migrations presents other problems and doesn't scale with
>> everyones' pet dependencies over an abi that's been
>> broken :(
>
> We changed userspace-visible behaviour and broke real applications.
> This is a serious matter.  So serious in fact that your report has
> languished without reply for a week.
>
> Reverting those changes until we have a suitable reimplementation which
> doesn't bust userspace is 100% justifiable.
>
> In which kernel versions is this regression present?

Commit is ee992744ea53db0a90c986fd0a70fbbf91e7f8bd, merged with 2.6.25

>
> What would a revert look like?  Big and ugly or small and simple?  Do
> the original commits (which were they?) still revert OK?
>

There is a race on namespaces that would have to be resolved.  From commit:
"... Currently (as pointed out by Oleg) do_task_stat has a race when calling
   task_pid_nr_ns with the task exiting.  In addition do_task_stat is not
   currently displaying information in the context of the pid namespace that
   mounted the /proc filesystem.  So "cut -d' ' -f 1 /proc/<pid>/stat" may not
   equal <pid>. ..."


>> Part of the problem in implementing pread in seq_files is that we don't
>> know know whether the read was issued by pread(2) or read(2).  It's not
>> nice to shoehorn this information down the stack.  I've attached a
>> skeleton patch which shows one way we could push it up (although something
>> like a second f_pos would be necessary to make it maintain pread
>> semantics against reads).
>>
>> One advantage of this style of approach is that it doesn't break on
>> partial record reads.  But it's a little gross at the same time.
>>
>
> Yes, that is a bit gross.
>
> Does this patch actually 100% solve the problem, or is it a precursor
> to some other fix or what?  It's hard to comment sensibly if it's a
> partial thing with no sign how it will be used.
>

It's not fully robust, the case of two simultaneous preaders on the
same fd isn't something I have any nice answer for given the nature of
seq_files.

Apart from that, as long as we maintain a separate f_pos for the
preads it should be ok.

>> diff --git a/fs/read_write.c b/fs/read_write.c
>> index 2fc2980..744094a 100644
>> --- a/fs/read_write.c
>> +++ b/fs/read_write.c
>> @@ -407,6 +407,16 @@ asmlinkage ssize_t sys_pread64(unsigned int fd, char __user *buf,
>>               ret = -ESPIPE;
>>               if (file->f_mode & FMODE_PREAD)
>>                       ret = vfs_read(file, buf, count, &pos);
>> +             else if (file->f_mode & FMODE_SEQ_FILE) {
>> +                     /*
>> +                      * We break the pread semantic and actually make it
>> +                      * seek, this prevents inconsistent record reads across
>> +                      * boundaries.
>> +                      */
>> +                     vfs_llseek(file, pos, SEEK_SET);
>> +                     ret = vfs_read(file, buf, count, &pos);
>> +                     file_pos_write(file, pos);
>> +             }
>
> Well yes, that's a userspace-visible wrong change too.

Yes -- I mentioned above that to make this into a 'real' patch and
remain not perturb reads we'd need to maintain a second file position
for the preader

But this is getting farther up the ugly tree, I was hoping someone
else might have a more palatable idea :)

>
>>               fput_light(file, fput_needed);
>>       }
>>
>> diff --git a/fs/seq_file.c b/fs/seq_file.c
>> index 3f54dbd..f8c5379 100644
>> --- a/fs/seq_file.c
>> +++ b/fs/seq_file.c
>> @@ -50,6 +50,8 @@ int seq_open(struct file *file, const struct seq_operations *op)
>>
>>       /* SEQ files support lseek, but not pread/pwrite */
>>       file->f_mode &= ~(FMODE_PREAD | FMODE_PWRITE);
>> +     file->f_mode |= FMODE_SEQ_FILE;
>> +
>>       return 0;
>>  }
>>  EXPORT_SYMBOL(seq_open);
>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>> index 5f7b912..c3b5916 100644
>> --- a/include/linux/fs.h
>> +++ b/include/linux/fs.h
>> @@ -76,6 +76,8 @@ extern int dir_notify_enable;
>>     behavior for cross-node execution/opening_for_writing of files */
>>  #define FMODE_EXEC   16
>>
>> +#define FMODE_SEQ_FILE_PREAD 32
>
> -EWONTCOMPILE, btw.
>

Re: Migration of kernel interfaces to seq_files breaks pread() consumers

[Alexey Dobriyan]

On Sat, Jan 24, 2009 at 06:19:24PM -0800, Andrew Morton wrote:
> On Fri, 16 Jan 2009 23:51:35 -0800 (PST) Paul Turner <pjt@google.com> wrote:
> 
> > 
> > (Specifically) Several interfaces under /proc have been migrated to use 
> > seq_files.  This was previously observed to be a problem with VMware's 
> > reading of /proc/uptime.  We're now running into the same problem on  
> > /proc/<pid>/stat; we have many consumers performing preads on this 
> > interface which break under new kernels.
> > 
> > Reverting these migrations presents other problems and doesn't scale with 
> > everyones' pet dependencies over an abi that's been
> > broken :(
> 
> We changed userspace-visible behaviour and broke real applications. 
> This is a serious matter.  So serious in fact that your report has
> languished without reply for a week.
> 
> Reverting those changes until we have a suitable reimplementation which
> doesn't bust userspace is 100% justifiable.
> 
> In which kernel versions is this regression present?
> 
> What would a revert look like?  Big and ugly or small and simple?  Do
> the original commits (which were they?) still revert OK?

This is bug http://bugzilla.kernel.org/show_bug.cgi?id=11856
Some of us think what to do here.

Original patch not revertable as is.

Re: Migration of kernel interfaces to seq_files breaks pread() consumers

[Eric W. Biederman]

Alexey Dobriyan <adobriyan@gmail.com> writes:

> On Sat, Jan 24, 2009 at 06:19:24PM -0800, Andrew Morton wrote:
>> On Fri, 16 Jan 2009 23:51:35 -0800 (PST) Paul Turner <pjt@google.com> wrote:
>> 
>> > 
>> > (Specifically) Several interfaces under /proc have been migrated to use 
>> > seq_files.  This was previously observed to be a problem with VMware's 
>> > reading of /proc/uptime.  We're now running into the same problem on  
>> > /proc/<pid>/stat; we have many consumers performing preads on this 
>> > interface which break under new kernels.
>> > 
>> > Reverting these migrations presents other problems and doesn't scale with 
>> > everyones' pet dependencies over an abi that's been
>> > broken :(
>> 
>> We changed userspace-visible behaviour and broke real applications. 
>> This is a serious matter.  So serious in fact that your report has
>> languished without reply for a week.
>> 
>> Reverting those changes until we have a suitable reimplementation which
>> doesn't bust userspace is 100% justifiable.
>> 
>> In which kernel versions is this regression present?
>> 
>> What would a revert look like?  Big and ugly or small and simple?  Do
>> the original commits (which were they?) still revert OK?
>
> This is bug http://bugzilla.kernel.org/show_bug.cgi?id=11856
> Some of us think what to do here.
>
> Original patch not revertable as is.

Interesting.  This seems like a bug in seq_file plain and simple.
Userspace appears to be acting very reasonable in this case.

Why is there a notion that we have to differentiate between read
and pread in seq_file to fix this.  That doesn't make much sense.

Anyway here is an untested but logically correct patch which should fix
this issue, without nasty special casing of pread.

Eric

[PATCH 1/2] seq_file: Move traverse so it can be used from seq_read

[Eric W. Biederman]

To handle pread correctly we can't assume that f_pos is
where we left it in seq_read.  So modify traverse
so we can use it in seq_read.

Signed-off-by: Eric Biederman <ebiederm@xmission.com>
---
 fs/seq_file.c |  114 ++++++++++++++++++++++++++++----------------------------
 1 files changed, 57 insertions(+), 57 deletions(-)

diff --git a/fs/seq_file.c b/fs/seq_file.c
index b569ff1..2716c12 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -54,6 +54,63 @@ int seq_open(struct file *file, const struct seq_operations *op)
 }
 EXPORT_SYMBOL(seq_open);
 
+static int traverse(struct seq_file *m, loff_t offset)
+{
+	loff_t pos = 0, index;
+	int error = 0;
+	void *p;
+
+	m->version = 0;
+	index = 0;
+	m->count = m->from = 0;
+	if (!offset) {
+		m->index = index;
+		return 0;
+	}
+	if (!m->buf) {
+		m->buf = kmalloc(m->size = PAGE_SIZE, GFP_KERNEL);
+		if (!m->buf)
+			return -ENOMEM;
+	}
+	p = m->op->start(m, &index);
+	while (p) {
+		error = PTR_ERR(p);
+		if (IS_ERR(p))
+			break;
+		error = m->op->show(m, p);
+		if (error < 0)
+			break;
+		if (unlikely(error)) {
+			error = 0;
+			m->count = 0;
+		}
+		if (m->count == m->size)
+			goto Eoverflow;
+		if (pos + m->count > offset) {
+			m->from = offset - pos;
+			m->count -= m->from;
+			m->index = index;
+			break;
+		}
+		pos += m->count;
+		m->count = 0;
+		if (pos == offset) {
+			index++;
+			m->index = index;
+			break;
+		}
+		p = m->op->next(m, p, &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_read -	->read() method for sequential files.
  *	@file: the file to read from
@@ -186,63 +243,6 @@ Efault:
 }
 EXPORT_SYMBOL(seq_read);
 
-static int traverse(struct seq_file *m, loff_t offset)
-{
-	loff_t pos = 0, index;
-	int error = 0;
-	void *p;
-
-	m->version = 0;
-	index = 0;
-	m->count = m->from = 0;
-	if (!offset) {
-		m->index = index;
-		return 0;
-	}
-	if (!m->buf) {
-		m->buf = kmalloc(m->size = PAGE_SIZE, GFP_KERNEL);
-		if (!m->buf)
-			return -ENOMEM;
-	}
-	p = m->op->start(m, &index);
-	while (p) {
-		error = PTR_ERR(p);
-		if (IS_ERR(p))
-			break;
-		error = m->op->show(m, p);
-		if (error < 0)
-			break;
-		if (unlikely(error)) {
-			error = 0;
-			m->count = 0;
-		}
-		if (m->count == m->size)
-			goto Eoverflow;
-		if (pos + m->count > offset) {
-			m->from = offset - pos;
-			m->count -= m->from;
-			m->index = index;
-			break;
-		}
-		pos += m->count;
-		m->count = 0;
-		if (pos == offset) {
-			index++;
-			m->index = index;
-			break;
-		}
-		p = m->op->next(m, p, &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: the file in question
-- 
1.5.6.3

[PATCH 2/2] seq_file: Properly cope with pread

[Eric W. Biederman]

Currently seq_read assumes that the offset passed
to it is always the offset it passed to user space.
In the case pread this assumption is broken and we
do the wrong thing when presented with pread.

To solve this I introduce an offset cache inside of
struct seq_file so we know where our logical file position
is.  Then in seq_read if we try to read from another
offset we reset our data structures and attempt
to go to the offset user space wanted.

Signed-off-by: Eric Biederman <ebiederm@xmission.com>
---
 fs/seq_file.c            |   24 ++++++++++++++++++++++--
 include/linux/seq_file.h |    1 +
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/fs/seq_file.c b/fs/seq_file.c
index 2716c12..cd63d69 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -130,6 +130,22 @@ ssize_t seq_read(struct file *file, char __user *buf, size_t size, loff_t *ppos)
 	int err = 0;
 
 	mutex_lock(&m->lock);
+
+	/* Don't assume *ppos is where we left it */
+	if (unlikely(*ppos != m->read_pos)) {
+		m->read_pos = *ppos;
+		while ((err = traverse(m, *ppos)) == -EAGAIN)
+			;
+		if (err) {
+			/* With prejudice... */
+			m->read_pos = 0;
+			m->version = 0;
+			m->index = 0;
+			m->count = 0;
+			goto Done;
+		}
+	}
+
 	/*
 	 * seq_file->op->..m_start/m_stop/m_next may do special actions
 	 * or optimisations based on the file->f_version, so we want to
@@ -229,8 +245,10 @@ Fill:
 Done:
 	if (!copied)
 		copied = err;
-	else
+	else {
 		*ppos += copied;
+		m->read_pos += copied;
+	}
 	file->f_version = m->version;
 	mutex_unlock(&m->lock);
 	return copied;
@@ -265,16 +283,18 @@ loff_t seq_lseek(struct file *file, loff_t offset, int origin)
 			if (offset < 0)
 				break;
 			retval = offset;
-			if (offset != file->f_pos) {
+			if (offset != m->read_pos) {
 				while ((retval=traverse(m, offset)) == -EAGAIN)
 					;
 				if (retval) {
 					/* with extreme prejudice... */
 					file->f_pos = 0;
+					m->read_pos = 0;
 					m->version = 0;
 					m->index = 0;
 					m->count = 0;
 				} else {
+					m->read_pos = offset;
 					retval = file->f_pos = offset;
 				}
 			}
diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h
index 40ea505..f616f31 100644
--- a/include/linux/seq_file.h
+++ b/include/linux/seq_file.h
@@ -19,6 +19,7 @@ struct seq_file {
 	size_t from;
 	size_t count;
 	loff_t index;
+	loff_t read_pos;
 	u64 version;
 	struct mutex lock;
 	const struct seq_operations *op;
-- 
1.5.6.3

Re: Migration of kernel interfaces to seq_files breaks pread() consumers

[Paul Turner]

On Tue, 27 Jan 2009, Eric W. Biederman wrote:

> Alexey Dobriyan <adobriyan@gmail.com> writes:
> 
> > On Sat, Jan 24, 2009 at 06:19:24PM -0800, Andrew Morton wrote:
> >> On Fri, 16 Jan 2009 23:51:35 -0800 (PST) Paul Turner <pjt@google.com> wrote:
> >> 
> >> > 
> >> > (Specifically) Several interfaces under /proc have been migrated to use 
> >> > seq_files.  This was previously observed to be a problem with VMware's 
> >> > reading of /proc/uptime.  We're now running into the same problem on  
> >> > /proc/<pid>/stat; we have many consumers performing preads on this 
> >> > interface which break under new kernels.
> >> > 
> >> > Reverting these migrations presents other problems and doesn't scale with 
> >> > everyones' pet dependencies over an abi that's been
> >> > broken :(
> >> 
> >> We changed userspace-visible behaviour and broke real applications. 
> >> This is a serious matter.  So serious in fact that your report has
> >> languished without reply for a week.
> >> 
> >> Reverting those changes until we have a suitable reimplementation which
> >> doesn't bust userspace is 100% justifiable.
> >> 
> >> In which kernel versions is this regression present?
> >> 
> >> What would a revert look like?  Big and ugly or small and simple?  Do
> >> the original commits (which were they?) still revert OK?
> >
> > This is bug http://bugzilla.kernel.org/show_bug.cgi?id=11856
> > Some of us think what to do here.
> >
> > Original patch not revertable as is.
> 
> Interesting.  This seems like a bug in seq_file plain and simple.
> Userspace appears to be acting very reasonable in this case.
> 
> Why is there a notion that we have to differentiate between read
> and pread in seq_file to fix this.  That doesn't make much sense.
> 
> Anyway here is an untested but logically correct patch which should fix
> this issue, without nasty special casing of pread.
> 
> Eric
> 

Thanks Eric,

Moving the position into the seq_file structure is much cleaner.  Basic 
tests seem to work ok.

Few comments:
- seq_open needs its fmode opened up to take advantage of this [patch 
below]
- There's still the inherent problem of reads peturbing other reads (both 
read and pread).  Our applications should be ok with this, however it does 
deviate from the supposedly thread-safe pread semantics.

- Paul

---
 fs/seq_file.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/fs/seq_file.c b/fs/seq_file.c
index cd63d69..aa3621f 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -48,8 +48,6 @@ int seq_open(struct file *file, const struct seq_operations *op)
 	 */
 	file->f_version = 0;
 
-	/* SEQ files support lseek, but not pread/pwrite */
-	file->f_mode &= ~(FMODE_PREAD | FMODE_PWRITE);
 	return 0;
 }
 EXPORT_SYMBOL(seq_open);
-- 
1.5.4.5

Re: Migration of kernel interfaces to seq_files breaks pread() consumers

[Eric W. Biederman]

Paul Turner <pjt@google.com> writes:
>
> Thanks Eric,
>
> Moving the position into the seq_file structure is much cleaner.  Basic 
> tests seem to work ok.

Thanks.

> Few comments:
> - seq_open needs its fmode opened up to take advantage of this [patch 
> below]

Good point.
Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>
                                   

> - There's still the inherent problem of reads peturbing other reads (both 
> read and pread).  Our applications should be ok with this, however it does 
> deviate from the supposedly thread-safe pread semantics.

I don't see this not being thread safe.

If the data that is returned is stable.  We should not see anything
changing.  Otherwise it is all bets are off in any case.

Because seq_read happens under m->lock I don't see how there will
be anything thread unsafe.

The position that changes is just an internal implementation detail to
keep track of which data that we have cached.

>
> - Paul
>
> ---
>  fs/seq_file.c |    2 --
>  1 files changed, 0 insertions(+), 2 deletions(-)
>
> diff --git a/fs/seq_file.c b/fs/seq_file.c
> index cd63d69..aa3621f 100644
> --- a/fs/seq_file.c
> +++ b/fs/seq_file.c
> @@ -48,8 +48,6 @@ int seq_open(struct file *file, const struct seq_operations
> *op)
>  	 */
>  	file->f_version = 0;
>  
> -	/* SEQ files support lseek, but not pread/pwrite */
> -	file->f_mode &= ~(FMODE_PREAD | FMODE_PWRITE);
>  	return 0;
>  }
>  EXPORT_SYMBOL(seq_open);
> -- 
> 1.5.4.5

Re: Migration of kernel interfaces to seq_files breaks pread() consumers

[Paul Turner]

On Thu, Jan 29, 2009 at 7:01 PM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
> Paul Turner <pjt@google.com> writes:
>>
>> Thanks Eric,
>>
>> Moving the position into the seq_file structure is much cleaner.  Basic
>> tests seem to work ok.
>
> Thanks.
>
>> Few comments:
>> - seq_open needs its fmode opened up to take advantage of this [patch
>> below]
>
> Good point.
> Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>
>
>
>> - There's still the inherent problem of reads peturbing other reads (both
>> read and pread).  Our applications should be ok with this, however it does
>> deviate from the supposedly thread-safe pread semantics.
>
> I don't see this not being thread safe.
>
> If the data that is returned is stable.  We should not see anything
> changing.  Otherwise it is all bets are off in any case.
>

With just reads you had stability on the record level at least.
Concurrent reads/seeks on a file made it obvious where things weren't
thread safe.

Obviously userspace doing stupid things isn't a kernel problem but it
could become one in the future if a previously safe concurrent pread
interface is migrated to use seqfile.

> Because seq_read happens under m->lock I don't see how there will
> be anything thread unsafe.
>
> The position that changes is just an internal implementation detail to
> keep track of which data that we have cached.

I'm only referring to unstable records since every time we seek we
trash the cache.

>
>>
>> - Paul
>>
>> ---
>>  fs/seq_file.c |    2 --
>>  1 files changed, 0 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/seq_file.c b/fs/seq_file.c
>> index cd63d69..aa3621f 100644
>> --- a/fs/seq_file.c
>> +++ b/fs/seq_file.c
>> @@ -48,8 +48,6 @@ int seq_open(struct file *file, const struct seq_operations
>> *op)
>>        */
>>       file->f_version = 0;
>>
>> -     /* SEQ files support lseek, but not pread/pwrite */
>> -     file->f_mode &= ~(FMODE_PREAD | FMODE_PWRITE);
>>       return 0;
>>  }
>>  EXPORT_SYMBOL(seq_open);
>> --
>> 1.5.4.5
>