Re: [PATCH v2] mm/shmem.c: check the return value of mpol_to_str()

Re: [PATCH v2] mm/shmem.c: check the return value of mpol_to_str()

[David Rientjes]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2016 bytes --]

On Thu, 19 Sep 2013, Chen,Gang( e??a??) wrote:

> PleaseA searchA BUG_ON()A inA kernelA wideA sourceA code,A weA canA knowA whether
> itA isA commonlyA usedA orA not.
> 
> PleaseA searchA BUGA inA arch/A sub-system,A weA canA knowA whichA architectures
> customizeA BUG/BUG_ON.
> 
> AfterA doA theA 2A things,A InA myA opinion,A weA canA treatA BUG/BUG_ON()A isA common
> implementation,A andA mostA ofA architecturesA usesA theA defaultA one.
> 
> PleaseA checkA again,A thanks.
> 

BUG_ON() is used for fatal conditions where continuing could potentially 
be harmful.  Obviously it is commonly used in a kernel.  That doesn't mean 
we BUG_ON() when a string hasn't been defined for a mempolicy mode.  
mpol_to_str() is not critical.

It is not a fatal condition, and nothing you say is going to convince 
anybody on this thread that it's a fatal condition.

> >A That'sA absolutelyA insane.A A IfA codeA isA notA allocatingA enoughA memoryA forA theA 
> >A maximumA possibleA lengthA ofA aA stringA toA beA storedA byA mpol_to_str(),A it'sA aA 
> >A bugA inA theA code.A A WeA doA notA panicA andA rebootA theA user'sA machineA forA suchA aA 
> >A bug.A A Instead,A weA breakA theA buildA andA requireA theA brokenA codeA toA beA fixed.
> >A 
> 
> PleaseA sayA inA polite.
> 

You want a polite response when you're insisting that we declare absolute 
failure, BUG_ON(), stop, and reboot the kernel because a mempolicy mode 
isn't defined as a string in mpol_to_str()?  That sounds like an impolite 
response to the user, so see my politeness to you as coming from the users 
of the systems you just crashed.

This is a compile-time problem, not run-time.

> CanA youA beA sure,A theA "maxlenA ==A 50"A inA "fs/proc/task_mmu()",A mustA beA aA bug??
> 

I asked you to figure out the longest string possible to be stored by 
mpol_to_str().  There's nothing mysterious about that function.  It's 
deterministic.  If you really can't figure out the value this should be, 
then you shouldn't be touching mpol_to_str().

Re: [PATCH v2] mm/shmem.c: check the return value of mpol_to_str()

[Chen Gang]

On 09/24/2013 05:46 AM, David Rientjes wrote:
> On Thu, 19 Sep 2013, Chen,Gang( e??a??) wrote:
> 
>> Please search BUG_ON() in kernel wide source code, we can know whether
>> it is commonly used or not.
>>
>> Please search BUG in arch/ sub-system, we can know which architectures
>> customize BUG/BUG_ON.
>>
>> After do the 2 things, In my opinion, we can treat BUG/BUG_ON() is common
>> implementation, and most of architectures uses the default one.
>>
>> Please check again, thanks.
>>
> 
> BUG_ON() is used for fatal conditions where continuing could potentially 
> be harmful.  Obviously it is commonly used in a kernel.  That doesn't mean 
> we BUG_ON() when a string hasn't been defined for a mempolicy mode.  
> mpol_to_str() is not critical.
> 
> It is not a fatal condition, and nothing you say is going to convince 
> anybody on this thread that it's a fatal condition.
> 

If mpol_to_str() fail, the buffer passed to next seq_printf() may cause
memory over flow.

So in current implementation, if mpol_to_str() fails, it may cause
critical issue ("it's a fatal condition").

My original fix is "check and return if fail", but related members think
it shouldn't fail, so use BUG_ON(): "when fails, means OS is continuing
blindly, and next, may cause direct critical issue".

>>>  That's absolutely insane.  If code is not allocating enough memory for the 
>>>  maximum possible length of a string to be stored by mpol_to_str(), it's a 
>>>  bug in the code.  We do not panic and reboot the user's machine for such a 
>>>  bug.  Instead, we break the build and require the broken code to be fixed.
>>>  
>>
>> Please say in polite.
>>
> 
> You want a polite response when you're insisting that we declare absolute 
> failure, BUG_ON(), stop, and reboot the kernel because a mempolicy mode 
> isn't defined as a string in mpol_to_str()?  That sounds like an impolite 
> response to the user, so see my politeness to you as coming from the users 
> of the systems you just crashed.
> 

Hmm... Except God, we (everyone, include real users) only can discuss
judge, and check things and actions based on the proves, but has no
right to discuss, judge and check persons.

When it is just discussing (not get a conclusion), it is impolite to
make a conclusion forcefully by oneself with his/her own feelings.

And when we really get a conclusion, we need use the words which only
can express the result clearly (e.g. correct, incorrect) to make a
conclusion, not use words also contents his/her own feelings.


> This is a compile-time problem, not run-time.
> 

Hmm... I am not quite familiar with the details, but at least for me,
what you said is acceptable (at least, we can try).

Current mpol_to_str() interface leads all readers have to treat it as
run-time problem, not as compile-time problem.

So in my opinion, if really try the compile-time fix, the interface need
be changed: "need use struct (have a member buf[64];) pointer instead of
'buffer' and 'maxlen' parameters".


>> Can you be sure, the "maxlen == 50" in "fs/proc/task_mmu()", must be a bug??
>>
> 
> I asked you to figure out the longest string possible to be stored by 
> mpol_to_str().  There's nothing mysterious about that function.  It's 
> deterministic.  If you really can't figure out the value this should be, 
> then you shouldn't be touching mpol_to_str().
> 

At present, it seems easy to get longest string, so you can try, don't
need me; in future, I don't know whether it will be changed, maybe you
know (need give a length long enough for future using).


I don't plan to touch mpol_to_str(), in my opinion, just treat it as
run-time problem is still acceptable: it is clear enough for readers and
writers.

Hmm... at last, I still welcome you to try to send your compile-time fix
patch for it (although I am not quite sure whether it will be acceptable
by other members).


Thanks.
-- 
Chen Gang

--
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 v2] mm/shmem.c: check the return value of mpol_to_str()

[Chen,Gang( 陈刚)]

