On Thu, Apr 19, 2018 at 6:37 PM, Song Liu <songliubrav...@fb.com> wrote: > > >> On Apr 19, 2018, at 7:44 AM, Miklos Szeredi <mik...@szeredi.hu> wrote: >> >> On Thu, Apr 19, 2018 at 10:58 AM, Miklos Szeredi <mik...@szeredi.hu> wrote: >>> On Wed, Apr 18, 2018 at 7:40 PM, Song Liu <songliubrav...@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
