[PATCH 0/3] mm: mempolicy: the failure processing about mpol_to_str()

[PATCH 0/3] mm: mempolicy: the failure processing about mpol_to_str()

[Chen Gang]

For the implementation (patch 1/3), need fill buffer as full as
possible when buffer space is not enough.

For the caller (patch 2/3, 3/3), need check the return value of
mpol_to_str().

Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 fs/proc/task_mmu.c |   16 ++++++++++++++--
 mm/mempolicy.c     |   14 ++++++++++----
 mm/shmem.c         |   15 ++++++++++++++-
 3 files changed, 38 insertions(+), 7 deletions(-)

--
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 1/3] mm/mempolicy.c: still fill buffer as full as possible when buffer space is not enough in mpol_to_str()

[Chen Gang]

Need still try to fill buffer as full as possible if the buffer space
is not enough, commonly, the caller can bear of it (e.g. print warning
and still continue).

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

diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 27022ca..c81b64f 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -2800,6 +2800,8 @@ out:
  * Convert a mempolicy into a string.
  * Returns the number of characters in buffer (if positive)
  * or an error (negative)
+ * If the buffer space is not enough, it will return -ENOSPC,
+ * and try to fill the buffer as full as possible.
  */
 int mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
 {
@@ -2842,11 +2844,10 @@ int mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
 		return -EINVAL;
 	}
 
+	strlcpy(p, policy_modes[mode], maxlen);
 	l = strlen(policy_modes[mode]);
 	if (buffer + maxlen < p + l + 1)
 		return -ENOSPC;
-
-	strcpy(p, policy_modes[mode]);
 	p += l;
 
 	if (flags & MPOL_MODE_FLAGS) {
@@ -2857,10 +2858,15 @@ int mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
 		/*
 		 * Currently, the only defined flags are mutually exclusive
 		 */
-		if (flags & MPOL_F_STATIC_NODES)
+		if (flags & MPOL_F_STATIC_NODES) {
 			p += snprintf(p, buffer + maxlen - p, "static");
-		else if (flags & MPOL_F_RELATIVE_NODES)
+			if (buffer + maxlen <= p)
+				return -ENOSPC;
+		} else if (flags & MPOL_F_RELATIVE_NODES) {
 			p += snprintf(p, buffer + maxlen - p, "relative");
+			if (buffer + maxlen <= p)
+				return -ENOSPC;
+		}
 	}
 
 	if (!nodes_empty(nodes)) {
-- 
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 2/3] fs/proc/task_mmu.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 print related warning when the buffer space is not enough.


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

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index a117207..1cb7445 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -1359,7 +1359,7 @@ static int show_numa_map(struct seq_file *m, void *v, int is_pid)
 	struct mm_struct *mm = vma->vm_mm;
 	struct mm_walk walk = {};
 	struct mempolicy *pol;
-	int n;
+	int n, ret;
 	char buffer[50];
 
 	if (!mm)
@@ -1376,7 +1376,19 @@ static int show_numa_map(struct seq_file *m, void *v, int is_pid)
 	walk.mm = mm;
 
 	pol = get_vma_policy(task, vma, vma->vm_start);
-	mpol_to_str(buffer, sizeof(buffer), pol);
+	ret = mpol_to_str(buffer, sizeof(buffer), pol);
+	if (ret < 0)
+		switch (ret) {
+		case -ENOSPC:
+			pr_warn("in %s: string is truncated in mpol_to_str().\n",
+				__func__);
+			break;
+		default:
+			pr_err("in %s: call mpol_to_str() fail, errcode: %d. buffer: %p, size: %zu,  pol: %p\n",
+				__func__, ret, buffer, sizeof(buffer), pol);
+			return ret;
+		}
+
 	mpol_cond_put(pol);
 
 	seq_printf(m, "%08lx %s", vma->vm_start, buffer);
-- 
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 3/3] 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 print related warning when the buffer space is not enough.


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

diff --git a/mm/shmem.c b/mm/shmem.c
index 75010ba..eb4eec8 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -886,11 +886,24 @@ redirty:
 static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
 {
 	char buffer[64];
+	int ret;
 
 	if (!mpol || mpol->mode == MPOL_DEFAULT)
 		return;		/* show nothing */
 
-	mpol_to_str(buffer, sizeof(buffer), mpol);
+	ret = mpol_to_str(buffer, sizeof(buffer), mpol);
+	if (ret < 0)
+		switch (ret) {
+		case -ENOSPC:
+			printk(KERN_WARNING
+				"in %s: string is truncated in mpol_to_str().\n",
+				__func__);
+		default:
+			printk(KERN_ERR
+				"in %s: call mpol_to_str() fail, errcode: %d. buffer: %p, size: %zu, pol: %p\n",
+				__func__, ret, buffer, sizeof(buffer), mpol);
+			return;
+		}
 
 	seq_printf(seq, ",mpol=%s", buffer);
 }
-- 
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 0/3] mm: mempolicy: the failure processing about mpol_to_str()

[Cyrill Gorcunov]

On Tue, Aug 20, 2013 at 11:56:15AM +0800, Chen Gang wrote:
> For the implementation (patch 1/3), need fill buffer as full as
> possible when buffer space is not enough.
> 
> For the caller (patch 2/3, 3/3), need check the return value of
> mpol_to_str().
> 
> Signed-off-by: Chen Gang <gang.chen@asianux.com>

Won't simple check for mpol_to_str() < 0 be enough? IOW fix all
callers to check that mpol_to_str exit without errors. As far
as I see here are only two users. Something like

show_numa_map
	ret = mpol_to_str();
	if (ret)
		return ret;

shmem_show_mpol
	ret = mpol_to_str();
	if (ret)
		return ret;

sure you'll have to change shmem_show_mpol statement to return int code.
Won't this be more short and convenient?

--
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 0/3] mm: mempolicy: the failure processing about mpol_to_str()

[Chen Gang]

On 08/20/2013 01:30 PM, Cyrill Gorcunov wrote:
> On Tue, Aug 20, 2013 at 11:56:15AM +0800, Chen Gang wrote:
>> For the implementation (patch 1/3), need fill buffer as full as
>> possible when buffer space is not enough.
>>
>> For the caller (patch 2/3, 3/3), need check the return value of
>> mpol_to_str().
>>
>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> 
> Won't simple check for mpol_to_str() < 0 be enough? IOW fix all
> callers to check that mpol_to_str exit without errors. As far
> as I see here are only two users. Something like
> 
> show_numa_map
> 	ret = mpol_to_str();
> 	if (ret)
> 		return ret;
> 
> shmem_show_mpol
> 	ret = mpol_to_str();
> 	if (ret)
> 		return ret;
> 

