Re: [PATCH 09/17] file: Implement fnext_task

From: Linus Torvalds <torvalds@linux-foundation.org>
To: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: "Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	criu@openvz.org, bpf <bpf@vger.kernel.org>,
	"Alexander Viro" <viro@zeniv.linux.org.uk>,
	"Christian Brauner" <christian.brauner@ubuntu.com>,
	"Oleg Nesterov" <oleg@redhat.com>,
	"Cyrill Gorcunov" <gorcunov@gmail.com>,
	"Jann Horn" <jann@thejh.net>, "Kees Cook" <keescook@chromium.org>,
	"Daniel P. Berrangé" <berrange@redhat.com>,
	"Jeff Layton" <jlayton@redhat.com>,
	"Miklos Szeredi" <miklos@szeredi.hu>,
	"Matthew Wilcox" <willy@debian.org>,
	"J. Bruce Fields" <bfields@fieldses.org>,
	"Matthew Wilcox" <matthew@wil.cx>,
	"Trond Myklebust" <trond.myklebust@fys.uio.no>,
	"Chris Wright" <chrisw@redhat.com>,
	"Alexei Starovoitov" <ast@kernel.org>,
	"Daniel Borkmann" <daniel@iogearbox.net>,
	"Martin KaFai Lau" <kafai@fb.com>,
	"Song Liu" <songliubraving@fb.com>, "Yonghong Song" <yhs@fb.com>,
	"Andrii Nakryiko" <andriin@fb.com>,
	"John Fastabend" <john.fastabend@gmail.com>,
	"KP Singh" <kpsingh@chromium.org>
Subject: Re: [PATCH 09/17] file: Implement fnext_task
Date: Mon, 17 Aug 2020 16:54:34 -0700	[thread overview]
Message-ID: <CAHk-=whCU_psWXHod0-WqXXKB4gKzgW9q=d_ZEFPNATr3kG=QQ@mail.gmail.com> (raw)
In-Reply-To: <20200817220425.9389-9-ebiederm@xmission.com>

I like the series, but I have to say that the extension of the
horrible "fcheck*()" interfaces raises my hackles..

That interface is very very questionable to begin with, and it's easy
to get wrong.

I don't see you getting it wrong - you do seem to hold the RCU read
lock in the places I checked, but it worries me.

I think my worry could be at least partially mitigated with more
comments and docs:

On Mon, Aug 17, 2020 at 3:10 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
>
> +struct file *fnext_task(struct task_struct *task, unsigned int *ret_fd)

Please please *please* make it clear that because this does *not*
increment any reference counts, you have to be very very careful using
the "struct file" pointer this returns.

I really dislike the naming. The old "fcheck()" naming came from the
fact that at least one original user just mainly checked if the result
was NULL or not. And then others had to be careful to either hold the
file_lock spinlock, or at least the RCU read lock so that the result
didn't go away.

Here, you have "fnext_task()", and it doesn't even have that "check"
warning mark, or any comment about how dangerous it can be to use..

So the rule is that the "struct file" pointer this returns can only be
read under RCU, but the 'fd' it returns has a longer lifetime.

I think your uses are ok, and I think this is an improvement in
general, I just want a bigger "WARNING! Here be dragons!" sign (and
naming could be nicer).

            Linus