[-- Attachment #1: Type: text/html, Size: 5029 bytes --]

[PATCH] mm/shmem.c: check the return value of mpol_to_str()

[Chen Gang]

mpol_to_str() may fail, and not fill the buffer (e.g. -EINVAL), so need
check about it, or buffer may not be zero based, and next seq_printf()
will cause issue.

Also need let shmem_show_mpol() return value, since it may fail.

Signed-off-by: Chen Gang <gang.chen@asianux.com>
Reviewed-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 mm/shmem.c |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/mm/shmem.c b/mm/shmem.c
index f00c1c1..b4d44db 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -883,16 +883,20 @@ redirty:
 
 #ifdef CONFIG_NUMA
 #ifdef CONFIG_TMPFS
-static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
+static int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
 {
 	char buffer[64];
+	int ret;
 
 	if (!mpol || mpol->mode == MPOL_DEFAULT)
-		return;		/* show nothing */
+		return 0;		/* show nothing */
 
-	mpol_to_str(buffer, sizeof(buffer), mpol);
+	ret = mpol_to_str(buffer, sizeof(buffer), mpol);
+	if (ret < 0)
+		return ret;
 
 	seq_printf(seq, ",mpol=%s", buffer);
+	return 0;
 }
 
 static struct mempolicy *shmem_get_sbmpol(struct shmem_sb_info *sbinfo)
@@ -951,8 +955,9 @@ static struct page *shmem_alloc_page(gfp_t gfp,
 }
 #else /* !CONFIG_NUMA */
 #ifdef CONFIG_TMPFS
-static inline void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
+static inline int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
 {
+	return 0;
 }
 #endif /* CONFIG_TMPFS */
 
@@ -2555,8 +2560,7 @@ static int shmem_show_options(struct seq_file *seq, struct dentry *root)
 	if (!gid_eq(sbinfo->gid, GLOBAL_ROOT_GID))
 		seq_printf(seq, ",gid=%u",
 				from_kgid_munged(&init_user_ns, sbinfo->gid));
-	shmem_show_mpol(seq, sbinfo->mpol);
-	return 0;
+	return shmem_show_mpol(seq, sbinfo->mpol);
 }
 #endif /* CONFIG_TMPFS */
 
-- 
1.7.7.6

--
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 v2] mm/shmem.c: check the return value of mpol_to_str()

[Chen Gang]

mpol_to_str() may fail, and not fill the buffer (e.g. -EINVAL), so need
check about it, or buffer may not be zero based, and next seq_printf()
will cause issue.

Also need let shmem_show_mpol() return value, since it may fail.

Signed-off-by: Chen Gang <gang.chen@asianux.com>
Reviewed-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 mm/shmem.c |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/mm/shmem.c b/mm/shmem.c
index f00c1c1..b4d44db 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -883,16 +883,20 @@ redirty:
 
 #ifdef CONFIG_NUMA
 #ifdef CONFIG_TMPFS
-static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
+static int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
 {
 	char buffer[64];
+	int ret;
 
 	if (!mpol || mpol->mode == MPOL_DEFAULT)
-		return;		/* show nothing */
+		return 0;		/* show nothing */
 
-	mpol_to_str(buffer, sizeof(buffer), mpol);
+	ret = mpol_to_str(buffer, sizeof(buffer), mpol);
+	if (ret < 0)
+		return ret;
 
 	seq_printf(seq, ",mpol=%s", buffer);
+	return 0;
 }
 
 static struct mempolicy *shmem_get_sbmpol(struct shmem_sb_info *sbinfo)
@@ -951,8 +955,9 @@ static struct page *shmem_alloc_page(gfp_t gfp,
 }
 #else /* !CONFIG_NUMA */
 #ifdef CONFIG_TMPFS
-static inline void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
+static inline int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
 {
+	return 0;
 }
 #endif /* CONFIG_TMPFS */
 
@@ -2555,8 +2560,7 @@ static int shmem_show_options(struct seq_file *seq, struct dentry *root)
 	if (!gid_eq(sbinfo->gid, GLOBAL_ROOT_GID))
 		seq_printf(seq, ",gid=%u",
 				from_kgid_munged(&init_user_ns, sbinfo->gid));
-	shmem_show_mpol(seq, sbinfo->mpol);
-	return 0;
+	return shmem_show_mpol(seq, sbinfo->mpol);
 }
 #endif /* CONFIG_TMPFS */
 
-- 
1.7.7.6


--
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 v2] mm/shmem.c: check the return value of mpol_to_str()

[David Rientjes]

On Thu, 5 Sep 2013, Chen Gang wrote:

> diff --git a/mm/shmem.c b/mm/shmem.c
> index f00c1c1..b4d44db 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -883,16 +883,20 @@ redirty:
>  
>  #ifdef CONFIG_NUMA
>  #ifdef CONFIG_TMPFS
> -static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
> +static int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
>  {
>  	char buffer[64];
> +	int ret;
>  
>  	if (!mpol || mpol->mode == MPOL_DEFAULT)
> -		return;		/* show nothing */
> +		return 0;		/* show nothing */
>  
> -	mpol_to_str(buffer, sizeof(buffer), mpol);
> +	ret = mpol_to_str(buffer, sizeof(buffer), mpol);

I was wondering how mpol_to_str() could fail given a pointer to a stack 
allocated buffer, so I checked and it happens if the mempolicy mode isn't 
known or the buffer isn't long enough.

I think it would be better to keep mpol_to_str() returning void, and hence 
avoiding the need for this patch, and make it so it cannot fail.  If the 
mode is invalid, just store a 0 to the buffer (or "unknown"); and if 
maxlen isn't large enough, make it a compile-time error (let's avoid 
trying to be fancy and allocating less than 64 bytes on the stack if a 
given context is known to have short mempolicy strings).

> +	if (ret < 0)
> +		return ret;
>  
>  	seq_printf(seq, ",mpol=%s", buffer);
> +	return 0;
>  }
>  
>  static struct mempolicy *shmem_get_sbmpol(struct shmem_sb_info *sbinfo)
> @@ -951,8 +955,9 @@ static struct page *shmem_alloc_page(gfp_t gfp,
>  }
>  #else /* !CONFIG_NUMA */
>  #ifdef CONFIG_TMPFS
> -static inline void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
> +static inline int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
>  {
> +	return 0;
>  }
>  #endif /* CONFIG_TMPFS */
>  
> @@ -2555,8 +2560,7 @@ static int shmem_show_options(struct seq_file *seq, struct dentry *root)
>  	if (!gid_eq(sbinfo->gid, GLOBAL_ROOT_GID))
>  		seq_printf(seq, ",gid=%u",
>  				from_kgid_munged(&init_user_ns, sbinfo->gid));
> -	shmem_show_mpol(seq, sbinfo->mpol);
> -	return 0;
> +	return shmem_show_mpol(seq, sbinfo->mpol);
>  }
>  #endif /* CONFIG_TMPFS */
>  

--
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 v2] mm/shmem.c: check the return value of mpol_to_str()

[Chen Gang]

On 09/10/2013 04:30 AM, David Rientjes wrote:
> On Thu, 5 Sep 2013, Chen Gang wrote:
> 
>> diff --git a/mm/shmem.c b/mm/shmem.c
>> index f00c1c1..b4d44db 100644
>> --- a/mm/shmem.c
>> +++ b/mm/shmem.c
>> @@ -883,16 +883,20 @@ redirty:
>>  
>>  #ifdef CONFIG_NUMA
>>  #ifdef CONFIG_TMPFS
>> -static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
>> +static int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
>>  {
>>  	char buffer[64];
>> +	int ret;
>>  
>>  	if (!mpol || mpol->mode == MPOL_DEFAULT)
>> -		return;		/* show nothing */
>> +		return 0;		/* show nothing */
>>  
>> -	mpol_to_str(buffer, sizeof(buffer), mpol);
>> +	ret = mpol_to_str(buffer, sizeof(buffer), mpol);
> 
> I was wondering how mpol_to_str() could fail given a pointer to a stack 
> allocated buffer, so I checked and it happens if the mempolicy mode isn't 
> known or the buffer isn't long enough.
> 

Yeah.

> I think it would be better to keep mpol_to_str() returning void, and hence 
> avoiding the need for this patch, and make it so it cannot fail.  If the 
> mode is invalid, just store a 0 to the buffer (or "unknown"); and if 
> maxlen isn't large enough, make it a compile-time error (let's avoid 
> trying to be fancy and allocating less than 64 bytes on the stack if a 
> given context is known to have short mempolicy strings).
> 

