From: Wanpeng Li <[email protected]>
------------------
Change in v2:
* add printk warning
* add description in Documentation
------------------
Signed-off-by: Wanpeng Li <[email protected]>
---
.../ABI/obsolete/proc-sys-vm-nr_pdflush_threads | 5 +++++
Documentation/feature-removal-schedule.txt | 8 ++++++++
Documentation/sysctl/vm.txt | 11 -----------
fs/fs-writeback.c | 5 -----
include/linux/sysctl.h | 3 +++
include/linux/writeback.h | 5 -----
kernel/sysctl.c | 18 +++++++++++++-----
kernel/sysctl_binary.c | 2 +-
8 files changed, 30 insertions(+), 27 deletions(-)
create mode 100644 Documentation/ABI/obsolete/proc-sys-vm-nr_pdflush_threads
diff --git a/Documentation/ABI/obsolete/proc-sys-vm-nr_pdflush_threads b/Documentation/ABI/obsolete/proc-sys-vm-nr_pdflush_threads
new file mode 100644
index 0000000..edbe548
--- /dev/null
+++ b/Documentation/ABI/obsolete/proc-sys-vm-nr_pdflush_threads
@@ -0,0 +1,5 @@
+What: /proc/sys/vm/nr_pdflush_threads
+Date: June 2012
+Contact: Wanpeng Li <[email protected]>
+Description: Since pdflush is replaced by per-BDI flusher, the interface of old pdflush
+ exported in /proc/sys/vm/ should be removed.
diff --git a/Documentation/feature-removal-schedule.txt b/Documentation/feature-removal-schedule.txt
index e4b5775..7eea1ee 100644
--- a/Documentation/feature-removal-schedule.txt
+++ b/Documentation/feature-removal-schedule.txt
@@ -6,6 +6,14 @@ be removed from this file.
---------------------------
+What: /proc/sys/vm/nr_pdflush_threads
+When: 2012
+Why: Since pdflush is deprecated, the interface exported in /proc/sys/vm/
+ should be removed.
+Who: Wanpeng Li <[email protected]>
+
+---------------------------
+
What: CONFIG_APM_CPU_IDLE, and its ability to call APM BIOS in idle
When: 2012
Why: This optional sub-feature of APM is of dubious reliability,
diff --git a/Documentation/sysctl/vm.txt b/Documentation/sysctl/vm.txt
index 96f0ee8..71c17d2 100644
--- a/Documentation/sysctl/vm.txt
+++ b/Documentation/sysctl/vm.txt
@@ -42,7 +42,6 @@ Currently, these files are in /proc/sys/vm:
- mmap_min_addr
- nr_hugepages
- nr_overcommit_hugepages
-- nr_pdflush_threads
- nr_trim_pages (only if CONFIG_MMU=n)
- numa_zonelist_order
- oom_dump_tasks
@@ -426,16 +425,6 @@ See Documentation/vm/hugetlbpage.txt
==============================================================
-nr_pdflush_threads
-
-The current number of pdflush threads. This value is read-only.
-The value changes according to the number of dirty pages in the system.
-
-When necessary, additional pdflush threads are created, one per second, up to
-nr_pdflush_threads_max.
-
-==============================================================
-
nr_trim_pages
This is available only on NOMMU kernels.
diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index 539f36c..30ddb8a 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -52,11 +52,6 @@ struct wb_writeback_work {
struct completion *done; /* set if the caller waits */
};
-/*
- * We don't actually have pdflush, but this one is exported though /proc...
- */
-int nr_pdflush_threads;
-
/**
* writeback_in_progress - determine whether there is writeback in progress
* @bdi: the device's backing_dev_info structure.
diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index c34b4c8..f25be74 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -964,6 +964,9 @@ extern int proc_doulongvec_ms_jiffies_minmax(struct ctl_table *table, int,
void __user *, size_t *, loff_t *);
extern int proc_do_large_bitmap(struct ctl_table *, int,
void __user *, size_t *, loff_t *);
+extern int proc_deprecated(struct ctl_table *, int,
+ void __user *, size_t *, loff_t *);
+
/*
* Register a set of sysctl names by calling register_sysctl_table
diff --git a/include/linux/writeback.h b/include/linux/writeback.h
index a2b84f5..13b5df7 100644
--- a/include/linux/writeback.h
+++ b/include/linux/writeback.h
@@ -193,9 +193,4 @@ void tag_pages_for_writeback(struct address_space *mapping,
void account_page_redirty(struct page *page);
-/* pdflush.c */
-extern int nr_pdflush_threads; /* Global so it can be exported to sysctl
- read-only. */
-
-
#endif /* WRITEBACK_H */
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 4ab1187..1ab53cb 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -1095,11 +1095,9 @@ static struct ctl_table vm_table[] = {
.extra1 = &zero,
},
{
- .procname = "nr_pdflush_threads",
- .data = &nr_pdflush_threads,
- .maxlen = sizeof nr_pdflush_threads,
- .mode = 0444 /* read-only*/,
- .proc_handler = proc_dointvec,
+ .procname = "nr_pdflush_threads",
+ .mode = 0444 /* read-only */,
+ .proc_handler = proc_deprecated,
},
{
.procname = "swappiness",
@@ -2505,6 +2503,15 @@ int proc_doulongvec_ms_jiffies_minmax(struct ctl_table *table, int write,
#endif /* CONFIG_PROC_SYSCTL */
+/* notice associated proc deprecated */
+int proc_deprecated(struct ctl_table *table, int write,
+ void __user *buffer, size_t *lenp, loff_t *ppos)
+{
+ printk(KERN_WARNING "%s exported in /proc is deprecated\n",
+ table->procname);
+ return -ENOSYS;
+}
+
/*
* No sense putting this after each symbol definition, twice,
* exception granted :-)
@@ -2517,3 +2524,4 @@ EXPORT_SYMBOL(proc_dointvec_ms_jiffies);
EXPORT_SYMBOL(proc_dostring);
EXPORT_SYMBOL(proc_doulongvec_minmax);
EXPORT_SYMBOL(proc_doulongvec_ms_jiffies_minmax);
+EXPORT_SYMBOL(proc_deprecated);
diff --git a/kernel/sysctl_binary.c b/kernel/sysctl_binary.c
index a650694..65bdcf1 100644
--- a/kernel/sysctl_binary.c
+++ b/kernel/sysctl_binary.c
@@ -147,7 +147,7 @@ static const struct bin_table bin_vm_table[] = {
{ CTL_INT, VM_DIRTY_RATIO, "dirty_ratio" },
/* VM_DIRTY_WB_CS "dirty_writeback_centisecs" no longer used */
/* VM_DIRTY_EXPIRE_CS "dirty_expire_centisecs" no longer used */
- { CTL_INT, VM_NR_PDFLUSH_THREADS, "nr_pdflush_threads" },
+ /* VM_NR_PDFLUSH_THREADS "nr_pdflush_threads" no longer used */
{ CTL_INT, VM_OVERCOMMIT_RATIO, "overcommit_ratio" },
/* VM_PAGEBUF unused */
/* VM_HUGETLB_PAGES "nr_hugepages" no longer used */
--
1.7.9.5
Hi WanPeng,
I have 2 questions here.
- Why not just remove the proc entry since we needn't it anymore?
- If we really want to keep the entry and just output warning message, we *only*
can see the message out of system console. It's reasonable to put the message
into the buffer directly so that any users from system console/telnet/ssh can
see the warning message?
Thanks,
Gavin
>From: Wanpeng Li <[email protected]>
>
>------------------
>
>Change in v2:
>
>* add printk warning
>* add description in Documentation
>
>------------------
>
>Signed-off-by: Wanpeng Li <[email protected]>
>---
> .../ABI/obsolete/proc-sys-vm-nr_pdflush_threads | 5 +++++
> Documentation/feature-removal-schedule.txt | 8 ++++++++
> Documentation/sysctl/vm.txt | 11 -----------
> fs/fs-writeback.c | 5 -----
> include/linux/sysctl.h | 3 +++
> include/linux/writeback.h | 5 -----
> kernel/sysctl.c | 18 +++++++++++++-----
> kernel/sysctl_binary.c | 2 +-
> 8 files changed, 30 insertions(+), 27 deletions(-)
> create mode 100644 Documentation/ABI/obsolete/proc-sys-vm-nr_pdflush_threads
>
>diff --git a/Documentation/ABI/obsolete/proc-sys-vm-nr_pdflush_threads b/Documentation/ABI/obsolete/proc-sys-vm-nr_pdflush_threads
>new file mode 100644
>index 0000000..edbe548
>--- /dev/null
>+++ b/Documentation/ABI/obsolete/proc-sys-vm-nr_pdflush_threads
>@@ -0,0 +1,5 @@
>+What: /proc/sys/vm/nr_pdflush_threads
>+Date: June 2012
>+Contact: Wanpeng Li <[email protected]>
>+Description: Since pdflush is replaced by per-BDI flusher, the interface of old pdflush
>+ exported in /proc/sys/vm/ should be removed.
>diff --git a/Documentation/feature-removal-schedule.txt b/Documentation/feature-removal-schedule.txt
>index e4b5775..7eea1ee 100644
>--- a/Documentation/feature-removal-schedule.txt
>+++ b/Documentation/feature-removal-schedule.txt
>@@ -6,6 +6,14 @@ be removed from this file.
>
> ---------------------------
>
>+What: /proc/sys/vm/nr_pdflush_threads
>+When: 2012
>+Why: Since pdflush is deprecated, the interface exported in /proc/sys/vm/
>+ should be removed.
>+Who: Wanpeng Li <[email protected]>
>+
>+---------------------------
>+
> What: CONFIG_APM_CPU_IDLE, and its ability to call APM BIOS in idle
> When: 2012
> Why: This optional sub-feature of APM is of dubious reliability,
>diff --git a/Documentation/sysctl/vm.txt b/Documentation/sysctl/vm.txt
>index 96f0ee8..71c17d2 100644
>--- a/Documentation/sysctl/vm.txt
>+++ b/Documentation/sysctl/vm.txt
>@@ -42,7 +42,6 @@ Currently, these files are in /proc/sys/vm:
> - mmap_min_addr
> - nr_hugepages
> - nr_overcommit_hugepages
>-- nr_pdflush_threads
> - nr_trim_pages (only if CONFIG_MMU=n)
> - numa_zonelist_order
> - oom_dump_tasks
>@@ -426,16 +425,6 @@ See Documentation/vm/hugetlbpage.txt
>
> ==============================================================
>
>-nr_pdflush_threads
>-
>-The current number of pdflush threads. This value is read-only.
>-The value changes according to the number of dirty pages in the system.
>-
>-When necessary, additional pdflush threads are created, one per second, up to
>-nr_pdflush_threads_max.
>-
>-==============================================================
>-
> nr_trim_pages
>
> This is available only on NOMMU kernels.
>diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
>index 539f36c..30ddb8a 100644
>--- a/fs/fs-writeback.c
>+++ b/fs/fs-writeback.c
>@@ -52,11 +52,6 @@ struct wb_writeback_work {
> struct completion *done; /* set if the caller waits */
> };
>
>-/*
>- * We don't actually have pdflush, but this one is exported though /proc...
>- */
>-int nr_pdflush_threads;
>-
> /**
> * writeback_in_progress - determine whether there is writeback in progress
> * @bdi: the device's backing_dev_info structure.
>diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
>index c34b4c8..f25be74 100644
>--- a/include/linux/sysctl.h
>+++ b/include/linux/sysctl.h
>@@ -964,6 +964,9 @@ extern int proc_doulongvec_ms_jiffies_minmax(struct ctl_table *table, int,
> void __user *, size_t *, loff_t *);
> extern int proc_do_large_bitmap(struct ctl_table *, int,
> void __user *, size_t *, loff_t *);
>+extern int proc_deprecated(struct ctl_table *, int,
>+ void __user *, size_t *, loff_t *);
>+
>
> /*
> * Register a set of sysctl names by calling register_sysctl_table
>diff --git a/include/linux/writeback.h b/include/linux/writeback.h
>index a2b84f5..13b5df7 100644
>--- a/include/linux/writeback.h
>+++ b/include/linux/writeback.h
>@@ -193,9 +193,4 @@ void tag_pages_for_writeback(struct address_space *mapping,
>
> void account_page_redirty(struct page *page);
>
>-/* pdflush.c */
>-extern int nr_pdflush_threads; /* Global so it can be exported to sysctl
>- read-only. */
>-
>-
> #endif /* WRITEBACK_H */
>diff --git a/kernel/sysctl.c b/kernel/sysctl.c
>index 4ab1187..1ab53cb 100644
>--- a/kernel/sysctl.c
>+++ b/kernel/sysctl.c
>@@ -1095,11 +1095,9 @@ static struct ctl_table vm_table[] = {
> .extra1 = &zero,
> },
> {
>- .procname = "nr_pdflush_threads",
>- .data = &nr_pdflush_threads,
>- .maxlen = sizeof nr_pdflush_threads,
>- .mode = 0444 /* read-only*/,
>- .proc_handler = proc_dointvec,
>+ .procname = "nr_pdflush_threads",
>+ .mode = 0444 /* read-only */,
>+ .proc_handler = proc_deprecated,
> },
> {
> .procname = "swappiness",
>@@ -2505,6 +2503,15 @@ int proc_doulongvec_ms_jiffies_minmax(struct ctl_table *table, int write,
>
> #endif /* CONFIG_PROC_SYSCTL */
>
>+/* notice associated proc deprecated */
>+int proc_deprecated(struct ctl_table *table, int write,
>+ void __user *buffer, size_t *lenp, loff_t *ppos)
>+{
>+ printk(KERN_WARNING "%s exported in /proc is deprecated\n",
>+ table->procname);
>+ return -ENOSYS;
>+}
>+
> /*
> * No sense putting this after each symbol definition, twice,
> * exception granted :-)
>@@ -2517,3 +2524,4 @@ EXPORT_SYMBOL(proc_dointvec_ms_jiffies);
> EXPORT_SYMBOL(proc_dostring);
> EXPORT_SYMBOL(proc_doulongvec_minmax);
> EXPORT_SYMBOL(proc_doulongvec_ms_jiffies_minmax);
>+EXPORT_SYMBOL(proc_deprecated);
>diff --git a/kernel/sysctl_binary.c b/kernel/sysctl_binary.c
>index a650694..65bdcf1 100644
>--- a/kernel/sysctl_binary.c
>+++ b/kernel/sysctl_binary.c
>@@ -147,7 +147,7 @@ static const struct bin_table bin_vm_table[] = {
> { CTL_INT, VM_DIRTY_RATIO, "dirty_ratio" },
> /* VM_DIRTY_WB_CS "dirty_writeback_centisecs" no longer used */
> /* VM_DIRTY_EXPIRE_CS "dirty_expire_centisecs" no longer used */
>- { CTL_INT, VM_NR_PDFLUSH_THREADS, "nr_pdflush_threads" },
>+ /* VM_NR_PDFLUSH_THREADS "nr_pdflush_threads" no longer used */
> { CTL_INT, VM_OVERCOMMIT_RATIO, "overcommit_ratio" },
> /* VM_PAGEBUF unused */
> /* VM_HUGETLB_PAGES "nr_hugepages" no longer used */
>--
>1.7.9.5
>
On Tue, 5 Jun 2012 17:26:11 +0800
Wanpeng Li <[email protected]> wrote:
> From: Wanpeng Li <[email protected]>
>
> ------------------
>
> Change in v2:
>
> * add printk warning
> * add description in Documentation
>
This is not a changelog. The patch should be accompanied by text
explaining the reasons for the removal, the back-compatibility issues,
how they were addressed, etc.
I do agree with the intent of the patch and the creation of
proc_deprecated() seems a good idea - something we can use in the
future.
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -1095,11 +1095,9 @@ static struct ctl_table vm_table[] = {
> .extra1 = &zero,
> },
> {
> - .procname = "nr_pdflush_threads",
> - .data = &nr_pdflush_threads,
> - .maxlen = sizeof nr_pdflush_threads,
> - .mode = 0444 /* read-only*/,
> - .proc_handler = proc_dointvec,
> + .procname = "nr_pdflush_threads",
> + .mode = 0444 /* read-only */,
> + .proc_handler = proc_deprecated,
> },
> {
> .procname = "swappiness",
> @@ -2505,6 +2503,15 @@ int proc_doulongvec_ms_jiffies_minmax(struct ctl_table *table, int write,
>
> #endif /* CONFIG_PROC_SYSCTL */
>
> +/* notice associated proc deprecated */
> +int proc_deprecated(struct ctl_table *table, int write,
> + void __user *buffer, size_t *lenp, loff_t *ppos)
> +{
> + printk(KERN_WARNING "%s exported in /proc is deprecated\n",
> + table->procname);
> + return -ENOSYS;
> +}
I see a couple of things here.
Firstly, I'd change the text from "deprecated" to "is scheduled for
removal". Which implies that the function should be called
proc_obsolete().
Secondly, this code will permit unprivileged users to flood the logs,
by repeatedly reading /proc/sys/vm/nr_pdflush_threads. We try to avoid
this, as it is a form of denial-of-service attack.
This is a bit hard to fix. The typical way of addressing this is to
use printk_once(), so the message only appears once per boot. But that
doesn't work for a generic function - we'd need to add one bit of state
to the ctl_table to do this. We can of course do that, but it's not
obvious that it's _worth_ doing that just for handling obsolete
entries.
So perhaps the solution is to give up on the generic proc_obsolete()
idea, and just add a handler specifically for nr_pdflush_threads, whcih
uses printk_once().
On Tue, Jun 05, 2012 at 10:45:20PM +0800, Gavin Shan wrote:
Hi Gavin,
>
>- Why not just remove the proc entry since we needn't it anymore?
Some user applications maybe still use this interface, so we should take
attention to back-compatibility issues.
>- If we really want to keep the entry and just output warning message, we *only*
> can see the message out of system console. It's reasonable to put the message
> into the buffer directly so that any users from system console/telnet/ssh can
> see the warning message?
Maybe this is a good idea.
>
>>From: Wanpeng Li <[email protected]>
>>
>>------------------
>>
>>Change in v2:
>>
>>* add printk warning
>>* add description in Documentation
>>
>>------------------
>>
>>Signed-off-by: Wanpeng Li <[email protected]>
>>---
>> .../ABI/obsolete/proc-sys-vm-nr_pdflush_threads | 5 +++++
>> Documentation/feature-removal-schedule.txt | 8 ++++++++
>> Documentation/sysctl/vm.txt | 11 -----------
>> fs/fs-writeback.c | 5 -----
>> include/linux/sysctl.h | 3 +++
>> include/linux/writeback.h | 5 -----
>> kernel/sysctl.c | 18 +++++++++++++-----
>> kernel/sysctl_binary.c | 2 +-
>> 8 files changed, 30 insertions(+), 27 deletions(-)
>> create mode 100644 Documentation/ABI/obsolete/proc-sys-vm-nr_pdflush_threads
>>
>>diff --git a/Documentation/ABI/obsolete/proc-sys-vm-nr_pdflush_threads b/Documentation/ABI/obsolete/proc-sys-vm-nr_pdflush_threads
>>new file mode 100644
>>index 0000000..edbe548
>>--- /dev/null
>>+++ b/Documentation/ABI/obsolete/proc-sys-vm-nr_pdflush_threads
>>@@ -0,0 +1,5 @@
>>+What: /proc/sys/vm/nr_pdflush_threads
>>+Date: June 2012
>>+Contact: Wanpeng Li <[email protected]>
>>+Description: Since pdflush is replaced by per-BDI flusher, the interface of old pdflush
>>+ exported in /proc/sys/vm/ should be removed.
>>diff --git a/Documentation/feature-removal-schedule.txt b/Documentation/feature-removal-schedule.txt
>>index e4b5775..7eea1ee 100644
>>--- a/Documentation/feature-removal-schedule.txt
>>+++ b/Documentation/feature-removal-schedule.txt
>>@@ -6,6 +6,14 @@ be removed from this file.
>>
>> ---------------------------
>>
>>+What: /proc/sys/vm/nr_pdflush_threads
>>+When: 2012
>>+Why: Since pdflush is deprecated, the interface exported in /proc/sys/vm/
>>+ should be removed.
>>+Who: Wanpeng Li <[email protected]>
>>+
>>+---------------------------
>>+
>> What: CONFIG_APM_CPU_IDLE, and its ability to call APM BIOS in idle
>> When: 2012
>> Why: This optional sub-feature of APM is of dubious reliability,
>>diff --git a/Documentation/sysctl/vm.txt b/Documentation/sysctl/vm.txt
>>index 96f0ee8..71c17d2 100644
>>--- a/Documentation/sysctl/vm.txt
>>+++ b/Documentation/sysctl/vm.txt
>>@@ -42,7 +42,6 @@ Currently, these files are in /proc/sys/vm:
>> - mmap_min_addr
>> - nr_hugepages
>> - nr_overcommit_hugepages
>>-- nr_pdflush_threads
>> - nr_trim_pages (only if CONFIG_MMU=n)
>> - numa_zonelist_order
>> - oom_dump_tasks
>>@@ -426,16 +425,6 @@ See Documentation/vm/hugetlbpage.txt
>>
>> ==============================================================
>>
>>-nr_pdflush_threads
>>-
>>-The current number of pdflush threads. This value is read-only.
>>-The value changes according to the number of dirty pages in the system.
>>-
>>-When necessary, additional pdflush threads are created, one per second, up to
>>-nr_pdflush_threads_max.
>>-
>>-==============================================================
>>-
>> nr_trim_pages
>>
>> This is available only on NOMMU kernels.
>>diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
>>index 539f36c..30ddb8a 100644
>>--- a/fs/fs-writeback.c
>>+++ b/fs/fs-writeback.c
>>@@ -52,11 +52,6 @@ struct wb_writeback_work {
>> struct completion *done; /* set if the caller waits */
>> };
>>
>>-/*
>>- * We don't actually have pdflush, but this one is exported though /proc...
>>- */
>>-int nr_pdflush_threads;
>>-
>> /**
>> * writeback_in_progress - determine whether there is writeback in progress
>> * @bdi: the device's backing_dev_info structure.
>>diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
>>index c34b4c8..f25be74 100644
>>--- a/include/linux/sysctl.h
>>+++ b/include/linux/sysctl.h
>>@@ -964,6 +964,9 @@ extern int proc_doulongvec_ms_jiffies_minmax(struct ctl_table *table, int,
>> void __user *, size_t *, loff_t *);
>> extern int proc_do_large_bitmap(struct ctl_table *, int,
>> void __user *, size_t *, loff_t *);
>>+extern int proc_deprecated(struct ctl_table *, int,
>>+ void __user *, size_t *, loff_t *);
>>+
>>
>> /*
>> * Register a set of sysctl names by calling register_sysctl_table
>>diff --git a/include/linux/writeback.h b/include/linux/writeback.h
>>index a2b84f5..13b5df7 100644
>>--- a/include/linux/writeback.h
>>+++ b/include/linux/writeback.h
>>@@ -193,9 +193,4 @@ void tag_pages_for_writeback(struct address_space *mapping,
>>
>> void account_page_redirty(struct page *page);
>>
>>-/* pdflush.c */
>>-extern int nr_pdflush_threads; /* Global so it can be exported to sysctl
>>- read-only. */
>>-
>>-
>> #endif /* WRITEBACK_H */
>>diff --git a/kernel/sysctl.c b/kernel/sysctl.c
>>index 4ab1187..1ab53cb 100644
>>--- a/kernel/sysctl.c
>>+++ b/kernel/sysctl.c
>>@@ -1095,11 +1095,9 @@ static struct ctl_table vm_table[] = {
>> .extra1 = &zero,
>> },
>> {
>>- .procname = "nr_pdflush_threads",
>>- .data = &nr_pdflush_threads,
>>- .maxlen = sizeof nr_pdflush_threads,
>>- .mode = 0444 /* read-only*/,
>>- .proc_handler = proc_dointvec,
>>+ .procname = "nr_pdflush_threads",
>>+ .mode = 0444 /* read-only */,
>>+ .proc_handler = proc_deprecated,
>> },
>> {
>> .procname = "swappiness",
>>@@ -2505,6 +2503,15 @@ int proc_doulongvec_ms_jiffies_minmax(struct ctl_table *table, int write,
>>
>> #endif /* CONFIG_PROC_SYSCTL */
>>
>>+/* notice associated proc deprecated */
>>+int proc_deprecated(struct ctl_table *table, int write,
>>+ void __user *buffer, size_t *lenp, loff_t *ppos)
>>+{
>>+ printk(KERN_WARNING "%s exported in /proc is deprecated\n",
>>+ table->procname);
>>+ return -ENOSYS;
>>+}
>>+
>> /*
>> * No sense putting this after each symbol definition, twice,
>> * exception granted :-)
>>@@ -2517,3 +2524,4 @@ EXPORT_SYMBOL(proc_dointvec_ms_jiffies);
>> EXPORT_SYMBOL(proc_dostring);
>> EXPORT_SYMBOL(proc_doulongvec_minmax);
>> EXPORT_SYMBOL(proc_doulongvec_ms_jiffies_minmax);
>>+EXPORT_SYMBOL(proc_deprecated);
>>diff --git a/kernel/sysctl_binary.c b/kernel/sysctl_binary.c
>>index a650694..65bdcf1 100644
>>--- a/kernel/sysctl_binary.c
>>+++ b/kernel/sysctl_binary.c
>>@@ -147,7 +147,7 @@ static const struct bin_table bin_vm_table[] = {
>> { CTL_INT, VM_DIRTY_RATIO, "dirty_ratio" },
>> /* VM_DIRTY_WB_CS "dirty_writeback_centisecs" no longer used */
>> /* VM_DIRTY_EXPIRE_CS "dirty_expire_centisecs" no longer used */
>>- { CTL_INT, VM_NR_PDFLUSH_THREADS, "nr_pdflush_threads" },
>>+ /* VM_NR_PDFLUSH_THREADS "nr_pdflush_threads" no longer used */
>> { CTL_INT, VM_OVERCOMMIT_RATIO, "overcommit_ratio" },
>> /* VM_PAGEBUF unused */
>> /* VM_HUGETLB_PAGES "nr_hugepages" no longer used */
>>--
>>1.7.9.5
>>
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>the body of a message to [email protected]
>More majordomo info at http://vger.kernel.org/majordomo-info.html
>Please read the FAQ at http://www.tux.org/lkml/
On Tue, Jun 05, 2012 at 03:28:45PM -0700, Andrew Morton wrote:
Hi Andrew,
>I do agree with the intent of the patch and the creation of
>proc_deprecated() seems a good idea - something we can use in the
>future.
Yes, actually I think many interfaces in /proc which will be removed in
the near future can take advantage of this function.
>
>> --- a/kernel/sysctl.c
>> +++ b/kernel/sysctl.c
>> @@ -1095,11 +1095,9 @@ static struct ctl_table vm_table[] = {
>> .extra1 = &zero,
>> },
>> {
>> - .procname = "nr_pdflush_threads",
>> - .data = &nr_pdflush_threads,
>> - .maxlen = sizeof nr_pdflush_threads,
>> - .mode = 0444 /* read-only*/,
>> - .proc_handler = proc_dointvec,
>> + .procname = "nr_pdflush_threads",
>> + .mode = 0444 /* read-only */,
>> + .proc_handler = proc_deprecated,
>> },
>> {
>> .procname = "swappiness",
>> @@ -2505,6 +2503,15 @@ int proc_doulongvec_ms_jiffies_minmax(struct ctl_table *table, int write,
>>
>> #endif /* CONFIG_PROC_SYSCTL */
>>
>> +/* notice associated proc deprecated */
>> +int proc_deprecated(struct ctl_table *table, int write,
>> + void __user *buffer, size_t *lenp, loff_t *ppos)
>> +{
>> + printk(KERN_WARNING "%s exported in /proc is deprecated\n",
>> + table->procname);
>> + return -ENOSYS;
>> +}
>
>I see a couple of things here.
>
>Firstly, I'd change the text from "deprecated" to "is scheduled for
>removal". Which implies that the function should be called
>proc_obsolete().
>
>
>Secondly, this code will permit unprivileged users to flood the logs,
>by repeatedly reading /proc/sys/vm/nr_pdflush_threads. We try to avoid
>this, as it is a form of denial-of-service attack.
>
>This is a bit hard to fix. The typical way of addressing this is to
>use printk_once(), so the message only appears once per boot. But that
>doesn't work for a generic function - we'd need to add one bit of state
>to the ctl_table to do this. We can of course do that, but it's not
>obvious that it's _worth_ doing that just for handling obsolete
>entries.
>
>So perhaps the solution is to give up on the generic proc_obsolete()
>idea, and just add a handler specifically for nr_pdflush_threads, whcih
>uses printk_once().
What about modify the generic proc_obsolete just to put the warning message into
the buffer, then transfer to userspace, in order to users can see this warning.Do
you think this is a better idea?
Regards,
Wanpeng
On Wed, 6 Jun 2012 11:21:27 +0800 Wanpeng Li <[email protected]> wrote:
> >Secondly, this code will permit unprivileged users to flood the logs,
> >by repeatedly reading /proc/sys/vm/nr_pdflush_threads. We try to avoid
> >this, as it is a form of denial-of-service attack.
> >
> >This is a bit hard to fix. The typical way of addressing this is to
> >use printk_once(), so the message only appears once per boot. But that
> >doesn't work for a generic function - we'd need to add one bit of state
> >to the ctl_table to do this. We can of course do that, but it's not
> >obvious that it's _worth_ doing that just for handling obsolete
> >entries.
> >
> >So perhaps the solution is to give up on the generic proc_obsolete()
> >idea, and just add a handler specifically for nr_pdflush_threads, whcih
> >uses printk_once().
>
> What about modify the generic proc_obsolete just to put the warning message into
> the buffer, then transfer to userspace, in order to users can see this warning.Do
> you think this is a better idea?
err, no. That's deliberately corrupting the output of an existing
interface. In response to this change userspace might crash, hand over
root privileges or tune to a porn channel. Instead we should yield
a well-formed response such as "0" (or maybe non-zero, if a zero
response might be unexpected) and inform the system operator via
conventional means. ie: syslog.
On 06/06/2012 01:28 AM, Andrew Morton wrote:
> On Tue, 5 Jun 2012 17:26:11 +0800
> Wanpeng Li <[email protected]> wrote:
>
> So perhaps the solution is to give up on the generic proc_obsolete()
> idea, and just add a handler specifically for nr_pdflush_threads, whcih
> uses printk_once().
>
If there are many users than a self made print once is worth it
we can use the ctl_table->data pointer for this, some thing like:
int nr_pdflush_was_printed;
@@ -1095,11 +1095,9 @@ static struct ctl_table vm_table[] = {
.extra1 = &zero,
},
{
- .procname = "nr_pdflush_threads",
- .data = &nr_pdflush_threads,
- .maxlen = sizeof nr_pdflush_threads,
- .mode = 0444 /* read-only*/,
- .proc_handler = proc_dointvec,
+ .data = &nr_pdflush_was_printed,
+ .procname = "nr_pdflush_threads",
+ .mode = 0444 /* read-only */,
+ .proc_handler = proc_obsolete,
},
+/* notice associated proc deprecated */
+int proc_deprecated(struct ctl_table *table, int write,
+ void __user *buffer, size_t *lenp, loff_t *ppos, )
+{
+ if (*ctl_table->data) {
+ printk(KERN_WARNING "%s exported in /proc is deprecated\n",
+ table->procname);
+ *ctl_table->data = 0;
+ }
+ return -ENOSYS;
+}
On 06/06/2012 10:31 AM, Boaz Harrosh wrote:
> On 06/06/2012 01:28 AM, Andrew Morton wrote:
>
>> On Tue, 5 Jun 2012 17:26:11 +0800
>> Wanpeng Li <[email protected]> wrote:
>>
>> So perhaps the solution is to give up on the generic proc_obsolete()
>> idea, and just add a handler specifically for nr_pdflush_threads, whcih
>> uses printk_once().
>>
>
>
> If there are many users than a self made print once is worth it
> we can use the ctl_table->data pointer for this, some thing like:
>
> int nr_pdflush_was_printed;
>
> @@ -1095,11 +1095,9 @@ static struct ctl_table vm_table[] = {
> .extra1 = &zero,
> },
> {
> - .procname = "nr_pdflush_threads",
> - .data = &nr_pdflush_threads,
> - .maxlen = sizeof nr_pdflush_threads,
> - .mode = 0444 /* read-only*/,
> - .proc_handler = proc_dointvec,
> + .data = &nr_pdflush_was_printed,
> + .procname = "nr_pdflush_threads",
> + .mode = 0444 /* read-only */,
> + .proc_handler = proc_obsolete,
> },
>
>
> +/* notice associated proc deprecated */
> +int proc_deprecated(struct ctl_table *table, int write,
> + void __user *buffer, size_t *lenp, loff_t *ppos, )
> +{
> + if (*ctl_table->data) {
> + printk(KERN_WARNING "%s exported in /proc is deprecated\n",
> + table->procname);
> + *ctl_table->data = 0;
+ *ctl_table->data = 1;
stupid me, time for coffee
Boaz
> + }
> + return -ENOSYS;
> +}
On 06/06/2012 10:31 AM, Boaz Harrosh wrote:
> On 06/06/2012 01:28 AM, Andrew Morton wrote:
>
>> On Tue, 5 Jun 2012 17:26:11 +0800
>> Wanpeng Li <[email protected]> wrote:
>>
>> So perhaps the solution is to give up on the generic proc_obsolete()
>> idea, and just add a handler specifically for nr_pdflush_threads, whcih
>> uses printk_once().
>>
>
>
> If there are many users than a self made print once is worth it
> we can use the ctl_table->data pointer for this, some thing like:
>
> int nr_pdflush_was_printed;
>
> @@ -1095,11 +1095,9 @@ static struct ctl_table vm_table[] = {
> .extra1 = &zero,
> },
> {
> - .procname = "nr_pdflush_threads",
> - .data = &nr_pdflush_threads,
> - .maxlen = sizeof nr_pdflush_threads,
> - .mode = 0444 /* read-only*/,
> - .proc_handler = proc_dointvec,
> + .data = &nr_pdflush_was_printed,
> + .procname = "nr_pdflush_threads",
> + .mode = 0444 /* read-only */,
> + .proc_handler = proc_obsolete,
> },
>
>
> +/* notice associated proc deprecated */
> +int proc_deprecated(struct ctl_table *table, int write,
> + void __user *buffer, size_t *lenp, loff_t *ppos, )
> +{
> + if (*ctl_table->data) {
+ if(!*ctl_table->data) {
> + printk(KERN_WARNING "%s exported in /proc is deprecated\n",
> + table->procname);
> + *ctl_table->data = 0;
+ *ctl_table->data = 1;
stupid me, time for coffee
Boaz
> + }
> + return -ENOSYS;
> +}
On Wed, Jun 06, 2012 at 10:37:34AM +0300, Boaz Harrosh wrote:
>On 06/06/2012 10:31 AM, Boaz Harrosh wrote:
>
>> On 06/06/2012 01:28 AM, Andrew Morton wrote:
>>
>>> On Tue, 5 Jun 2012 17:26:11 +0800
>>> Wanpeng Li <[email protected]> wrote:
>>>
>>> So perhaps the solution is to give up on the generic proc_obsolete()
>>> idea, and just add a handler specifically for nr_pdflush_threads, whcih
>>> uses printk_once().
>>>
>>
>>
>> If there are many users than a self made print once is worth it
>> we can use the ctl_table->data pointer for this, some thing like:
>>
>> int nr_pdflush_was_printed;
>>
>> @@ -1095,11 +1095,9 @@ static struct ctl_table vm_table[] = {
>> .extra1 = &zero,
>> },
>> {
>> - .procname = "nr_pdflush_threads",
>> - .data = &nr_pdflush_threads,
>> - .maxlen = sizeof nr_pdflush_threads,
>> - .mode = 0444 /* read-only*/,
>> - .proc_handler = proc_dointvec,
>> + .data = &nr_pdflush_was_printed,
>> + .procname = "nr_pdflush_threads",
>> + .mode = 0444 /* read-only */,
>> + .proc_handler = proc_obsolete,
>> },
>>
>>
>> +/* notice associated proc deprecated */
>> +int proc_deprecated(struct ctl_table *table, int write,
>> + void __user *buffer, size_t *lenp, loff_t *ppos, )
>> +{
>> + if (*ctl_table->data) {
>
>
>+ if(!*ctl_table->data) {
if(!*table->data) {
>
>> + printk(KERN_WARNING "%s exported in /proc is deprecated\n",
>> + table->procname);
>> + *ctl_table->data = 0;
>
>
>+ *ctl_table->data = 1;
*table->data = 1;
Regards,
Wanpeng Li
>
>stupid me, time for coffee
>
>Boaz
>
>> + }
>> + return -ENOSYS;
>> +}
>
>
On Wed, Jun 06, 2012 at 10:31:12AM +0300, Boaz Harrosh wrote:
>
>If there are many users than a self made print once is worth it
>we can use the ctl_table->data pointer for this, some thing like:
>
>int nr_pdflush_was_printed;
>
>@@ -1095,11 +1095,9 @@ static struct ctl_table vm_table[] = {
> .extra1 = &zero,
> },
> {
>- .procname = "nr_pdflush_threads",
>- .data = &nr_pdflush_threads,
>- .maxlen = sizeof nr_pdflush_threads,
>- .mode = 0444 /* read-only*/,
>- .proc_handler = proc_dointvec,
>+ .data = &nr_pdflush_was_printed,
>+ .procname = "nr_pdflush_threads",
>+ .mode = 0444 /* read-only */,
>+ .proc_handler = proc_obsolete,
> },
>
>
>+/* notice associated proc deprecated */
>+int proc_deprecated(struct ctl_table *table, int write,
>+ void __user *buffer, size_t *lenp, loff_t *ppos, )
>+{
>+ if (*ctl_table->data) {
>+ printk(KERN_WARNING "%s exported in /proc is deprecated\n",
>+ table->procname);
>+ *ctl_table->data = 0;
>+ }
>+ return -ENOSYS;
>+}
But this time you add another variable nr_pdflush_was_printed.
So remove variable nr_pdflush_threads does not make sense.
On Wed, Jun 06, 2012 at 06:55:52PM +0800, Wanpeng Li wrote:
> On Wed, Jun 06, 2012 at 10:31:12AM +0300, Boaz Harrosh wrote:
> >
> >If there are many users than a self made print once is worth it
> >we can use the ctl_table->data pointer for this, some thing like:
> >
> >int nr_pdflush_was_printed;
> >
> >@@ -1095,11 +1095,9 @@ static struct ctl_table vm_table[] = {
> > .extra1 = &zero,
> > },
> > {
> >- .procname = "nr_pdflush_threads",
> >- .data = &nr_pdflush_threads,
> >- .maxlen = sizeof nr_pdflush_threads,
> >- .mode = 0444 /* read-only*/,
> >- .proc_handler = proc_dointvec,
> >+ .data = &nr_pdflush_was_printed,
> >+ .procname = "nr_pdflush_threads",
> >+ .mode = 0444 /* read-only */,
> >+ .proc_handler = proc_obsolete,
> > },
> >
> >
> >+/* notice associated proc deprecated */
> >+int proc_deprecated(struct ctl_table *table, int write,
> >+ void __user *buffer, size_t *lenp, loff_t *ppos, )
> >+{
> >+ if (*ctl_table->data) {
> >+ printk(KERN_WARNING "%s exported in /proc is deprecated\n",
> >+ table->procname);
> >+ *ctl_table->data = 0;
> >+ }
> >+ return -ENOSYS;
> >+}
>
> But this time you add another variable nr_pdflush_was_printed.
> So remove variable nr_pdflush_threads does not make sense.
Yes that's exactly what we want: to warn about current
nr_pdflush_threads users for some releases, then to actually remove
the interface after the warning window passed. This allows potential
users to take action before the scheduled interface removal date.
Thanks,
Fengguang