[PATCH v2] procfs: fix numbering in /proc/locks

[PATCH v2] procfs: fix numbering in /proc/locks

[Jerome Marchand]

The lock number in /proc/locks (first field) is implemented by a counter
(private field of struct seq_file) which is incremented at each call of
locks_show() and reset to 1 in locks_start() whatever the offset is. It
should be reset according to the actual position in the list.

Moreover, locks_show() can be called twice to print a single line thus
skipping a number. The counter should be incremented in locks_next().

Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
---
 locks.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index ab24d49..49d7343 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -2166,19 +2166,19 @@ static int locks_show(struct seq_file *f, void *v)
 	list_for_each_entry(bfl, &fl->fl_block, fl_block)
 		lock_get_status(f, bfl, (long)f->private, " ->");
 
-	f->private++;
 	return 0;
 }
 
 static void *locks_start(struct seq_file *f, loff_t *pos)
 {
 	lock_kernel();
-	f->private = (void *)1;
+	f->private = (void *) (*pos + 1);
 	return seq_list_start(&file_lock_list, *pos);
 }
 
 static void *locks_next(struct seq_file *f, void *v, loff_t *pos)
 {
+	f->private++;
 	return seq_list_next(v, &file_lock_list, pos);
 }
 

Re: [PATCH v2] procfs: fix numbering in /proc/locks

[Américo Wang]

On Thu, Sep 30, 2010 at 02:38:18PM +0200, Jerome Marchand wrote:
>
>The lock number in /proc/locks (first field) is implemented by a counter
>(private field of struct seq_file) which is incremented at each call of
>locks_show() and reset to 1 in locks_start() whatever the offset is. It
>should be reset according to the actual position in the list.
>
>Moreover, locks_show() can be called twice to print a single line thus
>skipping a number. The counter should be incremented in locks_next().
>
>Signed-off-by: Jerome Marchand <jmarchan@redhat.com>

This patch looks good to me,

Reviewed-by: WANG Cong <xiyou.wangcong@gmail.com>

Need to Cc -stable?

>---
> locks.c |    4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/fs/locks.c b/fs/locks.c
>index ab24d49..49d7343 100644
>--- a/fs/locks.c
>+++ b/fs/locks.c
>@@ -2166,19 +2166,19 @@ static int locks_show(struct seq_file *f, void *v)
> 	list_for_each_entry(bfl, &fl->fl_block, fl_block)
> 		lock_get_status(f, bfl, (long)f->private, " ->");
> 
>-	f->private++;
> 	return 0;
> }
> 
> static void *locks_start(struct seq_file *f, loff_t *pos)
> {
> 	lock_kernel();
>-	f->private = (void *)1;
>+	f->private = (void *) (*pos + 1);
> 	return seq_list_start(&file_lock_list, *pos);
> }
> 
> static void *locks_next(struct seq_file *f, void *v, loff_t *pos)
> {
>+	f->private++;
> 	return seq_list_next(v, &file_lock_list, pos);
> }
> 
>--
>To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] procfs: fix numbering in /proc/locks

[Pavel Emelyanov]

On 09/30/2010 04:38 PM, Jerome Marchand wrote:
> 
> The lock number in /proc/locks (first field) is implemented by a counter
> (private field of struct seq_file) which is incremented at each call of
> locks_show() and reset to 1 in locks_start() whatever the offset is. It
> should be reset according to the actual position in the list.
> 
> Moreover, locks_show() can be called twice to print a single line thus
> skipping a number. The counter should be incremented in locks_next().
> 
> Signed-off-by: Jerome Marchand <jmarchan@redhat.com>

Acked-by: Pavel Emelyanov <xemul@openvz.org>