Hmm... at least, like most of print functions, it need return a value
to tell the length it writes, so in my opinion, I still suggest it can
return a value.

For common printing functions, caller knows about the string format and
all parameters, and also can control them,  so for callee, it is not
'quite polite' to return any failures to caller.  :-)

But for our function, caller may not know about the string format and
parameters' details, so callee has duty to check and process them:

  e.g. "if related parameter is invalid, it is neccessary to notifiy to caller".


Thanks.

>> +	if (ret < 0)
>> +		return ret;
>>  
>>  	seq_printf(seq, ",mpol=%s", buffer);
>> +	return 0;
>>  }
>>  
>>  static struct mempolicy *shmem_get_sbmpol(struct shmem_sb_info *sbinfo)
>> @@ -951,8 +955,9 @@ static struct page *shmem_alloc_page(gfp_t gfp,
>>  }
>>  #else /* !CONFIG_NUMA */
>>  #ifdef CONFIG_TMPFS
>> -static inline void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
>> +static inline int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
>>  {
>> +	return 0;
>>  }
>>  #endif /* CONFIG_TMPFS */
>>  
>> @@ -2555,8 +2560,7 @@ static int shmem_show_options(struct seq_file *seq, struct dentry *root)
>>  	if (!gid_eq(sbinfo->gid, GLOBAL_ROOT_GID))
>>  		seq_printf(seq, ",gid=%u",
>>  				from_kgid_munged(&init_user_ns, sbinfo->gid));
>> -	shmem_show_mpol(seq, sbinfo->mpol);
>> -	return 0;
>> +	return shmem_show_mpol(seq, sbinfo->mpol);
>>  }
>>  #endif /* CONFIG_TMPFS */
>>  
> 
> 


-- 
Chen Gang

--
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 v2] mm/shmem.c: check the return value of mpol_to_str()

[David Rientjes]

On Tue, 10 Sep 2013, Chen Gang wrote:

> > I think it would be better to keep mpol_to_str() returning void, and hence 
> > avoiding the need for this patch, and make it so it cannot fail.  If the 
> > mode is invalid, just store a 0 to the buffer (or "unknown"); and if 
> > maxlen isn't large enough, make it a compile-time error (let's avoid 
> > trying to be fancy and allocating less than 64 bytes on the stack if a 
> > given context is known to have short mempolicy strings).
> > 
> 
> Hmm... at least, like most of print functions, it need return a value
> to tell the length it writes, so in my opinion, I still suggest it can
> return a value.
> 

Why?  It can just store the string into the buffer pointed to by the 
char *buffer and terminate it appropriately while taking care that it 
doesn't exceed maxlen.  Why does the caller need to know the number of 
bytes written?  If it really does, you could just do strlen(buffer).

If there's a real reason for it, then that's fine, I just think it can be 
made to always succeed and never return < 0.  (And why is nobody checking 
the return value today if it's so necessary?)

> For common printing functions, caller knows about the string format and
> all parameters, and also can control them,  so for callee, it is not
> 'quite polite' to return any failures to caller.  :-)
> 
> But for our function, caller may not know about the string format and
> parameters' details, so callee has duty to check and process them:
> 
>   e.g. "if related parameter is invalid, it is neccessary to notifiy to caller".
> 

Nobody is using mpol_to_str() to determine if a mempolicy mode is valid :)  
If the struct mempolicy really has a bad mode, then just store "unknown" 
or store a 0.  If maxlen is insufficient for the longest possible string 
stored by mpol_to_str(), then it should be a compile-time error.

--
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 v2] mm/shmem.c: check the return value of mpol_to_str()

[Chen Gang]

On 09/10/2013 02:43 PM, David Rientjes wrote:
> On Tue, 10 Sep 2013, Chen Gang wrote:
> 
>>> I think it would be better to keep mpol_to_str() returning void, and hence 
>>> avoiding the need for this patch, and make it so it cannot fail.  If the 
>>> mode is invalid, just store a 0 to the buffer (or "unknown"); and if 
>>> maxlen isn't large enough, make it a compile-time error (let's avoid 
>>> trying to be fancy and allocating less than 64 bytes on the stack if a 
>>> given context is known to have short mempolicy strings).
>>>
>>
>> Hmm... at least, like most of print functions, it need return a value
>> to tell the length it writes, so in my opinion, I still suggest it can
>> return a value.
>>
> 
> Why?  It can just store the string into the buffer pointed to by the 
> char *buffer and terminate it appropriately while taking care that it 
> doesn't exceed maxlen.  Why does the caller need to know the number of 
> bytes written?  If it really does, you could just do strlen(buffer).
> 
> If there's a real reason for it, then that's fine, I just think it can be 
> made to always succeed and never return < 0.  (And why is nobody checking 
> the return value today if it's so necessary?)
> 

For common printing functions: sprintf(), snprintf(), scnprintf().

For some of specific printing functions: drivers/usb/host/uhci-debug.c.

at least they can let caller easy use.


>> For common printing functions, caller knows about the string format and
>> all parameters, and also can control them,  so for callee, it is not
>> 'quite polite' to return any failures to caller.  :-)
>>
>> But for our function, caller may not know about the string format and
>> parameters' details, so callee has duty to check and process them:
>>
>>   e.g. "if related parameter is invalid, it is neccessary to notifiy to caller".
>>
> 
> Nobody is using mpol_to_str() to determine if a mempolicy mode is valid :)  
> If the struct mempolicy really has a bad mode, then just store "unknown" 
> or store a 0.  If maxlen is insufficient for the longest possible string 
> stored by mpol_to_str(), then it should be a compile-time error.
> 
> 

Hmm... what you said sounds reasonable if mpol_to_str() is a normal
static funciton (only used within a file).

For extern function, callee (inside) can not assume anything of caller
(outside) beyond the interface. So if failure occurs, better to report
to caller only, and let caller to check what to do next.


Thanks.
-- 
Chen Gang

--
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 v2] mm/shmem.c: check the return value of mpol_to_str()

[David Rientjes]

On Tue, 10 Sep 2013, Chen Gang wrote:

