On 7/22/21 3:59 PM, Al Viro wrote:

On Tue, Apr 20, 2021 at 12:03:33PM +0100, Pavel Begunkov wrote:

quoted

Just a bit of code tossing in io_sq_offload_create(), so it looks a bit
better. No functional changes.

Does a use-after-free count as a functional change?

quoted

 		f = fdget(p->wq_fd);

Descriptor table is shared with another thread, grabbed a reference to file.
Refcount is 2 (1 from descriptor table, 1 held by us)

quoted

 		if (!f.file)
 			return -ENXIO;

Nope, not NULL.

quoted

-		if (f.file->f_op != &io_uring_fops) {
-			fdput(f);
-			return -EINVAL;
-		}
 		fdput(f);

Decrement refcount, get preempted away.  f.file->f_count is 1 now.

Another thread: close() on the same descriptor.  Final reference to
struct file (from descriptor table) is gone, file closed, memory freed.

Regain CPU...

quoted

+		if (f.file->f_op != &io_uring_fops)
+			return -EINVAL;

... and dereference an already freed structure.

What scares me here is that you are playing with bloody fundamental
objects, without understanding even the basics regarding their
handling ;-/

Let's calm down here, no need to resort to hyperbole. It looks like an
honest mistake to me, and I should have caught that in review. You don't
even need to understand file structure life times to realize that:

	put(shared_struct);
	if (shared_struct->foo)
		...

is a bad idea. Which Pavel obviously does.

But yes, that is not great and obviously a bug, and we'll of course get
it fixed up asap.

-- 
Jens Axboe