BPF Archive on lore.kernel.org
 help / color / Atom feed
From: Brendan Gregg <bgregg@netflix.com>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>,
	Wenbo Zhang <ethercflow@gmail.com>,
	bpf@vger.kernel.org, ast@kernel.org.com,
	Daniel Borkmann <daniel@iogearbox.net>,
	Yonghong Song <yhs@fb.com>,
	andrii.nakryiko@gmail.com, netdev@vger.kernel.org,
	linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH bpf-next v10 1/2] bpf: add new helper get_file_path for mapping a file descriptor to a pathname
Date: Fri, 13 Dec 2019 11:51:23 -0800
Message-ID: <CAJN39ogBTyXB2-3OHiJ-oQfBd75_axxvOLoABfXLopH_VsG6gw@mail.gmail.com> (raw)
In-Reply-To: <20191123060448.7crcqwkfmbq3gsze@ast-mbp.dhcp.thefacebook.com>

On Fri, Nov 22, 2019 at 10:05 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Sat, Nov 23, 2019 at 05:35:14AM +0000, Al Viro wrote:
> > On Fri, Nov 22, 2019 at 09:19:21PM -0800, Alexei Starovoitov wrote:
> >
> > > hard to tell. It will be run out of bpf prog that attaches to kprobe or
> > > tracepoint. What is the concern about locking?
> > > d_path() doesn't take any locks and doesn't depend on any locks. Above 'if'
> > > checks that plain d_path() is used and not some specilized callback with
> > > unknown logic.
> >
> > It sure as hell does.  It might end up taking rename_lock and/or mount_lock
> > spinlock components.  It'll try not to, but if the first pass ends up with
> > seqlock mismatch, it will just grab the spinlock the second time around.
>
> ohh. got it. I missed _or_lock() part in there.
> The need_seqretry() logic is tricky. afaics there is no way for the checks
> outside of prepend_path() to prevent spin_lock to happen. And adding a flag to
> prepend_path() to return early if retry is needed is too ugly. So this helper
> won't be safe to be run out of kprobe. But if we allow it for tracepoints only
> it should be ok. I think. There are no tracepoints in inner guts of vfs and I
> don't think they will ever be. So running in tracepoint->bpf_prog->d_path we
> will be sure that rename_lock+mount_lock can be safely spinlocked. Am I missing
> something?

It seems rather restrictive to only allow tracepoints (especially
without VFS tracepoints), although I'll use it to improve my syscall
tracepoint tools, so I'd be happy to see this merged even with that
restriction.

Just a thought: if *buffer is in BPF memory, can prepend_path() check
it's memory location and not try to grab the lock based on that? This
would be to avoid adding a flag.

>
> > > > with this number; quite possibly never before that function had been called
> > > > _and_ not once after it has returned.
> > >
> > > Right. TOCTOU is not a concern here. It's tracing. It's ok for full path to be
> > > 'one time deal'.
> >
> > It might very well be a full path of something completely unrelated to what
> > the syscall ends up operating upon.  It's not that the file might've been
> > moved; it might be a different file.  IOW, results of that tracing might be
> > misleading.
>
> That is correct. Tracing is fine with such limitation. Still better than probe_read.
>

+1

Tracing is observability tools and we document these caveats, and this
won't be the first time I've published tools where the printed path
may not be the one you think (e.g., the case of hard links.)

Brendan

-- 
Brendan Gregg, Senior Performance Architect, Netflix

  reply index