> > Why?  It can just store the string into the buffer pointed to by the 
> > char *buffer and terminate it appropriately while taking care that it 
> > doesn't exceed maxlen.  Why does the caller need to know the number of 
> > bytes written?  If it really does, you could just do strlen(buffer).
> > 
> > If there's a real reason for it, then that's fine, I just think it can be 
> > made to always succeed and never return < 0.  (And why is nobody checking 
> > the return value today if it's so necessary?)
> > 
> 
> For common printing functions: sprintf(), snprintf(), scnprintf().
> 
> For some of specific printing functions: drivers/usb/host/uhci-debug.c.
> 
> at least they can let caller easy use.
> 

Nobody needs mpol_to_str() to return the number of characters written, 
period.  It's one of the most trivial functions you're going to see in the 
mempolicy code, it takes a pointer to a buffer and it stores characters to 
it for display.  Nobody is going to use it for anything else.  Let's not 
overcomplicate this trivial function.

> > Nobody is using mpol_to_str() to determine if a mempolicy mode is valid :)  
> > If the struct mempolicy really has a bad mode, then just store "unknown" 
> > or store a 0.  If maxlen is insufficient for the longest possible string 
> > stored by mpol_to_str(), then it should be a compile-time error.
> > 
> > 
> 
> Hmm... what you said sounds reasonable if mpol_to_str() is a normal
> static funciton (only used within a file).
> 
> For extern function, callee (inside) can not assume anything of caller
> (outside) beyond the interface. So if failure occurs, better to report
> to caller only, and let caller to check what to do next.
> 

Are you just preaching about the best practices of software engineering?  
mpol_to_str() should never fail at runtime, plain and simple.  If somebody 
introduces a new mode and doesn't update it to print correctly, let's not 
fail the read().  Let's just print "unknown".  And if someone passes too 
small of a buffer, break it at compile time so it gets noticed and fixed.

I guarantee you that any kernel developer who writes code to call 
mpol_to_str() will be happy it never fails at runtime.  Really.

--
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 v2] mm/shmem.c: check the return value of mpol_to_str()

[KOSAKI Motohiro]

(9/11/13 8:33 PM), David Rientjes wrote:
> On Tue, 10 Sep 2013, Chen Gang wrote:
>
>>> Why?  It can just store the string into the buffer pointed to by the
>>> char *buffer and terminate it appropriately while taking care that it
>>> doesn't exceed maxlen.  Why does the caller need to know the number of
>>> bytes written?  If it really does, you could just do strlen(buffer).
>>>
>>> If there's a real reason for it, then that's fine, I just think it can be
>>> made to always succeed and never return < 0.  (And why is nobody checking
>>> the return value today if it's so necessary?)
>>>
>>
>> For common printing functions: sprintf(), snprintf(), scnprintf().
>>
>> For some of specific printing functions: drivers/usb/host/uhci-debug.c.
>>
>> at least they can let caller easy use.
>>
>
> Nobody needs mpol_to_str() to return the number of characters written,
> period.  It's one of the most trivial functions you're going to see in the
> mempolicy code, it takes a pointer to a buffer and it stores characters to
> it for display.  Nobody is going to use it for anything else.  Let's not
> overcomplicate this trivial function.
>
>>> Nobody is using mpol_to_str() to determine if a mempolicy mode is valid :)
>>> If the struct mempolicy really has a bad mode, then just store "unknown"
>>> or store a 0.  If maxlen is insufficient for the longest possible string
>>> stored by mpol_to_str(), then it should be a compile-time error.
>>>
>>>
>>
>> Hmm... what you said sounds reasonable if mpol_to_str() is a normal
>> static funciton (only used within a file).
>>
>> For extern function, callee (inside) can not assume anything of caller
>> (outside) beyond the interface. So if failure occurs, better to report
>> to caller only, and let caller to check what to do next.
>>
>
> Are you just preaching about the best practices of software engineering?
> mpol_to_str() should never fail at runtime, plain and simple.  If somebody
> introduces a new mode and doesn't update it to print correctly, let's not
> fail the read().  Let's just print "unknown".  And if someone passes too
> small of a buffer, break it at compile time so it gets noticed and fixed.
>
> I guarantee you that any kernel developer who writes code to call
> mpol_to_str() will be happy it never fails at runtime.  Really.

Agreed. Even though we don't change mpol_to_str() interface, please just
add BUG_ON into shmem_show_mpol(). It is much simpler than current proposal.

At least, currently mpol_to_str() already have following assertion. I mean,
the code assume every developer know maximum length of mempolicy. I have no
seen any reason to bring addional complication to shmem area.


	/*
	 * Sanity check:  room for longest mode, flag and some nodes
	 */
	VM_BUG_ON(maxlen < strlen("interleave") + strlen("relative") + 16);

Thanks.

--
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 v2] mm/shmem.c: check the return value of mpol_to_str()

[Chen Gang]

On 09/12/2013 10:19 AM, KOSAKI Motohiro wrote:
> (9/11/13 8:33 PM), David Rientjes wrote:
>> On Tue, 10 Sep 2013, Chen Gang wrote:
>>
>>>> Why?  It can just store the string into the buffer pointed to by the
>>>> char *buffer and terminate it appropriately while taking care that it
>>>> doesn't exceed maxlen.  Why does the caller need to know the number of
>>>> bytes written?  If it really does, you could just do strlen(buffer).
>>>>
>>>> If there's a real reason for it, then that's fine, I just think it
>>>> can be
>>>> made to always succeed and never return < 0.  (And why is nobody
>>>> checking
>>>> the return value today if it's so necessary?)
>>>>
>>>
>>> For common printing functions: sprintf(), snprintf(), scnprintf().
>>>
>>> For some of specific printing functions: drivers/usb/host/uhci-debug.c.
>>>
>>> at least they can let caller easy use.
>>>
>>
>> Nobody needs mpol_to_str() to return the number of characters written,
>> period.  It's one of the most trivial functions you're going to see in
>> the
>> mempolicy code, it takes a pointer to a buffer and it stores
>> characters to
>> it for display.  Nobody is going to use it for anything else.  Let's not
>> overcomplicate this trivial function.
>>
>>>> Nobody is using mpol_to_str() to determine if a mempolicy mode is
>>>> valid :)
>>>> If the struct mempolicy really has a bad mode, then just store
>>>> "unknown"
>>>> or store a 0.  If maxlen is insufficient for the longest possible
>>>> string
>>>> stored by mpol_to_str(), then it should be a compile-time error.
>>>>
>>>>
>>>
>>> Hmm... what you said sounds reasonable if mpol_to_str() is a normal
>>> static funciton (only used within a file).
>>>
>>> For extern function, callee (inside) can not assume anything of caller
>>> (outside) beyond the interface. So if failure occurs, better to report
>>> to caller only, and let caller to check what to do next.
>>>
>>
>> Are you just preaching about the best practices of software engineering?
>> mpol_to_str() should never fail at runtime, plain and simple.  If
>> somebody
>> introduces a new mode and doesn't update it to print correctly, let's not
>> fail the read().  Let's just print "unknown".  And if someone passes too
>> small of a buffer, break it at compile time so it gets noticed and fixed.
>>
>> I guarantee you that any kernel developer who writes code to call
>> mpol_to_str() will be happy it never fails at runtime.  Really.
> 
> Agreed. Even though we don't change mpol_to_str() interface, please just
> add BUG_ON into shmem_show_mpol(). It is much simpler than current
> proposal.
> 