need "if (ret < 0)" instead of.  ;-)

> sure you'll have to change shmem_show_mpol statement to return int code.
> Won't this be more short and convenient?
> 
> 

Hmm... if return -ENOSPC, in common processing, it still need continue
(but need let outside know about the string truncation).

So I still suggest to give more check for it.


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 0/3] mm: mempolicy: the failure processing about mpol_to_str()

[Cyrill Gorcunov]

On Tue, Aug 20, 2013 at 01:41:40PM +0800, Chen Gang wrote:
> 
> need "if (ret < 0)" instead of.  ;-)

sure, it's details

> 
> > sure you'll have to change shmem_show_mpol statement to return int code.
> > Won't this be more short and convenient?
> > 
> > 
> 
> Hmm... if return -ENOSPC, in common processing, it still need continue
> (but need let outside know about the string truncation).
> 
> So I still suggest to give more check for it.

I still don't like adding additional code like

+	ret = mpol_to_str(buffer, sizeof(buffer), mpol);
+	if (ret < 0)
+               switch (ret) {
+               case -ENOSPC:
+                       printk(KERN_WARNING
+                               "in %s: string is truncated in mpol_to_str().\n",
+                               __func__);
+               default:
+                       printk(KERN_ERR
+                               "in %s: call mpol_to_str() fail, errcode: %d. buffer: %p, size: %zu, pol: %p\n",
+                               __func__, ret, buffer, sizeof(buffer), mpol);
+                       return;
+               }

