LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Miklos Szeredi <miklos@szeredi.hu>
To: Song Liu <songliubraving@fb.com>
Cc: LKML <linux-kernel@vger.kernel.org>,
	Kernel Team <Kernel-team@fb.com>,
	Steven Rostedt <rostedt@goodmis.org>,
	Ingo Molnar <mingo@redhat.com>,
	Howard McLauchlan <hmclauchlan@fb.com>,
	Josef Bacik <jbacik@fb.com>,
	Srikar Dronamraju <srikar@linux.vnet.ibm.com>
Subject: Re: [PATCH v2] tracing: fix bad use of igrab in trace_uprobe.c
Date: Fri, 20 Apr 2018 16:34:25 +0200	[thread overview]
Message-ID: <CAJfpegsjgK3-q00qX2A9eopDydKXxSBMwprL8XZH5w=g2BvBZQ@mail.gmail.com> (raw)
In-Reply-To: <F7C1033B-A155-4DA9-A686-AD39BA4CCC03@fb.com>

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

  reply	other threads:[~2018-04-20 14:34 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-18 17:40 Song Liu
2018-04-18 17:47 ` Steven Rostedt
2018-04-18 17:50   ` Song Liu
2018-04-19  8:58 ` Miklos Szeredi
2018-04-19 14:44   ` Miklos Szeredi
2018-04-19 16:37     ` Song Liu
2018-04-20 14:34       ` Miklos Szeredi [this message]
2018-04-20 15:24         ` Steven Rostedt

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAJfpegsjgK3-q00qX2A9eopDydKXxSBMwprL8XZH5w=g2BvBZQ@mail.gmail.com' \
    --to=miklos@szeredi.hu \
    --cc=Kernel-team@fb.com \
    --cc=hmclauchlan@fb.com \
    --cc=jbacik@fb.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=rostedt@goodmis.org \
    --cc=songliubraving@fb.com \
    --cc=srikar@linux.vnet.ibm.com \
    --subject='Re: [PATCH v2] tracing: fix bad use of igrab in trace_uprobe.c' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).