[PATCH] proc: /proc/kpageflags: incorrect arg order in kpf_copy_bits()

[PATCH] proc: /proc/kpageflags: incorrect arg order in kpf_copy_bits()

[Jeffrey Mogul]

The code in fs/proc/page.c that implements /proc/kpageflags has
two similar but different bugs in the way that it copies bits
from the internal page->flags representation to the exported
representation:

(1) The kpf_copy_bits() macro was defined with argument order
(flags, srcpos, dstpos), which is the logical order, but all
but one of the uses of this macro invoke it instead in the order
(flags, dstpos, srcpos).  This only affects the output for a few
flags, since in most -- but not all! -- cases, srcpos == dstpos.

I fixed this by changing the macro definition rather than the macro
uses, which minimizes the number of lines to change but which might
not be the most obvious/logical design.

(2) One use of the kpf_copy_bit macro (for the PG_locked flag) had
the arguments in a random (rotated?) order, and was clearly wrong.

These bugs appear to have been in the code since /proc/kpageflags
was introduced (2.6.25).

CC: stable@kernel.org
Signed-off-by: Jeff Mogul <Jeff.Mogul@hp.com>
---
 fs/proc/page.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
--- linux-2.6.28.7/fs/proc/page.c.orig	2009-02-26 17:16:42.000000000 -0800
+++ linux-2.6.28.7/fs/proc/page.c	2009-02-26 17:18:04.000000000 -0800
@@ -80,7 +80,8 @@ static const struct file_operations proc
 #define KPF_RECLAIM    9
 #define KPF_BUDDY     10
 
-#define kpf_copy_bit(flags, srcpos, dstpos) (((flags >> srcpos) & 1) << dstpos)
+/* Arg order is dstpos,srcpos because that's how this macro is used below */
+#define kpf_copy_bit(flags, dstpos, srcpos) (((flags >> srcpos) & 1) << dstpos)
 
 static ssize_t kpageflags_read(struct file *file, char __user *buf,
 			     size_t count, loff_t *ppos)
@@ -107,7 +108,7 @@ static ssize_t kpageflags_read(struct fi
 		else
 			kflags = ppage->flags;
 
-		uflags = kpf_copy_bit(KPF_LOCKED, PG_locked, kflags) |
+		uflags = kpf_copy_bit(kflags, KPF_LOCKED, PG_locked) |
 			kpf_copy_bit(kflags, KPF_ERROR, PG_error) |
 			kpf_copy_bit(kflags, KPF_REFERENCED, PG_referenced) |
 			kpf_copy_bit(kflags, KPF_UPTODATE, PG_uptodate) |

Re: [PATCH] proc: /proc/kpageflags: incorrect arg order in kpf_copy_bits()

[Alexey Dobriyan]

On Fri, Feb 27, 2009 at 05:56:39PM -0800, Jeffrey Mogul wrote:
> The code in fs/proc/page.c that implements /proc/kpageflags has

Already fixed in mainline:

commit e07a4b9217d1e97d2f3a62b6b070efdc61212110
Author: Helge Bahmann <helge.bahmann@secunet.com>
Date:   Fri Feb 20 16:24:12 2009 +0300

    proc: fix PG_locked reporting in /proc/kpageflags
    
    Expr always evaluates to zero.
    
    Cc: Matt Mackall <mpm@selenic.com>
    Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
    Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>

diff --git a/fs/proc/page.c b/fs/proc/page.c
index 767d95a..2d13451 100644
--- a/fs/proc/page.c
+++ b/fs/proc/page.c
@@ -107,7 +107,7 @@ static ssize_t kpageflags_read(struct file *file, char __user *buf,
 		else
 			kflags = ppage->flags;
 
-		uflags = kpf_copy_bit(KPF_LOCKED, PG_locked, kflags) |
+		uflags = kpf_copy_bit(kflags, KPF_LOCKED, PG_locked) |
 			kpf_copy_bit(kflags, KPF_ERROR, PG_error) |
 			kpf_copy_bit(kflags, KPF_REFERENCED, PG_referenced) |
 			kpf_copy_bit(kflags, KPF_UPTODATE, PG_uptodate) |

Re: [PATCH] proc: /proc/kpageflags: incorrect arg order in kpf_copy_bits()

[Jeffrey Mogul]

    On Fri, Feb 27, 2009 at 05:56:39PM -0800, Jeffrey Mogul wrote:
    > The code in fs/proc/page.c that implements /proc/kpageflags has
    
    Already fixed in mainline:
    
    commit e07a4b9217d1e97d2f3a62b6b070efdc61212110
    Author: Helge Bahmann <helge.bahmann@secunet.com>
    Date:   Fri Feb 20 16:24:12 2009 +0300
    
        proc: fix PG_locked reporting in /proc/kpageflags
        
        Expr always evaluates to zero.
        
Unfortunately, I think that only fixes one of the two bugs
I reported.  The other one is that the argument order for the
kpf_copy_bits macro definition does not match the arg order
for all of the uses (including the one that was already "fixed").

Thanks
-Jeff

The kpf_copy_bits() macro was defined with argument order
(flags, srcpos, dstpos), which is the logical order, but all
but one of the uses of this macro invoke it instead in the order
(flags, dstpos, srcpos).  This only affects the output for a few
flags, since in most -- but not all! -- cases, srcpos == dstpos.

I fixed this by changing the macro definition rather than the macro
uses, which minimizes the number of lines to change but which might
not be the most obvious/logical design.

These bugs appear to have been in the code since /proc/kpageflags
was introduced (2.6.25).

CC: stable@kernel.org
Signed-off-by: Jeff Mogul <Jeff.Mogul@hp.com>
---

--- linux-2.6.28.7/fs/proc/page.c.orig  2009-02-26 17:16:42.000000000 -0800
+++ linux-2.6.28.7/fs/proc/page.c       2009-02-26 17:18:04.000000000 -0800
@@ -80,7 +80,8 @@ static const struct file_operations proc
 #define KPF_RECLAIM    9
 #define KPF_BUDDY     10
 
-#define kpf_copy_bit(flags, srcpos, dstpos) (((flags >> srcpos) & 1) << dstpos)
+/* Arg order is dstpos,srcpos because that's how this macro is used below */
+#define kpf_copy_bit(flags, dstpos, srcpos) (((flags >> srcpos) & 1) << dstpos)