x86/mm/dump_pagetables: Change to use DEFINE_SHOW_ATTRIBUTE macro

PATCH In review Yangtao Li 4 participants 9 messages Nov 19, 2018 – Dec 18, 2018

Subsystems (2)

x86 x86 MM

lore

Share & tools

lore

Yangtao Li

author

2018-11-19 15:44 UTC #0 lore raw

PATCH 1/1 x86/mm/dump_pagetables: Change to use DEFINE_SHOW_ATTRIBUTE macro

Use DEFINE_SHOW_ATTRIBUTE macro to simplify the code.

Signed-off-by Yangtao Li <[email protected]>

---
 arch/x86/mm/debug_pagetables.c | 58 ++++------------------------------
 1 file changed, 7 insertions(+), 51 deletions(-)

arch/x86/mm/debug_pagetables.c +7 ~~-51~~

index 225fe2f0bfec..cd84f067e41d 100644
@@ -10,20 +10,9 @@ static int ptdump_show(struct seq_file *m, void *v)
 	return 0;
 }
 
-static int ptdump_open(struct inode *inode, struct file *filp)
-{
-	return single_open(filp, ptdump_show, NULL);
-}
-
-static const struct file_operations ptdump_fops = {
-	.owner		= THIS_MODULE,
-	.open		= ptdump_open,
-	.read		= seq_read,
-	.llseek		= seq_lseek,
-	.release	= single_release,
-};
+DEFINE_SHOW_ATTRIBUTE(ptdump);
 
