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

[Dmitry V. Levin]

On Fri, Aug 07, 2020 at 09:12:05PM +0200, Ákos Uzonyi wrote:
> 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.

Indeed, I used to remember that, but it has eventually slipped my mind.
> 
> > >       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.

Sorry, I don't follow: translate_pid(NULL) calls both get_our_ns()
and is_proc_ours(), but I don't understand why it has to call
get_our_ns() if the result is essentially ignored anyway.


-- 
ldv