[PATCH] drivers/iommu: don't select DEBUG_FS for AMD_IOMMU_STATS

[PATCH] drivers/iommu: don't select DEBUG_FS for AMD_IOMMU_STATS

[Paul Gortmaker]

We have at least one big banner telling people that they should
not deploy production kernels with DEBUG options enabled, but
at the same time, we make it hard for people to turn DEBUG_FS
off when we select (vs. depend on) the CONFIG_DEBUG_FS option.

Since we actively discourage people using debug-like features
on any builds that are production oriented (see trace_printk
banner for one example), so a generic sounding option should
not select DEBUG_FS.

Since this is not a system critical option, convert it from a
select to a depend -- this makes it nicer for us old school folk
who edit .config and then run "make oldconfig" as a (bad?) habit.

Cc: Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
Cc: "Steven Rostedt (Red Hat)" <rostedt-nx8X9YLhiw1AfugRpC6u6w@public.gmane.org>
Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
Signed-off-by: Paul Gortmaker <paul.gortmaker-CWA4WttNNZF54TAoqtyWWQ@public.gmane.org>
---
 drivers/iommu/Kconfig | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index dd1dc39f84ff..959032bdbe12 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -126,8 +126,7 @@ config AMD_IOMMU
 
 config AMD_IOMMU_STATS
 	bool "Export AMD IOMMU statistics to debugfs"
-	depends on AMD_IOMMU
-	select DEBUG_FS
+	depends on AMD_IOMMU && DEBUG_FS
 	---help---
 	  This option enables code in the AMD IOMMU driver to collect various
 	  statistics about whats happening in the driver and exports that
-- 
2.6.1

[PATCH] drivers/iommu: don't select DEBUG_FS for AMD_IOMMU_STATS

[Paul Gortmaker]

We have at least one big banner telling people that they should
not deploy production kernels with DEBUG options enabled, but
at the same time, we make it hard for people to turn DEBUG_FS
off when we select (vs. depend on) the CONFIG_DEBUG_FS option.

Since we actively discourage people using debug-like features
on any builds that are production oriented (see trace_printk
banner for one example), so a generic sounding option should
not select DEBUG_FS.

Since this is not a system critical option, convert it from a
select to a depend -- this makes it nicer for us old school folk
who edit .config and then run "make oldconfig" as a (bad?) habit.

Cc: Joerg Roedel <joro@8bytes.org>
Cc: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
Cc: iommu@lists.linux-foundation.org
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 drivers/iommu/Kconfig | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index dd1dc39f84ff..959032bdbe12 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -126,8 +126,7 @@ config AMD_IOMMU
 
 config AMD_IOMMU_STATS
 	bool "Export AMD IOMMU statistics to debugfs"
-	depends on AMD_IOMMU
-	select DEBUG_FS
+	depends on AMD_IOMMU && DEBUG_FS
 	---help---
 	  This option enables code in the AMD IOMMU driver to collect various
 	  statistics about whats happening in the driver and exports that
-- 
2.6.1

Re: [PATCH] drivers/iommu: don't select DEBUG_FS for AMD_IOMMU_STATS

[Joerg Roedel]

On Sat, Mar 26, 2016 at 09:18:44PM -0400, Paul Gortmaker wrote:
> We have at least one big banner telling people that they should
> not deploy production kernels with DEBUG options enabled, but
> at the same time, we make it hard for people to turn DEBUG_FS
> off when we select (vs. depend on) the CONFIG_DEBUG_FS option.
> 
> Since we actively discourage people using debug-like features
> on any builds that are production oriented (see trace_printk
> banner for one example), so a generic sounding option should
> not select DEBUG_FS.

That reasoning sounds a bit odd, as most production kernels have
DEBUG_FS enabled anyway, and I see no problem with that.

Disabling all debug features in 'production kernels' is a bit overkill.
Only if the feature has any runtime impact (performance, memory
consumption, security, ...) it makes sense to disable it for production
kernels.

Other features could stay enabled, and DEBUG_FS is one of them. For some
debug features we even don't offer a way to disable them, see BUG_ON,
WARN_ON and friends.


	Joerg

Re: [PATCH] drivers/iommu: don't select DEBUG_FS for AMD_IOMMU_STATS

[Joerg Roedel]

