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 <[email protected]>
---
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;
}
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
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.)
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
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.
On Wed, 25 Sep 2013 10:58:27 -0700 (PDT) David Rientjes <[email protected]> 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?
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.