2019-11-27 17:52:59

by Jim Cromie

[permalink] [raw]
Subject: [PATCH 06/16] dyndbg: fix a BUG_ON in ddebug_describe_flags

ddebug_describe_flags currently fills a caller provided string buffer,
after testing its size (also passed) in a BUG_ON.

Fix this with a struct containing a known-big-enough string buffer,
and passing it instead.

Signed-off-by: Jim Cromie <[email protected]>
---
lib/dynamic_debug.c | 23 +++++++++++------------
1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index b5fb0aa0fbc3..0e4783e11755 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -62,6 +62,8 @@ struct ddebug_iter {
unsigned int idx;
};

+struct flagsbuf { char buf[12]; }; /* big enough to hold all the flags */
+
static DEFINE_MUTEX(ddebug_lock);
static LIST_HEAD(ddebug_tables);
static int verbose;
@@ -88,21 +90,19 @@ static struct { unsigned flag:8; char opt_char; } opt_array[] = {
};

/* format a string into buf[] which describes the _ddebug's flags */
-static char *ddebug_describe_flags(struct _ddebug *dp, char *buf,
- size_t maxlen)
+static char *ddebug_describe_flags(unsigned int flags, struct flagsbuf *fb)
{
- char *p = buf;
+ char *p = fb->buf;
int i;

- BUG_ON(maxlen < 6);
for (i = 0; i < ARRAY_SIZE(opt_array); ++i)
- if (dp->flags & opt_array[i].flag)
+ if (flags & opt_array[i].flag)
*p++ = opt_array[i].opt_char;
- if (p == buf)
+ if (p == fb->buf)
*p++ = '_';
*p = '\0';

- return buf;
+ return fb->buf;
}

#define vnpr_info(lvl, fmt, ...) \
@@ -148,7 +148,7 @@ static int ddebug_change(const struct ddebug_query *query,
struct ddebug_table *dt;
unsigned int newflags;
unsigned int nfound = 0;
- char flagbuf[10];
+ struct flagsbuf flags;

/* search for matching ddebugs */
mutex_lock(&ddebug_lock);
@@ -205,8 +205,7 @@ static int ddebug_change(const struct ddebug_query *query,
vpr_info("changed %s:%d [%s]%s =%s\n",
trim_prefix(dp->filename), dp->lineno,
dt->mod_name, dp->function,
- ddebug_describe_flags(dp, flagbuf,
- sizeof(flagbuf)));
+ ddebug_describe_flags(dp->flags, &flags));
}
}
mutex_unlock(&ddebug_lock);
@@ -820,7 +819,7 @@ static int ddebug_proc_show(struct seq_file *m, void *p)
{
struct ddebug_iter *iter = m->private;
struct _ddebug *dp = p;
- char flagsbuf[10];
+ struct flagsbuf flags;

v9pr_info("called m=%p p=%p\n", m, p);

@@ -833,7 +832,7 @@ static int ddebug_proc_show(struct seq_file *m, void *p)
seq_printf(m, "%s:%u [%s]%s =%s \"",
trim_prefix(dp->filename), dp->lineno,
iter->table->mod_name, dp->function,
- ddebug_describe_flags(dp, flagsbuf, sizeof(flagsbuf)));
+ ddebug_describe_flags(dp->flags, &flags));
seq_escape(m, dp->format, "\t\r\n\"");
seq_puts(m, "\"\n");

--
2.23.0


2019-12-02 17:41:58

by kernel test robot

[permalink] [raw]
Subject: Re: [PATCH 06/16] dyndbg: fix a BUG_ON in ddebug_describe_flags

Hi Jim,

I love your patch! Yet something to improve:

[auto build test ERROR on jeyu/modules-next]
[also build test ERROR on v5.4 next-20191202]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url: https://github.com/0day-ci/linux/commits/Jim-Cromie/dynamic-debug-cleanups-2-new-features/20191128-023731
base: https://git.kernel.org/pub/scm/linux/kernel/git/jeyu/linux.git modules-next
config: x86_64-rhel-7.6 (attached as .config)
compiler: gcc-7 (Debian 7.5.0-1) 7.5.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <[email protected]>