On Sat, Mar 26, 2016 at 09:18:44PM -0400, Paul Gortmaker wrote:
> We have at least one big banner telling people that they should
> not deploy production kernels with DEBUG options enabled, but
> at the same time, we make it hard for people to turn DEBUG_FS
> off when we select (vs. depend on) the CONFIG_DEBUG_FS option.
> 
> Since we actively discourage people using debug-like features
> on any builds that are production oriented (see trace_printk
> banner for one example), so a generic sounding option should
> not select DEBUG_FS.

That reasoning sounds a bit odd, as most production kernels have
DEBUG_FS enabled anyway, and I see no problem with that.

Disabling all debug features in 'production kernels' is a bit overkill.
Only if the feature has any runtime impact (performance, memory
consumption, security, ...) it makes sense to disable it for production
kernels.

Other features could stay enabled, and DEBUG_FS is one of them. For some
debug features we even don't offer a way to disable them, see BUG_ON,
WARN_ON and friends.


	Joerg

Re: [PATCH] drivers/iommu: don't select DEBUG_FS for AMD_IOMMU_STATS

[Steven Rostedt]

On Tue, 5 Apr 2016 17:19:51 +0200
Joerg Roedel <joro@8bytes.org> wrote:

> On Sat, Mar 26, 2016 at 09:18:44PM -0400, Paul Gortmaker wrote:
> > We have at least one big banner telling people that they should
> > not deploy production kernels with DEBUG options enabled, but
> > at the same time, we make it hard for people to turn DEBUG_FS
> > off when we select (vs. depend on) the CONFIG_DEBUG_FS option.
> > 
> > Since we actively discourage people using debug-like features
> > on any builds that are production oriented (see trace_printk
> > banner for one example), so a generic sounding option should
> > not select DEBUG_FS.  
> 
> That reasoning sounds a bit odd, as most production kernels have
> DEBUG_FS enabled anyway, and I see no problem with that.
> 
> Disabling all debug features in 'production kernels' is a bit overkill.
> Only if the feature has any runtime impact (performance, memory
> consumption, security, ...) it makes sense to disable it for production
> kernels.
> 
> Other features could stay enabled, and DEBUG_FS is one of them. For some
> debug features we even don't offer a way to disable them, see BUG_ON,
> WARN_ON and friends.
> 

I will argue that people have asked me to move tracing out of debugfs
(which is why I created tracefs) because the problem with debugfs is
that it opens up a entire system that is not well scrutinized, and
holds lots of possible ways to crack the kernel.

Disabling debugfs does help with the "security" point you mentioned
above.

-- Steve

Re: [PATCH] drivers/iommu: don't select DEBUG_FS for AMD_IOMMU_STATS

[Joerg Roedel]

On Tue, Apr 05, 2016 at 12:37:54PM -0400, Steven Rostedt wrote:
> I will argue that people have asked me to move tracing out of debugfs
> (which is why I created tracefs) because the problem with debugfs is
> that it opens up a entire system that is not well scrutinized, and
> holds lots of possible ways to crack the kernel.
> 
> Disabling debugfs does help with the "security" point you mentioned
> above.

Yes, that reasoning makes more sense than "debug features are bad for
production". It should've been part of the commit message.

The AMD_IOMMU_STATS feature is on my list of things to remove anyway, it
is mostly disabled and should be done differently (via per-device sysfs
stats). So I am just going to remove it for now.


	Joerg

Re: [PATCH] drivers/iommu: don't select DEBUG_FS for AMD_IOMMU_STATS

[Joerg Roedel]

On Tue, Apr 05, 2016 at 12:37:54PM -0400, Steven Rostedt wrote:
> I will argue that people have asked me to move tracing out of debugfs
> (which is why I created tracefs) because the problem with debugfs is
> that it opens up a entire system that is not well scrutinized, and
> holds lots of possible ways to crack the kernel.
> 
> Disabling debugfs does help with the "security" point you mentioned
> above.

Yes, that reasoning makes more sense than "debug features are bad for
production". It should've been part of the commit message.

The AMD_IOMMU_STATS feature is on my list of things to remove anyway, it
is mostly disabled and should be done differently (via per-device sysfs
stats). So I am just going to remove it for now.


	Joerg