On 7/22/21 3:59 PM, Al Viro wrote:
> On Tue, Apr 20, 2021 at 12:03:33PM +0100, Pavel Begunkov wrote:
>> 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?
>
>> 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)
>
>> if (!f.file)
>> return -ENXIO;
>
> Nope, not NULL.
>
>> - 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...
>
>> + 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