Hmm... that is simpler and clearer for writers, but may not for readers.

> At least, currently mpol_to_str() already have following assertion. I mean,
> the code assume every developer know maximum length of mempolicy. I have no
> seen any reason to bring addional complication to shmem area.
> 
> 
>     /*
>      * Sanity check:  room for longest mode, flag and some nodes
>      */
>     VM_BUG_ON(maxlen < strlen("interleave") + strlen("relative") + 16);
> 
> Thanks.
> 

Hmm... *BUG_ON() is for protecting the OS continue blindly, can we be
sure: "in current condition, OS is continuing blindly?"

If an extern function's parameter is invalid, it mainly means caller
incorrectly use the function (which need return -EINVAL), not means "the
OS is continuing blindly".


Thanks.
-- 
Chen Gang

--
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 v2] mm/shmem.c: check the return value of mpol_to_str()

[David Rientjes]

On Wed, 11 Sep 2013, KOSAKI Motohiro wrote:

> At least, currently mpol_to_str() already have following assertion. I mean,
> the code assume every developer know maximum length of mempolicy. I have no
> seen any reason to bring addional complication to shmem area.
> 
> 
> 	/*
> 	 * Sanity check:  room for longest mode, flag and some nodes
> 	 */
> 	VM_BUG_ON(maxlen < strlen("interleave") + strlen("relative") + 16);
> 

No need to make it a runtime error, the value passed as maxlen is a 
constant, as is the use of sizeof(buffer), so the value is known at 
compile-time.  You can make this a BUILD_BUG_ON() if you are creative.

--
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 v2] mm/shmem.c: check the return value of mpol_to_str()

[KOSAKI Motohiro]

On 9/13/2013 5:12 PM, David Rientjes wrote:
> On Wed, 11 Sep 2013, KOSAKI Motohiro wrote:
> 
>> At least, currently mpol_to_str() already have following assertion. I mean,
>> the code assume every developer know maximum length of mempolicy. I have no
>> seen any reason to bring addional complication to shmem area.
>>
>>
>> 	/*
>> 	 * Sanity check:  room for longest mode, flag and some nodes
>> 	 */
>> 	VM_BUG_ON(maxlen < strlen("interleave") + strlen("relative") + 16);
>>
> 
> No need to make it a runtime error, the value passed as maxlen is a 
> constant, as is the use of sizeof(buffer), so the value is known at 
> compile-time.  You can make this a BUILD_BUG_ON() if you are creative.

Making compile time error brings us another complication. I'd like to
keep just one line assertion.

--
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 v2] mm/shmem.c: check the return value of mpol_to_str()

[Chen Gang]

On 09/14/2013 10:51 AM, KOSAKI Motohiro wrote:
> On 9/13/2013 5:12 PM, David Rientjes wrote:
>> On Wed, 11 Sep 2013, KOSAKI Motohiro wrote:
>>
>>> At least, currently mpol_to_str() already have following assertion. I mean,
>>> the code assume every developer know maximum length of mempolicy. I have no
>>> seen any reason to bring addional complication to shmem area.
>>>
>>>
>>> 	/*
>>> 	 * Sanity check:  room for longest mode, flag and some nodes
>>> 	 */
>>> 	VM_BUG_ON(maxlen < strlen("interleave") + strlen("relative") + 16);
>>>
>>
>> No need to make it a runtime error, the value passed as maxlen is a 
>> constant, as is the use of sizeof(buffer), so the value is known at 
>> compile-time.  You can make this a BUILD_BUG_ON() if you are creative.
> 
> Making compile time error brings us another complication. I'd like to
> keep just one line assertion.
> 

Hmm... I am not quite sure: a C compiler is clever enough to know about
that.

At least, for ANSI C definition, the C compiler has no duty to know
about that.

And it is not for an optimization, either, so I guess the C compiler has
no enought interests to support this features (know about that).


Thanks.
-- 
Chen Gang

--
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 v2] mm/shmem.c: check the return value of mpol_to_str()

[David Rientjes]

On Mon, 16 Sep 2013, Chen Gang wrote:

> Hmm... I am not quite sure: a C compiler is clever enough to know about
> that.
> 
> At least, for ANSI C definition, the C compiler has no duty to know
> about that.
> 
> And it is not for an optimization, either, so I guess the C compiler has
> no enought interests to support this features (know about that).
> 

What on earth are we talking about in this thread?

Rename mpol_to_str() to __mpol_to_str().  Make a static inline function in 
mempolicy.h named mpol_to_str().  That function does BUILD_BUG_ON(maxlen < 
64) and then calls __mpol_to_str().

Modify __mpol_to_str() to store "unknown" when mpol->mode does not match 
any known MPOL_* constant.

Both functions can now return void.

This is like a ten line diff.  Seriously.

--
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 v2] mm/shmem.c: check the return value of mpol_to_str()

[Chen Gang]

On 09/17/2013 04:13 AM, David Rientjes wrote:
> On Mon, 16 Sep 2013, Chen Gang wrote:
> 
>> Hmm... I am not quite sure: a C compiler is clever enough to know about
>> that.
>>
>> At least, for ANSI C definition, the C compiler has no duty to know
>> about that.
>>
>> And it is not for an optimization, either, so I guess the C compiler has
>> no enought interests to support this features (know about that).
>>
> 
> What on earth are we talking about in this thread?
> 

??

> Rename mpol_to_str() to __mpol_to_str().  Make a static inline function in 
> mempolicy.h named mpol_to_str().  That function does BUILD_BUG_ON(maxlen < 
> 64) and then calls __mpol_to_str().
> 
> Modify __mpol_to_str() to store "unknown" when mpol->mode does not match 
> any known MPOL_* constant.
> 

Can we be sure 'maxlen' should not be less than 64?  For show_numa_map()
in fs/proc/task_mmu.c, it use 50 which is less than 64, is it correct?


> Both functions can now return void.
> 
> This is like a ten line diff.  Seriously.
> 
> 

Can we be sure that our output contents are always less than 64 bytes?
Do we need BUG_ON() instead of all '-ENOSPC' in mpol_to_str()?


Hmm... If assume what you said above was always correct: "we are always
sure 64 bytes is enough, and 'maxlen' should be never less than 64".

  It would be better to use a structure (which has a member "char buf[64]") pointer instead of 'buffer' and 'maxlen'.
   (and also still need check 64 memory bondary and '\0' within mpol_to_str).


Thanks.
-- 
Chen Gang

--
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 v2] mm/shmem.c: check the return value of mpol_to_str()

[David Rientjes]

On Tue, 17 Sep 2013, Chen Gang wrote:

> > Rename mpol_to_str() to __mpol_to_str().  Make a static inline function in 
> > mempolicy.h named mpol_to_str().  That function does BUILD_BUG_ON(maxlen < 
> > 64) and then calls __mpol_to_str().
> > 
> > Modify __mpol_to_str() to store "unknown" when mpol->mode does not match 
> > any known MPOL_* constant.
> > 
> 
> Can we be sure 'maxlen' should not be less than 64?  For show_numa_map()
> in fs/proc/task_mmu.c, it use 50 which is less than 64, is it correct?
> 