> ---
>  locks.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/locks.c b/fs/locks.c
> index ab24d49..49d7343 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -2166,19 +2166,19 @@ static int locks_show(struct seq_file *f, void *v)
>  	list_for_each_entry(bfl, &fl->fl_block, fl_block)
>  		lock_get_status(f, bfl, (long)f->private, " ->");
>  
> -	f->private++;
>  	return 0;
>  }
>  
>  static void *locks_start(struct seq_file *f, loff_t *pos)
>  {
>  	lock_kernel();
> -	f->private = (void *)1;
> +	f->private = (void *) (*pos + 1);
>  	return seq_list_start(&file_lock_list, *pos);
>  }
>  
>  static void *locks_next(struct seq_file *f, void *v, loff_t *pos)
>  {
> +	f->private++;
>  	return seq_list_next(v, &file_lock_list, pos);
>  }
>  
> .
> 

Re: [PATCH v2] procfs: fix numbering in /proc/locks

[Jerome Marchand]

On 09/30/2010 02:38 PM, Jerome Marchand wrote:
> 
> The lock number in /proc/locks (first field) is implemented by a counter
> (private field of struct seq_file) which is incremented at each call of
> locks_show() and reset to 1 in locks_start() whatever the offset is. It
> should be reset according to the actual position in the list.
> 
> Moreover, locks_show() can be called twice to print a single line thus
> skipping a number. The counter should be incremented in locks_next().
> 
> Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
> ---
>  locks.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/locks.c b/fs/locks.c
> index ab24d49..49d7343 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -2166,19 +2166,19 @@ static int locks_show(struct seq_file *f, void *v)
>  	list_for_each_entry(bfl, &fl->fl_block, fl_block)
>  		lock_get_status(f, bfl, (long)f->private, " ->");
>  
> -	f->private++;
>  	return 0;
>  }
>  
>  static void *locks_start(struct seq_file *f, loff_t *pos)
>  {
>  	lock_kernel();
> -	f->private = (void *)1;
> +	f->private = (void *) (*pos + 1);

That cast trigger a warning on some arch:
"warning: cast to pointer from integer of different size"

There is no real risk here. At worst /proc/locks will show wrong number
if there is more than 2^32 locks, but should I mute the warning it with
something like:
        f->private = (void *) (size_t) (*pos + 1);
?

Thanks,
Jerome

>  	return seq_list_start(&file_lock_list, *pos);
>  }
>  
>  static void *locks_next(struct seq_file *f, void *v, loff_t *pos)
>  {
> +	f->private++;
>  	return seq_list_next(v, &file_lock_list, pos);
>  }
>  
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] procfs: fix numbering in /proc/locks

[Américo Wang]

On Tue, Oct 05, 2010 at 02:14:11PM +0200, Jerome Marchand wrote:
>On 09/30/2010 02:38 PM, Jerome Marchand wrote:
...
>> -	f->private = (void *)1;
>> +	f->private = (void *) (*pos + 1);
>
>That cast trigger a warning on some arch:
>"warning: cast to pointer from integer of different size"
>
>There is no real risk here. At worst /proc/locks will show wrong number
>if there is more than 2^32 locks, but should I mute the warning it with
>something like:
>        f->private = (void *) (size_t) (*pos + 1);
>?

Or maybe cast to (void *) (unsigned long)?

Re: [PATCH v2] procfs: fix numbering in /proc/locks

[Andrew Morton]

On Tue, 05 Oct 2010 14:14:11 +0200
Jerome Marchand <jmarchan@redhat.com> wrote:

