[patch -mm] mm, mempolicy: silence gcc warning

[patch -mm] mm, mempolicy: silence gcc warning

[David Rientjes]

Fengguang Wu reports that compiling mm/mempolicy.c results in a warning:

	mm/mempolicy.c: In function 'mpol_to_str':
	mm/mempolicy.c:2878:2: error: format not a string literal and no format arguments

Kees says this is because he is using -Wformat-security.

Silence the warning.

Reported-by: Fengguang Wu <fengguang.wu@intel.com>
Suggested-by: Kees Cook <keescook@chromium.org>
Signed-off-by: David Rientjes <rientjes@google.com>
---
 mm/mempolicy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/mempolicy.c b/mm/mempolicy.c
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -2950,7 +2950,7 @@ void mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
 		return;
 	}
 
-	p += snprintf(p, maxlen, policy_modes[mode]);
+	p += snprintf(p, maxlen, "%s", policy_modes[mode]);
 
 	if (flags & MPOL_MODE_FLAGS) {
 		p += snprintf(p, buffer + maxlen - p, "=");

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch -mm] mm, mempolicy: silence gcc warning

[Kees Cook]

On Tue, Nov 12, 2013 at 6:12 PM, David Rientjes <rientjes@google.com> wrote:
> Fengguang Wu reports that compiling mm/mempolicy.c results in a warning:
>
>         mm/mempolicy.c: In function 'mpol_to_str':
>         mm/mempolicy.c:2878:2: error: format not a string literal and no format arguments
>
> Kees says this is because he is using -Wformat-security.
>
> Silence the warning.
>
> Reported-by: Fengguang Wu <fengguang.wu@intel.com>
> Suggested-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: David Rientjes <rientjes@google.com>

Thanks for helping silence my -Wformat-security warning checks. :)

Acked-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
>  mm/mempolicy.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -2950,7 +2950,7 @@ void mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
>                 return;
>         }
>
> -       p += snprintf(p, maxlen, policy_modes[mode]);
> +       p += snprintf(p, maxlen, "%s", policy_modes[mode]);
>
>         if (flags & MPOL_MODE_FLAGS) {
>                 p += snprintf(p, buffer + maxlen - p, "=");



-- 
Kees Cook
Chrome OS Security

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch -mm] mm, mempolicy: silence gcc warning

[Andrew Morton]

On Tue, 12 Nov 2013 18:12:32 -0800 (PST) David Rientjes <rientjes@google.com> wrote:

> Fengguang Wu reports that compiling mm/mempolicy.c results in a warning:
> 
> 	mm/mempolicy.c: In function 'mpol_to_str':
> 	mm/mempolicy.c:2878:2: error: format not a string literal and no format arguments
> 
> Kees says this is because he is using -Wformat-security.
> 
> Silence the warning.
> 
> ...
>
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -2950,7 +2950,7 @@ void mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
>  		return;
>  	}
>  
> -	p += snprintf(p, maxlen, policy_modes[mode]);
> +	p += snprintf(p, maxlen, "%s", policy_modes[mode]);
>  
>  	if (flags & MPOL_MODE_FLAGS) {
>  		p += snprintf(p, buffer + maxlen - p, "=");

mutter.  There are no '%'s in policy_modes[].  Maybe we should only do
this #ifdef CONFIG_KEES.

mpol_to_str() would be simpler (and slower) if it was switched to use
strncat().

It worries me that the CONFIG_NUMA=n version of mpol_to_str() doesn't
stick a '\0' into *buffer.  Hopefully it never gets called...

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch -mm] mm, mempolicy: silence gcc warning

[Kees Cook]

On Wed, Nov 20, 2013 at 2:15 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Tue, 12 Nov 2013 18:12:32 -0800 (PST) David Rientjes <rientjes@google.com> wrote:
>
>> Fengguang Wu reports that compiling mm/mempolicy.c results in a warning:
>>
>>       mm/mempolicy.c: In function 'mpol_to_str':
>>       mm/mempolicy.c:2878:2: error: format not a string literal and no format arguments
>>
>> Kees says this is because he is using -Wformat-security.
>>
>> Silence the warning.
>>
>> ...
>>
>> --- a/mm/mempolicy.c
>> +++ b/mm/mempolicy.c
>> @@ -2950,7 +2950,7 @@ void mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
>>               return;
>>       }
>>
>> -     p += snprintf(p, maxlen, policy_modes[mode]);
>> +     p += snprintf(p, maxlen, "%s", policy_modes[mode]);
>>
>>       if (flags & MPOL_MODE_FLAGS) {
>>               p += snprintf(p, buffer + maxlen - p, "=");
>
> mutter.  There are no '%'s in policy_modes[].  Maybe we should only do
> this #ifdef CONFIG_KEES.

Yeah, I had offered to just whitelist this in my checker, since it's
the type of const char array that gcc doesn't realize is harmless as
an arg-less format string. Fengguang's reports are way faster than me,
though. :)

