Re: [PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create()
On Mon, 2012-07-30 at 13:18 +0300, Pekka Enberg wrote: > On Sat, Jul 14, 2012 at 2:12 AM, Shuah Khan wrote: > > The label oops is used in CONFIG_DEBUG_VM ifdef block and is defined > > outside ifdef CONFIG_DEBUG_VM block. This results in the following > > build warning when built with CONFIG_DEBUG_VM disabled. Fix to move > > label oops definition to inside a CONFIG_DEBUG_VM block. > > > > mm/slab_common.c: In function ‘kmem_cache_create’: > > mm/slab_common.c:101:1: warning: label ‘oops’ defined but not used > > [-Wunused-label] > > > > Signed-off-by: Shuah Khan > > I merged this as an obvious and safe fix for current merge window. We > need to clean this up properly for v3.7. Thanks for merging the obvious fix. I was on vacation for the last two weeks, and just got back. I sent another patch that restructures the debug and non-debug code right before I went on vacation. Didn't get a chance to look at the responses (if any). Will get working on following up and re-working and re-sending the patch as needed this week. -- Shuah -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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 TRIVIAL] mm: Fix build warning in kmem_cache_create()
On Mon, 2012-07-30 at 13:18 +0300, Pekka Enberg wrote: On Sat, Jul 14, 2012 at 2:12 AM, Shuah Khan shuah.k...@hp.com wrote: The label oops is used in CONFIG_DEBUG_VM ifdef block and is defined outside ifdef CONFIG_DEBUG_VM block. This results in the following build warning when built with CONFIG_DEBUG_VM disabled. Fix to move label oops definition to inside a CONFIG_DEBUG_VM block. mm/slab_common.c: In function ‘kmem_cache_create’: mm/slab_common.c:101:1: warning: label ‘oops’ defined but not used [-Wunused-label] Signed-off-by: Shuah Khan shuah.k...@hp.com I merged this as an obvious and safe fix for current merge window. We need to clean this up properly for v3.7. Thanks for merging the obvious fix. I was on vacation for the last two weeks, and just got back. I sent another patch that restructures the debug and non-debug code right before I went on vacation. Didn't get a chance to look at the responses (if any). Will get working on following up and re-working and re-sending the patch as needed this week. -- Shuah -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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 TRIVIAL] mm: Fix build warning in kmem_cache_create()
On Mon, 30 Jul 2012, David Rientjes wrote: > So much for compromise, I thought we had agreed that at least some of the > checks for !name, in_interrupt() or bad size values should be moved out > from under the #ifdef CONFIG_DEBUG_VM, but this wasn't done. This > discussion would be irrelevent if we actually did what we talked about. I didn't want to change the checks at the last minute and invalidate testing in linux-next but I'm more than happy to merge such a patch when the merge window closes. Pekka -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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 TRIVIAL] mm: Fix build warning in kmem_cache_create()
On Mon, 30 Jul 2012, David Rientjes wrote: So much for compromise, I thought we had agreed that at least some of the checks for !name, in_interrupt() or bad size values should be moved out from under the #ifdef CONFIG_DEBUG_VM, but this wasn't done. This discussion would be irrelevent if we actually did what we talked about. I didn't want to change the checks at the last minute and invalidate testing in linux-next but I'm more than happy to merge such a patch when the merge window closes. Pekka -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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 TRIVIAL] mm: Fix build warning in kmem_cache_create()
On Mon, 30 Jul 2012, Pekka Enberg wrote: > > -Wunused-label is overridden in gcc for a label that is conditionally > > referenced by using __maybe_unused in the kernel. I'm not sure what's so > > obscure about > > > > out: __maybe_unused > > > > Are label attributes really that obsecure? > > I think they are. > > The real problem, however, is that label attributes would just paper > over the badly thought out control flow in the function and not make the > code any better or easier to read. > So much for compromise, I thought we had agreed that at least some of the checks for !name, in_interrupt() or bad size values should be moved out from under the #ifdef CONFIG_DEBUG_VM, but this wasn't done. This discussion would be irrelevent if we actually did what we talked about. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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 TRIVIAL] mm: Fix build warning in kmem_cache_create()
On Mon, Jul 30, 2012 at 10:56 PM, David Rientjes wrote: > -Wunused-label is overridden in gcc for a label that is conditionally > referenced by using __maybe_unused in the kernel. I'm not sure what's so > obscure about > > out: __maybe_unused > > Are label attributes really that obsecure? I think they are. The real problem, however, is that label attributes would just paper over the badly thought out control flow in the function and not make the code any better or easier to read. Pekka -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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 TRIVIAL] mm: Fix build warning in kmem_cache_create()
On Mon, 30 Jul 2012, Pekka Enberg wrote: > > The label oops is used in CONFIG_DEBUG_VM ifdef block and is defined > > outside ifdef CONFIG_DEBUG_VM block. This results in the following > > build warning when built with CONFIG_DEBUG_VM disabled. Fix to move > > label oops definition to inside a CONFIG_DEBUG_VM block. > > > > mm/slab_common.c: In function ‘kmem_cache_create’: > > mm/slab_common.c:101:1: warning: label ‘oops’ defined but not used > > [-Wunused-label] > > > > Signed-off-by: Shuah Khan > > I merged this as an obvious and safe fix for current merge window. We > need to clean this up properly for v3.7. > -Wunused-label is overridden in gcc for a label that is conditionally referenced by using __maybe_unused in the kernel. I'm not sure what's so obscure about out: __maybe_unused Are label attributes really that obsecure?
Re: [PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create()
On Sat, Jul 14, 2012 at 2:12 AM, Shuah Khan wrote: > The label oops is used in CONFIG_DEBUG_VM ifdef block and is defined > outside ifdef CONFIG_DEBUG_VM block. This results in the following > build warning when built with CONFIG_DEBUG_VM disabled. Fix to move > label oops definition to inside a CONFIG_DEBUG_VM block. > > mm/slab_common.c: In function ‘kmem_cache_create’: > mm/slab_common.c:101:1: warning: label ‘oops’ defined but not used > [-Wunused-label] > > Signed-off-by: Shuah Khan I merged this as an obvious and safe fix for current merge window. We need to clean this up properly for v3.7. > --- > mm/slab_common.c |2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/mm/slab_common.c b/mm/slab_common.c > index 12637ce..aa3ca5b 100644 > --- a/mm/slab_common.c > +++ b/mm/slab_common.c > @@ -98,7 +98,9 @@ struct kmem_cache *kmem_cache_create(const char *name, > size_t size, size_t align > > s = __kmem_cache_create(name, size, align, flags, ctor); > > +#ifdef CONFIG_DEBUG_VM > oops: > +#endif > mutex_unlock(_mutex); > put_online_cpus(); > > -- > 1.7.9.5 > > > > -- > To unsubscribe, send a message with 'unsubscribe linux-mm' in > the body to majord...@kvack.org. For more info on Linux MM, > see: http://www.linux-mm.org/ . > Don't email: mailto:"d...@kvack.org;> em...@kvack.org -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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 TRIVIAL] mm: Fix build warning in kmem_cache_create()
On Sat, Jul 14, 2012 at 2:12 AM, Shuah Khan shuah.k...@hp.com wrote: The label oops is used in CONFIG_DEBUG_VM ifdef block and is defined outside ifdef CONFIG_DEBUG_VM block. This results in the following build warning when built with CONFIG_DEBUG_VM disabled. Fix to move label oops definition to inside a CONFIG_DEBUG_VM block. mm/slab_common.c: In function ‘kmem_cache_create’: mm/slab_common.c:101:1: warning: label ‘oops’ defined but not used [-Wunused-label] Signed-off-by: Shuah Khan shuah.k...@hp.com I merged this as an obvious and safe fix for current merge window. We need to clean this up properly for v3.7. --- mm/slab_common.c |2 ++ 1 file changed, 2 insertions(+) diff --git a/mm/slab_common.c b/mm/slab_common.c index 12637ce..aa3ca5b 100644 --- a/mm/slab_common.c +++ b/mm/slab_common.c @@ -98,7 +98,9 @@ struct kmem_cache *kmem_cache_create(const char *name, size_t size, size_t align s = __kmem_cache_create(name, size, align, flags, ctor); +#ifdef CONFIG_DEBUG_VM oops: +#endif mutex_unlock(slab_mutex); put_online_cpus(); -- 1.7.9.5 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majord...@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: a href=mailto:d...@kvack.org; em...@kvack.org /a -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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 TRIVIAL] mm: Fix build warning in kmem_cache_create()
On Mon, 30 Jul 2012, Pekka Enberg wrote: The label oops is used in CONFIG_DEBUG_VM ifdef block and is defined outside ifdef CONFIG_DEBUG_VM block. This results in the following build warning when built with CONFIG_DEBUG_VM disabled. Fix to move label oops definition to inside a CONFIG_DEBUG_VM block. mm/slab_common.c: In function ‘kmem_cache_create’: mm/slab_common.c:101:1: warning: label ‘oops’ defined but not used [-Wunused-label] Signed-off-by: Shuah Khan shuah.k...@hp.com I merged this as an obvious and safe fix for current merge window. We need to clean this up properly for v3.7. -Wunused-label is overridden in gcc for a label that is conditionally referenced by using __maybe_unused in the kernel. I'm not sure what's so obscure about out: __maybe_unused Are label attributes really that obsecure?
Re: [PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create()
On Mon, Jul 30, 2012 at 10:56 PM, David Rientjes rient...@google.com wrote: -Wunused-label is overridden in gcc for a label that is conditionally referenced by using __maybe_unused in the kernel. I'm not sure what's so obscure about out: __maybe_unused Are label attributes really that obsecure? I think they are. The real problem, however, is that label attributes would just paper over the badly thought out control flow in the function and not make the code any better or easier to read. Pekka -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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 TRIVIAL] mm: Fix build warning in kmem_cache_create()
On Mon, 30 Jul 2012, Pekka Enberg wrote: -Wunused-label is overridden in gcc for a label that is conditionally referenced by using __maybe_unused in the kernel. I'm not sure what's so obscure about out: __maybe_unused Are label attributes really that obsecure? I think they are. The real problem, however, is that label attributes would just paper over the badly thought out control flow in the function and not make the code any better or easier to read. So much for compromise, I thought we had agreed that at least some of the checks for !name, in_interrupt() or bad size values should be moved out from under the #ifdef CONFIG_DEBUG_VM, but this wasn't done. This discussion would be irrelevent if we actually did what we talked about. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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 TRIVIAL] mm: Fix build warning in kmem_cache_create()
On Mon, 23 Jul 2012, Glauber Costa wrote: > >> worth including unconditionally. Furthermore, the size related checks > >> certainly make sense and I don't see any harm in having them as well. > > > > There is a WARN_ON() there and then it returns NULL!!! Crazy. Causes a > > NULL pointer dereference later in the caller? > > > > It obviously depends on the caller. This is a violation of the calling convention to say the least. This means if you have SLAB_PANIC set and accidentally set the name to NULL the function will return despite the error and not panic! > Although most of the calls to kmem_cache_create are made from static > data, we can't assume that. Of course whoever is using static data > should do those very same tests from the outside to be safe, but in case > they do not, this seems to fall in the category of things that make > debugging easier - even if we later on get to a NULL pointer dereference. > > Your mentioned bias towards minimum code size, however, is totally > valid, IMHO. But I doubt those checks would introduce a huge footprint. > I would imagine you being much more concerned about being able to wipe > out entire subsystems like memcg, which will give you a lot more. They are useless checks since any use of the name will also cause a NULL pointer dereference. Same is true for interrupt checks. Checks like that indicate a deterioration of the code base. People are afraid that something goes wrong because they no longer understand the code so they build a embroidery around it instead of relying on the already existing checks at vital places. The embroidery can be useful for debugging thats why I left it in for the CONFIG_DEBUG_VM but certainly should not be included in production kernels. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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 TRIVIAL] mm: Fix build warning in kmem_cache_create()
On Mon, 23 Jul 2012, Glauber Costa wrote: worth including unconditionally. Furthermore, the size related checks certainly make sense and I don't see any harm in having them as well. There is a WARN_ON() there and then it returns NULL!!! Crazy. Causes a NULL pointer dereference later in the caller? It obviously depends on the caller. This is a violation of the calling convention to say the least. This means if you have SLAB_PANIC set and accidentally set the name to NULL the function will return despite the error and not panic! Although most of the calls to kmem_cache_create are made from static data, we can't assume that. Of course whoever is using static data should do those very same tests from the outside to be safe, but in case they do not, this seems to fall in the category of things that make debugging easier - even if we later on get to a NULL pointer dereference. Your mentioned bias towards minimum code size, however, is totally valid, IMHO. But I doubt those checks would introduce a huge footprint. I would imagine you being much more concerned about being able to wipe out entire subsystems like memcg, which will give you a lot more. They are useless checks since any use of the name will also cause a NULL pointer dereference. Same is true for interrupt checks. Checks like that indicate a deterioration of the code base. People are afraid that something goes wrong because they no longer understand the code so they build a embroidery around it instead of relying on the already existing checks at vital places. The embroidery can be useful for debugging thats why I left it in for the CONFIG_DEBUG_VM but certainly should not be included in production kernels. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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 TRIVIAL] mm: Fix build warning in kmem_cache_create()
On 07/17/2012 07:11 PM, Christoph Lameter wrote: > On Tue, 17 Jul 2012, Pekka Enberg wrote: > >> Well, even SLUB checks for !name in mainline so that's definitely >> worth including unconditionally. Furthermore, the size related checks >> certainly make sense and I don't see any harm in having them as well. > > There is a WARN_ON() there and then it returns NULL!!! Crazy. Causes a > NULL pointer dereference later in the caller? > It obviously depends on the caller. Although most of the calls to kmem_cache_create are made from static data, we can't assume that. Of course whoever is using static data should do those very same tests from the outside to be safe, but in case they do not, this seems to fall in the category of things that make debugging easier - even if we later on get to a NULL pointer dereference. Your mentioned bias towards minimum code size, however, is totally valid, IMHO. But I doubt those checks would introduce a huge footprint. I would imagine you being much more concerned about being able to wipe out entire subsystems like memcg, which will give you a lot more. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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 TRIVIAL] mm: Fix build warning in kmem_cache_create()
On 07/17/2012 07:11 PM, Christoph Lameter wrote: On Tue, 17 Jul 2012, Pekka Enberg wrote: Well, even SLUB checks for !name in mainline so that's definitely worth including unconditionally. Furthermore, the size related checks certainly make sense and I don't see any harm in having them as well. There is a WARN_ON() there and then it returns NULL!!! Crazy. Causes a NULL pointer dereference later in the caller? It obviously depends on the caller. Although most of the calls to kmem_cache_create are made from static data, we can't assume that. Of course whoever is using static data should do those very same tests from the outside to be safe, but in case they do not, this seems to fall in the category of things that make debugging easier - even if we later on get to a NULL pointer dereference. Your mentioned bias towards minimum code size, however, is totally valid, IMHO. But I doubt those checks would introduce a huge footprint. I would imagine you being much more concerned about being able to wipe out entire subsystems like memcg, which will give you a lot more. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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 TRIVIAL] mm: Fix build warning in kmem_cache_create()
On Tue, 2012-07-17 at 17:46 +0300, Pekka Enberg wrote:
> On Mon, 16 Jul 2012, David Rientjes wrote:
> >> > The kernel cannot check everything and will blow up in unexpected ways if
> >> > someone codes something stupid. There are numerous debugging options that
> >> > need to be switched on to get better debugging information to investigate
> >> > deper. Adding special code to replicate these checks is bad.
> >>
> >> Disagree, CONFIG_SLAB does not blow up for a NULL name string and just
> >> corrupts userspace.
>
> On Tue, Jul 17, 2012 at 5:36 PM, Christoph Lameter wrote:
> > Ohh.. So far we only had science fiction. Now kernel fiction If you
> > could corrupt userspace using sysfs with a NULL string then you'd first
> > need to fix sysfs support.
> >
> > And if you really want to be totally safe then I guess you need to audit
> > the kernel and make sure that every core kernel function that takes a
> > string argument does check for it to be NULL just in case.
>
> Well, even SLUB checks for !name in mainline so that's definitely
> worth including unconditionally. Furthermore, the size related checks
> certainly make sense and I don't see any harm in having them as well.
>
> As for "in_interrupt()", I really don't see the point in keeping that
> around. We could push it down to mm/slab.c in "__kmem_cache_create()"
> if we wanted to.
Is it safe to hold slab_mutex when in interrupt context? Pushing
in_interrupt() check down to "__kmem_cache_create()" would mean, this
check is done while holding slab_mutex. If it is not safe to be in
interrupt context, then it would a bit late to do the check?
Also all of these checks (as I see it) will allow kmem_cache_create() to
fail gracefully. I understand that kernel doesn't do this type checking
consistently, guess that is larger scope issue. Does it make sense to do
these checks in this particular case?
I am working on two different restructuring options:
1. Move all of the debug code and the regular code into
kmem_cache_create_debug() which is called from kmem_cache_create() in
ifdef CONFIG_DEBUG block and push the regular code into #else case.
I don't like this a whole lot because of the duplication of normal code
path. However, this seems to be better than the second alternative,
because of the complexity involved in taking code paths based on where
the sanity checks failed.
2. Move just the debug code block that does sanity checks on slab_caches
list and have it return failure which will result in the
mutex_unlock(_mutex);
put_online_cpus();
if (!s && (flags & SLAB_PANIC))
panic("kmem_cache_create: Failed to create slab '%s'\n",
name):
get executed from the regular code path. I like this option because it
there is no duplication of regular code. However, if for any reason
debug code changes and results conditional code paths taken after
return, it will get very complex.
Any thoughts, should I send RFC patches so we can discuss the code.
-- Shuah
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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 TRIVIAL] mm: Fix build warning in kmem_cache_create()
On Tue, 17 Jul 2012, Pekka Enberg wrote: > Well, even SLUB checks for !name in mainline so that's definitely > worth including unconditionally. Furthermore, the size related checks > certainly make sense and I don't see any harm in having them as well. There is a WARN_ON() there and then it returns NULL!!! Crazy. Causes a NULL pointer dereference later in the caller? > As for "in_interrupt()", I really don't see the point in keeping that > around. We could push it down to mm/slab.c in "__kmem_cache_create()" > if we wanted to. Ok we could do that but I guess we are in the discussion of how much checking should be done for a production kernel. I think these checks are way out of hand. We cannot afford to consistently check parameters to all kernel functions in production. We will only do these checks in a select manner if these values could result in serious difficult to debug problems. The checks in slab look like debugging code that someone needed for a specific debugging scenario. I can understand that we would keep that in for development but not for production. Maybe I am a bit biased but my prod kernels need to have minimal memory footprint due to excessive code size causing regressions. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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 TRIVIAL] mm: Fix build warning in kmem_cache_create()
On Mon, 16 Jul 2012, David Rientjes wrote: >> > The kernel cannot check everything and will blow up in unexpected ways if >> > someone codes something stupid. There are numerous debugging options that >> > need to be switched on to get better debugging information to investigate >> > deper. Adding special code to replicate these checks is bad. >> >> Disagree, CONFIG_SLAB does not blow up for a NULL name string and just >> corrupts userspace. On Tue, Jul 17, 2012 at 5:36 PM, Christoph Lameter wrote: > Ohh.. So far we only had science fiction. Now kernel fiction If you > could corrupt userspace using sysfs with a NULL string then you'd first > need to fix sysfs support. > > And if you really want to be totally safe then I guess you need to audit > the kernel and make sure that every core kernel function that takes a > string argument does check for it to be NULL just in case. Well, even SLUB checks for !name in mainline so that's definitely worth including unconditionally. Furthermore, the size related checks certainly make sense and I don't see any harm in having them as well. As for "in_interrupt()", I really don't see the point in keeping that around. We could push it down to mm/slab.c in "__kmem_cache_create()" if we wanted to. Pekka -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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 TRIVIAL] mm: Fix build warning in kmem_cache_create()
On Mon, 16 Jul 2012, David Rientjes wrote: > > The kernel cannot check everything and will blow up in unexpected ways if > > someone codes something stupid. There are numerous debugging options that > > need to be switched on to get better debugging information to investigate > > deper. Adding special code to replicate these checks is bad. > > > > Disagree, CONFIG_SLAB does not blow up for a NULL name string and just > corrupts userspace. Ohh.. So far we only had science fiction. Now kernel fiction If you could corrupt userspace using sysfs with a NULL string then you'd first need to fix sysfs support. And if you really want to be totally safe then I guess you need to audit the kernel and make sure that every core kernel function that takes a string argument does check for it to be NULL just in case. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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 TRIVIAL] mm: Fix build warning in kmem_cache_create()
On Mon, 16 Jul 2012, David Rientjes wrote: The kernel cannot check everything and will blow up in unexpected ways if someone codes something stupid. There are numerous debugging options that need to be switched on to get better debugging information to investigate deper. Adding special code to replicate these checks is bad. Disagree, CONFIG_SLAB does not blow up for a NULL name string and just corrupts userspace. Ohh.. So far we only had science fiction. Now kernel fiction If you could corrupt userspace using sysfs with a NULL string then you'd first need to fix sysfs support. And if you really want to be totally safe then I guess you need to audit the kernel and make sure that every core kernel function that takes a string argument does check for it to be NULL just in case. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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 TRIVIAL] mm: Fix build warning in kmem_cache_create()
On Mon, 16 Jul 2012, David Rientjes wrote: The kernel cannot check everything and will blow up in unexpected ways if someone codes something stupid. There are numerous debugging options that need to be switched on to get better debugging information to investigate deper. Adding special code to replicate these checks is bad. Disagree, CONFIG_SLAB does not blow up for a NULL name string and just corrupts userspace. On Tue, Jul 17, 2012 at 5:36 PM, Christoph Lameter c...@linux.com wrote: Ohh.. So far we only had science fiction. Now kernel fiction If you could corrupt userspace using sysfs with a NULL string then you'd first need to fix sysfs support. And if you really want to be totally safe then I guess you need to audit the kernel and make sure that every core kernel function that takes a string argument does check for it to be NULL just in case. Well, even SLUB checks for !name in mainline so that's definitely worth including unconditionally. Furthermore, the size related checks certainly make sense and I don't see any harm in having them as well. As for in_interrupt(), I really don't see the point in keeping that around. We could push it down to mm/slab.c in __kmem_cache_create() if we wanted to. Pekka -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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 TRIVIAL] mm: Fix build warning in kmem_cache_create()
On Tue, 17 Jul 2012, Pekka Enberg wrote: Well, even SLUB checks for !name in mainline so that's definitely worth including unconditionally. Furthermore, the size related checks certainly make sense and I don't see any harm in having them as well. There is a WARN_ON() there and then it returns NULL!!! Crazy. Causes a NULL pointer dereference later in the caller? As for in_interrupt(), I really don't see the point in keeping that around. We could push it down to mm/slab.c in __kmem_cache_create() if we wanted to. Ok we could do that but I guess we are in the discussion of how much checking should be done for a production kernel. I think these checks are way out of hand. We cannot afford to consistently check parameters to all kernel functions in production. We will only do these checks in a select manner if these values could result in serious difficult to debug problems. The checks in slab look like debugging code that someone needed for a specific debugging scenario. I can understand that we would keep that in for development but not for production. Maybe I am a bit biased but my prod kernels need to have minimal memory footprint due to excessive code size causing regressions. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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 TRIVIAL] mm: Fix build warning in kmem_cache_create()
On Tue, 2012-07-17 at 17:46 +0300, Pekka Enberg wrote: On Mon, 16 Jul 2012, David Rientjes wrote: The kernel cannot check everything and will blow up in unexpected ways if someone codes something stupid. There are numerous debugging options that need to be switched on to get better debugging information to investigate deper. Adding special code to replicate these checks is bad. Disagree, CONFIG_SLAB does not blow up for a NULL name string and just corrupts userspace. On Tue, Jul 17, 2012 at 5:36 PM, Christoph Lameter c...@linux.com wrote: Ohh.. So far we only had science fiction. Now kernel fiction If you could corrupt userspace using sysfs with a NULL string then you'd first need to fix sysfs support. And if you really want to be totally safe then I guess you need to audit the kernel and make sure that every core kernel function that takes a string argument does check for it to be NULL just in case. Well, even SLUB checks for !name in mainline so that's definitely worth including unconditionally. Furthermore, the size related checks certainly make sense and I don't see any harm in having them as well. As for in_interrupt(), I really don't see the point in keeping that around. We could push it down to mm/slab.c in __kmem_cache_create() if we wanted to. Is it safe to hold slab_mutex when in interrupt context? Pushing in_interrupt() check down to __kmem_cache_create() would mean, this check is done while holding slab_mutex. If it is not safe to be in interrupt context, then it would a bit late to do the check? Also all of these checks (as I see it) will allow kmem_cache_create() to fail gracefully. I understand that kernel doesn't do this type checking consistently, guess that is larger scope issue. Does it make sense to do these checks in this particular case? I am working on two different restructuring options: 1. Move all of the debug code and the regular code into kmem_cache_create_debug() which is called from kmem_cache_create() in ifdef CONFIG_DEBUG block and push the regular code into #else case. I don't like this a whole lot because of the duplication of normal code path. However, this seems to be better than the second alternative, because of the complexity involved in taking code paths based on where the sanity checks failed. 2. Move just the debug code block that does sanity checks on slab_caches list and have it return failure which will result in the mutex_unlock(slab_mutex); put_online_cpus(); if (!s (flags SLAB_PANIC)) panic(kmem_cache_create: Failed to create slab '%s'\n, name): get executed from the regular code path. I like this option because it there is no duplication of regular code. However, if for any reason debug code changes and results conditional code paths taken after return, it will get very complex. Any thoughts, should I send RFC patches so we can discuss the code. -- Shuah -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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 TRIVIAL] mm: Fix build warning in kmem_cache_create()
On Mon, 16 Jul 2012, Christoph Lameter wrote: > > Sounds like a response from someone who is very familiar with slab > > allocators. The reality, though, is that very few people are going to be > > doing development with CONFIG_DEBUG_VM enabled unless they notice problems > > beforehand. > > Kernels are certainly run with CONFIG_DEBUG_VM before merges to mainstream > occur. If the developer does not do it then someone else will. > So let's say a developer wants to pass a dynamically allocated string to kmem_cache_create() for the cache name and it happens to be NULL because of a failed allocation but this never happened in testing (or it does happen but CONFIG_DEBUG_VM=n) and they are using CONFIG_SLAB. What would the failure be in linux-next? It looks like it would just result in a corrupted slabinfo. Bad result, we used to catch this problem before the extraction of common functionality and now we've allowed a corrupted slabinfo for nothing: optimizing kmem_cache_create() is pointless. > The kernel cannot check everything and will blow up in unexpected ways if > someone codes something stupid. There are numerous debugging options that > need to be switched on to get better debugging information to investigate > deper. Adding special code to replicate these checks is bad. > Disagree, CONFIG_SLAB does not blow up for a NULL name string and just corrupts userspace. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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 TRIVIAL] mm: Fix build warning in kmem_cache_create()
On Mon, 16 Jul 2012, David Rientjes wrote: > > These checks are useless for regular kernel operations. They are > > only useful when developing code and should only be enabled during > > development. There is no point in testing the size and the name which are > > typically constant when a slab is created with a stable kernel. > > > > Sounds like a response from someone who is very familiar with slab > allocators. The reality, though, is that very few people are going to be > doing development with CONFIG_DEBUG_VM enabled unless they notice problems > beforehand. Kernels are certainly run with CONFIG_DEBUG_VM before merges to mainstream occur. If the developer does not do it then someone else will. > Are you seriously trying to optimize kmem_cache_create()? These checks > certainly aren't going to hurt your perfromance and it seems appropriate > to do some sanity checking before blowing up in unexpected ways. It's > also the way it's been done for years before extracting common allocator > functions to their own file. The kernel cannot check everything and will blow up in unexpected ways if someone codes something stupid. There are numerous debugging options that need to be switched on to get better debugging information to investigate deper. Adding special code to replicate these checks is bad. Frankly, these checks are there only for legacy reasons in the common code due to SLAB having them. Checking for NULL pointers is pretty useless since any dereference will cause a oops that will show where this occurred. The other checks are of the same order of uselessness. The interrupt check f.e. is nonsense since the first attempt to acquire the slab futex will trigger another exception. I would suggest to rather drop these checks entirely. SLUB never had these braindead things in them and has been in use for quite a long time. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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 TRIVIAL] mm: Fix build warning in kmem_cache_create()
On Mon, 16 Jul 2012, Christoph Lameter wrote:
> > > struct kmem_cache *kmem_cache_create(const char *name, size_t size,
> > > size_t align,
> > > unsigned long flags, void (*ctor)(void *))
> > > {
> > > struct kmem_cache *s = NULL;
> > >
> > > #ifdef CONFIG_DEBUG_VM
> > > if (!name || in_interrupt() || size < sizeof(void *) ||
> > > size > KMALLOC_MAX_SIZE) {
> > > printk(KERN_ERR "kmem_cache_create(%s) integrity check"
> > > " failed\n", name);
> > > goto out;
> > > }
> > > #endif
> > >
> >
> > Agreed, this shouldn't depend on CONFIG_DEBUG_VM.
>
> These checks are useless for regular kernel operations. They are
> only useful when developing code and should only be enabled during
> development. There is no point in testing the size and the name which are
> typically constant when a slab is created with a stable kernel.
>
Sounds like a response from someone who is very familiar with slab
allocators. The reality, though, is that very few people are going to be
doing development with CONFIG_DEBUG_VM enabled unless they notice problems
beforehand.
Are you seriously trying to optimize kmem_cache_create()? These checks
certainly aren't going to hurt your perfromance and it seems appropriate
to do some sanity checking before blowing up in unexpected ways. It's
also the way it's been done for years before extracting common allocator
functions to their own file.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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 TRIVIAL] mm: Fix build warning in kmem_cache_create()
On Mon, 2012-07-16 at 09:17 -0500, Christoph Lameter wrote:
> On Mon, 16 Jul 2012, David Rientjes wrote:
>
> > On Sun, 15 Jul 2012, Shuah Khan wrote:
> >
> > > I can work on reshuffling the code. Do have a question though. This
> > > following sanity check is currently done only when CONFIG_DEBUG_VM is
> > > defined. However, it does appear to be something that is that should be
> > > checked even in regular path.
> > >
> > > struct kmem_cache *kmem_cache_create(const char *name, size_t size,
> > > size_t align,
> > > unsigned long flags, void (*ctor)(void *))
> > > {
> > > struct kmem_cache *s = NULL;
> > >
> > > #ifdef CONFIG_DEBUG_VM
> > > if (!name || in_interrupt() || size < sizeof(void *) ||
> > > size > KMALLOC_MAX_SIZE) {
> > > printk(KERN_ERR "kmem_cache_create(%s) integrity check"
> > > " failed\n", name);
> > > goto out;
> > > }
> > > #endif
> > >
> >
> > Agreed, this shouldn't depend on CONFIG_DEBUG_VM.
>
> These checks are useless for regular kernel operations. They are
> only useful when developing code and should only be enabled during
> development. There is no point in testing the size and the name which are
> typically constant when a slab is created with a stable kernel.
>
ok. The first debug section is done prior to holding the slab mutex and
the second debug section is after holding mutex. I will have to think
about the best way to restructure the code. I will send the re-worked
patch soon, so we start refining it if need be.
-- Shuah
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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 TRIVIAL] mm: Fix build warning in kmem_cache_create()
On Mon, 16 Jul 2012, David Rientjes wrote:
> On Sun, 15 Jul 2012, Shuah Khan wrote:
>
> > I can work on reshuffling the code. Do have a question though. This
> > following sanity check is currently done only when CONFIG_DEBUG_VM is
> > defined. However, it does appear to be something that is that should be
> > checked even in regular path.
> >
> > struct kmem_cache *kmem_cache_create(const char *name, size_t size,
> > size_t align,
> > unsigned long flags, void (*ctor)(void *))
> > {
> > struct kmem_cache *s = NULL;
> >
> > #ifdef CONFIG_DEBUG_VM
> > if (!name || in_interrupt() || size < sizeof(void *) ||
> > size > KMALLOC_MAX_SIZE) {
> > printk(KERN_ERR "kmem_cache_create(%s) integrity check"
> > " failed\n", name);
> > goto out;
> > }
> > #endif
> >
>
> Agreed, this shouldn't depend on CONFIG_DEBUG_VM.
These checks are useless for regular kernel operations. They are
only useful when developing code and should only be enabled during
development. There is no point in testing the size and the name which are
typically constant when a slab is created with a stable kernel.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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 TRIVIAL] mm: Fix build warning in kmem_cache_create()
On Sun, 15 Jul 2012, Shuah Khan wrote:
> I can work on reshuffling the code. Do have a question though. This
> following sanity check is currently done only when CONFIG_DEBUG_VM is
> defined. However, it does appear to be something that is that should be
> checked even in regular path.
>
> struct kmem_cache *kmem_cache_create(const char *name, size_t size,
> size_t align,
> unsigned long flags, void (*ctor)(void *))
> {
> struct kmem_cache *s = NULL;
>
> #ifdef CONFIG_DEBUG_VM
> if (!name || in_interrupt() || size < sizeof(void *) ||
> size > KMALLOC_MAX_SIZE) {
> printk(KERN_ERR "kmem_cache_create(%s) integrity check"
> " failed\n", name);
> goto out;
> }
> #endif
>
Agreed, this shouldn't depend on CONFIG_DEBUG_VM.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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 TRIVIAL] mm: Fix build warning in kmem_cache_create()
On Sun, 15 Jul 2012, Shuah Khan wrote:
I can work on reshuffling the code. Do have a question though. This
following sanity check is currently done only when CONFIG_DEBUG_VM is
defined. However, it does appear to be something that is that should be
checked even in regular path.
struct kmem_cache *kmem_cache_create(const char *name, size_t size,
size_t align,
unsigned long flags, void (*ctor)(void *))
{
struct kmem_cache *s = NULL;
#ifdef CONFIG_DEBUG_VM
if (!name || in_interrupt() || size sizeof(void *) ||
size KMALLOC_MAX_SIZE) {
printk(KERN_ERR kmem_cache_create(%s) integrity check
failed\n, name);
goto out;
}
#endif
Agreed, this shouldn't depend on CONFIG_DEBUG_VM.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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 TRIVIAL] mm: Fix build warning in kmem_cache_create()
On Mon, 16 Jul 2012, David Rientjes wrote:
On Sun, 15 Jul 2012, Shuah Khan wrote:
I can work on reshuffling the code. Do have a question though. This
following sanity check is currently done only when CONFIG_DEBUG_VM is
defined. However, it does appear to be something that is that should be
checked even in regular path.
struct kmem_cache *kmem_cache_create(const char *name, size_t size,
size_t align,
unsigned long flags, void (*ctor)(void *))
{
struct kmem_cache *s = NULL;
#ifdef CONFIG_DEBUG_VM
if (!name || in_interrupt() || size sizeof(void *) ||
size KMALLOC_MAX_SIZE) {
printk(KERN_ERR kmem_cache_create(%s) integrity check
failed\n, name);
goto out;
}
#endif
Agreed, this shouldn't depend on CONFIG_DEBUG_VM.
These checks are useless for regular kernel operations. They are
only useful when developing code and should only be enabled during
development. There is no point in testing the size and the name which are
typically constant when a slab is created with a stable kernel.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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 TRIVIAL] mm: Fix build warning in kmem_cache_create()
On Mon, 2012-07-16 at 09:17 -0500, Christoph Lameter wrote:
On Mon, 16 Jul 2012, David Rientjes wrote:
On Sun, 15 Jul 2012, Shuah Khan wrote:
I can work on reshuffling the code. Do have a question though. This
following sanity check is currently done only when CONFIG_DEBUG_VM is
defined. However, it does appear to be something that is that should be
checked even in regular path.
struct kmem_cache *kmem_cache_create(const char *name, size_t size,
size_t align,
unsigned long flags, void (*ctor)(void *))
{
struct kmem_cache *s = NULL;
#ifdef CONFIG_DEBUG_VM
if (!name || in_interrupt() || size sizeof(void *) ||
size KMALLOC_MAX_SIZE) {
printk(KERN_ERR kmem_cache_create(%s) integrity check
failed\n, name);
goto out;
}
#endif
Agreed, this shouldn't depend on CONFIG_DEBUG_VM.
These checks are useless for regular kernel operations. They are
only useful when developing code and should only be enabled during
development. There is no point in testing the size and the name which are
typically constant when a slab is created with a stable kernel.
ok. The first debug section is done prior to holding the slab mutex and
the second debug section is after holding mutex. I will have to think
about the best way to restructure the code. I will send the re-worked
patch soon, so we start refining it if need be.
-- Shuah
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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 TRIVIAL] mm: Fix build warning in kmem_cache_create()
On Mon, 16 Jul 2012, Christoph Lameter wrote:
struct kmem_cache *kmem_cache_create(const char *name, size_t size,
size_t align,
unsigned long flags, void (*ctor)(void *))
{
struct kmem_cache *s = NULL;
#ifdef CONFIG_DEBUG_VM
if (!name || in_interrupt() || size sizeof(void *) ||
size KMALLOC_MAX_SIZE) {
printk(KERN_ERR kmem_cache_create(%s) integrity check
failed\n, name);
goto out;
}
#endif
Agreed, this shouldn't depend on CONFIG_DEBUG_VM.
These checks are useless for regular kernel operations. They are
only useful when developing code and should only be enabled during
development. There is no point in testing the size and the name which are
typically constant when a slab is created with a stable kernel.
Sounds like a response from someone who is very familiar with slab
allocators. The reality, though, is that very few people are going to be
doing development with CONFIG_DEBUG_VM enabled unless they notice problems
beforehand.
Are you seriously trying to optimize kmem_cache_create()? These checks
certainly aren't going to hurt your perfromance and it seems appropriate
to do some sanity checking before blowing up in unexpected ways. It's
also the way it's been done for years before extracting common allocator
functions to their own file.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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 TRIVIAL] mm: Fix build warning in kmem_cache_create()
On Mon, 16 Jul 2012, David Rientjes wrote: These checks are useless for regular kernel operations. They are only useful when developing code and should only be enabled during development. There is no point in testing the size and the name which are typically constant when a slab is created with a stable kernel. Sounds like a response from someone who is very familiar with slab allocators. The reality, though, is that very few people are going to be doing development with CONFIG_DEBUG_VM enabled unless they notice problems beforehand. Kernels are certainly run with CONFIG_DEBUG_VM before merges to mainstream occur. If the developer does not do it then someone else will. Are you seriously trying to optimize kmem_cache_create()? These checks certainly aren't going to hurt your perfromance and it seems appropriate to do some sanity checking before blowing up in unexpected ways. It's also the way it's been done for years before extracting common allocator functions to their own file. The kernel cannot check everything and will blow up in unexpected ways if someone codes something stupid. There are numerous debugging options that need to be switched on to get better debugging information to investigate deper. Adding special code to replicate these checks is bad. Frankly, these checks are there only for legacy reasons in the common code due to SLAB having them. Checking for NULL pointers is pretty useless since any dereference will cause a oops that will show where this occurred. The other checks are of the same order of uselessness. The interrupt check f.e. is nonsense since the first attempt to acquire the slab futex will trigger another exception. I would suggest to rather drop these checks entirely. SLUB never had these braindead things in them and has been in use for quite a long time. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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 TRIVIAL] mm: Fix build warning in kmem_cache_create()
On Mon, 16 Jul 2012, Christoph Lameter wrote: Sounds like a response from someone who is very familiar with slab allocators. The reality, though, is that very few people are going to be doing development with CONFIG_DEBUG_VM enabled unless they notice problems beforehand. Kernels are certainly run with CONFIG_DEBUG_VM before merges to mainstream occur. If the developer does not do it then someone else will. So let's say a developer wants to pass a dynamically allocated string to kmem_cache_create() for the cache name and it happens to be NULL because of a failed allocation but this never happened in testing (or it does happen but CONFIG_DEBUG_VM=n) and they are using CONFIG_SLAB. What would the failure be in linux-next? It looks like it would just result in a corrupted slabinfo. Bad result, we used to catch this problem before the extraction of common functionality and now we've allowed a corrupted slabinfo for nothing: optimizing kmem_cache_create() is pointless. The kernel cannot check everything and will blow up in unexpected ways if someone codes something stupid. There are numerous debugging options that need to be switched on to get better debugging information to investigate deper. Adding special code to replicate these checks is bad. Disagree, CONFIG_SLAB does not blow up for a NULL name string and just corrupts userspace. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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 TRIVIAL] mm: Fix build warning in kmem_cache_create()
On Sat, 2012-07-14 at 15:01 +0300, Pekka Enberg wrote:
> I'm not exactly loving that either.
>
> It'd probably be better to reshuffle the code so that the debug checks
> end up in separate functions that are no-op for !CONFIG_DEBUG_VM. That
> way the _labels_ are used unconditionally although there's no actual
> code generated.
I can work on reshuffling the code. Do have a question though. This
following sanity check is currently done only when CONFIG_DEBUG_VM is
defined. However, it does appear to be something that is that should be
checked even in regular path.
struct kmem_cache *kmem_cache_create(const char *name, size_t size,
size_t align,
unsigned long flags, void (*ctor)(void *))
{
struct kmem_cache *s = NULL;
#ifdef CONFIG_DEBUG_VM
if (!name || in_interrupt() || size < sizeof(void *) ||
size > KMALLOC_MAX_SIZE) {
printk(KERN_ERR "kmem_cache_create(%s) integrity check"
" failed\n", name);
goto out;
}
#endif
---
}
Am I reading this right?
Thanks,
-- Shuah
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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 TRIVIAL] mm: Fix build warning in kmem_cache_create()
On Sat, 2012-07-14 at 15:01 +0300, Pekka Enberg wrote:
I'm not exactly loving that either.
It'd probably be better to reshuffle the code so that the debug checks
end up in separate functions that are no-op for !CONFIG_DEBUG_VM. That
way the _labels_ are used unconditionally although there's no actual
code generated.
I can work on reshuffling the code. Do have a question though. This
following sanity check is currently done only when CONFIG_DEBUG_VM is
defined. However, it does appear to be something that is that should be
checked even in regular path.
struct kmem_cache *kmem_cache_create(const char *name, size_t size,
size_t align,
unsigned long flags, void (*ctor)(void *))
{
struct kmem_cache *s = NULL;
#ifdef CONFIG_DEBUG_VM
if (!name || in_interrupt() || size sizeof(void *) ||
size KMALLOC_MAX_SIZE) {
printk(KERN_ERR kmem_cache_create(%s) integrity check
failed\n, name);
goto out;
}
#endif
---
}
Am I reading this right?
Thanks,
-- Shuah
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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 TRIVIAL] mm: Fix build warning in kmem_cache_create()
On Sat, Jul 14, 2012 at 12:18 PM, David Rientjes wrote: > On Fri, 13 Jul 2012, Shuah Khan wrote: > >> diff --git a/mm/slab_common.c b/mm/slab_common.c >> index 12637ce..aa3ca5b 100644 >> --- a/mm/slab_common.c >> +++ b/mm/slab_common.c >> @@ -98,7 +98,9 @@ struct kmem_cache *kmem_cache_create(const char *name, >> size_t size, size_t align >> >> s = __kmem_cache_create(name, size, align, flags, ctor); >> >> +#ifdef CONFIG_DEBUG_VM >> oops: >> +#endif >> mutex_unlock(_mutex); >> put_online_cpus(); >> > > Tip: gcc allows label attributes so you could actually do > > oops: __maybe_unused > > to silence the warning and do the same for the "out" label later in the > function. I'm not exactly loving that either. It'd probably be better to reshuffle the code so that the debug checks end up in separate functions that are no-op for !CONFIG_DEBUG_VM. That way the _labels_ are used unconditionally although there's no actual code generated. Pekka -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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 TRIVIAL] mm: Fix build warning in kmem_cache_create()
On Fri, 13 Jul 2012, Shuah Khan wrote: > diff --git a/mm/slab_common.c b/mm/slab_common.c > index 12637ce..aa3ca5b 100644 > --- a/mm/slab_common.c > +++ b/mm/slab_common.c > @@ -98,7 +98,9 @@ struct kmem_cache *kmem_cache_create(const char *name, > size_t size, size_t align > > s = __kmem_cache_create(name, size, align, flags, ctor); > > +#ifdef CONFIG_DEBUG_VM > oops: > +#endif > mutex_unlock(_mutex); > put_online_cpus(); > Tip: gcc allows label attributes so you could actually do oops: __maybe_unused to silence the warning and do the same for the "out" label later in the function. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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 TRIVIAL] mm: Fix build warning in kmem_cache_create()
On Fri, 13 Jul 2012, Shuah Khan wrote: diff --git a/mm/slab_common.c b/mm/slab_common.c index 12637ce..aa3ca5b 100644 --- a/mm/slab_common.c +++ b/mm/slab_common.c @@ -98,7 +98,9 @@ struct kmem_cache *kmem_cache_create(const char *name, size_t size, size_t align s = __kmem_cache_create(name, size, align, flags, ctor); +#ifdef CONFIG_DEBUG_VM oops: +#endif mutex_unlock(slab_mutex); put_online_cpus(); Tip: gcc allows label attributes so you could actually do oops: __maybe_unused to silence the warning and do the same for the out label later in the function. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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 TRIVIAL] mm: Fix build warning in kmem_cache_create()
On Sat, Jul 14, 2012 at 12:18 PM, David Rientjes rient...@google.com wrote: On Fri, 13 Jul 2012, Shuah Khan wrote: diff --git a/mm/slab_common.c b/mm/slab_common.c index 12637ce..aa3ca5b 100644 --- a/mm/slab_common.c +++ b/mm/slab_common.c @@ -98,7 +98,9 @@ struct kmem_cache *kmem_cache_create(const char *name, size_t size, size_t align s = __kmem_cache_create(name, size, align, flags, ctor); +#ifdef CONFIG_DEBUG_VM oops: +#endif mutex_unlock(slab_mutex); put_online_cpus(); Tip: gcc allows label attributes so you could actually do oops: __maybe_unused to silence the warning and do the same for the out label later in the function. I'm not exactly loving that either. It'd probably be better to reshuffle the code so that the debug checks end up in separate functions that are no-op for !CONFIG_DEBUG_VM. That way the _labels_ are used unconditionally although there's no actual code generated. Pekka -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