Whatever the max string length is that can be stored by mpol_to_str() 
preferably rounded to the nearest power of two.

> Can we be sure that our output contents are always less than 64 bytes?
> Do we need BUG_ON() instead of all '-ENOSPC' in mpol_to_str()?
> 

You can determine the maximum string length by looking at the 
implementation of mpol_to_str().

> Hmm... If assume what you said above was always correct: "we are always
> sure 64 bytes is enough, and 'maxlen' should be never less than 64".
> 
>   It would be better to use a structure (which has a member "char buf[64]") pointer instead of 'buffer' and 'maxlen'.
>    (and also still need check 64 memory bondary and '\0' within mpol_to_str).
> 

That's ridiculous, kernel developers who call mpol_to_str() aren't idiots.

I think at this point it will just be best if I propose a patch and ask 
for it to be merged into the -mm tree rather than continue this thread.

--
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 v2] mm/shmem.c: check the return value of mpol_to_str()

[Chen Gang]

On 09/18/2013 06:51 AM, David Rientjes wrote:
> On Tue, 17 Sep 2013, Chen Gang wrote:
> 
>>> Rename mpol_to_str() to __mpol_to_str().  Make a static inline function in 
>>> mempolicy.h named mpol_to_str().  That function does BUILD_BUG_ON(maxlen < 
>>> 64) and then calls __mpol_to_str().
>>>
>>> Modify __mpol_to_str() to store "unknown" when mpol->mode does not match 
>>> any known MPOL_* constant.
>>>
>>
>> Can we be sure 'maxlen' should not be less than 64?  For show_numa_map()
>> in fs/proc/task_mmu.c, it use 50 which is less than 64, is it correct?
>>
> 
> Whatever the max string length is that can be stored by mpol_to_str() 
> preferably rounded to the nearest power of two.
> 

Do you mean: show_numa_map() in "fs/proc/task_mmu.c" also need be
'fixed', what it has done (use 50) is incorrect?


>> Can we be sure that our output contents are always less than 64 bytes?
>> Do we need BUG_ON() instead of all '-ENOSPC' in mpol_to_str()?
>>
> 
> You can determine the maximum string length by looking at the 
> implementation of mpol_to_str().
> 

Can we be sure maximum string will be never changed in future?

>> Hmm... If assume what you said above was always correct: "we are always
>> sure 64 bytes is enough, and 'maxlen' should be never less than 64".
>>
>>   It would be better to use a structure (which has a member "char buf[64]") pointer instead of 'buffer' and 'maxlen'.
>>    (and also still need check 64 memory bondary and '\0' within mpol_to_str).
>>
> 
> That's ridiculous, kernel developers who call mpol_to_str() aren't idiots.
> 

It seems, it is not quite polite. ;-)


Hmm... for extern function, caller has duty to understand interface
precisely, but no duty to understand internal implementation.

So if callee wants caller to know about something, it needs 'express' it
through its' interface, callee can not assume that caller should
understand internal implementation.


> I think at this point it will just be best if I propose a patch and ask 
> for it to be merged into the -mm tree rather than continue this thread.
> 
> 

Hmm... you can try to send a related patch for it, but I am not quite
sure whether can pass reviewers' checking or not.


Thanks.
-- 
Chen Gang

--
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 v2] mm/shmem.c: check the return value of mpol_to_str()

[Chen Gang]

On 09/12/2013 08:33 AM, David Rientjes wrote:
> On Tue, 10 Sep 2013, Chen Gang wrote:
> 
>>> Why?  It can just store the string into the buffer pointed to by the 
>>> char *buffer and terminate it appropriately while taking care that it 
>>> doesn't exceed maxlen.  Why does the caller need to know the number of 
>>> bytes written?  If it really does, you could just do strlen(buffer).
>>>
>>> If there's a real reason for it, then that's fine, I just think it can be 
>>> made to always succeed and never return < 0.  (And why is nobody checking 
>>> the return value today if it's so necessary?)
>>>
>>
>> For common printing functions: sprintf(), snprintf(), scnprintf().
>>
>> For some of specific printing functions: drivers/usb/host/uhci-debug.c.
>>
>> at least they can let caller easy use.
>>
> 
> Nobody needs mpol_to_str() to return the number of characters written, 
> period.  It's one of the most trivial functions you're going to see in the 
> mempolicy code, it takes a pointer to a buffer and it stores characters to 
> it for display.  Nobody is going to use it for anything else.  Let's not 
> overcomplicate this trivial function.
> 

Most of tools functions in kernel are return a value to let the caller
easy use, e.g. memset() which much simpler than our case, still return a
value.

>>> Nobody is using mpol_to_str() to determine if a mempolicy mode is valid :)  
>>> If the struct mempolicy really has a bad mode, then just store "unknown" 
>>> or store a 0.  If maxlen is insufficient for the longest possible string 
>>> stored by mpol_to_str(), then it should be a compile-time error.
>>>
>>>
>>
>> Hmm... what you said sounds reasonable if mpol_to_str() is a normal
>> static funciton (only used within a file).
>>
>> For extern function, callee (inside) can not assume anything of caller
>> (outside) beyond the interface. So if failure occurs, better to report
>> to caller only, and let caller to check what to do next.
>>
> 
> Are you just preaching about the best practices of software engineering?  
> mpol_to_str() should never fail at runtime, plain and simple.  If somebody 
> introduces a new mode and doesn't update it to print correctly, let's not 
> fail the read().  Let's just print "unknown".  And if someone passes too 
> small of a buffer, break it at compile time so it gets noticed and fixed.
> 

For normal static function, that sounds reasonable.

> I guarantee you that any kernel developer who writes code to call 
> mpol_to_str() will be happy it never fails at runtime.  Really.
> 
> 

Hmm... for extern function, at present, maybe you can guarantee, but may
not always in the future. And "Code is mainly for making readers 'happy'
(e.g version mergers), not writers".

For extern function which more than 50 lines (used by 2 sub-systems), it
is strange for readers to find it no return value, also strange to let
*BUG_ON() on the extern function's input parameters directly.

If one caller wants to treat failure as BUG, can "*BUG_ON(mpol_to_str()
< 0)", that will be more clearer to all members (need this patch do like
it? :-) ).


BTW: in my opinion, within mpol_to_str(), the VM_BUG_ON() need be
replaced by returning -EINVAL.

Thanks.
-- 
Chen Gang

--
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 v2] mm/shmem.c: check the return value of mpol_to_str()

[KOSAKI Motohiro]

> BTW: in my opinion, within mpol_to_str(), the VM_BUG_ON() need be
> replaced by returning -EINVAL.

Nope. mpol_to_str() is not carefully designed since it was born. It
doesn't have a way to get proper buffer size. That said, the function
assume all caller know proper buffer size. So, just adding EINVAL
doesn't solve anything. we need to add a way to get proper buffer length
at least if we take your way. However it is overengineering because
current all caller doesn't need it.

--
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 v2] mm/shmem.c: check the return value of mpol_to_str()

[Chen Gang]