> On 09/30/2010 02:38 PM, Jerome Marchand wrote:
> > 
> > The lock number in /proc/locks (first field) is implemented by a counter
> > (private field of struct seq_file) which is incremented at each call of
> > locks_show() and reset to 1 in locks_start() whatever the offset is. It
> > should be reset according to the actual position in the list.
> > 
> > Moreover, locks_show() can be called twice to print a single line thus
> > skipping a number. The counter should be incremented in locks_next().
> > 
> > Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
> > ---
> >  locks.c |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/locks.c b/fs/locks.c
> > index ab24d49..49d7343 100644
> > --- a/fs/locks.c
> > +++ b/fs/locks.c
> > @@ -2166,19 +2166,19 @@ static int locks_show(struct seq_file *f, void *v)
> >  	list_for_each_entry(bfl, &fl->fl_block, fl_block)
> >  		lock_get_status(f, bfl, (long)f->private, " ->");
> >  
> > -	f->private++;
> >  	return 0;
> >  }
> >  
> >  static void *locks_start(struct seq_file *f, loff_t *pos)
> >  {
> >  	lock_kernel();
> > -	f->private = (void *)1;
> > +	f->private = (void *) (*pos + 1);
> 
> That cast trigger a warning on some arch:
> "warning: cast to pointer from integer of different size"
> 
> There is no real risk here. At worst /proc/locks will show wrong number
> if there is more than 2^32 locks, but should I mute the warning it with
> something like:
>         f->private = (void *) (size_t) (*pos + 1);
> ?

Putting a loff_t into a void* is a pretty alarming thing to do.  If
we're really going to do that then use a (long) cast and put a very
good comment at the code site explaining why the bug doesn't matter, so
people aren't misled.

But really, why sweat it?  kmalloc the eight bytes, make ->private
point at that and we never have to think about it again.  Bonus points
for doing this without any typecasts ;)

Re: [PATCH v2] procfs: fix numbering in /proc/locks

[Jerome Marchand]

On 10/08/2010 01:27 AM, Andrew Morton wrote:
> On Tue, 05 Oct 2010 14:14:11 +0200
> Jerome Marchand <jmarchan@redhat.com> wrote:
> 
>> On 09/30/2010 02:38 PM, Jerome Marchand wrote:
>>>
>>> The lock number in /proc/locks (first field) is implemented by a counter
>>> (private field of struct seq_file) which is incremented at each call of
>>> locks_show() and reset to 1 in locks_start() whatever the offset is. It
>>> should be reset according to the actual position in the list.
>>>
>>> Moreover, locks_show() can be called twice to print a single line thus
>>> skipping a number. The counter should be incremented in locks_next().
>>>
>>> Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
>>> ---
>>>  locks.c |    4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/fs/locks.c b/fs/locks.c
>>> index ab24d49..49d7343 100644
>>> --- a/fs/locks.c
>>> +++ b/fs/locks.c
>>> @@ -2166,19 +2166,19 @@ static int locks_show(struct seq_file *f, void *v)
>>>  	list_for_each_entry(bfl, &fl->fl_block, fl_block)
>>>  		lock_get_status(f, bfl, (long)f->private, " ->");
>>>  
>>> -	f->private++;
>>>  	return 0;
>>>  }
>>>  
>>>  static void *locks_start(struct seq_file *f, loff_t *pos)
>>>  {
>>>  	lock_kernel();
>>> -	f->private = (void *)1;
>>> +	f->private = (void *) (*pos + 1);
>>
>> That cast trigger a warning on some arch:
>> "warning: cast to pointer from integer of different size"
>>
>> There is no real risk here. At worst /proc/locks will show wrong number
>> if there is more than 2^32 locks, but should I mute the warning it with
>> something like:
>>         f->private = (void *) (size_t) (*pos + 1);
>> ?
> 
> Putting a loff_t into a void* is a pretty alarming thing to do.  If
> we're really going to do that then use a (long) cast and put a very
> good comment at the code site explaining why the bug doesn't matter, so
> people aren't misled.
> 
> But really, why sweat it?  kmalloc the eight bytes, make ->private
> point at that and we never have to think about it again.  Bonus points
> for doing this without any typecasts ;)
> 

That's definitely cleaner. I'll do that.

Thanks,
Jerome

[PATCH v3] procfs: fix numbering in /proc/locks

[Jerome Marchand]

The lock number in /proc/locks (first field) is implemented by a counter
(private field of struct seq_file) which is incremented at each call of
locks_show() and reset to 1 in locks_start() whatever the offset is. It
should be reset according to the actual position in the list.

Moreover, locks_show() can be called twice to print a single line thus
skipping a number. The counter should be incremented in locks_next().

