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

[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

[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

[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

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?

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

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

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

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

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

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

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

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

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

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

[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

[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

[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

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>

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

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)

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

[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

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

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

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

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

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

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.

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?

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.