BPF Archive on lore.kernel.org
 help / color / Atom feed
From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: Al Viro <viro@zeniv.linux.org.uk>
Cc: Wenbo Zhang <ethercflow@gmail.com>,
	bpf@vger.kernel.org, ast@kernel.org.com, daniel@iogearbox.net,
	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, 22 Nov 2019 21:19:21 -0800
Message-ID: <20191123051919.dsw7v6jyad4j4ilc@ast-mbp.dhcp.thefacebook.com> (raw)
In-Reply-To: <20191123045151.GH26530@ZenIV.linux.org.uk>

On Sat, Nov 23, 2019 at 04:51:51AM +0000, Al Viro wrote:
> On Fri, Nov 22, 2019 at 07:18:28PM -0800, Alexei Starovoitov wrote:
> > > +	f = fget_raw(fd);
> > > +	if (!f)
> > > +		goto error;
> > > +
> > > +	/* For unmountable pseudo filesystem, it seems to have no meaning
> > > +	 * to get their fake paths as they don't have path, and to be no
> > > +	 * way to validate this function pointer can be always safe to call
> > > +	 * in the current context.
> > > +	 */
> > > +	if (f->f_path.dentry->d_op && f->f_path.dentry->d_op->d_dname)
> > > +		return -EINVAL;
> 
> An obvious leak here, BTW.

ohh. right.

> Depends.  Which context is it running in?  In particular, which
> locks might be already held?

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.

> Anyway, what could that be used for?  I mean, if you want to check
> something about syscall arguments, that's an unfixably racy way to go.
> Descriptor table can be a shared data structure, and two consequent
> fdget() on the same number can bloody well yield completely unrelated
> struct file references.

yes. It is racy. There are no guarantees on correctness of FD.
The program can pass arbitrary integer into this helper.

> IOW, anything that does descriptor -> struct file * translation more than
> once is an instant TOCTOU suspect.  In this particular case, the function
> will produce a pathname of something that was once reachable via descriptor
> 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'. Right now people use bpf_probe_read() to replicate what
d_path() does.
See https://github.com/iovisor/bcc/issues/237#issuecomment-547564661
It sort of works, but calling d_path() is simpler and more accurate.
The key thing that bpf helpers need to make sure is that regardless of
how they're called and what integer is passed in as an FD the helper
must not crash or lockup the kernel or cause it to misbehave.
Hence above in_interrupt() and other checks to limit the context.


  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 [this message]
2019-11-23  5:35         ` Al Viro
2019-11-23  6:04           ` Alexei Starovoitov
2019-12-13 19:51             ` Brendan Gregg
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=20191123051919.dsw7v6jyad4j4ilc@ast-mbp.dhcp.thefacebook.com \
    --to=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