Thread overview: 48+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-19 13:27 [PATCH bpf-next v10 0/2] bpf: adding get_file_path helper Wenbo Zhang
2019-11-19 13:27 ` [PATCH bpf-next v10 1/2] bpf: add new helper get_file_path for mapping a file descriptor to a pathname Wenbo Zhang
2019-11-23  3:18   ` Alexei Starovoitov
2019-11-23  4:43     ` Al Viro
2019-11-23  4:51     ` Al Viro
2019-11-23  5:19       ` Alexei Starovoitov
2019-11-23  5:35         ` Al Viro
2019-11-23  6:04           ` Alexei Starovoitov
2019-12-13 19:51             ` Brendan Gregg [this message]
2019-12-05  4:20   ` [PATCH bpf-next v11 0/2] bpf: adding get_file_path helper Wenbo Zhang
2019-12-05  4:20     ` [PATCH bpf-next v11 1/2] bpf: add new helper get_file_path for mapping a file descriptor to a pathname Wenbo Zhang
2019-12-05  7:19       ` Alexei Starovoitov
2019-12-05  9:47         ` Wenbo Zhang
2019-12-15  4:01       ` [PATCH bpf-next v12 0/2] bpf: adding get_file_path helper Wenbo Zhang
2019-12-15  4:01         ` [PATCH bpf-next v12 1/2] bpf: add new helper get_file_path for mapping a file descriptor to a pathname Wenbo Zhang
2019-12-15 16:05           ` Yonghong Song
2019-12-17  6:26             ` Wenbo Zhang
2019-12-17  6:33               ` Yonghong Song
2019-12-15 16:10           ` Yonghong Song
2019-12-17  6:27             ` Wenbo Zhang
2019-12-16 22:09           ` Brendan Gregg
2019-12-17  4:05             ` Wenbo Zhang
2019-12-17  9:47           ` [PATCH bpf-next v13 0/2] bpf: adding get_fd_path helper Wenbo Zhang
2019-12-17  9:47             ` [PATCH bpf-next v13 1/2] bpf: add new helper get_fd_path for mapping a file descriptor to a pathname Wenbo Zhang
2019-12-17 16:29               ` Yonghong Song
2019-12-17 19:39                 ` Daniel Borkmann
2019-12-18  0:11                   ` Wenbo Zhang
2019-12-18  0:06                 ` Wenbo Zhang
2019-12-18  0:56               ` [PATCH bpf-next v14 0/2] bpf: adding get_fd_path helper Wenbo Zhang
2019-12-18  0:56                 ` [PATCH bpf-next v14 1/2] bpf: add new helper get_fd_path for mapping a file descriptor to a pathname Wenbo Zhang
2019-12-18  3:27                   ` Yonghong Song
2019-12-19 16:14                   ` Daniel Borkmann
2019-12-20  3:35                     ` Wenbo Zhang
2020-01-16  8:59                       ` Jiri Olsa
2020-02-10  4:43                         ` Brendan Gregg
2020-02-11  0:01                           ` Daniel Borkmann
2020-02-12 15:21                             ` Jiri Olsa
2019-12-18  0:56                 ` [PATCH bpf-next v14 2/2] selftests/bpf: test for bpf_get_fd_path() from tracepoint Wenbo Zhang
2019-12-18  3:27                   ` Yonghong Song
2019-12-17  9:47             ` [PATCH bpf-next v13 " Wenbo Zhang
2019-12-17 16:32               ` Yonghong Song
2019-12-15  4:01         ` [PATCH bpf-next v12 2/2] selftests/bpf: test for bpf_get_file_path() " Wenbo Zhang
2019-12-15 16:24           ` Yonghong Song
2019-12-17  4:01             ` Wenbo Zhang
2019-12-17  4:13               ` Yonghong Song
2019-12-17  9:44                 ` Wenbo Zhang
2019-12-05  4:20     ` [PATCH bpf-next v11 " Wenbo Zhang
2019-11-19 13:27 ` [PATCH bpf-next v10 " Wenbo Zhang

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=CAJN39ogBTyXB2-3OHiJ-oQfBd75_axxvOLoABfXLopH_VsG6gw@mail.gmail.com \
    --to=bgregg@netflix.com \
    --cc=alexei.starovoitov@gmail.com \
    --cc=andrii.nakryiko@gmail.com \
    --cc=ast@kernel.org.com \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=ethercflow@gmail.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=viro@zeniv.linux.org.uk \
    --cc=yhs@fb.com \
    /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

BPF Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/bpf/0 bpf/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 bpf bpf/ https://lore.kernel.org/bpf \
		bpf@vger.kernel.org
	public-inbox-index bpf

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.bpf


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git