On 09/13/2013 02:19 AM, KOSAKI Motohiro wrote:
>> BTW: in my opinion, within mpol_to_str(), the VM_BUG_ON() need be
>> replaced by returning -EINVAL.
> 
> Nope. mpol_to_str() is not carefully designed since it was born. It
> doesn't have a way to get proper buffer size. That said, the function
> assume all caller know proper buffer size. So, just adding EINVAL
> doesn't solve anything. we need to add a way to get proper buffer length
> at least if we take your way. However it is overengineering because
> current all caller doesn't need it.
> 


That sounds reasonable.

Hmm... but I still believe there must be a fixing way to satisfy us all.

Please check the patch below whether can satisfy us all, thanks.


-------------------------------patch begin-----------------------------

mm/shmem.c: use VM_BUG_ON() for mpol_to_str() when it fails.

  mpol_to_str() is an extern function which may return a failure. But in
  our case, it should not,

  If it really return a failure, that means current kernel is continuing
  blindly (e.g. some kernel structures are corrupted), should be stopped
  as soon as possible.

Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 mm/shmem.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/mm/shmem.c b/mm/shmem.c
index 8612a95..3f81120 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -890,7 +890,7 @@ static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
 	if (!mpol || mpol->mode == MPOL_DEFAULT)
 		return;		/* show nothing */
 
-	mpol_to_str(buffer, sizeof(buffer), mpol);
+	VM_BUG_ON(mpol_to_str(buffer, sizeof(buffer), mpol) < 0);
 
 	seq_printf(seq, ",mpol=%s", buffer);
 }
-- 
1.7.7.6

-------------------------------patch end-------------------------------


Thanks.
-- 
Chen Gang

--
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 v2] mm/shmem.c: check the return value of mpol_to_str()

[KOSAKI Motohiro]

> ---
>   mm/shmem.c |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 8612a95..3f81120 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -890,7 +890,7 @@ static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
>   	if (!mpol || mpol->mode == MPOL_DEFAULT)
>   		return;		/* show nothing */
>
> -	mpol_to_str(buffer, sizeof(buffer), mpol);
> +	VM_BUG_ON(mpol_to_str(buffer, sizeof(buffer), mpol) < 0);

NAK. VM_BUG_ON is a kind of assertion. It erase the contents if CONFIG_DEBUG_VM not set.
An argument of assertion should not have any side effect.

--
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 v2] mm/shmem.c: check the return value of mpol_to_str()

[Chen Gang]

On 09/14/2013 12:50 AM, KOSAKI Motohiro wrote:
>> ---
>>   mm/shmem.c |    2 +-
>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/mm/shmem.c b/mm/shmem.c
>> index 8612a95..3f81120 100644
>> --- a/mm/shmem.c
>> +++ b/mm/shmem.c
>> @@ -890,7 +890,7 @@ static void shmem_show_mpol(struct seq_file *seq,
>> struct mempolicy *mpol)
>>       if (!mpol || mpol->mode == MPOL_DEFAULT)
>>           return;        /* show nothing */
>>
>> -    mpol_to_str(buffer, sizeof(buffer), mpol);
>> +    VM_BUG_ON(mpol_to_str(buffer, sizeof(buffer), mpol) < 0);
> 
> NAK. VM_BUG_ON is a kind of assertion. It erase the contents if
> CONFIG_DEBUG_VM not set.
> An argument of assertion should not have any side effect.
> 
> 
> 

Oh, really it is. In my opinion, need use "BUG_ON(mpol_to_str() < 0)"
instead of "VM_BUG_ON(mpol_to_str() < 0);".


Thanks.
-- 
Chen Gang

--
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 v2] mm/shmem.c: check the return value of mpol_to_str()

[KOSAKI Motohiro]

(9/15/13 10:55 PM), Chen Gang wrote:
> On 09/14/2013 12:50 AM, KOSAKI Motohiro wrote:
>>> ---
>>>    mm/shmem.c |    2 +-
>>>    1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/mm/shmem.c b/mm/shmem.c
>>> index 8612a95..3f81120 100644
>>> --- a/mm/shmem.c
>>> +++ b/mm/shmem.c
>>> @@ -890,7 +890,7 @@ static void shmem_show_mpol(struct seq_file *seq,
>>> struct mempolicy *mpol)
>>>        if (!mpol || mpol->mode == MPOL_DEFAULT)
>>>            return;        /* show nothing */
>>>
>>> -    mpol_to_str(buffer, sizeof(buffer), mpol);
>>> +    VM_BUG_ON(mpol_to_str(buffer, sizeof(buffer), mpol) < 0);
>>
>> NAK. VM_BUG_ON is a kind of assertion. It erase the contents if
>> CONFIG_DEBUG_VM not set.
>> An argument of assertion should not have any side effect.
>
> Oh, really it is. In my opinion, need use "BUG_ON(mpol_to_str() < 0)"
> instead of "VM_BUG_ON(mpol_to_str() < 0);".

BUG_ON() is safe. but I still don't like it. As far as I heard, Google
changes BUG_ON as nop. So, BUG_ON(mpol_to_str() < 0) breaks google.
Please treat an assertion as assertion. Not any other something.


--
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 v2] mm/shmem.c: check the return value of mpol_to_str()

[Chen Gang]

On 09/17/2013 12:16 AM, KOSAKI Motohiro wrote:
> (9/15/13 10:55 PM), Chen Gang wrote:
>> On 09/14/2013 12:50 AM, KOSAKI Motohiro wrote:
>>>> ---
>>>>    mm/shmem.c |    2 +-
>>>>    1 files changed, 1 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/mm/shmem.c b/mm/shmem.c
>>>> index 8612a95..3f81120 100644
>>>> --- a/mm/shmem.c
>>>> +++ b/mm/shmem.c
>>>> @@ -890,7 +890,7 @@ static void shmem_show_mpol(struct seq_file *seq,
>>>> struct mempolicy *mpol)
>>>>        if (!mpol || mpol->mode == MPOL_DEFAULT)
>>>>            return;        /* show nothing */
>>>>
>>>> -    mpol_to_str(buffer, sizeof(buffer), mpol);
>>>> +    VM_BUG_ON(mpol_to_str(buffer, sizeof(buffer), mpol) < 0);
>>>
>>> NAK. VM_BUG_ON is a kind of assertion. It erase the contents if
>>> CONFIG_DEBUG_VM not set.
>>> An argument of assertion should not have any side effect.
>>
>> Oh, really it is. In my opinion, need use "BUG_ON(mpol_to_str() < 0)"
>> instead of "VM_BUG_ON(mpol_to_str() < 0);".
> 
> BUG_ON() is safe. but I still don't like it. As far as I heard, Google
> changes BUG_ON as nop. So, BUG_ON(mpol_to_str() < 0) breaks google.
> Please treat an assertion as assertion. Not any other something.
> 

Hmm... in kernel wide, BUG_ON() is 'common' 'standard' assertion, and
"mm/" is a common sub-system (not architecture specific), so when we
use BUG_ON(), we already 'express' our 'opinion' enough to readers.

