[PATCH v6 3/5] Use get_proc_pid for /proc paths

[Ákos Uzonyi]

On Fri, 7 Aug 2020 at 20:53, Dmitry V. Levin <ldv at altlinux.org> wrote:
> On Thu, Aug 06, 2020 at 09:02:09PM +0200, Ákos Uzonyi wrote:
> > * mmap_cache.c (mmap_cache_rebuild_if_invalid): Use proc pid instead of
> > tcp->pid for /proc path.
> > * util.c (getfdproto): Likewise.
> > (pidfd_get_pid): Likewise.
> > * pathtrace.c (getfdpath_pid): Likewise.
> > * strace.c (attach_tcb): Likewise.
> > ---
> >  mmap_cache.c | 2 +-
> >  pathtrace.c  | 5 ++++-
> >  strace.c     | 2 +-
> >  util.c       | 7 +++++--
> >  4 files changed, 11 insertions(+), 5 deletions(-)
> >
> > diff --git a/mmap_cache.c b/mmap_cache.c
> > index 89c62254..9825df26 100644
> > --- a/mmap_cache.c
> > +++ b/mmap_cache.c
> > @@ -84,7 +84,7 @@ mmap_cache_rebuild_if_invalid(struct tcb *tcp, const char *caller)
> >               return MMAP_CACHE_REBUILD_READY;
> >
> >       char filename[sizeof("/proc/4294967296/maps")];
> > -     xsprintf(filename, "/proc/%u/maps", tcp->pid);
> > +     xsprintf(filename, "/proc/%u/maps", get_proc_pid(tcp));
> >
> >       FILE *fp = fopen_stream(filename, "r");
> >       if (!fp) {
> > diff --git a/pathtrace.c b/pathtrace.c
> > index 5b60762b..74717a8a 100644
> > --- a/pathtrace.c
> > +++ b/pathtrace.c
> > @@ -87,7 +87,10 @@ getfdpath_pid(pid_t pid, int fd, char *buf, unsigned bufsize)
> >       if (fd < 0)
> >               return -1;
> >
> > -     xsprintf(linkpath, "/proc/%u/fd/%u", pid, fd);
> > +     int proc_pid = 0;
> > +     translate_pid(NULL, pid, PT_TID, &proc_pid);
> > +
> > +     xsprintf(linkpath, "/proc/%u/fd/%u", proc_pid, fd);
>
> This looks like an emulation of get_proc_pid(tcp) for the case
> when we have no tcp.
>
> Fortunately, we have a tcp argument, it's in function getfdpath.
> Since there are no users of getfdpath_pid that couldn't use getfdpath
> instead, I suggest removing getfdpath_pid and defining just getfdpath,
> this would make the code clearer.

getfdpath_pid was introduced by me recently. It's used in printfd_pid
(util.c), and it cannot be replaced by getfdpath. getfdpath_pid is
necessary for decoding a fd of another process (not the tracee), which
is the case in kcmp.

> >       n = readlink(linkpath, buf, bufsize - 1);
> >       /*
> >        * NB: if buf is too small, readlink doesn't fail,
> > diff --git a/util.c b/util.c
> > index 094e5818..d87d022e 100644
> > --- a/util.c
> > +++ b/util.c
> > @@ -582,8 +582,11 @@ printdev(struct tcb *tcp, int fd, const char *path)
> >  pid_t
> >  pidfd_get_pid(pid_t pid_of_fd, int fd)
> >  {
> > +     int proc_pid = 0;
> > +     translate_pid(NULL, pid_of_fd, PT_TID, &proc_pid);
> > +
> >       char fdi_path[sizeof("/proc/%u/fdinfo/%u") + 2 * sizeof(int) * 3];
> > -     xsprintf(fdi_path, "/proc/%u/fdinfo/%u", pid_of_fd, fd);
> > +     xsprintf(fdi_path, "/proc/%u/fdinfo/%u", proc_pid, fd);
> >
> >       FILE *f = fopen_stream(fdi_path, "r");
> >       if (!f)
>
> Likewise, this would invoke the whole translate_pid machinery even for the
> case when is_proc_ours() returns true.  Could something be done about it?
>
> The idea is not to bring all these heavy translation mechanisms up
> unnecessarily, that is, unless /proc is messed up or --pidns-translation
> is specified.

Actually, translation happens very fast in that case, as the following
condition will be true (in translate_pid):

/* If translation is trivial */
if (tip.from_ns == our_ns && (is_proc_ours() || !proc_pid_ptr)) {

So we could even remove the "if (is_proc_ours())" optimization from
get_proc_pid, as it is already done in translate_pid.