And last, pos is a loff_t, which can be bigger than a pointer, so we 
don't use the pointer as an integer anymore, and allocate a loff_t
instead.

Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
---
 locks.c |   19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index ab24d49..767ef8e 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -2085,7 +2085,7 @@ EXPORT_SYMBOL_GPL(vfs_cancel_lock);
 #include <linux/seq_file.h>
 
 static void lock_get_status(struct seq_file *f, struct file_lock *fl,
-							int id, char *pfx)
+			    loff_t id, char *pfx)
 {
 	struct inode *inode = NULL;
 	unsigned int fl_pid;
@@ -2098,7 +2098,7 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl,
 	if (fl->fl_file != NULL)
 		inode = fl->fl_file->f_path.dentry->d_inode;
 
-	seq_printf(f, "%d:%s ", id, pfx);
+	seq_printf(f, "%lld:%s ", id, pfx);
 	if (IS_POSIX(fl)) {
 		seq_printf(f, "%6s %s ",
 			     (fl->fl_flags & FL_ACCESS) ? "ACCESS" : "POSIX ",
@@ -2161,24 +2161,27 @@ static int locks_show(struct seq_file *f, void *v)
 
 	fl = list_entry(v, struct file_lock, fl_link);
 
-	lock_get_status(f, fl, (long)f->private, "");
+	lock_get_status(f, fl, *((loff_t *)f->private), "");
 
 	list_for_each_entry(bfl, &fl->fl_block, fl_block)
-		lock_get_status(f, bfl, (long)f->private, " ->");
+		lock_get_status(f, bfl, *((loff_t *)f->private), " ->");
 
-	f->private++;
 	return 0;
 }
 
 static void *locks_start(struct seq_file *f, loff_t *pos)
 {
+	loff_t *p = f->private;
+
 	lock_kernel();
-	f->private = (void *)1;
+	*p = (*pos + 1);
 	return seq_list_start(&file_lock_list, *pos);
 }
 
 static void *locks_next(struct seq_file *f, void *v, loff_t *pos)
 {
+	loff_t *p = f->private;
+	++*p;
 	return seq_list_next(v, &file_lock_list, pos);
 }
 
@@ -2196,14 +2199,14 @@ static const struct seq_operations locks_seq_operations = {
 
 static int locks_open(struct inode *inode, struct file *filp)
 {
-	return seq_open(filp, &locks_seq_operations);
+	return seq_open_private(filp, &locks_seq_operations, sizeof(loff_t));
 }
 
 static const struct file_operations proc_locks_operations = {
 	.open		= locks_open,
 	.read		= seq_read,
 	.llseek		= seq_lseek,
-	.release	= seq_release,
+	.release	= seq_release_private,
 };
 
 static int __init proc_locks_init(void)

Re: [PATCH v3] procfs: fix numbering in /proc/locks

[Andrew Morton]

On Fri, 15 Oct 2010 13:32:29 +0200
Jerome Marchand <jmarchan@redhat.com> wrote:

> 
> The lock number in /proc/locks (first field) is implemented by a counter
> (private field of struct seq_file) which is incremented at each call of
> locks_show() and reset to 1 in locks_start() whatever the offset is. It
> should be reset according to the actual position in the list.
> 
> Moreover, locks_show() can be called twice to print a single line thus
> skipping a number. The counter should be incremented in locks_next().
> 
> And last, pos is a loff_t, which can be bigger than a pointer, so we 
> don't use the pointer as an integer anymore, and allocate a loff_t
> instead.
> 

The changelog didn't actually describe the bug.  At least, not in any
way I can understand.

So I went back to your first patch and stole the following sentence:

   Because of this, the numbering erratically restarts at 1 several
   times when reading a long /proc/locks file.

and appended it to the first paragraph.

It's somewhat important to describe the user-visible effects of a bug,
please.  It helps engineers when deciding whether to backport a patch
into their kernels and it helps end-users decide whether a particular
patch will solve a problem they're experiencing.  It's also useful to
those who must decide which kernel.org release(s) should include the
patch.

Which reminds me.  Do you think this bug is serious enough to warrant
backporting the fix into -stable?


>  locks.c |   19 +++++++++++--------

Patch looks good.  seq_open_private()'s psize should have been size_t,
not int.  But that's not your doing ;)

I had to fiddle the patch a bit due to BKL changes in linux-next.  Then
I mucked it up, then I fixed it.  Please double-check my handiwork.



From: Jerome Marchand <jmarchan@redhat.com>

The lock number in /proc/locks (first field) is implemented by a counter
(private field of struct seq_file) which is incremented at each call of
locks_show() and reset to 1 in locks_start() whatever the offset is.  It
should be reset according to the actual position in the list.  Because of
this, the numbering erratically restarts at 1 several times when reading a
long /proc/locks file.

Moreover, locks_show() can be called twice to print a single line thus
skipping a number.  The counter should be incremented in locks_next().

And last, pos is a loff_t, which can be bigger than a pointer, so we don't
use the pointer as an integer anymore, and allocate a loff_t instead.

Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
Cc: Pavel Emelyanov <xemul@parallels.com>
Cc: Matthew Wilcox <matthew@wil.cx>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 fs/locks.c |   19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff -puN fs/locks.c~procfs-fix-numbering-in-proc-locks fs/locks.c
--- a/fs/locks.c~procfs-fix-numbering-in-proc-locks
+++ a/fs/locks.c
@@ -2109,7 +2109,7 @@ EXPORT_SYMBOL_GPL(vfs_cancel_lock);
 #include <linux/seq_file.h>
 
 static void lock_get_status(struct seq_file *f, struct file_lock *fl,
-							int id, char *pfx)
+			    loff_t id, char *pfx)
 {
 	struct inode *inode = NULL;
 	unsigned int fl_pid;
@@ -2122,7 +2122,7 @@ static void lock_get_status(struct seq_f
 	if (fl->fl_file != NULL)
 		inode = fl->fl_file->f_path.dentry->d_inode;
 
-	seq_printf(f, "%d:%s ", id, pfx);
+	seq_printf(f, "%lld:%s ", id, pfx);
 	if (IS_POSIX(fl)) {
 		seq_printf(f, "%6s %s ",
 			     (fl->fl_flags & FL_ACCESS) ? "ACCESS" : "POSIX ",
@@ -2185,24 +2185,27 @@ static int locks_show(struct seq_file *f
 
 	fl = list_entry(v, struct file_lock, fl_link);
 
-	lock_get_status(f, fl, (long)f->private, "");
+	lock_get_status(f, fl, *((loff_t *)f->private), "");
 
 	list_for_each_entry(bfl, &fl->fl_block, fl_block)
-		lock_get_status(f, bfl, (long)f->private, " ->");
+		lock_get_status(f, bfl, *((loff_t *)f->private), " ->");
 
-	f->private++;
 	return 0;
 }
 
 static void *locks_start(struct seq_file *f, loff_t *pos)
 {
+	loff_t *p = f->private;
+
 	lock_flocks();
-	f->private = (void *)1;
+	*p = (*pos + 1);
 	return seq_list_start(&file_lock_list, *pos);
 }
 
 static void *locks_next(struct seq_file *f, void *v, loff_t *pos)
 {
+	loff_t *p = f->private;
+	++*p;
 	return seq_list_next(v, &file_lock_list, pos);
 }
 
@@ -2220,14 +2223,14 @@ static const struct seq_operations locks
 
 static int locks_open(struct inode *inode, struct file *filp)
 {
-	return seq_open(filp, &locks_seq_operations);
+	return seq_open_private(filp, &locks_seq_operations, sizeof(loff_t));
 }
 
 static const struct file_operations proc_locks_operations = {
 	.open		= locks_open,
 	.read		= seq_read,
 	.llseek		= seq_lseek,
-	.release	= seq_release,
+	.release	= seq_release_private,
 };
 
 static int __init proc_locks_init(void)
_