On Sat, Apr 17, 2021 at 04:48:53PM +0000, Al Viro wrote:

On Sat, Apr 17, 2021 at 07:36:39AM -0700, Alexei Starovoitov wrote:

quoted

The kernel will perform the same work with FDs. The same locks are held
and the same execution conditions are in both cases. The LSM hooks,
fsnotify, etc will be called the same way.
It's no different if new syscall was introduced "sys_foo(int num)" that
would do { return close_fd(num); }.
It would opearate in the same user context.

Hmm...  unless I'm misreading the code, one of the call chains would seem to
be sys_bpf() -> bpf_prog_test_run() -> ->test_run() -> ... -> bpf_sys_close().
OK, as long as you make sure bpf_prog_get() does fdput() (i.e. that we
don't have it restructured so that fdget()/fdput() pair would be lifted into
bpf_prog_test_run(), with fdput() moved in place of bpf_prog_put()).

Got it. There is no fdget/put bracketing in the code.
On the way to test_run we do __bpf_prog_get() which does fdget and immediately
fdput after incrementing refcnt of the prog.
I believe this pattern is consistent everywhere in kernel/bpf/*

Note that we *really* can not allow close_fd() on anything to be bracketed
by fdget()/fdput() pair; we had bugs of that sort and, as the matter of fact,
still have one in autofs_dev_ioctl().

The trouble happens if you have file F with 2 references, held by descriptor
tables of different processes.  Say, process A has descriptor 6 refering to
it, while B has descriptor 42 doing the same.  Descriptor tables of A and B
are not shared with anyone.

A: fdget(6) 	-> returns a reference to F, refcount _not_ touched
A: close_fd(6)	-> rips the reference to F from descriptor table, does fput(F)
		   refcount drops to 1.
B: close(42)	-> rips the reference to F from B's descriptor table, does fput(F)
		   This time refcount does reach 0 and we use task_work_add() to
		   make sure the destructor (__fput()) runs before B returns to
		   userland.  sys_close() returns and B goes off to userland.
		   On the way out __fput() is run, and among other things,
		   ->release() of F is executed, doing whatever it wants to do.
		   F is freed.
And at that point A, which presumably is using the guts of F, gets screwed.

Thanks for these details. That's really helpful.

	So please, mark all call sites with "make very sure you never get
here with unpaired fdget()".

Good point. Will add this comment.

	BTW, if my reading (re ->test_run()) is correct, what limits the recursion
via bpf_sys_bpf()?

Glad you asked! This kind of code review questions are much appreciated.

It's an allowlist of possible commands in bpf_sys_bpf().
'case BPF_PROG_TEST_RUN:' is not there for this exact reason.
I'll add a comment to make it more obvious.