Messages in this thread | | | From | Miklos Szeredi <> | Date | Fri, 20 Apr 2018 16:34:25 +0200 | Subject | Re: [PATCH v2] tracing: fix bad use of igrab in trace_uprobe.c |
| |
On Thu, Apr 19, 2018 at 6:37 PM, Song Liu <songliubraving@fb.com> wrote: > > >> On Apr 19, 2018, at 7:44 AM, Miklos Szeredi <miklos@szeredi.hu> wrote: >> >> On Thu, Apr 19, 2018 at 10:58 AM, Miklos Szeredi <miklos@szeredi.hu> wrote: >>> On Wed, Apr 18, 2018 at 7:40 PM, Song Liu <songliubraving@fb.com> wrote:
>>>> *arg++ = '\0'; >>>> filename = argv[1]; >>>> ret = kern_path(filename, LOOKUP_FOLLOW, &path); >>>> if (ret) >>>> - goto fail_address_parse; >>>> - >>>> - inode = igrab(d_real_inode(path.dentry)); >> >> Also, where has the d_real_inode() gone? >> >> Looks like we need tu->inode back, since the return value of >> d_real_inode() may change over time. I'd do the "tu->inode = >> d_real_inode(tu->path.dentry)" just before first use (i.e. when >> enabling the tracepoint). >>
> > Do we need mechanism to prevent the return value of d_real_inode() > to change? Would the following sequence happen? > > create trace_uprobe > enable trace_uprobe (uprobe_register) > d_real changes > disable trace_uprobe (uprobe_unregister get wrong inode?)
Yes.
> > Another case might be: > > create trace_uprobe > enable trace_uprobe (uprobe_register) > disable trace_uprobe (uprobe_unregister) > d_real changes > enable trace_uprobe (do we need new inode for uprobe_register)
Probably a good idea to use the new one, but doesn't really matter. Do the one that's simpler.
This corner case is simply not interesting (modifying a binary while it is being debugged with uprobe). Let's just concentrate on making this crash and leak free.
Thanks, Miklos
| |