> mpol_to_str() would be simpler (and slower) if it was switched to use
> strncat().
>
> It worries me that the CONFIG_NUMA=n version of mpol_to_str() doesn't
> stick a '\0' into *buffer.  Hopefully it never gets called...

-Kees

-- 
Kees Cook
Chrome OS Security

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch -mm] mm, mempolicy: silence gcc warning

[KOSAKI Motohiro]

>> --- a/mm/mempolicy.c
>> +++ b/mm/mempolicy.c
>> @@ -2950,7 +2950,7 @@ void mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
>>               return;
>>       }
>>
>> -     p += snprintf(p, maxlen, policy_modes[mode]);
>> +     p += snprintf(p, maxlen, "%s", policy_modes[mode]);
>>
>>       if (flags & MPOL_MODE_FLAGS) {
>>               p += snprintf(p, buffer + maxlen - p, "=");
>
> mutter.  There are no '%'s in policy_modes[].  Maybe we should only do
> this #ifdef CONFIG_KEES.
>
> mpol_to_str() would be simpler (and slower) if it was switched to use
> strncat().

IMHO, you should queue this patch. mpol_to_str() is not fast path at all and
I don't want worry about false positive warning.

> It worries me that the CONFIG_NUMA=n version of mpol_to_str() doesn't
> stick a '\0' into *buffer.  Hopefully it never gets called...

Don't worry. It never happens. Currently, all of caller depend on CONFIG_NUMA.
However it would be nice if CONFIG_NUMA=n version of mpol_to_str() is
implemented
more carefully. I don't know who's mistake.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch -mm] mm, mempolicy: silence gcc warning

[Andrew Morton]

On Sat, 23 Nov 2013 15:49:08 -0500 KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote:

> >> --- a/mm/mempolicy.c
> >> +++ b/mm/mempolicy.c
> >> @@ -2950,7 +2950,7 @@ void mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
> >>               return;
> >>       }
> >>
> >> -     p += snprintf(p, maxlen, policy_modes[mode]);
> >> +     p += snprintf(p, maxlen, "%s", policy_modes[mode]);
> >>
> >>       if (flags & MPOL_MODE_FLAGS) {
> >>               p += snprintf(p, buffer + maxlen - p, "=");
> >
> > mutter.  There are no '%'s in policy_modes[].  Maybe we should only do
> > this #ifdef CONFIG_KEES.
> >
> > mpol_to_str() would be simpler (and slower) if it was switched to use
> > strncat().
> 
> IMHO, you should queue this patch. mpol_to_str() is not fast path at all and
> I don't want worry about false positive warning.

Yup, it's in mainline.

> > It worries me that the CONFIG_NUMA=n version of mpol_to_str() doesn't
> > stick a '\0' into *buffer.  Hopefully it never gets called...
> 
> Don't worry. It never happens. Currently, all of caller depend on CONFIG_NUMA.
> However it would be nice if CONFIG_NUMA=n version of mpol_to_str() is
> implemented
> more carefully. I don't know who's mistake.

Put a BUG() in there?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch -mm] mm, mempolicy: silence gcc warning

[David Rientjes]

On Mon, 25 Nov 2013, Andrew Morton wrote:

> > > It worries me that the CONFIG_NUMA=n version of mpol_to_str() doesn't
> > > stick a '\0' into *buffer.  Hopefully it never gets called...
> > 
> > Don't worry. It never happens. Currently, all of caller depend on CONFIG_NUMA.
> > However it would be nice if CONFIG_NUMA=n version of mpol_to_str() is
> > implemented
> > more carefully. I don't know who's mistake.
> 
> Put a BUG() in there?
> 