this code is pretty neat for debugging purpose I think but in most case (if
only I've not missed something obvious) it simply won't be the case.

Won't somthing like below do the same but with smaller code change?
Note I've not even compiled it but it shows the idea.
---
 fs/proc/task_mmu.c |    4 +++-
 mm/shmem.c         |   17 +++++++++--------
 2 files changed, 12 insertions(+), 9 deletions(-)

Index: linux-2.6.git/fs/proc/task_mmu.c
===================================================================
--- linux-2.6.git.orig/fs/proc/task_mmu.c
+++ linux-2.6.git/fs/proc/task_mmu.c
@@ -1402,8 +1402,10 @@ static int show_numa_map(struct seq_file
 	walk.mm = mm;
 
 	pol = get_vma_policy(task, vma, vma->vm_start);
-	mpol_to_str(buffer, sizeof(buffer), pol);
+	n = mpol_to_str(buffer, sizeof(buffer), pol);
 	mpol_cond_put(pol);
+	if (n < 0)
+		return n;
 
 	seq_printf(m, "%08lx %s", vma->vm_start, buffer);
 
Index: linux-2.6.git/mm/shmem.c
===================================================================
--- linux-2.6.git.orig/mm/shmem.c
+++ linux-2.6.git/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,9 +955,7 @@ static struct page *shmem_alloc_page(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 */
 
 static inline struct page *shmem_swapin(swp_entry_t swap, gfp_t gfp,
@@ -2577,8 +2579,7 @@ static int shmem_show_options(struct seq
 	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 0/3] mm: mempolicy: the failure processing about mpol_to_str()

[Chen Gang]

On 08/20/2013 02:47 PM, Cyrill Gorcunov wrote:
> On Tue, Aug 20, 2013 at 01:41:40PM +0800, Chen Gang wrote:
>>
>>> sure you'll have to change shmem_show_mpol statement to return int code.
>>> Won't this be more short and convenient?
>>>
>>>
>>
>> Hmm... if return -ENOSPC, in common processing, it still need continue
>> (but need let outside know about the string truncation).
>>
>> So I still suggest to give more check for it.
> 
> I still don't like adding additional code like
> 
> +	ret = mpol_to_str(buffer, sizeof(buffer), mpol);
> +	if (ret < 0)
> +               switch (ret) {
> +               case -ENOSPC:
> +                       printk(KERN_WARNING
> +                               "in %s: string is truncated in mpol_to_str().\n",
> +                               __func__);
> +               default:
> +                       printk(KERN_ERR
> +                               "in %s: call mpol_to_str() fail, errcode: %d. buffer: %p, size: %zu, pol: %p\n",
> +                               __func__, ret, buffer, sizeof(buffer), mpol);
> +                       return;
> +               }
> 
> this code is pretty neat for debugging purpose I think but in most case (if
> only I've not missed something obvious) it simply won't be the case.
>

For mpol_to_str(), it is for printing string, I suggest to fill buffer
as full as possible like another printing string functions, -ENOSPC is
not critical error, callers may can bear it, and still want to continue.

For 2 callers, I still suggest to process '-ENOSPC' and continue, it is
really not a critical error, they can continue.

For the 'default' error processing:

  I still suggest to 'printk' in shmem_show_mpol(), because when failure occurs, it has no return value to mark the failure to upper caller.
  Hmm... but for show_numa_map(), may remove the 'printk', only return the error code is OK. :-)


Thanks.

> Won't somthing like below do the same but with smaller code change?
> Note I've not even compiled it but it shows the idea.
> ---
>  fs/proc/task_mmu.c |    4 +++-
>  mm/shmem.c         |   17 +++++++++--------
>  2 files changed, 12 insertions(+), 9 deletions(-)
> 
> Index: linux-2.6.git/fs/proc/task_mmu.c
> ===================================================================
> --- linux-2.6.git.orig/fs/proc/task_mmu.c
> +++ linux-2.6.git/fs/proc/task_mmu.c
> @@ -1402,8 +1402,10 @@ static int show_numa_map(struct seq_file
>  	walk.mm = mm;
>  
>  	pol = get_vma_policy(task, vma, vma->vm_start);
> -	mpol_to_str(buffer, sizeof(buffer), pol);
> +	n = mpol_to_str(buffer, sizeof(buffer), pol);
>  	mpol_cond_put(pol);
> +	if (n < 0)
> +		return n;
>  
>  	seq_printf(m, "%08lx %s", vma->vm_start, buffer);
>  
> Index: linux-2.6.git/mm/shmem.c
> ===================================================================
> --- linux-2.6.git.orig/mm/shmem.c
> +++ linux-2.6.git/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,9 +955,7 @@ static struct page *shmem_alloc_page(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 */
>  
>  static inline struct page *shmem_swapin(swp_entry_t swap, gfp_t gfp,
> @@ -2577,8 +2579,7 @@ static int shmem_show_options(struct seq
>  	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 0/3] mm: mempolicy: the failure processing about mpol_to_str()

[Chen Gang]

On 08/20/2013 03:48 PM, Chen Gang wrote:
> On 08/20/2013 02:47 PM, Cyrill Gorcunov wrote:
>> On Tue, Aug 20, 2013 at 01:41:40PM +0800, Chen Gang wrote:
>>>
>>>> sure you'll have to change shmem_show_mpol statement to return int code.
>>>> Won't this be more short and convenient?
>>>>
>>>>
>>>
>>> Hmm... if return -ENOSPC, in common processing, it still need continue
>>> (but need let outside know about the string truncation).
>>>
>>> So I still suggest to give more check for it.
>>
>> I still don't like adding additional code like
>>
>> +	ret = mpol_to_str(buffer, sizeof(buffer), mpol);
>> +	if (ret < 0)
>> +               switch (ret) {
>> +               case -ENOSPC:
>> +                       printk(KERN_WARNING
>> +                               "in %s: string is truncated in mpol_to_str().\n",
>> +                               __func__);

Oh, that need 'break' in my original patch. :-)

>> +               default:
>> +                       printk(KERN_ERR
>> +                               "in %s: call mpol_to_str() fail, errcode: %d. buffer: %p, size: %zu, pol: %p\n",
>> +                               __func__, ret, buffer, sizeof(buffer), mpol);
>> +                       return;
>> +               }
>>
>> this code is pretty neat for debugging purpose I think but in most case (if
>> only I've not missed something obvious) it simply won't be the case.
>>
> 
> For mpol_to_str(), it is for printing string, I suggest to fill buffer
> as full as possible like another printing string functions, -ENOSPC is
> not critical error, callers may can bear it, and still want to continue.
> 
> For 2 callers, I still suggest to process '-ENOSPC' and continue, it is
> really not a critical error, they can continue.
> 
> For the 'default' error processing:
> 
>   I still suggest to 'printk' in shmem_show_mpol(), because when failure occurs, it has no return value to mark the failure to upper caller.
>   Hmm... but for show_numa_map(), may remove the 'printk', only return the error code is OK. :-)
> 
> 
> Thanks.
> 
>> Won't somthing like below do the same but with smaller code change?
>> Note I've not even compiled it but it shows the idea.
>> ---
>>  fs/proc/task_mmu.c |    4 +++-
>>  mm/shmem.c         |   17 +++++++++--------
>>  2 files changed, 12 insertions(+), 9 deletions(-)
>>
>> Index: linux-2.6.git/fs/proc/task_mmu.c
>> ===================================================================
>> --- linux-2.6.git.orig/fs/proc/task_mmu.c
>> +++ linux-2.6.git/fs/proc/task_mmu.c
>> @@ -1402,8 +1402,10 @@ static int show_numa_map(struct seq_file
>>  	walk.mm = mm;
>>  
>>  	pol = get_vma_policy(task, vma, vma->vm_start);
>> -	mpol_to_str(buffer, sizeof(buffer), pol);
>> +	n = mpol_to_str(buffer, sizeof(buffer), pol);
>>  	mpol_cond_put(pol);
>> +	if (n < 0)
>> +		return n;
>>  
>>  	seq_printf(m, "%08lx %s", vma->vm_start, buffer);
>>  
>> Index: linux-2.6.git/mm/shmem.c
>> ===================================================================
>> --- linux-2.6.git.orig/mm/shmem.c
>> +++ linux-2.6.git/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,9 +955,7 @@ static struct page *shmem_alloc_page(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 */
>>  
>>  static inline struct page *shmem_swapin(swp_entry_t swap, gfp_t gfp,
>> @@ -2577,8 +2579,7 @@ static int shmem_show_options(struct seq
>>  	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 0/3] mm: mempolicy: the failure processing about mpol_to_str()

[Chen Gang]

On 08/20/2013 03:51 PM, Chen Gang wrote:
> On 08/20/2013 03:48 PM, Chen Gang wrote:
>> On 08/20/2013 02:47 PM, Cyrill Gorcunov wrote:
>>> On Tue, Aug 20, 2013 at 01:41:40PM +0800, Chen Gang wrote:
>>>>
>>>>> sure you'll have to change shmem_show_mpol statement to return int code.
>>>>> Won't this be more short and convenient?
>>>>>
>>>>>
>>>>
>>>> Hmm... if return -ENOSPC, in common processing, it still need continue
>>>> (but need let outside know about the string truncation).
>>>>
>>>> So I still suggest to give more check for it.
>>>
>>> I still don't like adding additional code like
>>>
>>> +	ret = mpol_to_str(buffer, sizeof(buffer), mpol);
>>> +	if (ret < 0)
>>> +               switch (ret) {
>>> +               case -ENOSPC:
>>> +                       printk(KERN_WARNING
>>> +                               "in %s: string is truncated in mpol_to_str().\n",
>>> +                               __func__);
> 
> Oh, that need 'break' in my original patch. :-)
> 
>>> +               default:
>>> +                       printk(KERN_ERR
>>> +                               "in %s: call mpol_to_str() fail, errcode: %d. buffer: %p, size: %zu, pol: %p\n",
>>> +                               __func__, ret, buffer, sizeof(buffer), mpol);
>>> +                       return;
>>> +               }
>>>
>>> this code is pretty neat for debugging purpose I think but in most case (if
>>> only I've not missed something obvious) it simply won't be the case.
>>>
>>
>> For mpol_to_str(), it is for printing string, I suggest to fill buffer
>> as full as possible like another printing string functions, -ENOSPC is
>> not critical error, callers may can bear it, and still want to continue.
>>
>> For 2 callers, I still suggest to process '-ENOSPC' and continue, it is
>> really not a critical error, they can continue.
>>
>> For the 'default' error processing:
>>
>>   I still suggest to 'printk' in shmem_show_mpol(), because when failure occurs, it has no return value to mark the failure to upper caller.
>>   Hmm... but for show_numa_map(), may remove the 'printk', only return the error code is OK. :-)
>>
>>
>> Thanks.
>>

Oh, for '-ENOSPC', it means critical error, it is my fault.

So, for simplify thinking and implementation, use your patch below is OK
to me (but I suggest to print error information in the none return value
function).

:-)

>>> Won't somthing like below do the same but with smaller code change?
>>> Note I've not even compiled it but it shows the idea.
>>> ---
>>>  fs/proc/task_mmu.c |    4 +++-
>>>  mm/shmem.c         |   17 +++++++++--------
>>>  2 files changed, 12 insertions(+), 9 deletions(-)
>>>
>>> Index: linux-2.6.git/fs/proc/task_mmu.c
>>> ===================================================================
>>> --- linux-2.6.git.orig/fs/proc/task_mmu.c
>>> +++ linux-2.6.git/fs/proc/task_mmu.c
>>> @@ -1402,8 +1402,10 @@ static int show_numa_map(struct seq_file
>>>  	walk.mm = mm;
>>>  
>>>  	pol = get_vma_policy(task, vma, vma->vm_start);
>>> -	mpol_to_str(buffer, sizeof(buffer), pol);
>>> +	n = mpol_to_str(buffer, sizeof(buffer), pol);
>>>  	mpol_cond_put(pol);
>>> +	if (n < 0)
>>> +		return n;
>>>  
>>>  	seq_printf(m, "%08lx %s", vma->vm_start, buffer);
>>>  
>>> Index: linux-2.6.git/mm/shmem.c
>>> ===================================================================
>>> --- linux-2.6.git.orig/mm/shmem.c
>>> +++ linux-2.6.git/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,9 +955,7 @@ static struct page *shmem_alloc_page(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 */
>>>  
>>>  static inline struct page *shmem_swapin(swp_entry_t swap, gfp_t gfp,
>>> @@ -2577,8 +2579,7 @@ static int shmem_show_options(struct seq
>>>  	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 0/3] mm: mempolicy: the failure processing about mpol_to_str()

[Chen Gang F T]

On 08/20/2013 04:09 PM, Chen Gang wrote:
> On 08/20/2013 03:51 PM, Chen Gang wrote:
>> On 08/20/2013 03:48 PM, Chen Gang wrote:
>>> On 08/20/2013 02:47 PM, Cyrill Gorcunov wrote:
>>>> On Tue, Aug 20, 2013 at 01:41:40PM +0800, Chen Gang wrote:
>>>>>
>>>>>> sure you'll have to change shmem_show_mpol statement to return int code.
>>>>>> Won't this be more short and convenient?
>>>>>>
>>>>>>
>>>>>
>>>>> Hmm... if return -ENOSPC, in common processing, it still need continue
>>>>> (but need let outside know about the string truncation).
>>>>>
>>>>> So I still suggest to give more check for it.
>>>>
>>>> I still don't like adding additional code like
>>>>
>>>> +	ret = mpol_to_str(buffer, sizeof(buffer), mpol);
>>>> +	if (ret < 0)
>>>> +               switch (ret) {
>>>> +               case -ENOSPC:
>>>> +                       printk(KERN_WARNING
>>>> +                               "in %s: string is truncated in mpol_to_str().\n",
>>>> +                               __func__);
>>
>> Oh, that need 'break' in my original patch. :-)
>>
>>>> +               default:
>>>> +                       printk(KERN_ERR
>>>> +                               "in %s: call mpol_to_str() fail, errcode: %d. buffer: %p, size: %zu, pol: %p\n",
>>>> +                               __func__, ret, buffer, sizeof(buffer), mpol);
>>>> +                       return;
>>>> +               }
>>>>
>>>> this code is pretty neat for debugging purpose I think but in most case (if
>>>> only I've not missed something obvious) it simply won't be the case.
>>>>
>>>
>>> For mpol_to_str(), it is for printing string, I suggest to fill buffer
>>> as full as possible like another printing string functions, -ENOSPC is
>>> not critical error, callers may can bear it, and still want to continue.
>>>
>>> For 2 callers, I still suggest to process '-ENOSPC' and continue, it is
>>> really not a critical error, they can continue.
>>>
>>> For the 'default' error processing:
>>>
>>>   I still suggest to 'printk' in shmem_show_mpol(), because when failure occurs, it has no return value to mark the failure to upper caller.
>>>   Hmm... but for show_numa_map(), may remove the 'printk', only return the error code is OK. :-)
>>>
>>>
>>> Thanks.
>>>
> 
> Oh, for '-ENOSPC', it means critical error, it is my fault.
> 
> So, for simplify thinking and implementation, use your patch below is OK
> to me (but I suggest to print error information in the none return value
> function).
> 
> :-)
> 
>>>> Won't somthing like below do the same but with smaller code change?
>>>> Note I've not even compiled it but it shows the idea.
>>>> ---
>>>>  fs/proc/task_mmu.c |    4 +++-
>>>>  mm/shmem.c         |   17 +++++++++--------
>>>>  2 files changed, 12 insertions(+), 9 deletions(-)
>>>>
>>>> Index: linux-2.6.git/fs/proc/task_mmu.c
>>>> ===================================================================
>>>> --- linux-2.6.git.orig/fs/proc/task_mmu.c
>>>> +++ linux-2.6.git/fs/proc/task_mmu.c
>>>> @@ -1402,8 +1402,10 @@ static int show_numa_map(struct seq_file
>>>>  	walk.mm = mm;
>>>>  
>>>>  	pol = get_vma_policy(task, vma, vma->vm_start);
>>>> -	mpol_to_str(buffer, sizeof(buffer), pol);
>>>> +	n = mpol_to_str(buffer, sizeof(buffer), pol);
>>>>  	mpol_cond_put(pol);
>>>> +	if (n < 0)
>>>> +		return n;
>>>>  
>>>>  	seq_printf(m, "%08lx %s", vma->vm_start, buffer);
>>>>  
>>>> Index: linux-2.6.git/mm/shmem.c
>>>> ===================================================================
>>>> --- linux-2.6.git.orig/mm/shmem.c
>>>> +++ linux-2.6.git/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,9 +955,7 @@ static struct page *shmem_alloc_page(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 */
>>>>  
>>>>  static inline struct page *shmem_swapin(swp_entry_t swap, gfp_t gfp,
>>>> @@ -2577,8 +2579,7 @@ static int shmem_show_options(struct seq
>>>>  	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 */
>>>>  
>>>>
>>>>

Oh, you have done, sorry.

>>>
>>>
>>
>>
> 
> 


-- 
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 0/3] mm: mempolicy: the failure processing about mpol_to_str()

[Chen Gang]

Sorry for reply multiple times to bother many members.

It seems, I am tired, and need have a rest today. :-(


On 08/20/2013 04:13 PM, Chen Gang F T wrote:
> On 08/20/2013 04:09 PM, Chen Gang wrote:
>> On 08/20/2013 03:51 PM, Chen Gang wrote:
>>> On 08/20/2013 03:48 PM, Chen Gang wrote:
>>>> On 08/20/2013 02:47 PM, Cyrill Gorcunov wrote:
>>>>> On Tue, Aug 20, 2013 at 01:41:40PM +0800, Chen Gang wrote:
>>>>>>
>>>>>>> sure you'll have to change shmem_show_mpol statement to return int code.
>>>>>>> Won't this be more short and convenient?
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> Hmm... if return -ENOSPC, in common processing, it still need continue
>>>>>> (but need let outside know about the string truncation).
>>>>>>
>>>>>> So I still suggest to give more check for it.
>>>>>
>>>>> I still don't like adding additional code like
>>>>>
>>>>> +	ret = mpol_to_str(buffer, sizeof(buffer), mpol);
>>>>> +	if (ret < 0)
>>>>> +               switch (ret) {
>>>>> +               case -ENOSPC:
>>>>> +                       printk(KERN_WARNING
>>>>> +                               "in %s: string is truncated in mpol_to_str().\n",
>>>>> +                               __func__);
>>>
>>> Oh, that need 'break' in my original patch. :-)
>>>
>>>>> +               default:
>>>>> +                       printk(KERN_ERR
>>>>> +                               "in %s: call mpol_to_str() fail, errcode: %d. buffer: %p, size: %zu, pol: %p\n",
>>>>> +                               __func__, ret, buffer, sizeof(buffer), mpol);
>>>>> +                       return;
>>>>> +               }
>>>>>
>>>>> this code is pretty neat for debugging purpose I think but in most case (if
>>>>> only I've not missed something obvious) it simply won't be the case.
>>>>>
>>>>
>>>> For mpol_to_str(), it is for printing string, I suggest to fill buffer
>>>> as full as possible like another printing string functions, -ENOSPC is
>>>> not critical error, callers may can bear it, and still want to continue.
>>>>
>>>> For 2 callers, I still suggest to process '-ENOSPC' and continue, it is
>>>> really not a critical error, they can continue.
>>>>
>>>> For the 'default' error processing:
>>>>
>>>>   I still suggest to 'printk' in shmem_show_mpol(), because when failure occurs, it has no return value to mark the failure to upper caller.
>>>>   Hmm... but for show_numa_map(), may remove the 'printk', only return the error code is OK. :-)
>>>>
>>>>
>>>> Thanks.
>>>>
>>
>> Oh, for '-ENOSPC', it means critical error, it is my fault.
>>
>> So, for simplify thinking and implementation, use your patch below is OK
>> to me (but I suggest to print error information in the none return value
>> function).
>>
>> :-)
>>
>>>>> Won't somthing like below do the same but with smaller code change?
>>>>> Note I've not even compiled it but it shows the idea.
>>>>> ---
>>>>>  fs/proc/task_mmu.c |    4 +++-
>>>>>  mm/shmem.c         |   17 +++++++++--------
>>>>>  2 files changed, 12 insertions(+), 9 deletions(-)
>>>>>
>>>>> Index: linux-2.6.git/fs/proc/task_mmu.c
>>>>> ===================================================================
>>>>> --- linux-2.6.git.orig/fs/proc/task_mmu.c
>>>>> +++ linux-2.6.git/fs/proc/task_mmu.c
>>>>> @@ -1402,8 +1402,10 @@ static int show_numa_map(struct seq_file
>>>>>  	walk.mm = mm;
>>>>>  
>>>>>  	pol = get_vma_policy(task, vma, vma->vm_start);
>>>>> -	mpol_to_str(buffer, sizeof(buffer), pol);
>>>>> +	n = mpol_to_str(buffer, sizeof(buffer), pol);
>>>>>  	mpol_cond_put(pol);
>>>>> +	if (n < 0)
>>>>> +		return n;
>>>>>  
>>>>>  	seq_printf(m, "%08lx %s", vma->vm_start, buffer);
>>>>>  
>>>>> Index: linux-2.6.git/mm/shmem.c
>>>>> ===================================================================
>>>>> --- linux-2.6.git.orig/mm/shmem.c
>>>>> +++ linux-2.6.git/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,9 +955,7 @@ static struct page *shmem_alloc_page(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 */
>>>>>  
>>>>>  static inline struct page *shmem_swapin(swp_entry_t swap, gfp_t gfp,
>>>>> @@ -2577,8 +2579,7 @@ static int shmem_show_options(struct seq
>>>>>  	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 */
>>>>>  
>>>>>
>>>>>
> 
> Oh, you have done, sorry.
> 
>>>>
>>>>
>>>
>>>
>>
>>
> 
> 


-- 
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 0/3] mm: mempolicy: the failure processing about mpol_to_str()

[Cyrill Gorcunov]

On Tue, Aug 20, 2013 at 04:09:22PM +0800, Chen Gang wrote:
> So, for simplify thinking and implementation, use your patch below is OK
> to me (but I suggest to print error information in the none return value
> function).

Chen, I'm not going to dive into this area now, too busy with other stuff
sorry, so if you consider my preliminary draft patch useful -- pick it up,
modify it, test it and send to lkml then (just CC me).

--
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 0/3] mm: mempolicy: the failure processing about mpol_to_str()

[Chen Gang]

On 08/20/2013 04:25 PM, Cyrill Gorcunov wrote:
> On Tue, Aug 20, 2013 at 04:09:22PM +0800, Chen Gang wrote:
>> So, for simplify thinking and implementation, use your patch below is OK
>> to me (but I suggest to print error information in the none return value
>> function).
> 
> Chen, I'm not going to dive into this area now, too busy with other stuff
> sorry, so if you consider my preliminary draft patch useful -- pick it up,
> modify it, test it and send to lkml then (just CC me).
> 
> 

OK, I will do tomorrow. :-)

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>

[PATCH 0/3] mm: shmem: 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.

Signed-off-by: Chen Gang <gang.chen@asianux.com>
Cc: Cyrill Gorcunov <gorcunov@gmail.com>
---
 fs/proc/task_mmu.c |    4 +++-
 mm/shmem.c         |   16 ++++++++++------
 2 files changed, 13 insertions(+), 7 deletions(-)

--
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 1/3] fs/proc/task_mmu.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.

The failure return need after mpol_cond_put() to match get_vma_policy().


Signed-off-by: Chen Gang <gang.chen@asianux.com>
Cc: Cyrill Gorcunov <gorcunov@gmail.com>
---
 fs/proc/task_mmu.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 107d026..d87f5da 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -1376,8 +1376,10 @@ static int show_numa_map(struct seq_file *m, void *v, int is_pid)
 	walk.mm = mm;
 
 	pol = get_vma_policy(task, vma, vma->vm_start);
-	mpol_to_str(buffer, sizeof(buffer), pol);
+	n = mpol_to_str(buffer, sizeof(buffer), pol);
 	mpol_cond_put(pol);
+	if (n < 0)
+		return n;
 
 	seq_printf(m, "%08lx %s", vma->vm_start, buffer);
 
-- 
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 2/3] mm/shmem.c: let shmem_show_mpol() return value.

[Chen Gang]

Let shmem_show_mpol() return value, since it may fail.

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

diff --git a/mm/shmem.c b/mm/shmem.c
index 75010ba..e59d332 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -883,16 +883,17 @@ 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];
 
 	if (!mpol || mpol->mode == MPOL_DEFAULT)
-		return;		/* show nothing */
+		return 0;		/* show nothing */
 
 	mpol_to_str(buffer, sizeof(buffer), mpol);
 
 	seq_printf(seq, ",mpol=%s", buffer);
+	return 0;
 }
 
 static struct mempolicy *shmem_get_sbmpol(struct shmem_sb_info *sbinfo)
@@ -951,8 +952,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 */
 
@@ -2578,8 +2580,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 3/3] 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.


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

diff --git a/mm/shmem.c b/mm/shmem.c
index e59d332..ae5112f 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -886,11 +886,14 @@ redirty:
 static int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
 {
 	char buffer[64];
+	int ret;
 
 	if (!mpol || mpol->mode == MPOL_DEFAULT)
 		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;
-- 
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 2/3] mm/shmem.c: let shmem_show_mpol() return value.

[Andrew Morton]

On Wed, 21 Aug 2013 10:23:58 +0800 Chen Gang <gang.chen@asianux.com> wrote:

> Let shmem_show_mpol() return value, since it may fail.
> 

This patch has no effect.

> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -883,16 +883,17 @@ 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];
>  
>  	if (!mpol || mpol->mode == MPOL_DEFAULT)
> -		return;		/* show nothing */
> +		return 0;		/* show nothing */
>  
>  	mpol_to_str(buffer, sizeof(buffer), mpol);

Perhaps you meant to check the mpol_to_str() return value here.

>  	seq_printf(seq, ",mpol=%s", buffer);
> +	return 0;
>  }
>  
>  static struct mempolicy *shmem_get_sbmpol(struct shmem_sb_info *sbinfo)

--
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 2/3] mm/shmem.c: let shmem_show_mpol() return value.

[Chen Gang]

On 08/22/2013 06:03 AM, Andrew Morton wrote:
> On Wed, 21 Aug 2013 10:23:58 +0800 Chen Gang <gang.chen@asianux.com> wrote:
> 
>> Let shmem_show_mpol() return value, since it may fail.
>>
> 
> This patch has no effect.
> 
>> --- a/mm/shmem.c
>> +++ b/mm/shmem.c
>> @@ -883,16 +883,17 @@ 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];
>>  
>>  	if (!mpol || mpol->mode == MPOL_DEFAULT)
>> -		return;		/* show nothing */
>> +		return 0;		/* show nothing */
>>  
>>  	mpol_to_str(buffer, sizeof(buffer), mpol);
> 
> Perhaps you meant to check the mpol_to_str() return value here.
> 

Yes, I will merge them together (merge Patch 2/3 and Patch 3/3).

>>  	seq_printf(seq, ",mpol=%s", buffer);
>> +	return 0;
>>  }
>>  
>>  static struct mempolicy *shmem_get_sbmpol(struct shmem_sb_info *sbinfo)
> 
> 
> 

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>

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

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

[Chen Gang]

Hello Maintainers:

Please help check this patch, when you have time.

If it need additional test, please let me know, I should try (better to
provide some suggestions for test).


Thanks.

On 08/22/2013 09:04 AM, Chen Gang wrote:
> 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 */
>  
> 


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

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

[patch] mm, mempolicy: make mpol_to_str robust and always succeed

[David Rientjes]

mpol_to_str() should not fail.  Currently, it either fails because the
string buffer is too small or because a string hasn't been defined for a
mempolicy mode.

If a new mempolicy mode is introduced and no string is defined for it,
just warn and return "unknown".

If the buffer is too small, just truncate the string and return, the same
behavior as snprintf().

This also fixes a bug where there was no NULL-byte termination when doing
*p++ = '=' and *p++ ':' and maxlen has been reached.

Signed-off-by: David Rientjes <rientjes@google.com>
---
 fs/proc/task_mmu.c        | 14 ++++++-------
 include/linux/mempolicy.h |  5 ++---
 mm/mempolicy.c            | 52 +++++++++++++++--------------------------------
 3 files changed, 24 insertions(+), 47 deletions(-)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -1385,8 +1385,8 @@ static int show_numa_map(struct seq_file *m, void *v, int is_pid)
 	struct mm_struct *mm = vma->vm_mm;
 	struct mm_walk walk = {};
 	struct mempolicy *pol;
-	int n;
-	char buffer[50];
+	char buffer[64];
+	int nid;
 
 	if (!mm)
 		return 0;
@@ -1402,10 +1402,8 @@ static int show_numa_map(struct seq_file *m, void *v, int is_pid)
 	walk.mm = mm;
 
 	pol = get_vma_policy(task, vma, vma->vm_start);
-	n = mpol_to_str(buffer, sizeof(buffer), pol);
+	mpol_to_str(buffer, sizeof(buffer), pol);
 	mpol_cond_put(pol);
-	if (n < 0)
-		return n;
 
 	seq_printf(m, "%08lx %s", vma->vm_start, buffer);
 
@@ -1458,9 +1456,9 @@ static int show_numa_map(struct seq_file *m, void *v, int is_pid)
 	if (md->writeback)
 		seq_printf(m, " writeback=%lu", md->writeback);
 
-	for_each_node_state(n, N_MEMORY)
-		if (md->node[n])
-			seq_printf(m, " N%d=%lu", n, md->node[n]);
+	for_each_node_state(nid, N_MEMORY)
+		if (md->node[nid])
+			seq_printf(m, " N%d=%lu", nid, md->node[nid]);
 out:
 	seq_putc(m, '\n');
 
diff --git a/include/linux/mempolicy.h b/include/linux/mempolicy.h
--- a/include/linux/mempolicy.h
+++ b/include/linux/mempolicy.h
@@ -168,7 +168,7 @@ int do_migrate_pages(struct mm_struct *mm, const nodemask_t *from,
 extern int mpol_parse_str(char *str, struct mempolicy **mpol);
 #endif
 
-extern int mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol);
+extern void mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol);
 
 /* Check if a vma is migratable */
 static inline int vma_migratable(struct vm_area_struct *vma)