Note: the linux-review/Jim-Cromie/dynamic-debug-cleanups-2-new-features/20191128-023731 HEAD 89f783b27d95e352559b645885086a41e514b5eb builds fine.
It only hurts bisectibility.

All errors (new ones prefixed by >>):

lib/dynamic_debug.c: In function 'ddebug_change':
>> lib/dynamic_debug.c:151:18: error: 'flags' redeclared as different kind of symbol
struct flagsbuf flags;
^~~~~
lib/dynamic_debug.c:145:17: note: previous definition of 'flags' was here
unsigned int flags, unsigned int mask)
^~~~~
>> lib/dynamic_debug.c:194:34: error: invalid operands to binary | (have 'unsigned int' and 'struct flagsbuf')
newflags = (dp->flags & mask) | flags;
~~~~~~~~~~~~~~~~~~ ^
>> lib/dynamic_debug.c:199:17: error: invalid operands to binary & (have 'struct flagsbuf' and 'int')
if (!(flags & _DPRINTK_FLAGS_PRINT))
^
lib/dynamic_debug.c:201:21: error: invalid operands to binary & (have 'struct flagsbuf' and 'int')
} else if (flags & _DPRINTK_FLAGS_PRINT)
^

vim +/flags +151 lib/dynamic_debug.c

137
138 /*
139 * Search the tables for _ddebug's which match the given `query' and
140 * apply the `flags' and `mask' to them. Returns number of matching
141 * callsites, normally the same as number of changes. If verbose,
142 * logs the changes. Takes ddebug_lock.
143 */
144 static int ddebug_change(const struct ddebug_query *query,
145 unsigned int flags, unsigned int mask)
146 {
147 int i;
148 struct ddebug_table *dt;
149 unsigned int newflags;
150 unsigned int nfound = 0;
> 151 struct flagsbuf flags;
152
153 /* search for matching ddebugs */
154 mutex_lock(&ddebug_lock);
155 list_for_each_entry(dt, &ddebug_tables, link) {
156
157 /* match against the module name */
158 if (query->module &&
159 !match_wildcard(query->module, dt->mod_name))
160 continue;
161
162 for (i = 0; i < dt->num_ddebugs; i++) {
163 struct _ddebug *dp = &dt->ddebugs[i];
164
165 /* match against the source filename */
166 if (query->filename &&
167 !match_wildcard(query->filename, dp->filename) &&
168 !match_wildcard(query->filename,
169 kbasename(dp->filename)) &&
170 !match_wildcard(query->filename,
171 trim_prefix(dp->filename)))
172 continue;
173
174 /* match against the function */
175 if (query->function &&
176 !match_wildcard(query->function, dp->function))
177 continue;
178
179 /* match against the format */
180 if (query->format &&
181 !strstr(dp->format, query->format))
182 continue;
183
184 /* match against the line number range */
185 if (query->first_lineno &&
186 dp->lineno < query->first_lineno)
187 continue;
188 if (query->last_lineno &&
189 dp->lineno > query->last_lineno)
190 continue;
191
192 nfound++;
193
> 194 newflags = (dp->flags & mask) | flags;
195 if (newflags == dp->flags)
196 continue;
197 #ifdef CONFIG_JUMP_LABEL
198 if (dp->flags & _DPRINTK_FLAGS_PRINT) {
> 199 if (!(flags & _DPRINTK_FLAGS_PRINT))
200 static_branch_disable(&dp->key.dd_key_true);
201 } else if (flags & _DPRINTK_FLAGS_PRINT)
202 static_branch_enable(&dp->key.dd_key_true);
203 #endif
204 dp->flags = newflags;
205 vpr_info("changed %s:%d [%s]%s =%s\n",
206 trim_prefix(dp->filename), dp->lineno,
207 dt->mod_name, dp->function,
208 ddebug_describe_flags(dp->flags, &flags));
209 }
210 }
211 mutex_unlock(&ddebug_lock);
212
213 if (!nfound && verbose)
214 pr_info("no matches for query\n");
215
216 return nfound;
217 }
218

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/[email protected] Intel Corporation


Attachments:
(No filename) (4.96 kB)
.config.gz (47.18 kB)
Download all attachments