Why make it a fatal runtime error when it can simply be a compile time 
error since calling mpol_to_str() without CONFIG_NUMA is unnecessary?  
There wouldn't be a mempolicy to convert, the struct has no fields in such 
a configuration.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[patch] mm, mempolicy: remove unneeded functions for UMA configs

[David Rientjes]

Mempolicies only exist for CONFIG_NUMA configurations.  Therefore, a 
certain class of functions are unneeded in configurations where 
CONFIG_NUMA is disabled such as functions that duplicate existing 
mempolicies, lookup existing policies, set certain mempolicy traits, or 
test mempolicies for certain attributes.

Remove the unneeded functions so that any future callers get a compile-
time error and protect their code with CONFIG_NUMA as required.

Signed-off-by: David Rientjes <rientjes@google.com>
---
 include/linux/mempolicy.h | 32 --------------------------------
 1 file changed, 32 deletions(-)

diff --git a/include/linux/mempolicy.h b/include/linux/mempolicy.h
--- a/include/linux/mempolicy.h
+++ b/include/linux/mempolicy.h
@@ -211,20 +211,8 @@ static inline void mpol_get(struct mempolicy *pol)
 {
 }
 
-static inline struct mempolicy *mpol_dup(struct mempolicy *old)
-{
-	return NULL;
-}
-
 struct shared_policy {};
 
-static inline int mpol_set_shared_policy(struct shared_policy *info,
-					struct vm_area_struct *vma,
-					struct mempolicy *new)
-{
-	return -EINVAL;
-}
-
 static inline void mpol_shared_policy_init(struct shared_policy *sp,
 						struct mempolicy *mpol)
 {
@@ -234,12 +222,6 @@ static inline void mpol_free_shared_policy(struct shared_policy *p)
 {
 }
 
-static inline struct mempolicy *
-mpol_shared_policy_lookup(struct shared_policy *sp, unsigned long idx)
-{
-	return NULL;
-}
-
 #define vma_policy(vma) NULL
 
 static inline int
@@ -266,10 +248,6 @@ static inline void mpol_rebind_mm(struct mm_struct *mm, nodemask_t *new)
 {
 }
 
-static inline void mpol_fix_fork_child_flag(struct task_struct *p)
-{
-}
-
 static inline struct zonelist *huge_zonelist(struct vm_area_struct *vma,
 				unsigned long addr, gfp_t gfp_flags,
 				struct mempolicy **mpol, nodemask_t **nodemask)
@@ -284,12 +262,6 @@ static inline bool init_nodemask_of_mempolicy(nodemask_t *m)
 	return false;
 }
 
-static inline bool mempolicy_nodemask_intersects(struct task_struct *tsk,
-			const nodemask_t *mask)
-{
-	return false;
-}
-
 static inline int do_migrate_pages(struct mm_struct *mm, const nodemask_t *from,
 				   const nodemask_t *to, int flags)
 {
@@ -307,10 +279,6 @@ static inline int mpol_parse_str(char *str, struct mempolicy **mpol)
 }
 #endif
 
-static inline void mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
-{
-}
-
 static inline int mpol_misplaced(struct page *page, struct vm_area_struct *vma,
 				 unsigned long address)
 {

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch -mm] mm, mempolicy: silence gcc warning

[KOSAKI Motohiro]