And some architectures/users really can customize/config 'BUG/BUG_ON'
(they can implement it by themselves, or 'nop').

If they choose 'nop', they can let code size smaller (also may faster),
but they (not we) also have duty to face related risk: "when we find OS
is continuing blindly, we do not let it stop".



Related information for BUG in "init/Kconfig" (which BUG_ON based on):

config BUG
        bool "BUG() support" if EXPERT
        default y
        help
          Disabling this option eliminates support for BUG and WARN, reducing
          the size of your kernel image and potentially quietly ignoring
          numerous fatal conditions. You should only consider disabling this
          option for embedded systems with no facilities for reporting errors.
          Just say Y.



Thanks.
-- 
Chen Gang

--
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 v2] mm/shmem.c: check the return value of mpol_to_str()

[David Rientjes]

On Tue, 17 Sep 2013, Chen Gang wrote:

> > BUG_ON() is safe. but I still don't like it. As far as I heard, Google
> > changes BUG_ON as nop. So, BUG_ON(mpol_to_str() < 0) breaks google.
> > Please treat an assertion as assertion. Not any other something.
> > 

Google does not disable BUG_ON(), sheesh.

> Hmm... in kernel wide, BUG_ON() is 'common' 'standard' assertion, and
> "mm/" is a common sub-system (not architecture specific), so when we
> use BUG_ON(), we already 'express' our 'opinion' enough to readers.
> 

That's ridiculous, we're not going to panic the kernel at runtime because 
a buffer is too small.  Make it a compile-time error like I suggested so 
we catch this before we even build the kernel.

--
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 v2] mm/shmem.c: check the return value of mpol_to_str()

[Chen Gang]

On 09/18/2013 06:53 AM, David Rientjes wrote:
> On Tue, 17 Sep 2013, Chen Gang wrote:
> 
>>> BUG_ON() is safe. but I still don't like it. As far as I heard, Google
>>> changes BUG_ON as nop. So, BUG_ON(mpol_to_str() < 0) breaks google.
>>> Please treat an assertion as assertion. Not any other something.
>>>
> 
> Google does not disable BUG_ON(), sheesh.
> 

That sounds a good news.

>> Hmm... in kernel wide, BUG_ON() is 'common' 'standard' assertion, and
>> "mm/" is a common sub-system (not architecture specific), so when we
>> use BUG_ON(), we already 'express' our 'opinion' enough to readers.
>>
> 
> That's ridiculous, we're not going to panic the kernel at runtime because 
> a buffer is too small.  Make it a compile-time error like I suggested so 
> we catch this before we even build the kernel.
> 

It seems not quite polite?  ;-)


BUG_ON() is widely and commonly used in kernel wide, and BUG_ON() can be
customized by any architectures, so I guess, if google really think it
is necessary, it will customize it.

If "compile-time error" will make code complex to both readers and
writers (e.g. our case), forcing "compile-time error" may still be good
enough to google, but may not be good enough for others.

So in my opinion, for our case which is a common sub-system, not an
architecture specific sub-system, better use "run-time error".


Thanks.
-- 
Chen Gang

--
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 v2] mm/shmem.c: check the return value of mpol_to_str()

[David Rientjes]

On Wed, 18 Sep 2013, Chen Gang wrote:

> BUG_ON() is widely and commonly used in kernel wide, and BUG_ON() can be
> customized by any architectures, so I guess, if google really think it
> is necessary, it will customize it.
> 
> If "compile-time error" will make code complex to both readers and
> writers (e.g. our case), forcing "compile-time error" may still be good
> enough to google, but may not be good enough for others.
> 

Google has nothing to do with this, it treats BUG_ON() just like 99.99% of 
others do.

> So in my opinion, for our case which is a common sub-system, not an
> architecture specific sub-system, better use "run-time error".
> 

That's absolutely insane.  If code is not allocating enough memory for the 
maximum possible length of a string to be stored by mpol_to_str(), it's a 
bug in the code.  We do not panic and reboot the user's machine for such a 
bug.  Instead, we break the build and require the broken code to be fixed.

I have told you exactly how to introduce such a compile-time error.

--
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 v2] mm/shmem.c: check the return value of mpol_to_str()

[David Rientjes]

On Thu, 12 Sep 2013, Chen Gang wrote:

> Hmm... for extern function, at present, maybe you can guarantee, but may
> not always in the future. And "Code is mainly for making readers 'happy'
> (e.g version mergers), not writers".
> 
> For extern function which more than 50 lines (used by 2 sub-systems), it
> is strange for readers to find it no return value, also strange to let
> *BUG_ON() on the extern function's input parameters directly.
> 
> If one caller wants to treat failure as BUG, can "*BUG_ON(mpol_to_str()
> < 0)", that will be more clearer to all members (need this patch do like
> it? :-) ).
> 
> 
> BTW: in my opinion, within mpol_to_str(), the VM_BUG_ON() need be
> replaced by returning -EINVAL.
> 

Are you reading my emails?

I'm asking for a compile-time error if the maxlen passed to mpol_to_str() 
is too small; it's a constant value and can be evaluated at compile-time.  
Then mpol_to_str() can return void if you simply store "unknown" when it's 
an unknown mode.

Sheesh.

--
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 v2] mm/shmem.c: check the return value of mpol_to_str()

[Chen Gang]

On 09/14/2013 05:14 AM, David Rientjes wrote:
> On Thu, 12 Sep 2013, Chen Gang wrote:
> 
>> Hmm... for extern function, at present, maybe you can guarantee, but may
>> not always in the future. And "Code is mainly for making readers 'happy'
>> (e.g version mergers), not writers".
>>
>> For extern function which more than 50 lines (used by 2 sub-systems), it
>> is strange for readers to find it no return value, also strange to let
>> *BUG_ON() on the extern function's input parameters directly.
>>
>> If one caller wants to treat failure as BUG, can "*BUG_ON(mpol_to_str()
>> < 0)", that will be more clearer to all members (need this patch do like
>> it? :-) ).
>>
>>
>> BTW: in my opinion, within mpol_to_str(), the VM_BUG_ON() need be
>> replaced by returning -EINVAL.
>>
> 
> Are you reading my emails?
> 

Yes.

> I'm asking for a compile-time error if the maxlen passed to mpol_to_str() 
> is too small; it's a constant value and can be evaluated at compile-time.  
> Then mpol_to_str() can return void if you simply store "unknown" when it's 
> an unknown mode.
> 

Are/were you saying: 'gcc' can realize an extern functions' input
parameter whether is a constant??

If so, excuse me, I really did not quite understand what you were
saying, but I am still trying to understand.


As far as I know:

  mpol_to_str() is called by 2 areas, which will input different maxlen.
  for a none-inline function, compiler treats parameters as variables.
  for ANSI C compiler, for function's parameter, "array == pointer".

Hmm... maybe you see 'sizeof()'? if so, we also need notice: "multiple
callers only call one callee with there different 'sizeof()'", callee
has to treat these 'sizeof()' values as variable, not constant.


If I am still misunderstanding, please say more with details, thanks.

> Sheesh.
> 
> 

Thanks.
-- 
Chen Gang

--
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>