Re: [PATCH 1/3] ftrace: add function tracing to single thread

[Eric W. Biederman]

Ingo Molnar <mingo@xxxxxxx> writes:

> * Eric W. Biederman <ebiederm@xxxxxxxxxxxx> wrote:
>
>> Ingo Molnar <mingo@xxxxxxx> writes:
>> 
>> > i dont see the point of the complexity you are advocating. 99.9% of 
>> > the users run a unique PID space.
>> 
>> I'm not advocating complexity.  I'm advocating using the same APIs as
>> the rest of the kernel, for doing the same functions.
>> 
>> > Tracing is about keeping stuff simple. On containers we could also 
>> > trace the namespace ID (is there an easy ID for the namespace, as an 
>> > easy extension to the very nice PID concept that Unix introduced 
>> > decades ago?) and be done with it.
>> 
>> I don't really care about the pid namespace in this context.
>> 
>> I am just asking that we compare a different field in the task 
>> struct.
>> 
>> I am asking that we don't accumulate new users of an old crufty bug 
>> prone API, for no good reason.
>
> i dont disagree about the change, but i'm curious, what's bug-prone 
> about current->pid? It certainly worked quite well for the first 15 
> years.

Nothing especially serious.

- Just plain general sloppiness that you can get into with using numeric
  pids because you can get away with that you can't get away with if you
  are dealing with a real data structure.

  One of which is classic data type confusion.  Is a pid stored in an
  int a pid_t a short or something else.  Which leads to the difficult
  question if you need to change something how do you find and update
  all of the users.

  Other cases are the horrible to convert cases where we in practice
  leaked pids all over the place, got the locking totally all confused
  simply because we never noticed the races.  Code like that is a
  nightmare to convert to using struct pid *.  Even if struct pid pointers
  are faster to use.

- There is the interesting case that the original unix usage of pids
  and process and session groups with ttys, did not have any problems
  with pid wrap around.  But as pids became more common we added the
  SIGIO support which theoretically at least could allow send a signal
  to the wrong process due to pid wrap around.

  I'm not especially security paranoid but I suspect someone intent
  on such things could likely find a way to send a signal to a process they usually
  could not using pid wrap around.

- As for current->pid itself it is on my hit list for a different reason.

  It is one of the very few left overs from the old pid API, where we
  assume pids numbers are global. Being able to successfully remove it
  would greatly increase the confidence that we haven't missed
  something in the pid namespace implementation.

  The __pgrp and the __session fields in signal_struct are much higher
  on my hit list.  Darn I thought I had already removed them.  But unfortunately
  the final finishing touches on the pid namespace got stalled.

  Currently the preferred patterns are struct pid pointers internal to
  the kernel ( any place we are likely to save them ) and pid_t values
  right on the edge of user space. 

  current->pid is very handy for debugging.  Certainly global pid numbers
  aka pid numbers in the initial pid namespace are the pids we want
  to print with printk, and in any very light weight tracing.  As long
  as they don't creep into APIs where people turn around and use those
  those pids I don't have a problem with privileged users seeing them.


Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/