(11/25/2013 3:31 PM), Andrew Morton wrote:
> On Sat, 23 Nov 2013 15:49:08 -0500 KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote:
> 
>>>> --- a/mm/mempolicy.c
>>>> +++ b/mm/mempolicy.c
>>>> @@ -2950,7 +2950,7 @@ void mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
>>>>               return;
>>>>       }
>>>>
>>>> -     p += snprintf(p, maxlen, policy_modes[mode]);
>>>> +     p += snprintf(p, maxlen, "%s", policy_modes[mode]);
>>>>
>>>>       if (flags & MPOL_MODE_FLAGS) {
>>>>               p += snprintf(p, buffer + maxlen - p, "=");
>>>
>>> mutter.  There are no '%'s in policy_modes[].  Maybe we should only do
>>> this #ifdef CONFIG_KEES.
>>>
>>> mpol_to_str() would be simpler (and slower) if it was switched to use
>>> strncat().
>>
>> IMHO, you should queue this patch. mpol_to_str() is not fast path at all and
>> I don't want worry about false positive warning.
> 
> Yup, it's in mainline.

Thanks.

> 
>>> It worries me that the CONFIG_NUMA=n version of mpol_to_str() doesn't
>>> stick a '\0' into *buffer.  Hopefully it never gets called...
>>
>> Don't worry. It never happens. Currently, all of caller depend on CONFIG_NUMA.
>> However it would be nice if CONFIG_NUMA=n version of mpol_to_str() is
>> implemented
>> more carefully. I don't know who's mistake.
> 
> Put a BUG() in there?

I think this is enough. What do you think?


commit 5691f7f336c511d39fc05821d204a8f7ba18c0cf
Author: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Date:   Mon Nov 25 18:38:25 2013 -0500

    mempolicy: implement mpol_to_str() fallback implementation when !CONFIG_NUMA

    Andrew Morton pointed out mpol_to_str() has no fallback implementation
    for !CONFIG_NUMA and it could be dangerous because callers might assume
    buffer is filled zero terminated string. Fortunately there is no such
    caller. But it would be nice to provide default safe implementation.

    Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>

diff --git a/include/linux/mempolicy.h b/include/linux/mempolicy.h
index 9fe426b..eee0597 100644
--- a/include/linux/mempolicy.h
+++ b/include/linux/mempolicy.h
@@ -309,6 +309,8 @@ static inline int mpol_parse_str(char *str, struct mempolicy **mpol)

 static inline void mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
 {
+	strncpy(buffer, "default", maxlen-1);
+	buffer[maxlen-1] = '\0';
 }

 static inline int mpol_misplaced(struct page *page, struct vm_area_struct *vma,




--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch -mm] mm, mempolicy: silence gcc warning

[Andrew Morton]

On Mon, 25 Nov 2013 19:08:15 -0500 KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote:

> >>> It worries me that the CONFIG_NUMA=n version of mpol_to_str() doesn't
> >>> stick a '\0' into *buffer.  Hopefully it never gets called...
> >>
> >> Don't worry. It never happens. Currently, all of caller depend on CONFIG_NUMA.
> >> However it would be nice if CONFIG_NUMA=n version of mpol_to_str() is
> >> implemented
> >> more carefully. I don't know who's mistake.
> > 
> > Put a BUG() in there?
> 
> I think this is enough. What do you think?
> 
> 
> commit 5691f7f336c511d39fc05821d204a8f7ba18c0cf
> Author: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> Date:   Mon Nov 25 18:38:25 2013 -0500
> 
>     mempolicy: implement mpol_to_str() fallback implementation when !CONFIG_NUMA
> 
>     Andrew Morton pointed out mpol_to_str() has no fallback implementation
>     for !CONFIG_NUMA and it could be dangerous because callers might assume
>     buffer is filled zero terminated string. Fortunately there is no such
>     caller. But it would be nice to provide default safe implementation.
> 
>     Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> 
> diff --git a/include/linux/mempolicy.h b/include/linux/mempolicy.h
> index 9fe426b..eee0597 100644
> --- a/include/linux/mempolicy.h
> +++ b/include/linux/mempolicy.h
> @@ -309,6 +309,8 @@ static inline int mpol_parse_str(char *str, struct mempolicy **mpol)
> 
>  static inline void mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
>  {
> +	strncpy(buffer, "default", maxlen-1);
> +	buffer[maxlen-1] = '\0';
>  }
> 

Well, as David said, BUILD_BUG() would be the preferred cleanup.  I'll
stick one in there and see what the build bot has to say?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch -mm] mm, mempolicy: silence gcc warning

[KOSAKI Motohiro]

>> diff --git a/include/linux/mempolicy.h b/include/linux/mempolicy.h
>> index 9fe426b..eee0597 100644
>> --- a/include/linux/mempolicy.h
>> +++ b/include/linux/mempolicy.h
>> @@ -309,6 +309,8 @@ static inline int mpol_parse_str(char *str, struct mempolicy **mpol)
>>
>>  static inline void mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
>>  {
>> +     strncpy(buffer, "default", maxlen-1);
>> +     buffer[maxlen-1] = '\0';
>>  }
>>
>
> Well, as David said, BUILD_BUG() would be the preferred cleanup.  I'll
> stick one in there and see what the build bot has to say?

Sigh, I can't understand why you always prefer to increase maintenance annoying.
However up to you.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>