Skip to contentSkip to conversation

tracing: Make tracer_flags use the right set_flag callback

DISCUSSION Dan Carpenter 3 participants 4 messages Mar 10, 2016 – Mar 14, 2016 Kernel janitors

lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

Hello Chunyu Hu,

The patch d39cdd2036a6: "tracing: Make tracer_flags use the right set_flag callback" from Mar 8, 2016, leads to the following Smatch warning:

kernel/trace/trace.c:1303 register_tracer()
warn: inconsistent returns 'mutex:&trace_types_lock'.
Locked on: line 1260
Unlocked on: line 1232

               line 1237
               line 1303

kernel/trace/trace.c
1253
1254 if (!type->set_flag)
1255 type->set_flag = &dummy_set_flag;
1256 if (!type->flags) {

  1257                  /*allocate a dummy tracer_flags*/
  1258                  type->flags = kmalloc(sizeof(*type->flags), GFP_KERNEL);
  1259                  if (!type->flags)

1260 return -ENOMEM;

Should probably be a goto out.

  1261                  type->flags->val = 0;
  1262                  type->flags->opts = dummy_tracer_opt;
  1263          } else
  1264                  if (!type->flags->opts)
  1265                          type->flags->opts = dummy_tracer_opt;
  1266  
  1267          /* store the tracer for __set_tracer_option */
  1268          type->flags->trace = type;
  1269  
  1270          ret = run_tracer_selftest(type);
  1271          if (ret < 0)
  1272                  goto out;
  1273  
  1274          type->next = trace_types;
  1275          trace_types = type;
  1276          add_tracer_options(&global_trace, type);
  1277  
  1278   out:
  1279          tracing_selftest_running = false;
  1280          mutex_unlock(&trace_types_lock);
  1281  
  1282          if (ret || !default_bootup_tracer)
  1283                  goto out_unlock;

It's weird that goto out_unlock doesn't unlock.

1284

regards,
dan carpenter

#1 lore raw

----- Original Message -----

Quoted 31 lines

From: "Dan Carpenter" <[email protected]>
To: [email protected]
Cc: "Steven Rostedt" <[email protected]>, "Ingo Molnar" <[email protected]>, [email protected]
Sent: Friday, March 11, 2016 3:44:03 AM
Subject: re: tracing: Make tracer_flags use the right set_flag callback

Hello Chunyu Hu,

The patch d39cdd2036a6: "tracing: Make tracer_flags use the right set_flag callback" from Mar 8, 2016, leads to the following Smatch warning:

kernel/trace/trace.c:1303 register_tracer()
warn: inconsistent returns 'mutex:&trace_types_lock'.
Locked on: line 1260
Unlocked on: line 1232

               line 1237
               line 1303

kernel/trace/trace.c
1253
1254 if (!type->set_flag)
1255 type->set_flag = &dummy_set_flag;
1256 if (!type->flags) {
1257 /*allocate a dummy tracer_flags*/
1258 type->flags = kmalloc(sizeof(*type->flags),
GFP_KERNEL);
1259 if (!type->flags)
1260 return -ENOMEM;

Quoted 1 line

Should probably be a goto out.

Hi Dan,

Thanks so much for catching this. I missed this check. I was just thinking that this fail was nearly impossible, and during my tests I didn't hit any warn, so i missed this. You taught me a lesson. BTW, How did you hit the warn? a special gcc version / feature?

You are right, It should be a 'goto out', this has already tested by me.

                          if (!type->flags) {
                                  ret = -ENOMEM;
                                  goto out;
                          }

Hi Steve,
I'm so sorry for this. Can I resubmit a v2 for my part of the patch?

Quoted 24 lines
  1261                  type->flags->val = 0;
  1262                  type->flags->opts = dummy_tracer_opt;
  1263          } else
  1264                  if (!type->flags->opts)
  1265                          type->flags->opts = dummy_tracer_opt;
  1266
  1267          /* store the tracer for __set_tracer_option */
  1268          type->flags->trace = type;
  1269
  1270          ret = run_tracer_selftest(type);
  1271          if (ret < 0)
  1272                  goto out;
  1273
  1274          type->next = trace_types;
  1275          trace_types = type;
  1276          add_tracer_options(&global_trace, type);
  1277
  1278   out:
  1279          tracing_selftest_running = false;
  1280          mutex_unlock(&trace_types_lock);
  1281
  1282          if (ret || !default_bootup_tracer)
  1283                  goto out_unlock;
Quoted 1 line

It's weird that goto out_unlock doesn't unlock.

looks like exchange the name of out and out_unlock will make it more readable, but seems also needs a bit further cleanup, as the 'out' part will startup an bootup tracing if a kenrel parameter 'ftrace=one of the tracer' is defined in cmdline.

Quoted 5 lines

1284

regards,
dan carpenter

Signature · 2 lines
Regards,
Chunyu Hu
#2 lore raw
In reply to Dan Carpenter
Quoted 7 lines · On Fri, 11 Mar 2016 06:50:07 -0500 (EST) Chunyu Hu <[email protected]> wrote:

Thanks so much for catching this. I missed this check. I was just thinking that this fail was nearly impossible, and during my tests I didn't hit any warn, so i missed this. You taught me a lesson. BTW, How did you hit the warn? a special gcc version / feature?

You are right, It should be a 'goto out', this has already tested by me.

Quoted 8 lines
                          if (!type->flags) {
                                  ret = -ENOMEM;
                                  goto out;
                          }

Hi Steve,
I'm so sorry for this. Can I resubmit a v2 for my part of the patch?

What I push to linux-next doesn't get rebased. Please just send a patch to fix this on top of what's already in my for-next branch.

Thanks!

-- Steve