@@ -306,9 +306,8 @@ static inline int mpol_parse_str(char *str, struct mempolicy **mpol)
 }
 #endif
 
-static inline int mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
+static inline void mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
 {
-	return 0;
 }
 
 static inline int mpol_misplaced(struct page *page, struct vm_area_struct *vma,
diff --git a/mm/mempolicy.c b/mm/mempolicy.c
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -2840,62 +2840,45 @@ out:
  * @maxlen:  length of @buffer
  * @pol:  pointer to mempolicy to be formatted
  *
- * Convert a mempolicy into a string.
- * Returns the number of characters in buffer (if positive)
- * or an error (negative)
+ * Convert @pol into a string.  If @buffer is too short, truncate the string.
+ * Recommend a @maxlen of at least 32 for the longest mode, "interleave", the
+ * longest flag, "relative", and to display at least a few node ids.
  */
-int mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
+void mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
 {
 	char *p = buffer;
-	int l;
-	nodemask_t nodes;
-	unsigned short mode;
-	unsigned short flags = pol ? pol->flags : 0;
-
-	/*
-	 * Sanity check:  room for longest mode, flag and some nodes
-	 */
-	VM_BUG_ON(maxlen < strlen("interleave") + strlen("relative") + 16);
+	nodemask_t nodes = NODE_MASK_NONE;
+	unsigned short mode = MPOL_DEFAULT;
+	unsigned short flags = 0;
 
-	if (!pol || pol == &default_policy)
-		mode = MPOL_DEFAULT;
-	else
+	if (pol && pol != &default_policy) {
 		mode = pol->mode;
+		flags = pol->flags;
+	}
 
 	switch (mode) {
 	case MPOL_DEFAULT:
-		nodes_clear(nodes);
 		break;
-
 	case MPOL_PREFERRED:
-		nodes_clear(nodes);
 		if (flags & MPOL_F_LOCAL)
 			mode = MPOL_LOCAL;
 		else
 			node_set(pol->v.preferred_node, nodes);
 		break;
-
 	case MPOL_BIND:
-		/* Fall through */
 	case MPOL_INTERLEAVE:
 		nodes = pol->v.nodes;
 		break;
-
 	default:
-		return -EINVAL;
+		WARN_ON_ONCE(1);
+		snprintf(p, maxlen, "unknown");
+		return;
 	}
 
-	l = strlen(policy_modes[mode]);
-	if (buffer + maxlen < p + l + 1)
-		return -ENOSPC;
-
-	strcpy(p, policy_modes[mode]);
-	p += l;
+	p += snprintf(p, maxlen, policy_modes[mode]);
 
 	if (flags & MPOL_MODE_FLAGS) {
-		if (buffer + maxlen < p + 2)
-			return -ENOSPC;
-		*p++ = '=';
+		p += snprintf(p, buffer + maxlen - p, "=");
 
 		/*
 		 * Currently, the only defined flags are mutually exclusive
@@ -2907,10 +2890,7 @@ int mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
 	}
 
 	if (!nodes_empty(nodes)) {
-		if (buffer + maxlen < p + 2)
-			return -ENOSPC;
-		*p++ = ':';
+		p += snprintf(p, buffer + maxlen - p, ":");
 	 	p += nodelist_scnprintf(p, buffer + maxlen - p, nodes);
 	}
-	return p - buffer;
 }

--
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, mempolicy: make mpol_to_str robust and always succeed

[Dave Jones]

On Tue, Sep 24, 2013 at 07:58:22PM -0700, David Rientjes wrote:

 >  	case MPOL_BIND:
 > -		/* Fall through */
 >  	case MPOL_INTERLEAVE:
 >  		nodes = pol->v.nodes;
 >  		break;

Any reason not to leave this ?

"missing break" is the 2nd most common thing that coverity picks up.
Most of them are false positives like the above, but the lack of annotations
in our source makes it time-consuming to pick through them all to find the
real bugs.

	Dave

--
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, mempolicy: make mpol_to_str robust and always succeed

[David Rientjes]

On Tue, 24 Sep 2013, Dave Jones wrote:

>  >  	case MPOL_BIND:
>  > -		/* Fall through */
>  >  	case MPOL_INTERLEAVE:
>  >  		nodes = pol->v.nodes;
>  >  		break;
> 
> Any reason not to leave this ?
> 
> "missing break" is the 2nd most common thing that coverity picks up.
> Most of them are false positives like the above, but the lack of annotations
> in our source makes it time-consuming to pick through them all to find the
> real bugs.
> 

Check out things like drivers/mfd/wm5110-tables.c that do things like

	switch (reg) {
	case ARIZONA_SOFTWARE_RESET:
	case ARIZONA_DEVICE_REVISION:
	case ARIZONA_CTRL_IF_SPI_CFG_1:
	case ARIZONA_CTRL_IF_I2C1_CFG_1:
	case ARIZONA_CTRL_IF_I2C2_CFG_1:
	case ARIZONA_CTRL_IF_I2C1_CFG_2:
	case ARIZONA_CTRL_IF_I2C2_CFG_2:
	...

and that file has over 1,000 case statements.  Having a

	/* fall through */

for all of them would be pretty annoying.

I don't remember any coding style rule about this (in fact 
Documentation/CodingStyle has examples of case statements without such a 
comment), I think it's just personal preference so I'll leave it to Andrew 
and what he prefers.

(And if he prefers the /* fall through */ then we should ask that it be 
added to checkpatch.pl since it warns about a million other things and not 
this.)

--
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, mempolicy: make mpol_to_str robust and always succeed

[Dave Jones]

On Tue, Sep 24, 2013 at 08:18:16PM -0700, David Rientjes wrote:
 > On Tue, 24 Sep 2013, Dave Jones wrote:
 > 
 > >  >  	case MPOL_BIND:
 > >  > -		/* Fall through */
 > >  >  	case MPOL_INTERLEAVE:
 > >  >  		nodes = pol->v.nodes;
 > >  >  		break;
 > > 
 > > Any reason not to leave this ?
 > > 
 > > "missing break" is the 2nd most common thing that coverity picks up.
 > > Most of them are false positives like the above, but the lack of annotations
 > > in our source makes it time-consuming to pick through them all to find the
 > > real bugs.
 > > 
 > 
 > Check out things like drivers/mfd/wm5110-tables.c that do things like
 > 
 > 	switch (reg) {
 > 	case ARIZONA_SOFTWARE_RESET:
 > 	case ARIZONA_DEVICE_REVISION:
 > 	case ARIZONA_CTRL_IF_SPI_CFG_1:
 > 	case ARIZONA_CTRL_IF_I2C1_CFG_1:
 > 	case ARIZONA_CTRL_IF_I2C2_CFG_1:
 > 	case ARIZONA_CTRL_IF_I2C1_CFG_2:
 > 	case ARIZONA_CTRL_IF_I2C2_CFG_2:
 > 	...
 > 
 > and that file has over 1,000 case statements.  Having a

yikes, at first I thought that was output from a code generator.
 
 > 	/* fall through */
 > 
 > for all of them would be pretty annoying.
 
agreed, but with that example, it seems pretty obvious (to me at least)
that the lack of break's is intentional.  Where it gets trickier to
make quick judgment calls is cases like the one I mentioned above,
where there are only a few cases, and there's real code involved in
some but not all cases.

 > I don't remember any coding style rule about this (in fact 
 > Documentation/CodingStyle has examples of case statements without such a 
 > comment), I think it's just personal preference so I'll leave it to Andrew 
 > and what he prefers.
 > 
 > (And if he prefers the /* fall through */ then we should ask that it be 
 > added to checkpatch.pl since it warns about a million other things and not 
 > this.)

The question of course is how much gain there is in doing anything at all here.
So far, I've only seen false positives from that checker, but there are hundreds
of them to pick through, so who knows what's further down the pile.

	Dave

--
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, mempolicy: make mpol_to_str robust and always succeed

[David Rientjes]

On Tue, 24 Sep 2013, Dave Jones wrote:

>  > 	/* fall through */
>  > 
>  > for all of them would be pretty annoying.
>  
> agreed, but with that example, it seems pretty obvious (to me at least)
> that the lack of break's is intentional.  Where it gets trickier to
> make quick judgment calls is cases like the one I mentioned above,
> where there are only a few cases, and there's real code involved in
> some but not all cases.
> 

I fully agree and have code in the oom killer that has the "fall through" 
comment if there's code in between the case statements, but I think things 
like

	case MPOL_BIND:
	case MPOL_INTERLEAVE:
		...

is quite easy to read.  I don't feel strongly at all, though, so I'll just 
leave it to Andrew's preference.

--
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, mempolicy: make mpol_to_str robust and always succeed

[Andrew Morton]

On Wed, 25 Sep 2013 10:58:27 -0700 (PDT) David Rientjes <rientjes@google.com> wrote:

> On Tue, 24 Sep 2013, Dave Jones wrote:
> 
> >  > 	/* fall through */
> >  > 
> >  > for all of them would be pretty annoying.
> >  
> > agreed, but with that example, it seems pretty obvious (to me at least)
> > that the lack of break's is intentional.  Where it gets trickier to
> > make quick judgment calls is cases like the one I mentioned above,
> > where there are only a few cases, and there's real code involved in
> > some but not all cases.
> > 
> 
> I fully agree and have code in the oom killer that has the "fall through" 
> comment if there's code in between the case statements, but I think things 
> like
> 
> 	case MPOL_BIND:
> 	case MPOL_INTERLEAVE:
> 		...
> 
> is quite easy to read.  I don't feel strongly at all, though, so I'll just 
> leave it to Andrew's preference.

I've never even thought about it, but that won't prevent me from
pretending otherwise!  How about:

This:

	case WIBBLE:
		something();
		something_else();
	case WOBBLE:

needs a /* fall through */ comment (because it *looks* like a mistake),
whereas

	case WIBBLE:
	case WOBBLE:

does not?

--
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, mempolicy: make mpol_to_str robust and always succeed

[David Rientjes]

On Wed, 25 Sep 2013, Andrew Morton wrote:

> > I fully agree and have code in the oom killer that has the "fall through" 
> > comment if there's code in between the case statements, but I think things 
> > like
> > 
> > 	case MPOL_BIND:
> > 	case MPOL_INTERLEAVE:
> > 		...
> > 
> > is quite easy to read.  I don't feel strongly at all, though, so I'll just 
> > leave it to Andrew's preference.
> 
> I've never even thought about it, but that won't prevent me from
> pretending otherwise!  How about:
> 
> This:
> 
> 	case WIBBLE:
> 		something();
> 		something_else();
> 	case WOBBLE:
> 
> needs a /* fall through */ comment (because it *looks* like a mistake),
> whereas
> 
> 	case WIBBLE:
> 	case WOBBLE:
> 
> does not?
> 

The switch-case examples given in Documentation/CodingStyle agree with 
that.

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

[Cyrill Gorcunov]

On Wed, Aug 21, 2013 at 10:21:22AM +0800, Chen Gang wrote:
> 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.
> 
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> Cc: Cyrill Gorcunov <gorcunov@gmail.com>

Looks good to me, thanks!

Reviewed-by: Cyrill Gorcunov <gorcunov@gmail.com>

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

[Chen Gang]

On 08/21/2013 01:31 PM, Cyrill Gorcunov wrote:
> On Wed, Aug 21, 2013 at 10:21:22AM +0800, Chen Gang wrote:
>> 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.
>>
>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
>> Cc: Cyrill Gorcunov <gorcunov@gmail.com>
> 
> Looks good to me, thanks!
> 
> Reviewed-by: Cyrill Gorcunov <gorcunov@gmail.com>
> 

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


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