-static int ptdump_show_curknl(struct seq_file *m, void *v)
+static int ptdump_curknl_show(struct seq_file *m, void *v)
 {
 	if (current->mm->pgd) {
 		down_read(&current->mm->mmap_sem);

@@ -33,23 +22,12 @@ static int ptdump_show_curknl(struct seq_file *m, void *v)
 	return 0;
 }
 
-static int ptdump_open_curknl(struct inode *inode, struct file *filp)
-{
-	return single_open(filp, ptdump_show_curknl, NULL);
-}
-
-static const struct file_operations ptdump_curknl_fops = {
-	.owner		= THIS_MODULE,
-	.open		= ptdump_open_curknl,
-	.read		= seq_read,
-	.llseek		= seq_lseek,
-	.release	= single_release,
-};
+DEFINE_SHOW_ATTRIBUTE(ptdump_curknl);
 
 #ifdef CONFIG_PAGE_TABLE_ISOLATION
 static struct dentry *pe_curusr;
 
-static int ptdump_show_curusr(struct seq_file *m, void *v)
+static int ptdump_curusr_show(struct seq_file *m, void *v)
 {
 	if (current->mm->pgd) {
 		down_read(&current->mm->mmap_sem);

@@ -59,42 +37,20 @@ static int ptdump_show_curusr(struct seq_file *m, void *v)
 	return 0;
 }
 
-static int ptdump_open_curusr(struct inode *inode, struct file *filp)
-{
-	return single_open(filp, ptdump_show_curusr, NULL);
-}
-
-static const struct file_operations ptdump_curusr_fops = {
-	.owner		= THIS_MODULE,
-	.open		= ptdump_open_curusr,
-	.read		= seq_read,
-	.llseek		= seq_lseek,
-	.release	= single_release,
-};
+DEFINE_SHOW_ATTRIBUTE(ptdump_curusr);
 #endif
 
 #if defined(CONFIG_EFI) && defined(CONFIG_X86_64)
 static struct dentry *pe_efi;
 
-static int ptdump_show_efi(struct seq_file *m, void *v)
+static int ptdump_efi_show(struct seq_file *m, void *v)
 {
 	if (efi_mm.pgd)
 		ptdump_walk_pgd_level_debugfs(m, efi_mm.pgd, false);
 	return 0;
 }
 
-static int ptdump_open_efi(struct inode *inode, struct file *filp)
-{
-	return single_open(filp, ptdump_show_efi, NULL);
-}
-
-static const struct file_operations ptdump_efi_fops = {
-	.owner		= THIS_MODULE,
-	.open		= ptdump_open_efi,
-	.read		= seq_read,
-	.llseek		= seq_lseek,
-	.release	= single_release,
-};
+DEFINE_SHOW_ATTRIBUTE(ptdump_efi);
 #endif
 
 static struct dentry *dir, *pe_knl, *pe_curknl;

Signature · 1 line

2.17.0

Dave Hansen

2018-11-19 17:06 UTC #1 lore raw

arch/x86/mm/debug_pagetables.c static int ptdump_show_curusr(struct seq_file *m, void *v) · line 40

Quoted 8 lines · Yangtao Li

arch/x86/mm/debug_pagetables.c +1 -7

-static const struct file_operations ptdump_curusr_fops = {
-	.owner		= THIS_MODULE,
-	.open		= ptdump_open_curusr,
-	.read		= seq_read,
-	.llseek		= seq_lseek,
-	.release	= single_release,
-};
+DEFINE_SHOW_ATTRIBUTE(ptdump_curusr);

FWIW, I rather dislike this conversion and the DEFINE_SHOW_ATTRIBUTE() approach in general. It makes it basically impossible to go from ptdump_curusr to ptdump_open_curusr without opening up the macro and reverse-engineering it.

My test is that for these macros to be sane, I need to be able to find "ptdump_open_curusr" by grepping for "ptdump_curusr". This fails the test.

I don't think saving a few lines of code is worth the obfuscation.

Frank Lee

author

2018-11-20 00:50 UTC #2 lore raw

Hi Dava:

How about just change the ptdump_fops and ptdump_open?

Yours,
Yangtao

Quoted 20 lines · Dave Hansen

arch/x86/mm/debug_pagetables.c static int ptdump_show_curusr(struct seq_file *m, void *v) · line 40
Quoted 8 lines · Yangtao Li
arch/x86/mm/debug_pagetables.c +1 -7
-static const struct file_operations ptdump_curusr_fops = {
-     .owner          = THIS_MODULE,
-     .open           = ptdump_open_curusr,
-     .read           = seq_read,
-     .llseek         = seq_lseek,
-     .release        = single_release,
-};
+DEFINE_SHOW_ATTRIBUTE(ptdump_curusr);
FWIW, I rather dislike this conversion and the DEFINE_SHOW_ATTRIBUTE() approach in general. It makes it basically impossible to go from ptdump_curusr to ptdump_open_curusr without opening up the macro and reverse-engineering it.
My test is that for these macros to be sane, I need to be able to find "ptdump_open_curusr" by grepping for "ptdump_curusr". This fails the test.
I don't think saving a few lines of code is worth the obfuscation.

Frank Lee

author

2018-11-20 00:51 UTC #3 lore raw

Hi Kees:

I want to listen to you.

--Yangtao

Quoted 28 lines · Frank Lee

Hi Dava:
How about just change the ptdump_fops and ptdump_open?
Yours,
Yangtao
Quoted 20 lines · Dave Hansen
arch/x86/mm/debug_pagetables.c static int ptdump_show_curusr(struct seq_file *m, void *v) · line 40
Quoted 8 lines · Yangtao Li
arch/x86/mm/debug_pagetables.c +1 -7
-static const struct file_operations ptdump_curusr_fops = {
-     .owner          = THIS_MODULE,
-     .open           = ptdump_open_curusr,
-     .read           = seq_read,
-     .llseek         = seq_lseek,
-     .release        = single_release,
-};
+DEFINE_SHOW_ATTRIBUTE(ptdump_curusr);
FWIW, I rather dislike this conversion and the DEFINE_SHOW_ATTRIBUTE() approach in general. It makes it basically impossible to go from ptdump_curusr to ptdump_open_curusr without opening up the macro and reverse-engineering it.
My test is that for these macros to be sane, I need to be able to find "ptdump_open_curusr" by grepping for "ptdump_curusr". This fails the test.
I don't think saving a few lines of code is worth the obfuscation.

Kees Cook

2018-11-27 22:50 UTC #4 lore raw

In reply to Dave Hansen

Quoted 15 lines · Dave Hansen

arch/x86/mm/debug_pagetables.c static int ptdump_show_curusr(struct seq_file *m, void *v) · line 40
Quoted 8 lines · Yangtao Li
arch/x86/mm/debug_pagetables.c +1 -7
-static const struct file_operations ptdump_curusr_fops = {
-     .owner          = THIS_MODULE,
-     .open           = ptdump_open_curusr,
-     .read           = seq_read,
-     .llseek         = seq_lseek,
-     .release        = single_release,
-};
+DEFINE_SHOW_ATTRIBUTE(ptdump_curusr);
FWIW, I rather dislike this conversion and the DEFINE_SHOW_ATTRIBUTE() approach in general. It makes it basically impossible to go from ptdump_curusr to ptdump_open_curusr without opening up the macro and reverse-engineering it.

Quoted 2 lines

My test is that for these macros to be sane, I need to be able to find "ptdump_open_curusr" by grepping for "ptdump_curusr". This fails the test.

Er, "ptdump_curusr" matches the generated name "ptdump_curusr_show", is that what you mean?

Quoted 1 line

I don't think saving a few lines of code is worth the obfuscation.

This is the standard boilerplate for attributes, though. I'd be nice to drop all the copy/pasted code...

Signature · 1 line

Kees Cook

Frank Lee

author

2018-11-28 00:57 UTC #5 lore raw

Quoted 23 lines · Kees Cook

Quoted 15 lines · Dave Hansen
arch/x86/mm/debug_pagetables.c static int ptdump_show_curusr(struct seq_file *m, void *v) · line 40
Quoted 8 lines · Yangtao Li
arch/x86/mm/debug_pagetables.c +1 -7
-static const struct file_operations ptdump_curusr_fops = {
-     .owner          = THIS_MODULE,
-     .open           = ptdump_open_curusr,
-     .read           = seq_read,
-     .llseek         = seq_lseek,
-     .release        = single_release,
-};
+DEFINE_SHOW_ATTRIBUTE(ptdump_curusr);
FWIW, I rather dislike this conversion and the DEFINE_SHOW_ATTRIBUTE() approach in general. It makes it basically impossible to go from ptdump_curusr to ptdump_open_curusr without opening up the macro and reverse-engineering it.
Quoted 2 lines
My test is that for these macros to be sane, I need to be able to find "ptdump_open_curusr" by grepping for "ptdump_curusr". This fails the test.
Er, "ptdump_curusr" matches the generated name "ptdump_curusr_show", is that what you mean?

Quoted 7 lines

Quoted 1 line
I don't think saving a few lines of code is worth the obfuscation.
This is the standard boilerplate for attributes, though. I'd be nice to drop all the copy/pasted code...
--
Kees Cook

I also think that we should delete the duplicate code which doesn't make much sense.

MBR,
Yangtao

Dave Hansen

Reviewed-by

2018-11-28 18:45 UTC #6 lore raw

In reply to Kees Cook

Quoted 16 lines · Kees Cook

Quoted 15 lines · Dave Hansen
arch/x86/mm/debug_pagetables.c static int ptdump_show_curusr(struct seq_file *m, void *v) · line 40
Quoted 8 lines · Yangtao Li
arch/x86/mm/debug_pagetables.c +1 -7
-static const struct file_operations ptdump_curusr_fops = {
-     .owner          = THIS_MODULE,
-     .open           = ptdump_open_curusr,
-     .read           = seq_read,
-     .llseek         = seq_lseek,
-     .release        = single_release,
-};
+DEFINE_SHOW_ATTRIBUTE(ptdump_curusr);
FWIW, I rather dislike this conversion and the DEFINE_SHOW_ATTRIBUTE() approach in general. It makes it basically impossible to go from ptdump_curusr to ptdump_open_curusr without opening up the macro and reverse-engineering it.

Quoted 5 lines

Quoted 2 lines
My test is that for these macros to be sane, I need to be able to find "ptdump_open_curusr" by grepping for "ptdump_curusr". This fails the test.
Er, "ptdump_curusr" matches the generated name "ptdump_curusr_show", is that what you mean?

Ahh, I also missed some of the renames to make this OK, like:

arch/x86/mm/debug_pagetables.c static int ptdump_show_curusr(struct seq_file *m, void *v) · line 46

Quoted 2 lines

arch/x86/mm/debug_pagetables.c +1 -1

-static int ptdump_show_efi(struct seq_file *m, void *v)
+static int ptdump_efi_show(struct seq_file *m, void *v)

I thought there was some macro magic going on that screwed the names up.

This looks fine to me.

Reviewed-by Dave Hansen <[email protected]>

Kees Cook

Reviewed-by

2018-11-29 21:29 UTC #7 lore raw

Quoted 26 lines · Dave Hansen

Quoted 16 lines · Kees Cook
Quoted 15 lines · Dave Hansen
arch/x86/mm/debug_pagetables.c static int ptdump_show_curusr(struct seq_file *m, void *v) · line 40
Quoted 8 lines · Yangtao Li
arch/x86/mm/debug_pagetables.c +1 -7
-static const struct file_operations ptdump_curusr_fops = {
-     .owner          = THIS_MODULE,
-     .open           = ptdump_open_curusr,
-     .read           = seq_read,
-     .llseek         = seq_lseek,
-     .release        = single_release,
-};
+DEFINE_SHOW_ATTRIBUTE(ptdump_curusr);
FWIW, I rather dislike this conversion and the DEFINE_SHOW_ATTRIBUTE() approach in general. It makes it basically impossible to go from ptdump_curusr to ptdump_open_curusr without opening up the macro and reverse-engineering it.
Quoted 5 lines
Quoted 2 lines
My test is that for these macros to be sane, I need to be able to find "ptdump_open_curusr" by grepping for "ptdump_curusr". This fails the test.
Er, "ptdump_curusr" matches the generated name "ptdump_curusr_show", is that what you mean?
Ahh, I also missed some of the renames to make this OK, like:

Quoted 8 lines

arch/x86/mm/debug_pagetables.c static int ptdump_show_curusr(struct seq_file *m, void *v) · line 46
Quoted 2 lines
arch/x86/mm/debug_pagetables.c +1 -1
-static int ptdump_show_efi(struct seq_file *m, void *v)
+static int ptdump_efi_show(struct seq_file *m, void *v)
I thought there was some macro magic going on that screwed the names up.
This looks fine to me.
Reviewed-by Dave Hansen <[email protected]>

Cool, thanks!

Reviewed-by Kees Cook <[email protected]>

Ingo, can you toss this in -tip please?

Signature · 1 line

Kees Cook

tip-bot for Yangtao Li

merge bot

2018-12-18 12:10 UTC #8 lore raw

[tip:x86/mm] x86/mm/dump_pagetables: Use DEFINE_SHOW_ATTRIBUTE()

In reply to Yangtao Li

x86/mm/dump_pagetables: Use DEFINE_SHOW_ATTRIBUTE()

Commit-ID:  6848ac7ca39a226ede5df7af0efcc4ef0611e99c
Gitweb:     https://git.kernel.org/tip/6848ac7ca39a226ede5df7af0efcc4ef0611e99c
Author:     Yangtao Li <[email protected]>
AuthorDate: Mon, 19 Nov 2018 10:43:34 -0500
Committer:  Thomas Gleixner <[email protected]>
CommitDate: Tue, 18 Dec 2018 13:05:54 +0100

x86/mm/dump_pagetables: Use DEFINE_SHOW_ATTRIBUTE()

Use DEFINE_SHOW_ATTRIBUTE() instead of open coding it.

Signed-off-by Yangtao Li <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

Reviewed-by Dave Hansen <[email protected]>

Cc [email protected]

Link https://lkml.kernel.org/r/[email protected] open here

---
 arch/x86/mm/debug_pagetables.c | 58 +++++-------------------------------------
 1 file changed, 7 insertions(+), 51 deletions(-)

arch/x86/mm/debug_pagetables.c +7 ~~-51~~

index 225fe2f0bfec..cd84f067e41d 100644
@@ -10,20 +10,9 @@ static int ptdump_show(struct seq_file *m, void *v)
 	return 0;
 }
 
-static int ptdump_open(struct inode *inode, struct file *filp)
-{
-	return single_open(filp, ptdump_show, NULL);
-}
-
-static const struct file_operations ptdump_fops = {
-	.owner		= THIS_MODULE,
-	.open		= ptdump_open,
-	.read		= seq_read,
-	.llseek		= seq_lseek,
-	.release	= single_release,
-};
+DEFINE_SHOW_ATTRIBUTE(ptdump);
 
-static int ptdump_show_curknl(struct seq_file *m, void *v)
+static int ptdump_curknl_show(struct seq_file *m, void *v)
 {
 	if (current->mm->pgd) {
 		down_read(&current->mm->mmap_sem);

@@ -33,23 +22,12 @@ static int ptdump_show_curknl(struct seq_file *m, void *v)
 	return 0;
 }
 
-static int ptdump_open_curknl(struct inode *inode, struct file *filp)
-{
-	return single_open(filp, ptdump_show_curknl, NULL);
-}
-
-static const struct file_operations ptdump_curknl_fops = {
-	.owner		= THIS_MODULE,
-	.open		= ptdump_open_curknl,
-	.read		= seq_read,
-	.llseek		= seq_lseek,
-	.release	= single_release,
-};
+DEFINE_SHOW_ATTRIBUTE(ptdump_curknl);
 
 #ifdef CONFIG_PAGE_TABLE_ISOLATION
 static struct dentry *pe_curusr;
 
-static int ptdump_show_curusr(struct seq_file *m, void *v)
+static int ptdump_curusr_show(struct seq_file *m, void *v)
 {
 	if (current->mm->pgd) {
 		down_read(&current->mm->mmap_sem);

@@ -59,42 +37,20 @@ static int ptdump_show_curusr(struct seq_file *m, void *v)
 	return 0;
 }
 
-static int ptdump_open_curusr(struct inode *inode, struct file *filp)
-{
-	return single_open(filp, ptdump_show_curusr, NULL);
-}
-
-static const struct file_operations ptdump_curusr_fops = {
-	.owner		= THIS_MODULE,
-	.open		= ptdump_open_curusr,
-	.read		= seq_read,
-	.llseek		= seq_lseek,
-	.release	= single_release,
-};
+DEFINE_SHOW_ATTRIBUTE(ptdump_curusr);
 #endif
 
 #if defined(CONFIG_EFI) && defined(CONFIG_X86_64)
 static struct dentry *pe_efi;
 
-static int ptdump_show_efi(struct seq_file *m, void *v)
+static int ptdump_efi_show(struct seq_file *m, void *v)
 {
 	if (efi_mm.pgd)
 		ptdump_walk_pgd_level_debugfs(m, efi_mm.pgd, false);
 	return 0;
 }
 
-static int ptdump_open_efi(struct inode *inode, struct file *filp)
-{
-	return single_open(filp, ptdump_show_efi, NULL);
-}
-
-static const struct file_operations ptdump_efi_fops = {
-	.owner		= THIS_MODULE,
-	.open		= ptdump_open_efi,
-	.read		= seq_read,
-	.llseek		= seq_lseek,
-	.release	= single_release,
-};
+DEFINE_SHOW_ATTRIBUTE(ptdump_efi);
 #endif
 
 static struct dentry *dir, *pe_knl, *pe_curknl;